On Fri, Nov 14, 2025 at 11:23:44AM +0300, Vitaliy Makkoveev wrote:
> On Thu, Nov 13, 2025 at 11:22:27PM +0100, Alexander Bluhm wrote:
> > On Thu, Nov 13, 2025 at 11:39:27PM +0300, Vitaliy Makkoveev wrote:
> > > On Thu, Nov 13, 2025 at 11:21:51PM +0300, Vitaliy Makkoveev wrote:
> > > > On Thu, Nov 13, 2025 at 11:07:28PM +0300, Vitaliy Makkoveev wrote:
> > > > > On Thu, Nov 13, 2025 at 11:05:46AM -0800, Mark McBride wrote:
> > > > > > I can trigger a kernel panic if I start the syncthing service.
> > > > > >
> > > > > > rcctl start syncthing_markmcb
> > > > > >
> > > > > > ... wait 5-10 seconds ...
> > > > > >
> > > > > > panic: netlock rwlock 0xffffff80012492d0: exit write when lock not
> > > > > > held (owner 0
> > > > > > x10, self 0xffffff809ee7ad24)
> > > > > > Stopped at db_enter+0x18: brk #0xf000
> > > > > > TID PID UID PRFLAGS PFLAGS CPU COMMAND
> > > > > > 87509 73883 1000 0x3 0 2 syncthing
> > > > > > 468519 73883 1000 0x3 0x4000000 3 syncthing
> > > > > > * 29160 73883 1000 0x3 0x4000000 1 syncthing
> > > > > > 3 22537 73883 1000 0x3 0x4000000 0 syncthing
> > > > > > db_enter() at panic+0x138
> > > > > > panic() at rw_exit_write+0xd0
> > > > > > _rw_init_flags() at cad_ioctl+0x80
> > > > > > cad_ioctl() at in_delmulti+0xbc
> > > > > > in_delmulti() at ip_freemoptions+0x3c
> > > > > > ip_freemoptions() at in_pcbdetach+0x5c
> > > > > > in_pcbdetach() at udp_detach+0x2c
> > > > > >
> > > > >
> > > > > We don't hold netlock wile performing SIOC{G,S}IFMEDIA ifioctl()
> > > > > commands, so add them to exception list too.
> > > > >
> > > >
> > > > Ops, they already are. Please drop this diff.
> > > >
> > >
> > > We hold shared netlock, so we trigger the assertion. This should not be
> > > the problem for cad(4), but this should be the problem for interface
> > > addresses list.
> >
> > This code in cad_ioctl() looks ugly.
> >
> > if (netlock_held)
> > NET_UNLOCK();
> > rw_enter_write(&sc->sc_cfg_lock);
> > if (netlock_held)
> > NET_LOCK();
> > s = splnet();
> >
> > Why does cad(4) fiddle with net lock at all?
> >
> > Drivers, not aware of MP, should rely on kernel lock. New drivers,
> > that announce themselves as MPSAFE, should implement their own
> > locking. I think no driver should ever touch net lock.
> >
> > bluhm
> >
>
> This is the known locks order problem between driver's lock and netlock.
Comment says net unlock is used due to barriers. Timeout barrier
cannot take net lock as it is not started with TIMEOUT_PROC. It
is not running on a thread. So barrier works even if net lock is
held by caller. At least I hope that.
Task barrier can be removed. cad_statchg_task() can use driver
lock instead, check IFF_RUNNING, and avoid barrier.
I do not have that device and cannot test. But I would guess, no
netlock in driver causes no problem anymore.
bluhm
Index: dev/fdt/if_cad.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/dev/fdt/if_cad.c,v
diff -u -p -r1.16 if_cad.c
--- dev/fdt/if_cad.c 17 Sep 2025 09:17:12 -0000 1.16
+++ dev/fdt/if_cad.c 14 Nov 2025 17:32:39 -0000
@@ -589,22 +589,10 @@ cad_ioctl(struct ifnet *ifp, u_long cmd,
{
struct cad_softc *sc = ifp->if_softc;
struct ifreq *ifr = (struct ifreq *)data;
- int error = 0, netlock_held = 1;
+ int error = 0;
int s;
- switch (cmd) {
- case SIOCGIFMEDIA:
- case SIOCSIFMEDIA:
- case SIOCGIFSFFPAGE:
- netlock_held = 0;
- break;
- }
-
- if (netlock_held)
- NET_UNLOCK();
rw_enter_write(&sc->sc_cfg_lock);
- if (netlock_held)
- NET_LOCK();
s = splnet();
switch (cmd) {
@@ -722,9 +710,6 @@ cad_up(struct cad_softc *sc)
rw_assert_wrlock(&sc->sc_cfg_lock);
- /* Release lock for memory allocation. */
- NET_UNLOCK();
-
if (sc->sc_dma64)
flags |= BUS_DMA_64BIT;
@@ -880,8 +865,6 @@ cad_up(struct cad_softc *sc)
}
}
- NET_LOCK();
-
/*
* Set MAC address filters.
*/
@@ -985,9 +968,6 @@ cad_down(struct cad_softc *sc)
ifq_clr_oactive(&ifp->if_snd);
ifp->if_timer = 0;
- /* Avoid lock order issues with barriers. */
- NET_UNLOCK();
-
timeout_del_barrier(&sc->sc_tick);
/* Disable data transfer. */
@@ -1011,7 +991,7 @@ cad_down(struct cad_softc *sc)
/* Wait for activity to cease. */
intr_barrier(sc->sc_ih);
ifq_barrier(&ifp->if_snd);
- taskq_del_barrier(systq, &sc->sc_statchg_task);
+ taskq_del(systq, &sc->sc_statchg_task);
/* Disable the packet clock as it is not needed any longer. */
clock_disable(sc->sc_node, GEM_CLK_TX);
@@ -1059,8 +1039,6 @@ cad_down(struct cad_softc *sc)
cad_dmamem_free(sc, sc->sc_rxring);
sc->sc_rxring = NULL;
sc->sc_rxdesc = NULL;
-
- NET_LOCK();
}
uint8_t
@@ -1683,8 +1661,14 @@ void
cad_statchg_task(void *arg)
{
struct cad_softc *sc = arg;
+ struct ifnet *ifp = &sc->sc_ac.ac_if;
- clock_set_frequency(sc->sc_node, GEM_CLK_TX, sc->sc_tx_freq);
+ rw_enter_write(&sc->sc_cfg_lock);
+
+ if ((ifp->if_flags & IFF_RUNNING))
+ clock_set_frequency(sc->sc_node, GEM_CLK_TX, sc->sc_tx_freq);
+
+ rw_exit_write(&sc->sc_cfg_lock);
}
struct cad_dmamem *
