Page MenuHomeFreeBSD

Add ifa_try_ref() to simplify ifa handling inside epoch.
ClosedPublic

Authored by melifaro on Sat, Feb 13, 12:21 AM.

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 ifa 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 ifa and schedule its deletion.
  • in thread 2 we use this ifa and reference it
  • destroy callout kicks in
  • unhappy user reports bug

To address it, ifa_try_ref() function is added, allowing to return failure when we try to reference ifa with 0 refcount.
Additionally, existing ifa_ref() is enforced with KASSERT to provide cleaner error in such scenarious.

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

melifaro added a reviewer: network.

Is the destroy callout guaranteed to run outside (or on the border) of epoch? Can this callout check the refcount and chase to abort itself?

Is the destroy callout guaranteed to run outside (or on the border) of epoch? Can this callout check the refcount and chase to abort itself?

Destroy callout runs outside the current epoch. Callout can check the refcount value and prevent _direct_ use-after-free.

There is a potential problem with this approach.
As ifa refcount==0 means that it has been deleted from the lists, we're adding "orphan" ifa, which, by the way, still has ifp pointer.
It may complicate ifp/ifa teardown procedure by allowing to endlessly refcount half-dead ifa pointer.
IMO it is always easier to think and operate with object lifecycle model which has a clear "dying" step which is a one-way door towards the destruction.

This revision is now accepted and ready to land.Tue, Feb 16, 7:55 PM