Nikolay Aleksandrov
2017-Feb-13 13:59 UTC
[Bridge] [PATCH net-next 0/3] bridge: minor fdb cleanup
Hi, These patches aim to simplify the bridge fdb API a little by removing some redundant functions and converting them into wrappers of a single function. Also add proper lock checking to avoid future mistakes for the search functions. Thanks, Nik Nikolay Aleksandrov (3): bridge: fdb: converge fdb searching functions into one bridge: fdb: add proper lock checks in searching functions bridge: fdb: converge fdb_delete_by functions into one net/bridge/br_device.c | 2 +- net/bridge/br_fdb.c | 177 ++++++++++++++++++------------------------------ net/bridge/br_input.c | 4 +- net/bridge/br_private.h | 14 +++- 4 files changed, 81 insertions(+), 116 deletions(-) -- 2.1.4
Nikolay Aleksandrov
2017-Feb-13 13:59 UTC
[Bridge] [PATCH net-next 1/3] bridge: fdb: converge fdb searching functions into one
Before this patch we had 3 different fdb searching functions which was
confusing. This patch reduces all of them to one - fdb_find_rcu(), and
two flavors: br_fdb_find() which requires hash_lock and br_fdb_find_rcu
which requires RCU. This makes it clear what needs to be used, we also
remove two abusers of __br_fdb_get which called it under hash_lock and
replace them with br_fdb_find().
Signed-off-by: Nikolay Aleksandrov <nikolay at cumulusnetworks.com>
---
net/bridge/br_device.c | 2 +-
net/bridge/br_fdb.c | 113 ++++++++++++++++++++----------------------------
net/bridge/br_input.c | 4 +-
net/bridge/br_private.h | 5 ++-
4 files changed, 54 insertions(+), 70 deletions(-)
diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index d208ee9ab60a..ea71513fca21 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -79,7 +79,7 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device
*dev)
br_multicast_flood(mdst, skb, false, true);
else
br_flood(br, skb, BR_PKT_MULTICAST, false, true);
- } else if ((dst = __br_fdb_get(br, dest, vid)) != NULL) {
+ } else if ((dst = br_fdb_find_rcu(br, dest, vid)) != NULL) {
br_forward(dst->dst, skb, false, true);
} else {
br_flood(br, skb, BR_PKT_UNICAST, false, true);
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 5693168e88b6..04a53a0b733d 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -28,9 +28,6 @@
#include "br_private.h"
static struct kmem_cache *br_fdb_cache __read_mostly;
-static struct net_bridge_fdb_entry *fdb_find(struct hlist_head *head,
- const unsigned char *addr,
- __u16 vid);
static int fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
const unsigned char *addr, u16 vid);
static void fdb_notify(struct net_bridge *br,
@@ -86,6 +83,43 @@ static void fdb_rcu_free(struct rcu_head *head)
kmem_cache_free(br_fdb_cache, ent);
}
+static struct net_bridge_fdb_entry *fdb_find_rcu(struct hlist_head *head,
+ const unsigned char *addr,
+ __u16 vid)
+{
+ struct net_bridge_fdb_entry *f;
+
+ hlist_for_each_entry_rcu(f, head, hlist)
+ if (ether_addr_equal(f->addr.addr, addr) && f->vlan_id == vid)
+ break;
+
+ return f;
+}
+
+/* requires bridge hash_lock */
+static struct net_bridge_fdb_entry *br_fdb_find(struct net_bridge *br,
+ const unsigned char *addr,
+ __u16 vid)
+{
+ struct hlist_head *head = &br->hash[br_mac_hash(addr, vid)];
+ struct net_bridge_fdb_entry *fdb;
+
+ rcu_read_lock();
+ fdb = fdb_find_rcu(head, addr, vid);
+ rcu_read_unlock();
+
+ return fdb;
+}
+
+struct net_bridge_fdb_entry *br_fdb_find_rcu(struct net_bridge *br,
+ const unsigned char *addr,
+ __u16 vid)
+{
+ struct hlist_head *head = &br->hash[br_mac_hash(addr, vid)];
+
+ return fdb_find_rcu(head, addr, vid);
+}
+
/* When a static FDB entry is added, the mac address from the entry is
* added to the bridge private HW address list and all required ports
* are then updated with the new information.
@@ -198,11 +232,10 @@ void br_fdb_find_delete_local(struct net_bridge *br,
const struct net_bridge_port *p,
const unsigned char *addr, u16 vid)
{
- struct hlist_head *head = &br->hash[br_mac_hash(addr, vid)];
struct net_bridge_fdb_entry *f;
spin_lock_bh(&br->hash_lock);
- f = fdb_find(head, addr, vid);
+ f = br_fdb_find(br, addr, vid);
if (f && f->is_local && !f->added_by_user &&
f->dst == p)
fdb_delete_local(br, p, f);
spin_unlock_bh(&br->hash_lock);
@@ -266,7 +299,7 @@ void br_fdb_change_mac_address(struct net_bridge *br, const
u8 *newaddr)
spin_lock_bh(&br->hash_lock);
/* If old entry was unassociated with any port, then delete it. */
- f = __br_fdb_get(br, br->dev->dev_addr, 0);
+ f = br_fdb_find(br, br->dev->dev_addr, 0);
if (f && f->is_local && !f->dst &&
!f->added_by_user)
fdb_delete_local(br, NULL, f);
@@ -281,7 +314,7 @@ void br_fdb_change_mac_address(struct net_bridge *br, const
u8 *newaddr)
list_for_each_entry(v, &vg->vlan_list, vlist) {
if (!br_vlan_should_use(v))
continue;
- f = __br_fdb_get(br, br->dev->dev_addr, v->vid);
+ f = br_fdb_find(br, br->dev->dev_addr, v->vid);
if (f && f->is_local && !f->dst &&
!f->added_by_user)
fdb_delete_local(br, NULL, f);
fdb_insert(br, NULL, newaddr, v->vid);
@@ -380,24 +413,6 @@ void br_fdb_delete_by_port(struct net_bridge *br,
spin_unlock_bh(&br->hash_lock);
}
-/* No locking or refcounting, assumes caller has rcu_read_lock */
-struct net_bridge_fdb_entry *__br_fdb_get(struct net_bridge *br,
- const unsigned char *addr,
- __u16 vid)
-{
- struct net_bridge_fdb_entry *fdb;
-
- hlist_for_each_entry_rcu(fdb,
- &br->hash[br_mac_hash(addr, vid)], hlist) {
- if (ether_addr_equal(fdb->addr.addr, addr) &&
- fdb->vlan_id == vid) {
- return fdb;
- }
- }
-
- return NULL;
-}
-
#if IS_ENABLED(CONFIG_ATM_LANE)
/* Interface used by ATM LANE hook to test
* if an addr is on some other bridge port */
@@ -412,7 +427,7 @@ int br_fdb_test_addr(struct net_device *dev, unsigned char
*addr)
if (!port)
ret = 0;
else {
- fdb = __br_fdb_get(port->br, addr, 0);
+ fdb = br_fdb_find_rcu(port->br, addr, 0);
ret = fdb && fdb->dst && fdb->dst->dev != dev
&&
fdb->dst->state == BR_STATE_FORWARDING;
}
@@ -474,34 +489,6 @@ int br_fdb_fillbuf(struct net_bridge *br, void *buf,
return num;
}
-static struct net_bridge_fdb_entry *fdb_find(struct hlist_head *head,
- const unsigned char *addr,
- __u16 vid)
-{
- struct net_bridge_fdb_entry *fdb;
-
- hlist_for_each_entry(fdb, head, hlist) {
- if (ether_addr_equal(fdb->addr.addr, addr) &&
- fdb->vlan_id == vid)
- return fdb;
- }
- return NULL;
-}
-
-static struct net_bridge_fdb_entry *fdb_find_rcu(struct hlist_head *head,
- const unsigned char *addr,
- __u16 vid)
-{
- struct net_bridge_fdb_entry *fdb;
-
- hlist_for_each_entry_rcu(fdb, head, hlist) {
- if (ether_addr_equal(fdb->addr.addr, addr) &&
- fdb->vlan_id == vid)
- return fdb;
- }
- return NULL;
-}
-
static struct net_bridge_fdb_entry *fdb_create(struct hlist_head *head,
struct net_bridge_port *source,
const unsigned char *addr,
@@ -535,7 +522,7 @@ static int fdb_insert(struct net_bridge *br, struct
net_bridge_port *source,
if (!is_valid_ether_addr(addr))
return -EINVAL;
- fdb = fdb_find(head, addr, vid);
+ fdb = br_fdb_find(br, addr, vid);
if (fdb) {
/* it is okay to have multiple ports with same
* address, just use the first one.
@@ -608,7 +595,7 @@ void br_fdb_update(struct net_bridge *br, struct
net_bridge_port *source,
}
} else {
spin_lock(&br->hash_lock);
- if (likely(!fdb_find(head, addr, vid))) {
+ if (likely(!fdb_find_rcu(head, addr, vid))) {
fdb = fdb_create(head, source, addr, vid, 0, 0);
if (fdb) {
if (unlikely(added_by_user))
@@ -792,7 +779,7 @@ static int fdb_add_entry(struct net_bridge *br, struct
net_bridge_port *source,
return -EINVAL;
}
- fdb = fdb_find(head, addr, vid);
+ fdb = br_fdb_find(br, addr, vid);
if (fdb == NULL) {
if (!(flags & NLM_F_CREATE))
return -ENOENT;
@@ -942,10 +929,9 @@ int br_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
static int fdb_delete_by_addr(struct net_bridge *br, const u8 *addr,
u16 vid)
{
- struct hlist_head *head = &br->hash[br_mac_hash(addr, vid)];
struct net_bridge_fdb_entry *fdb;
- fdb = fdb_find(head, addr, vid);
+ fdb = br_fdb_find(br, addr, vid);
if (!fdb)
return -ENOENT;
@@ -969,10 +955,9 @@ static int fdb_delete_by_addr_and_port(struct
net_bridge_port *p,
const u8 *addr, u16 vlan)
{
struct net_bridge *br = p->br;
- struct hlist_head *head = &br->hash[br_mac_hash(addr, vlan)];
struct net_bridge_fdb_entry *fdb;
- fdb = fdb_find(head, addr, vlan);
+ fdb = br_fdb_find(br, addr, vlan);
if (!fdb || fdb->dst != p)
return -ENOENT;
@@ -1117,7 +1102,7 @@ int br_fdb_external_learn_add(struct net_bridge *br,
struct net_bridge_port *p,
spin_lock_bh(&br->hash_lock);
head = &br->hash[br_mac_hash(addr, vid)];
- fdb = fdb_find(head, addr, vid);
+ fdb = br_fdb_find(br, addr, vid);
if (!fdb) {
fdb = fdb_create(head, p, addr, vid, 0, 0);
if (!fdb) {
@@ -1145,15 +1130,13 @@ int br_fdb_external_learn_add(struct net_bridge *br,
struct net_bridge_port *p,
int br_fdb_external_learn_del(struct net_bridge *br, struct net_bridge_port *p,
const unsigned char *addr, u16 vid)
{
- struct hlist_head *head;
struct net_bridge_fdb_entry *fdb;
int err = 0;
ASSERT_RTNL();
spin_lock_bh(&br->hash_lock);
- head = &br->hash[br_mac_hash(addr, vid)];
- fdb = fdb_find(head, addr, vid);
+ fdb = br_fdb_find(br, addr, vid);
if (fdb && fdb->added_by_external_learn)
fdb_delete(br, fdb);
else
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 4615a9b3e26c..236f34244dbe 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -114,7 +114,7 @@ static void br_do_proxy_arp(struct sk_buff *skb, struct
net_bridge *br,
return;
}
- f = __br_fdb_get(br, n->ha, vid);
+ f = br_fdb_find_rcu(br, n->ha, vid);
if (f && ((p->flags & BR_PROXYARP) ||
(f->dst && (f->dst->flags & BR_PROXYARP_WIFI)))) {
arp_send(ARPOP_REPLY, ETH_P_ARP, sip, skb->dev, tip,
@@ -189,7 +189,7 @@ int br_handle_frame_finish(struct net *net, struct sock *sk,
struct sk_buff *skb
}
break;
case BR_PKT_UNICAST:
- dst = __br_fdb_get(br, dest, vid);
+ dst = br_fdb_find_rcu(br, dest, vid);
default:
break;
}
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 1cbbf63a5ef7..61368186edea 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -507,8 +507,9 @@ void br_fdb_change_mac_address(struct net_bridge *br, const
u8 *newaddr);
void br_fdb_cleanup(struct work_struct *work);
void br_fdb_delete_by_port(struct net_bridge *br,
const struct net_bridge_port *p, u16 vid, int do_all);
-struct net_bridge_fdb_entry *__br_fdb_get(struct net_bridge *br,
- const unsigned char *addr, __u16 vid);
+struct net_bridge_fdb_entry *br_fdb_find_rcu(struct net_bridge *br,
+ const unsigned char *addr,
+ __u16 vid);
int br_fdb_test_addr(struct net_device *dev, unsigned char *addr);
int br_fdb_fillbuf(struct net_bridge *br, void *buf, unsigned long count,
unsigned long off);
--
2.1.4
Nikolay Aleksandrov
2017-Feb-13 13:59 UTC
[Bridge] [PATCH net-next 2/3] bridge: fdb: add proper lock checks in searching functions
In order to avoid new errors add checks to br_fdb_find and fdb_find_rcu
functions. The first requires hash_lock, the second obviously RCU.
Signed-off-by: Nikolay Aleksandrov <nikolay at cumulusnetworks.com>
---
net/bridge/br_fdb.c | 4 ++++
net/bridge/br_private.h | 9 +++++++++
2 files changed, 13 insertions(+)
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 04a53a0b733d..da5ed7d0a3c8 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -89,6 +89,8 @@ static struct net_bridge_fdb_entry *fdb_find_rcu(struct
hlist_head *head,
{
struct net_bridge_fdb_entry *f;
+ WARN_ON_ONCE(!rcu_read_lock_held());
+
hlist_for_each_entry_rcu(f, head, hlist)
if (ether_addr_equal(f->addr.addr, addr) && f->vlan_id == vid)
break;
@@ -104,6 +106,8 @@ static struct net_bridge_fdb_entry *br_fdb_find(struct
net_bridge *br,
struct hlist_head *head = &br->hash[br_mac_hash(addr, vid)];
struct net_bridge_fdb_entry *fdb;
+ WARN_ON_ONCE(!br_hash_lock_held(br));
+
rcu_read_lock();
fdb = fdb_find_rcu(head, addr, vid);
rcu_read_unlock();
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 61368186edea..2288fca7756c 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -531,6 +531,15 @@ int br_fdb_external_learn_add(struct net_bridge *br, struct
net_bridge_port *p,
int br_fdb_external_learn_del(struct net_bridge *br, struct net_bridge_port *p,
const unsigned char *addr, u16 vid);
+static inline bool br_hash_lock_held(struct net_bridge *br)
+{
+#ifdef CONFIG_LOCKDEP
+ return lockdep_is_held(&br->hash_lock);
+#else
+ return true;
+#endif
+}
+
/* br_forward.c */
enum br_pkt_type {
BR_PKT_UNICAST,
--
2.1.4
Nikolay Aleksandrov
2017-Feb-13 13:59 UTC
[Bridge] [PATCH net-next 3/3] bridge: fdb: converge fdb_delete_by functions into one
We can simplify the logic of entries pointing to the bridge by
converging the fdb_delete_by functions, this would allow us to use the
same function for both cases since the fdb's dst is set to NULL if it is
pointing to the bridge thus we can always check for a port match.
Signed-off-by: Nikolay Aleksandrov <nikolay at cumulusnetworks.com>
---
net/bridge/br_fdb.c | 62 +++++++++++++----------------------------------------
1 file changed, 15 insertions(+), 47 deletions(-)
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index da5ed7d0a3c8..4ac11574117c 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -930,35 +930,10 @@ int br_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
return err;
}
-static int fdb_delete_by_addr(struct net_bridge *br, const u8 *addr,
- u16 vid)
-{
- struct net_bridge_fdb_entry *fdb;
-
- fdb = br_fdb_find(br, addr, vid);
- if (!fdb)
- return -ENOENT;
-
- fdb_delete(br, fdb);
- return 0;
-}
-
-static int __br_fdb_delete_by_addr(struct net_bridge *br,
- const unsigned char *addr, u16 vid)
-{
- int err;
-
- spin_lock_bh(&br->hash_lock);
- err = fdb_delete_by_addr(br, addr, vid);
- spin_unlock_bh(&br->hash_lock);
-
- return err;
-}
-
-static int fdb_delete_by_addr_and_port(struct net_bridge_port *p,
+static int fdb_delete_by_addr_and_port(struct net_bridge *br,
+ const struct net_bridge_port *p,
const u8 *addr, u16 vlan)
{
- struct net_bridge *br = p->br;
struct net_bridge_fdb_entry *fdb;
fdb = br_fdb_find(br, addr, vlan);
@@ -966,17 +941,19 @@ static int fdb_delete_by_addr_and_port(struct
net_bridge_port *p,
return -ENOENT;
fdb_delete(br, fdb);
+
return 0;
}
-static int __br_fdb_delete(struct net_bridge_port *p,
+static int __br_fdb_delete(struct net_bridge *br,
+ const struct net_bridge_port *p,
const unsigned char *addr, u16 vid)
{
int err;
- spin_lock_bh(&p->br->hash_lock);
- err = fdb_delete_by_addr_and_port(p, addr, vid);
- spin_unlock_bh(&p->br->hash_lock);
+ spin_lock_bh(&br->hash_lock);
+ err = fdb_delete_by_addr_and_port(br, p, addr, vid);
+ spin_unlock_bh(&br->hash_lock);
return err;
}
@@ -989,7 +966,7 @@ int br_fdb_delete(struct ndmsg *ndm, struct nlattr *tb[],
struct net_bridge_vlan_group *vg;
struct net_bridge_port *p = NULL;
struct net_bridge_vlan *v;
- struct net_bridge *br = NULL;
+ struct net_bridge *br;
int err;
if (dev->priv_flags & IFF_EBRIDGE) {
@@ -1003,6 +980,7 @@ int br_fdb_delete(struct ndmsg *ndm, struct nlattr *tb[],
return -EINVAL;
}
vg = nbp_vlan_group(p);
+ br = p->br;
}
if (vid) {
@@ -1012,30 +990,20 @@ int br_fdb_delete(struct ndmsg *ndm, struct nlattr *tb[],
return -EINVAL;
}
- if (dev->priv_flags & IFF_EBRIDGE)
- err = __br_fdb_delete_by_addr(br, addr, vid);
- else
- err = __br_fdb_delete(p, addr, vid);
+ err = __br_fdb_delete(br, p, addr, vid);
} else {
err = -ENOENT;
- if (dev->priv_flags & IFF_EBRIDGE)
- err = __br_fdb_delete_by_addr(br, addr, 0);
- else
- err &= __br_fdb_delete(p, addr, 0);
-
+ err &= __br_fdb_delete(br, p, addr, 0);
if (!vg || !vg->num_vlans)
- goto out;
+ return err;
list_for_each_entry(v, &vg->vlan_list, vlist) {
if (!br_vlan_should_use(v))
continue;
- if (dev->priv_flags & IFF_EBRIDGE)
- err = __br_fdb_delete_by_addr(br, addr, v->vid);
- else
- err &= __br_fdb_delete(p, addr, v->vid);
+ err &= __br_fdb_delete(br, p, addr, v->vid);
}
}
-out:
+
return err;
}
--
2.1.4
David Miller
2017-Feb-14 17:41 UTC
[Bridge] [PATCH net-next 0/3] bridge: minor fdb cleanup
From: Nikolay Aleksandrov <nikolay at cumulusnetworks.com> Date: Mon, 13 Feb 2017 14:59:08 +0100> These patches aim to simplify the bridge fdb API a little by removing some > redundant functions and converting them into wrappers of a single function. > Also add proper lock checking to avoid future mistakes for the search > functions.This looks fine, series applied. Thanks.