[RFC PATCH v2 net-next 0/7] Non-promisc bidge ports support

[RFC PATCH v2 net-next 0/7] Non-promisc bidge ports support

[Vlad Yasevich]

This patch series is a re-implementation of prior attempts to support
non-promiscuous bridge ports.

The basic concept is the same as before.  The bridge keeps
track of the ports that support learning and flooding packets
to unknown destinations.  We call these ports auto-discovery
ports since they automatically discover who is behind them through
learning and flooding.  

If flooding and learning are disabled via flags, then the port
requires static configuration to tell it which mac addresses
are behind it.  This is accomplished through adding of fdbs.
These fdbs should be static as dynamic fdbs can expire and systems
will become unreachable due to lack of flooding.

If the user marks all ports are needing static configuration then
we can safely make them non-promiscuous, we will know all the
information about them.

If the user leaves only 1 port as automatic, then we can mark
that port as not-promiscuous as well.  One could think of
this a edge relay similar to what's support by embedded switches
in SRIOV devices.  Since we have all the information about the
other ports, we can just program the mac addresses into the
single automatic port to receive all necessary traffic.

In other cases, we keep all ports promiscuous as before.

There are some other cases when promiscuous mode has to be turned
back on.  One is when the bridge itself if placed in promiscuous
mode (use sets promisc flag).  The other is if vlan filtering is
turned off.  Since this is the default configuration, the default
bridge operation is not changed.

Changes since v1:
 - Removed private list.  We now traverse the fdb hashtable itself
   to write necessary addresses to the ports (Stephen's concern)
 - Add learning flag to the mask for flags that decides if the port
   is 'auto' or not (suggest by MST and Jamal).
 - Simplified tracking of such ports at the cost of a loop over all
   ports (suggested by MST)

I've played with quite a large number of ports and the current approach
seems to work fairly well.

Thanks
-vlad

Vlad Yasevich (7):
  bridge: Turn flag change macro into a function.
  bridge: Keep track of ports capable of automatic discovery.
  bridge: Add functionality to sync static fdb entries to hw
  bridge: Automatically manage port promiscuous mode.
  bridge: Add addresses from static fdbs to non-promisc ports
  bridge: Correctly manage promiscuity when user requested it.
  bridge: Automatically manage promisc mode when vlan filtering is on.

 net/bridge/br_device.c   |   7 +++
 net/bridge/br_fdb.c      | 132 ++++++++++++++++++++++++++++++++++++++++++++---
 net/bridge/br_if.c       | 119 ++++++++++++++++++++++++++++++++++++++++--
 net/bridge/br_netlink.c  |   3 ++
 net/bridge/br_private.h  |  20 +++++++
 net/bridge/br_sysfs_if.c |  31 +++++++----
 net/bridge/br_vlan.c     |   1 +
 7 files changed, 293 insertions(+), 20 deletions(-)

-- 
1.9.0

[Bridge] [RFC PATCH v2 net-next 0/7] Non-promisc bidge ports support

[Vlad Yasevich]

This patch series is a re-implementation of prior attempts to support
non-promiscuous bridge ports.

The basic concept is the same as before.  The bridge keeps
track of the ports that support learning and flooding packets
to unknown destinations.  We call these ports auto-discovery
ports since they automatically discover who is behind them through
learning and flooding.  

If flooding and learning are disabled via flags, then the port
requires static configuration to tell it which mac addresses
are behind it.  This is accomplished through adding of fdbs.
These fdbs should be static as dynamic fdbs can expire and systems
will become unreachable due to lack of flooding.

If the user marks all ports are needing static configuration then
we can safely make them non-promiscuous, we will know all the
information about them.

If the user leaves only 1 port as automatic, then we can mark
that port as not-promiscuous as well.  One could think of
this a edge relay similar to what's support by embedded switches
in SRIOV devices.  Since we have all the information about the
other ports, we can just program the mac addresses into the
single automatic port to receive all necessary traffic.

In other cases, we keep all ports promiscuous as before.

There are some other cases when promiscuous mode has to be turned
back on.  One is when the bridge itself if placed in promiscuous
mode (use sets promisc flag).  The other is if vlan filtering is
turned off.  Since this is the default configuration, the default
bridge operation is not changed.

Changes since v1:
 - Removed private list.  We now traverse the fdb hashtable itself
   to write necessary addresses to the ports (Stephen's concern)
 - Add learning flag to the mask for flags that decides if the port
   is 'auto' or not (suggest by MST and Jamal).
 - Simplified tracking of such ports at the cost of a loop over all
   ports (suggested by MST)

I've played with quite a large number of ports and the current approach
seems to work fairly well.

Thanks
-vlad

Vlad Yasevich (7):
  bridge: Turn flag change macro into a function.
  bridge: Keep track of ports capable of automatic discovery.
  bridge: Add functionality to sync static fdb entries to hw
  bridge: Automatically manage port promiscuous mode.
  bridge: Add addresses from static fdbs to non-promisc ports
  bridge: Correctly manage promiscuity when user requested it.
  bridge: Automatically manage promisc mode when vlan filtering is on.

 net/bridge/br_device.c   |   7 +++
 net/bridge/br_fdb.c      | 132 ++++++++++++++++++++++++++++++++++++++++++++---
 net/bridge/br_if.c       | 119 ++++++++++++++++++++++++++++++++++++++++--
 net/bridge/br_netlink.c  |   3 ++
 net/bridge/br_private.h  |  20 +++++++
 net/bridge/br_sysfs_if.c |  31 +++++++----
 net/bridge/br_vlan.c     |   1 +
 7 files changed, 293 insertions(+), 20 deletions(-)

-- 
1.9.0

[RFC PATCH v2 net-next 1/7] bridge: Turn flag change macro into a function.

[Vlad Yasevich]

Turn the flag change macro into a function to allow
easier updates and to reduce space.

Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
 net/bridge/br_sysfs_if.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
index dd595bd..112a25e 100644
--- a/net/bridge/br_sysfs_if.c
+++ b/net/bridge/br_sysfs_if.c
@@ -41,20 +41,27 @@ static ssize_t show_##_name(struct net_bridge_port *p, char *buf) \
 }								\
 static int store_##_name(struct net_bridge_port *p, unsigned long v) \
 {								\
-	unsigned long flags = p->flags;				\
-	if (v)							\
-		flags |= _mask;					\
-	else							\
-		flags &= ~_mask;				\
-	if (flags != p->flags) {				\
-		p->flags = flags;				\
-		br_ifinfo_notify(RTM_NEWLINK, p);		\
-	}							\
-	return 0;						\
+	return store_flag(p, v, _mask);				\
 }								\
 static BRPORT_ATTR(_name, S_IRUGO | S_IWUSR,			\
 		   show_##_name, store_##_name)
 
+static int store_flag(struct net_bridge_port *p, unsigned long v,
+		     unsigned long mask)
+{
+	unsigned long flags = p->flags;
+
+	if (v)
+		flags |= mask;
+	else
+		flags &= ~mask;
+
+	if (flags != p->flags) {
+		p->flags = flags;
+		br_ifinfo_notify(RTM_NEWLINK, p);
+	}
+	return 0;
+}
 
 static ssize_t show_path_cost(struct net_bridge_port *p, char *buf)
 {
-- 
1.9.0

[Bridge] [RFC PATCH v2 net-next 1/7] bridge: Turn flag change macro into a function.

[Vlad Yasevich]

Turn the flag change macro into a function to allow
easier updates and to reduce space.

Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
 net/bridge/br_sysfs_if.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
index dd595bd..112a25e 100644
--- a/net/bridge/br_sysfs_if.c
+++ b/net/bridge/br_sysfs_if.c
@@ -41,20 +41,27 @@ static ssize_t show_##_name(struct net_bridge_port *p, char *buf) \
 }								\
 static int store_##_name(struct net_bridge_port *p, unsigned long v) \
 {								\
-	unsigned long flags = p->flags;				\
-	if (v)							\
-		flags |= _mask;					\
-	else							\
-		flags &= ~_mask;				\
-	if (flags != p->flags) {				\
-		p->flags = flags;				\
-		br_ifinfo_notify(RTM_NEWLINK, p);		\
-	}							\
-	return 0;						\
+	return store_flag(p, v, _mask);				\
 }								\
 static BRPORT_ATTR(_name, S_IRUGO | S_IWUSR,			\
 		   show_##_name, store_##_name)
 
+static int store_flag(struct net_bridge_port *p, unsigned long v,
+		     unsigned long mask)
+{
+	unsigned long flags = p->flags;
+
+	if (v)
+		flags |= mask;
+	else
+		flags &= ~mask;
+
+	if (flags != p->flags) {
+		p->flags = flags;
+		br_ifinfo_notify(RTM_NEWLINK, p);
+	}
+	return 0;
+}
 
 static ssize_t show_path_cost(struct net_bridge_port *p, char *buf)
 {
-- 
1.9.0

[RFC PATCH v2 net-next 2/7] bridge: Keep track of ports capable of automatic discovery.

[Vlad Yasevich]

By default, ports on the bridge are capable of automatic
discovery of nodes located behind the port.  This is accomplished
via flooding of unknown traffic (BR_FLOOD) and learning the
mac addresses from these packets (BR_LEARNING).
If the above functionality is diabled by turning off these
flags, the port requires static configuration in the form
of static FDB entries to function properly.

This patch adds functionality to keep track of all ports
capable of automatic discovery.  This will later be used
to control promiscuity settings.

Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
 net/bridge/br_if.c       | 27 +++++++++++++++++++++++++++
 net/bridge/br_netlink.c  |  3 +++
 net/bridge/br_private.h  |  6 ++++++
 net/bridge/br_sysfs_if.c |  6 +++++-
 4 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 5262b86..7e60c4c 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -85,6 +85,18 @@ void br_port_carrier_check(struct net_bridge_port *p)
 	spin_unlock_bh(&br->lock);
 }
 
+static void nbp_update_port_count(struct net_bridge *br)
+{
+	struct net_bridge_port *p;
+	u32 cnt = 0;
+
+	list_for_each_entry(p, &br->port_list, list) {
+		if (br_is_auto_port(p))
+			cnt++;
+	}
+	br->auto_cnt = cnt;
+}
+
 static void release_nbp(struct kobject *kobj)
 {
 	struct net_bridge_port *p
@@ -146,6 +158,8 @@ static void del_nbp(struct net_bridge_port *p)
 
 	list_del_rcu(&p->list);
 
+	nbp_update_port_count(br);
+
 	dev->priv_flags &= ~IFF_BRIDGE_PORT;
 
 	netdev_rx_handler_unregister(dev);
@@ -384,6 +398,8 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
 
 	list_add_rcu(&p->list, &br->port_list);
 
+	nbp_update_port_count(br);
+
 	netdev_update_features(br->dev);
 
 	if (br->dev->needed_headroom < dev->needed_headroom)
@@ -455,3 +471,14 @@ int br_del_if(struct net_bridge *br, struct net_device *dev)
 
 	return 0;
 }
+
+void br_port_flags_change(struct net_bridge_port *p, unsigned long old_flags)
+{
+	struct net_bridge *br = p->br;
+	unsigned long mask = old_flags ^ p->flags;
+
+	if (!(mask & BR_AUTO_MASK))
+		return;
+
+	nbp_update_port_count(br);
+}
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index e74b6d53..01382b9 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -328,6 +328,7 @@ 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 old_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);
@@ -353,6 +354,8 @@ static int br_setport(struct net_bridge_port *p, struct nlattr *tb[])
 		if (err)
 			return err;
 	}
+
+	br_port_flags_change(p, old_flags ^ p->flags);
 	return 0;
 }
 
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 06811d7..db714a1 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -174,6 +174,7 @@ struct net_bridge_port
 #define BR_ADMIN_COST		0x00000010
 #define BR_LEARNING		0x00000020
 #define BR_FLOOD		0x00000040
+#define BR_AUTO_MASK (BR_FLOOD | BR_LEARNING)
 
 #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
 	struct bridge_mcast_query	ip4_query;
@@ -198,6 +199,8 @@ struct net_bridge_port
 #endif
 };
 
+#define br_is_auto_port(p) (((p)->flags & BR_AUTO_MASK) == BR_AUTO_MASK)
+
 #define br_port_exists(dev) (dev->priv_flags & IFF_BRIDGE_PORT)
 
 static inline struct net_bridge_port *br_port_get_rcu(const struct net_device *dev)
@@ -290,6 +293,7 @@ struct net_bridge
 	struct timer_list		topology_change_timer;
 	struct timer_list		gc_timer;
 	struct kobject			*ifobj;
+	u32				auto_cnt;
 #ifdef CONFIG_BRIDGE_VLAN_FILTERING
 	u8				vlan_enabled;
 	struct net_port_vlans __rcu	*vlan_info;
@@ -415,6 +419,8 @@ int br_del_if(struct net_bridge *br, struct net_device *dev);
 int br_min_mtu(const struct net_bridge *br);
 netdev_features_t br_features_recompute(struct net_bridge *br,
 					netdev_features_t features);
+void br_port_flags_change(struct net_bridge_port *port,
+			  unsigned long old_flags);
 
 /* br_input.c */
 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 112a25e..58c26e3 100644
--- a/net/bridge/br_sysfs_if.c
+++ b/net/bridge/br_sysfs_if.c
@@ -49,7 +49,10 @@ static BRPORT_ATTR(_name, S_IRUGO | S_IWUSR,			\
 static int store_flag(struct net_bridge_port *p, unsigned long v,
 		     unsigned long mask)
 {
-	unsigned long flags = p->flags;
+	unsigned long flags;
+	unsigned long old_flags;
+
+	old_flags = flags = p->flags;
 
 	if (v)
 		flags |= mask;
@@ -58,6 +61,7 @@ static int store_flag(struct net_bridge_port *p, unsigned long v,
 
 	if (flags != p->flags) {
 		p->flags = flags;
+		br_port_flags_change(p, old_flags ^ flags);
 		br_ifinfo_notify(RTM_NEWLINK, p);
 	}
 	return 0;
-- 
1.9.0

[Bridge] [RFC PATCH v2 net-next 2/7] bridge: Keep track of ports capable of automatic discovery.

[Vlad Yasevich]

By default, ports on the bridge are capable of automatic
discovery of nodes located behind the port.  This is accomplished
via flooding of unknown traffic (BR_FLOOD) and learning the
mac addresses from these packets (BR_LEARNING).
If the above functionality is diabled by turning off these
flags, the port requires static configuration in the form
of static FDB entries to function properly.

This patch adds functionality to keep track of all ports
capable of automatic discovery.  This will later be used
to control promiscuity settings.

Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
 net/bridge/br_if.c       | 27 +++++++++++++++++++++++++++
 net/bridge/br_netlink.c  |  3 +++
 net/bridge/br_private.h  |  6 ++++++
 net/bridge/br_sysfs_if.c |  6 +++++-
 4 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 5262b86..7e60c4c 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -85,6 +85,18 @@ void br_port_carrier_check(struct net_bridge_port *p)
 	spin_unlock_bh(&br->lock);
 }
 
+static void nbp_update_port_count(struct net_bridge *br)
+{
+	struct net_bridge_port *p;
+	u32 cnt = 0;
+
+	list_for_each_entry(p, &br->port_list, list) {
+		if (br_is_auto_port(p))
+			cnt++;
+	}
+	br->auto_cnt = cnt;
+}
+
 static void release_nbp(struct kobject *kobj)
 {
 	struct net_bridge_port *p
@@ -146,6 +158,8 @@ static void del_nbp(struct net_bridge_port *p)
 
 	list_del_rcu(&p->list);
 
+	nbp_update_port_count(br);
+
 	dev->priv_flags &= ~IFF_BRIDGE_PORT;
 
 	netdev_rx_handler_unregister(dev);
@@ -384,6 +398,8 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
 
 	list_add_rcu(&p->list, &br->port_list);
 
+	nbp_update_port_count(br);
+
 	netdev_update_features(br->dev);
 
 	if (br->dev->needed_headroom < dev->needed_headroom)
@@ -455,3 +471,14 @@ int br_del_if(struct net_bridge *br, struct net_device *dev)
 
 	return 0;
 }
+
+void br_port_flags_change(struct net_bridge_port *p, unsigned long old_flags)
+{
+	struct net_bridge *br = p->br;
+	unsigned long mask = old_flags ^ p->flags;
+
+	if (!(mask & BR_AUTO_MASK))
+		return;
+
+	nbp_update_port_count(br);
+}
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index e74b6d53..01382b9 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -328,6 +328,7 @@ 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 old_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);
@@ -353,6 +354,8 @@ static int br_setport(struct net_bridge_port *p, struct nlattr *tb[])
 		if (err)
 			return err;
 	}
+
+	br_port_flags_change(p, old_flags ^ p->flags);
 	return 0;
 }
 
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 06811d7..db714a1 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -174,6 +174,7 @@ struct net_bridge_port
 #define BR_ADMIN_COST		0x00000010
 #define BR_LEARNING		0x00000020
 #define BR_FLOOD		0x00000040
+#define BR_AUTO_MASK (BR_FLOOD | BR_LEARNING)
 
 #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
 	struct bridge_mcast_query	ip4_query;
@@ -198,6 +199,8 @@ struct net_bridge_port
 #endif
 };
 
+#define br_is_auto_port(p) (((p)->flags & BR_AUTO_MASK) == BR_AUTO_MASK)
+
 #define br_port_exists(dev) (dev->priv_flags & IFF_BRIDGE_PORT)
 
 static inline struct net_bridge_port *br_port_get_rcu(const struct net_device *dev)
@@ -290,6 +293,7 @@ struct net_bridge
 	struct timer_list		topology_change_timer;
 	struct timer_list		gc_timer;
 	struct kobject			*ifobj;
+	u32				auto_cnt;
 #ifdef CONFIG_BRIDGE_VLAN_FILTERING
 	u8				vlan_enabled;
 	struct net_port_vlans __rcu	*vlan_info;
@@ -415,6 +419,8 @@ int br_del_if(struct net_bridge *br, struct net_device *dev);
 int br_min_mtu(const struct net_bridge *br);
 netdev_features_t br_features_recompute(struct net_bridge *br,
 					netdev_features_t features);
+void br_port_flags_change(struct net_bridge_port *port,
+			  unsigned long old_flags);
 
 /* br_input.c */
 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 112a25e..58c26e3 100644
