Re: [Bridge] [RFC PATCH v2 net-next 0/3] Allow bridge to function in non-promisc mode

[Vlad Yasevich <vyasevic@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2133 bytes --]

On 03/08/2013 10:17 AM, Vlad Yasevich wrote:
> On 03/08/2013 12:43 AM, Stephen Hemminger wrote:
>> On Thu,  7 Mar 2013 16:28:45 -0500
>> Vlad Yasevich <vyasevic@redhat.com> wrote:
>>
>>> The series adds an ability to configure the bridge into a
>>> non-primiscuous
>>> mode.   Instead, it provides the ability to identitfy some set of bridge
>>> ports as uplinks and allows for MAC addresses to be programmed onto
>>> those ports.  In case the port hardware does not support mac filter,
>>> that port will be placed in promiscuous mode.
>>>
>>> Default bridge operation continues to remain as "promiscuous".  The new
>>> functionality has to be enabled via sysfs (similar to other bridge
>>> extensions).
>>>
>>> The uplink mode is implemented as a flag on a bridge port.  The api to
>>> change that flag follows the existing api to enable/disable other
>>> existing
>>> flags.
>>>
>>> All comments are welcome.
>>>
>>
>> Can we make this a one step process and less visible to the user.
>> If user defines an uplink device, and the uplink device is capable of
>> filtering
>> (and what ever other pre-conditions people can think of), then the
>> bridge will
>> transparently switch to uplink/non-promisc mode.  This can also be
>> used to trigger
>> edge only mode in RSTP in the future.
>>
>> Less knobs.
>>
>
> Ok.  Let me see what I can do.

So I started working through this and realized that this complicates the 
code significantly.

* I have to re-introduce the uplink-list since now I need to track
"filter capable" uplinks in addition to non-capable ones.
* The really nice and simple sysfs interface to set a flag turns into 
something that duplicates code.
* The bridge port removal can effect the promiscuity setting of the 
bridge if the last uplink is removed.
* We lose the ability to run a promisc edge bridge with uplinks.

I am really starting wonder if this is any better?  The changes
are much bigger and more complex while the functional flexibility is
reduced.  Is it really worth removing a configuration knob?

I've attached an in-progress patch to demonstrate the above.

-vlad

-vlad
>
> Thanks
> -vlad
>


[-- Attachment #2: test.patch --]
[-- Type: text/x-patch, Size: 8158 bytes --]

diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index c4edfe1..b9444e9 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -220,6 +220,7 @@ enum {
 	IFLA_BRPORT_GUARD,	/* bpdu guard              */
 	IFLA_BRPORT_PROTECT,	/* root port protection    */
 	IFLA_BRPORT_FAST_LEAVE,	/* multicast fast leave    */
+	IFLA_BRPORT_UPLINK,	/* uplink port */
 	__IFLA_BRPORT_MAX
 };
 #define IFLA_BRPORT_MAX (__IFLA_BRPORT_MAX - 1)
diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index d5f1d3f..df0afb2 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -356,6 +356,7 @@ void br_dev_setup(struct net_device *dev)
 	br->dev = dev;
 	spin_lock_init(&br->lock);
 	INIT_LIST_HEAD(&br->port_list);
+	INIT_LIST_HEAD(&br->uplink_list);
 	spin_lock_init(&br->hash_lock);
 
 	br->bridge_id.prio[0] = 0x80;
@@ -371,6 +372,7 @@ void br_dev_setup(struct net_device *dev)
 	br->bridge_hello_time = br->hello_time = 2 * HZ;
 	br->bridge_forward_delay = br->forward_delay = 15 * HZ;
 	br->ageing_time = 300 * HZ;
+	br->promisc_enabled = 1;
 
 	br_netfilter_rtable_init(br);
 	br_stp_timer_init(br);
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index ef1b914..8a3a085 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -60,6 +60,29 @@ static int port_cost(struct net_device *dev)
 	return 100;	/* assume old 10Mbps */
 }
 
