Page MenuHomeFreeBSD

Add if_try_ref() to simplify refcount handling inside epoch
ClosedPublic

Authored by melifaro on Feb 21 2021, 2:00 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 13, 10:03 PM
Unknown Object (File)
Tue, Nov 12, 5:22 AM
Unknown Object (File)
Oct 11 2024, 3:21 AM
Unknown Object (File)
Sep 28 2024, 6:58 AM
Unknown Object (File)
Sep 25 2024, 6:46 AM
Unknown Object (File)
Sep 25 2024, 1:42 AM
Unknown Object (File)
Sep 24 2024, 8:11 PM
Unknown Object (File)
Sep 24 2024, 8:43 AM
Subscribers

Details

Summary

More and more code migrates from lock-based protection to the NET_EPOCH umbrella.
It requires some logic changes, including, notably, refcount handling.

When we have an ifp pointer and we're running inside epoch we're guaranteed that this pointer will not be freed.
However, the following case can still happen:

  • in thread 1 we drop to 0 refcount for ifp and schedule its deletion.
  • in thread 2 we use this ifp and reference it
  • destroy callout kicks in
  • unhappy user reports bug

This can happen with the current implementation of ifnet_byindex_ref(), as we're not holding any locks preventing ifnet deletion by a parallel thread.

To address it, if_try_ref() function is added, allowing to return failure when we try to reference ifp with 0 refcount.
Additionally, existing if_ref() is enforced with KASSERT to provide a cleaner error in such scenarios.

Finally, fix ifnet_byindex_ref() by using if_try_ref() and returning NULL if the latter fails.

This is pretty much the same as D28639.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

melifaro retitled this revision from Add if_try_ref() failing when current refcount=0. Fix ifnet_byindex_ref() by using if_try_ref() instead of if_ref(). to Add if_try_ref() to simplify refcount handling inside epoch.Feb 21 2021, 2:08 PM
melifaro edited the summary of this revision. (Show Details)
melifaro added a reviewer: network.
This revision was not accepted when it landed; it landed in state Needs Review.Feb 22 2021, 11:38 PM
This revision was automatically updated to reflect the committed changes.