--- a/net/bridge/br_sysfs_if.c
+++ b/net/bridge/br_sysfs_if.c
@@ -49,7 +49,10 @@ static BRPORT_ATTR(_name, S_IRUGO | S_IWUSR,			\
 static int store_flag(struct net_bridge_port *p, unsigned long v,
 		     unsigned long mask)
 {
-	unsigned long flags = p->flags;
+	unsigned long flags;
+	unsigned long old_flags;
+
+	old_flags = flags = p->flags;
 
 	if (v)
 		flags |= mask;
@@ -58,6 +61,7 @@ static int store_flag(struct net_bridge_port *p, unsigned long v,
 
 	if (flags != p->flags) {
 		p->flags = flags;
+		br_port_flags_change(p, old_flags ^ flags);
 		br_ifinfo_notify(RTM_NEWLINK, p);
 	}
 	return 0;
-- 
1.9.0

[RFC PATCH v2 net-next 3/7] bridge: Add functionality to sync static fdb entries to hw

[Vlad Yasevich]

Add code that allows static fdb entires to be synced to the
hw list for a specified port.  This will be used later to
program ports that can function in non-promiscous mode.

Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
 net/bridge/br_fdb.c     | 56 +++++++++++++++++++++++++++++++++++++++++++++++++
 net/bridge/br_private.h |  2 ++
 2 files changed, 58 insertions(+)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 9203d5a..9759b61 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -874,3 +874,59 @@ int br_fdb_delete(struct ndmsg *ndm, struct nlattr *tb[],
 out:
 	return err;
 }
+
+int br_fdb_sync_static(struct net_bridge *br, struct net_bridge_port *p)
+{
+	struct net_bridge_fdb_entry *fdb, *tmp = NULL;
+	int i;
+	int err = 0;
+
+	ASSERT_RTNL();
+
+	for (i = 0; i < BR_HASH_SIZE; i++) {
+		hlist_for_each_entry(fdb, &br->hash[i], hlist) {
+			/* We only care for static entries */
+			if (!fdb->is_static)
+				continue;
+
+			err = dev_uc_add(p->dev, fdb->addr.addr);
+			if (err)
+				goto rollback;
+		}
+	}
+	return 0;
+
+rollback:
+	for (i = 0; i < BR_HASH_SIZE; i++) {
+		hlist_for_each_entry(tmp, &br->hash[i], hlist) {
+			/* If we reached the fdb that failed, we can stop */
+			if (tmp == fdb)
+				break;
+
+			/* We only care for static entries */
+			if (!tmp->is_static)
+				continue;
+
+			dev_uc_del(p->dev, tmp->addr.addr);
+		}
+	}
+	return err;
+}
+
+void br_fdb_unsync_static(struct net_bridge *br, struct net_bridge_port *p)
+{
+	struct net_bridge_fdb_entry *fdb;
+	int i;
+
+	ASSERT_RTNL();
+
+	for (i = 0; i < BR_HASH_SIZE; i++) {
+		hlist_for_each_entry_rcu(fdb, &br->hash[i], hlist) {
+			/* We only care for static entries */
+			if (!fdb->is_static)
+				continue;
+
+			dev_uc_del(p->dev, fdb->addr.addr);
+		}
+	}
+}
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index db714a1..2023671 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -399,6 +399,8 @@ int br_fdb_add(struct ndmsg *nlh, struct nlattr *tb[], struct net_device *dev,
 	       const unsigned char *addr, u16 nlh_flags);
 int br_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb,
 		struct net_device *dev, int idx);
+int br_fdb_sync_static(struct net_bridge *br, struct net_bridge_port *p);
+void br_fdb_unsync_static(struct net_bridge *br, struct net_bridge_port *p);
 
 /* br_forward.c */
 void br_deliver(const struct net_bridge_port *to, struct sk_buff *skb);
-- 
1.9.0

[Bridge] [RFC PATCH v2 net-next 3/7] bridge: Add functionality to sync static fdb entries to hw

[Vlad Yasevich]

Add code that allows static fdb entires to be synced to the
hw list for a specified port.  This will be used later to
program ports that can function in non-promiscous mode.

Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
 net/bridge/br_fdb.c     | 56 +++++++++++++++++++++++++++++++++++++++++++++++++
 net/bridge/br_private.h |  2 ++
 2 files changed, 58 insertions(+)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 9203d5a..9759b61 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -874,3 +874,59 @@ int br_fdb_delete(struct ndmsg *ndm, struct nlattr *tb[],
 out:
 	return err;
 }
+
+int br_fdb_sync_static(struct net_bridge *br, struct net_bridge_port *p)
+{
+	struct net_bridge_fdb_entry *fdb, *tmp = NULL;
+	int i;
+	int err = 0;
+
+	ASSERT_RTNL();
+
+	for (i = 0; i < BR_HASH_SIZE; i++) {
+		hlist_for_each_entry(fdb, &br->hash[i], hlist) {
+			/* We only care for static entries */
+			if (!fdb->is_static)
+				continue;
+
+			err = dev_uc_add(p->dev, fdb->addr.addr);
+			if (err)
+				goto rollback;
+		}
+	}
+	return 0;
+
+rollback:
+	for (i = 0; i < BR_HASH_SIZE; i++) {
+		hlist_for_each_entry(tmp, &br->hash[i], hlist) {
+			/* If we reached the fdb that failed, we can stop */
+			if (tmp == fdb)
+				break;
+
+			/* We only care for static entries */
+			if (!tmp->is_static)
+				continue;
+
+			dev_uc_del(p->dev, tmp->addr.addr);
+		}
+	}
+	return err;
+}
+
+void br_fdb_unsync_static(struct net_bridge *br, struct net_bridge_port *p)
+{
+	struct net_bridge_fdb_entry *fdb;
+	int i;
+
+	ASSERT_RTNL();
+
+	for (i = 0; i < BR_HASH_SIZE; i++) {
+		hlist_for_each_entry_rcu(fdb, &br->hash[i], hlist) {
+			/* We only care for static entries */
+			if (!fdb->is_static)
+				continue;
+
+			dev_uc_del(p->dev, fdb->addr.addr);
+		}
+	}
+}
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index db714a1..2023671 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -399,6 +399,8 @@ int br_fdb_add(struct ndmsg *nlh, struct nlattr *tb[], struct net_device *dev,
 	       const unsigned char *addr, u16 nlh_flags);
 int br_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb,
 		struct net_device *dev, int idx);
+int br_fdb_sync_static(struct net_bridge *br, struct net_bridge_port *p);
+void br_fdb_unsync_static(struct net_bridge *br, struct net_bridge_port *p);
 
 /* br_forward.c */
 void br_deliver(const struct net_bridge_port *to, struct sk_buff *skb);
-- 
1.9.0

[RFC PATCH v2 net-next 4/7] bridge: Automatically manage port promiscuous mode.

[Vlad Yasevich]

When there is only 1 port that can learn mac addresses and flood
unknown traffic, this port can function in non-promiscouse mode.
The simplest example of this configuration is a bridge with only
1 port.  In this case, the bridge can't forward traffic anywhere.
All it can do is recevie traffic destined to it.  It doesn't need
to the interface into promiscouse mode.
A more complicated example is a bridge with 2 or more ports where
only 1 port remains capable of auto-discover and all others disable
mac learning and flooding thus requiring static configuration.  You
could think of this configuraiton as mimicking and edge relay with
1 uplink port (the one can still learn things), and N downlink ports
that have to be manually programmed by user or managment entity.
In this case, the 1 uplink doesn't really have
to be promiscous either since the bridge would have all the necessary
data in the fdb through static configuration.  This one port can be
manually programmed to allow to recieve necessary traffic.
Another case where there is no need for promiscuous mode is when
there are no "proverbial" uplinks and all ports are manually managed.
All necessary information will be provided, so anything an interface
receives in promiscouse mode is extra and could even be considered
security threat.

This patch supports the above scenarios.

Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
 net/bridge/br_if.c      | 85 +++++++++++++++++++++++++++++++++++++++++++++----
 net/bridge/br_private.h |  2 ++
 2 files changed, 81 insertions(+), 6 deletions(-)

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 7e60c4c..5a26ca2 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -85,6 +85,64 @@ void br_port_carrier_check(struct net_bridge_port *p)
 	spin_unlock_bh(&br->lock);
 }
 
+static void br_port_set_promisc(struct net_bridge_port *p)
+{
+	int err = 0;
+
+	if (br_is_promisc_port(p))
+		return;
+
+	err = dev_set_promiscuity(p->dev, 1);
+	if (err)
+		return;
+
+	br_fdb_unsync_static(p->br, p);
+	p->flags |= BR_PROMISC;
+}
+
+static void br_port_clear_promisc(struct net_bridge_port *p)
+{
+	int err;
+
+	if (!br_is_promisc_port(p))
+		return;
+
+	/* Since we'll be clearing the promisc mode, program the port
+	 * first so that we don't have interruption in traffic.
+	 */
+	err = br_fdb_sync_static(p->br, p);
+	if (err)
+		return;
+
+	dev_set_promiscuity(p->dev, -1);
+	p->flags &= ~BR_PROMISC;
+}
+
+/* When a port is added or removed or when certain port flags
+ * change, this function is called to automatically mange
+ * promiscuity setting of all the bridge ports.  We are always called
+ * under RTNL so can skip using rcu primitives.
+ */
+static void br_manage_promisc(struct net_bridge *br)
+{
+	struct net_bridge_port *p;
+
+	/* Algorithm is simple.  If all the port require static
+	 * configuration, we know everything and can simply write
+	 * that down to the ports and clear promisc.
+	 * If only 1 port is automatic and all the others require
+	 * static configuration, we can write all the static data
+	 * to this one automatic port and still make non-promisc.
+	 */
+	list_for_each_entry(p, &br->port_list, list) {
+		if (br->auto_cnt == 0 ||
+		    (br->auto_cnt == 1 && br_is_auto_port(p)))
+			br_port_clear_promisc(p);
+		else
+			br_port_set_promisc(p);
+	}
+}
+
 static void nbp_update_port_count(struct net_bridge *br)
 {
 	struct net_bridge_port *p;
@@ -94,7 +152,23 @@ static void nbp_update_port_count(struct net_bridge *br)
 		if (br_is_auto_port(p))
 			cnt++;
 	}
-	br->auto_cnt = cnt;
+	if (br->auto_cnt != cnt) {
+		br->auto_cnt = cnt;
+		br_manage_promisc(br);
+	}
+}
+
+static void nbp_delete_promisc(struct net_bridge_port *p)
+{
+	/* If port is currently promiscous, unset promiscuity.
+	 * Otherwise, it is a static port so remove all addresses
+	 * from it.
+	 */
+	dev_set_allmulti(p->dev, -1);
+	if (br_is_promisc_port(p))
+		dev_set_promiscuity(p->dev, -1);
+	else
+		br_fdb_unsync_static(p->br, p);
 }
 
 static void release_nbp(struct kobject *kobj)
@@ -145,7 +219,7 @@ static void del_nbp(struct net_bridge_port *p)
 
 	sysfs_remove_link(br->ifobj, p->dev->name);
 
-	dev_set_promiscuity(dev, -1);
+	nbp_delete_promisc(p);
 
 	spin_lock_bh(&br->lock);
 	br_stp_disable_port(p);
@@ -153,11 +227,10 @@ static void del_nbp(struct net_bridge_port *p)
 
 	br_ifinfo_notify(RTM_DELLINK, p);
 
-	nbp_vlan_flush(p);
-	br_fdb_delete_by_port(br, p, 1);
-
 	list_del_rcu(&p->list);
 
+	nbp_vlan_flush(p);
+	br_fdb_delete_by_port(br, p, 1);
 	nbp_update_port_count(br);
 
 	dev->priv_flags &= ~IFF_BRIDGE_PORT;
@@ -367,7 +440,7 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
 
 	call_netdevice_notifiers(NETDEV_JOIN, dev);
 
-	err = dev_set_promiscuity(dev, 1);
+	err = dev_set_allmulti(dev, 1);
 	if (err)
 		goto put_back;
 
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 2023671..1c794fef 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -175,6 +175,7 @@ struct net_bridge_port
 #define BR_LEARNING		0x00000020
 #define BR_FLOOD		0x00000040
 #define BR_AUTO_MASK (BR_FLOOD | BR_LEARNING)
+#define BR_PROMISC		0x00000080
 
 #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
 	struct bridge_mcast_query	ip4_query;
@@ -200,6 +201,7 @@ struct net_bridge_port
 };
 
 #define br_is_auto_port(p) (((p)->flags & BR_AUTO_MASK) == BR_AUTO_MASK)
+#define br_is_promisc_port(p) ((p)->flags & BR_PROMISC)
 
 #define br_port_exists(dev) (dev->priv_flags & IFF_BRIDGE_PORT)
 
-- 
1.9.0

[Bridge] [RFC PATCH v2 net-next 4/7] bridge: Automatically manage port promiscuous mode.

[Vlad Yasevich]

When there is only 1 port that can learn mac addresses and flood
unknown traffic, this port can function in non-promiscouse mode.
The simplest example of this configuration is a bridge with only
1 port.  In this case, the bridge can't forward traffic anywhere.
All it can do is recevie traffic destined to it.  It doesn't need
to the interface into promiscouse mode.
A more complicated example is a bridge with 2 or more ports where
only 1 port remains capable of auto-discover and all others disable
mac learning and flooding thus requiring static configuration.  You
could think of this configuraiton as mimicking and edge relay with
1 uplink port (the one can still learn things), and N downlink ports
that have to be manually programmed by user or managment entity.
In this case, the 1 uplink doesn't really have
to be promiscous either since the bridge would have all the necessary
data in the fdb through static configuration.  This one port can be
manually programmed to allow to recieve necessary traffic.
Another case where there is no need for promiscuous mode is when
there are no "proverbial" uplinks and all ports are manually managed.
All necessary information will be provided, so anything an interface
receives in promiscouse mode is extra and could even be considered
security threat.

This patch supports the above scenarios.

Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
 net/bridge/br_if.c      | 85 +++++++++++++++++++++++++++++++++++++++++++++----
 net/bridge/br_private.h |  2 ++
 2 files changed, 81 insertions(+), 6 deletions(-)

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 7e60c4c..5a26ca2 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -85,6 +85,64 @@ void br_port_carrier_check(struct net_bridge_port *p)
 	spin_unlock_bh(&br->lock);
 }
 
+static void br_port_set_promisc(struct net_bridge_port *p)
+{
+	int err = 0;
+
+	if (br_is_promisc_port(p))
+		return;
+
+	err = dev_set_promiscuity(p->dev, 1);
+	if (err)
+		return;
+
+	br_fdb_unsync_static(p->br, p);
+	p->flags |= BR_PROMISC;
+}
+
+static void br_port_clear_promisc(struct net_bridge_port *p)
+{
+	int err;
+
+	if (!br_is_promisc_port(p))
+		return;
+
+	/* Since we'll be clearing the promisc mode, program the port
+	 * first so that we don't have interruption in traffic.
+	 */
+	err = br_fdb_sync_static(p->br, p);
+	if (err)
+		return;
+
+	dev_set_promiscuity(p->dev, -1);
+	p->flags &= ~BR_PROMISC;
+}
+
+/* When a port is added or removed or when certain port flags
+ * change, this function is called to automatically mange
+ * promiscuity setting of all the bridge ports.  We are always called
+ * under RTNL so can skip using rcu primitives.
+ */
+static void br_manage_promisc(struct net_bridge *br)
+{
+	struct net_bridge_port *p;
+
+	/* Algorithm is simple.  If all the port require static
+	 * configuration, we know everything and can simply write
+	 * that down to the ports and clear promisc.
+	 * If only 1 port is automatic and all the others require
+	 * static configuration, we can write all the static data
+	 * to this one automatic port and still make non-promisc.
+	 */
+	list_for_each_entry(p, &br->port_list, list) {
+		if (br->auto_cnt == 0 ||
+		    (br->auto_cnt == 1 && br_is_auto_port(p)))
+			br_port_clear_promisc(p);
+		else
+			br_port_set_promisc(p);
+	}
+}
+
 static void nbp_update_port_count(struct net_bridge *br)
 {
 	struct net_bridge_port *p;
@@ -94,7 +152,23 @@ static void nbp_update_port_count(struct net_bridge *br)
 		if (br_is_auto_port(p))
 			cnt++;
 	}
-	br->auto_cnt = cnt;
+	if (br->auto_cnt != cnt) {
+		br->auto_cnt = cnt;
+		br_manage_promisc(br);
+	}
+}
+
+static void nbp_delete_promisc(struct net_bridge_port *p)
+{
+	/* If port is currently promiscous, unset promiscuity.
+	 * Otherwise, it is a static port so remove all addresses
+	 * from it.
+	 */
+	dev_set_allmulti(p->dev, -1);
+	if (br_is_promisc_port(p))
+		dev_set_promiscuity(p->dev, -1);
+	else
+		br_fdb_unsync_static(p->br, p);
 }
 
 static void release_nbp(struct kobject *kobj)
@@ -145,7 +219,7 @@ static void del_nbp(struct net_bridge_port *p)
 
 	sysfs_remove_link(br->ifobj, p->dev->name);
 
-	dev_set_promiscuity(dev, -1);
+	nbp_delete_promisc(p);
 
 	spin_lock_bh(&br->lock);
 	br_stp_disable_port(p);
@@ -153,11 +227,10 @@ static void del_nbp(struct net_bridge_port *p)
 
 	br_ifinfo_notify(RTM_DELLINK, p);
 
-	nbp_vlan_flush(p);
-	br_fdb_delete_by_port(br, p, 1);
-
 	list_del_rcu(&p->list);
 
+	nbp_vlan_flush(p);
+	br_fdb_delete_by_port(br, p, 1);
 	nbp_update_port_count(br);
 
 	dev->priv_flags &= ~IFF_BRIDGE_PORT;
@@ -367,7 +440,7 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
 
 	call_netdevice_notifiers(NETDEV_JOIN, dev);
 
-	err = dev_set_promiscuity(dev, 1);
+	err = dev_set_allmulti(dev, 1);
 	if (err)
 		goto put_back;
 
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 2023671..1c794fef 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -175,6 +175,7 @@ struct net_bridge_port
 #define BR_LEARNING		0x00000020
 #define BR_FLOOD		0x00000040
 #define BR_AUTO_MASK (BR_FLOOD | BR_LEARNING)
+#define BR_PROMISC		0x00000080
 
 #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
 	struct bridge_mcast_query	ip4_query;
@@ -200,6 +201,7 @@ struct net_bridge_port
 };
 
 #define br_is_auto_port(p) (((p)->flags & BR_AUTO_MASK) == BR_AUTO_MASK)
+#define br_is_promisc_port(p) ((p)->flags & BR_PROMISC)
 
 #define br_port_exists(dev) (dev->priv_flags & IFF_BRIDGE_PORT)
 
-- 
1.9.0

[RFC PATCH v2 net-next 5/7] bridge: Add addresses from static fdbs to non-promisc ports

[Vlad Yasevich]

When a static fdb entry is created, add the mac address
from this fdb entry to any ports that are currently running
in non-promiscuous mode.  These ports need this data so that
they can receive traffic destined to these addresses.

Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
 net/bridge/br_fdb.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 70 insertions(+), 6 deletions(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 9759b61..e0a4632 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -85,8 +85,58 @@ static void fdb_rcu_free(struct rcu_head *head)
 	kmem_cache_free(br_fdb_cache, ent);
 }
 
+/* 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.
+ * Called under RTNL.
+ */
+static void fdb_add_hw(struct net_bridge *br, const unsigned char *addr)
+{
+	int err;
+	struct net_bridge_port *p, *tmp = NULL;
+
+	ASSERT_RTNL();
+
+	list_for_each_entry(p, &br->port_list, list) {
+		if (!br_is_promisc_port(p)) {
+			err = dev_uc_add(p->dev, addr);
+			if (err)
+				goto undo;
+		}
+	}
+
+	return;
+undo:
+	list_for_each_entry(tmp, &br->port_list, list) {
+		if (tmp == p)
+			break;
+		if (!br_is_promisc_port(tmp))
+			dev_uc_del(tmp->dev, addr);
+	}
+}
+
+/* When a static FDB entry is deleted, the HW address from that entry is
+ * also removed from the bridge private HW address list and updates all
+ * the ports with needed information.
+ * Called under RTNL.
+ */
+static void fdb_del_hw(struct net_bridge *br, const unsigned char *addr)
+{
+	struct net_bridge_port *p;
+
+	ASSERT_RTNL();
+
+	list_for_each_entry(p, &br->port_list, list) {
+		if (!br_is_promisc_port(p))
+			dev_uc_del(p->dev, addr);
+	}
+}
+
 static void fdb_delete(struct net_bridge *br, struct net_bridge_fdb_entry *f)
 {
+	if (f->is_static)
+		fdb_del_hw(br, f->addr.addr);
+
 	hlist_del_rcu(&f->hlist);
 	fdb_notify(br, f, RTM_DELNEIGH);
 	call_rcu(&f->rcu, fdb_rcu_free);
@@ -466,6 +516,7 @@ static int fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
 		return -ENOMEM;
 
 	fdb->is_local = fdb->is_static = 1;
+	fdb_add_hw(br, addr);
 	fdb_notify(br, fdb, RTM_NEWNEIGH);
 	return 0;
 }
@@ -678,16 +729,29 @@ static int fdb_add_entry(struct net_bridge_port *source, const __u8 *addr,
 	}
 
 	if (fdb_to_nud(fdb) != state) {
-		if (state & NUD_PERMANENT)
-			fdb->is_local = fdb->is_static = 1;
-		else if (state & NUD_NOARP) {
+		if (state & NUD_PERMANENT) {
+			fdb->is_local = 1;
+			if (!fdb->is_static) {
+				fdb->is_static = 1;
+				fdb_add_hw(br, addr);
+			}
+		} else if (state & NUD_NOARP) {
+			fdb->is_local = 0;
+			if (!fdb->is_static) {
+				fdb->is_static = 1;
+				fdb_add_hw(br, addr);
+			}
+		} else {
 			fdb->is_local = 0;
-			fdb->is_static = 1;
-		} else
-			fdb->is_local = fdb->is_static = 0;
+			if (fdb->is_static) {
+				fdb->is_static = 0;
+				fdb_del_hw(br, addr);
+			}
+		}
 
 		modified = true;
 	}