+static void br_set_promisc(struct net_bridge *br, int val)
+{
+	struct net_bridge_port *p;
+
+	if (val) {
+		br->promisc_enabled = 1;
+		/* For each port on the bridge, turn on promisc mode and
+		 * turn off ALLMULTI to handle multicast traffic.
+		 */
+		list_for_each_entry(p, &br->port_list, list) {
+			dev_set_promiscuity(p->dev, 1);
+			dev_set_allmulti(p->dev, -1);
+		}
+
+	} else {
+		br->promisc_enabled = 0;
+		/* For each port on the bridge, turn off promisc mode */
+		list_for_each_entry(p, &br->port_list, list) {
+			dev_set_allmulti(p->dev, 1);
+			dev_set_promiscuity(p->dev, -1);
+		}
+	}
+}
 
 /* Check for port carrier transistions. */
 void br_port_carrier_check(struct net_bridge_port *p)
@@ -132,7 +155,14 @@ static void del_nbp(struct net_bridge_port *p)
 
 	sysfs_remove_link(br->ifobj, p->dev->name);
 
-	dev_set_promiscuity(dev, -1);
+	if (br->promisc_enabled)
+		dev_set_promiscuity(dev, -1);
+	else
+		dev_set_allmulti(dev, -1);
+
+	list_del_rcu(&p->uplink_list);
+	if (list_empty(&br->uplink_list))
+		br_set_promisc(br, 1);
 
 	spin_lock_bh(&br->lock);
 	br_stp_disable_port(p);
@@ -222,6 +252,7 @@ static struct net_bridge_port *new_nbp(struct net_bridge *br,
 	p->priority = 0x8000 >> BR_PORT_BITS;
 	p->port_no = index;
 	p->flags = 0;
+	INIT_LIST_HEAD(&p->uplink_list);
 	br_init_port(p);
 	p->state = BR_STATE_DISABLED;
 	br_stp_port_timer_init(p);
@@ -350,9 +381,15 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
 
 	call_netdevice_notifiers(NETDEV_JOIN, dev);
 
-	err = dev_set_promiscuity(dev, 1);
-	if (err)
-		goto put_back;
+	if (br->promisc_enabled) {
+		err = dev_set_promiscuity(dev, 1);
+		if (err)
+			goto put_back;
+	} else {
+		err = dev_set_allmulti(dev, 1);
+		if (err)
+			goto put_back;
+	}
 
 	err = kobject_init_and_add(&p->kobj, &brport_ktype, &(dev->dev.kobj),
 				   SYSFS_BRIDGE_PORT_ATTR);
@@ -414,7 +451,10 @@ err2:
 	kobject_put(&p->kobj);
 	p = NULL; /* kobject_put frees */
 err1:
-	dev_set_promiscuity(dev, -1);
+	if (br->promisc_enabled)
+		dev_set_promiscuity(dev, -1);
+	else
+		dev_set_allmulti(dev, -1);
 put_back:
 	dev_put(dev);
 	kfree(p);
@@ -449,6 +489,30 @@ int br_del_if(struct net_bridge *br, struct net_device *dev)
 	return 0;
 }
 
