> Date: Thu, 28 Jul 2022 13:30:12 +0200
> From: Alexander Bluhm <[email protected]>
>
> 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;
>
>