+
 	fdb->added_by_user = 1;
 
 	fdb->used = jiffies;
-- 
1.9.0

[Bridge] [RFC PATCH v2 net-next 5/7] bridge: Add addresses from static fdbs to non-promisc ports

[Vlad Yasevich]

When a static fdb entry is created, add the mac address
from this fdb entry to any ports that are currently running
in non-promiscuous mode.  These ports need this data so that
they can receive traffic destined to these addresses.

Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
 net/bridge/br_fdb.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 70 insertions(+), 6 deletions(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 9759b61..e0a4632 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -85,8 +85,58 @@ static void fdb_rcu_free(struct rcu_head *head)
 	kmem_cache_free(br_fdb_cache, ent);
 }
 
+/* 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.
+ * Called under RTNL.
+ */
+static void fdb_add_hw(struct net_bridge *br, const unsigned char *addr)
+{
+	int err;
+	struct net_bridge_port *p, *tmp = NULL;
+
+	ASSERT_RTNL();
+
+	list_for_each_entry(p, &br->port_list, list) {
+		if (!br_is_promisc_port(p)) {
+			err = dev_uc_add(p->dev, addr);
+			if (err)
+				goto undo;
+		}
+	}
+
+	return;
+undo:
+	list_for_each_entry(tmp, &br->port_list, list) {
+		if (tmp == p)
+			break;
+		if (!br_is_promisc_port(tmp))
+			dev_uc_del(tmp->dev, addr);
+	}
+}
+
+/* When a static FDB entry is deleted, the HW address from that entry is
+ * also removed from the bridge private HW address list and updates all
+ * the ports with needed information.
+ * Called under RTNL.
+ */
+static void fdb_del_hw(struct net_bridge *br, const unsigned char *addr)
+{
+	struct net_bridge_port *p;
+
+	ASSERT_RTNL();
+
+	list_for_each_entry(p, &br->port_list, list) {
+		if (!br_is_promisc_port(p))
+			dev_uc_del(p->dev, addr);
+	}
+}
+
 static void fdb_delete(struct net_bridge *br, struct net_bridge_fdb_entry *f)
 {
+	if (f->is_static)
+		fdb_del_hw(br, f->addr.addr);
+
 	hlist_del_rcu(&f->hlist);
 	fdb_notify(br, f, RTM_DELNEIGH);
 	call_rcu(&f->rcu, fdb_rcu_free);
@@ -466,6 +516,7 @@ static int fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
 		return -ENOMEM;
 
 	fdb->is_local = fdb->is_static = 1;
+	fdb_add_hw(br, addr);
 	fdb_notify(br, fdb, RTM_NEWNEIGH);
 	return 0;
 }
@@ -678,16 +729,29 @@ static int fdb_add_entry(struct net_bridge_port *source, const __u8 *addr,
 	}
 
 	if (fdb_to_nud(fdb) != state) {
-		if (state & NUD_PERMANENT)
-			fdb->is_local = fdb->is_static = 1;
-		else if (state & NUD_NOARP) {
+		if (state & NUD_PERMANENT) {
+			fdb->is_local = 1;
+			if (!fdb->is_static) {
+				fdb->is_static = 1;
+				fdb_add_hw(br, addr);
+			}
+		} else if (state & NUD_NOARP) {
+			fdb->is_local = 0;
+			if (!fdb->is_static) {
+				fdb->is_static = 1;
+				fdb_add_hw(br, addr);
+			}
+		} else {
 			fdb->is_local = 0;
-			fdb->is_static = 1;
-		} else
-			fdb->is_local = fdb->is_static = 0;
+			if (fdb->is_static) {
+				fdb->is_static = 0;
+				fdb_del_hw(br, addr);
+			}
+		}
 
 		modified = true;
 	}
+
 	fdb->added_by_user = 1;
 
 	fdb->used = jiffies;
-- 
1.9.0

[RFC PATCH v2 net-next 6/7] bridge: Correctly manage promiscuity when user requested it.

[Vlad Yasevich]

When the user places the bridge device in promiscuous mode,
all ports are placed in promisc mode regardless of the number
of flooding ports configured.

Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>

Conflicts:
	net/bridge/br_if.c
	net/bridge/br_private.h
---
 net/bridge/br_device.c  |  7 +++++++
 net/bridge/br_if.c      | 17 ++++++++++++-----
 net/bridge/br_private.h |  1 +
 3 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 3e2da2c..64f8112 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -112,6 +112,12 @@ static void br_dev_set_multicast_list(struct net_device *dev)
 {
 }
 
