> 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).
