Switch the list of span interfaces and interfaces to SMR.

This removes the KERNEL_LOCK() around the list iteration in bridge_enqueue().

Since the NET_LOCK() isn't protecting any data structure, release it early
in all the code paths coming from the Network Stack to prevent possible
deadlock situations with smr_barrier().

bridge_input() is still KERNEL_LOCK()ed as well as bridge_filterrule().

ok visa@

3 files changed, 123 insertions, 86 deletions

diff --git a/sys/net/bridgectl.c b/sys/net/bridgectl.c
index 4d195aa4af3..b31bcd07ca1 100644
--- a/sys/net/bridgectl.c
+++ b/sys/net/bridgectl.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: bridgectl.c,v 1.18 2019/04/28 22:15:57 mpi Exp $	*/
+/*	$OpenBSD: bridgectl.c,v 1.19 2019/05/12 19:53:22 mpi Exp $	*/
 
 /*
  * Copyright (c) 1999, 2000 Jason L. Wright (jason@thought.net)
@@ -723,8 +723,12 @@ u_int8_t
 bridge_filterrule(struct brl_head *h, struct ether_header *eh, struct mbuf *m)
 {
 	struct brl_node *n;
-	u_int8_t flags;
+	u_int8_t action, flags;
 
+	if (SIMPLEQ_EMPTY(h))
+		return (BRL_ACTION_PASS);
+
+	KERNEL_LOCK();
 	SIMPLEQ_FOREACH(n, h, brl_next) {
 		if (!bridge_arpfilter(n, eh, m))
 			continue;
@@ -753,13 +757,16 @@ bridge_filterrule(struct brl_head *h, struct ether_header *eh, struct mbuf *m)
 			goto return_action;
 		}
 	}
+	KERNEL_UNLOCK();
 	return (BRL_ACTION_PASS);
 
 return_action:
 #if NPF > 0
 	pf_tag_packet(m, n->brl_tag, -1);
 #endif
-	return (n->brl_action);
+	action = n->brl_action;
+	KERNEL_UNLOCK();
+	return (action);
 }
 
 int
@@ -781,6 +788,9 @@ bridge_addrule(struct bridge_iflist *bif, struct ifbrlreq *req, int out)
 	else
 		n->brl_tag = 0;
 #endif
+
+	KERNEL_ASSERT_LOCKED();
+
 	if (out) {
 		n->brl_flags &= ~BRL_FLAG_IN;
 		n->brl_flags |= BRL_FLAG_OUT;
@@ -798,6 +808,8 @@ bridge_flushrule(struct bridge_iflist *bif)
 {
 	struct brl_node *p;
 
+	KERNEL_ASSERT_LOCKED();
+
 	while (!SIMPLEQ_EMPTY(&bif->bif_brlin)) {
 		p = SIMPLEQ_FIRST(&bif->bif_brlin);
 		SIMPLEQ_REMOVE_HEAD(&bif->bif_brlin, brl_next);
diff --git a/sys/net/if_bridge.c b/sys/net/if_bridge.c
index ca082a07eab..42d4a8b8503 100644
--- a/sys/net/if_bridge.c
+++ b/sys/net/if_bridge.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: if_bridge.c,v 1.331 2019/05/12 16:38:02 sashan Exp $	*/
+/*	$OpenBSD: if_bridge.c,v 1.332 2019/05/12 19:53:22 mpi Exp $	*/
 
 /*
  * Copyright (c) 1999, 2000 Jason L. Wright (jason@thought.net)
@@ -137,7 +137,6 @@ int bridge_ipsec(struct ifnet *, struct ether_header *, int, struct llc *,
 #endif
 int     bridge_clone_create(struct if_clone *, int);
 int	bridge_clone_destroy(struct ifnet *);
-int	bridge_delete(struct bridge_softc *, struct bridge_iflist *);
 
 #define	ETHERADDR_IS_IP_MCAST(a) \
 	/* struct etheraddr *a;	*/				\