+static void br_dev_change_rx_flags(struct net_device *dev, int change)
+{
+	if (change & IFF_PROMISC)
+		br_manage_promisc(netdev_priv(dev));
+}
+
 static int br_dev_stop(struct net_device *dev)
 {
 	struct net_bridge *br = netdev_priv(dev);
@@ -309,6 +315,7 @@ static const struct net_device_ops br_netdev_ops = {
 	.ndo_get_stats64	 = br_get_stats64,
 	.ndo_set_mac_address	 = br_set_mac_address,
 	.ndo_set_rx_mode	 = br_dev_set_multicast_list,
+	.ndo_change_rx_flags	 = br_dev_change_rx_flags,
 	.ndo_change_mtu		 = br_change_mtu,
 	.ndo_do_ioctl		 = br_dev_ioctl,
 #ifdef CONFIG_NET_POLL_CONTROLLER
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 5a26ca2..d227ad6 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -123,7 +123,7 @@ static void br_port_clear_promisc(struct net_bridge_port *p)
  * promiscuity setting of all the bridge ports.  We are always called
  * under RTNL so can skip using rcu primitives.
  */
-static void br_manage_promisc(struct net_bridge *br)
+void br_manage_promisc(struct net_bridge *br)
 {
 	struct net_bridge_port *p;
 
@@ -135,11 +135,18 @@ static void br_manage_promisc(struct net_bridge *br)
 	 * to this one automatic port and still make non-promisc.
 	 */
 	list_for_each_entry(p, &br->port_list, list) {
-		if (br->auto_cnt == 0 ||
-		    (br->auto_cnt == 1 && br_is_auto_port(p)))
-			br_port_clear_promisc(p);
-		else
+		if (br->dev->flags & IFF_PROMISC) {
+			/* PROMISC flag has been turned on for the bridge
+			 * itself.  Turn on promisc on all ports.
+			 */
 			br_port_set_promisc(p);
+		} else {
+			if (br->auto_cnt == 0 ||
+			(br->auto_cnt == 1 && br_is_auto_port(p)))
+				br_port_clear_promisc(p);
+			else
+				br_port_set_promisc(p);
+		}
 	}
 }
 
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 1c794fef..f2d93d7 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -425,6 +425,7 @@ netdev_features_t br_features_recompute(struct net_bridge *br,
 					netdev_features_t features);
 void br_port_flags_change(struct net_bridge_port *port,
 			  unsigned long old_flags);
+void br_manage_promisc(struct net_bridge *br);
 
 /* br_input.c */
 int br_handle_frame_finish(struct sk_buff *skb);
-- 
1.9.0

[Bridge] [RFC PATCH v2 net-next 6/7] bridge: Correctly manage promiscuity when user requested it.

[Vlad Yasevich]

When the user places the bridge device in promiscuous mode,
all ports are placed in promisc mode regardless of the number
of flooding ports configured.

Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>

Conflicts:
	net/bridge/br_if.c
	net/bridge/br_private.h
---
 net/bridge/br_device.c  |  7 +++++++
 net/bridge/br_if.c      | 17 ++++++++++++-----
 net/bridge/br_private.h |  1 +
 3 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 3e2da2c..64f8112 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -112,6 +112,12 @@ static void br_dev_set_multicast_list(struct net_device *dev)
 {
 }
 
+static void br_dev_change_rx_flags(struct net_device *dev, int change)
+{
+	if (change & IFF_PROMISC)
+		br_manage_promisc(netdev_priv(dev));
+}
+
 static int br_dev_stop(struct net_device *dev)
 {
 	struct net_bridge *br = netdev_priv(dev);
@@ -309,6 +315,7 @@ static const struct net_device_ops br_netdev_ops = {
 	.ndo_get_stats64	 = br_get_stats64,
 	.ndo_set_mac_address	 = br_set_mac_address,
 	.ndo_set_rx_mode	 = br_dev_set_multicast_list,
+	.ndo_change_rx_flags	 = br_dev_change_rx_flags,
 	.ndo_change_mtu		 = br_change_mtu,
 	.ndo_do_ioctl		 = br_dev_ioctl,
 #ifdef CONFIG_NET_POLL_CONTROLLER
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 5a26ca2..d227ad6 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -123,7 +123,7 @@ static void br_port_clear_promisc(struct net_bridge_port *p)
  * promiscuity setting of all the bridge ports.  We are always called
  * under RTNL so can skip using rcu primitives.
  */
-static void br_manage_promisc(struct net_bridge *br)
+void br_manage_promisc(struct net_bridge *br)
 {
 	struct net_bridge_port *p;
 
@@ -135,11 +135,18 @@ static void br_manage_promisc(struct net_bridge *br)
 	 * to this one automatic port and still make non-promisc.
 	 */
 	list_for_each_entry(p, &br->port_list, list) {
-		if (br->auto_cnt == 0 ||
-		    (br->auto_cnt == 1 && br_is_auto_port(p)))
-			br_port_clear_promisc(p);
-		else
+		if (br->dev->flags & IFF_PROMISC) {
+			/* PROMISC flag has been turned on for the bridge
+			 * itself.  Turn on promisc on all ports.
+			 */
 			br_port_set_promisc(p);
+		} else {
+			if (br->auto_cnt == 0 ||
+			(br->auto_cnt == 1 && br_is_auto_port(p)))
+				br_port_clear_promisc(p);
+			else
+				br_port_set_promisc(p);
+		}
 	}
 }
 
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 1c794fef..f2d93d7 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -425,6 +425,7 @@ netdev_features_t br_features_recompute(struct net_bridge *br,
 					netdev_features_t features);
 void br_port_flags_change(struct net_bridge_port *port,
 			  unsigned long old_flags);
+void br_manage_promisc(struct net_bridge *br);
 
 /* br_input.c */
 int br_handle_frame_finish(struct sk_buff *skb);
-- 
1.9.0

[RFC PATCH v2 net-next 7/7] bridge: Automatically manage promisc mode when vlan filtering is on.

[Vlad Yasevich]

If the user doesn't enable vlan filtering, we have to place all
bridge ports in promsic mode so that we retain the capability of
of receiving tagged frames.
When vlan filtering is enabled, the each port will be provided with
necessary vlan configuration and would be able to receive tagged
traffic without promiscouse mode set, thus allowing us to automatically
turn promiscuity on or off depending on the configuration.

Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
 net/bridge/br_if.c      | 12 ++++++++----
 net/bridge/br_private.h |  9 +++++++++
 net/bridge/br_vlan.c    |  1 +
 3 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index d227ad6..ca1953c 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -126,6 +126,13 @@ static void br_port_clear_promisc(struct net_bridge_port *p)
 void br_manage_promisc(struct net_bridge *br)
 {
 	struct net_bridge_port *p;
+	int set_all = false;
+
+	/* If vlan filtering is disabled or bridge interface is placed
+	 * into promiscouse mode, place all ports in promiscuous mode.
+	 */
+	if ((br->dev->flags & IFF_PROMISC) || !br_vlan_enabled(br))
+		set_all = true;
 
 	/* Algorithm is simple.  If all the port require static
 	 * configuration, we know everything and can simply write
@@ -135,10 +142,7 @@ void br_manage_promisc(struct net_bridge *br)
 	 * to this one automatic port and still make non-promisc.
 	 */
 	list_for_each_entry(p, &br->port_list, list) {
-		if (br->dev->flags & IFF_PROMISC) {
-			/* PROMISC flag has been turned on for the bridge
-			 * itself.  Turn on promisc on all ports.
-			 */
+		if (set_all) {
 			br_port_set_promisc(p);
 		} else {
 			if (br->auto_cnt == 0 ||
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index f2d93d7..df0bb49 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -643,6 +643,10 @@ static inline u16 br_get_pvid(const struct net_port_vlans *v)
 	return v->pvid ?: VLAN_N_VID;
 }
 
+static inline int br_vlan_enabled(struct net_bridge *br)
+{
+	return br->vlan_enabled;
+}
 #else
 static inline bool br_allowed_ingress(struct net_bridge *br,
 				      struct net_port_vlans *v,
@@ -723,6 +727,11 @@ static inline u16 br_get_pvid(const struct net_port_vlans *v)
 {
 	return VLAN_N_VID;	/* Returns invalid vid */
 }
+
+static inline int br_vlan_enabled(struct net_bridge *br);
+{
+	return 0;
+}
 #endif
 
 /* br_netfilter.c */
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 4a37161..24c5cc5 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -332,6 +332,7 @@ int br_vlan_filter_toggle(struct net_bridge *br, unsigned long val)
 		goto unlock;
 
 	br->vlan_enabled = val;
+	br_manage_promisc(br);
 
 unlock:
 	rtnl_unlock();
-- 
1.9.0

[Bridge] [RFC PATCH v2 net-next 7/7] bridge: Automatically manage promisc mode when vlan filtering is on.

[Vlad Yasevich]

If the user doesn't enable vlan filtering, we have to place all
bridge ports in promsic mode so that we retain the capability of
of receiving tagged frames.
When vlan filtering is enabled, the each port will be provided with
necessary vlan configuration and would be able to receive tagged
traffic without promiscouse mode set, thus allowing us to automatically
turn promiscuity on or off depending on the configuration.

Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
 net/bridge/br_if.c      | 12 ++++++++----
 net/bridge/br_private.h |  9 +++++++++
 net/bridge/br_vlan.c    |  1 +
 3 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index d227ad6..ca1953c 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -126,6 +126,13 @@ static void br_port_clear_promisc(struct net_bridge_port *p)
 void br_manage_promisc(struct net_bridge *br)
 {
 	struct net_bridge_port *p;
+	int set_all = false;
+
+	/* If vlan filtering is disabled or bridge interface is placed
+	 * into promiscouse mode, place all ports in promiscuous mode.
+	 */
+	if ((br->dev->flags & IFF_PROMISC) || !br_vlan_enabled(br))
+		set_all = true;
 
 	/* Algorithm is simple.  If all the port require static
 	 * configuration, we know everything and can simply write
@@ -135,10 +142,7 @@ void br_manage_promisc(struct net_bridge *br)
 	 * to this one automatic port and still make non-promisc.
 	 */
 	list_for_each_entry(p, &br->port_list, list) {
-		if (br->dev->flags & IFF_PROMISC) {
-			/* PROMISC flag has been turned on for the bridge
-			 * itself.  Turn on promisc on all ports.
-			 */
+		if (set_all) {
 			br_port_set_promisc(p);
 		} else {
 			if (br->auto_cnt == 0 ||
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index f2d93d7..df0bb49 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -643,6 +643,10 @@ static inline u16 br_get_pvid(const struct net_port_vlans *v)
 	return v->pvid ?: VLAN_N_VID;
 }
 
+static inline int br_vlan_enabled(struct net_bridge *br)
+{
+	return br->vlan_enabled;
+}
 #else
 static inline bool br_allowed_ingress(struct net_bridge *br,
 				      struct net_port_vlans *v,
@@ -723,6 +727,11 @@ static inline u16 br_get_pvid(const struct net_port_vlans *v)
 {
 	return VLAN_N_VID;	/* Returns invalid vid */
 }
+
+static inline int br_vlan_enabled(struct net_bridge *br);
+{
+	return 0;
+}
 #endif
 
 /* br_netfilter.c */
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 4a37161..24c5cc5 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -332,6 +332,7 @@ int br_vlan_filter_toggle(struct net_bridge *br, unsigned long val)
 		goto unlock;
 
 	br->vlan_enabled = val;
+	br_manage_promisc(br);
 
 unlock:
 	rtnl_unlock();
-- 
1.9.0

Re: [RFC PATCH v2 net-next 2/7] bridge: Keep track of ports capable of automatic discovery.

[Michael S. Tsirkin]

On Tue, Apr 29, 2014 at 05:20:23PM -0400, Vlad Yasevich wrote:
> By default, ports on the bridge are capable of automatic
> discovery of nodes located behind the port.  This is accomplished
> via flooding of unknown traffic (BR_FLOOD) and learning the
> mac addresses from these packets (BR_LEARNING).
> If the above functionality is diabled by turning off these
> flags, the port requires static configuration in the form
> of static FDB entries to function properly.
> 
> This patch adds functionality to keep track of all ports
> capable of automatic discovery.  This will later be used
> to control promiscuity settings.
> 
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> ---
>  net/bridge/br_if.c       | 27 +++++++++++++++++++++++++++
>  net/bridge/br_netlink.c  |  3 +++
>  net/bridge/br_private.h  |  6 ++++++
>  net/bridge/br_sysfs_if.c |  6 +++++-
>  4 files changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> index 5262b86..7e60c4c 100644
> --- a/net/bridge/br_if.c
> +++ b/net/bridge/br_if.c
> @@ -85,6 +85,18 @@ void br_port_carrier_check(struct net_bridge_port *p)
>  	spin_unlock_bh(&br->lock);
>  }
>  
> +static void nbp_update_port_count(struct net_bridge *br)
> +{
> +	struct net_bridge_port *p;
> +	u32 cnt = 0;
> +
> +	list_for_each_entry(p, &br->port_list, list) {
> +		if (br_is_auto_port(p))
> +			cnt++;
> +	}
> +	br->auto_cnt = cnt;
> +}
> +
>  static void release_nbp(struct kobject *kobj)
>  {
>  	struct net_bridge_port *p
> @@ -146,6 +158,8 @@ static void del_nbp(struct net_bridge_port *p)
>  
>  	list_del_rcu(&p->list);
>  
> +	nbp_update_port_count(br);
> +
>  	dev->priv_flags &= ~IFF_BRIDGE_PORT;
>  
>  	netdev_rx_handler_unregister(dev);
> @@ -384,6 +398,8 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
>  
>  	list_add_rcu(&p->list, &br->port_list);
>  
> +	nbp_update_port_count(br);
> +
>  	netdev_update_features(br->dev);
>  
>  	if (br->dev->needed_headroom < dev->needed_headroom)
> @@ -455,3 +471,14 @@ int br_del_if(struct net_bridge *br, struct net_device *dev)
>  
>  	return 0;
>  }
> +
> +void br_port_flags_change(struct net_bridge_port *p, unsigned long old_flags)
> +{
> +	struct net_bridge *br = p->br;
> +	unsigned long mask = old_flags ^ p->flags;
> +
> +	if (!(mask & BR_AUTO_MASK))
> +		return;
> +
> +	nbp_update_port_count(br);
> +}
> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> index e74b6d53..01382b9 100644
> --- a/net/bridge/br_netlink.c
> +++ b/net/bridge/br_netlink.c
> @@ -328,6 +328,7 @@ 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 old_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);
> @@ -353,6 +354,8 @@ static int br_setport(struct net_bridge_port *p, struct nlattr *tb[])
>  		if (err)
>  			return err;
>  	}
> +
> +	br_port_flags_change(p, old_flags ^ p->flags);
>  	return 0;
>  }
>  
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 06811d7..db714a1 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -174,6 +174,7 @@ struct net_bridge_port
>  #define BR_ADMIN_COST		0x00000010
>  #define BR_LEARNING		0x00000020
>  #define BR_FLOOD		0x00000040
> +#define BR_AUTO_MASK (BR_FLOOD | BR_LEARNING)
>  
>  #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
>  	struct bridge_mcast_query	ip4_query;
> @@ -198,6 +199,8 @@ struct net_bridge_port
>  #endif
>  };
>  
> +#define br_is_auto_port(p) (((p)->flags & BR_AUTO_MASK) == BR_AUTO_MASK)
> +

I think you
really want ((p)->flags & BR_AUTO_MASK) instead without
 == BR_AUTO_MASK.

For example if learning is off but flood is on, things work
(slowly) automatically without configuring static entries.

OTOH if flood is off but learning is on, a bunch of packets
might get dropped but eventually you might learn the
mac and then networking will start working.


>  #define br_port_exists(dev) (dev->priv_flags & IFF_BRIDGE_PORT)
>  
>  static inline struct net_bridge_port *br_port_get_rcu(const struct net_device *dev)
> @@ -290,6 +293,7 @@ struct net_bridge
>  	struct timer_list		topology_change_timer;
>  	struct timer_list		gc_timer;
>  	struct kobject			*ifobj;
> +	u32				auto_cnt;
>  #ifdef CONFIG_BRIDGE_VLAN_FILTERING
>  	u8				vlan_enabled;
>  	struct net_port_vlans __rcu	*vlan_info;
> @@ -415,6 +419,8 @@ int br_del_if(struct net_bridge *br, struct net_device *dev);
>  int br_min_mtu(const struct net_bridge *br);
>  netdev_features_t br_features_recompute(struct net_bridge *br,
>  					netdev_features_t features);
> +void br_port_flags_change(struct net_bridge_port *port,
> +			  unsigned long old_flags);
>  
>  /* br_input.c */
>  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 112a25e..58c26e3 100644
> --- a/net/bridge/br_sysfs_if.c
> +++ b/net/bridge/br_sysfs_if.c
> @@ -49,7 +49,10 @@ static BRPORT_ATTR(_name, S_IRUGO | S_IWUSR,			\
>  static int store_flag(struct net_bridge_port *p, unsigned long v,
>  		     unsigned long mask)
>  {
> -	unsigned long flags = p->flags;
> +	unsigned long flags;
> +	unsigned long old_flags;
> +
> +	old_flags = flags = p->flags;
>  
>  	if (v)
>  		flags |= mask;
> @@ -58,6 +61,7 @@ static int store_flag(struct net_bridge_port *p, unsigned long v,
>  
>  	if (flags != p->flags) {
>  		p->flags = flags;
> +		br_port_flags_change(p, old_flags ^ flags);
>  		br_ifinfo_notify(RTM_NEWLINK, p);
>  	}
>  	return 0;
> -- 
> 1.9.0

Re: [Bridge] [RFC PATCH v2 net-next 2/7] bridge: Keep track of ports capable of automatic discovery.

[Michael S. Tsirkin]

On Tue, Apr 29, 2014 at 05:20:23PM -0400, Vlad Yasevich wrote:
> By default, ports on the bridge are capable of automatic
> discovery of nodes located behind the port.  This is accomplished
> via flooding of unknown traffic (BR_FLOOD) and learning the
> mac addresses from these packets (BR_LEARNING).
> If the above functionality is diabled by turning off these
> flags, the port requires static configuration in the form
> of static FDB entries to function properly.
> 
> This patch adds functionality to keep track of all ports
> capable of automatic discovery.  This will later be used
> to control promiscuity settings.
> 
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> ---
>  net/bridge/br_if.c       | 27 +++++++++++++++++++++++++++
>  net/bridge/br_netlink.c  |  3 +++
>  net/bridge/br_private.h  |  6 ++++++
>  net/bridge/br_sysfs_if.c |  6 +++++-
>  4 files changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> index 5262b86..7e60c4c 100644
> --- a/net/bridge/br_if.c
> +++ b/net/bridge/br_if.c
> @@ -85,6 +85,18 @@ void br_port_carrier_check(struct net_bridge_port *p)
>  	spin_unlock_bh(&br->lock);
>  }
>  
> +static void nbp_update_port_count(struct net_bridge *br)
> +{
> +	struct net_bridge_port *p;
> +	u32 cnt = 0;
> +
> +	list_for_each_entry(p, &br->port_list, list) {
> +		if (br_is_auto_port(p))
> +			cnt++;
> +	}
> +	br->auto_cnt = cnt;
> +}
> +
>  static void release_nbp(struct kobject *kobj)
>  {
>  	struct net_bridge_port *p
> @@ -146,6 +158,8 @@ static void del_nbp(struct net_bridge_port *p)
>  
>  	list_del_rcu(&p->list);
>  
> +	nbp_update_port_count(br);
> +
>  	dev->priv_flags &= ~IFF_BRIDGE_PORT;
>  
>  	netdev_rx_handler_unregister(dev);
> @@ -384,6 +398,8 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
>  
>  	list_add_rcu(&p->list, &br->port_list);
>  
> +	nbp_update_port_count(br);
> +
>  	netdev_update_features(br->dev);
>  
>  	if (br->dev->needed_headroom < dev->needed_headroom)
> @@ -455,3 +471,14 @@ int br_del_if(struct net_bridge *br, struct net_device *dev)
>  
>  	return 0;
>  }
> +
> +void br_port_flags_change(struct net_bridge_port *p, unsigned long old_flags)
> +{
> +	struct net_bridge *br = p->br;
> +	unsigned long mask = old_flags ^ p->flags;
> +
> +	if (!(mask & BR_AUTO_MASK))
> +		return;
> +
> +	nbp_update_port_count(br);
> +}
> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> index e74b6d53..01382b9 100644
> --- a/net/bridge/br_netlink.c
> +++ b/net/bridge/br_netlink.c
> @@ -328,6 +328,7 @@ 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 old_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);
> @@ -353,6 +354,8 @@ static int br_setport(struct net_bridge_port *p, struct nlattr *tb[])
>  		if (err)
>  			return err;
>  	}
> +
> +	br_port_flags_change(p, old_flags ^ p->flags);
>  	return 0;
>  }
>  
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 06811d7..db714a1 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -174,6 +174,7 @@ struct net_bridge_port
>  #define BR_ADMIN_COST		0x00000010
>  #define BR_LEARNING		0x00000020
>  #define BR_FLOOD		0x00000040
> +#define BR_AUTO_MASK (BR_FLOOD | BR_LEARNING)
>  
>  #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
>  	struct bridge_mcast_query	ip4_query;
> @@ -198,6 +199,8 @@ struct net_bridge_port
>  #endif
>  };
>  
> +#define br_is_auto_port(p) (((p)->flags & BR_AUTO_MASK) == BR_AUTO_MASK)
> +

I think you
really want ((p)->flags & BR_AUTO_MASK) instead without
 == BR_AUTO_MASK.

For example if learning is off but flood is on, things work
(slowly) automatically without configuring static entries.

OTOH if flood is off but learning is on, a bunch of packets
might get dropped but eventually you might learn the
mac and then networking will start working.


>  #define br_port_exists(dev) (dev->priv_flags & IFF_BRIDGE_PORT)
>  
>  static inline struct net_bridge_port *br_port_get_rcu(const struct net_device *dev)
> @@ -290,6 +293,7 @@ struct net_bridge
>  	struct timer_list		topology_change_timer;
>  	struct timer_list		gc_timer;
>  	struct kobject			*ifobj;
> +	u32				auto_cnt;
>  #ifdef CONFIG_BRIDGE_VLAN_FILTERING
>  	u8				vlan_enabled;
>  	struct net_port_vlans __rcu	*vlan_info;
> @@ -415,6 +419,8 @@ int br_del_if(struct net_bridge *br, struct net_device *dev);
>  int br_min_mtu(const struct net_bridge *br);
>  netdev_features_t br_features_recompute(struct net_bridge *br,
>  					netdev_features_t features);
> +void br_port_flags_change(struct net_bridge_port *port,
> +			  unsigned long old_flags);
>  
>  /* br_input.c */
>  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 112a25e..58c26e3 100644
> --- a/net/bridge/br_sysfs_if.c
> +++ b/net/bridge/br_sysfs_if.c
> @@ -49,7 +49,10 @@ static BRPORT_ATTR(_name, S_IRUGO | S_IWUSR,			\
>  static int store_flag(struct net_bridge_port *p, unsigned long v,
>  		     unsigned long mask)
>  {
> -	unsigned long flags = p->flags;
> +	unsigned long flags;
> +	unsigned long old_flags;
> +
> +	old_flags = flags = p->flags;
>  
>  	if (v)
>  		flags |= mask;
> @@ -58,6 +61,7 @@ static int store_flag(struct net_bridge_port *p, unsigned long v,
>  
>  	if (flags != p->flags) {
>  		p->flags = flags;
> +		br_port_flags_change(p, old_flags ^ flags);
>  		br_ifinfo_notify(RTM_NEWLINK, p);
>  	}
>  	return 0;
> -- 
> 1.9.0

Re: [RFC PATCH v2 net-next 4/7] bridge: Automatically manage port promiscuous mode.

[Michael S. Tsirkin]

On Tue, Apr 29, 2014 at 05:20:25PM -0400, Vlad Yasevich wrote:
> When there is only 1 port that can learn mac addresses and flood
> unknown traffic, this port can function in non-promiscouse mode.

typo: non-promiscous

I think the code is correct here. But I'm not sure the explanation
addresses the question that has been raised in the past:
why is it correct to tie the output (flood) and input (learning)
together and have it affect input (promisc).

I would offer the following explanation to address the above
question:


Consider a specific port X. All traffic on its input will be
output through one of the other ports (or dropped).
Thus if *no* other port need to flood traffic
on its output, then the combination of FDB entries for other
ports completely specifies the set of legal mac addresses on the input
of port X.

If, additionally, learning is disabled for all other ports,
these FDB entries for other ports are static, so this set
of mac addresses on port X does not change much.

This patch detects that no other port needs to flood or has learning
enabled, and disables promisc mode for port X, instead programming the
set of output mac addresses for other ports from the static fdb into the
hardware filtering table in the underlying device.

Note: an alternative would be to simply check that no other port needs
to flood, however, this would mean that non-static FDB updates by
learning on other ports need to be propagated to hardware on port X,
which is harder to implement efficiently.

It might be a good idea to put something like this above in
commit log, along with the examples you provide below.
A shortened version might be helpful in code comments as well.

> The simplest example of this configuration is a bridge with only
> 1 port.  In this case, the bridge can't forward traffic anywhere.
> All it can do is recevie traffic destined to it.  It doesn't need
> to the interface into promiscouse mode.
> A more complicated example is a bridge with 2 or more ports where
> only 1 port remains capable of auto-discover and all others disable
> mac learning and flooding thus requiring static configuration.  You
> could think of this configuraiton as mimicking and edge relay with
> 1 uplink port (the one can still learn things), and N downlink ports
> that have to be manually programmed by user or managment entity.
> In this case, the 1 uplink doesn't really have
> to be promiscous either since the bridge would have all the necessary
> data in the fdb through static configuration.  This one port can be
> manually programmed to allow to recieve necessary traffic.
> Another case where there is no need for promiscuous mode is when
> there are no "proverbial" uplinks and all ports are manually managed.
> All necessary information will be provided, so anything an interface
> receives in promiscouse mode is extra and could even be considered
> security threat.

Might be a good idea to clarify: the security problem is that with
hardware filtering by MAC being off, it's easier to cause load spikes on
multiple bridges on the LAN.


> This patch supports the above scenarios.
> 
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>


I think what this patchset is trying to do correct.
Minor comments below.


Would bisect hit partially broken configurations if
it picks patch 4 but not 5-7?
If yes it's best to smash patches 5-7 in with this one.


> ---
>  net/bridge/br_if.c      | 85 +++++++++++++++++++++++++++++++++++++++++++++----
>  net/bridge/br_private.h |  2 ++
>  2 files changed, 81 insertions(+), 6 deletions(-)
> 
> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> index 7e60c4c..5a26ca2 100644
> --- a/net/bridge/br_if.c
> +++ b/net/bridge/br_if.c
> @@ -85,6 +85,64 @@ void br_port_carrier_check(struct net_bridge_port *p)
>  	spin_unlock_bh(&br->lock);
>  }
>  
> +static void br_port_set_promisc(struct net_bridge_port *p)
> +{
> +	int err = 0;
> +
> +	if (br_is_promisc_port(p))
> +		return;
> +
> +	err = dev_set_promiscuity(p->dev, 1);
> +	if (err)
> +		return;

Can this happen here? We can't recover so maybe warn if it does?

> +
> +	br_fdb_unsync_static(p->br, p);
> +	p->flags |= BR_PROMISC;
> +}
> +
> +static void br_port_clear_promisc(struct net_bridge_port *p)
> +{
> +	int err;
> +
> +	if (!br_is_promisc_port(p))
> +		return;
> +
> +	/* Since we'll be clearing the promisc mode, program the port
> +	 * first so that we don't have interruption in traffic.
> +	 */
> +	err = br_fdb_sync_static(p->br, p);
> +	if (err)
> +		return;
> +
> +	dev_set_promiscuity(p->dev, -1);
> +	p->flags &= ~BR_PROMISC;
> +}
> +
> +/* When a port is added or removed or when certain port flags
> + * change, this function is called to automatically mange
> + * promiscuity setting of all the bridge ports.  We are always called
> + * under RTNL so can skip using rcu primitives.
> + */
> +static void br_manage_promisc(struct net_bridge *br)
> +{
> +	struct net_bridge_port *p;
> +
> +	/* Algorithm is simple.

I think it's best to drop this line.
It doesn't really clarify anything :)

>  If all the port

all the ports?

> require static
> +	 * configuration, we know everything and can simply write
> +	 * that down to the ports and clear promisc.
> +	 * If only 1 port is automatic and all the others require
> +	 * static configuration, we can write all the static data
> +	 * to this one automatic port and still make non-promisc.


make *this port* non promisc.

> +	 */

It's best to move this comment within list_for_each_entry,
near the code it documents.

> +	list_for_each_entry(p, &br->port_list, list) {
> +		if (br->auto_cnt == 0 ||
> +		    (br->auto_cnt == 1 && br_is_auto_port(p)))


This is correct but I think it would be clearer as follows:

	list_for_each_entry(p, &br->port_list, list) {
		/* If this is the only automatic port, others are not automatic so
		 * their output configuration is determined by static fdb entries.
		 * Thus, since input on this port will become output on one of the
		 * others, this means our input configuration is static: write it out to
		 * hardware and disable promiscous mode.
		 * Otherwise, make the port promiscous.
		 */
		if (br->auto_cnt <= br_is_auto_port(p))
				br_port_clear_promisc(p);
			else
				br_port_set_promisc(p);
	}


instead of enumerating cases in comments and in code like you did above.

	


> +			br_port_clear_promisc(p);
> +		else
> +			br_port_set_promisc(p);
> +	}
> +}
> +
>  static void nbp_update_port_count(struct net_bridge *br)
>  {
>  	struct net_bridge_port *p;
> @@ -94,7 +152,23 @@ static void nbp_update_port_count(struct net_bridge *br)
>  		if (br_is_auto_port(p))
>  			cnt++;
>  	}
> -	br->auto_cnt = cnt;
> +	if (br->auto_cnt != cnt) {
> +		br->auto_cnt = cnt;
> +		br_manage_promisc(br);
> +	}
> +}
> +
> +static void nbp_delete_promisc(struct net_bridge_port *p)
> +{
> +	/* If port is currently promiscous, unset promiscuity.
> +	 * Otherwise, it is a static port so remove all addresses
> +	 * from it.
> +	 */
> +	dev_set_allmulti(p->dev, -1);
> +	if (br_is_promisc_port(p))
> +		dev_set_promiscuity(p->dev, -1);
> +	else
> +		br_fdb_unsync_static(p->br, p);
>  }
>  
>  static void release_nbp(struct kobject *kobj)
> @@ -145,7 +219,7 @@ static void del_nbp(struct net_bridge_port *p)
>  
>  	sysfs_remove_link(br->ifobj, p->dev->name);
>  
> -	dev_set_promiscuity(dev, -1);
> +	nbp_delete_promisc(p);
>  
>  	spin_lock_bh(&br->lock);
>  	br_stp_disable_port(p);
> @@ -153,11 +227,10 @@ static void del_nbp(struct net_bridge_port *p)
>  
>  	br_ifinfo_notify(RTM_DELLINK, p);
>  
> -	nbp_vlan_flush(p);
> -	br_fdb_delete_by_port(br, p, 1);
> -
>  	list_del_rcu(&p->list);
>  
> +	nbp_vlan_flush(p);
> +	br_fdb_delete_by_port(br, p, 1);
>  	nbp_update_port_count(br);
>  
>  	dev->priv_flags &= ~IFF_BRIDGE_PORT;
> @@ -367,7 +440,7 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
>  
>  	call_netdevice_notifiers(NETDEV_JOIN, dev);
>  
> -	err = dev_set_promiscuity(dev, 1);
> +	err = dev_set_allmulti(dev, 1);
>  	if (err)
>  		goto put_back;
>  
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 2023671..1c794fef 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -175,6 +175,7 @@ struct net_bridge_port
>  #define BR_LEARNING		0x00000020
>  #define BR_FLOOD		0x00000040
>  #define BR_AUTO_MASK (BR_FLOOD | BR_LEARNING)
> +#define BR_PROMISC		0x00000080
>  
>  #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
>  	struct bridge_mcast_query	ip4_query;
> @@ -200,6 +201,7 @@ struct net_bridge_port
>  };
>  
>  #define br_is_auto_port(p) (((p)->flags & BR_AUTO_MASK) == BR_AUTO_MASK)
> +#define br_is_promisc_port(p) ((p)->flags & BR_PROMISC)
>  
>  #define br_port_exists(dev) (dev->priv_flags & IFF_BRIDGE_PORT)
>  
> -- 
> 1.9.0

Re: [Bridge] [RFC PATCH v2 net-next 4/7] bridge: Automatically manage port promiscuous mode.

[Michael S. Tsirkin]

On Tue, Apr 29, 2014 at 05:20:25PM -0400, Vlad Yasevich wrote:
> When there is only 1 port that can learn mac addresses and flood
> unknown traffic, this port can function in non-promiscouse mode.

typo: non-promiscous

I think the code is correct here. But I'm not sure the explanation
addresses the question that has been raised in the past:
why is it correct to tie the output (flood) and input (learning)
together and have it affect input (promisc).

I would offer the following explanation to address the above
question:


Consider a specific port X. All traffic on its input will be
output through one of the other ports (or dropped).
Thus if *no* other port need to flood traffic
on its output, then the combination of FDB entries for other
ports completely specifies the set of legal mac addresses on the input
of port X.

If, additionally, learning is disabled for all other ports,
these FDB entries for other ports are static, so this set
of mac addresses on port X does not change much.

This patch detects that no other port needs to flood or has learning
enabled, and disables promisc mode for port X, instead programming the
set of output mac addresses for other ports from the static fdb into the
hardware filtering table in the underlying device.

Note: an alternative would be to simply check that no other port needs
to flood, however, this would mean that non-static FDB updates by
learning on other ports need to be propagated to hardware on port X,
which is harder to implement efficiently.

It might be a good idea to put something like this above in
commit log, along with the examples you provide below.
A shortened version might be helpful in code comments as well.

> The simplest example of this configuration is a bridge with only
> 1 port.  In this case, the bridge can't forward traffic anywhere.
> All it can do is recevie traffic destined to it.  It doesn't need
> to the interface into promiscouse mode.
> A more complicated example is a bridge with 2 or more ports where
> only 1 port remains capable of auto-discover and all others disable
> mac learning and flooding thus requiring static configuration.  You
> could think of this configuraiton as mimicking and edge relay with
> 1 uplink port (the one can still learn things), and N downlink ports
> that have to be manually programmed by user or managment entity.
> In this case, the 1 uplink doesn't really have
> to be promiscous either since the bridge would have all the necessary
> data in the fdb through static configuration.  This one port can be
> manually programmed to allow to recieve necessary traffic.
> Another case where there is no need for promiscuous mode is when
> there are no "proverbial" uplinks and all ports are manually managed.
> All necessary information will be provided, so anything an interface
> receives in promiscouse mode is extra and could even be considered
> security threat.

Might be a good idea to clarify: the security problem is that with
hardware filtering by MAC being off, it's easier to cause load spikes on
multiple bridges on the LAN.


> This patch supports the above scenarios.
> 
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>


I think what this patchset is trying to do correct.
Minor comments below.


Would bisect hit partially broken configurations if
it picks patch 4 but not 5-7?
If yes it's best to smash patches 5-7 in with this one.


> ---
>  net/bridge/br_if.c      | 85 +++++++++++++++++++++++++++++++++++++++++++++----
>  net/bridge/br_private.h |  2 ++
>  2 files changed, 81 insertions(+), 6 deletions(-)
> 
> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> index 7e60c4c..5a26ca2 100644
> --- a/net/bridge/br_if.c
> +++ b/net/bridge/br_if.c
> @@ -85,6 +85,64 @@ void br_port_carrier_check(struct net_bridge_port *p)
>  	spin_unlock_bh(&br->lock);
>  }
>  
> +static void br_port_set_promisc(struct net_bridge_port *p)
> +{
> +	int err = 0;
> +
> +	if (br_is_promisc_port(p))
> +		return;
> +
> +	err = dev_set_promiscuity(p->dev, 1);
> +	if (err)
> +		return;

Can this happen here? We can't recover so maybe warn if it does?

> +
> +	br_fdb_unsync_static(p->br, p);
> +	p->flags |= BR_PROMISC;
> +}
> +
> +static void br_port_clear_promisc(struct net_bridge_port *p)
> +{
> +	int err;
> +
> +	if (!br_is_promisc_port(p))
> +		return;
> +
> +	/* Since we'll be clearing the promisc mode, program the port
> +	 * first so that we don't have interruption in traffic.
> +	 */
> +	err = br_fdb_sync_static(p->br, p);
> +	if (err)
> +		return;
> +
> +	dev_set_promiscuity(p->dev, -1);
> +	p->flags &= ~BR_PROMISC;
> +}
> +
> +/* When a port is added or removed or when certain port flags
> + * change, this function is called to automatically mange
> + * promiscuity setting of all the bridge ports.  We are always called
> + * under RTNL so can skip using rcu primitives.
> + */
> +static void br_manage_promisc(struct net_bridge *br)
> +{
> +	struct net_bridge_port *p;
> +
> +	/* Algorithm is simple.

I think it's best to drop this line.
It doesn't really clarify anything :)

>  If all the port

all the ports?

> require static
> +	 * configuration, we know everything and can simply write
> +	 * that down to the ports and clear promisc.
> +	 * If only 1 port is automatic and all the others require
> +	 * static configuration, we can write all the static data
> +	 * to this one automatic port and still make non-promisc.


make *this port* non promisc.

> +	 */

It's best to move this comment within list_for_each_entry,
near the code it documents.

> +	list_for_each_entry(p, &br->port_list, list) {
> +		if (br->auto_cnt == 0 ||
> +		    (br->auto_cnt == 1 && br_is_auto_port(p)))


This is correct but I think it would be clearer as follows:

	list_for_each_entry(p, &br->port_list, list) {
		/* If this is the only automatic port, others are not automatic so
		 * their output configuration is determined by static fdb entries.
		 * Thus, since input on this port will become output on one of the
		 * others, this means our input configuration is static: write it out to
		 * hardware and disable promiscous mode.
		 * Otherwise, make the port promiscous.
		 */
		if (br->auto_cnt <= br_is_auto_port(p))
				br_port_clear_promisc(p);
			else
				br_port_set_promisc(p);
	}


instead of enumerating cases in comments and in code like you did above.

	


> +			br_port_clear_promisc(p);
> +		else
> +			br_port_set_promisc(p);
> +	}
> +}
> +
>  static void nbp_update_port_count(struct net_bridge *br)
>  {
>  	struct net_bridge_port *p;
> @@ -94,7 +152,23 @@ static void nbp_update_port_count(struct net_bridge *br)
>  		if (br_is_auto_port(p))
>  			cnt++;
>  	}
> -	br->auto_cnt = cnt;
> +	if (br->auto_cnt != cnt) {
> +		br->auto_cnt = cnt;
> +		br_manage_promisc(br);
> +	}
> +}
> +
> +static void nbp_delete_promisc(struct net_bridge_port *p)
> +{
> +	/* If port is currently promiscous, unset promiscuity.
> +	 * Otherwise, it is a static port so remove all addresses
> +	 * from it.
> +	 */
> +	dev_set_allmulti(p->dev, -1);
> +	if (br_is_promisc_port(p))
> +		dev_set_promiscuity(p->dev, -1);
> +	else
> +		br_fdb_unsync_static(p->br, p);
>  }
>  
>  static void release_nbp(struct kobject *kobj)
> @@ -145,7 +219,7 @@ static void del_nbp(struct net_bridge_port *p)
>  
>  	sysfs_remove_link(br->ifobj, p->dev->name);
>  
> -	dev_set_promiscuity(dev, -1);
> +	nbp_delete_promisc(p);
>  
>  	spin_lock_bh(&br->lock);
>  	br_stp_disable_port(p);
> @@ -153,11 +227,10 @@ static void del_nbp(struct net_bridge_port *p)
>  
>  	br_ifinfo_notify(RTM_DELLINK, p);
>  
> -	nbp_vlan_flush(p);
> -	br_fdb_delete_by_port(br, p, 1);
> -
>  	list_del_rcu(&p->list);
>  
> +	nbp_vlan_flush(p);
> +	br_fdb_delete_by_port(br, p, 1);
>  	nbp_update_port_count(br);
>  
>  	dev->priv_flags &= ~IFF_BRIDGE_PORT;
> @@ -367,7 +440,7 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
>  
>  	call_netdevice_notifiers(NETDEV_JOIN, dev);
>  
> -	err = dev_set_promiscuity(dev, 1);
> +	err = dev_set_allmulti(dev, 1);
>  	if (err)
>  		goto put_back;
>  
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 2023671..1c794fef 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -175,6 +175,7 @@ struct net_bridge_port
>  #define BR_LEARNING		0x00000020
>  #define BR_FLOOD		0x00000040
>  #define BR_AUTO_MASK (BR_FLOOD | BR_LEARNING)
> +#define BR_PROMISC		0x00000080
>  
>  #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
>  	struct bridge_mcast_query	ip4_query;
> @@ -200,6 +201,7 @@ struct net_bridge_port
>  };
>  
>  #define br_is_auto_port(p) (((p)->flags & BR_AUTO_MASK) == BR_AUTO_MASK)
> +#define br_is_promisc_port(p) ((p)->flags & BR_PROMISC)
>  
>  #define br_port_exists(dev) (dev->priv_flags & IFF_BRIDGE_PORT)
>  
> -- 
> 1.9.0

Re: [RFC PATCH v2 net-next 2/7] bridge: Keep track of ports capable of automatic discovery.

[Vlad Yasevich]

On 04/30/2014 06:18 AM, Michael S. Tsirkin wrote:
> On Tue, Apr 29, 2014 at 05:20:23PM -0400, Vlad Yasevich wrote:
>> By default, ports on the bridge are capable of automatic
>> discovery of nodes located behind the port.  This is accomplished
>> via flooding of unknown traffic (BR_FLOOD) and learning the
>> mac addresses from these packets (BR_LEARNING).
>> If the above functionality is diabled by turning off these
>> flags, the port requires static configuration in the form
>> of static FDB entries to function properly.
>>
>> This patch adds functionality to keep track of all ports
>> capable of automatic discovery.  This will later be used
>> to control promiscuity settings.
>>
>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
>> ---
>>  net/bridge/br_if.c       | 27 +++++++++++++++++++++++++++
>>  net/bridge/br_netlink.c  |  3 +++
>>  net/bridge/br_private.h  |  6 ++++++
>>  net/bridge/br_sysfs_if.c |  6 +++++-
>>  4 files changed, 41 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
>> index 5262b86..7e60c4c 100644
>> --- a/net/bridge/br_if.c
>> +++ b/net/bridge/br_if.c
>> @@ -85,6 +85,18 @@ void br_port_carrier_check(struct net_bridge_port *p)
>>  	spin_unlock_bh(&br->lock);
>>  }
>>  
>> +static void nbp_update_port_count(struct net_bridge *br)
>> +{
>> +	struct net_bridge_port *p;
>> +	u32 cnt = 0;
>> +
>> +	list_for_each_entry(p, &br->port_list, list) {
>> +		if (br_is_auto_port(p))
>> +			cnt++;
>> +	}
>> +	br->auto_cnt = cnt;
>> +}
>> +
>>  static void release_nbp(struct kobject *kobj)
>>  {
>>  	struct net_bridge_port *p
>> @@ -146,6 +158,8 @@ static void del_nbp(struct net_bridge_port *p)
>>  
>>  	list_del_rcu(&p->list);
>>  
>> +	nbp_update_port_count(br);
>> +
>>  	dev->priv_flags &= ~IFF_BRIDGE_PORT;
>>  
>>  	netdev_rx_handler_unregister(dev);
>> @@ -384,6 +398,8 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
>>  
>>  	list_add_rcu(&p->list, &br->port_list);
>>  
>> +	nbp_update_port_count(br);
>> +
>>  	netdev_update_features(br->dev);
>>  
>>  	if (br->dev->needed_headroom < dev->needed_headroom)
>> @@ -455,3 +471,14 @@ int br_del_if(struct net_bridge *br, struct net_device *dev)
>>  
>>  	return 0;
>>  }
>> +
>> +void br_port_flags_change(struct net_bridge_port *p, unsigned long old_flags)
>> +{
>> +	struct net_bridge *br = p->br;
>> +	unsigned long mask = old_flags ^ p->flags;
>> +
>> +	if (!(mask & BR_AUTO_MASK))
>> +		return;
>> +
>> +	nbp_update_port_count(br);
>> +}
>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>> index e74b6d53..01382b9 100644
>> --- a/net/bridge/br_netlink.c
>> +++ b/net/bridge/br_netlink.c
>> @@ -328,6 +328,7 @@ 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 old_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);
>> @@ -353,6 +354,8 @@ static int br_setport(struct net_bridge_port *p, struct nlattr *tb[])
>>  		if (err)
>>  			return err;
>>  	}
>> +
>> +	br_port_flags_change(p, old_flags ^ p->flags);
>>  	return 0;
>>  }
>>  
>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>> index 06811d7..db714a1 100644
>> --- a/net/bridge/br_private.h
>> +++ b/net/bridge/br_private.h
>> @@ -174,6 +174,7 @@ struct net_bridge_port
>>  #define BR_ADMIN_COST		0x00000010
>>  #define BR_LEARNING		0x00000020
>>  #define BR_FLOOD		0x00000040
>> +#define BR_AUTO_MASK (BR_FLOOD | BR_LEARNING)
>>  
>>  #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
>>  	struct bridge_mcast_query	ip4_query;
>> @@ -198,6 +199,8 @@ struct net_bridge_port
>>  #endif
>>  };
>>  
>> +#define br_is_auto_port(p) (((p)->flags & BR_AUTO_MASK) == BR_AUTO_MASK)
>> +
> 
> I think you
> really want ((p)->flags & BR_AUTO_MASK) instead without
>  == BR_AUTO_MASK.
> 
> For example if learning is off but flood is on, things work
> (slowly) automatically without configuring static entries.
> 
> OTOH if flood is off but learning is on, a bunch of packets
> might get dropped but eventually you might learn the
> mac and then networking will start working.
> 

