Page MenuHomeFreeBSD

ifnet: fix a race in if_detach_internal on clearing if_afdata
AbandonedPublic

Authored by takahiro.kurosawa_gmail.com on Jun 22 2024, 4:20 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Dec 9, 2:43 AM
Unknown Object (File)
Thu, Dec 5, 1:14 AM
Unknown Object (File)
Nov 6 2024, 10:51 PM
Unknown Object (File)
Oct 27 2024, 8:49 PM
Unknown Object (File)
Oct 18 2024, 1:41 PM
Unknown Object (File)
Oct 18 2024, 8:25 AM
Unknown Object (File)
Sep 13 2024, 4:28 AM
Unknown Object (File)
Sep 12 2024, 2:41 PM

Details

Reviewers
None
Group Reviewers
network
Summary

Functions that access if_afdata (like in6_selecthlim) are called during
packet transmission. ifnets may be detached while other threads call
these functions.
Add epoch_wait_preempt() and NET_EPOCH_DRAIN_CALLBACKS just before
clearing if_afdata since object references are guarded with net_epoch.

Test Plan

Run the following script posted in FreeBSD bugzilla 279653:
https://bugs.freebsd.org/bugzilla/attachment.cgi?id=251613

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 58329
Build 55217: arc lint + arc unit

Event Timeline

@glebius

I have mixed filling about this. I presume the above epoch_wait_preempt(net_epoch_preempt); combining with NET_EPOCH_DRAIN_CALLBACKS() should be sufficient but that is not true.

So how is this change sufficient then ?

And is it possible to defer the uninitialization of domain private data, aka if_afdata[], into if_free_deferred() ?

PS: I do not like the event ordering of ifnet_departure_event . The event handlers for ifnet_departure_event are invoked too late IMO. I guess it is more appropriate to invoke them right after if_unlink_ifnet() or so.

sys/net/if.c
1252

Use NET_EPOCH_WAIT() instead.

Replace epoch_wait_preempt() with NET_EPOCH_WAIT()

sys/net/if.c
1252

Thanks for looking at the problem in detail.
Updated according to the comment though further work would be needed for the complete fix.

And is it possible to defer the uninitialization of domain private data, aka if_afdata[], into if_free_deferred() ?

Yes, freeing afdata with epoch is the right way to do it. Not necessarily in if_free_deferred() though, cause it was not allocated by the ifnet layer. It is IPv6 that should do that.

In general a proper use of epoch(9) doesn't use epoch_drain_callbacks at all. This function was added later as a crutch to solve complicated scenarios. If you need to resort to using it, then definitely there is a room for improvement. IMHO.

I will retract this since it does not completely fix the problem and I have not come up with a good fix so far.