> Date: Thu, 28 Jul 2022 13:30:12 +0200 > From: Alexander Bluhm <alexander.bl...@gmx.net> > > Hi, > > The netlock for SIOCSIFMEDIA and SIOCGIFMEDIA ioctl is not necessary. > Legacy drivers run with kernel lock, interface media is MP safe or > has kernel lock. > > ixl(4) talks about net lock but in fact has kernel lock. > > Problem is that smtpd(8) periodically checks media status. This > stops network if netlock was taken. > > ok?
ok kettenis@ (my reasoning for the ix(4) and ixl(4) below) > Index: dev/pci/if_ix.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/dev/pci/if_ix.c,v > retrieving revision 1.186 > diff -u -p -r1.186 if_ix.c > --- dev/pci/if_ix.c 27 Jun 2022 15:11:23 -0000 1.186 > +++ dev/pci/if_ix.c 27 Jul 2022 20:38:00 -0000 > @@ -1576,6 +1576,8 @@ ixgbe_update_link_status(struct ix_softc > struct ifnet *ifp = &sc->arpcom.ac_if; > int link_state = LINK_STATE_DOWN; > > + KERNEL_ASSERT_LOCKED(); > + > ixgbe_check_link(&sc->hw, &sc->link_speed, &sc->link_up, 0); > > ifp->if_baudrate = 0; The driver clearly assumes that the kernel lock is used as an interlock between interrupt context and normal context. Could add an splassert() here as well, since that is obviously a requirement. > Index: dev/pci/if_ixl.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/dev/pci/if_ixl.c,v > retrieving revision 1.83 > diff -u -p -r1.83 if_ixl.c > --- dev/pci/if_ixl.c 11 Mar 2022 18:00:45 -0000 1.83 > +++ dev/pci/if_ixl.c 27 Jul 2022 20:38:00 -0000 > @@ -2069,7 +2069,7 @@ ixl_media_status(struct ifnet *ifp, stru > { > struct ixl_softc *sc = ifp->if_softc; > > - NET_ASSERT_LOCKED(); > + KERNEL_ASSERT_LOCKED(); > > ifm->ifm_status = sc->sc_media_status; > ifm->ifm_active = sc->sc_media_active; > @@ -3517,7 +3517,9 @@ ixl_link_state_update_iaq(struct ixl_sof > return; > } > > + KERNEL_LOCK(); > link_state = ixl_set_link_status(sc, iaq); > + KERNEL_UNLOCK(); > mtx_enter(&sc->sc_link_state_mtx); > if (ifp->if_link_state != link_state) { > ifp->if_link_state = link_state; > @@ -4488,6 +4490,8 @@ ixl_set_link_status(struct ixl_softc *sc > const struct ixl_aq_link_status *status; > const struct ixl_phy_type *itype; > > + KERNEL_ASSERT_LOCKED(); > + > uint64_t ifm_active = IFM_ETHER; > uint64_t ifm_status = IFM_AVALID; > int link_state = LINK_STATE_DOWN; > @@ -4513,7 +4517,6 @@ ixl_set_link_status(struct ixl_softc *sc > baudrate = ixl_search_link_speed(status->link_speed); > > done: > - /* NET_ASSERT_LOCKED() except during attach */ > sc->sc_media_active = ifm_active; > sc->sc_media_status = ifm_status; > sc->sc_ac.ac_if.if_baudrate = baudrate; Right. This may be a bit heavy-handed, but it does mean ifm_actiave and ifm_status are consistent. > Index: net/if.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/net/if.c,v > retrieving revision 1.659 > diff -u -p -r1.659 if.c > --- net/if.c 14 Jul 2022 11:03:15 -0000 1.659 > +++ net/if.c 27 Jul 2022 21:49:04 -0000 > @@ -2259,6 +2259,15 @@ forceup: > error = ((*ifp->if_ioctl)(ifp, cmd, data)); > break; > > + case SIOCSIFMEDIA: > + if ((error = suser(p)) != 0) > + break; > + /* FALLTHROUGH */ > + case SIOCGIFMEDIA: > + /* net lock is not needed */ > + error = ((*ifp->if_ioctl)(ifp, cmd, data)); > + break; > + > case SIOCSETKALIVE: > case SIOCDIFPHYADDR: > case SIOCSLIFPHYADDR: > @@ -2268,7 +2277,6 @@ forceup: > case SIOCSLIFPHYECN: > case SIOCADDMULTI: > case SIOCDELMULTI: > - case SIOCSIFMEDIA: > case SIOCSVNETID: > case SIOCDVNETID: > case SIOCSVNETFLOWID: > Index: net/if_media.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/net/if_media.c,v > retrieving revision 1.36 > diff -u -p -r1.36 if_media.c > --- net/if_media.c 14 Jul 2022 13:46:25 -0000 1.36 > +++ net/if_media.c 27 Jul 2022 21:49:14 -0000 > @@ -312,7 +312,7 @@ ifmedia_ioctl(struct ifnet *ifp, struct > /* > * Get list of available media and current media on interface. > */ > - case SIOCGIFMEDIA: > + case SIOCGIFMEDIA: > { > struct ifmediareq *ifmr = (struct ifmediareq *) ifr; > size_t nwords; > >