Yes, you are completely correct.  I flipped the meaning of the
macro, but forgot to account correctly for it.

static == not flooding && not learning.
auto != static.

The reason I count "auto" ports is so I only have to keep 1 counter
around.  If I counted static, I'd also have to count all ports as well.
Not much of a difference, but it's one less counter.

Fill fix.
Thanks
-vlad

> 
>>  #define br_port_exists(dev) (dev->priv_flags & IFF_BRIDGE_PORT)
>>  
>>  static inline struct net_bridge_port *br_port_get_rcu(const struct net_device *dev)
>> @@ -290,6 +293,7 @@ struct net_bridge
>>  	struct timer_list		topology_change_timer;
>>  	struct timer_list		gc_timer;
>>  	struct kobject			*ifobj;
>> +	u32				auto_cnt;
>>  #ifdef CONFIG_BRIDGE_VLAN_FILTERING
>>  	u8				vlan_enabled;
>>  	struct net_port_vlans __rcu	*vlan_info;
>> @@ -415,6 +419,8 @@ int br_del_if(struct net_bridge *br, struct net_device *dev);
>>  int br_min_mtu(const struct net_bridge *br);
>>  netdev_features_t br_features_recompute(struct net_bridge *br,
>>  					netdev_features_t features);
>> +void br_port_flags_change(struct net_bridge_port *port,
>> +			  unsigned long old_flags);
>>  
>>  /* br_input.c */
>>  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 112a25e..58c26e3 100644
>> --- a/net/bridge/br_sysfs_if.c
>> +++ b/net/bridge/br_sysfs_if.c
>> @@ -49,7 +49,10 @@ static BRPORT_ATTR(_name, S_IRUGO | S_IWUSR,			\
>>  static int store_flag(struct net_bridge_port *p, unsigned long v,
>>  		     unsigned long mask)
>>  {
>> -	unsigned long flags = p->flags;
>> +	unsigned long flags;
>> +	unsigned long old_flags;
>> +
>> +	old_flags = flags = p->flags;
>>  
>>  	if (v)
>>  		flags |= mask;
>> @@ -58,6 +61,7 @@ static int store_flag(struct net_bridge_port *p, unsigned long v,
>>  
>>  	if (flags != p->flags) {
>>  		p->flags = flags;
>> +		br_port_flags_change(p, old_flags ^ flags);
>>  		br_ifinfo_notify(RTM_NEWLINK, p);
>>  	}
>>  	return 0;
>> -- 
>> 1.9.0
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [Bridge] [RFC PATCH v2 net-next 2/7] bridge: Keep track of ports capable of automatic discovery.

[Vlad Yasevich]

On 04/30/2014 06:18 AM, Michael S. Tsirkin wrote:
> On Tue, Apr 29, 2014 at 05:20:23PM -0400, Vlad Yasevich wrote:
>> By default, ports on the bridge are capable of automatic
>> discovery of nodes located behind the port.  This is accomplished
>> via flooding of unknown traffic (BR_FLOOD) and learning the
>> mac addresses from these packets (BR_LEARNING).
>> If the above functionality is diabled by turning off these
>> flags, the port requires static configuration in the form
>> of static FDB entries to function properly.
>>
>> This patch adds functionality to keep track of all ports
>> capable of automatic discovery.  This will later be used
>> to control promiscuity settings.
>>
>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
>> ---
>>  net/bridge/br_if.c       | 27 +++++++++++++++++++++++++++
>>  net/bridge/br_netlink.c  |  3 +++
>>  net/bridge/br_private.h  |  6 ++++++
>>  net/bridge/br_sysfs_if.c |  6 +++++-
>>  4 files changed, 41 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
>> index 5262b86..7e60c4c 100644
>> --- a/net/bridge/br_if.c
>> +++ b/net/bridge/br_if.c
>> @@ -85,6 +85,18 @@ void br_port_carrier_check(struct net_bridge_port *p)
>>  	spin_unlock_bh(&br->lock);
>>  }
>>  
>> +static void nbp_update_port_count(struct net_bridge *br)
>> +{
>> +	struct net_bridge_port *p;
>> +	u32 cnt = 0;
>> +
>> +	list_for_each_entry(p, &br->port_list, list) {
>> +		if (br_is_auto_port(p))
>> +			cnt++;
>> +	}
>> +	br->auto_cnt = cnt;
>> +}
>> +
>>  static void release_nbp(struct kobject *kobj)
>>  {
>>  	struct net_bridge_port *p
>> @@ -146,6 +158,8 @@ static void del_nbp(struct net_bridge_port *p)
>>  
>>  	list_del_rcu(&p->list);
>>  
>> +	nbp_update_port_count(br);
>> +
>>  	dev->priv_flags &= ~IFF_BRIDGE_PORT;
>>  
>>  	netdev_rx_handler_unregister(dev);
>> @@ -384,6 +398,8 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
>>  
>>  	list_add_rcu(&p->list, &br->port_list);
>>  
>> +	nbp_update_port_count(br);
>> +
>>  	netdev_update_features(br->dev);
>>  
>>  	if (br->dev->needed_headroom < dev->needed_headroom)
>> @@ -455,3 +471,14 @@ int br_del_if(struct net_bridge *br, struct net_device *dev)
>>  
>>  	return 0;
>>  }
>> +
>> +void br_port_flags_change(struct net_bridge_port *p, unsigned long old_flags)
>> +{
>> +	struct net_bridge *br = p->br;
>> +	unsigned long mask = old_flags ^ p->flags;
>> +
>> +	if (!(mask & BR_AUTO_MASK))
>> +		return;
>> +
>> +	nbp_update_port_count(br);
>> +}
>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>> index e74b6d53..01382b9 100644
>> --- a/net/bridge/br_netlink.c
>> +++ b/net/bridge/br_netlink.c
>> @@ -328,6 +328,7 @@ 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 old_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);
>> @@ -353,6 +354,8 @@ static int br_setport(struct net_bridge_port *p, struct nlattr *tb[])
>>  		if (err)
>>  			return err;
>>  	}
>> +
>> +	br_port_flags_change(p, old_flags ^ p->flags);
>>  	return 0;
>>  }
>>  
>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>> index 06811d7..db714a1 100644
>> --- a/net/bridge/br_private.h
>> +++ b/net/bridge/br_private.h
>> @@ -174,6 +174,7 @@ struct net_bridge_port
>>  #define BR_ADMIN_COST		0x00000010
>>  #define BR_LEARNING		0x00000020
>>  #define BR_FLOOD		0x00000040
>> +#define BR_AUTO_MASK (BR_FLOOD | BR_LEARNING)
>>  
>>  #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
>>  	struct bridge_mcast_query	ip4_query;
>> @@ -198,6 +199,8 @@ struct net_bridge_port
>>  #endif
>>  };
>>  
>> +#define br_is_auto_port(p) (((p)->flags & BR_AUTO_MASK) == BR_AUTO_MASK)
>> +
> 
> I think you
> really want ((p)->flags & BR_AUTO_MASK) instead without
>  == BR_AUTO_MASK.
> 
> For example if learning is off but flood is on, things work
> (slowly) automatically without configuring static entries.
> 
> OTOH if flood is off but learning is on, a bunch of packets
> might get dropped but eventually you might learn the
> mac and then networking will start working.
> 

Yes, you are completely correct.  I flipped the meaning of the
macro, but forgot to account correctly for it.

static == not flooding && not learning.
auto != static.

The reason I count "auto" ports is so I only have to keep 1 counter
around.  If I counted static, I'd also have to count all ports as well.
Not much of a difference, but it's one less counter.

Fill fix.
Thanks
-vlad

