X_ip_mrouter_done() was calling the interface ioctl routines via
if_allmulti() while holding a write lock. However, some interface
ioctl routines, including em/iflib and tap, use sxlocks, which are
not permitted while holding a non-sleepable lock, and this elicits
a warning from WITNESS. Fix the locking issue by recording the
affected interface pointers in a malloc'ed array, and call
if_allmulti() on each after dropping the rwlock.
Details
Details
- Reviewers
glebius wma bz - Commits
- rG6ca0ca7b4cb5: IPv4 multicast: fix LOR in shutdown path
restart mrouted and check for errors
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
There is a potential race here, if a multicast router socket is opened while this is still closing, as the if_allmulti() calls are done with no lock. I don't see a good solution, and that seems highly unlikely.
Comment Actions
I understand the tiny race; I am pondering if it might be worth mentioning it in the comment?
The other question is -- why are they using sxlocks (in these code paths)? But that might be a lot more complicated to answer than this fix/workaround.
So in general I am okay with this.
| sys/netinet/ip_mroute.c | ||
|---|---|---|
| 751 | I'd only initialize this before first need to along with the malloc (given there is no return path in between that anymore). | |
Comment Actions
I'll mention the race in the comment.
I haven't looked at the code using the sx locks, I just know that I ran into two of them on my test system. I don't know how many more there might be. I was also wondering why they use sx locks; probably because they are sleepable.
| sys/netinet/ip_mroute.c | ||
|---|---|---|
| 751 | OK. Do you prefer putting in near the malloc, or the loop that increments it? | |
Comment Actions
I looked again at the race. I don't think there is one. As ALLMULTI is reference-counted, it doesn't really matter if we drop it after another mrouter has added it.
Comment Actions
I moved the initialization of nifp into the loop construct. I'm also going to correct the comment at line 833 to say that we release our request for promiscuous multicast. If this sounds OK, I'll commit, otherwise push a new version for the review.