Page MenuHomeFreeBSD

ifnet: Traverse interface groups under the network epoch
AbandonedPublic

Authored by kbowling on Mon, Aug 31, 3:22 PM.
Tags
None
Referenced Files
F169225994: D59289.diff
Mon, Aug 31, 11:02 PM
F169225747: D59289.diff
Mon, Aug 31, 10:59 PM
F169188720: D59289.id185474.diff
Mon, Aug 31, 6:34 PM
F169185749: D59289.diff
Mon, Aug 31, 6:22 PM
F169181944: D59289.id.diff
Mon, Aug 31, 6:01 PM
F169177919: D59289.diff
Mon, Aug 31, 5:34 PM
Subscribers

Details

Reviewers
pouria
melifaro
glebius
markj
zlei
Group Reviewers
network
Summary
iflib holds IFNET_WLOCK while ether_ifattach() synchronously emits
the ifnet_attached_event.  The rtnetlink group dump calls
if_foreach_group(), which tried to acquire IFNET_RLOCK and panicked
on an exclusive-to-shared recursion.

The group lists use CK_STAILQ, and deletion already waits for
network epoch readers before freeing members.  Traverse the list in
the network epoch, as the link layer address walkers do, to avoid the
recursive lock while retaining lifetime protection.

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

I'm not sure if this is the right approach because if_detach_internal() is not the only consumer of if_delgroup. But since we're using ck_queue here, I think it should be fine.
I prefer to have other opinions on this change as well.

Thank you!

It is a problem of iflib. Drivers shall not use IFNET_WLOCK(). Ideally the lock should be private to net/if.c, in reality some other files in the network stack use it, but not drivers. iflib violates that, and it seems to be a pretty recent thing.

This revision now requires changes to proceed.Mon, Aug 31, 10:44 PM

Mkay. We were actually already 100% correct on this lock until in 2022 in e0e12405285b6 vmaffione@ abused it for the first time. That wasn't noticed, since that was specific to netmap and due to me not keeping eye on changes close back in 2022. And very recently you pushed some LLM generated code in fe00ca2f88718 that introduced more IFNET_WLOCK() into iflib.c. Most likely you are stepping on the latter.

Agreed, abandoning this approach

Please refactor the fe00ca2f88718 and I will see if anything can be done to e0e12405285b6. If successful with the latter, I will move the lock to if.c and that will prevent LLMs to produce this kind of problem again.

@glebius I am stress testing a fix for the fail point registration, I will tag you when I post it.