> 
>>  #define br_port_exists(dev) (dev->priv_flags & IFF_BRIDGE_PORT)
>>  
>>  static inline struct net_bridge_port *br_port_get_rcu(const struct net_device *dev)
>> @@ -290,6 +293,7 @@ struct net_bridge
>>  	struct timer_list		topology_change_timer;
>>  	struct timer_list		gc_timer;
>>  	struct kobject			*ifobj;
>> +	u32				auto_cnt;
>>  #ifdef CONFIG_BRIDGE_VLAN_FILTERING
>>  	u8				vlan_enabled;
>>  	struct net_port_vlans __rcu	*vlan_info;
>> @@ -415,6 +419,8 @@ int br_del_if(struct net_bridge *br, struct net_device *dev);
>>  int br_min_mtu(const struct net_bridge *br);
>>  netdev_features_t br_features_recompute(struct net_bridge *br,
>>  					netdev_features_t features);
>> +void br_port_flags_change(struct net_bridge_port *port,
>> +			  unsigned long old_flags);
>>  
>>  /* br_input.c */
>>  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 112a25e..58c26e3 100644
>> --- a/net/bridge/br_sysfs_if.c
>> +++ b/net/bridge/br_sysfs_if.c
>> @@ -49,7 +49,10 @@ static BRPORT_ATTR(_name, S_IRUGO | S_IWUSR,			\
>>  static int store_flag(struct net_bridge_port *p, unsigned long v,
>>  		     unsigned long mask)
>>  {
>> -	unsigned long flags = p->flags;
>> +	unsigned long flags;
>> +	unsigned long old_flags;
>> +
>> +	old_flags = flags = p->flags;
>>  
>>  	if (v)
>>  		flags |= mask;
>> @@ -58,6 +61,7 @@ static int store_flag(struct net_bridge_port *p, unsigned long v,
>>  
>>  	if (flags != p->flags) {
>>  		p->flags = flags;
>> +		br_port_flags_change(p, old_flags ^ flags);
>>  		br_ifinfo_notify(RTM_NEWLINK, p);
>>  	}
>>  	return 0;
>> -- 
>> 1.9.0
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [RFC PATCH v2 net-next 4/7] bridge: Automatically manage port promiscuous mode.

[Vlad Yasevich]

On 04/30/2014 07:46 AM, Michael S. Tsirkin wrote:
> On Tue, Apr 29, 2014 at 05:20:25PM -0400, Vlad Yasevich wrote:
>> When there is only 1 port that can learn mac addresses and flood
>> unknown traffic, this port can function in non-promiscouse mode.
> 
> typo: non-promiscous
> 
> I think the code is correct here. But I'm not sure the explanation
> addresses the question that has been raised in the past:
> why is it correct to tie the output (flood) and input (learning)
> together and have it affect input (promisc).
> 

I found that thinking of the code in terms of possible configuration
examples helped clarify the behavior in my head.

> I would offer the following explanation to address the above
> question:
> 
> 
> Consider a specific port X. All traffic on its input will be
> output through one of the other ports (or dropped).
> Thus if *no* other port need to flood traffic
> on its output, then the combination of FDB entries for other
> ports completely specifies the set of legal mac addresses on the input
> of port X.

Right.  I tried to lead to this with the first two examples where one is
an extension of another.  It is sometimes easier to think about the
behavior when you have specifics instead of abstract concepts.
May be I didn't do this very well.

> 
> If, additionally, learning is disabled for all other ports,
> these FDB entries for other ports are static, so this set
> of mac addresses on port X does not change much.
> 
> This patch detects that no other port needs to flood or has learning
> enabled, and disables promisc mode for port X, instead programming the
> set of output mac addresses for other ports from the static fdb into the
> hardware filtering table in the underlying device.
> 
> Note: an alternative would be to simply check that no other port needs
> to flood, however, this would mean that non-static FDB updates by
> learning on other ports need to be propagated to hardware on port X,
> which is harder to implement efficiently.

Not only this, but without some sort of mac validation, this is a source
of attack against the bridge as it a single malicious node can now
saturate the hw filter and cause the bridge to consume extra memory.

> 
> It might be a good idea to put something like this above in
> commit log, along with the examples you provide below.
> A shortened version might be helpful in code comments as well.
> 
>> The simplest example of this configuration is a bridge with only
>> 1 port.  In this case, the bridge can't forward traffic anywhere.
>> All it can do is recevie traffic destined to it.  It doesn't need
>> to the interface into promiscouse mode.
>> A more complicated example is a bridge with 2 or more ports where
>> only 1 port remains capable of auto-discover and all others disable
>> mac learning and flooding thus requiring static configuration.  You
>> could think of this configuraiton as mimicking and edge relay with
>> 1 uplink port (the one can still learn things), and N downlink ports
>> that have to be manually programmed by user or managment entity.
>> In this case, the 1 uplink doesn't really have
>> to be promiscous either since the bridge would have all the necessary
>> data in the fdb through static configuration.  This one port can be
>> manually programmed to allow to recieve necessary traffic.
>> Another case where there is no need for promiscuous mode is when
>> there are no "proverbial" uplinks and all ports are manually managed.
>> All necessary information will be provided, so anything an interface
>> receives in promiscouse mode is extra and could even be considered
>> security threat.
> 
> Might be a good idea to clarify: the security problem is that with
> hardware filtering by MAC being off, it's easier to cause load spikes on
> multiple bridges on the LAN.
> 
> 
>> This patch supports the above scenarios.
>>
>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> 
> 
> I think what this patchset is trying to do correct.
> Minor comments below.
> 
> 
> Would bisect hit partially broken configurations if
> it picks patch 4 but not 5-7?
> If yes it's best to smash patches 5-7 in with this one.

I think I can restructure patch 5 so that it can be before this one
and stand on its own.
Patches 6 and 7 could potentially break some configurations during
bisect.  They are small enough that they can be squashed, but anything
that makes this patch more complicated to review is not so good IMO.
I'll give it a go and see what it looks like.

> 
> 
>> ---
>>  net/bridge/br_if.c      | 85 +++++++++++++++++++++++++++++++++++++++++++++----
>>  net/bridge/br_private.h |  2 ++
>>  2 files changed, 81 insertions(+), 6 deletions(-)
>>
>> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
>> index 7e60c4c..5a26ca2 100644
>> --- a/net/bridge/br_if.c
>> +++ b/net/bridge/br_if.c
>> @@ -85,6 +85,64 @@ void br_port_carrier_check(struct net_bridge_port *p)
>>  	spin_unlock_bh(&br->lock);
>>  }
>>  
>> +static void br_port_set_promisc(struct net_bridge_port *p)
>> +{
>> +	int err = 0;
>> +
>> +	if (br_is_promisc_port(p))
>> +		return;
>> +
>> +	err = dev_set_promiscuity(p->dev, 1);
>> +	if (err)
>> +		return;
> 
> Can this happen here? We can't recover so maybe warn if it does?
> 

It can happen if the device promiscuity count overflows.  If that
happens a warning is already printed.
The good thing is that the device is already in promisc mode so
everything still works.

>> +
>> +	br_fdb_unsync_static(p->br, p);
>> +	p->flags |= BR_PROMISC;
>> +}
>> +
>> +static void br_port_clear_promisc(struct net_bridge_port *p)
>> +{
>> +	int err;
>> +
>> +	if (!br_is_promisc_port(p))
>> +		return;
>> +
>> +	/* Since we'll be clearing the promisc mode, program the port
>> +	 * first so that we don't have interruption in traffic.
>> +	 */
>> +	err = br_fdb_sync_static(p->br, p);
>> +	if (err)
>> +		return;
>> +
>> +	dev_set_promiscuity(p->dev, -1);
>> +	p->flags &= ~BR_PROMISC;
>> +}
>> +
>> +/* When a port is added or removed or when certain port flags
>> + * change, this function is called to automatically mange
>> + * promiscuity setting of all the bridge ports.  We are always called
>> + * under RTNL so can skip using rcu primitives.
>> + */
>> +static void br_manage_promisc(struct net_bridge *br)
>> +{
>> +	struct net_bridge_port *p;
>> +
>> +	/* Algorithm is simple.
> 
> I think it's best to drop this line.
> It doesn't really clarify anything :)
> 
>>  If all the port
> 
> all the ports?
> 
>> require static
>> +	 * configuration, we know everything and can simply write
>> +	 * that down to the ports and clear promisc.
>> +	 * If only 1 port is automatic and all the others require
>> +	 * static configuration, we can write all the static data
>> +	 * to this one automatic port and still make non-promisc.
> 
> 
> make *this port* non promisc.
> 
>> +	 */
> 
> It's best to move this comment within list_for_each_entry,
> near the code it documents.
> 

Will fix

>> +	list_for_each_entry(p, &br->port_list, list) {
>> +		if (br->auto_cnt == 0 ||
>> +		    (br->auto_cnt == 1 && br_is_auto_port(p)))
> 
> 
> This is correct but I think it would be clearer as follows:
> 
> 	list_for_each_entry(p, &br->port_list, list) {
> 		/* If this is the only automatic port, others are not automatic so
> 		 * their output configuration is determined by static fdb entries.
> 		 * Thus, since input on this port will become output on one of the
> 		 * others, this means our input configuration is static: write it out to
> 		 * hardware and disable promiscous mode.
> 		 * Otherwise, make the port promiscous.
> 		 */
> 		if (br->auto_cnt <= br_is_auto_port(p))
> 				br_port_clear_promisc(p);
> 			else
> 				br_port_set_promisc(p);
> 	}
> 
> 
> instead of enumerating cases in comments and in code like you did above.
> 

br_is_auto_port give a boolean.  I suppose I can make it return an
integer, but that IMO makes it more confusing.  I can questions about
why do we clear it when there are no automatic ports.  I was trying to
be very explicit in the conditions.

Thanks
-vlad

> 	
> 
> 
>> +			br_port_clear_promisc(p);
>> +		else
>> +			br_port_set_promisc(p);
>> +	}
>> +}
>> +
>>  static void nbp_update_port_count(struct net_bridge *br)
>>  {
>>  	struct net_bridge_port *p;
>> @@ -94,7 +152,23 @@ static void nbp_update_port_count(struct net_bridge *br)
>>  		if (br_is_auto_port(p))
>>  			cnt++;
>>  	}
>> -	br->auto_cnt = cnt;
>> +	if (br->auto_cnt != cnt) {
>> +		br->auto_cnt = cnt;
>> +		br_manage_promisc(br);
>> +	}
>> +}
>> +
>> +static void nbp_delete_promisc(struct net_bridge_port *p)
>> +{
>> +	/* If port is currently promiscous, unset promiscuity.
>> +	 * Otherwise, it is a static port so remove all addresses
>> +	 * from it.
>> +	 */
>> +	dev_set_allmulti(p->dev, -1);
>> +	if (br_is_promisc_port(p))
>> +		dev_set_promiscuity(p->dev, -1);
>> +	else
>> +		br_fdb_unsync_static(p->br, p);
>>  }
>>  
>>  static void release_nbp(struct kobject *kobj)
>> @@ -145,7 +219,7 @@ static void del_nbp(struct net_bridge_port *p)
>>  
>>  	sysfs_remove_link(br->ifobj, p->dev->name);
>>  
>> -	dev_set_promiscuity(dev, -1);
>> +	nbp_delete_promisc(p);
>>  
>>  	spin_lock_bh(&br->lock);
>>  	br_stp_disable_port(p);
>> @@ -153,11 +227,10 @@ static void del_nbp(struct net_bridge_port *p)
>>  
>>  	br_ifinfo_notify(RTM_DELLINK, p);
>>  
>> -	nbp_vlan_flush(p);
>> -	br_fdb_delete_by_port(br, p, 1);
>> -
>>  	list_del_rcu(&p->list);
>>  
>> +	nbp_vlan_flush(p);
>> +	br_fdb_delete_by_port(br, p, 1);
>>  	nbp_update_port_count(br);
>>  
>>  	dev->priv_flags &= ~IFF_BRIDGE_PORT;
>> @@ -367,7 +440,7 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
>>  
>>  	call_netdevice_notifiers(NETDEV_JOIN, dev);
>>  
>> -	err = dev_set_promiscuity(dev, 1);
>> +	err = dev_set_allmulti(dev, 1);
>>  	if (err)
>>  		goto put_back;
>>  
>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>> index 2023671..1c794fef 100644
>> --- a/net/bridge/br_private.h
>> +++ b/net/bridge/br_private.h
>> @@ -175,6 +175,7 @@ struct net_bridge_port
>>  #define BR_LEARNING		0x00000020
>>  #define BR_FLOOD		0x00000040
>>  #define BR_AUTO_MASK (BR_FLOOD | BR_LEARNING)
>> +#define BR_PROMISC		0x00000080
>>  
>>  #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
>>  	struct bridge_mcast_query	ip4_query;
>> @@ -200,6 +201,7 @@ struct net_bridge_port
>>  };
>>  
>>  #define br_is_auto_port(p) (((p)->flags & BR_AUTO_MASK) == BR_AUTO_MASK)
>> +#define br_is_promisc_port(p) ((p)->flags & BR_PROMISC)
>>  
>>  #define br_port_exists(dev) (dev->priv_flags & IFF_BRIDGE_PORT)
>>  
>> -- 
>> 1.9.0
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [Bridge] [RFC PATCH v2 net-next 4/7] bridge: Automatically manage port promiscuous mode.

[Vlad Yasevich]

On 04/30/2014 07:46 AM, Michael S. Tsirkin wrote:
> On Tue, Apr 29, 2014 at 05:20:25PM -0400, Vlad Yasevich wrote:
>> When there is only 1 port that can learn mac addresses and flood
>> unknown traffic, this port can function in non-promiscouse mode.
> 
> typo: non-promiscous
> 
> I think the code is correct here. But I'm not sure the explanation
> addresses the question that has been raised in the past:
> why is it correct to tie the output (flood) and input (learning)
> together and have it affect input (promisc).
> 

I found that thinking of the code in terms of possible configuration
examples helped clarify the behavior in my head.

> I would offer the following explanation to address the above
> question:
> 
> 
> Consider a specific port X. All traffic on its input will be
> output through one of the other ports (or dropped).
> Thus if *no* other port need to flood traffic
> on its output, then the combination of FDB entries for other
> ports completely specifies the set of legal mac addresses on the input
> of port X.

Right.  I tried to lead to this with the first two examples where one is
an extension of another.  It is sometimes easier to think about the
behavior when you have specifics instead of abstract concepts.
May be I didn't do this very well.

> 
> If, additionally, learning is disabled for all other ports,
> these FDB entries for other ports are static, so this set
> of mac addresses on port X does not change much.
> 
> This patch detects that no other port needs to flood or has learning
> enabled, and disables promisc mode for port X, instead programming the
> set of output mac addresses for other ports from the static fdb into the
> hardware filtering table in the underlying device.
> 
> Note: an alternative would be to simply check that no other port needs
> to flood, however, this would mean that non-static FDB updates by
> learning on other ports need to be propagated to hardware on port X,
> which is harder to implement efficiently.

Not only this, but without some sort of mac validation, this is a source
of attack against the bridge as it a single malicious node can now
saturate the hw filter and cause the bridge to consume extra memory.

> 
> It might be a good idea to put something like this above in
> commit log, along with the examples you provide below.
> A shortened version might be helpful in code comments as well.
> 
>> The simplest example of this configuration is a bridge with only
>> 1 port.  In this case, the bridge can't forward traffic anywhere.
>> All it can do is recevie traffic destined to it.  It doesn't need
>> to the interface into promiscouse mode.
>> A more complicated example is a bridge with 2 or more ports where
>> only 1 port remains capable of auto-discover and all others disable
>> mac learning and flooding thus requiring static configuration.  You
>> could think of this configuraiton as mimicking and edge relay with
>> 1 uplink port (the one can still learn things), and N downlink ports
>> that have to be manually programmed by user or managment entity.
>> In this case, the 1 uplink doesn't really have
>> to be promiscous either since the bridge would have all the necessary
>> data in the fdb through static configuration.  This one port can be
>> manually programmed to allow to recieve necessary traffic.
>> Another case where there is no need for promiscuous mode is when
>> there are no "proverbial" uplinks and all ports are manually managed.
>> All necessary information will be provided, so anything an interface
>> receives in promiscouse mode is extra and could even be considered
>> security threat.
> 
> Might be a good idea to clarify: the security problem is that with
> hardware filtering by MAC being off, it's easier to cause load spikes on
> multiple bridges on the LAN.
> 
> 
>> This patch supports the above scenarios.
>>
>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> 
> 
> I think what this patchset is trying to do correct.
> Minor comments below.
> 
> 
> Would bisect hit partially broken configurations if
> it picks patch 4 but not 5-7?
> If yes it's best to smash patches 5-7 in with this one.

I think I can restructure patch 5 so that it can be before this one
and stand on its own.
Patches 6 and 7 could potentially break some configurations during
bisect.  They are small enough that they can be squashed, but anything
that makes this patch more complicated to review is not so good IMO.
I'll give it a go and see what it looks like.

> 
> 
>> ---
>>  net/bridge/br_if.c      | 85 +++++++++++++++++++++++++++++++++++++++++++++----
>>  net/bridge/br_private.h |  2 ++
>>  2 files changed, 81 insertions(+), 6 deletions(-)
>>
>> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
>> index 7e60c4c..5a26ca2 100644
>> --- a/net/bridge/br_if.c
>> +++ b/net/bridge/br_if.c
>> @@ -85,6 +85,64 @@ void br_port_carrier_check(struct net_bridge_port *p)
>>  	spin_unlock_bh(&br->lock);
>>  }
>>  
>> +static void br_port_set_promisc(struct net_bridge_port *p)
>> +{
>> +	int err = 0;
>> +
>> +	if (br_is_promisc_port(p))
>> +		return;
>> +
>> +	err = dev_set_promiscuity(p->dev, 1);
>> +	if (err)
>> +		return;
> 
> Can this happen here? We can't recover so maybe warn if it does?
> 

It can happen if the device promiscuity count overflows.  If that
happens a warning is already printed.
The good thing is that the device is already in promisc mode so
everything still works.

>> +
>> +	br_fdb_unsync_static(p->br, p);
>> +	p->flags |= BR_PROMISC;
>> +}
>> +
>> +static void br_port_clear_promisc(struct net_bridge_port *p)
>> +{
>> +	int err;
>> +
>> +	if (!br_is_promisc_port(p))
>> +		return;
>> +
>> +	/* Since we'll be clearing the promisc mode, program the port
>> +	 * first so that we don't have interruption in traffic.
>> +	 */
>> +	err = br_fdb_sync_static(p->br, p);
>> +	if (err)
>> +		return;
>> +
>> +	dev_set_promiscuity(p->dev, -1);
>> +	p->flags &= ~BR_PROMISC;
>> +}
>> +
>> +/* When a port is added or removed or when certain port flags
>> + * change, this function is called to automatically mange
>> + * promiscuity setting of all the bridge ports.  We are always called
>> + * under RTNL so can skip using rcu primitives.
>> + */
>> +static void br_manage_promisc(struct net_bridge *br)
>> +{
>> +	struct net_bridge_port *p;
>> +
>> +	/* Algorithm is simple.
> 
> I think it's best to drop this line.
> It doesn't really clarify anything :)
> 
>>  If all the port
> 
> all the ports?
> 
>> require static
>> +	 * configuration, we know everything and can simply write
>> +	 * that down to the ports and clear promisc.
>> +	 * If only 1 port is automatic and all the others require
>> +	 * static configuration, we can write all the static data
>> +	 * to this one automatic port and still make non-promisc.
> 
> 
> make *this port* non promisc.
> 
>> +	 */
> 
> It's best to move this comment within list_for_each_entry,
> near the code it documents.
> 

Will fix

>> +	list_for_each_entry(p, &br->port_list, list) {
>> +		if (br->auto_cnt == 0 ||
>> +		    (br->auto_cnt == 1 && br_is_auto_port(p)))
> 
> 
> This is correct but I think it would be clearer as follows:
> 
> 	list_for_each_entry(p, &br->port_list, list) {
> 		/* If this is the only automatic port, others are not automatic so
> 		 * their output configuration is determined by static fdb entries.
> 		 * Thus, since input on this port will become output on one of the
> 		 * others, this means our input configuration is static: write it out to
> 		 * hardware and disable promiscous mode.
> 		 * Otherwise, make the port promiscous.
> 		 */
> 		if (br->auto_cnt <= br_is_auto_port(p))
> 				br_port_clear_promisc(p);
> 			else
> 				br_port_set_promisc(p);
> 	}
> 
> 
> instead of enumerating cases in comments and in code like you did above.
> 

br_is_auto_port give a boolean.  I suppose I can make it return an
integer, but that IMO makes it more confusing.  I can questions about
why do we clear it when there are no automatic ports.  I was trying to
be very explicit in the conditions.

Thanks
-vlad

> 	
> 
> 
>> +			br_port_clear_promisc(p);
>> +		else
>> +			br_port_set_promisc(p);
>> +	}
>> +}
>> +
>>  static void nbp_update_port_count(struct net_bridge *br)
>>  {
>>  	struct net_bridge_port *p;
>> @@ -94,7 +152,23 @@ static void nbp_update_port_count(struct net_bridge *br)
>>  		if (br_is_auto_port(p))
>>  			cnt++;
>>  	}
>> -	br->auto_cnt = cnt;
>> +	if (br->auto_cnt != cnt) {
>> +		br->auto_cnt = cnt;
>> +		br_manage_promisc(br);
>> +	}
>> +}
>> +
>> +static void nbp_delete_promisc(struct net_bridge_port *p)
>> +{
>> +	/* If port is currently promiscous, unset promiscuity.
>> +	 * Otherwise, it is a static port so remove all addresses
>> +	 * from it.
>> +	 */
>> +	dev_set_allmulti(p->dev, -1);
>> +	if (br_is_promisc_port(p))
>> +		dev_set_promiscuity(p->dev, -1);
>> +	else
>> +		br_fdb_unsync_static(p->br, p);
>>  }
>>  
>>  static void release_nbp(struct kobject *kobj)
>> @@ -145,7 +219,7 @@ static void del_nbp(struct net_bridge_port *p)
>>  
>>  	sysfs_remove_link(br->ifobj, p->dev->name);
>>  
>> -	dev_set_promiscuity(dev, -1);
>> +	nbp_delete_promisc(p);
>>  
>>  	spin_lock_bh(&br->lock);
>>  	br_stp_disable_port(p);
>> @@ -153,11 +227,10 @@ static void del_nbp(struct net_bridge_port *p)
>>  
>>  	br_ifinfo_notify(RTM_DELLINK, p);
>>  
>> -	nbp_vlan_flush(p);
>> -	br_fdb_delete_by_port(br, p, 1);
>> -
>>  	list_del_rcu(&p->list);
>>  
>> +	nbp_vlan_flush(p);
>> +	br_fdb_delete_by_port(br, p, 1);
>>  	nbp_update_port_count(br);
>>  
>>  	dev->priv_flags &= ~IFF_BRIDGE_PORT;
>> @@ -367,7 +440,7 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
>>  
>>  	call_netdevice_notifiers(NETDEV_JOIN, dev);
>>  
>> -	err = dev_set_promiscuity(dev, 1);
>> +	err = dev_set_allmulti(dev, 1);
>>  	if (err)
>>  		goto put_back;
>>  
>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>> index 2023671..1c794fef 100644
>> --- a/net/bridge/br_private.h
>> +++ b/net/bridge/br_private.h
>> @@ -175,6 +175,7 @@ struct net_bridge_port
>>  #define BR_LEARNING		0x00000020
>>  #define BR_FLOOD		0x00000040
>>  #define BR_AUTO_MASK (BR_FLOOD | BR_LEARNING)
>> +#define BR_PROMISC		0x00000080
>>  
>>  #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
>>  	struct bridge_mcast_query	ip4_query;
>> @@ -200,6 +201,7 @@ struct net_bridge_port
>>  };
>>  
>>  #define br_is_auto_port(p) (((p)->flags & BR_AUTO_MASK) == BR_AUTO_MASK)
>> +#define br_is_promisc_port(p) ((p)->flags & BR_PROMISC)
>>  
>>  #define br_port_exists(dev) (dev->priv_flags & IFF_BRIDGE_PORT)
>>  
>> -- 
>> 1.9.0
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [RFC PATCH v2 net-next 4/7] bridge: Automatically manage port promiscuous mode.

