Page MenuHomeFreeBSD

Need to wait for epoch callbacks to complete before detaching network interface
ClosedPublic

Authored by hselasky on Apr 30 2019, 12:16 PM.

Details

Summary

Need to wait for epoch callbacks to complete before detaching network the interface. This particularly manifests itself when an INP has multicast options attached during a network interface detach. Then the IPv4 and IPv6 leave group call which results from freeing the multicast address, may access a freed ifnet structure.

MFC after: 1 week
Sponsored by: Mellanox Technologies

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 24005

Event Timeline

hselasky created this revision.Apr 30 2019, 12:16 PM

@mmacy : Can you implement a function in the EPOCH API to drain all pending EPOCH callbacks? And take over this patch and commit a proper fix?

ae added a comment.EditedApr 30 2019, 1:34 PM

The epoch_call_drain() function is indeed needed (at least to fix such panic https://reviews.freebsd.org/F4491011).
But your example shows that epoch based reclamation is just wrongly used. The right solution should be keeping ifnet detached until all possible consumers stop reference it, and only then it will be safe to free ifnet pointer.

hselasky updated this revision to Diff 56998.May 3 2019, 12:11 PM

Properly implement needed epoch support.

In D20109#432627, @ae wrote:

The epoch_call_drain() function is indeed needed (at least to fix such panic https://reviews.freebsd.org/F4491011).
But your example shows that epoch based reclamation is just wrongly used. The right solution should be keeping ifnet detached until all possible consumers stop reference it, and only then it will be safe to free ifnet pointer.

Using EPOCH to do this is faster than touching an atomic and global refcount on the ifnet structure.

mmacy added a comment.May 3 2019, 10:20 PM

I feel like there's a better way to do this, but haven't come up with a solution yet.

mmacy added a comment.May 3 2019, 10:23 PM

@hselasky this is completely contrary to the intent of epoch - what callbacks are you seeing executed after a destroy?

@mmacy: The epoch_call() is not covered by epoch_wait_preempt(). We simply see deferred epoch calls being executed after the the IFP has been destroyed. Typically the multicast destroy ones.

mmacy added a comment.May 6 2019, 6:49 PM

@mmacy: The epoch_call() is not covered by epoch_wait_preempt(). We simply see deferred epoch calls being executed after the the IFP has been destroyed. Typically the multicast destroy ones.

You need to bump the refcount before the epoch_call. That's how this is handled elsewhere.

@mmacy : The e_drain_count is incremented in the previous for loop. Which refcount are you referring to?

I also see that as contrary to intent of epoch.

How exactly we dereference the ifnet being freed leading to panic? Is there some sort of asynchronous destruction callback in multicast code?

@glebius: Multicast destruction is deferred. When we destroy a multicast address we need to call the if_ioctl of the belonging network interface to remove any multicast addresses. That's the problem. I think draining is a good way to implement a safe solution instead of using refcounts. Then we ensure that the ifnet is in a certain state when the multicast destruction callbacks are invoked.

jtl added a subscriber: jtl.May 24 2019, 2:30 PM

@glebius: Multicast destruction is deferred. When we destroy a multicast address we need to call the if_ioctl of the belonging network interface to remove any multicast addresses. That's the problem. I think draining is a good way to implement a safe solution instead of using refcounts. Then we ensure that the ifnet is in a certain state when the multicast destruction callbacks are invoked.

Personally, I would prefer the refcount approach outlined by @mmacy. Can you explain why that is insufficient? Is multicast address destruction something that happens commonly enough that it is so highly performance sensitive that the atomics are a performance bottleneck?

@jtl: The refcount approach is not insufficient. We can do both ways, only that the synchronous approach is more safe-playing in my opinion and it is not that expensive and doesn't need additional checking in the multicast deferred destruction code-path for NULL pointers and destroyed resources, which the refcount approach will require.

@jtl: Imagine the following scenario.

Network drivers typically free their if_softc after if_free(). But deferred multicast epoch callbacks may still be running inside the driver if_ioctl() handler.

I don't see any other way than to make a synchronous barrier, because network drivers softc's are not refcounted like the ifnet softc and that is out of our control.

Refcounting everything is currently impractical like mandated by EPOCH, so a synchronous barrier may be a the best way forward here!

hselasky updated this revision to Diff 59150.Jun 28 2019, 9:43 AM

Update manual page to describe new epoch(9) APIs.

hselasky updated this revision to Diff 59151.Jun 28 2019, 10:12 AM

Optimize epoch_drain_cb() by using an atomic operation.

This revision was not accepted when it landed; it landed in state Needs Review.Jun 28 2019, 10:39 AM
This revision was automatically updated to reflect the committed changes.