@@ -173,8 +172,8 @@ bridge_clone_create(struct if_clone *ifc, int unit)
 	sc->sc_brtmax = BRIDGE_RTABLE_MAX;
 	sc->sc_brttimeout = BRIDGE_RTABLE_TIMEOUT;
 	timeout_set(&sc->sc_brtimeout, bridge_rtage, sc);
-	SLIST_INIT(&sc->sc_iflist);
-	SLIST_INIT(&sc->sc_spanlist);
+	SMR_SLIST_INIT(&sc->sc_iflist);
+	SMR_SLIST_INIT(&sc->sc_spanlist);
 	mtx_init(&sc->sc_mtx, IPL_MPFLOOR);
 	for (i = 0; i < BRIDGE_RTABLE_SIZE; i++)
 		LIST_INIT(&sc->sc_rts[i]);
@@ -218,11 +217,18 @@ bridge_clone_destroy(struct ifnet *ifp)
 	struct bridge_softc *sc = ifp->if_softc;
 	struct bridge_iflist *bif;
 
+	/*
+	 * bridge(4) detach hook doesn't need the NET_LOCK(), worst the
+	 * use of smr_barrier() while holding the lock might lead to a
+	 * deadlock situation.
+	 */
+	NET_ASSERT_UNLOCKED();
+
 	bridge_stop(sc);
 	bridge_rtflush(sc, IFBF_FLUSHALL);
-	while ((bif = SLIST_FIRST(&sc->sc_iflist)) != NULL)
+	while ((bif = SMR_SLIST_FIRST_LOCKED(&sc->sc_iflist)) != NULL)
 		bridge_ifremove(bif);
-	while ((bif = SLIST_FIRST(&sc->sc_spanlist)) != NULL)
+	while ((bif = SMR_SLIST_FIRST_LOCKED(&sc->sc_spanlist)) != NULL)
 		bridge_spanremove(bif);
 
 	bstp_destroy(sc->sc_stp);
@@ -241,26 +247,6 @@ bridge_clone_destroy(struct ifnet *ifp)
 }
 
 int
-bridge_delete(struct bridge_softc *sc, struct bridge_iflist *bif)
-{
-	int error;
-
-	if (bif->bif_flags & IFBIF_STP)
-		bstp_delete(bif->bif_stp);
-
-	bif->ifp->if_bridgeidx = 0;
-	error = ifpromisc(bif->ifp, 0);
-	hook_disestablish(bif->ifp->if_detachhooks, bif->bif_dhcookie);
-
-	if_ih_remove(bif->ifp, bridge_input, NULL);
-	bridge_rtdelete(sc, bif->ifp, 0);
-	bridge_flushrule(bif);
-	free(bif, M_DEVBUF, sizeof *bif);
-
-	return (error);
-}
-
-int
 bridge_ioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
 {
 	struct bridge_softc *sc = (struct bridge_softc *)ifp->if_softc;
@@ -272,6 +258,13 @@ bridge_ioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
 	struct bstp_state *bs = sc->sc_stp;
 	int error = 0;
 
+	/*
+	 * bridge(4) data structure aren't protected by the NET_LOCK().
+	 * Idealy it shouldn't be taken before calling `ifp->if_ioctl'
+	 * but we aren't there yet.
+	 */
+	NET_UNLOCK();
+
 	switch (cmd) {
 	case SIOCBRDGADD:
 	/* bridge(4) does not distinguish between routing/forwarding ports */
@@ -283,10 +276,7 @@ bridge_ioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
 
 		/* try to create the interface if it does't exist */
 		if (ifs == NULL) {
-			/* XXXSMP breaks atomicity */
-			NET_UNLOCK();
 			error = if_clone_create(req->ifbr_ifsname, 0);
-			NET_LOCK();
 			if (error == 0)
 				ifs = ifunit(req->ifbr_ifsname);
 		}
@@ -305,7 +295,7 @@ bridge_ioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
 		}
 
 		/* If it's in the span list, it can't be a member. */
-		SLIST_FOREACH(bif, &sc->sc_spanlist, bif_next) {
+		SMR_SLIST_FOREACH_LOCKED(bif, &sc->sc_spanlist, bif_next) {
 			if (bif->ifp == ifs)
 				break;
 		}
@@ -340,7 +330,7 @@ bridge_ioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
 		bif->bif_dhcookie = hook_establish(ifs->if_detachhooks, 0,
 		    bridge_ifdetach, bif);
 		if_ih_insert(bif->ifp, bridge_input, NULL);
-		SLIST_INSERT_HEAD(&sc->sc_iflist, bif, bif_next);
+		SMR_SLIST_INSERT_HEAD_LOCKED(&sc->sc_iflist, bif, bif_next);
 		break;
 	case SIOCBRDGDEL:
 		if ((error = suser(curproc)) != 0)
@@ -355,7 +345,7 @@ bridge_ioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
 			break;
 		}
 		bif = bridge_getbif(ifs);
-		error = bridge_ifremove(bif);
+		bridge_ifremove(bif);
 		break;
 	case SIOCBRDGIFS:
 		error = bridge_bifconf(sc, (struct ifbifconf *)data);
@@ -376,7 +366,7 @@ bridge_ioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
 			error = EBUSY;
 			break;
 		}
