From 586c2b573ee4c2c4ba03e16318a16614ebf876f8 Mon Sep 17 00:00:00 2001 From: Nikolay Aleksandrov Date: Fri, 2 Oct 2015 15:05:10 +0200 Subject: [PATCH 1/4] bridge: vlan: use rcu list for the ordered vlan list When I did the conversion to rhashtable I missed the required locking of one important user of the vlan list - br_get_link_af_size_filtered() which is called: br_ifinfo_notify() -> br_nlmsg_size() -> br_get_link_af_size_filtered() and the notifications can be sent without holding rtnl. Before this conversion the function relied on using rcu and since we already use rcu to destroy the vlans, we can simply migrate the list to use the rcu helpers. Signed-off-by: Nikolay Aleksandrov Signed-off-by: David S. Miller --- net/bridge/br_netlink.c | 10 ++++++++-- net/bridge/br_vlan.c | 4 ++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c index c64dcad11662..c3186198d46d 100644 --- a/net/bridge/br_netlink.c +++ b/net/bridge/br_netlink.c @@ -34,7 +34,7 @@ static int __get_num_vlan_infos(struct net_bridge_vlan_group *vg, pvid = br_get_pvid(vg); /* Count number of vlan infos */ - list_for_each_entry(v, &vg->vlan_list, vlist) { + list_for_each_entry_rcu(v, &vg->vlan_list, vlist) { flags = 0; /* only a context, bridge vlan not activated */ if (!br_vlan_should_use(v)) @@ -76,13 +76,19 @@ static int __get_num_vlan_infos(struct net_bridge_vlan_group *vg, static int br_get_num_vlan_infos(struct net_bridge_vlan_group *vg, u32 filter_mask) { + int num_vlans; + if (!vg) return 0; if (filter_mask & RTEXT_FILTER_BRVLAN) return vg->num_vlans; - return __get_num_vlan_infos(vg, filter_mask); + rcu_read_lock(); + num_vlans = __get_num_vlan_infos(vg, filter_mask); + rcu_read_unlock(); + + return num_vlans; } static size_t br_get_link_af_size_filtered(const struct net_device *dev, diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c index 1a79e199ca3b..d97a55e97830 100644 --- a/net/bridge/br_vlan.c +++ b/net/bridge/br_vlan.c @@ -111,12 +111,12 @@ static void __vlan_add_list(struct net_bridge_vlan *v) else break; } - list_add(&v->vlist, hpos); + list_add_rcu(&v->vlist, hpos); } static void __vlan_del_list(struct net_bridge_vlan *v) { - list_del(&v->vlist); + list_del_rcu(&v->vlist); } static int __vlan_vid_del(struct net_device *dev, struct net_bridge *br, From f8ed289fab843fbc9251aa2f5c3d416f09b5fc7e Mon Sep 17 00:00:00 2001 From: Nikolay Aleksandrov Date: Fri, 2 Oct 2015 15:05:11 +0200 Subject: [PATCH 2/4] bridge: vlan: use br_vlan_(get|put)_master to deal with refcounts Introduce br_vlan_(get|put)_master which take a reference (or create the master vlan first if it didn't exist) and drop a reference respectively. Signed-off-by: Nikolay Aleksandrov Signed-off-by: David S. Miller --- net/bridge/br_vlan.c | 56 ++++++++++++++++++++++++++++++-------------- 1 file changed, 39 insertions(+), 17 deletions(-) diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c index d97a55e97830..6e41fba47e97 100644 --- a/net/bridge/br_vlan.c +++ b/net/bridge/br_vlan.c @@ -146,6 +146,40 @@ static int __vlan_vid_del(struct net_device *dev, struct net_bridge *br, return err; } +/* Returns a master vlan, if it didn't exist it gets created. In all cases a + * a reference is taken to the master vlan before returning. + */ +static struct net_bridge_vlan *br_vlan_get_master(struct net_bridge *br, u16 vid) +{ + struct net_bridge_vlan *masterv; + + masterv = br_vlan_find(br->vlgrp, vid); + if (!masterv) { + /* missing global ctx, create it now */ + if (br_vlan_add(br, vid, 0)) + return NULL; + masterv = br_vlan_find(br->vlgrp, vid); + if (WARN_ON(!masterv)) + return NULL; + } + atomic_inc(&masterv->refcnt); + + return masterv; +} + +static void br_vlan_put_master(struct net_bridge_vlan *masterv) +{ + if (!br_vlan_is_master(masterv)) + return; + + if (atomic_dec_and_test(&masterv->refcnt)) { + rhashtable_remove_fast(&masterv->br->vlgrp->vlan_hash, + &masterv->vnode, br_vlan_rht_params); + __vlan_del_list(masterv); + kfree_rcu(masterv, rcu); + } +} + /* This is the shared VLAN add function which works for both ports and bridge * devices. There are four possible calls to this function in terms of the * vlan entry type: @@ -196,16 +230,9 @@ static int __vlan_add(struct net_bridge_vlan *v, u16 flags) goto out_filt; } - masterv = br_vlan_find(br->vlgrp, v->vid); - if (!masterv) { - /* missing global ctx, create it now */ - err = br_vlan_add(br, v->vid, 0); - if (err) - goto out_filt; - masterv = br_vlan_find(br->vlgrp, v->vid); - WARN_ON(!masterv); - } - atomic_inc(&masterv->refcnt); + masterv = br_vlan_get_master(br, v->vid); + if (!masterv) + goto out_filt; v->brvlan = masterv; } @@ -240,7 +267,7 @@ static int __vlan_add(struct net_bridge_vlan *v, u16 flags) if (p) { __vlan_vid_del(dev, br, v->vid); if (masterv) { - atomic_dec(&masterv->refcnt); + br_vlan_put_master(masterv); v->brvlan = NULL; } } @@ -289,12 +316,7 @@ static int __vlan_del(struct net_bridge_vlan *v) kfree_rcu(v, rcu); } - if (atomic_dec_and_test(&masterv->refcnt)) { - rhashtable_remove_fast(&masterv->br->vlgrp->vlan_hash, - &masterv->vnode, br_vlan_rht_params); - __vlan_del_list(masterv); - kfree_rcu(masterv, rcu); - } + br_vlan_put_master(masterv); out: return err; } From 2ffdf508d278d48ccb928238846df352db21f4eb Mon Sep 17 00:00:00 2001 From: Nikolay Aleksandrov Date: Fri, 2 Oct 2015 15:05:12 +0200 Subject: [PATCH 3/4] bridge: vlan: drop master_flags from __vlan_add There's only one user now and we can include the flag directly. Signed-off-by: Nikolay Aleksandrov Signed-off-by: David S. Miller --- net/bridge/br_vlan.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c index 6e41fba47e97..2c1fdf94badc 100644 --- a/net/bridge/br_vlan.c +++ b/net/bridge/br_vlan.c @@ -212,8 +212,6 @@ static int __vlan_add(struct net_bridge_vlan *v, u16 flags) } if (p) { - u16 master_flags = flags; - /* Add VLAN to the device filter if it is supported. * This ensures tagged traffic enters the bridge when * promiscuous mode is disabled by br_manage_promisc(). @@ -224,8 +222,8 @@ static int __vlan_add(struct net_bridge_vlan *v, u16 flags) /* need to work on the master vlan too */ if (flags & BRIDGE_VLAN_INFO_MASTER) { - master_flags |= BRIDGE_VLAN_INFO_BRENTRY; - err = br_vlan_add(br, v->vid, master_flags); + err = br_vlan_add(br, v->vid, flags | + BRIDGE_VLAN_INFO_BRENTRY); if (err) goto out_filt; } From 6be144f62f64c8a67e11b2f8b86c7bf390b87411 Mon Sep 17 00:00:00 2001 From: Nikolay Aleksandrov Date: Fri, 2 Oct 2015 15:05:13 +0200 Subject: [PATCH 4/4] bridge: vlan: use br_vlan_should_use to simplify __vlan_add/del The checks that lead to num_vlans change are always what br_vlan_should_use checks for, namely if the vlan is only a context or not and depending on that it's either not counted or counted as a real/used vlan respectively. Also give better explanation in br_vlan_should_use's comment. Signed-off-by: Nikolay Aleksandrov Signed-off-by: David S. Miller --- net/bridge/br_private.h | 2 +- net/bridge/br_vlan.c | 36 ++++++++++++++---------------------- 2 files changed, 15 insertions(+), 23 deletions(-) diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h index 4ed8308db66e..1ff6a0faef3f 100644 --- a/net/bridge/br_private.h +++ b/net/bridge/br_private.h @@ -400,7 +400,7 @@ static inline bool br_vlan_is_brentry(const struct net_bridge_vlan *v) return v->flags & BRIDGE_VLAN_INFO_BRENTRY; } -/* check if we should use the vlan entry is usable */ +/* check if we should use the vlan entry, returns false if it's only context */ static inline bool br_vlan_should_use(const struct net_bridge_vlan *v) { if (br_vlan_is_master(v)) { diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c index 2c1fdf94badc..b879111582f0 100644 --- a/net/bridge/br_vlan.c +++ b/net/bridge/br_vlan.c @@ -195,7 +195,7 @@ static int __vlan_add(struct net_bridge_vlan *v, u16 flags) { struct net_bridge_vlan *masterv = NULL; struct net_bridge_port *p = NULL; - struct rhashtable *tbl; + struct net_bridge_vlan_group *vg; struct net_device *dev; struct net_bridge *br; int err; @@ -203,12 +203,12 @@ static int __vlan_add(struct net_bridge_vlan *v, u16 flags) if (br_vlan_is_master(v)) { br = v->br; dev = br->dev; - tbl = &br->vlgrp->vlan_hash; + vg = br->vlgrp; } else { p = v->port; br = p->br; dev = p->dev; - tbl = &p->vlgrp->vlan_hash; + vg = p->vlgrp; } if (p) { @@ -234,32 +234,31 @@ static int __vlan_add(struct net_bridge_vlan *v, u16 flags) v->brvlan = masterv; } - /* Add the dev mac only if it's a usable vlan */ + /* Add the dev mac and count the vlan only if it's usable */ if (br_vlan_should_use(v)) { err = br_fdb_insert(br, p, dev->dev_addr, v->vid); if (err) { br_err(br, "failed insert local address into bridge forwarding table\n"); goto out_filt; } + vg->num_vlans++; } - err = rhashtable_lookup_insert_fast(tbl, &v->vnode, br_vlan_rht_params); + err = rhashtable_lookup_insert_fast(&vg->vlan_hash, &v->vnode, + br_vlan_rht_params); if (err) goto out_fdb_insert; __vlan_add_list(v); __vlan_add_flags(v, flags); - if (br_vlan_is_master(v)) { - if (br_vlan_is_brentry(v)) - br->vlgrp->num_vlans++; - } else { - p->vlgrp->num_vlans++; - } out: return err; out_fdb_insert: - br_fdb_find_delete_local(br, p, br->dev->dev_addr, v->vid); + if (br_vlan_should_use(v)) { + br_fdb_find_delete_local(br, p, dev->dev_addr, v->vid); + vg->num_vlans--; + } out_filt: if (p) { @@ -278,15 +277,12 @@ static int __vlan_del(struct net_bridge_vlan *v) struct net_bridge_vlan *masterv = v; struct net_bridge_vlan_group *vg; struct net_bridge_port *p = NULL; - struct net_bridge *br; int err = 0; if (br_vlan_is_master(v)) { - br = v->br; vg = v->br->vlgrp; } else { p = v->port; - br = p->br; vg = v->port->vlgrp; masterv = v->brvlan; } @@ -298,13 +294,9 @@ static int __vlan_del(struct net_bridge_vlan *v) goto out; } - if (br_vlan_is_master(v)) { - if (br_vlan_is_brentry(v)) { - v->flags &= ~BRIDGE_VLAN_INFO_BRENTRY; - br->vlgrp->num_vlans--; - } - } else { - p->vlgrp->num_vlans--; + if (br_vlan_should_use(v)) { + v->flags &= ~BRIDGE_VLAN_INFO_BRENTRY; + vg->num_vlans--; } if (masterv != v) {