+void br_set_uplink(struct net_bridge_port *p)
+{
+	if (p->flags & BR_UPLINK) {
+		if (p->dev->priv_flags & IFF_UNICAST_FLT) {
+			/* Turn off promisc mode if this is the first
+			 * filter capable uplink
+			 */
+			if (list_empty(&p->br->uplink_list))
+				br_set_promisc(p->br, 0);
+
+			list_add_rcu(&p->uplink_list, &p->br->uplink_list);
+		}
+	} else {
+		if (p->dev->priv_flags & IFF_UNICAST_FLT) {
+			list_del_rcu(&p->uplink_list);
+			/* Turn on promisc mode if this was the last
+			 * filter capable uplink
+			 */
+			if (list_empty(&p->br->uplink_list))
+				br_set_promisc(p->br, 1);
+		}
+	}
+}
+
 void __net_exit br_net_exit(struct net *net)
 {
 	struct net_device *dev;
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index 27aa3ee..e53eb0b 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -283,6 +283,7 @@ static const struct nla_policy ifla_brport_policy[IFLA_BRPORT_MAX + 1] = {
 	[IFLA_BRPORT_MODE]	= { .type = NLA_U8 },
 	[IFLA_BRPORT_GUARD]	= { .type = NLA_U8 },
 	[IFLA_BRPORT_PROTECT]	= { .type = NLA_U8 },
+	[IFLA_BRPORT_UPLINK]	= { .type = NLA_U8 },
 };
 
 /* Change the state of the port and notify spanning tree */
@@ -325,10 +326,12 @@ static void br_set_port_flag(struct net_bridge_port *p, struct nlattr *tb[],
 static int br_setport(struct net_bridge_port *p, struct nlattr *tb[])
 {
 	int err;
+	unsigned long flags = p->flags;
 
 	br_set_port_flag(p, tb, IFLA_BRPORT_MODE, BR_HAIRPIN_MODE);
 	br_set_port_flag(p, tb, IFLA_BRPORT_GUARD, BR_BPDU_GUARD);
 	br_set_port_flag(p, tb, IFLA_BRPORT_FAST_LEAVE, BR_MULTICAST_FAST_LEAVE);
+	br_set_port_flag(p, tb, IFLA_BRPORT_UPLINK, BR_UPLINK);
 
 	if (tb[IFLA_BRPORT_COST]) {
 		err = br_stp_set_path_cost(p, nla_get_u32(tb[IFLA_BRPORT_COST]));
@@ -347,6 +350,11 @@ static int br_setport(struct net_bridge_port *p, struct nlattr *tb[])
 		if (err)
 			return err;
 	}
+
+	/* If the uplink flag has changed do additional processing */
+	if ((flags ^ p->flags) & BR_UPLINK)
+		br_set_uplink(p);
+
 	return 0;
 }
 
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 6d314c4..ab8cb1a9 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -130,6 +130,7 @@ struct net_bridge_port
 	struct net_bridge		*br;
 	struct net_device		*dev;
 	struct list_head		list;
+	struct list_head		uplink_list;
 
 	/* STP */
 	u8				priority;
@@ -156,6 +157,7 @@ struct net_bridge_port
 #define BR_BPDU_GUARD           0x00000002
 #define BR_ROOT_BLOCK		0x00000004
 #define BR_MULTICAST_FAST_LEAVE	0x00000008
+#define BR_UPLINK		0x00000010
 
 #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
 	u32				multicast_startup_queries_sent;
@@ -277,6 +279,8 @@ struct net_bridge
 	struct timer_list		topology_change_timer;
 	struct timer_list		gc_timer;
 	struct kobject			*ifobj;
+	struct list_head		uplink_list;
+ 	u8				promisc_enabled;
 #ifdef CONFIG_BRIDGE_VLAN_FILTERING
 	u8				vlan_enabled;
 	struct net_port_vlans __rcu	*vlan_info;
@@ -426,6 +430,7 @@ extern int br_del_if(struct net_bridge *br,
 extern int br_min_mtu(const struct net_bridge *br);
 extern netdev_features_t br_features_recompute(struct net_bridge *br,
 	netdev_features_t features);
+extern void br_set_uplink(struct net_bridge_port *port);
 
 /* br_input.c */
 extern int br_handle_frame_finish(struct sk_buff *skb);
diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
index a1ef1b6..454a0cf 100644
--- a/net/bridge/br_sysfs_if.c
+++ b/net/bridge/br_sysfs_if.c
@@ -159,6 +159,29 @@ BRPORT_ATTR_FLAG(hairpin_mode, BR_HAIRPIN_MODE);
 BRPORT_ATTR_FLAG(bpdu_guard, BR_BPDU_GUARD);
 BRPORT_ATTR_FLAG(root_block, BR_ROOT_BLOCK);
 
+static ssize_t show_uplink(struct net_bridge_port *p, char *buf)
+{
+	return sprintf(buf, "%d\n", !!(p->flags & BR_UPLINK));
+}
+
+static int store_uplink(struct net_bridge_port *p, unsigned long v)
+{
+	unsigned long flags = p->flags;
+
+	if (v)
+		flags |= BR_UPLINK;
+	else
+		flags &= ~BR_UPLINK;
+
+	if (flags != p->flags) {
+		p->flags = flags;
+		br_set_uplink(p);
+		br_ifinfo_notify(RTM_NEWLINK, p);
+	}
+	return 0;
+}
+static BRPORT_ATTR(uplink, S_IRUGO | S_IWUSR, show_uplink, store_uplink);
+
 #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
 static ssize_t show_multicast_router(struct net_bridge_port *p, char *buf)
 {
@@ -195,6 +218,7 @@ static const struct brport_attribute *brport_attrs[] = {
 	&brport_attr_hairpin_mode,
 	&brport_attr_bpdu_guard,
 	&brport_attr_root_block,
+	&brport_attr_uplink,
 #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
 	&brport_attr_multicast_router,
 	&brport_attr_multicast_fast_leave,