-		SLIST_FOREACH(bif, &sc->sc_spanlist, bif_next) {
+		SMR_SLIST_FOREACH_LOCKED(bif, &sc->sc_spanlist, bif_next) {
 			if (bif->ifp == ifs)
 				break;
 		}
@@ -389,14 +379,14 @@ bridge_ioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
 			error = ENOMEM;
 			break;
 		}
+		bif->bridge_sc = sc;
 		bif->ifp = ifs;
 		bif->bif_flags = IFBIF_SPAN;
-		bif->bridge_sc = sc;
-		bif->bif_dhcookie = hook_establish(ifs->if_detachhooks, 0,
-		    bridge_spandetach, bif);
 		SIMPLEQ_INIT(&bif->bif_brlin);
 		SIMPLEQ_INIT(&bif->bif_brlout);
-		SLIST_INSERT_HEAD(&sc->sc_spanlist, bif, bif_next);
+		bif->bif_dhcookie = hook_establish(ifs->if_detachhooks, 0,
+		    bridge_spandetach, bif);
+		SMR_SLIST_INSERT_HEAD_LOCKED(&sc->sc_spanlist, bif, bif_next);
 		break;
 	case SIOCBRDGDELS:
 		if ((error = suser(curproc)) != 0)
@@ -406,7 +396,7 @@ bridge_ioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
 			error = ENOENT;
 			break;
 		}
-		SLIST_FOREACH(bif, &sc->sc_spanlist, bif_next) {
+		SMR_SLIST_FOREACH_LOCKED(bif, &sc->sc_spanlist, bif_next) {
 			if (bif->ifp == ifs)
 				break;
 		}
@@ -544,6 +534,7 @@ bridge_ioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
 	if (!error)
 		error = bstp_ioctl(ifp, cmd, data);
 
+	NET_LOCK();
 	return (error);
 }
 
@@ -552,9 +543,29 @@ int
 bridge_ifremove(struct bridge_iflist *bif)
 {
 	struct bridge_softc *sc = bif->bridge_sc;
+	int error;
+
+	SMR_SLIST_REMOVE_LOCKED(&sc->sc_iflist, bif, bridge_iflist, bif_next);
+	hook_disestablish(bif->ifp->if_detachhooks, bif->bif_dhcookie);
+	if_ih_remove(bif->ifp, bridge_input, NULL);
 
-	SLIST_REMOVE(&sc->sc_iflist, bif, bridge_iflist, bif_next);
-	return (bridge_delete(sc, bif));
+	smr_barrier();
+
+	if (bif->bif_flags & IFBIF_STP) {
+		bstp_delete(bif->bif_stp);
+		bif->bif_stp = NULL;
+	}
+
+	bif->ifp->if_bridgeidx = 0;
+	error = ifpromisc(bif->ifp, 0);
+
+	bridge_rtdelete(sc, bif->ifp, 0);
+	bridge_flushrule(bif);
+
+	bif->ifp = NULL;
+	free(bif, M_DEVBUF, sizeof(*bif));
+
+	return (error);
 }
 
 void
