From nobody Thu Apr 14 19:23:11 2022 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 2236F1B57980; Thu, 14 Apr 2022 19:23:13 +0000 (UTC) (envelope-from mike@karels.net) Received: from mail.karels.net (mail.karels.net [216.160.39.52]) by mx1.freebsd.org (Postfix) with ESMTP id 4KfTrc0JCRz4hVL; Thu, 14 Apr 2022 19:23:11 +0000 (UTC) (envelope-from mike@karels.net) Received: from mail.karels.net (localhost [127.0.0.1]) by mail.karels.net (8.16.1/8.16.1) with ESMTP id 23EJNBCu013178; Thu, 14 Apr 2022 14:23:11 -0500 (CDT) (envelope-from mike@karels.net) Received: from [10.0.2.130] ([10.0.1.1]) by mail.karels.net with ESMTPSA id /RrWI590WGJ4MwAA4+wvSQ (envelope-from ); Thu, 14 Apr 2022 14:23:11 -0500 From: Mike Karels To: Gleb Smirnoff Cc: Mike Karels , 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 Date: Thu, 14 Apr 2022 14:23:11 -0500 X-Mailer: MailMate (1.14r5818) Message-ID: <68C5E41D-FB03-4E97-8417-0A44FF012771@karels.net> In-Reply-To: References: <202204111952.23BJqD1A028664@gitrepo.freebsd.org> List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: dev-commits-src-all@freebsd.org MIME-Version: 1.0 Content-Type: text/plain Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 4KfTrc0JCRz4hVL X-Spamd-Bar: / Authentication-Results: mx1.freebsd.org; dkim=none; dmarc=none; spf=pass (mx1.freebsd.org: domain of mike@karels.net designates 216.160.39.52 as permitted sender) smtp.mailfrom=mike@karels.net X-Spamd-Result: default: False [-0.39 / 15.00]; ARC_NA(0.00)[]; RCVD_VIA_SMTP_AUTH(0.00)[]; FREEFALL_USER(0.00)[mike]; FROM_HAS_DN(0.00)[]; TO_DN_SOME(0.00)[]; R_SPF_ALLOW(-0.20)[+ip4:216.160.39.52:c]; R_MISSING_CHARSET(2.50)[]; MIME_GOOD(-0.10)[text/plain]; TO_MATCH_ENVRCPT_ALL(0.00)[]; DMARC_NA(0.00)[karels.net]; RCPT_COUNT_FIVE(0.00)[5]; NEURAL_HAM_LONG(-1.00)[-1.000]; RCVD_COUNT_THREE(0.00)[3]; NEURAL_HAM_MEDIUM(-1.00)[-1.000]; NEURAL_HAM_SHORT(-0.69)[-0.694]; MLMMJ_DEST(0.00)[dev-commits-src-main,dev-commits-src-all]; RCVD_NO_TLS_LAST(0.10)[]; FROM_EQ_ENVFROM(0.00)[]; R_DKIM_NA(0.00)[]; MIME_TRACE(0.00)[0:+]; ASN(0.00)[asn:209, ipnet:216.160.36.0/22, country:US]; MID_RHS_MATCH_FROM(0.00)[] X-ThisMailContainsUnwantedMimeParts: N 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 > M> AuthorDate: 2022-04-08 12:37:15 +0000 > M> Commit: Mike Karels > 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