Date: Thu, 14 Apr 2022 14:23:11 -0500 From: Mike Karels <mike@karels.net> To: Gleb Smirnoff <glebius@freebsd.org> Cc: Mike Karels <karels@freebsd.org>, src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: 6ca0ca7b4cb5 - main - IPv4 multicast: fix LOR in shutdown path Message-ID: <68C5E41D-FB03-4E97-8417-0A44FF012771@karels.net> In-Reply-To: <YlSlxQvJiv3tZQLo@FreeBSD.org> References: <202204111952.23BJqD1A028664@gitrepo.freebsd.org> <YlSlxQvJiv3tZQLo@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 11 Apr 2022, at 17:03, Gleb Smirnoff wrote: > Hi Mike, > > On Mon, Apr 11, 2022 at 07:52:13PM +0000, Mike Karels wrote: > M> The branch main has been updated by karels: > M> > M> URL: https://cgit.FreeBSD.org/src/commit/?id=3D6ca0ca7b4cb527dc17c28= 9f1ae177ec267fd1add > M> > M> commit 6ca0ca7b4cb527dc17c289f1ae177ec267fd1add > M> Author: Mike Karels <karels@FreeBSD.org> > M> AuthorDate: 2022-04-08 12:37:15 +0000 > M> Commit: Mike Karels <karels@FreeBSD.org> > M> CommitDate: 2022-04-11 19:51:16 +0000 > M> > M> IPv4 multicast: fix LOR in shutdown path > M> > M> X_ip_mrouter_done() was calling the interface ioctl routines via= > M> if_allmulti() while holding a write lock. However, some interfa= ce > M> ioctl routines, including em/iflib and tap, use sxlocks, which a= re > M> not permitted while holding a non-sleepable lock, and this elici= ts > M> a warning from WITNESS. Fix the locking issue by recording the > M> affected interface pointers in a malloc'ed array, and call > M> if_allmulti() on each after dropping the rwlock. > > I'm sorry for not reviewing that in time. I think this change is a good= > fix but would great to bring more generality here. We already have > mechanism to handle exactly this problem, but with SIOCADDMULTI, see > if_addmulti(). > > This mechanism can't be used to handle SIOCSIFFLAGS as is. We need to > store the command and its argument somewhere, probably in the struct > ifnet itself. This has two complications: > - We won't be able to to queue multiple events on this task. I think th= is > is fine. > - This will require some locking. But if_amcount (and other similar fie= lds) > aren't properly locked now, so locking of interface flags and their c= ounts > is required anyway. Thanks for the suggestions. I looked at this a little, and see that it i= s not a simple project. I am not sure why the drivers need sxlocks for SIOCSIFFLAGS, but I suppose some drivers do processing that blocks. It would be nice if the driver did that asynchronously, but error handling would be an issue. The ioctls could be done with separate tasks for Promiscuous and allmulti, but that seems like a bit of a hack. I will think about it, and put it on my list. > -- = > Gleb Smirnoff
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?68C5E41D-FB03-4E97-8417-0A44FF012771>