@@ -562,8 +573,12 @@ bridge_spanremove(struct bridge_iflist *bif)
 {
 	struct bridge_softc *sc = bif->bridge_sc;
 
-	SLIST_REMOVE(&sc->sc_spanlist, bif, bridge_iflist, bif_next);
+	SMR_SLIST_REMOVE_LOCKED(&sc->sc_spanlist, bif, bridge_iflist, bif_next);
 	hook_disestablish(bif->ifp->if_detachhooks, bif->bif_dhcookie);
+
+	smr_barrier();
+
+	bif->ifp = NULL;
 	free(bif, M_DEVBUF, sizeof(*bif));
 }
 
@@ -572,7 +587,14 @@ bridge_ifdetach(void *xbif)
 {
 	struct bridge_iflist *bif = xbif;
 
+	/*
+	 * bridge(4) detach hook doesn't need the NET_LOCK(), worst the
+	 * use of smr_barrier() while holding the lock might lead to a
+	 * deadlock situation.
+	 */
+	NET_UNLOCK();
 	bridge_ifremove(bif);
+	NET_LOCK();
 }
 
 void
@@ -580,7 +602,14 @@ bridge_spandetach(void *xbif)
 {
 	struct bridge_iflist *bif = xbif;
 
+	/*
+	 * bridge(4) detach hook doesn't need the NET_LOCK(), worst the
+	 * use of smr_barrier() while holding the lock might lead to a
+	 * deadlock situation.
+	 */
+	NET_UNLOCK();
 	bridge_spanremove(bif);
+	NET_LOCK();
 }
 
 void
@@ -622,10 +651,10 @@ bridge_bifconf(struct bridge_softc *sc, struct ifbifconf *bifc)
 	int error = 0;
 	struct ifbreq *breq, *breqs = NULL;
 
-	SLIST_FOREACH(bif, &sc->sc_iflist, bif_next)
+	SMR_SLIST_FOREACH_LOCKED(bif, &sc->sc_iflist, bif_next)
 		total++;
 
