From edce10f0bf583dd1330223d6bf623e5377c28528 Mon Sep 17 00:00:00 2001 From: mpi Date: Tue, 2 Jan 2018 12:52:17 +0000 Subject: [PATCH] Move the NET_LOCK() inside the switch and start documenting which field is protected by which lock. ok bluhm@, visa@ --- sys/net/if.c | 57 ++++++++++++++++++++------------- sys/net/if_var.h | 82 +++++++++++++++++++++++++----------------------- 2 files changed, 78 insertions(+), 61 deletions(-) diff --git a/sys/net/if.c b/sys/net/if.c index 184867b8d61..20e68cc498d 100644 --- a/sys/net/if.c +++ b/sys/net/if.c @@ -1,4 +1,4 @@ -/* $OpenBSD: if.c,v 1.532 2017/12/29 17:05:25 bluhm Exp $ */ +/* $OpenBSD: if.c,v 1.533 2018/01/02 12:52:17 mpi Exp $ */ /* $NetBSD: if.c,v 1.35 1996/05/07 05:26:04 thorpej Exp $ */ /* @@ -1854,13 +1854,12 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data, struct proc *p) oif_flags = ifp->if_flags; oif_xflags = ifp->if_xflags; - NET_LOCK(); - switch (cmd) { case SIOCIFAFATTACH: case SIOCIFAFDETACH: if ((error = suser(p, 0)) != 0) break; + NET_LOCK(); switch (ifar->ifar_af) { case AF_INET: /* attach is a noop for AF_INET */ @@ -1878,22 +1877,21 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data, struct proc *p) default: error = EAFNOSUPPORT; } + NET_UNLOCK(); break; case SIOCSIFFLAGS: if ((error = suser(p, 0)) != 0) break; + NET_LOCK(); ifp->if_flags = (ifp->if_flags & IFF_CANTCHANGE) | (ifr->ifr_flags & ~IFF_CANTCHANGE); error = (*ifp->if_ioctl)(ifp, cmd, data); if (error != 0) { ifp->if_flags = oif_flags; - break; - } - - if (ISSET(oif_flags ^ ifp->if_flags, IFF_UP)) { + } else if (ISSET(oif_flags ^ ifp->if_flags, IFF_UP)) { s = splnet(); if (ISSET(ifp->if_flags, IFF_UP)) if_up(ifp); @@ -1901,17 +1899,21 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data, struct proc *p) if_down(ifp); splx(s); } + NET_UNLOCK(); break; case SIOCSIFXFLAGS: if ((error = suser(p, 0)) != 0) break; + NET_LOCK(); #ifdef INET6 if (ISSET(ifr->ifr_flags, IFXF_AUTOCONF6)) { error = in6_ifattach(ifp); - if (error != 0) + if (error != 0) { + NET_UNLOCK(); break; + } } #endif /* INET6 */ @@ -1942,8 +1944,6 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data, struct proc *p) ifp->if_xflags |= IFXF_WOL; error = ifp->if_wol(ifp, 1); splx(s); - if (error) - break; } if (ISSET(ifp->if_xflags, IFXF_WOL) && !ISSET(ifr->ifr_flags, IFXF_WOL)) { @@ -1951,8 +1951,6 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data, struct proc *p) ifp->if_xflags &= ~IFXF_WOL; error = ifp->if_wol(ifp, 0); splx(s); - if (error) - break; } } else if (ISSET(ifr->ifr_flags, IFXF_WOL)) { ifr->ifr_flags &= ~IFXF_WOL; @@ -1960,20 +1958,26 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data, struct proc *p) } #endif - ifp->if_xflags = (ifp->if_xflags & IFXF_CANTCHANGE) | - (ifr->ifr_flags & ~IFXF_CANTCHANGE); + if (error == 0) + ifp->if_xflags = (ifp->if_xflags & IFXF_CANTCHANGE) | + (ifr->ifr_flags & ~IFXF_CANTCHANGE); + NET_UNLOCK(); break; case SIOCSIFMETRIC: if ((error = suser(p, 0)) != 0) break; + NET_LOCK(); ifp->if_metric = ifr->ifr_metric; + NET_UNLOCK(); break; case SIOCSIFMTU: if ((error = suser(p, 0)) != 0) break; + NET_LOCK(); error = (*ifp->if_ioctl)(ifp, cmd, data); + NET_UNLOCK(); if (!error) rtm_ifchg(ifp); break; @@ -2013,27 +2017,34 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data, struct proc *p) case SIOCSIFRDOMAIN: if ((error = suser(p, 0)) != 0) break; + NET_LOCK(); error = if_setrdomain(ifp, ifr->ifr_rdomainid); + NET_UNLOCK(); break; case SIOCAIFGROUP: if ((error = suser(p, 0))) break; - if ((error = if_addgroup(ifp, ifgr->ifgr_group))) - break; - error = (*ifp->if_ioctl)(ifp, cmd, data); - if (error == ENOTTY) - error = 0; + NET_LOCK(); + error = if_addgroup(ifp, ifgr->ifgr_group); + if (error == 0) { + error = (*ifp->if_ioctl)(ifp, cmd, data); + if (error == ENOTTY) + error = 0; + } + NET_UNLOCK(); break; case SIOCDIFGROUP: if ((error = suser(p, 0))) break; + NET_LOCK(); error = (*ifp->if_ioctl)(ifp, cmd, data); if (error == ENOTTY) error = 0; if (error == 0) error = if_delgroup(ifp, ifgr->ifgr_group); + NET_UNLOCK(); break; case SIOCSIFLLADDR: @@ -2045,6 +2056,7 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data, struct proc *p) error = EINVAL; break; } + NET_LOCK(); switch (ifp->if_type) { case IFT_ETHER: case IFT_CARP: @@ -2063,6 +2075,7 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data, struct proc *p) if (error == 0) ifnewlladdr(ifp); + NET_UNLOCK(); break; case SIOCSIFLLPRIO: @@ -2072,7 +2085,9 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data, struct proc *p) error = EINVAL; break; } + NET_LOCK(); ifp->if_llprio = ifr->ifr_llprio; + NET_UNLOCK(); break; case SIOCDIFPHYADDR: @@ -2090,11 +2105,13 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data, struct proc *p) break; /* FALLTHROUGH */ default: + NET_LOCK(); error = ((*so->so_proto->pr_usrreq)(so, PRU_CONTROL, (struct mbuf *) cmd, (struct mbuf *) data, (struct mbuf *) ifp, p)); if (error == EOPNOTSUPP) error = ((*ifp->if_ioctl)(ifp, cmd, data)); + NET_UNLOCK(); break; } @@ -2104,8 +2121,6 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data, struct proc *p) if (((oif_flags ^ ifp->if_flags) & IFF_UP) != 0) getmicrotime(&ifp->if_lastchange); - NET_UNLOCK(); - return (error); } diff --git a/sys/net/if_var.h b/sys/net/if_var.h index 65ce87be4d3..b45c88b22c1 100644 --- a/sys/net/if_var.h +++ b/sys/net/if_var.h @@ -1,4 +1,4 @@ -/* $OpenBSD: if_var.h,v 1.85 2017/12/15 01:37:30 dlg Exp $ */ +/* $OpenBSD: if_var.h,v 1.86 2018/01/02 12:52:17 mpi Exp $ */ /* $NetBSD: if.h,v 1.23 1996/05/07 02:40:27 thorpej Exp $ */ /* @@ -91,7 +91,14 @@ struct if_clone { #define IF_CLONE_INITIALIZER(name, create, destroy) \ { { 0 }, name, sizeof(name) - 1, create, destroy } - +/* + * Locks used to protect struct members in this file: + * I immutable after creation + * d protection left do the driver + * c only used in ioctl or routing socket contexts (kernel lock) + * k kernel lock + * N net lock + */ /* * Structure defining a queue for a network interface. * @@ -102,17 +109,17 @@ TAILQ_HEAD(ifnet_head, ifnet); /* the actual queue head */ struct ifnet { /* and the entries */ void *if_softc; /* lower-level data for this if */ struct refcnt if_refcnt; - TAILQ_ENTRY(ifnet) if_list; /* all struct ifnets are chained */ - TAILQ_HEAD(, ifaddr) if_addrlist; /* linked list of addresses per if */ - TAILQ_HEAD(, ifmaddr) if_maddrlist; /* list of multicast records */ - TAILQ_HEAD(, ifg_list) if_groups; /* linked list of groups per if */ - struct hook_desc_head *if_addrhooks; /* address change callbacks */ - struct hook_desc_head *if_linkstatehooks; /* link change callbacks */ - struct hook_desc_head *if_detachhooks; /* detach callbacks */ - /* check or clean routes (+ or -)'d */ + TAILQ_ENTRY(ifnet) if_list; /* [k] all struct ifnets are chained */ + TAILQ_HEAD(, ifaddr) if_addrlist; /* [N] list of addresses per if */ + TAILQ_HEAD(, ifmaddr) if_maddrlist; /* [N] list of multicast records */ + TAILQ_HEAD(, ifg_list) if_groups; /* [N] list of groups per if */ + struct hook_desc_head *if_addrhooks; /* [I] address change callbacks */ + struct hook_desc_head *if_linkstatehooks; /* [I] link change callbacks*/ + struct hook_desc_head *if_detachhooks; /* [I] detach callbacks */ + /* [I] check or clean routes (+ or -)'d */ void (*if_rtrequest)(struct ifnet *, int, struct rtentry *); - char if_xname[IFNAMSIZ]; /* external name (name + unit) */ - int if_pcount; /* number of promiscuous listeners */ + char if_xname[IFNAMSIZ]; /* [I] external name (name + unit) */ + int if_pcount; /* [k] # of promiscuous listeners */ caddr_t if_bpf; /* packet filter structure */ caddr_t if_bridgeport; /* used by bridge ports */ caddr_t if_switchport; /* used by switch ports */ @@ -125,48 +132,43 @@ struct ifnet { /* and the entries */ } if_carp_ptr; #define if_carp if_carp_ptr.carp_s #define if_carpdev if_carp_ptr.carp_d - unsigned int if_index; /* numeric abbreviation for this if */ + unsigned int if_index; /* [I] unique index for this if */ short if_timer; /* time 'til if_watchdog called */ - unsigned short if_flags; /* up/down, broadcast, etc. */ - int if_xflags; /* extra softnet flags */ + unsigned short if_flags; /* [N] up/down, broadcast, etc. */ + int if_xflags; /* [N] extra softnet flags */ struct if_data if_data; /* stats and other data about if */ - u_int32_t if_hardmtu; /* maximum MTU device supports */ - char if_description[IFDESCRSIZE]; /* interface description */ - u_short if_rtlabelid; /* next route label */ - u_int8_t if_priority; - u_int8_t if_llprio; /* link layer priority */ - struct timeout *if_slowtimo; /* watchdog timeout */ - struct task *if_watchdogtask; /* watchdog task */ - struct task *if_linkstatetask; /* task to do route updates */ + uint32_t if_hardmtu; /* [d] maximum MTU device supports */ + char if_description[IFDESCRSIZE]; /* [c] interface description */ + u_short if_rtlabelid; /* [c] next route label */ + uint8_t if_priority; /* [c] route priority offset */ + uint8_t if_llprio; /* [N] link layer priority */ + struct timeout *if_slowtimo; /* [I] watchdog timeout */ + struct task *if_watchdogtask; /* [I] watchdog task */ + struct task *if_linkstatetask; /* [I] task to do route updates */ /* procedure handles */ SRPL_HEAD(, ifih) if_inputs; /* input routines (dequeue) */ - - /* output routine (enqueue) */ int (*if_output)(struct ifnet *, struct mbuf *, struct sockaddr *, - struct rtentry *); - + struct rtentry *); /* output routine (enqueue) */ /* link level output function */ int (*if_ll_output)(struct ifnet *, struct mbuf *, struct sockaddr *, struct rtentry *); - /* initiate output routine */ - void (*if_start)(struct ifnet *); - /* ioctl routine */ - int (*if_ioctl)(struct ifnet *, u_long, caddr_t); - /* timer routine */ - void (*if_watchdog)(struct ifnet *); - int (*if_wol)(struct ifnet *, int); + void (*if_start)(struct ifnet *); /* initiate output */ + int (*if_ioctl)(struct ifnet *, u_long, caddr_t); /* ioctl hook */ + void (*if_watchdog)(struct ifnet *); /* timer routine */ + int (*if_wol)(struct ifnet *, int); /* WoL routine **/ + /* queues */ struct ifqueue if_snd; /* transmit queue */ - struct ifqueue **if_ifqs; /* pointer to an array of sndqs */ + struct ifqueue **if_ifqs; /* [I] pointer to an array of sndqs */ void (*if_qstart)(struct ifqueue *); - unsigned int if_nifqs; + unsigned int if_nifqs; /* [I] number of output queues */ struct ifiqueue if_rcv; /* rx/input queue */ - struct ifiqueue **if_iqs; /* pointer to the array of iqs */ - unsigned int if_niqs; + struct ifiqueue **if_iqs; /* [I] pointer to the array of iqs */ + unsigned int if_niqs; /* [I] number of input queues */ - struct sockaddr_dl *if_sadl; /* pointer to our sockaddr_dl */ + struct sockaddr_dl *if_sadl; /* [N] pointer to our sockaddr_dl */ void *if_afdata[AF_MAX]; }; @@ -189,7 +191,7 @@ struct ifnet { /* and the entries */ #define if_iqdrops if_data.ifi_iqdrops #define if_oqdrops if_data.ifi_oqdrops #define if_noproto if_data.ifi_noproto -#define if_lastchange if_data.ifi_lastchange +#define if_lastchange if_data.ifi_lastchange /* [c] last op. state change */ #define if_capabilities if_data.ifi_capabilities #define if_rdomain if_data.ifi_rdomain -- 2.20.1