[PATCH net-next 0/4] bridge: vlan: cleanups & fixes (part 3)

[PATCH net-next 0/4] bridge: vlan: cleanups & fixes (part 3)

[Nikolay Aleksandrov]

From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>

Hi,
Patch 01 converts the vlgrp member to use rcu as it was already used in a
similar way so better to make it official and use all the available RCU
instrumentation. Patch 02 fixes a bug where the vlan_list can be traversed
without rtnl or rcu held which could lead to using freed entries.
I'm not intializing vlgrp to null before kfree_rcu because Ido has a patch
for that which fixes a warning from kasan.
Patch 03 fixes a bug reported by Ido Schimmel about the vlan_flush order
and switchdevs, and patch 04 refactors (br|nbp)_vlan_flush and combines
them into a single function.

Thank you,
 Nik

Nikolay Aleksandrov (4):
  bridge: vlan: use proper rcu for the vlgrp member
  bridge: vlan: use rcu for vlan_list traversal in br_fill_ifinfo
  bridge: vlan: break vlan_flush in two phases to keep old order
  bridge: vlan: combine (br|nbp)_vlan_flush into one

 net/bridge/br_device.c  |   2 +-
 net/bridge/br_forward.c |   6 +--
 net/bridge/br_if.c      |  13 +++--
 net/bridge/br_input.c   |   4 +-
 net/bridge/br_netlink.c |  25 ++++++----
 net/bridge/br_private.h |  43 ++++++++++++-----
 net/bridge/br_vlan.c    | 124 +++++++++++++++++++++++++++---------------------
 7 files changed, 132 insertions(+), 85 deletions(-)

-- 
2.4.3

[PATCH net-next 1/4] bridge: vlan: use proper rcu for the vlgrp member

[Nikolay Aleksandrov]

From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>

The bridge and port's vlgrp member is already used in RCU way, currently
we rely on the fact that it cannot disappear while the port exists but
that is error-prone and we might miss places with improper locking
(either RCU or RTNL must be held to walk the vlan_list). So make it
official and use RCU for vlgrp to catch offenders. Introduce proper vlgrp
accessors and use them consistently throughout the code.

Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
 net/bridge/br_device.c  |   2 +-
 net/bridge/br_forward.c |   6 +--
 net/bridge/br_input.c   |   4 +-
 net/bridge/br_netlink.c |   4 +-
 net/bridge/br_private.h |  34 +++++++++++++--
 net/bridge/br_vlan.c    | 107 +++++++++++++++++++++++++++++-------------------
 6 files changed, 104 insertions(+), 53 deletions(-)

diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index bdfb9544ca03..5e88d3e17546 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -56,7 +56,7 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev)
 	skb_reset_mac_header(skb);
 	skb_pull(skb, ETH_HLEN);
 
-	if (!br_allowed_ingress(br, br_vlan_group(br), skb, &vid))
+	if (!br_allowed_ingress(br, br_vlan_group_rcu(br), skb, &vid))
 		goto out;
 
 	if (is_broadcast_ether_addr(dest))
diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
index 6d5ed795c3e2..a9d424e20229 100644
--- a/net/bridge/br_forward.c
+++ b/net/bridge/br_forward.c
@@ -32,7 +32,7 @@ static inline int should_deliver(const struct net_bridge_port *p,
 {
 	struct net_bridge_vlan_group *vg;
 
-	vg = nbp_vlan_group(p);
+	vg = nbp_vlan_group_rcu(p);
 	return ((p->flags & BR_HAIRPIN_MODE) || skb->dev != p->dev) &&
 		br_allowed_egress(vg, skb) && p->state == BR_STATE_FORWARDING;
 }
@@ -80,7 +80,7 @@ static void __br_deliver(const struct net_bridge_port *to, struct sk_buff *skb)
 {
 	struct net_bridge_vlan_group *vg;
 
-	vg = nbp_vlan_group(to);
+	vg = nbp_vlan_group_rcu(to);
 	skb = br_handle_vlan(to->br, vg, skb);
 	if (!skb)
 		return;
@@ -112,7 +112,7 @@ static void __br_forward(const struct net_bridge_port *to, struct sk_buff *skb)
 		return;
 	}
 
-	vg = nbp_vlan_group(to);
+	vg = nbp_vlan_group_rcu(to);
 	skb = br_handle_vlan(to->br, vg, skb);
 	if (!skb)
 		return;
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index f5c5a4500e2f..f7fba74108a9 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -44,7 +44,7 @@ static int br_pass_frame_up(struct sk_buff *skb)
 	brstats->rx_bytes += skb->len;
 	u64_stats_update_end(&brstats->syncp);
 
-	vg = br_vlan_group(br);
+	vg = br_vlan_group_rcu(br);
 	/* Bridge is just like any other port.  Make sure the
 	 * packet is allowed except in promisc modue when someone
 	 * may be running packet capture.
@@ -140,7 +140,7 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 	if (!p || p->state == BR_STATE_DISABLED)
 		goto drop;
 
-	if (!br_allowed_ingress(p->br, nbp_vlan_group(p), skb, &vid))
+	if (!br_allowed_ingress(p->br, nbp_vlan_group_rcu(p), skb, &vid))
 		goto out;
 
 	/* insert into forwarding database after filtering to avoid spoofing */
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index d78b4429505a..edee48e9aa8f 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -102,10 +102,10 @@ static size_t br_get_link_af_size_filtered(const struct net_device *dev,
 	rcu_read_lock();
 	if (br_port_exists(dev)) {
 		p = br_port_get_rcu(dev);
-		vg = nbp_vlan_group(p);
+		vg = nbp_vlan_group_rcu(p);
 	} else if (dev->priv_flags & IFF_EBRIDGE) {
 		br = netdev_priv(dev);
-		vg = br_vlan_group(br);
+		vg = br_vlan_group_rcu(br);
 	}
 	num_vlan_infos = br_get_num_vlan_infos(vg, filter_mask);
 	rcu_read_unlock();
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 09d3ecbcb4f0..7d14ba93bba4 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -132,6 +132,7 @@ struct net_bridge_vlan_group {
 	struct list_head		vlan_list;
 	u16				num_vlans;
 	u16				pvid;
+	struct rcu_head			rcu;
 };
 
 struct net_bridge_fdb_entry
@@ -229,7 +230,7 @@ struct net_bridge_port
 	struct netpoll			*np;
 #endif
 #ifdef CONFIG_BRIDGE_VLAN_FILTERING
-	struct net_bridge_vlan_group	*vlgrp;
+	struct net_bridge_vlan_group	__rcu *vlgrp;
 #endif
 };
 
@@ -337,7 +338,7 @@ struct net_bridge
 	struct kobject			*ifobj;
 	u32				auto_cnt;
 #ifdef CONFIG_BRIDGE_VLAN_FILTERING
-	struct net_bridge_vlan_group	*vlgrp;
+	struct net_bridge_vlan_group	__rcu *vlgrp;
 	u8				vlan_enabled;
 	__be16				vlan_proto;
 	u16				default_pvid;
@@ -700,13 +701,25 @@ int nbp_get_num_vlan_infos(struct net_bridge_port *p, u32 filter_mask);
 static inline struct net_bridge_vlan_group *br_vlan_group(
 					const struct net_bridge *br)
 {
-	return br->vlgrp;
+	return rtnl_dereference(br->vlgrp);
 }
 
 static inline struct net_bridge_vlan_group *nbp_vlan_group(
 					const struct net_bridge_port *p)
 {
-	return p->vlgrp;
+	return rtnl_dereference(p->vlgrp);
+}
+
+static inline struct net_bridge_vlan_group *br_vlan_group_rcu(
+					const struct net_bridge *br)
+{
+	return rcu_dereference(br->vlgrp);
+}
+
+static inline struct net_bridge_vlan_group *nbp_vlan_group_rcu(
+					const struct net_bridge_port *p)
+{
+	return rcu_dereference(p->vlgrp);
 }
 
 /* Since bridge now depends on 8021Q module, but the time bridge sees the
@@ -853,6 +866,19 @@ static inline struct net_bridge_vlan_group *nbp_vlan_group(
 {
 	return NULL;
 }
+
+static inline struct net_bridge_vlan_group *br_vlan_group_rcu(
+					const struct net_bridge *br)
+{
+	return NULL;
+}
+
+static inline struct net_bridge_vlan_group *nbp_vlan_group_rcu(
+					const struct net_bridge_port *p)
+{
+	return NULL;
+}
+
 #endif
 
 struct nf_br_ops {
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index ad7e4f6b6d6b..a212979257b1 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -54,9 +54,9 @@ static void __vlan_add_flags(struct net_bridge_vlan *v, u16 flags)
 	struct net_bridge_vlan_group *vg;
 
 	if (br_vlan_is_master(v))
-		vg = v->br->vlgrp;
+		vg = br_vlan_group(v->br);
 	else
-		vg = v->port->vlgrp;
+		vg = nbp_vlan_group(v->port);
 
 	if (flags & BRIDGE_VLAN_INFO_PVID)
 		__vlan_add_pvid(vg, v->vid);
@@ -91,11 +91,16 @@ static int __vlan_vid_add(struct net_device *dev, struct net_bridge *br,
 
 static void __vlan_add_list(struct net_bridge_vlan *v)
 {
+	struct net_bridge_vlan_group *vg;
 	struct list_head *headp, *hpos;
 	struct net_bridge_vlan *vent;
 
-	headp = br_vlan_is_master(v) ? &v->br->vlgrp->vlan_list :
-				       &v->port->vlgrp->vlan_list;
+	if (br_vlan_is_master(v))
+		vg = br_vlan_group(v->br);
+	else
+		vg = nbp_vlan_group(v->port);
+
+	headp = &vg->vlan_list;
 	list_for_each_prev(hpos, headp) {
 		vent = list_entry(hpos, struct net_bridge_vlan, vlist);
 		if (v->vid < vent->vid)
@@ -137,14 +142,16 @@ static int __vlan_vid_del(struct net_device *dev, struct net_bridge *br,
  */
 static struct net_bridge_vlan *br_vlan_get_master(struct net_bridge *br, u16 vid)
 {
+	struct net_bridge_vlan_group *vg;
 	struct net_bridge_vlan *masterv;
 
-	masterv = br_vlan_find(br->vlgrp, vid);
+	vg = br_vlan_group(br);
+	masterv = br_vlan_find(vg, 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);
+		masterv = br_vlan_find(vg, vid);
 		if (WARN_ON(!masterv))
 			return NULL;
 	}
@@ -155,11 +162,14 @@ static struct net_bridge_vlan *br_vlan_get_master(struct net_bridge *br, u16 vid
 
 static void br_vlan_put_master(struct net_bridge_vlan *masterv)
 {
+	struct net_bridge_vlan_group *vg;
+
 	if (!br_vlan_is_master(masterv))
 		return;
 
+	vg = br_vlan_group(masterv->br);
 	if (atomic_dec_and_test(&masterv->refcnt)) {
-		rhashtable_remove_fast(&masterv->br->vlgrp->vlan_hash,
+		rhashtable_remove_fast(&vg->vlan_hash,
 				       &masterv->vnode, br_vlan_rht_params);
 		__vlan_del_list(masterv);
 		kfree_rcu(masterv, rcu);
@@ -189,12 +199,12 @@ static int __vlan_add(struct net_bridge_vlan *v, u16 flags)
 	if (br_vlan_is_master(v)) {
 		br = v->br;
 		dev = br->dev;
-		vg = br->vlgrp;
+		vg = br_vlan_group(br);
 	} else {
 		p = v->port;
 		br = p->br;
 		dev = p->dev;
-		vg = p->vlgrp;
+		vg = nbp_vlan_group(p);
 	}
 
 	if (p) {
@@ -266,10 +276,10 @@ static int __vlan_del(struct net_bridge_vlan *v)
 	int err = 0;
 
 	if (br_vlan_is_master(v)) {
-		vg = v->br->vlgrp;
+		vg = br_vlan_group(v->br);
 	} else {
 		p = v->port;
-		vg = v->port->vlgrp;
+		vg = nbp_vlan_group(v->port);
 		masterv = v->brvlan;
 	}
 
@@ -305,7 +315,7 @@ static void __vlan_flush(struct net_bridge_vlan_group *vlgrp)
 	list_for_each_entry_safe(vlan, tmp, &vlgrp->vlan_list, vlist)
 		__vlan_del(vlan);
 	rhashtable_destroy(&vlgrp->vlan_hash);
-	kfree(vlgrp);
+	kfree_rcu(vlgrp, rcu);
 }
 
 struct sk_buff *br_handle_vlan(struct net_bridge *br,
@@ -467,7 +477,7 @@ bool br_should_learn(struct net_bridge_port *p, struct sk_buff *skb, u16 *vid)
 	if (!br->vlan_enabled)
 		return true;
 
-	vg = p->vlgrp;
+	vg = nbp_vlan_group(p);
 	if (!vg || !vg->num_vlans)
 		return false;
 
@@ -493,12 +503,14 @@ bool br_should_learn(struct net_bridge_port *p, struct sk_buff *skb, u16 *vid)
  */
 int br_vlan_add(struct net_bridge *br, u16 vid, u16 flags)
 {
+	struct net_bridge_vlan_group *vg;
 	struct net_bridge_vlan *vlan;
 	int ret;
 
 	ASSERT_RTNL();
 
-	vlan = br_vlan_find(br->vlgrp, vid);
+	vg = br_vlan_group(br);
+	vlan = br_vlan_find(vg, vid);
 	if (vlan) {
 		if (!br_vlan_is_brentry(vlan)) {
 			/* Trying to change flags of non-existent bridge vlan */
@@ -513,7 +525,7 @@ int br_vlan_add(struct net_bridge *br, u16 vid, u16 flags)
 			}
 			atomic_inc(&vlan->refcnt);
 			vlan->flags |= BRIDGE_VLAN_INFO_BRENTRY;
-			br->vlgrp->num_vlans++;
+			vg->num_vlans++;
 		}
 		__vlan_add_flags(vlan, flags);
 		return 0;
@@ -541,11 +553,13 @@ int br_vlan_add(struct net_bridge *br, u16 vid, u16 flags)
  */
 int br_vlan_delete(struct net_bridge *br, u16 vid)
 {
+	struct net_bridge_vlan_group *vg;
 	struct net_bridge_vlan *v;
 
 	ASSERT_RTNL();
 
-	v = br_vlan_find(br->vlgrp, vid);
+	vg = br_vlan_group(br);
+	v = br_vlan_find(vg, vid);
 	if (!v || !br_vlan_is_brentry(v))
 		return -ENOENT;
 
@@ -626,6 +640,7 @@ int __br_vlan_set_proto(struct net_bridge *br, __be16 proto)
 	int err = 0;
 	struct net_bridge_port *p;
 	struct net_bridge_vlan *vlan;
+	struct net_bridge_vlan_group *vg;
 	__be16 oldproto;
 
 	if (br->vlan_proto == proto)
@@ -633,7 +648,8 @@ int __br_vlan_set_proto(struct net_bridge *br, __be16 proto)
 
 	/* Add VLANs for the new proto to the device filter. */
 	list_for_each_entry(p, &br->port_list, list) {
-		list_for_each_entry(vlan, &p->vlgrp->vlan_list, vlist) {
+		vg = nbp_vlan_group(p);
+		list_for_each_entry(vlan, &vg->vlan_list, vlist) {
 			err = vlan_vid_add(p->dev, proto, vlan->vid);
 			if (err)
 				goto err_filt;
@@ -647,19 +663,23 @@ int __br_vlan_set_proto(struct net_bridge *br, __be16 proto)
 	br_recalculate_fwd_mask(br);
 
 	/* Delete VLANs for the old proto from the device filter. */
-	list_for_each_entry(p, &br->port_list, list)
-		list_for_each_entry(vlan, &p->vlgrp->vlan_list, vlist)
+	list_for_each_entry(p, &br->port_list, list) {
+		vg = nbp_vlan_group(p);
+		list_for_each_entry(vlan, &vg->vlan_list, vlist)
 			vlan_vid_del(p->dev, oldproto, vlan->vid);
+	}
 
 	return 0;
 
 err_filt:
-	list_for_each_entry_continue_reverse(vlan, &p->vlgrp->vlan_list, vlist)
+	list_for_each_entry_continue_reverse(vlan, &vg->vlan_list, vlist)
 		vlan_vid_del(p->dev, proto, vlan->vid);
 
-	list_for_each_entry_continue_reverse(p, &br->port_list, list)
-		list_for_each_entry(vlan, &p->vlgrp->vlan_list, vlist)
+	list_for_each_entry_continue_reverse(p, &br->port_list, list) {
+		vg = nbp_vlan_group(p);
+		list_for_each_entry(vlan, &vg->vlan_list, vlist)
 			vlan_vid_del(p->dev, proto, vlan->vid);
+	}
 
 	return err;
 }
@@ -703,11 +723,11 @@ static void br_vlan_disable_default_pvid(struct net_bridge *br)
 	/* Disable default_pvid on all ports where it is still
 	 * configured.
 	 */
-	if (vlan_default_pvid(br->vlgrp, pvid))
+	if (vlan_default_pvid(br_vlan_group(br), pvid))
 		br_vlan_delete(br, pvid);
 
 	list_for_each_entry(p, &br->port_list, list) {
-		if (vlan_default_pvid(p->vlgrp, pvid))
+		if (vlan_default_pvid(nbp_vlan_group(p), pvid))
 			nbp_vlan_delete(p, pvid);
 	}
 
@@ -717,6 +737,7 @@ static void br_vlan_disable_default_pvid(struct net_bridge *br)
 int __br_vlan_set_default_pvid(struct net_bridge *br, u16 pvid)
 {
 	const struct net_bridge_vlan *pvent;
+	struct net_bridge_vlan_group *vg;
 	struct net_bridge_port *p;
 	u16 old_pvid;
 	int err = 0;
@@ -737,8 +758,9 @@ int __br_vlan_set_default_pvid(struct net_bridge *br, u16 pvid)
 	/* Update default_pvid config only if we do not conflict with
 	 * user configuration.
 	 */
-	pvent = br_vlan_find(br->vlgrp, pvid);
-	if ((!old_pvid || vlan_default_pvid(br->vlgrp, old_pvid)) &&
+	vg = br_vlan_group(br);
+	pvent = br_vlan_find(vg, pvid);
+	if ((!old_pvid || vlan_default_pvid(vg, old_pvid)) &&
 	    (!pvent || !br_vlan_should_use(pvent))) {
 		err = br_vlan_add(br, pvid,
 				  BRIDGE_VLAN_INFO_PVID |
@@ -754,9 +776,10 @@ int __br_vlan_set_default_pvid(struct net_bridge *br, u16 pvid)
 		/* Update default_pvid config only if we do not conflict with
 		 * user configuration.
 		 */
+		vg = nbp_vlan_group(p);
 		if ((old_pvid &&
-		     !vlan_default_pvid(p->vlgrp, old_pvid)) ||
-		    br_vlan_find(p->vlgrp, pvid))
+		     !vlan_default_pvid(vg, old_pvid)) ||
+		    br_vlan_find(vg, pvid))
 			continue;
 
 		err = nbp_vlan_add(p, pvid,
@@ -825,17 +848,19 @@ unlock:
 
 int br_vlan_init(struct net_bridge *br)
 {
+	struct net_bridge_vlan_group *vg;
 	int ret = -ENOMEM;
 
-	br->vlgrp = kzalloc(sizeof(struct net_bridge_vlan_group), GFP_KERNEL);
-	if (!br->vlgrp)
+	vg = kzalloc(sizeof(struct net_bridge_vlan_group), GFP_KERNEL);
+	if (!vg)
 		goto out;
-	ret = rhashtable_init(&br->vlgrp->vlan_hash, &br_vlan_rht_params);
+	ret = rhashtable_init(&vg->vlan_hash, &br_vlan_rht_params);
 	if (ret)
 		goto err_rhtbl;
-	INIT_LIST_HEAD(&br->vlgrp->vlan_list);
+	INIT_LIST_HEAD(&vg->vlan_list);
 	br->vlan_proto = htons(ETH_P_8021Q);
 	br->default_pvid = 1;
+	rcu_assign_pointer(br->vlgrp, vg);
 	ret = br_vlan_add(br, 1,
 			  BRIDGE_VLAN_INFO_PVID | BRIDGE_VLAN_INFO_UNTAGGED |
 			  BRIDGE_VLAN_INFO_BRENTRY);
@@ -846,9 +871,9 @@ out:
 	return ret;
 
 err_vlan_add:
-	rhashtable_destroy(&br->vlgrp->vlan_hash);
+	rhashtable_destroy(&vg->vlan_hash);
 err_rhtbl:
-	kfree(br->vlgrp);
+	kfree(vg);
 
 	goto out;
 }
@@ -866,9 +891,7 @@ int nbp_vlan_init(struct net_bridge_port *p)
 	if (ret)
 		goto err_rhtbl;
 	INIT_LIST_HEAD(&vg->vlan_list);
-	/* Make sure everything's committed before publishing vg */
-	smp_wmb();
-	p->vlgrp = vg;
+	rcu_assign_pointer(p->vlgrp, vg);
 	if (p->br->default_pvid) {
 		ret = nbp_vlan_add(p, p->br->default_pvid,
 				   BRIDGE_VLAN_INFO_PVID |
@@ -897,7 +920,7 @@ int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags)
 
 	ASSERT_RTNL();
 
-	vlan = br_vlan_find(port->vlgrp, vid);
+	vlan = br_vlan_find(nbp_vlan_group(port), vid);
 	if (vlan) {
 		__vlan_add_flags(vlan, flags);
 		return 0;
@@ -925,7 +948,7 @@ int nbp_vlan_delete(struct net_bridge_port *port, u16 vid)
 
 	ASSERT_RTNL();
 
-	v = br_vlan_find(port->vlgrp, vid);
+	v = br_vlan_find(nbp_vlan_group(port), vid);
 	if (!v)
 		return -ENOENT;
 	br_fdb_find_delete_local(port->br, port, port->dev->dev_addr, vid);
@@ -936,12 +959,14 @@ int nbp_vlan_delete(struct net_bridge_port *port, u16 vid)
 
 void nbp_vlan_flush(struct net_bridge_port *port)
 {
+	struct net_bridge_vlan_group *vg;
 	struct net_bridge_vlan *vlan;
 
 	ASSERT_RTNL();
 
-	list_for_each_entry(vlan, &port->vlgrp->vlan_list, vlist)
+	vg = nbp_vlan_group(port);
+	list_for_each_entry(vlan, &vg->vlan_list, vlist)
 		vlan_vid_del(port->dev, port->br->vlan_proto, vlan->vid);
 
-	__vlan_flush(nbp_vlan_group(port));
+	__vlan_flush(vg);
 }
-- 
2.4.3

Re: [PATCH net-next 1/4] bridge: vlan: use proper rcu for the vlgrp member

[Ido Schimmel]

Mon, Oct 12, 2015 at 02:41:06PM IDT, razor@blackwall.org wrote:
>From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>
Hi Nik,

Small nitpick:

<snip>

>@@ -825,17 +848,19 @@ unlock:
> 
> int br_vlan_init(struct net_bridge *br)
> {
>+	struct net_bridge_vlan_group *vg;
> 	int ret = -ENOMEM;
> 
>-	br->vlgrp = kzalloc(sizeof(struct net_bridge_vlan_group), GFP_KERNEL);
>-	if (!br->vlgrp)
>+	vg = kzalloc(sizeof(struct net_bridge_vlan_group), GFP_KERNEL);
Maybe just do sizeof(*vg)?
>+	if (!vg)
> 		goto out;
>-	ret = rhashtable_init(&br->vlgrp->vlan_hash, &br_vlan_rht_params);
>+	ret = rhashtable_init(&vg->vlan_hash, &br_vlan_rht_params);
> 	if (ret)
> 		goto err_rhtbl;
>-	INIT_LIST_HEAD(&br->vlgrp->vlan_list);
>+	INIT_LIST_HEAD(&vg->vlan_list);
> 	br->vlan_proto = htons(ETH_P_8021Q);
> 	br->default_pvid = 1;
>+	rcu_assign_pointer(br->vlgrp, vg);
> 	ret = br_vlan_add(br, 1,
> 			  BRIDGE_VLAN_INFO_PVID | BRIDGE_VLAN_INFO_UNTAGGED |
> 			  BRIDGE_VLAN_INFO_BRENTRY);
>@@ -846,9 +871,9 @@ out:
> 	return ret;

</snip>

>-- 
>2.4.3
>

[PATCH net-next 2/4] bridge: vlan: use rcu for vlan_list traversal in br_fill_ifinfo

[Nikolay Aleksandrov]

From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>

br_fill_ifinfo is called by br_ifinfo_notify which can be called from
many contexts with different locks held, sometimes it relies upon
bridge's spinlock only which is a problem for the vlan code, so use
explicitly rcu for that to avoid problems.

Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
 net/bridge/br_netlink.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index edee48e9aa8f..e27bde2642cc 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -253,7 +253,7 @@ static int br_fill_ifvlaninfo_compressed(struct sk_buff *skb,
 	 * if vlaninfo represents a range
 	 */
 	pvid = br_get_pvid(vg);
-	list_for_each_entry(v, &vg->vlan_list, vlist) {
+	list_for_each_entry_rcu(v, &vg->vlan_list, vlist) {
 		flags = 0;
 		if (!br_vlan_should_use(v))
 			continue;
@@ -303,7 +303,7 @@ static int br_fill_ifvlaninfo(struct sk_buff *skb,
 	u16 pvid;
 
 	pvid = br_get_pvid(vg);
-	list_for_each_entry(v, &vg->vlan_list, vlist) {
+	list_for_each_entry_rcu(v, &vg->vlan_list, vlist) {
 		if (!br_vlan_should_use(v))
 			continue;
 
@@ -386,22 +386,27 @@ static int br_fill_ifinfo(struct sk_buff *skb,
 		struct nlattr *af;
 		int err;
 
+		/* RCU needed because of the VLAN locking rules (rcu || rtnl) */
+		rcu_read_lock();
 		if (port)
-			vg = nbp_vlan_group(port);
+			vg = nbp_vlan_group_rcu(port);
 		else
-			vg = br_vlan_group(br);
+			vg = br_vlan_group_rcu(br);
 
-		if (!vg || !vg->num_vlans)
+		if (!vg || !vg->num_vlans) {
+			rcu_read_unlock();
 			goto done;
-
+		}
 		af = nla_nest_start(skb, IFLA_AF_SPEC);
-		if (!af)
+		if (!af) {
+			rcu_read_unlock();
 			goto nla_put_failure;
-
+		}
 		if (filter_mask & RTEXT_FILTER_BRVLAN_COMPRESSED)
 			err = br_fill_ifvlaninfo_compressed(skb, vg);
 		else
 			err = br_fill_ifvlaninfo(skb, vg);
+		rcu_read_unlock();
 		if (err)
 			goto nla_put_failure;
 		nla_nest_end(skb, af);
-- 
2.4.3

[PATCH net-next 3/4] bridge: vlan: break vlan_flush in two phases to keep old order

[Nikolay Aleksandrov]

From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>

Ido Schimmel reported a problem with switchdev devices because of the
order change of del_nbp operations, more specifically the move of
nbp_vlan_flush() which deletes all vlans and frees vlgrp after the
rx_handler has been unregistered. So in order to fix this break
vlan_flush in two phases:
1. delete all of vlan_group's vlans
2. destroy rhtable and free vlgrp
We execute phase I (free_rht == false) in the same place as before so the
vlans can be cleared and free the vlgrp after the rx_handler has been
unregistered in phase II (free_rht == true).

Reported-by: Ido Schimmel <idosch@mellanox.com>
Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
Ido: I hope this fixes it for your case, a test would be much appreciated.

 net/bridge/br_if.c      | 11 ++++++++---
 net/bridge/br_private.h |  8 ++++----
 net/bridge/br_vlan.c    | 17 ++++++++++-------
 3 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 934cae9fa317..74a03c0a4e5f 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -248,6 +248,8 @@ static void del_nbp(struct net_bridge_port *p)
 
 	list_del_rcu(&p->list);
 
+	/* vlan_flush phase I: remove vlans */
+	nbp_vlan_flush(p, false);
 	br_fdb_delete_by_port(br, p, 0, 1);
 	nbp_update_port_count(br);
 
@@ -256,8 +258,10 @@ static void del_nbp(struct net_bridge_port *p)
 	dev->priv_flags &= ~IFF_BRIDGE_PORT;
 
 	netdev_rx_handler_unregister(dev);
-	/* use the synchronize_rcu done by netdev_rx_handler_unregister */
-	nbp_vlan_flush(p);
+	/* use the synchronize_rcu done by netdev_rx_handler_unregister
+	 * vlan_flush phase II: free rht and vlgrp
+	 */
+	nbp_vlan_flush(p, true);
 
 	br_multicast_del_port(p);
 
@@ -281,7 +285,8 @@ void br_dev_delete(struct net_device *dev, struct list_head *head)
 
 	br_fdb_delete_by_port(br, NULL, 0, 1);
 
-	br_vlan_flush(br);
+	/* vlan_flush execute both phases (see del_nbp) */
+	br_vlan_flush(br, true);
 	br_multicast_dev_del(br);
 	del_timer_sync(&br->gc_timer);
 
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 7d14ba93bba4..3938a976417f 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -682,7 +682,7 @@ struct sk_buff *br_handle_vlan(struct net_bridge *br,
 			       struct sk_buff *skb);
 int br_vlan_add(struct net_bridge *br, u16 vid, u16 flags);
 int br_vlan_delete(struct net_bridge *br, u16 vid);
-void br_vlan_flush(struct net_bridge *br);
+void br_vlan_flush(struct net_bridge *br, bool free_rht);
 struct net_bridge_vlan *br_vlan_find(struct net_bridge_vlan_group *vg, u16 vid);
 void br_recalculate_fwd_mask(struct net_bridge *br);
 int __br_vlan_filter_toggle(struct net_bridge *br, unsigned long val);
@@ -694,7 +694,7 @@ int br_vlan_set_default_pvid(struct net_bridge *br, unsigned long val);
 int __br_vlan_set_default_pvid(struct net_bridge *br, u16 pvid);
 int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags);
 int nbp_vlan_delete(struct net_bridge_port *port, u16 vid);
-void nbp_vlan_flush(struct net_bridge_port *port);
+void nbp_vlan_flush(struct net_bridge_port *port, bool free_rht);
 int nbp_vlan_init(struct net_bridge_port *port);
 int nbp_get_num_vlan_infos(struct net_bridge_port *p, u32 filter_mask);
 
@@ -790,7 +790,7 @@ static inline int br_vlan_delete(struct net_bridge *br, u16 vid)
 	return -EOPNOTSUPP;
 }
 
-static inline void br_vlan_flush(struct net_bridge *br)
+static inline void br_vlan_flush(struct net_bridge *br, bool free_rht)
 {
 }
 
@@ -813,7 +813,7 @@ static inline int nbp_vlan_delete(struct net_bridge_port *port, u16 vid)
 	return -EOPNOTSUPP;
 }
 
-static inline void nbp_vlan_flush(struct net_bridge_port *port)
+static inline void nbp_vlan_flush(struct net_bridge_port *port, bool free_rht)
 {
 }
 
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index a212979257b1..4fb9b23c9838 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -307,15 +307,18 @@ out:
 	return err;
 }
 
-static void __vlan_flush(struct net_bridge_vlan_group *vlgrp)
+static void __vlan_flush(struct net_bridge_vlan_group *vlgrp, bool free_rht)
 {
 	struct net_bridge_vlan *vlan, *tmp;
 
 	__vlan_delete_pvid(vlgrp, vlgrp->pvid);
 	list_for_each_entry_safe(vlan, tmp, &vlgrp->vlan_list, vlist)
 		__vlan_del(vlan);
-	rhashtable_destroy(&vlgrp->vlan_hash);
-	kfree_rcu(vlgrp, rcu);
+
+	if (free_rht) {
+		rhashtable_destroy(&vlgrp->vlan_hash);
+		kfree_rcu(vlgrp, rcu);
+	}
 }
 
 struct sk_buff *br_handle_vlan(struct net_bridge *br,
@@ -569,11 +572,11 @@ int br_vlan_delete(struct net_bridge *br, u16 vid)
 	return __vlan_del(v);
 }
 
-void br_vlan_flush(struct net_bridge *br)
+void br_vlan_flush(struct net_bridge *br, bool free_rht)
 {
 	ASSERT_RTNL();
 
-	__vlan_flush(br_vlan_group(br));
+	__vlan_flush(br_vlan_group(br), free_rht);
 }
 
 struct net_bridge_vlan *br_vlan_find(struct net_bridge_vlan_group *vg, u16 vid)
@@ -957,7 +960,7 @@ int nbp_vlan_delete(struct net_bridge_port *port, u16 vid)
 	return __vlan_del(v);
 }
 
-void nbp_vlan_flush(struct net_bridge_port *port)
+void nbp_vlan_flush(struct net_bridge_port *port, bool free_rht)
 {
 	struct net_bridge_vlan_group *vg;
 	struct net_bridge_vlan *vlan;
@@ -968,5 +971,5 @@ void nbp_vlan_flush(struct net_bridge_port *port)
 	list_for_each_entry(vlan, &vg->vlan_list, vlist)
 		vlan_vid_del(port->dev, port->br->vlan_proto, vlan->vid);
 
-	__vlan_flush(vg);
+	__vlan_flush(vg, free_rht);
 }
-- 
2.4.3

Re: [PATCH net-next 3/4] bridge: vlan: break vlan_flush in two phases to keep old order

[Ido Schimmel]

Mon, Oct 12, 2015 at 02:41:08PM IDT, razor@blackwall.org wrote:
>From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>
Hi,

>Ido Schimmel reported a problem with switchdev devices because of the
>order change of del_nbp operations, more specifically the move of
>nbp_vlan_flush() which deletes all vlans and frees vlgrp after the
>rx_handler has been unregistered. So in order to fix this break
>vlan_flush in two phases:
>1. delete all of vlan_group's vlans
>2. destroy rhtable and free vlgrp
>We execute phase I (free_rht == false) in the same place as before so the
>vlans can be cleared and free the vlgrp after the rx_handler has been
>unregistered in phase II (free_rht == true).
I don't fully understand the reason for the two-phase cleanup. Please
see below.
>
>Reported-by: Ido Schimmel <idosch@mellanox.com>
>Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>---
>Ido: I hope this fixes it for your case, a test would be much appreciated.
This indeed fixes our switchdev issue. Thanks for the fix!
>
> net/bridge/br_if.c      | 11 ++++++++---
> net/bridge/br_private.h |  8 ++++----
> net/bridge/br_vlan.c    | 17 ++++++++++-------
> 3 files changed, 22 insertions(+), 14 deletions(-)
>
>diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
>index 934cae9fa317..74a03c0a4e5f 100644
>--- a/net/bridge/br_if.c
>+++ b/net/bridge/br_if.c
>@@ -248,6 +248,8 @@ static void del_nbp(struct net_bridge_port *p)
> 
> 	list_del_rcu(&p->list);
> 
>+	/* vlan_flush phase I: remove vlans */
>+	nbp_vlan_flush(p, false);
> 	br_fdb_delete_by_port(br, p, 0, 1);
> 	nbp_update_port_count(br);
> 
>@@ -256,8 +258,10 @@ static void del_nbp(struct net_bridge_port *p)
> 	dev->priv_flags &= ~IFF_BRIDGE_PORT;
> 
> 	netdev_rx_handler_unregister(dev);
>-	/* use the synchronize_rcu done by netdev_rx_handler_unregister */
>-	nbp_vlan_flush(p);
>+	/* use the synchronize_rcu done by netdev_rx_handler_unregister
>+	 * vlan_flush phase II: free rht and vlgrp
>+	 */
>+	nbp_vlan_flush(p, true);
> 
> 	br_multicast_del_port(p);
> 
>@@ -281,7 +285,8 @@ void br_dev_delete(struct net_device *dev, struct list_head *head)
> 
> 	br_fdb_delete_by_port(br, NULL, 0, 1);
> 
>-	br_vlan_flush(br);
>+	/* vlan_flush execute both phases (see del_nbp) */
>+	br_vlan_flush(br, true);
> 	br_multicast_dev_del(br);
> 	del_timer_sync(&br->gc_timer);
> 
>diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>index 7d14ba93bba4..3938a976417f 100644
>--- a/net/bridge/br_private.h
>+++ b/net/bridge/br_private.h
>@@ -682,7 +682,7 @@ struct sk_buff *br_handle_vlan(struct net_bridge *br,
> 			       struct sk_buff *skb);
> int br_vlan_add(struct net_bridge *br, u16 vid, u16 flags);
> int br_vlan_delete(struct net_bridge *br, u16 vid);
>-void br_vlan_flush(struct net_bridge *br);
>+void br_vlan_flush(struct net_bridge *br, bool free_rht);
> struct net_bridge_vlan *br_vlan_find(struct net_bridge_vlan_group *vg, u16 vid);
> void br_recalculate_fwd_mask(struct net_bridge *br);
> int __br_vlan_filter_toggle(struct net_bridge *br, unsigned long val);
>@@ -694,7 +694,7 @@ int br_vlan_set_default_pvid(struct net_bridge *br, unsigned long val);
> int __br_vlan_set_default_pvid(struct net_bridge *br, u16 pvid);
> int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags);
> int nbp_vlan_delete(struct net_bridge_port *port, u16 vid);
>-void nbp_vlan_flush(struct net_bridge_port *port);
>+void nbp_vlan_flush(struct net_bridge_port *port, bool free_rht);
> int nbp_vlan_init(struct net_bridge_port *port);
> int nbp_get_num_vlan_infos(struct net_bridge_port *p, u32 filter_mask);
> 
>@@ -790,7 +790,7 @@ static inline int br_vlan_delete(struct net_bridge *br, u16 vid)
> 	return -EOPNOTSUPP;
> }
> 
>-static inline void br_vlan_flush(struct net_bridge *br)
>+static inline void br_vlan_flush(struct net_bridge *br, bool free_rht)
> {
> }
> 
>@@ -813,7 +813,7 @@ static inline int nbp_vlan_delete(struct net_bridge_port *port, u16 vid)
> 	return -EOPNOTSUPP;
> }
> 
>-static inline void nbp_vlan_flush(struct net_bridge_port *port)
>+static inline void nbp_vlan_flush(struct net_bridge_port *port, bool free_rht)
> {
> }
> 
>diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
>index a212979257b1..4fb9b23c9838 100644
>--- a/net/bridge/br_vlan.c
>+++ b/net/bridge/br_vlan.c
>@@ -307,15 +307,18 @@ out:
> 	return err;
> }
> 
>-static void __vlan_flush(struct net_bridge_vlan_group *vlgrp)
>+static void __vlan_flush(struct net_bridge_vlan_group *vlgrp, bool free_rht)
> {
> 	struct net_bridge_vlan *vlan, *tmp;
> 
> 	__vlan_delete_pvid(vlgrp, vlgrp->pvid);
> 	list_for_each_entry_safe(vlan, tmp, &vlgrp->vlan_list, vlist)
> 		__vlan_del(vlan);
>-	rhashtable_destroy(&vlgrp->vlan_hash);
>-	kfree_rcu(vlgrp, rcu);
>+
Why not just issue a synchronize_rcu here and remove the if statement? I
believe that would also be better for when we remove the bridge device
itself. It's fully symmetric with init that way.
>+	if (free_rht) {
>+		rhashtable_destroy(&vlgrp->vlan_hash);
>+		kfree_rcu(vlgrp, rcu);
>+	}
> }
> 
> struct sk_buff *br_handle_vlan(struct net_bridge *br,
>@@ -569,11 +572,11 @@ int br_vlan_delete(struct net_bridge *br, u16 vid)
> 	return __vlan_del(v);
> }
> 
>-void br_vlan_flush(struct net_bridge *br)
>+void br_vlan_flush(struct net_bridge *br, bool free_rht)
> {
> 	ASSERT_RTNL();
> 
>-	__vlan_flush(br_vlan_group(br));
>+	__vlan_flush(br_vlan_group(br), free_rht);
> }
> 
> struct net_bridge_vlan *br_vlan_find(struct net_bridge_vlan_group *vg, u16 vid)
>@@ -957,7 +960,7 @@ int nbp_vlan_delete(struct net_bridge_port *port, u16 vid)
> 	return __vlan_del(v);
> }
> 
>-void nbp_vlan_flush(struct net_bridge_port *port)
>+void nbp_vlan_flush(struct net_bridge_port *port, bool free_rht)
> {
> 	struct net_bridge_vlan_group *vg;
> 	struct net_bridge_vlan *vlan;
>@@ -968,5 +971,5 @@ void nbp_vlan_flush(struct net_bridge_port *port)
> 	list_for_each_entry(vlan, &vg->vlan_list, vlist)
> 		vlan_vid_del(port->dev, port->br->vlan_proto, vlan->vid);
> 
>-	__vlan_flush(vg);
>+	__vlan_flush(vg, free_rht);
> }
>-- 
>2.4.3
>

Re: [PATCH net-next 3/4] bridge: vlan: break vlan_flush in two phases to keep old order

[Nikolay Aleksandrov]

On 10/12/2015 07:39 PM, Ido Schimmel wrote:
> Mon, Oct 12, 2015 at 02:41:08PM IDT, razor@blackwall.org wrote:
>> From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>>
> Hi,
> 
>> Ido Schimmel reported a problem with switchdev devices because of the
>> order change of del_nbp operations, more specifically the move of
>> nbp_vlan_flush() which deletes all vlans and frees vlgrp after the
>> rx_handler has been unregistered. So in order to fix this break
>> vlan_flush in two phases:
>> 1. delete all of vlan_group's vlans
>> 2. destroy rhtable and free vlgrp
>> We execute phase I (free_rht == false) in the same place as before so the
>> vlans can be cleared and free the vlgrp after the rx_handler has been
>> unregistered in phase II (free_rht == true).
> I don't fully understand the reason for the two-phase cleanup. Please
> see below.
>>
>> Reported-by: Ido Schimmel <idosch@mellanox.com>
>> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>> ---
>> Ido: I hope this fixes it for your case, a test would be much appreciated.
> This indeed fixes our switchdev issue. Thanks for the fix!
>>
[snip]
>>
>> -static void __vlan_flush(struct net_bridge_vlan_group *vlgrp)
>> +static void __vlan_flush(struct net_bridge_vlan_group *vlgrp, bool free_rht)
>> {
>> 	struct net_bridge_vlan *vlan, *tmp;
>>
>> 	__vlan_delete_pvid(vlgrp, vlgrp->pvid);
>> 	list_for_each_entry_safe(vlan, tmp, &vlgrp->vlan_list, vlist)
>> 		__vlan_del(vlan);
>> -	rhashtable_destroy(&vlgrp->vlan_hash);
>> -	kfree_rcu(vlgrp, rcu);
>> +
> Why not just issue a synchronize_rcu here and remove the if statement? I
> believe that would also be better for when we remove the bridge device
> itself. It's fully symmetric with init that way.
Hi,
I considered that, but I don't want to issue a second synchronize_rcu() for each
port when deleting them, with this change we avoid a second synchronize_rcu()
and use the rx_handler unregister one. In complex setups with lots of ports
this is a considerable overhead.
For the bridge device del case - the call is the same, there're no two phases
there.

Cheers,
 Nik

Re: [PATCH net-next 3/4] bridge: vlan: break vlan_flush in two phases to keep old order

[Ido Schimmel]

Mon, Oct 12, 2015 at 08:55:31PM IDT, nikolay@cumulusnetworks.com wrote:
>On 10/12/2015 07:39 PM, Ido Schimmel wrote:
>> Mon, Oct 12, 2015 at 02:41:08PM IDT, razor@blackwall.org wrote:
>>> From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>>>
>> Hi,
>> 
>>> Ido Schimmel reported a problem with switchdev devices because of the
>>> order change of del_nbp operations, more specifically the move of
>>> nbp_vlan_flush() which deletes all vlans and frees vlgrp after the
>>> rx_handler has been unregistered. So in order to fix this break
>>> vlan_flush in two phases:
>>> 1. delete all of vlan_group's vlans
>>> 2. destroy rhtable and free vlgrp
>>> We execute phase I (free_rht == false) in the same place as before so the
>>> vlans can be cleared and free the vlgrp after the rx_handler has been
>>> unregistered in phase II (free_rht == true).
>> I don't fully understand the reason for the two-phase cleanup. Please
>> see below.
>>>
>>> Reported-by: Ido Schimmel <idosch@mellanox.com>
>>> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>>> ---
>>> Ido: I hope this fixes it for your case, a test would be much appreciated.
>> This indeed fixes our switchdev issue. Thanks for the fix!
>>>
>[snip]
>>>
>>> -static void __vlan_flush(struct net_bridge_vlan_group *vlgrp)
>>> +static void __vlan_flush(struct net_bridge_vlan_group *vlgrp, bool free_rht)
>>> {
>>> 	struct net_bridge_vlan *vlan, *tmp;
>>>
>>> 	__vlan_delete_pvid(vlgrp, vlgrp->pvid);
>>> 	list_for_each_entry_safe(vlan, tmp, &vlgrp->vlan_list, vlist)
>>> 		__vlan_del(vlan);
>>> -	rhashtable_destroy(&vlgrp->vlan_hash);
>>> -	kfree_rcu(vlgrp, rcu);
>>> +
>> Why not just issue a synchronize_rcu here and remove the if statement? I
>> believe that would also be better for when we remove the bridge device
>> itself. It's fully symmetric with init that way.
>Hi,
>I considered that, but I don't want to issue a second synchronize_rcu() for each
>port when deleting them, with this change we avoid a second synchronize_rcu()
>and use the rx_handler unregister one. In complex setups with lots of ports
>this is a considerable overhead.
Yep, I assumed that was the reason.
>For the bridge device del case - the call is the same, there're no two phases
>there.
I know, but wouldn't it be a problem to delete the rhashtable in case of
a bridge? You don't have a synchronize_rcu just before as with ports or
are you relying on the kfree_rcu(masterv, rcu) in br_vlan_put_master?

It's probably a non-issue, but I want to make sure I'm not missing
something.

Thanks.
>
>Cheers,
> Nik

Re: [PATCH net-next 3/4] bridge: vlan: break vlan_flush in two phases to keep old order

[Nikolay Aleksandrov]

On 10/12/2015 07:55 PM, Nikolay Aleksandrov wrote:
> On 10/12/2015 07:39 PM, Ido Schimmel wrote:
>> Mon, Oct 12, 2015 at 02:41:08PM IDT, razor@blackwall.org wrote:
>>> From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>>>
>> Hi,
>>
>>> Ido Schimmel reported a problem with switchdev devices because of the
>>> order change of del_nbp operations, more specifically the move of
>>> nbp_vlan_flush() which deletes all vlans and frees vlgrp after the
>>> rx_handler has been unregistered. So in order to fix this break
>>> vlan_flush in two phases:
>>> 1. delete all of vlan_group's vlans
>>> 2. destroy rhtable and free vlgrp
>>> We execute phase I (free_rht == false) in the same place as before so the
>>> vlans can be cleared and free the vlgrp after the rx_handler has been
>>> unregistered in phase II (free_rht == true).
>> I don't fully understand the reason for the two-phase cleanup. Please
>> see below.
>>>
>>> Reported-by: Ido Schimmel <idosch@mellanox.com>
>>> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>>> ---
>>> Ido: I hope this fixes it for your case, a test would be much appreciated.
>> This indeed fixes our switchdev issue. Thanks for the fix!
>>>
> [snip]
>>>
>>> -static void __vlan_flush(struct net_bridge_vlan_group *vlgrp)
>>> +static void __vlan_flush(struct net_bridge_vlan_group *vlgrp, bool free_rht)
>>> {
>>> 	struct net_bridge_vlan *vlan, *tmp;
>>>
>>> 	__vlan_delete_pvid(vlgrp, vlgrp->pvid);
>>> 	list_for_each_entry_safe(vlan, tmp, &vlgrp->vlan_list, vlist)
>>> 		__vlan_del(vlan);
>>> -	rhashtable_destroy(&vlgrp->vlan_hash);
>>> -	kfree_rcu(vlgrp, rcu);
>>> +
>> Why not just issue a synchronize_rcu here and remove the if statement? I
>> believe that would also be better for when we remove the bridge device
>> itself. It's fully symmetric with init that way.
> Hi,
> I considered that, but I don't want to issue a second synchronize_rcu() for each
> port when deleting them, with this change we avoid a second synchronize_rcu()
> and use the rx_handler unregister one. In complex setups with lots of ports
> this is a considerable overhead.
> For the bridge device del case - the call is the same, there're no two phases
> there.
> 
> Cheers,
>  Nik
> 

Actually I have a better idea, we can use the delayed rcu free and destroy the
rhashtable there. v2 coming soon :-)

Thanks,
 Nik

[PATCH net-next 4/4] bridge: vlan: combine (br|nbp)_vlan_flush into one

[Nikolay Aleksandrov]

From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>

As Ido Schimmel pointed out the vlan_vid_del() loop in nbp_vlan_flush is
unnecessary (and is actually a remnant of the old vlan code) so we can
remove it and combine both br/nbp vlan_flush functions into one.

Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
 net/bridge/br_if.c      |  8 +++++---
 net/bridge/br_private.h |  9 ++-------
 net/bridge/br_vlan.c    | 16 +---------------
 3 files changed, 8 insertions(+), 25 deletions(-)

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 74a03c0a4e5f..ed431cc80b3d 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -233,6 +233,7 @@ static void destroy_nbp_rcu(struct rcu_head *head)
  */
 static void del_nbp(struct net_bridge_port *p)
 {
+	struct net_bridge_vlan_group *vg;
 	struct net_bridge *br = p->br;
 	struct net_device *dev = p->dev;
 
@@ -249,7 +250,8 @@ static void del_nbp(struct net_bridge_port *p)
 	list_del_rcu(&p->list);
 
 	/* vlan_flush phase I: remove vlans */
-	nbp_vlan_flush(p, false);
+	vg = nbp_vlan_group(p);
+	br_vlan_flush(vg, false);
 	br_fdb_delete_by_port(br, p, 0, 1);
 	nbp_update_port_count(br);
 
@@ -261,7 +263,7 @@ static void del_nbp(struct net_bridge_port *p)
 	/* use the synchronize_rcu done by netdev_rx_handler_unregister
 	 * vlan_flush phase II: free rht and vlgrp
 	 */
-	nbp_vlan_flush(p, true);
+	br_vlan_flush(vg, true);
 
 	br_multicast_del_port(p);
 
@@ -286,7 +288,7 @@ void br_dev_delete(struct net_device *dev, struct list_head *head)
 	br_fdb_delete_by_port(br, NULL, 0, 1);
 
 	/* vlan_flush execute both phases (see del_nbp) */
-	br_vlan_flush(br, true);
+	br_vlan_flush(br_vlan_group(br), true);
 	br_multicast_dev_del(br);
 	del_timer_sync(&br->gc_timer);
 
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 3938a976417f..73ee71c0a960 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -682,7 +682,7 @@ struct sk_buff *br_handle_vlan(struct net_bridge *br,
 			       struct sk_buff *skb);
 int br_vlan_add(struct net_bridge *br, u16 vid, u16 flags);
 int br_vlan_delete(struct net_bridge *br, u16 vid);
-void br_vlan_flush(struct net_bridge *br, bool free_rht);
+void br_vlan_flush(struct net_bridge_vlan_group *vg, bool free_rht);
 struct net_bridge_vlan *br_vlan_find(struct net_bridge_vlan_group *vg, u16 vid);
 void br_recalculate_fwd_mask(struct net_bridge *br);
 int __br_vlan_filter_toggle(struct net_bridge *br, unsigned long val);
@@ -694,7 +694,6 @@ int br_vlan_set_default_pvid(struct net_bridge *br, unsigned long val);
 int __br_vlan_set_default_pvid(struct net_bridge *br, u16 pvid);
 int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags);
 int nbp_vlan_delete(struct net_bridge_port *port, u16 vid);
-void nbp_vlan_flush(struct net_bridge_port *port, bool free_rht);
 int nbp_vlan_init(struct net_bridge_port *port);
 int nbp_get_num_vlan_infos(struct net_bridge_port *p, u32 filter_mask);
 
@@ -790,7 +789,7 @@ static inline int br_vlan_delete(struct net_bridge *br, u16 vid)
 	return -EOPNOTSUPP;
 }
 
-static inline void br_vlan_flush(struct net_bridge *br, bool free_rht)
+static inline void br_vlan_flush(struct net_bridge_vlan_group *vg, bool free_rht)
 {
 }
 
@@ -813,10 +812,6 @@ static inline int nbp_vlan_delete(struct net_bridge_port *port, u16 vid)
 	return -EOPNOTSUPP;
 }
 
-static inline void nbp_vlan_flush(struct net_bridge_port *port, bool free_rht)
-{
-}
-
 static inline struct net_bridge_vlan *br_vlan_find(struct net_bridge_vlan_group *vg,
 						   u16 vid)
 {
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 4fb9b23c9838..11ac14f60206 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -572,13 +572,6 @@ int br_vlan_delete(struct net_bridge *br, u16 vid)
 	return __vlan_del(v);
 }
 
-void br_vlan_flush(struct net_bridge *br, bool free_rht)
-{
-	ASSERT_RTNL();
-
-	__vlan_flush(br_vlan_group(br), free_rht);
-}
-
 struct net_bridge_vlan *br_vlan_find(struct net_bridge_vlan_group *vg, u16 vid)
 {
 	if (!vg)
@@ -960,16 +953,9 @@ int nbp_vlan_delete(struct net_bridge_port *port, u16 vid)
 	return __vlan_del(v);
 }
 
-void nbp_vlan_flush(struct net_bridge_port *port, bool free_rht)
+void br_vlan_flush(struct net_bridge_vlan_group *vg, bool free_rht)
 {
-	struct net_bridge_vlan_group *vg;
-	struct net_bridge_vlan *vlan;
-
 	ASSERT_RTNL();
 
-	vg = nbp_vlan_group(port);
-	list_for_each_entry(vlan, &vg->vlan_list, vlist)
-		vlan_vid_del(port->dev, port->br->vlan_proto, vlan->vid);
-
 	__vlan_flush(vg, free_rht);
 }
-- 
2.4.3

Re: [PATCH net-next 4/4] bridge: vlan: combine (br|nbp)_vlan_flush into one

[Ido Schimmel]

Mon, Oct 12, 2015 at 02:41:09PM IDT, razor@blackwall.org wrote:
>From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>
>As Ido Schimmel pointed out the vlan_vid_del() loop in nbp_vlan_flush is
>unnecessary (and is actually a remnant of the old vlan code) so we can
>remove it and combine both br/nbp vlan_flush functions into one.
Just a small note to Scott and Vivien:

One of the side effects of Nik's recent patchsets is that when VLANs are
flushed on a port the deletion is propagated to the driver via
switchdev ops, as __vlan_vid_del is called.

Therefore there is no need to do internal bookkeeping and remove VLANs
yourself when port is removed from bridge.
>
>Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>

Re: [PATCH net-next 4/4] bridge: vlan: combine (br|nbp)_vlan_flush into one

[Vivien Didelot]

Hi,

On Oct. Monday 12 (42) 08:51 PM, Ido Schimmel wrote:
> Mon, Oct 12, 2015 at 02:41:09PM IDT, razor@blackwall.org wrote:
> >From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> >
> >As Ido Schimmel pointed out the vlan_vid_del() loop in nbp_vlan_flush is
> >unnecessary (and is actually a remnant of the old vlan code) so we can
> >remove it and combine both br/nbp vlan_flush functions into one.
> Just a small note to Scott and Vivien:
> 
> One of the side effects of Nik's recent patchsets is that when VLANs are
> flushed on a port the deletion is propagated to the driver via
> switchdev ops, as __vlan_vid_del is called.
> 
> Therefore there is no need to do internal bookkeeping and remove VLANs
> yourself when port is removed from bridge.

I was thinking about caching VLAN entries in the mv88e6xxx driver to
improve look up on VLAN and FDB operations, but it's a bit prematurate.

But when VLAN are flushed, we still need to remove them from the
hardware table, right?

Flushing is interesting though, most hardware have flush operations and
it would be interesting to have switchdev fdb_flush and vlan_flush ops.

Thanks!
-v

Re: [PATCH net-next 4/4] bridge: vlan: combine (br|nbp)_vlan_flush into one

[Ido Schimmel]

Mon, Oct 12, 2015 at 09:15:39PM IDT, vivien.didelot@savoirfairelinux.com wrote:
>Hi,
>
>On Oct. Monday 12 (42) 08:51 PM, Ido Schimmel wrote:
>> Mon, Oct 12, 2015 at 02:41:09PM IDT, razor@blackwall.org wrote:
>> >From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>> >
>> >As Ido Schimmel pointed out the vlan_vid_del() loop in nbp_vlan_flush is
>> >unnecessary (and is actually a remnant of the old vlan code) so we can
>> >remove it and combine both br/nbp vlan_flush functions into one.
>> Just a small note to Scott and Vivien:
>> 
>> One of the side effects of Nik's recent patchsets is that when VLANs are
>> flushed on a port the deletion is propagated to the driver via
>> switchdev ops, as __vlan_vid_del is called.
>> 
>> Therefore there is no need to do internal bookkeeping and remove VLANs
>> yourself when port is removed from bridge.
>
>I was thinking about caching VLAN entries in the mv88e6xxx driver to
>improve look up on VLAN and FDB operations, but it's a bit prematurate.
>
>But when VLAN are flushed, we still need to remove them from the
>hardware table, right?
Hi,

Not sure I'm following. You'll simply get a SWITCHDEV_OBJ_ID_PORT_VLAN
(del) for each VLAN configured on the port you just removed from the bridge.
I guess you remove them from your hardware table in the implementation
of these ops?
>
>Flushing is interesting though, most hardware have flush operations and
>it would be interesting to have switchdev fdb_flush and vlan_flush ops.
>
>Thanks!
>-v

Re: [PATCH net-next 4/4] bridge: vlan: combine (br|nbp)_vlan_flush into one

[Vivien Didelot]

On Oct. Monday 12 (42) 09:27 PM, Ido Schimmel wrote:
> Mon, Oct 12, 2015 at 09:15:39PM IDT, vivien.didelot@savoirfairelinux.com wrote:
> >Hi,
> >
> >On Oct. Monday 12 (42) 08:51 PM, Ido Schimmel wrote:
> >> Mon, Oct 12, 2015 at 02:41:09PM IDT, razor@blackwall.org wrote:
> >> >From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> >> >
> >> >As Ido Schimmel pointed out the vlan_vid_del() loop in nbp_vlan_flush is
> >> >unnecessary (and is actually a remnant of the old vlan code) so we can
> >> >remove it and combine both br/nbp vlan_flush functions into one.
> >> Just a small note to Scott and Vivien:
> >> 
> >> One of the side effects of Nik's recent patchsets is that when VLANs are
> >> flushed on a port the deletion is propagated to the driver via
> >> switchdev ops, as __vlan_vid_del is called.
> >> 
> >> Therefore there is no need to do internal bookkeeping and remove VLANs
> >> yourself when port is removed from bridge.
> >
> >I was thinking about caching VLAN entries in the mv88e6xxx driver to
> >improve look up on VLAN and FDB operations, but it's a bit prematurate.
> >
> >But when VLAN are flushed, we still need to remove them from the
> >hardware table, right?
> Hi,
> 
> Not sure I'm following. You'll simply get a SWITCHDEV_OBJ_ID_PORT_VLAN
> (del) for each VLAN configured on the port you just removed from the bridge.
> I guess you remove them from your hardware table in the implementation
> of these ops?

Yes we do remove VLAN entries from the hardware table when
switchdev_port_obj_del(SWITCHDEV_OBJ_ID_PORT_VLAN) is called.

I may have misunderstood your previous note, I thought we were talking
about implementing the flush operation in switchdev drivers.

> >
> >Flushing is interesting though, most hardware have flush operations and
> >it would be interesting to have switchdev fdb_flush and vlan_flush ops.

Thanks,
-v

Re: [PATCH net-next 4/4] bridge: vlan: combine (br|nbp)_vlan_flush into one

[Scott Feldman]

On Mon, Oct 12, 2015 at 10:51 AM, Ido Schimmel <idosch@mellanox.com> wrote:
> Mon, Oct 12, 2015 at 02:41:09PM IDT, razor@blackwall.org wrote:
>>From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>>
>>As Ido Schimmel pointed out the vlan_vid_del() loop in nbp_vlan_flush is
>>unnecessary (and is actually a remnant of the old vlan code) so we can
>>remove it and combine both br/nbp vlan_flush functions into one.
> Just a small note to Scott and Vivien:
>
> One of the side effects of Nik's recent patchsets is that when VLANs are
> flushed on a port the deletion is propagated to the driver via
> switchdev ops, as __vlan_vid_del is called.
>
> Therefore there is no need to do internal bookkeeping and remove VLANs
> yourself when port is removed from bridge.

Thanks for the heads-up.  Seems like a nice result.