Re: git: 6ca0ca7b4cb5 - main - IPv4 multicast: fix LOR in shutdown path

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=6ca0ca7b4cb527dc17c289f1ae177ec267fd1add
> 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 interface
> M>     ioctl routines, including em/iflib and tap, use sxlocks, which are
> M>     not permitted while holding a non-sleepable lock, and this elicits
> 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 this
>   is fine.
> - This will require some locking. But if_amcount (and other similar fields)
>   aren't properly locked now, so locking of interface flags and their counts
>   is required anyway.

Thanks for the suggestions.  I looked at this a little, and see that it is
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