-	SLIST_FOREACH(bif, &sc->sc_spanlist, bif_next)
+	SMR_SLIST_FOREACH_LOCKED(bif, &sc->sc_spanlist, bif_next)
 		total++;
 
 	if (bifc->ifbic_len == 0) {
@@ -637,7 +666,7 @@ bridge_bifconf(struct bridge_softc *sc, struct ifbifconf *bifc)
 	if (breqs == NULL)
 		goto done;
 
-	SLIST_FOREACH(bif, &sc->sc_iflist, bif_next) {
+	SMR_SLIST_FOREACH_LOCKED(bif, &sc->sc_iflist, bif_next) {
 		if (bifc->ifbic_len < (i + 1) * sizeof(*breqs))
 			break;
 		breq = &breqs[i];
@@ -650,7 +679,7 @@ bridge_bifconf(struct bridge_softc *sc, struct ifbifconf *bifc)
 			bridge_bifgetstp(sc, bif, breq);
 		i++;
 	}
-	SLIST_FOREACH(bif, &sc->sc_spanlist, bif_next) {
+	SMR_SLIST_FOREACH_LOCKED(bif, &sc->sc_spanlist, bif_next) {
 		if (bifc->ifbic_len < (i + 1) * sizeof(*breqs))
 			break;
 		breq = &breqs[i];
@@ -675,12 +704,14 @@ bridge_getbif(struct ifnet *ifp)
 	struct bridge_softc *sc;
 	struct ifnet *bifp;
 
+	KERNEL_ASSERT_LOCKED();
+
 	bifp = if_get(ifp->if_bridgeidx);
 	if (bifp == NULL)
 		return (NULL);
 
 	sc = bifp->if_softc;
-	SLIST_FOREACH(bif, &sc->sc_iflist, bif_next) {
+	SMR_SLIST_FOREACH_LOCKED(bif, &sc->sc_iflist, bif_next) {
 		if (bif->ifp == ifp)
 			break;
 	}
@@ -792,10 +823,9 @@ bridge_enqueue(struct ifnet *ifp, struct mbuf *m)
 		struct bridge_softc *sc = brifp->if_softc;
 		struct bridge_iflist *bif;
 		struct mbuf *mc;
-		int used = 0;
 
-		KERNEL_LOCK();
-		SLIST_FOREACH(bif, &sc->sc_iflist, bif_next) {
+		smr_read_enter();
+		SMR_SLIST_FOREACH(bif, &sc->sc_iflist, bif_next) {
 			dst_if = bif->ifp;
 			if ((dst_if->if_flags & IFF_RUNNING) == 0)
 				continue;
@@ -818,24 +848,18 @@ bridge_enqueue(struct ifnet *ifp, struct mbuf *m)
 			    BRL_ACTION_BLOCK)
 				continue;
 
-			if (SLIST_NEXT(bif, bif_next) == NULL) {
-				used = 1;
-				mc = m;
-			} else {
-				mc = m_dup_pkt(m, ETHER_ALIGN, M_NOWAIT);
-				if (mc == NULL) {
-					brifp->if_oerrors++;
-					continue;
-				}
+			mc = m_dup_pkt(m, ETHER_ALIGN, M_NOWAIT);
+			if (mc == NULL) {
+				brifp->if_oerrors++;
+				continue;
 			}
 
 			error = bridge_ifenqueue(brifp, dst_if, mc);
 			if (error)
 				continue;
 		}
-		KERNEL_UNLOCK();
-		if (!used)
-			m_freem(m);
+		smr_read_leave();
+		m_freem(m);
 		goto out;
 	}
 
@@ -1125,7 +1149,7 @@ bridge_process(struct ifnet *ifp, struct mbuf *m)
 #endif
 
 	sc = brifp->if_softc;
-	SLIST_FOREACH(bif, &sc->sc_iflist, bif_next) {
+	SMR_SLIST_FOREACH_LOCKED(bif, &sc->sc_iflist, bif_next) {
 		if (bif->ifp == ifp)
 			break;
 	}
@@ -1176,7 +1200,7 @@ bridge_process(struct ifnet *ifp, struct mbuf *m)
 	 * Unicast, make sure it's not for us.
 	 */
 	bif0 = bif;
-	SLIST_FOREACH(bif, &sc->sc_iflist, bif_next) {
+	SMR_SLIST_FOREACH_LOCKED(bif, &sc->sc_iflist, bif_next) {
 		if (bif->ifp->if_type != IFT_ETHER)
 			continue;
 		if (bridge_ourether(bif->ifp, eh->ether_dhost)) {
@@ -1229,7 +1253,7 @@ bridge_broadcast(struct bridge_softc *sc, struct ifnet *ifp,
 	bif = bridge_getbif(ifp);
 	protected = bif->bif_protected;
 
-	SLIST_FOREACH(bif, &sc->sc_iflist, bif_next) {
+	SMR_SLIST_FOREACH_LOCKED(bif, &sc->sc_iflist, bif_next) {
 		dst_if = bif->ifp;
 
 		if ((dst_if->if_flags & IFF_RUNNING) == 0)
@@ -1270,7 +1294,7 @@ bridge_broadcast(struct bridge_softc *sc, struct ifnet *ifp,
 			sc->sc_if.if_oerrors++;
 
 		/* If last one, reuse the passed-in mbuf */
-		if (SLIST_NEXT(bif, bif_next) == NULL) {
+		if (SMR_SLIST_NEXT_LOCKED(bif, bif_next) == NULL) {
 			mc = m;
 			used = 1;
 		} else {
@@ -1346,11 +1370,8 @@ bridge_span(struct ifnet *brifp, struct mbuf *m)
 	struct mbuf *mc;
 	int error;
 
-	if (SLIST_EMPTY(&sc->sc_spanlist))
-		return;
-
-	KERNEL_LOCK();
-	SLIST_FOREACH(bif, &sc->sc_spanlist, bif_next) {
+	smr_read_enter();
+	SMR_SLIST_FOREACH(bif, &sc->sc_spanlist, bif_next) {
 		ifp = bif->ifp;
 
 		if ((ifp->if_flags & IFF_RUNNING) == 0)
@@ -1366,7 +1387,7 @@ bridge_span(struct ifnet *brifp, struct mbuf *m)
 		if (error)
 			continue;
 	}
-	KERNEL_UNLOCK();
+	smr_read_leave();
 }
 
 /*
diff --git a/sys/net/if_bridge.h b/sys/net/if_bridge.h
index 1495f83846f..74e1cfd9838 100644
--- a/sys/net/if_bridge.h
+++ b/sys/net/if_bridge.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: if_bridge.h,v 1.64 2019/04/28 22:15:57 mpi Exp $	*/
+/*	$OpenBSD: if_bridge.h,v 1.65 2019/05/12 19:53:22 mpi Exp $	*/
 
 /*
  * Copyright (c) 1999, 2000 Jason L. Wright (jason@thought.net)
@@ -35,6 +35,7 @@
 #ifndef _NET_IF_BRIDGE_H_
 #define _NET_IF_BRIDGE_H_
 
+#include <sys/smr.h>
 #include <sys/timeout.h>
 #include <net/pfvar.h>
 
@@ -410,14 +411,18 @@ struct bstp_state {
 
 /*
  * Bridge interface list
+ *
+ *  Locks used to protect struct members in this file:
+ *	I	immutable after creation
+ *	k	kernel lock
  */
 struct bridge_iflist {
-	SLIST_ENTRY(bridge_iflist)	bif_next;	/* next in list */
-	struct bridge_softc		*bridge_sc;
-	struct bstp_port		*bif_stp;	/* STP port state */
-	struct brl_head			bif_brlin;	/* input rules */
-	struct brl_head			bif_brlout;	/* output rules */
-	struct				ifnet *ifp;	/* member interface */
+	SMR_SLIST_ENTRY(bridge_iflist)	bif_next;	/* [k] next in list */
+	struct bridge_softc		*bridge_sc;	/* [I] sc backpointer */
+	struct bstp_port		*bif_stp;	/* [I] STP port state */
+	struct brl_head			bif_brlin;	/* [k] input rules */
+	struct brl_head			bif_brlout;	/* [k] output rules */
+	struct ifnet			*ifp;		/* [I] net interface */
 	u_int32_t			bif_flags;	/* member flags */
 	u_int32_t			bif_protected;	/* protected domains */
 	void				*bif_dhcookie;
@@ -463,14 +468,13 @@ struct bridge_rtnode {
 #define BRIDGE_RTABLE_MASK	(BRIDGE_RTABLE_SIZE - 1)
 
 /*
+ * Software state for each bridge
+ *
  *  Locks used to protect struct members in this file:
  *	I	immutable after creation
  *	m	per-softc mutex
  *	k	kernel lock
  */
-/*
- * Software state for each bridge
- */
 struct bridge_softc {
 	struct ifnet			sc_if;	/* the interface */
 	uint32_t			sc_brtmax;	/* [m] max # addresses */
@@ -479,8 +483,8 @@ struct bridge_softc {
 	uint64_t			sc_hashkey[2];	/* [I] siphash key */
 	struct timeout			sc_brtimeout;	/* timeout state */
 	struct bstp_state		*sc_stp;	/* stp state */
-	SLIST_HEAD(, bridge_iflist)	sc_iflist;	/* [k] interface list */
-	SLIST_HEAD(, bridge_iflist)	sc_spanlist;	/* [k] span ports */
+	SMR_SLIST_HEAD(, bridge_iflist)	sc_iflist;	/* [k] interface list */
+	SMR_SLIST_HEAD(, bridge_iflist)	sc_spanlist;	/* [k] span ports */
 	struct mutex			sc_mtx;		/* mutex */
 	LIST_HEAD(, bridge_rtnode)	sc_rts[BRIDGE_RTABLE_SIZE];	/* [m] hash table */
 };