[Michael S. Tsirkin]

On Wed, Apr 30, 2014 at 10:17:21AM -0400, Vlad Yasevich wrote:
> On 04/30/2014 07:46 AM, Michael S. Tsirkin wrote:
> > On Tue, Apr 29, 2014 at 05:20:25PM -0400, Vlad Yasevich wrote:
> >> When there is only 1 port that can learn mac addresses and flood
> >> unknown traffic, this port can function in non-promiscouse mode.
> > 
> > typo: non-promiscous
> > 
> > I think the code is correct here. But I'm not sure the explanation
> > addresses the question that has been raised in the past:
> > why is it correct to tie the output (flood) and input (learning)
> > together and have it affect input (promisc).
> > 
> 
> I found that thinking of the code in terms of possible configuration
> examples helped clarify the behavior in my head.

I guess different people just think differently.
Maybe include both the general rule, then add:
examples: and list them.

> > I would offer the following explanation to address the above
> > question:
> > 
> > 
> > Consider a specific port X. All traffic on its input will be
> > output through one of the other ports (or dropped).
> > Thus if *no* other port need to flood traffic
> > on its output, then the combination of FDB entries for other
> > ports completely specifies the set of legal mac addresses on the input
> > of port X.
> 
> Right.  I tried to lead to this with the first two examples where one is
> an extension of another.  It is sometimes easier to think about the
> behavior when you have specifics instead of abstract concepts.
> May be I didn't do this very well.
> 
> > 
> > If, additionally, learning is disabled for all other ports,
> > these FDB entries for other ports are static, so this set
> > of mac addresses on port X does not change much.
> > 
> > This patch detects that no other port needs to flood or has learning
> > enabled, and disables promisc mode for port X, instead programming the
> > set of output mac addresses for other ports from the static fdb into the
> > hardware filtering table in the underlying device.
> > 
> > Note: an alternative would be to simply check that no other port needs
> > to flood, however, this would mean that non-static FDB updates by
> > learning on other ports need to be propagated to hardware on port X,
> > which is harder to implement efficiently.
> 
> Not only this, but without some sort of mac validation, this is a source
> of attack against the bridge as it a single malicious node can now
> saturate the hw filter and cause the bridge to consume extra memory.
> > 
> > It might be a good idea to put something like this above in
> > commit log, along with the examples you provide below.
> > A shortened version might be helpful in code comments as well.
> > 
> >> The simplest example of this configuration is a bridge with only
> >> 1 port.  In this case, the bridge can't forward traffic anywhere.
> >> All it can do is recevie traffic destined to it.  It doesn't need
> >> to the interface into promiscouse mode.
> >> A more complicated example is a bridge with 2 or more ports where
> >> only 1 port remains capable of auto-discover and all others disable
> >> mac learning and flooding thus requiring static configuration.  You
> >> could think of this configuraiton as mimicking and edge relay with
> >> 1 uplink port (the one can still learn things), and N downlink ports
> >> that have to be manually programmed by user or managment entity.
> >> In this case, the 1 uplink doesn't really have
> >> to be promiscous either since the bridge would have all the necessary
> >> data in the fdb through static configuration.  This one port can be
> >> manually programmed to allow to recieve necessary traffic.
> >> Another case where there is no need for promiscuous mode is when
> >> there are no "proverbial" uplinks and all ports are manually managed.
> >> All necessary information will be provided, so anything an interface
> >> receives in promiscouse mode is extra and could even be considered
> >> security threat.
> > 
> > Might be a good idea to clarify: the security problem is that with
> > hardware filtering by MAC being off, it's easier to cause load spikes on
> > multiple bridges on the LAN.
> > 
> > 
> >> This patch supports the above scenarios.
> >>
> >> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> > 
> > 
> > I think what this patchset is trying to do correct.
> > Minor comments below.
> > 
> > 
> > Would bisect hit partially broken configurations if
> > it picks patch 4 but not 5-7?
> > If yes it's best to smash patches 5-7 in with this one.
> 
> I think I can restructure patch 5 so that it can be before this one
> and stand on its own.
> Patches 6 and 7 could potentially break some configurations during
> bisect.  They are small enough that they can be squashed, but anything
> that makes this patch more complicated to review is not so good IMO.
> I'll give it a go and see what it looks like.
> 
> > 
> > 
> >> ---
> >>  net/bridge/br_if.c      | 85 +++++++++++++++++++++++++++++++++++++++++++++----
> >>  net/bridge/br_private.h |  2 ++
> >>  2 files changed, 81 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> >> index 7e60c4c..5a26ca2 100644
> >> --- a/net/bridge/br_if.c
> >> +++ b/net/bridge/br_if.c
> >> @@ -85,6 +85,64 @@ void br_port_carrier_check(struct net_bridge_port *p)
> >>  	spin_unlock_bh(&br->lock);
> >>  }
> >>  
> >> +static void br_port_set_promisc(struct net_bridge_port *p)
> >> +{
> >> +	int err = 0;
> >> +
> >> +	if (br_is_promisc_port(p))
> >> +		return;
> >> +
> >> +	err = dev_set_promiscuity(p->dev, 1);
> >> +	if (err)
> >> +		return;
> > 
> > Can this happen here? We can't recover so maybe warn if it does?
> > 
> 
> It can happen if the device promiscuity count overflows.  If that
> happens a warning is already printed.
> The good thing is that the device is already in promisc mode so
> everything still works.

I see. Yes I think that is enough.

> >> +
> >> +	br_fdb_unsync_static(p->br, p);
> >> +	p->flags |= BR_PROMISC;
> >> +}
> >> +
> >> +static void br_port_clear_promisc(struct net_bridge_port *p)
> >> +{
> >> +	int err;
> >> +
> >> +	if (!br_is_promisc_port(p))
> >> +		return;
> >> +
> >> +	/* Since we'll be clearing the promisc mode, program the port
> >> +	 * first so that we don't have interruption in traffic.
> >> +	 */
> >> +	err = br_fdb_sync_static(p->br, p);
> >> +	if (err)
> >> +		return;
> >> +
> >> +	dev_set_promiscuity(p->dev, -1);
> >> +	p->flags &= ~BR_PROMISC;
> >> +}
> >> +
> >> +/* When a port is added or removed or when certain port flags
> >> + * change, this function is called to automatically mange
> >> + * promiscuity setting of all the bridge ports.  We are always called
> >> + * under RTNL so can skip using rcu primitives.
> >> + */
> >> +static void br_manage_promisc(struct net_bridge *br)
> >> +{
> >> +	struct net_bridge_port *p;
> >> +
> >> +	/* Algorithm is simple.
> > 
> > I think it's best to drop this line.
> > It doesn't really clarify anything :)
> > 
> >>  If all the port
> > 
> > all the ports?
> > 
> >> require static
> >> +	 * configuration, we know everything and can simply write
> >> +	 * that down to the ports and clear promisc.
> >> +	 * If only 1 port is automatic and all the others require
> >> +	 * static configuration, we can write all the static data
> >> +	 * to this one automatic port and still make non-promisc.
> > 
> > 
> > make *this port* non promisc.
> > 
> >> +	 */
> > 
> > It's best to move this comment within list_for_each_entry,
> > near the code it documents.
> > 
> 
> Will fix
> 
> >> +	list_for_each_entry(p, &br->port_list, list) {
> >> +		if (br->auto_cnt == 0 ||
> >> +		    (br->auto_cnt == 1 && br_is_auto_port(p)))
> > 
> > 
> > This is correct but I think it would be clearer as follows:
> > 
> > 	list_for_each_entry(p, &br->port_list, list) {
> > 		/* If this is the only automatic port, others are not automatic so
> > 		 * their output configuration is determined by static fdb entries.
> > 		 * Thus, since input on this port will become output on one of the
> > 		 * others, this means our input configuration is static: write it out to
> > 		 * hardware and disable promiscous mode.
> > 		 * Otherwise, make the port promiscous.
> > 		 */
> > 		if (br->auto_cnt <= br_is_auto_port(p))
> > 				br_port_clear_promisc(p);
> > 			else
> > 				br_port_set_promisc(p);
> > 	}
> > 
> > 
> > instead of enumerating cases in comments and in code like you did above.
> > 
> 
> br_is_auto_port give a boolean.
>  I suppose I can make it return an
> integer, but that IMO makes it more confusing.

In C boolean is 0 or 1 so no need to change it to integer.

> I can questions about
> why do we clear it when there are no automatic ports.

Sorry don't understand this sentence.

You comment doesn't explicitly explain the case of auto_cnt == 0
either, but if you want to enumarate both examples maybe combine the
two comments and list all cases, e.g.

	Cases:
		auto_cnt == 0
			clear promisc
		auto_cnt == 1 br_is_auto_port == 1
			clear promisc
		auto_cnt == 1 br_is_auto_port == 0
			set promisc


> I was trying to
> be very explicit in the conditions.
> 
> Thanks
> -vlad


I think a general rule in code is always better than a
bunch of cases listed.
listing cases might be good for documentation / comments
but you need to check them against code anyway, so the
simpler the code the better.

> > 	
> > 
> > 
> >> +			br_port_clear_promisc(p);
> >> +		else
> >> +			br_port_set_promisc(p);
> >> +	}
> >> +}
> >> +
> >>  static void nbp_update_port_count(struct net_bridge *br)
> >>  {
> >>  	struct net_bridge_port *p;
> >> @@ -94,7 +152,23 @@ static void nbp_update_port_count(struct net_bridge *br)
> >>  		if (br_is_auto_port(p))
> >>  			cnt++;
> >>  	}
> >> -	br->auto_cnt = cnt;
> >> +	if (br->auto_cnt != cnt) {
> >> +		br->auto_cnt = cnt;
> >> +		br_manage_promisc(br);
> >> +	}
> >> +}
> >> +
> >> +static void nbp_delete_promisc(struct net_bridge_port *p)
> >> +{
> >> +	/* If port is currently promiscous, unset promiscuity.
> >> +	 * Otherwise, it is a static port so remove all addresses
> >> +	 * from it.
> >> +	 */
> >> +	dev_set_allmulti(p->dev, -1);
> >> +	if (br_is_promisc_port(p))
> >> +		dev_set_promiscuity(p->dev, -1);
> >> +	else
> >> +		br_fdb_unsync_static(p->br, p);
> >>  }
> >>  
> >>  static void release_nbp(struct kobject *kobj)
> >> @@ -145,7 +219,7 @@ static void del_nbp(struct net_bridge_port *p)
> >>  
> >>  	sysfs_remove_link(br->ifobj, p->dev->name);
> >>  
> >> -	dev_set_promiscuity(dev, -1);
> >> +	nbp_delete_promisc(p);
> >>  
> >>  	spin_lock_bh(&br->lock);
> >>  	br_stp_disable_port(p);
> >> @@ -153,11 +227,10 @@ static void del_nbp(struct net_bridge_port *p)
> >>  
> >>  	br_ifinfo_notify(RTM_DELLINK, p);
> >>  
> >> -	nbp_vlan_flush(p);
> >> -	br_fdb_delete_by_port(br, p, 1);
> >> -
> >>  	list_del_rcu(&p->list);
> >>  
> >> +	nbp_vlan_flush(p);
> >> +	br_fdb_delete_by_port(br, p, 1);
> >>  	nbp_update_port_count(br);
> >>  
> >>  	dev->priv_flags &= ~IFF_BRIDGE_PORT;
> >> @@ -367,7 +440,7 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
> >>  
> >>  	call_netdevice_notifiers(NETDEV_JOIN, dev);
> >>  
> >> -	err = dev_set_promiscuity(dev, 1);
> >> +	err = dev_set_allmulti(dev, 1);
> >>  	if (err)
> >>  		goto put_back;
> >>  
> >> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> >> index 2023671..1c794fef 100644
> >> --- a/net/bridge/br_private.h
> >> +++ b/net/bridge/br_private.h
> >> @@ -175,6 +175,7 @@ struct net_bridge_port
> >>  #define BR_LEARNING		0x00000020
> >>  #define BR_FLOOD		0x00000040
> >>  #define BR_AUTO_MASK (BR_FLOOD | BR_LEARNING)
> >> +#define BR_PROMISC		0x00000080
> >>  
> >>  #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
> >>  	struct bridge_mcast_query	ip4_query;
> >> @@ -200,6 +201,7 @@ struct net_bridge_port
> >>  };
> >>  
> >>  #define br_is_auto_port(p) (((p)->flags & BR_AUTO_MASK) == BR_AUTO_MASK)
> >> +#define br_is_promisc_port(p) ((p)->flags & BR_PROMISC)
> >>  
> >>  #define br_port_exists(dev) (dev->priv_flags & IFF_BRIDGE_PORT)
> >>  
> >> -- 
> >> 1.9.0
> > --
> > To unsubscribe from this list: send the line "unsubscribe netdev" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 

Re: [Bridge] [RFC PATCH v2 net-next 4/7] bridge: Automatically manage port promiscuous mode.

[Michael S. Tsirkin]

On Wed, Apr 30, 2014 at 10:17:21AM -0400, Vlad Yasevich wrote:
> On 04/30/2014 07:46 AM, Michael S. Tsirkin wrote:
> > On Tue, Apr 29, 2014 at 05:20:25PM -0400, Vlad Yasevich wrote:
> >> When there is only 1 port that can learn mac addresses and flood
> >> unknown traffic, this port can function in non-promiscouse mode.
> > 
> > typo: non-promiscous
> > 
> > I think the code is correct here. But I'm not sure the explanation
> > addresses the question that has been raised in the past:
> > why is it correct to tie the output (flood) and input (learning)
> > together and have it affect input (promisc).
> > 
> 
> I found that thinking of the code in terms of possible configuration
> examples helped clarify the behavior in my head.

I guess different people just think differently.
Maybe include both the general rule, then add:
examples: and list them.

