Re: CVS commit: src/sys/net

[Taylor R Campbell <riastradh%NetBSD.org@localhost>]

> Module Name:    src
> Committed By:   ozaki-r
> Date:           Wed Apr 16 05:29:45 UTC 2025
> 
> Modified Files:
>         src/sys/net: if_bridge.c
> 
> Log Message:
> bridge: avoid a race condition on stopping callout
> 
> Without BRIDGE_LOCK, the callout can be scheduled after callout_halt.

Oof!  Is there a PR?  This probably warrants pullup to 9 and 10 (and a
PR is helpful for tracking that).

But I think this is still racy, because bridge_init and bridge_stop,
which set and clear IFF_RUNNING in ifp->if_flags, do so without
BRIDGE_LOCK.  So we could see this sequence:

cpu0 (bridge_timer)		cpu1 (bridge_stop)
-------------------		------------------
observe IFF_RUNNING is set
				ifp->if_flags &= ~IFF_RUNNING
				callout_halt(&sc->sc_brcallout, NULL)
callout_reset(&sc->sc_callout, ...)

In general, access to ifp->if_flags is forbidden without holding
IFNET_LOCK (there is still a lot code that does it but it's buggy!).
And the workqueue can't take IFNET_LOCK because bridge_stop does
workqueue_wait, which could deadlock.

So I think we need a separate variable in struct bridge_softc to cache
the value of ifp->if_flags & IFF_RUNNING for use under BRIDGE_LOCK
instead of IFNET_LOCK -- or its complement, like we do in usbnet.c
(unp_txstopped/rxstopped, or unp_mcastactive) and other drivers like
if_vioif.c (netq_stopping).