Module Name: src
Committed By: martin
Date: Wed Apr 18 14:11:43 UTC 2018
Modified Files:
src/sys/net [netbsd-8]: if_bridge.c if_bridgevar.h
src/tests/net/if_bridge [netbsd-8]: t_rtable.sh
Log Message:
Pull up following revision(s) (requested by ozaki-r in ticket #777):
tests/net/if_bridge/t_rtable.sh: revision 1.3
sys/net/if_bridge.c: revision 1.150-1.154
sys/net/if_bridgevar.h: revision 1.32
Remove obsolete NULL checks
Simplify bridge_rtnode_insert (NFC)
bridge: use pslist(9) for rtlist and rthash
The change fixes race conditions on list operations. One example is that a
reader may see invalid pointers on a looking item in a list due to lack of
membar_producer.
Add a test that checks if brconfig flush surely removes all entries
Get rid of a unnecessary semicolon
Pointed out by kamil@
Add missing PSLIST_ENTRY_INIT and PSLIST_ENTRY_DESTROY
To generate a diff of this commit:
cvs rdiff -u -r1.134.6.8 -r1.134.6.9 src/sys/net/if_bridge.c
cvs rdiff -u -r1.31 -r1.31.10.1 src/sys/net/if_bridgevar.h
cvs rdiff -u -r1.1.8.1 -r1.1.8.2 src/tests/net/if_bridge/t_rtable.sh
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/sys/net/if_bridge.c
diff -u src/sys/net/if_bridge.c:1.134.6.8 src/sys/net/if_bridge.c:1.134.6.9
--- src/sys/net/if_bridge.c:1.134.6.8 Tue Apr 10 11:48:29 2018
+++ src/sys/net/if_bridge.c Wed Apr 18 14:11:42 2018
@@ -1,4 +1,4 @@
-/* $NetBSD: if_bridge.c,v 1.134.6.8 2018/04/10 11:48:29 martin Exp $ */
+/* $NetBSD: if_bridge.c,v 1.134.6.9 2018/04/18 14:11:42 martin Exp $ */
/*
* Copyright 2001 Wasabi Systems, Inc.
@@ -80,7 +80,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: if_bridge.c,v 1.134.6.8 2018/04/10 11:48:29 martin Exp $");
+__KERNEL_RCSID(0, "$NetBSD: if_bridge.c,v 1.134.6.9 2018/04/18 14:11:42 martin Exp $");
#ifdef _KERNEL_OPT
#include "opt_bridge_ipf.h"
@@ -181,20 +181,39 @@ __CTASSERT(offsetof(struct ifbifconf, if
#define BRIDGE_RTABLE_PRUNE_PERIOD (5 * 60)
#endif
-#define BRIDGE_RT_LOCK(_sc) if ((_sc)->sc_rtlist_lock) \
- mutex_enter((_sc)->sc_rtlist_lock)
-#define BRIDGE_RT_UNLOCK(_sc) if ((_sc)->sc_rtlist_lock) \
- mutex_exit((_sc)->sc_rtlist_lock)
-#define BRIDGE_RT_LOCKED(_sc) (!(_sc)->sc_rtlist_lock || \
- mutex_owned((_sc)->sc_rtlist_lock))
+#define BRIDGE_RT_LOCK(_sc) mutex_enter((_sc)->sc_rtlist_lock)
+#define BRIDGE_RT_UNLOCK(_sc) mutex_exit((_sc)->sc_rtlist_lock)
+#define BRIDGE_RT_LOCKED(_sc) mutex_owned((_sc)->sc_rtlist_lock)
#define BRIDGE_RT_PSZ_PERFORM(_sc) \
- if ((_sc)->sc_rtlist_psz != NULL) \
- pserialize_perform((_sc)->sc_rtlist_psz);
+ pserialize_perform((_sc)->sc_rtlist_psz)
#define BRIDGE_RT_RENTER(__s) do { __s = pserialize_read_enter(); } while (0)
#define BRIDGE_RT_REXIT(__s) do { pserialize_read_exit(__s); } while (0)
+#define BRIDGE_RTLIST_READER_FOREACH(_brt, _sc) \
+ PSLIST_READER_FOREACH((_brt), &((_sc)->sc_rtlist), \
+ struct bridge_rtnode, brt_list)
+#define BRIDGE_RTLIST_WRITER_FOREACH(_brt, _sc) \
+ PSLIST_WRITER_FOREACH((_brt), &((_sc)->sc_rtlist), \
+ struct bridge_rtnode, brt_list)
+#define BRIDGE_RTLIST_WRITER_INSERT_HEAD(_sc, _brt) \
+ PSLIST_WRITER_INSERT_HEAD(&(_sc)->sc_rtlist, brt, brt_list)
+#define BRIDGE_RTLIST_WRITER_REMOVE(_brt) \
+ PSLIST_WRITER_REMOVE((_brt), brt_list)
+
+#define BRIDGE_RTHASH_READER_FOREACH(_brt, _sc, _hash) \
+ PSLIST_READER_FOREACH((_brt), &(_sc)->sc_rthash[(_hash)], \
+ struct bridge_rtnode, brt_hash)
+#define BRIDGE_RTHASH_WRITER_FOREACH(_brt, _sc, _hash) \
+ PSLIST_WRITER_FOREACH((_brt), &(_sc)->sc_rthash[(_hash)], \
+ struct bridge_rtnode, brt_hash)
+#define BRIDGE_RTHASH_WRITER_INSERT_HEAD(_sc, _hash, _brt) \
+ PSLIST_WRITER_INSERT_HEAD(&(_sc)->sc_rthash[(_hash)], brt, brt_hash)
+#define BRIDGE_RTHASH_WRITER_INSERT_AFTER(_brt, _new) \
+ PSLIST_WRITER_INSERT_AFTER((_brt), (_new), brt_hash)
+#define BRIDGE_RTHASH_WRITER_REMOVE(_brt) \
+ PSLIST_WRITER_REMOVE((_brt), brt_hash)
#ifdef NET_MPSAFE
#define DECLARE_LOCK_VARIABLE
@@ -1043,7 +1062,7 @@ bridge_ioctl_rts(struct bridge_softc *sc
BRIDGE_RT_LOCK(sc);
len = bac->ifbac_len;
- LIST_FOREACH(brt, &sc->sc_rtlist, brt_list) {
+ BRIDGE_RTLIST_WRITER_FOREACH(brt, sc) {
if (len < sizeof(bareq))
goto out;
memset(&bareq, 0, sizeof(bareq));
@@ -2013,6 +2032,8 @@ bridge_rtalloc(struct bridge_softc *sc,
brt->brt_expire = time_uptime + sc->sc_brttimeout;
brt->brt_flags = IFBAF_DYNAMIC;
memcpy(brt->brt_addr, dst, ETHER_ADDR_LEN);
+ PSLIST_ENTRY_INIT(brt, brt_list);
+ PSLIST_ENTRY_INIT(brt, brt_hash);
BRIDGE_RT_LOCK(sc);
error = bridge_rtnode_insert(sc, brt);
@@ -2109,7 +2130,7 @@ typedef bool (*bridge_iterate_cb_t)
static void
bridge_rtlist_iterate_remove(struct bridge_softc *sc, bridge_iterate_cb_t func, void *arg)
{
- struct bridge_rtnode *brt, *nbrt;
+ struct bridge_rtnode *brt;
struct bridge_rtnode **brt_list;
int i, count;
@@ -2128,7 +2149,12 @@ retry:
}
i = 0;
- LIST_FOREACH_SAFE(brt, &sc->sc_rtlist, brt_list, nbrt) {
+ /*
+ * We don't need to use a _SAFE variant here because we know
+ * that a removed item keeps its next pointer as-is thanks to
+ * pslist(9) and isn't freed in the loop.
+ */
+ BRIDGE_RTLIST_WRITER_FOREACH(brt, sc) {
bool need_break = false;
if (func(sc, brt, &need_break, arg)) {
bridge_rtnode_remove(sc, brt);
@@ -2302,7 +2328,7 @@ bridge_rtdelete(struct bridge_softc *sc,
/* XXX pserialize_perform for each entry is slow */
again:
BRIDGE_RT_LOCK(sc);
- LIST_FOREACH(brt, &sc->sc_rtlist, brt_list) {
+ BRIDGE_RTLIST_WRITER_FOREACH(brt, sc) {
if (brt->brt_ifp == ifp)
break;
}
@@ -2333,11 +2359,11 @@ bridge_rtable_init(struct bridge_softc *
KM_SLEEP);
for (i = 0; i < BRIDGE_RTHASH_SIZE; i++)
- LIST_INIT(&sc->sc_rthash[i]);
+ PSLIST_INIT(&sc->sc_rthash[i]);
sc->sc_rthash_key = cprng_fast32();
- LIST_INIT(&sc->sc_rtlist);
+ PSLIST_INIT(&sc->sc_rtlist);
sc->sc_rtlist_psz = pserialize_create();
sc->sc_rtlist_lock = mutex_obj_alloc(MUTEX_DEFAULT, IPL_SOFTNET);
@@ -2353,10 +2379,8 @@ bridge_rtable_fini(struct bridge_softc *
{
kmem_free(sc->sc_rthash, sizeof(*sc->sc_rthash) * BRIDGE_RTHASH_SIZE);
- if (sc->sc_rtlist_lock)
- mutex_obj_free(sc->sc_rtlist_lock);
- if (sc->sc_rtlist_psz)
- pserialize_destroy(sc->sc_rtlist_psz);
+ mutex_obj_free(sc->sc_rtlist_lock);
+ pserialize_destroy(sc->sc_rtlist_psz);
}
/*
@@ -2408,7 +2432,7 @@ bridge_rtnode_lookup(struct bridge_softc
int dir;
hash = bridge_rthash(sc, addr);
- LIST_FOREACH(brt, &sc->sc_rthash[hash], brt_hash) {
+ BRIDGE_RTHASH_READER_FOREACH(brt, sc, hash) {
dir = memcmp(addr, brt->brt_addr, ETHER_ADDR_LEN);
if (dir == 0)
return brt;
@@ -2428,41 +2452,26 @@ bridge_rtnode_lookup(struct bridge_softc
static int
bridge_rtnode_insert(struct bridge_softc *sc, struct bridge_rtnode *brt)
{
- struct bridge_rtnode *lbrt;
+ struct bridge_rtnode *lbrt, *prev = NULL;
uint32_t hash;
- int dir;
KASSERT(BRIDGE_RT_LOCKED(sc));
hash = bridge_rthash(sc, brt->brt_addr);
-
- lbrt = LIST_FIRST(&sc->sc_rthash[hash]);
- if (lbrt == NULL) {
- LIST_INSERT_HEAD(&sc->sc_rthash[hash], brt, brt_hash);
- goto out;
- }
-
- do {
- dir = memcmp(brt->brt_addr, lbrt->brt_addr, ETHER_ADDR_LEN);
+ BRIDGE_RTHASH_WRITER_FOREACH(lbrt, sc, hash) {
+ int dir = memcmp(brt->brt_addr, lbrt->brt_addr, ETHER_ADDR_LEN);
if (dir == 0)
return EEXIST;
- if (dir > 0) {
- LIST_INSERT_BEFORE(lbrt, brt, brt_hash);
- goto out;
- }
- if (LIST_NEXT(lbrt, brt_hash) == NULL) {
- LIST_INSERT_AFTER(lbrt, brt, brt_hash);
- goto out;
- }
- lbrt = LIST_NEXT(lbrt, brt_hash);
- } while (lbrt != NULL);
-
-#ifdef DIAGNOSTIC
- panic("bridge_rtnode_insert: impossible");
-#endif
+ if (dir > 0)
+ break;
+ prev = lbrt;
+ }
+ if (prev == NULL)
+ BRIDGE_RTHASH_WRITER_INSERT_HEAD(sc, hash, brt);
+ else
+ BRIDGE_RTHASH_WRITER_INSERT_AFTER(prev, brt);
- out:
- LIST_INSERT_HEAD(&sc->sc_rtlist, brt, brt_list);
+ BRIDGE_RTLIST_WRITER_INSERT_HEAD(sc, brt);
sc->sc_brtcnt++;
return 0;
@@ -2479,8 +2488,8 @@ bridge_rtnode_remove(struct bridge_softc
KASSERT(BRIDGE_RT_LOCKED(sc));
- LIST_REMOVE(brt, brt_hash);
- LIST_REMOVE(brt, brt_list);
+ BRIDGE_RTHASH_WRITER_REMOVE(brt);
+ BRIDGE_RTLIST_WRITER_REMOVE(brt);
sc->sc_brtcnt--;
}
@@ -2493,6 +2502,8 @@ static void
bridge_rtnode_destroy(struct bridge_rtnode *brt)
{
+ PSLIST_ENTRY_DESTROY(brt, brt_list);
+ PSLIST_ENTRY_DESTROY(brt, brt_hash);
pool_put(&bridge_rtnode_pool, brt);
}
Index: src/sys/net/if_bridgevar.h
diff -u src/sys/net/if_bridgevar.h:1.31 src/sys/net/if_bridgevar.h:1.31.10.1
--- src/sys/net/if_bridgevar.h:1.31 Thu Apr 28 00:16:56 2016
+++ src/sys/net/if_bridgevar.h Wed Apr 18 14:11:42 2018
@@ -1,4 +1,4 @@
-/* $NetBSD: if_bridgevar.h,v 1.31 2016/04/28 00:16:56 ozaki-r Exp $ */
+/* $NetBSD: if_bridgevar.h,v 1.31.10.1 2018/04/18 14:11:42 martin Exp $ */
/*
* Copyright 2001 Wasabi Systems, Inc.
@@ -275,8 +275,8 @@ struct bridge_iflist {
* Bridge route node.
*/
struct bridge_rtnode {
- LIST_ENTRY(bridge_rtnode) brt_hash; /* hash table linkage */
- LIST_ENTRY(bridge_rtnode) brt_list; /* list linkage */
+ struct pslist_entry brt_hash; /* hash table linkage */
+ struct pslist_entry brt_list; /* list linkage */
struct ifnet *brt_ifp; /* destination if */
time_t brt_expire; /* expiration time */
uint8_t brt_flags; /* address flags */
@@ -319,8 +319,8 @@ struct bridge_softc {
callout_t sc_brcallout; /* bridge callout */
callout_t sc_bstpcallout; /* STP callout */
struct bridge_iflist_psref sc_iflist_psref;
- LIST_HEAD(, bridge_rtnode) *sc_rthash; /* our forwarding table */
- LIST_HEAD(, bridge_rtnode) sc_rtlist; /* list version of above */
+ struct pslist_head *sc_rthash; /* our forwarding table */
+ struct pslist_head sc_rtlist; /* list version of above */
kmutex_t *sc_rtlist_lock;
pserialize_t sc_rtlist_psz;
struct workqueue *sc_rtage_wq;
Index: src/tests/net/if_bridge/t_rtable.sh
diff -u src/tests/net/if_bridge/t_rtable.sh:1.1.8.1 src/tests/net/if_bridge/t_rtable.sh:1.1.8.2
--- src/tests/net/if_bridge/t_rtable.sh:1.1.8.1 Tue Apr 10 11:48:28 2018
+++ src/tests/net/if_bridge/t_rtable.sh Wed Apr 18 14:11:42 2018
@@ -1,4 +1,4 @@
-# $NetBSD: t_rtable.sh,v 1.1.8.1 2018/04/10 11:48:28 martin Exp $
+# $NetBSD: t_rtable.sh,v 1.1.8.2 2018/04/18 14:11:42 martin Exp $
#
# Copyright (c) 2017 Internet Initiative Japan Inc.
# All rights reserved.
@@ -169,6 +169,7 @@ bridge_rtable_flush_head()
bridge_rtable_flush_body()
{
local addr1= addr3=
+ local n=
setup
setup_bridge
@@ -195,6 +196,34 @@ bridge_rtable_flush_body()
atf_check -s exit:0 -o not-match:"$addr3 shmif1" /sbin/brconfig bridge0
unset LD_PRELOAD
+ # Add extra interfaces and addresses
+ export RUMP_SERVER=$SOCK1
+ rump_server_add_iface $SOCK1 shmif1 bus1
+ atf_check -s exit:0 rump.ifconfig shmif1 10.0.0.11/24
+ atf_check -s exit:0 rump.ifconfig -w 10
+
+ export RUMP_SERVER=$SOCK3
+ rump_server_add_iface $SOCK3 shmif1 bus2
+ atf_check -s exit:0 rump.ifconfig shmif1 10.0.0.12/24
+ atf_check -s exit:0 rump.ifconfig -w 10
+
+ # Let cache entries
+ export RUMP_SERVER=$SOCK1
+ atf_check -s exit:0 -o ignore rump.ping -n -w $TIMEOUT -c 1 10.0.0.12
+ export RUMP_SERVER=$SOCK3
+ atf_check -s exit:0 -o ignore rump.ping -n -w $TIMEOUT -c 1 10.0.0.11
+
+ export RUMP_SERVER=$SOCK2
+ export LD_PRELOAD=/usr/lib/librumphijack.so
+ $DEBUG && /sbin/brconfig bridge0
+ n=$(get_number_of_caches)
+ atf_check_equal $n 4
+
+ atf_check -s exit:0 -o ignore /sbin/brconfig bridge0 flush
+ n=$(get_number_of_caches)
+ atf_check_equal $n 0
+ unset LD_PRELOAD
+
rump_server_destroy_ifaces
}