> > I would offer the following explanation to address the above
> > question:
> > 
> > 
> > Consider a specific port X. All traffic on its input will be
> > output through one of the other ports (or dropped).
> > Thus if *no* other port need to flood traffic
> > on its output, then the combination of FDB entries for other
> > ports completely specifies the set of legal mac addresses on the input
> > of port X.
> 
> Right.  I tried to lead to this with the first two examples where one is
> an extension of another.  It is sometimes easier to think about the
> behavior when you have specifics instead of abstract concepts.
> May be I didn't do this very well.
> 
> > 
> > If, additionally, learning is disabled for all other ports,
> > these FDB entries for other ports are static, so this set
> > of mac addresses on port X does not change much.
> > 
> > This patch detects that no other port needs to flood or has learning
> > enabled, and disables promisc mode for port X, instead programming the
> > set of output mac addresses for other ports from the static fdb into the
> > hardware filtering table in the underlying device.
> > 
> > Note: an alternative would be to simply check that no other port needs
> > to flood, however, this would mean that non-static FDB updates by
> > learning on other ports need to be propagated to hardware on port X,
> > which is harder to implement efficiently.
> 
> Not only this, but without some sort of mac validation, this is a source
> of attack against the bridge as it a single malicious node can now
> saturate the hw filter and cause the bridge to consume extra memory.
> > 
> > It might be a good idea to put something like this above in
> > commit log, along with the examples you provide below.
> > A shortened version might be helpful in code comments as well.
> > 
> >> The simplest example of this configuration is a bridge with only
> >> 1 port.  In this case, the bridge can't forward traffic anywhere.
> >> All it can do is recevie traffic destined to it.  It doesn't need
> >> to the interface into promiscouse mode.
> >> A more complicated example is a bridge with 2 or more ports where
> >> only 1 port remains capable of auto-discover and all others disable
> >> mac learning and flooding thus requiring static configuration.  You
> >> could think of this configuraiton as mimicking and edge relay with
> >> 1 uplink port (the one can still learn things), and N downlink ports
> >> that have to be manually programmed by user or managment entity.
> >> In this case, the 1 uplink doesn't really have
> >> to be promiscous either since the bridge would have all the necessary
> >> data in the fdb through static configuration.  This one port can be
> >> manually programmed to allow to recieve necessary traffic.
> >> Another case where there is no need for promiscuous mode is when
> >> there are no "proverbial" uplinks and all ports are manually managed.
> >> All necessary information will be provided, so anything an interface
> >> receives in promiscouse mode is extra and could even be considered
> >> security threat.
> > 
> > Might be a good idea to clarify: the security problem is that with
> > hardware filtering by MAC being off, it's easier to cause load spikes on
> > multiple bridges on the LAN.
> > 
> > 
> >> This patch supports the above scenarios.
> >>
> >> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> > 
> > 
> > I think what this patchset is trying to do correct.
> > Minor comments below.
> > 
> > 
> > Would bisect hit partially broken configurations if
> > it picks patch 4 but not 5-7?
> > If yes it's best to smash patches 5-7 in with this one.
> 
> I think I can restructure patch 5 so that it can be before this one
> and stand on its own.
> Patches 6 and 7 could potentially break some configurations during
> bisect.  They are small enough that they can be squashed, but anything
> that makes this patch more complicated to review is not so good IMO.
> I'll give it a go and see what it looks like.
> 
> > 
> > 
> >> ---
> >>  net/bridge/br_if.c      | 85 +++++++++++++++++++++++++++++++++++++++++++++----
> >>  net/bridge/br_private.h |  2 ++
> >>  2 files changed, 81 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> >> index 7e60c4c..5a26ca2 100644
> >> --- a/net/bridge/br_if.c
> >> +++ b/net/bridge/br_if.c
> >> @@ -85,6 +85,64 @@ void br_port_carrier_check(struct net_bridge_port *p)
> >>  	spin_unlock_bh(&br->lock);
> >>  }
> >>  
> >> +static void br_port_set_promisc(struct net_bridge_port *p)
> >> +{
> >> +	int err = 0;
> >> +
> >> +	if (br_is_promisc_port(p))
> >> +		return;
> >> +
> >> +	err = dev_set_promiscuity(p->dev, 1);
> >> +	if (err)
> >> +		return;
> > 
> > Can this happen here? We can't recover so maybe warn if it does?
> > 
> 
> It can happen if the device promiscuity count overflows.  If that
> happens a warning is already printed.
> The good thing is that the device is already in promisc mode so
> everything still works.

I see. Yes I think that is enough.

> >> +
> >> +	br_fdb_unsync_static(p->br, p);
> >> +	p->flags |= BR_PROMISC;
> >> +}
> >> +
> >> +static void br_port_clear_promisc(struct net_bridge_port *p)
> >> +{
> >> +	int err;
> >> +
> >> +	if (!br_is_promisc_port(p))
> >> +		return;
> >> +
> >> +	/* Since we'll be clearing the promisc mode, program the port
> >> +	 * first so that we don't have interruption in traffic.
> >> +	 */
> >> +	err = br_fdb_sync_static(p->br, p);
> >> +	if (err)
> >> +		return;
> >> +
> >> +	dev_set_promiscuity(p->dev, -1);
> >> +	p->flags &= ~BR_PROMISC;
> >> +}
> >> +
> >> +/* When a port is added or removed or when certain port flags
> >> + * change, this function is called to automatically mange
> >> + * promiscuity setting of all the bridge ports.  We are always called
> >> + * under RTNL so can skip using rcu primitives.
> >> + */
> >> +static void br_manage_promisc(struct net_bridge *br)
> >> +{
> >> +	struct net_bridge_port *p;
> >> +
> >> +	/* Algorithm is simple.
> > 
> > I think it's best to drop this line.
> > It doesn't really clarify anything :)
> > 
> >>  If all the port
> > 
> > all the ports?
> > 
> >> require static
> >> +	 * configuration, we know everything and can simply write
> >> +	 * that down to the ports and clear promisc.
> >> +	 * If only 1 port is automatic and all the others require
> >> +	 * static configuration, we can write all the static data
> >> +	 * to this one automatic port and still make non-promisc.
> > 
> > 
> > make *this port* non promisc.
> > 
> >> +	 */
> > 
> > It's best to move this comment within list_for_each_entry,
> > near the code it documents.
> > 
> 
> Will fix
> 
> >> +	list_for_each_entry(p, &br->port_list, list) {
> >> +		if (br->auto_cnt == 0 ||
> >> +		    (br->auto_cnt == 1 && br_is_auto_port(p)))
> > 
> > 
> > This is correct but I think it would be clearer as follows:
> > 
> > 	list_for_each_entry(p, &br->port_list, list) {
> > 		/* If this is the only automatic port, others are not automatic so
> > 		 * their output configuration is determined by static fdb entries.
> > 		 * Thus, since input on this port will become output on one of the
> > 		 * others, this means our input configuration is static: write it out to
> > 		 * hardware and disable promiscous mode.
> > 		 * Otherwise, make the port promiscous.
> > 		 */
> > 		if (br->auto_cnt <= br_is_auto_port(p))
> > 				br_port_clear_promisc(p);
> > 			else
> > 				br_port_set_promisc(p);
> > 	}
> > 
> > 
> > instead of enumerating cases in comments and in code like you did above.
> > 
> 
> br_is_auto_port give a boolean.
>  I suppose I can make it return an
> integer, but that IMO makes it more confusing.

In C boolean is 0 or 1 so no need to change it to integer.

> I can questions about
> why do we clear it when there are no automatic ports.

Sorry don't understand this sentence.

You comment doesn't explicitly explain the case of auto_cnt == 0
either, but if you want to enumarate both examples maybe combine the
two comments and list all cases, e.g.

	Cases:
		auto_cnt == 0
			clear promisc
		auto_cnt == 1 br_is_auto_port == 1
			clear promisc
		auto_cnt == 1 br_is_auto_port == 0
			set promisc


> I was trying to
> be very explicit in the conditions.
> 
> Thanks
> -vlad


I think a general rule in code is always better than a
bunch of cases listed.
listing cases might be good for documentation / comments
but you need to check them against code anyway, so the
simpler the code the better.

> > 	
> > 
> > 
> >> +			br_port_clear_promisc(p);
> >> +		else
> >> +			br_port_set_promisc(p);
> >> +	}
> >> +}
> >> +
> >>  static void nbp_update_port_count(struct net_bridge *br)
> >>  {
> >>  	struct net_bridge_port *p;
> >> @@ -94,7 +152,23 @@ static void nbp_update_port_count(struct net_bridge *br)
> >>  		if (br_is_auto_port(p))
> >>  			cnt++;
> >>  	}
> >> -	br->auto_cnt = cnt;
> >> +	if (br->auto_cnt != cnt) {
> >> +		br->auto_cnt = cnt;
> >> +		br_manage_promisc(br);
> >> +	}
> >> +}
> >> +
> >> +static void nbp_delete_promisc(struct net_bridge_port *p)
> >> +{
> >> +	/* If port is currently promiscous, unset promiscuity.
> >> +	 * Otherwise, it is a static port so remove all addresses
> >> +	 * from it.
> >> +	 */
> >> +	dev_set_allmulti(p->dev, -1);
> >> +	if (br_is_promisc_port(p))
> >> +		dev_set_promiscuity(p->dev, -1);
> >> +	else
> >> +		br_fdb_unsync_static(p->br, p);
> >>  }
> >>  
> >>  static void release_nbp(struct kobject *kobj)
> >> @@ -145,7 +219,7 @@ static void del_nbp(struct net_bridge_port *p)
> >>  
> >>  	sysfs_remove_link(br->ifobj, p->dev->name);
> >>  
> >> -	dev_set_promiscuity(dev, -1);
> >> +	nbp_delete_promisc(p);
> >>  
> >>  	spin_lock_bh(&br->lock);
> >>  	br_stp_disable_port(p);
> >> @@ -153,11 +227,10 @@ static void del_nbp(struct net_bridge_port *p)
> >>  
> >>  	br_ifinfo_notify(RTM_DELLINK, p);
> >>  
> >> -	nbp_vlan_flush(p);
> >> -	br_fdb_delete_by_port(br, p, 1);
> >> -
> >>  	list_del_rcu(&p->list);
> >>  
> >> +	nbp_vlan_flush(p);
> >> +	br_fdb_delete_by_port(br, p, 1);
> >>  	nbp_update_port_count(br);
> >>  
> >>  	dev->priv_flags &= ~IFF_BRIDGE_PORT;
> >> @@ -367,7 +440,7 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
> >>  
> >>  	call_netdevice_notifiers(NETDEV_JOIN, dev);
> >>  
> >> -	err = dev_set_promiscuity(dev, 1);
> >> +	err = dev_set_allmulti(dev, 1);
> >>  	if (err)
> >>  		goto put_back;
> >>  
> >> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> >> index 2023671..1c794fef 100644
> >> --- a/net/bridge/br_private.h
> >> +++ b/net/bridge/br_private.h
> >> @@ -175,6 +175,7 @@ struct net_bridge_port
> >>  #define BR_LEARNING		0x00000020
> >>  #define BR_FLOOD		0x00000040
> >>  #define BR_AUTO_MASK (BR_FLOOD | BR_LEARNING)
> >> +#define BR_PROMISC		0x00000080
> >>  
> >>  #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
> >>  	struct bridge_mcast_query	ip4_query;
> >> @@ -200,6 +201,7 @@ struct net_bridge_port
> >>  };
> >>  
> >>  #define br_is_auto_port(p) (((p)->flags & BR_AUTO_MASK) == BR_AUTO_MASK)
> >> +#define br_is_promisc_port(p) ((p)->flags & BR_PROMISC)
> >>  
> >>  #define br_port_exists(dev) (dev->priv_flags & IFF_BRIDGE_PORT)
> >>  
> >> -- 
> >> 1.9.0
> > --
> > To unsubscribe from this list: send the line "unsubscribe netdev" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 

Re: [RFC PATCH v2 net-next 0/7] Non-promisc bidge ports support

[Stephen Hemminger]

On Tue, 29 Apr 2014 17:20:21 -0400
Vlad Yasevich <vyasevic@redhat.com> wrote:

> This patch series is a re-implementation of prior attempts to support
> non-promiscuous bridge ports.
> 
> The basic concept is the same as before.  The bridge keeps
> track of the ports that support learning and flooding packets
> to unknown destinations.  We call these ports auto-discovery
> ports since they automatically discover who is behind them through
> learning and flooding.  
> 
> If flooding and learning are disabled via flags, then the port
> requires static configuration to tell it which mac addresses
> are behind it.  This is accomplished through adding of fdbs.
> These fdbs should be static as dynamic fdbs can expire and systems
> will become unreachable due to lack of flooding.
> 
> If the user marks all ports are needing static configuration then
> we can safely make them non-promiscuous, we will know all the
> information about them.
> 
> If the user leaves only 1 port as automatic, then we can mark
> that port as not-promiscuous as well.  One could think of
> this a edge relay similar to what's support by embedded switches
> in SRIOV devices.  Since we have all the information about the
> other ports, we can just program the mac addresses into the
> single automatic port to receive all necessary traffic.
> 
> In other cases, we keep all ports promiscuous as before.
> 
> There are some other cases when promiscuous mode has to be turned
> back on.  One is when the bridge itself if placed in promiscuous
> mode (use sets promisc flag).  The other is if vlan filtering is
> turned off.  Since this is the default configuration, the default
> bridge operation is not changed.

I like this because it does the right thing and is transparent to
the user. You might also not want to do it if the underlying device
does not support multiple MAC addresses 
ie !(dev->priv_flags & IFF_UNICAST_FLT)


You could even go into looking at L2 offload on lower device.

Re: [Bridge] [RFC PATCH v2 net-next 0/7] Non-promisc bidge ports support

[Stephen Hemminger]

On Tue, 29 Apr 2014 17:20:21 -0400
Vlad Yasevich <vyasevic@redhat.com> wrote:

> This patch series is a re-implementation of prior attempts to support
> non-promiscuous bridge ports.
> 
> The basic concept is the same as before.  The bridge keeps
> track of the ports that support learning and flooding packets
> to unknown destinations.  We call these ports auto-discovery
> ports since they automatically discover who is behind them through
> learning and flooding.  
> 
> If flooding and learning are disabled via flags, then the port
> requires static configuration to tell it which mac addresses
> are behind it.  This is accomplished through adding of fdbs.
> These fdbs should be static as dynamic fdbs can expire and systems
> will become unreachable due to lack of flooding.
> 
> If the user marks all ports are needing static configuration then
> we can safely make them non-promiscuous, we will know all the
> information about them.
> 
> If the user leaves only 1 port as automatic, then we can mark
> that port as not-promiscuous as well.  One could think of
> this a edge relay similar to what's support by embedded switches
> in SRIOV devices.  Since we have all the information about the
> other ports, we can just program the mac addresses into the
> single automatic port to receive all necessary traffic.
> 
> In other cases, we keep all ports promiscuous as before.
> 
> There are some other cases when promiscuous mode has to be turned
> back on.  One is when the bridge itself if placed in promiscuous
> mode (use sets promisc flag).  The other is if vlan filtering is
> turned off.  Since this is the default configuration, the default
> bridge operation is not changed.

I like this because it does the right thing and is transparent to
the user. You might also not want to do it if the underlying device
does not support multiple MAC addresses 
ie !(dev->priv_flags & IFF_UNICAST_FLT)


You could even go into looking at L2 offload on lower device.

Re: [RFC PATCH v2 net-next 0/7] Non-promisc bidge ports support

[Michael S. Tsirkin]

On Wed, Apr 30, 2014 at 10:22:41AM -0700, Stephen Hemminger wrote:
> On Tue, 29 Apr 2014 17:20:21 -0400
> Vlad Yasevich <vyasevic@redhat.com> wrote:
> 
> > This patch series is a re-implementation of prior attempts to support
> > non-promiscuous bridge ports.
> > 
> > The basic concept is the same as before.  The bridge keeps
> > track of the ports that support learning and flooding packets
> > to unknown destinations.  We call these ports auto-discovery
> > ports since they automatically discover who is behind them through
> > learning and flooding.  
> > 
> > If flooding and learning are disabled via flags, then the port
> > requires static configuration to tell it which mac addresses
> > are behind it.  This is accomplished through adding of fdbs.
> > These fdbs should be static as dynamic fdbs can expire and systems
> > will become unreachable due to lack of flooding.
> > 
> > If the user marks all ports are needing static configuration then
> > we can safely make them non-promiscuous, we will know all the
> > information about them.
> > 
> > If the user leaves only 1 port as automatic, then we can mark
> > that port as not-promiscuous as well.  One could think of
> > this a edge relay similar to what's support by embedded switches
> > in SRIOV devices.  Since we have all the information about the
> > other ports, we can just program the mac addresses into the
> > single automatic port to receive all necessary traffic.
> > 
> > In other cases, we keep all ports promiscuous as before.
> > 
> > There are some other cases when promiscuous mode has to be turned
> > back on.  One is when the bridge itself if placed in promiscuous
> > mode (use sets promisc flag).  The other is if vlan filtering is
> > turned off.  Since this is the default configuration, the default
> > bridge operation is not changed.
> 
> I like this because it does the right thing and is transparent to
> the user. You might also not want to do it if the underlying device
> does not support multiple MAC addresses 
> ie !(dev->priv_flags & IFF_UNICAST_FLT)

The point being, attempt to add an address to a device without IFF_UNICAST_FLT
will put it right back in promisc mode?

Good point.

> 
> You could even go into looking at L2 offload on lower device.

Re: [Bridge] [RFC PATCH v2 net-next 0/7] Non-promisc bidge ports support

[Michael S. Tsirkin]

On Wed, Apr 30, 2014 at 10:22:41AM -0700, Stephen Hemminger wrote:
> On Tue, 29 Apr 2014 17:20:21 -0400
> Vlad Yasevich <vyasevic@redhat.com> wrote:
> 
> > This patch series is a re-implementation of prior attempts to support
> > non-promiscuous bridge ports.
> > 
> > The basic concept is the same as before.  The bridge keeps
> > track of the ports that support learning and flooding packets
> > to unknown destinations.  We call these ports auto-discovery
> > ports since they automatically discover who is behind them through
> > learning and flooding.  
> > 
> > If flooding and learning are disabled via flags, then the port
> > requires static configuration to tell it which mac addresses
> > are behind it.  This is accomplished through adding of fdbs.
> > These fdbs should be static as dynamic fdbs can expire and systems
> > will become unreachable due to lack of flooding.
> > 
> > If the user marks all ports are needing static configuration then
> > we can safely make them non-promiscuous, we will know all the
> > information about them.
> > 
> > If the user leaves only 1 port as automatic, then we can mark
> > that port as not-promiscuous as well.  One could think of
> > this a edge relay similar to what's support by embedded switches
> > in SRIOV devices.  Since we have all the information about the
> > other ports, we can just program the mac addresses into the
> > single automatic port to receive all necessary traffic.
> > 
> > In other cases, we keep all ports promiscuous as before.
> > 
> > There are some other cases when promiscuous mode has to be turned
> > back on.  One is when the bridge itself if placed in promiscuous
> > mode (use sets promisc flag).  The other is if vlan filtering is
> > turned off.  Since this is the default configuration, the default
> > bridge operation is not changed.
> 
> I like this because it does the right thing and is transparent to
> the user. You might also not want to do it if the underlying device
> does not support multiple MAC addresses 
> ie !(dev->priv_flags & IFF_UNICAST_FLT)

The point being, attempt to add an address to a device without IFF_UNICAST_FLT
will put it right back in promisc mode?

Good point.

> 
> You could even go into looking at L2 offload on lower device.

Re: [RFC PATCH v2 net-next 0/7] Non-promisc bidge ports support

[Vlad Yasevich]

On 04/30/2014 01:22 PM, Stephen Hemminger wrote:
> On Tue, 29 Apr 2014 17:20:21 -0400
> Vlad Yasevich <vyasevic@redhat.com> wrote:
> 
>> This patch series is a re-implementation of prior attempts to support
>> non-promiscuous bridge ports.
>>
>> The basic concept is the same as before.  The bridge keeps
>> track of the ports that support learning and flooding packets
>> to unknown destinations.  We call these ports auto-discovery
>> ports since they automatically discover who is behind them through
>> learning and flooding.  
>>
>> If flooding and learning are disabled via flags, then the port
>> requires static configuration to tell it which mac addresses
>> are behind it.  This is accomplished through adding of fdbs.
>> These fdbs should be static as dynamic fdbs can expire and systems
>> will become unreachable due to lack of flooding.
>>
>> If the user marks all ports are needing static configuration then
>> we can safely make them non-promiscuous, we will know all the
>> information about them.
>>
>> If the user leaves only 1 port as automatic, then we can mark
>> that port as not-promiscuous as well.  One could think of
>> this a edge relay similar to what's support by embedded switches
>> in SRIOV devices.  Since we have all the information about the
>> other ports, we can just program the mac addresses into the
>> single automatic port to receive all necessary traffic.
>>
>> In other cases, we keep all ports promiscuous as before.
>>
>> There are some other cases when promiscuous mode has to be turned
>> back on.  One is when the bridge itself if placed in promiscuous
>> mode (use sets promisc flag).  The other is if vlan filtering is
>> turned off.  Since this is the default configuration, the default
>> bridge operation is not changed.
> 
> I like this because it does the right thing and is transparent to
> the user. You might also not want to do it if the underlying device
> does not support multiple MAC addresses 
> ie !(dev->priv_flags & IFF_UNICAST_FLT)
> 
> 
> You could even go into looking at L2 offload on lower device.
> 


Hmm.  dev_uc_add() would already take care of that, but yes, we could
optimized it a bit more by looking at the flag as well and not bother
with the overhead if the device will just function in promiscuous mode
anyway.

-vlad

Re: [Bridge] [RFC PATCH v2 net-next 0/7] Non-promisc bidge ports support

[Vlad Yasevich]

On 04/30/2014 01:22 PM, Stephen Hemminger wrote:
> On Tue, 29 Apr 2014 17:20:21 -0400
> Vlad Yasevich <vyasevic@redhat.com> wrote:
> 
>> This patch series is a re-implementation of prior attempts to support
>> non-promiscuous bridge ports.
>>
>> The basic concept is the same as before.  The bridge keeps
>> track of the ports that support learning and flooding packets
>> to unknown destinations.  We call these ports auto-discovery
>> ports since they automatically discover who is behind them through
>> learning and flooding.  
>>
>> If flooding and learning are disabled via flags, then the port
>> requires static configuration to tell it which mac addresses
>> are behind it.  This is accomplished through adding of fdbs.
>> These fdbs should be static as dynamic fdbs can expire and systems
>> will become unreachable due to lack of flooding.
>>
>> If the user marks all ports are needing static configuration then
>> we can safely make them non-promiscuous, we will know all the
>> information about them.
>>
>> If the user leaves only 1 port as automatic, then we can mark
>> that port as not-promiscuous as well.  One could think of
>> this a edge relay similar to what's support by embedded switches
>> in SRIOV devices.  Since we have all the information about the
>> other ports, we can just program the mac addresses into the
>> single automatic port to receive all necessary traffic.
>>
>> In other cases, we keep all ports promiscuous as before.
>>
>> There are some other cases when promiscuous mode has to be turned
>> back on.  One is when the bridge itself if placed in promiscuous
>> mode (use sets promisc flag).  The other is if vlan filtering is
>> turned off.  Since this is the default configuration, the default
>> bridge operation is not changed.
> 
> I like this because it does the right thing and is transparent to
> the user. You might also not want to do it if the underlying device
> does not support multiple MAC addresses 
> ie !(dev->priv_flags & IFF_UNICAST_FLT)
> 
> 
> You could even go into looking at L2 offload on lower device.
> 


Hmm.  dev_uc_add() would already take care of that, but yes, we could
optimized it a bit more by looking at the flag as well and not bother
with the overhead if the device will just function in promiscuous mode
anyway.

-vlad