From 3722a8d173ca7515f472caf3051f94e36d367d53 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Fri, 24 Dec 2021 20:55:45 +0000 Subject: [PATCH 1/9] sys/net: New functions if_ioctl, if_init, and if_stop. These are wrappers, suitable for inserting appropriate kasserts regarding the API's locking contract, for the corresponding functions in struct ifnet. Since these are intended to commit configuration changes to the interface, which may involve resetting the device, the caller should hold IFNET_LOCK. However, I can't straightforwardly prove that all callers do yet, so the assertion is disabled for now. --- sys/net/if.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++--- sys/net/if.h | 4 ++++ 2 files changed, 62 insertions(+), 3 deletions(-) diff --git a/sys/net/if.c b/sys/net/if.c index 130ca14235ad..d3d1985b6a98 100644 --- a/sys/net/if.c +++ b/sys/net/if.c @@ -1622,7 +1622,7 @@ if_clone_destroy(const char *name) struct ifnet *ifp; struct psref psref; int error; - int (*if_ioctl)(struct ifnet *, u_long, void *); + int (*if_ioctlfn)(struct ifnet *, u_long, void *); KASSERT(mutex_owned(&if_clone_mtx)); @@ -1639,7 +1639,7 @@ if_clone_destroy(const char *name) /* We have to disable ioctls here */ IFNET_LOCK(ifp); - if_ioctl = ifp->if_ioctl; + if_ioctlfn = ifp->if_ioctl; ifp->if_ioctl = if_nullioctl; IFNET_UNLOCK(ifp); @@ -1654,7 +1654,7 @@ if_clone_destroy(const char *name) if (error != 0) { /* We have to restore if_ioctl on error */ IFNET_LOCK(ifp); - ifp->if_ioctl = if_ioctl; + ifp->if_ioctl = if_ioctlfn; IFNET_UNLOCK(ifp); } @@ -2727,6 +2727,61 @@ ifpromisc(struct ifnet *ifp, int pswitch) return e; } +/* + * if_ioctl(ifp, cmd, data) + * + * Apply an ioctl command to the interface. Returns 0 on success, + * nonzero errno(3) number on failure. + * + * May sleep. Caller must hold ifp->if_ioctl_lock. + */ +int +if_ioctl(struct ifnet *ifp, u_long cmd, void *data) +{ + + KASSERT(1 || IFNET_LOCKED(ifp)); /* XXX not yet */ + + return (*ifp->if_ioctl)(ifp, cmd, data); +} + +/* + * if_init(ifp) + * + * Prepare the hardware underlying ifp to process packets + * according to its current configuration. Returns 0 on success, + * nonzero errno(3) number on failure. + * + * May sleep. Caller must hold ifp->if_ioctl_lock, a.k.a + * IFNET_LOCK. + */ +int +if_init(struct ifnet *ifp) +{ + + KASSERT(1 || IFNET_LOCKED(ifp)); /* XXX not yet */ + + return (*ifp->if_init)(ifp); +} + +/* + * if_stop(ifp, disable) + * + * Stop the hardware underlying ifp from processing packets. + * + * If disable is true, ... XXX(?) + * + * May sleep. Caller must hold ifp->if_ioctl_lock, a.k.a + * IFNET_LOCK. + */ +void +if_stop(struct ifnet *ifp, int disable) +{ + + KASSERT(1 || IFNET_LOCKED(ifp)); /* XXX not yet */ + + (*ifp->if_stop)(ifp, disable); +} + /* * Map interface name to * interface structure pointer. diff --git a/sys/net/if.h b/sys/net/if.h index 00cfaf3aca23..722979c1c5b0 100644 --- a/sys/net/if.h +++ b/sys/net/if.h @@ -1143,6 +1143,10 @@ int if_mcast_op(ifnet_t *, const unsigned long, const struct sockaddr *); int if_flags_set(struct ifnet *, const u_short); int if_clone_list(int, char *, int *); +int if_ioctl(struct ifnet *, u_long, void *); +int if_init(struct ifnet *); +void if_stop(struct ifnet *, int); + struct ifnet *ifunit(const char *); struct ifnet *if_get(const char *, struct psref *); ifnet_t *if_byindex(u_int); From cb9171e8437b382dc19e8171ba0e89c9657717c6 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Fri, 24 Dec 2021 21:07:05 +0000 Subject: [PATCH 2/9] sys: Use if_stop wrapper function. Exception: Not in kern_pmf.c, for the kind of silly reason that it avoids having kern_pmf.c refer to symbols defined only in net; this avoids a pain in the rump. --- sys/arch/arm/xscale/ixp425_if_npe.c | 2 +- sys/net/if_bridge.c | 2 +- sys/net/if_ethersubr.c | 2 +- sys/net/if_wg.c | 2 +- sys/net/lagg/if_lagg.c | 6 +++--- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/sys/arch/arm/xscale/ixp425_if_npe.c b/sys/arch/arm/xscale/ixp425_if_npe.c index 4533c483c1c3..98691efd0a55 100644 --- a/sys/arch/arm/xscale/ixp425_if_npe.c +++ b/sys/arch/arm/xscale/ixp425_if_npe.c @@ -1429,7 +1429,7 @@ npeioctl(struct ifnet *ifp, u_long cmd, void *data) * If interface is marked down and it is running, * then stop and disable it. */ - (*ifp->if_stop)(ifp, 1); + if_stop(ifp, 1); } else if ((ifp->if_flags & (IFF_UP |IFF_RUNNING)) == IFF_UP) { /* * If interface is marked up and it is stopped, then diff --git a/sys/net/if_bridge.c b/sys/net/if_bridge.c index 817469654f04..c11e57f94242 100644 --- a/sys/net/if_bridge.c +++ b/sys/net/if_bridge.c @@ -611,7 +611,7 @@ bridge_ioctl(struct ifnet *ifp, u_long cmd, void *data) * If interface is marked down and it is running, * then stop and disable it. */ - (*ifp->if_stop)(ifp, 1); + if_stop(ifp, 1); break; case IFF_UP: /* diff --git a/sys/net/if_ethersubr.c b/sys/net/if_ethersubr.c index 2848be7ce6d1..443214459f2b 100644 --- a/sys/net/if_ethersubr.c +++ b/sys/net/if_ethersubr.c @@ -1461,7 +1461,7 @@ ether_ioctl_reinit(struct ethercom *ec) * If interface is marked down and it is running, * then stop and disable it. */ - (*ifp->if_stop)(ifp, 1); + if_stop(ifp, 1); break; case IFF_UP: /* diff --git a/sys/net/if_wg.c b/sys/net/if_wg.c index 696ad075d05e..ee9963e33f9a 100644 --- a/sys/net/if_wg.c +++ b/sys/net/if_wg.c @@ -4666,7 +4666,7 @@ wg_ioctl(struct ifnet *ifp, u_long cmd, void *data) * If interface is marked down and it is running, * then stop and disable it. */ - (*ifp->if_stop)(ifp, 1); + if_stop(ifp, 1); break; case IFF_UP: /* diff --git a/sys/net/lagg/if_lagg.c b/sys/net/lagg/if_lagg.c index 326680c5a3ef..01e21defb574 100644 --- a/sys/net/lagg/if_lagg.c +++ b/sys/net/lagg/if_lagg.c @@ -721,7 +721,7 @@ lagg_ioctl(struct ifnet *ifp, u_long cmd, void *data) switch (ifp->if_flags & (IFF_UP | IFF_RUNNING)) { case IFF_RUNNING: - ifp->if_stop(ifp, 1); + if_stop(ifp, 1); break; case IFF_UP: case IFF_UP | IFF_RUNNING: @@ -2354,7 +2354,7 @@ lagg_port_setup(struct lagg_softc *sc, if (ISSET(ifp_port->if_flags, IFF_RUNNING) && ifp_port->if_init != NULL) { - ifp_port->if_stop(ifp_port, 0); + if_stop(ifp_port, 0); stopped = true; } @@ -2475,7 +2475,7 @@ lagg_port_teardown(struct lagg_softc *sc, struct lagg_port *lp, IFNET_LOCK(ifp_port); if (ISSET(ifp_port->if_flags, IFF_RUNNING) && ifp_port->if_init != NULL) { - ifp_port->if_stop(ifp_port, 0); + if_stop(ifp_port, 0); stopped = true; } From 5ffa4d26519d7fc5818cc881054fa6433875bf8c Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Fri, 24 Dec 2021 21:07:43 +0000 Subject: [PATCH 3/9] sys: Use if_ioctl wrapper function. --- sys/altq/altq_afmap.c | 2 +- sys/net/if.c | 10 +++++----- sys/net/if_bridge.c | 2 +- sys/net/lagg/if_lagg_lacp.c | 2 +- sys/netcan/can.c | 2 +- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/sys/altq/altq_afmap.c b/sys/altq/altq_afmap.c index ae04bddbc3a2..d210eae19468 100644 --- a/sys/altq/altq_afmap.c +++ b/sys/altq/altq_afmap.c @@ -372,7 +372,7 @@ afmioctl(dev_t dev, ioctlcmd_t cmd, void *addr, int flag, if (ifp == NULL || (ifp->if_flags & IFF_RUNNING) == 0) error = ENXIO; else - error = ifp->if_ioctl(ifp, cmd, addr); + error = if_ioctl(ifp, cmd, addr); return error; } diff --git a/sys/net/if.c b/sys/net/if.c index d3d1985b6a98..9fb661b39731 100644 --- a/sys/net/if.c +++ b/sys/net/if.c @@ -3482,7 +3482,7 @@ doifioctl(struct socket *so, u_long cmd, void *data, struct lwp *l) KERNEL_LOCK_UNLESS_IFP_MPSAFE(ifp); IFNET_LOCK(ifp); - error = (*ifp->if_ioctl)(ifp, cmd, data); + error = if_ioctl(ifp, cmd, data); if (error != ENOTTY) ; else if (so->so_proto == NULL) @@ -3781,8 +3781,8 @@ if_addr_init(ifnet_t *ifp, struct ifaddr *ifa, const bool src) if (ifp->if_initaddr != NULL) rc = (*ifp->if_initaddr)(ifp, ifa, src); else if (src || - (rc = (*ifp->if_ioctl)(ifp, SIOCSIFDSTADDR, ifa)) == ENOTTY) - rc = (*ifp->if_ioctl)(ifp, SIOCINITIFADDR, ifa); + (rc = if_ioctl(ifp, SIOCSIFDSTADDR, ifa)) == ENOTTY) + rc = if_ioctl(ifp, SIOCINITIFADDR, ifa); return rc; } @@ -3849,7 +3849,7 @@ if_flags_set(ifnet_t *ifp, const u_short flags) memset(&ifr, 0, sizeof(ifr)); ifr.ifr_flags = flags & ~IFF_CANTCHANGE; - rc = (*ifp->if_ioctl)(ifp, SIOCSIFFLAGS, &ifr); + rc = if_ioctl(ifp, SIOCSIFFLAGS, &ifr); if (rc != 0 && cantflags != 0) ifp->if_flags ^= cantflags; @@ -3870,7 +3870,7 @@ if_mcast_op(ifnet_t *ifp, const unsigned long cmd, const struct sockaddr *sa) * directly rather than via doifoictl() */ ifreq_setaddr(cmd, &ifr, sa); - rc = (*ifp->if_ioctl)(ifp, cmd, &ifr); + rc = if_ioctl(ifp, cmd, &ifr); return rc; } diff --git a/sys/net/if_bridge.c b/sys/net/if_bridge.c index c11e57f94242..4c8b655ea9c4 100644 --- a/sys/net/if_bridge.c +++ b/sys/net/if_bridge.c @@ -869,7 +869,7 @@ bridge_ioctl_add(struct bridge_softc *sc, void *arg) memset(&ifr, 0, sizeof(ifr)); ifr.ifr_mtu = sc->sc_if.if_mtu; IFNET_LOCK(ifs); - error = ifs->if_ioctl(ifs, SIOCSIFMTU, &ifr); + error = if_ioctl(ifs, SIOCSIFMTU, &ifr); IFNET_UNLOCK(ifs); if (error != 0) goto out; diff --git a/sys/net/lagg/if_lagg_lacp.c b/sys/net/lagg/if_lagg_lacp.c index dfc7a6635a4f..abec367c43f2 100644 --- a/sys/net/lagg/if_lagg_lacp.c +++ b/sys/net/lagg/if_lagg_lacp.c @@ -839,7 +839,7 @@ lacp_linkstate(struct lagg_proto_softc *xlsc, struct lagg_port *lp) memset(&ifmr, 0, sizeof(ifmr)); ifmr.ifm_count = 0; - error = ifp_port->if_ioctl(ifp_port, SIOCGIFMEDIA, (void *)&ifmr); + error = if_ioctl(ifp_port, SIOCGIFMEDIA, (void *)&ifmr); if (error == 0) { media = lacp_ifmedia2lacpmedia(ifmr.ifm_active); } else if (error != ENOTTY){ diff --git a/sys/netcan/can.c b/sys/netcan/can.c index 0a0c67c0704e..adf70a9495ed 100644 --- a/sys/netcan/can.c +++ b/sys/netcan/can.c @@ -187,7 +187,7 @@ can_control(struct socket *so, u_long cmd, void *data, struct ifnet *ifp) default: if (ifp->if_ioctl == 0) return (EOPNOTSUPP); - return ((*ifp->if_ioctl)(ifp, cmd, data)); + return (if_ioctl(ifp, cmd, data)); } return (0); } From 2ff4b1f4ca6bbe4f00ece9a111f9a79551a44c6b Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Fri, 24 Dec 2021 21:26:29 +0000 Subject: [PATCH 4/9] sys: Use if_init wrapper function. Exception: Not in kern_pmf.c, for the kind of silly reason that it avoids having kern_pmf.c refer to symbols defined only in net; this avoids a pain in the rump. --- sys/arch/arm/imx/if_enet.c | 2 +- sys/arch/arm/sociox/if_ave.c | 2 +- sys/arch/arm/sociox/if_scx.c | 2 +- sys/arch/arm/sunxi/sunxi_emac.c | 2 +- sys/arch/arm/xscale/ixp425_if_npe.c | 4 ++-- sys/dev/cadence/if_cemac.c | 2 +- sys/dev/ic/bcmgenet.c | 2 +- sys/dev/ic/dm9000.c | 2 +- sys/dev/ic/rtl8169.c | 2 +- sys/dev/pci/if_dge.c | 4 ++-- sys/dev/pci/if_iwn.c | 2 +- sys/dev/pci/if_kse.c | 2 +- sys/dev/pci/if_sip.c | 2 +- sys/dev/pci/if_wm.c | 2 +- sys/dev/pci/ixgbe/ixgbe.c | 4 ++-- sys/dev/usb/if_urtw.c | 6 +++--- sys/net/agr/if_agr.c | 2 +- sys/net/if_bridge.c | 2 +- sys/net/if_ethersubr.c | 10 +++++----- sys/net/if_ieee1394subr.c | 4 ++-- sys/net/if_wg.c | 4 ++-- sys/net/lagg/if_lagg.c | 12 ++++++------ sys/net/link_proto.c | 2 +- 23 files changed, 39 insertions(+), 39 deletions(-) diff --git a/sys/arch/arm/imx/if_enet.c b/sys/arch/arm/imx/if_enet.c index e2e93a03b1cb..a50e74006338 100644 --- a/sys/arch/arm/imx/if_enet.c +++ b/sys/arch/arm/imx/if_enet.c @@ -1027,7 +1027,7 @@ enet_ioctl(struct ifnet *ifp, u_long command, void *data) error = 0; switch (command) { case SIOCSIFCAP: - error = (*ifp->if_init)(ifp); + error = if_init(ifp); break; case SIOCADDMULTI: case SIOCDELMULTI: diff --git a/sys/arch/arm/sociox/if_ave.c b/sys/arch/arm/sociox/if_ave.c index afde8ca26ea4..b7f5305ec75b 100644 --- a/sys/arch/arm/sociox/if_ave.c +++ b/sys/arch/arm/sociox/if_ave.c @@ -790,7 +790,7 @@ ave_ioctl(struct ifnet *ifp, u_long cmd, void *data) error = 0; if (cmd == SIOCSIFCAP) - error = (*ifp->if_init)(ifp); + error = if_init(ifp); if (cmd != SIOCADDMULTI && cmd != SIOCDELMULTI) ; else if (ifp->if_flags & IFF_RUNNING) { diff --git a/sys/arch/arm/sociox/if_scx.c b/sys/arch/arm/sociox/if_scx.c index 49514a1455aa..6ddc33c62a17 100644 --- a/sys/arch/arm/sociox/if_scx.c +++ b/sys/arch/arm/sociox/if_scx.c @@ -1201,7 +1201,7 @@ scx_ioctl(struct ifnet *ifp, u_long cmd, void *data) break; error = 0; if (cmd == SIOCSIFCAP) - error = (*ifp->if_init)(ifp); + error = if_init(ifp); if (cmd != SIOCADDMULTI && cmd != SIOCDELMULTI) ; else if (ifp->if_flags & IFF_RUNNING) { diff --git a/sys/arch/arm/sunxi/sunxi_emac.c b/sys/arch/arm/sunxi/sunxi_emac.c index 60c01c989466..c4de05281e98 100644 --- a/sys/arch/arm/sunxi/sunxi_emac.c +++ b/sys/arch/arm/sunxi/sunxi_emac.c @@ -932,7 +932,7 @@ sunxi_emac_ioctl(struct ifnet *ifp, u_long cmd, void *data) error = 0; if (cmd == SIOCSIFCAP) - error = (*ifp->if_init)(ifp); + error = if_init(ifp); else if (cmd != SIOCADDMULTI && cmd != SIOCDELMULTI) ; else if ((ifp->if_flags & IFF_RUNNING) != 0) { diff --git a/sys/arch/arm/xscale/ixp425_if_npe.c b/sys/arch/arm/xscale/ixp425_if_npe.c index 98691efd0a55..49aabbb5b4af 100644 --- a/sys/arch/arm/xscale/ixp425_if_npe.c +++ b/sys/arch/arm/xscale/ixp425_if_npe.c @@ -1435,7 +1435,7 @@ npeioctl(struct ifnet *ifp, u_long cmd, void *data) * If interface is marked up and it is stopped, then * start it. */ - error = (*ifp->if_init)(ifp); + error = if_init(ifp); } else if ((ifp->if_flags & IFF_UP) != 0) { u_short diff; @@ -1457,7 +1457,7 @@ npeioctl(struct ifnet *ifp, u_long cmd, void *data) * any other flags that affect the hardware * state. */ - error = (*ifp->if_init)(ifp); + error = if_init(ifp); } } sc->sc_if_flags = ifp->if_flags; diff --git a/sys/dev/cadence/if_cemac.c b/sys/dev/cadence/if_cemac.c index fd5aaf36ad3b..ce9457c32e2d 100644 --- a/sys/dev/cadence/if_cemac.c +++ b/sys/dev/cadence/if_cemac.c @@ -759,7 +759,7 @@ cemac_ifioctl(struct ifnet *ifp, u_long cmd, void *data) error = 0; if (cmd == SIOCSIFCAP) { - error = (*ifp->if_init)(ifp); + error = if_init(ifp); } else if (cmd != SIOCADDMULTI && cmd != SIOCDELMULTI) ; else if (ifp->if_flags & IFF_RUNNING) { diff --git a/sys/dev/ic/bcmgenet.c b/sys/dev/ic/bcmgenet.c index 2040f1e12bc6..2043655959a7 100644 --- a/sys/dev/ic/bcmgenet.c +++ b/sys/dev/ic/bcmgenet.c @@ -916,7 +916,7 @@ genet_ioctl(struct ifnet *ifp, u_long cmd, void *data) error = 0; if (cmd == SIOCSIFCAP) - error = (*ifp->if_init)(ifp); + error = if_init(ifp); else if (cmd != SIOCADDMULTI && cmd != SIOCDELMULTI) ; else if ((ifp->if_flags & IFF_RUNNING) != 0) { diff --git a/sys/dev/ic/dm9000.c b/sys/dev/ic/dm9000.c index e3c68052c42f..93ead7296756 100644 --- a/sys/dev/ic/dm9000.c +++ b/sys/dev/ic/dm9000.c @@ -833,7 +833,7 @@ dme_ioctl(struct ifnet *ifp, u_long cmd, void *data) break; error = 0; if (cmd == SIOCSIFCAP) - error = (*ifp->if_init)(ifp); + error = if_init(ifp); else if (cmd != SIOCADDMULTI && cmd != SIOCDELMULTI) ; else if (ifp->if_flags && IFF_RUNNING) { diff --git a/sys/dev/ic/rtl8169.c b/sys/dev/ic/rtl8169.c index dd36ea119cbf..a71d27f8e72c 100644 --- a/sys/dev/ic/rtl8169.c +++ b/sys/dev/ic/rtl8169.c @@ -2130,7 +2130,7 @@ re_ioctl(struct ifnet *ifp, u_long command, void *data) error = 0; if (command == SIOCSIFCAP) - error = (*ifp->if_init)(ifp); + error = if_init(ifp); else if (command != SIOCADDMULTI && command != SIOCDELMULTI) ; else if (ifp->if_flags & IFF_RUNNING) diff --git a/sys/dev/pci/if_dge.c b/sys/dev/pci/if_dge.c index 565a16a70b01..ff4b04187384 100644 --- a/sys/dev/pci/if_dge.c +++ b/sys/dev/pci/if_dge.c @@ -1453,7 +1453,7 @@ dge_ioctl(struct ifnet *ifp, u_long cmd, void *data) else if ((error = ifioctl_common(ifp, cmd, data)) != ENETRESET) break; else if (ifp->if_flags & IFF_UP) - error = (*ifp->if_init)(ifp); + error = if_init(ifp); else error = 0; break; @@ -1488,7 +1488,7 @@ dge_ioctl(struct ifnet *ifp, u_long cmd, void *data) error = 0; if (cmd == SIOCSIFCAP) - error = (*ifp->if_init)(ifp); + error = if_init(ifp); else if (cmd != SIOCADDMULTI && cmd != SIOCDELMULTI) ; else if (ifp->if_flags & IFF_RUNNING) { diff --git a/sys/dev/pci/if_iwn.c b/sys/dev/pci/if_iwn.c index 0c63cef8c1dd..5fad5e8a915f 100644 --- a/sys/dev/pci/if_iwn.c +++ b/sys/dev/pci/if_iwn.c @@ -973,7 +973,7 @@ iwn_power(int why, void *arg) s = splnet(); ifp = &sc->sc_ic.ic_if; if (ifp->if_flags & IFF_UP) { - ifp->if_init(ifp); + if_init(ifp); if (ifp->if_flags & IFF_RUNNING) ifp->if_start(ifp); } diff --git a/sys/dev/pci/if_kse.c b/sys/dev/pci/if_kse.c index f3f5c2ce1021..26dd7d0bf5b4 100644 --- a/sys/dev/pci/if_kse.c +++ b/sys/dev/pci/if_kse.c @@ -689,7 +689,7 @@ kse_ioctl(struct ifnet *ifp, u_long cmd, void *data) break; error = 0; if (cmd == SIOCSIFCAP) - error = (*ifp->if_init)(ifp); + error = if_init(ifp); if (cmd != SIOCADDMULTI && cmd != SIOCDELMULTI) ; else if (ifp->if_flags & IFF_RUNNING) { diff --git a/sys/dev/pci/if_sip.c b/sys/dev/pci/if_sip.c index bc340603ffc4..cd345843495d 100644 --- a/sys/dev/pci/if_sip.c +++ b/sys/dev/pci/if_sip.c @@ -1869,7 +1869,7 @@ sipcom_ioctl(struct ifnet *ifp, u_long cmd, void *data) error = 0; if (cmd == SIOCSIFCAP) - error = (*ifp->if_init)(ifp); + error = if_init(ifp); else if (cmd != SIOCADDMULTI && cmd != SIOCDELMULTI) ; else if (ifp->if_flags & IFF_RUNNING) { diff --git a/sys/dev/pci/if_wm.c b/sys/dev/pci/if_wm.c index 1c2a6a9f2b56..951e3b746bc1 100644 --- a/sys/dev/pci/if_wm.c +++ b/sys/dev/pci/if_wm.c @@ -3675,7 +3675,7 @@ wm_ioctl(struct ifnet *ifp, u_long cmd, void *data) error = 0; if (cmd == SIOCSIFCAP) - error = (*ifp->if_init)(ifp); + error = if_init(ifp); else if (cmd != SIOCADDMULTI && cmd != SIOCDELMULTI) ; else if (ifp->if_flags & IFF_RUNNING) { diff --git a/sys/dev/pci/ixgbe/ixgbe.c b/sys/dev/pci/ixgbe/ixgbe.c index 1bdc1a8db3b4..7c9ff7c1f76f 100644 --- a/sys/dev/pci/ixgbe/ixgbe.c +++ b/sys/dev/pci/ixgbe/ixgbe.c @@ -5770,7 +5770,7 @@ ixgbe_sysctl_dmac(SYSCTLFN_ARGS) /* Re-initialize hardware if it's already running */ if (ifp->if_flags & IFF_RUNNING) - ifp->if_init(ifp); + if_init(ifp); return (0); } @@ -6103,7 +6103,7 @@ ixgbe_sysctl_eee_state(SYSCTLFN_ARGS) } /* Restart auto-neg */ - ifp->if_init(ifp); + if_init(ifp); device_printf(dev, "New EEE state: %d\n", new_eee); diff --git a/sys/dev/usb/if_urtw.c b/sys/dev/usb/if_urtw.c index d0a55c951839..e451bbb48d82 100644 --- a/sys/dev/usb/if_urtw.c +++ b/sys/dev/usb/if_urtw.c @@ -1045,7 +1045,7 @@ urtw_media_change(struct ifnet *ifp) if ((ifp->if_flags & (IFF_UP | IFF_RUNNING)) == (IFF_UP | IFF_RUNNING)) - ifp->if_init(ifp); + if_init(ifp); return 0; } @@ -2395,7 +2395,7 @@ urtw_ioctl(struct ifnet *ifp, u_long cmd, void *data) case IFF_UP|IFF_RUNNING: break; case IFF_UP: - ifp->if_init(ifp); + if_init(ifp); break; case IFF_RUNNING: urtw_stop(ifp, 1); @@ -2419,7 +2419,7 @@ urtw_ioctl(struct ifnet *ifp, u_long cmd, void *data) if (error == ENETRESET) { if (IS_RUNNING(ifp) && (ic->ic_roaming != IEEE80211_ROAMING_MANUAL)) - ifp->if_init(ifp); + if_init(ifp); error = 0; } diff --git a/sys/net/agr/if_agr.c b/sys/net/agr/if_agr.c index 735d15f5b74b..75e6fd3e68de 100644 --- a/sys/net/agr/if_agr.c +++ b/sys/net/agr/if_agr.c @@ -759,7 +759,7 @@ agrport_cleanup(struct agr_softc *sc, struct agr_port *port) memcpy(LLADDR(ifp_port->if_sadl), port->port_origlladdr, ifp_port->if_addrlen); if (ifp_port->if_init != NULL) { - error = (*ifp_port->if_init)(ifp_port); + error = if_init(ifp_port); } #else union { diff --git a/sys/net/if_bridge.c b/sys/net/if_bridge.c index 4c8b655ea9c4..cbac8bcea4ad 100644 --- a/sys/net/if_bridge.c +++ b/sys/net/if_bridge.c @@ -618,7 +618,7 @@ bridge_ioctl(struct ifnet *ifp, u_long cmd, void *data) * If interface is marked up and it is stopped, then * start it. */ - error = (*ifp->if_init)(ifp); + error = if_init(ifp); break; default: break; diff --git a/sys/net/if_ethersubr.c b/sys/net/if_ethersubr.c index 443214459f2b..ae8d6621d72c 100644 --- a/sys/net/if_ethersubr.c +++ b/sys/net/if_ethersubr.c @@ -1468,7 +1468,7 @@ ether_ioctl_reinit(struct ethercom *ec) * If interface is marked up and it is stopped, then * start it. */ - return (*ifp->if_init)(ifp); + return if_init(ifp); case IFF_UP | IFF_RUNNING: error = 0; if (ec->ec_ifflags_cb != NULL) { @@ -1479,10 +1479,10 @@ ether_ioctl_reinit(struct ethercom *ec) * changes in any other flags that * affect the hardware state. */ - return (*ifp->if_init)(ifp); + return if_init(ifp); } } else - error = (*ifp->if_init)(ifp); + error = if_init(ifp); return error; case 0: break; @@ -1514,7 +1514,7 @@ ether_ioctl(struct ifnet *ifp, u_long cmd, void *data) && (ifp->if_flags & (IFF_UP | IFF_RUNNING)) != (IFF_UP | IFF_RUNNING)) { ifp->if_flags |= IFF_UP; - if ((error = (*ifp->if_init)(ifp)) != 0) + if ((error = if_init(ifp)) != 0) return error; } #ifdef INET @@ -1539,7 +1539,7 @@ ether_ioctl(struct ifnet *ifp, u_long cmd, void *data) return error; else if (ifp->if_flags & IFF_UP) { /* Make sure the device notices the MTU change. */ - return (*ifp->if_init)(ifp); + return if_init(ifp); } else return 0; } diff --git a/sys/net/if_ieee1394subr.c b/sys/net/if_ieee1394subr.c index 94f895c91d0b..ca4825f0171d 100644 --- a/sys/net/if_ieee1394subr.c +++ b/sys/net/if_ieee1394subr.c @@ -681,13 +681,13 @@ ieee1394_ioctl(struct ifnet *ifp, u_long cmd, void *data) switch (ifa->ifa_addr->sa_family) { #ifdef INET case AF_INET: - if ((error = (*ifp->if_init)(ifp)) != 0) + if ((error = if_init(ifp)) != 0) break; arp_ifinit(ifp, ifa); break; #endif /* INET */ default: - error = (*ifp->if_init)(ifp); + error = if_init(ifp); break; } break; diff --git a/sys/net/if_wg.c b/sys/net/if_wg.c index ee9963e33f9a..ea9fc2afe8b5 100644 --- a/sys/net/if_wg.c +++ b/sys/net/if_wg.c @@ -4619,7 +4619,7 @@ wg_ioctl(struct ifnet *ifp, u_long cmd, void *data) (ifp->if_flags & (IFF_UP | IFF_RUNNING)) != (IFF_UP | IFF_RUNNING)) { ifp->if_flags |= IFF_UP; - error = ifp->if_init(ifp); + error = if_init(ifp); } return error; case SIOCADDMULTI: @@ -4673,7 +4673,7 @@ wg_ioctl(struct ifnet *ifp, u_long cmd, void *data) * If interface is marked up and it is stopped, then * start it. */ - error = (*ifp->if_init)(ifp); + error = if_init(ifp); break; default: break; diff --git a/sys/net/lagg/if_lagg.c b/sys/net/lagg/if_lagg.c index 01e21defb574..7443adafd697 100644 --- a/sys/net/lagg/if_lagg.c +++ b/sys/net/lagg/if_lagg.c @@ -725,7 +725,7 @@ lagg_ioctl(struct ifnet *ifp, u_long cmd, void *data) break; case IFF_UP: case IFF_UP | IFF_RUNNING: - error = ifp->if_init(ifp); + error = if_init(ifp); break; } @@ -2115,7 +2115,7 @@ lagg_port_setsadl(struct lagg_port *lp, uint8_t *lladdr, if (ifp_port->if_init != NULL) { error = 0; if (ISSET(ifp_port->if_flags, IFF_RUNNING)) - error = ifp_port->if_init(ifp_port); + error = if_init(ifp_port); if (error != 0) { lagg_log(lp->lp_softc, LOG_WARNING, @@ -2161,7 +2161,7 @@ lagg_port_unsetsadl(struct lagg_port *lp) if (ifp_port->if_init != NULL) { error = 0; if (ISSET(ifp_port->if_flags, IFF_RUNNING)) - error = ifp_port->if_init(ifp_port); + error = if_init(ifp_port); if (error != 0) { lagg_log(lp->lp_softc, LOG_WARNING, @@ -2385,7 +2385,7 @@ lagg_port_setup(struct lagg_softc *sc, lagg_port_syncvlan(sc, lp); if (stopped) { - error = ifp_port->if_init(ifp_port); + error = if_init(ifp_port); if (error != 0) goto remove_port; } @@ -2415,7 +2415,7 @@ restore_ipv6lla: KASSERT(IFNET_LOCKED(ifp_port)); lagg_in6_ifdetach(ifp_port); if (stopped) { - if (ifp_port->if_init(ifp_port) != 0) { + if (if_init(ifp_port) != 0) { lagg_log(sc, LOG_WARNING, "couldn't re-start port %s\n", ifp_port->if_xname); @@ -2487,7 +2487,7 @@ lagg_port_teardown(struct lagg_softc *sc, struct lagg_port *lp, IFNET_UNLOCK(ifp_port); if (stopped) { - ifp_port->if_init(ifp_port); + if_init(ifp_port); } if (is_ifdetach == false) { diff --git a/sys/net/link_proto.c b/sys/net/link_proto.c index 2c4ecf621ba4..58531aacb620 100644 --- a/sys/net/link_proto.c +++ b/sys/net/link_proto.c @@ -255,7 +255,7 @@ link_control(struct socket *so, unsigned long cmd, void *data, return error; else if ((ifp->if_flags & IFF_RUNNING) != 0 && ifp->if_init != NULL) - return (*ifp->if_init)(ifp); + return if_init(ifp); else return 0; default: From afa5ce1c4a403494e85aac57a1147698f6e0574c Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Fri, 24 Dec 2021 21:28:51 +0000 Subject: [PATCH 5/9] altq(9): Fix missing IFNET_LOCK around if_ioctl. --- sys/altq/altq_afmap.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/sys/altq/altq_afmap.c b/sys/altq/altq_afmap.c index d210eae19468..5f80b20bd10a 100644 --- a/sys/altq/altq_afmap.c +++ b/sys/altq/altq_afmap.c @@ -369,10 +369,15 @@ afmioctl(dev_t dev, ioctlcmd_t cmd, void *addr, int flag, flowmap = (struct atm_flowmap *)addr; flowmap->af_ifname[IFNAMSIZ-1] = '\0'; ifp = ifunit(flowmap->af_ifname); - if (ifp == NULL || (ifp->if_flags & IFF_RUNNING) == 0) + if (ifp == NULL) + return ENXIO; + + IFNET_LOCK(ifp); + if ((ifp->if_flags & IFF_RUNNING) == 0) error = ENXIO; else error = if_ioctl(ifp, cmd, addr); + IFNET_UNLOCK(ifp); return error; } From 797415402e25c360215eb92ea3e1da27d8b1185f Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Sat, 25 Dec 2021 01:54:20 +0000 Subject: [PATCH 6/9] sys/net: Assert IFNET_LOCKED in if_ioctl, if_init, and if_stop. Exception: Not for SIOCADDMULTI/SIOCDELMULTI, for which it is the driver's responsibility to take internal locks. Typically this is already done via struct ethercom::ec_lock. --- sys/net/if.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/sys/net/if.c b/sys/net/if.c index 9fb661b39731..9afc286be5eb 100644 --- a/sys/net/if.c +++ b/sys/net/if.c @@ -2733,13 +2733,25 @@ ifpromisc(struct ifnet *ifp, int pswitch) * Apply an ioctl command to the interface. Returns 0 on success, * nonzero errno(3) number on failure. * - * May sleep. Caller must hold ifp->if_ioctl_lock. + * For SIOCADDMULTI/SIOCDELMULTI, caller need not hold locks -- it + * is the driver's responsibility to take any internal locks. + * (Kernel logic should generally invoke these only through + * if_mcast_op.) + * + * For all other ioctls, caller must hold ifp->if_ioctl_lock, + * a.k.a. IFNET_LOCK. May sleep. */ int if_ioctl(struct ifnet *ifp, u_long cmd, void *data) { - KASSERT(1 || IFNET_LOCKED(ifp)); /* XXX not yet */ + switch (cmd) { + case SIOCADDMULTI: + case SIOCDELMULTI: + break; + default: + KASSERTMSG(IFNET_LOCKED(ifp), "%s", ifp->if_xname); + } return (*ifp->if_ioctl)(ifp, cmd, data); } @@ -2758,7 +2770,7 @@ int if_init(struct ifnet *ifp) { - KASSERT(1 || IFNET_LOCKED(ifp)); /* XXX not yet */ + KASSERTMSG(IFNET_LOCKED(ifp), "%s", ifp->if_xname); return (*ifp->if_init)(ifp); } @@ -2777,7 +2789,7 @@ void if_stop(struct ifnet *ifp, int disable) { - KASSERT(1 || IFNET_LOCKED(ifp)); /* XXX not yet */ + KASSERTMSG(IFNET_LOCKED(ifp), "%s", ifp->if_xname); (*ifp->if_stop)(ifp, disable); } @@ -3864,11 +3876,6 @@ if_mcast_op(ifnet_t *ifp, const unsigned long cmd, const struct sockaddr *sa) int rc; struct ifreq ifr; - /* - * XXX NOMPSAFE - this calls if_ioctl without holding IFNET_LOCK() - * in some cases - e.g. when called from vlan/netinet/netinet6 code - * directly rather than via doifoictl() - */ ifreq_setaddr(cmd, &ifr, sa); rc = if_ioctl(ifp, cmd, &ifr); From 636da2bbad65fc0e293d0619406b76ce295f29a3 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Fri, 24 Dec 2021 23:33:27 +0000 Subject: [PATCH 7/9] ethersubr(9): Assert IFNET_LOCKED in ether_ioctl_reinit. Changes to if_flags are nontrivial configuration changes that require the long-term ioctl lock. --- sys/net/if_ethersubr.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sys/net/if_ethersubr.c b/sys/net/if_ethersubr.c index ae8d6621d72c..05adeccb0159 100644 --- a/sys/net/if_ethersubr.c +++ b/sys/net/if_ethersubr.c @@ -1455,6 +1455,8 @@ ether_ioctl_reinit(struct ethercom *ec) struct ifnet *ifp = &ec->ec_if; int error; + KASSERTMSG(IFNET_LOCKED(ifp), "%s", ifp->if_xname); + switch (ifp->if_flags & (IFF_UP | IFF_RUNNING)) { case IFF_RUNNING: /* From 386cc172d755218c69eab77755b4433f74457ef4 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Sun, 26 Dec 2021 11:11:13 +0000 Subject: [PATCH 8/9] sys/net: Document if_flags_set with a comment. --- sys/net/if.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/sys/net/if.c b/sys/net/if.c index 9afc286be5eb..334fb3e18230 100644 --- a/sys/net/if.c +++ b/sys/net/if.c @@ -3832,6 +3832,15 @@ if_do_dad(struct ifnet *ifp) } } +/* + * if_flags_set(ifp, flags) + * + * Ask ifp to change ifp->if_flags to flags, as if with the + * SIOCSIFFLAGS ioctl command. + * + * May sleep. Caller must hold ifp->if_ioctl_lock, a.k.a + * IFNET_LOCK. + */ int if_flags_set(ifnet_t *ifp, const u_short flags) { From b5aabd3bb196c7a01ca8ec7239c3a7f4dff5db5c Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Sun, 26 Dec 2021 11:11:27 +0000 Subject: [PATCH 9/9] sys/net: Document if_mcast_op with comment and refuse other commands. Meant only for multicast addition/deletion operations, nothing else. --- sys/net/if.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/sys/net/if.c b/sys/net/if.c index 334fb3e18230..c6fe8066c3a7 100644 --- a/sys/net/if.c +++ b/sys/net/if.c @@ -3879,12 +3879,31 @@ if_flags_set(ifnet_t *ifp, const u_short flags) return rc; } +/* + * if_mcast_op(ifp, cmd, sa) + * + * Apply a multicast command, SIOCADDMULTI/SIOCDELMULTI, to the + * interface. Returns 0 on success, nonzero errno(3) number on + * failure. + * + * May sleep. + * + * Use this, not if_ioctl, for the multicast commands. + */ int if_mcast_op(ifnet_t *ifp, const unsigned long cmd, const struct sockaddr *sa) { int rc; struct ifreq ifr; + switch (cmd) { + case SIOCADDMULTI: + case SIOCDELMULTI: + break; + default: + panic("invalid ifnet multicast command: 0x%lx", cmd); + } + ifreq_setaddr(cmd, &ifr, sa); rc = if_ioctl(ifp, cmd, &ifr);