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

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

[Vlad Yasevich]

This series adds functionality to the bridge device to enable
operations without setting all ports to promiscuous mode.

The basic concept is this.  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 as needing static configuration then
we can safely make them non-promiscuous since 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.
More information about this is patch 6.

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 (user 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:
 - Address issues rasied by Stephen Heminger
 - Address initializer comments raised by Sergey Shtylyov
 - Rebased recent net-next.

Changes since rfc v2:
 - Better description of in the commit logs
 - Leave port in promiscuous mode if IFF_UNICAST_FLT is disabled on the
   device.
 - Fix issue with flag masking
 - Rework patch ordering a bit.

Changes since rfc 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 (8):
  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: Introduce BR_PROMISC flag
  bridge: Add addresses from static fdbs to non-promisc ports
  bridge: Automatically manage port promiscuous mode.
  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       | 123 +++++++++++++++++++++++++++++++++++++++++--
 net/bridge/br_netlink.c  |   3 ++
 net/bridge/br_private.h  |  19 +++++++
 net/bridge/br_sysfs_if.c |  30 +++++++----
 net/bridge/br_vlan.c     |   1 +
 7 files changed, 295 insertions(+), 20 deletions(-)

-- 
1.9.0

[PATCH v2 net-next 1/8] 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

[PATCH v2 net-next 2/8] 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 disabled 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       | 24 ++++++++++++++++++++++++
 net/bridge/br_netlink.c  |  3 +++
 net/bridge/br_private.h  |  5 +++++
 net/bridge/br_sysfs_if.c |  5 ++++-
 4 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 5262b86..f7ef5f2 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_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,11 @@ 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 mask)
+{
+	struct net_bridge *br = p->br;
+
+	if (mask & BR_AUTO_MASK)
+		nbp_update_port_count(br);
+}
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index e8844d9..26edb51 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..5ce3191 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_auto_port(p) ((p)->flags & 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,7 @@ 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 mask);
 
 /* 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..c7fbe38 100644
--- a/net/bridge/br_sysfs_if.c
+++ b/net/bridge/br_sysfs_if.c
@@ -49,7 +49,9 @@ 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;
+
+	flags = p->flags;
 
 	if (v)
 		flags |= mask;
@@ -58,6 +60,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, mask);
 		br_ifinfo_notify(RTM_NEWLINK, p);
 	}
 	return 0;
-- 
1.9.0

[PATCH v2 net-next 3/8] 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-promiscuous 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..fe124e5 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;
+	int i;
+	int err;
+
+	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 5ce3191..c0a804b 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

[PATCH v2 net-next 4/8] bridge: Introduce BR_PROMISC flag

[Vlad Yasevich]

Introduce a BR_PROMISC per-port flag that will help us track if the
current port is supposed to be in promiscuous mode or not.  For now,
always start in promiscuous mode.

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

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index f7ef5f2..3fefff9 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -238,7 +238,7 @@ static struct net_bridge_port *new_nbp(struct net_bridge *br,
 	p->path_cost = port_cost(dev);
 	p->priority = 0x8000 >> BR_PORT_BITS;
 	p->port_no = index;
-	p->flags = BR_LEARNING | BR_FLOOD;
+	p->flags = BR_LEARNING | BR_FLOOD | BR_PROMISC;
 	br_init_port(p);
 	p->state = BR_STATE_DISABLED;
 	br_stp_port_timer_init(p);
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index c0a804b..00922a4 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_auto_port(p) ((p)->flags & BR_AUTO_MASK)
+#define br_promisc_port(p) ((p)->flags & BR_PROMISC)
 
 #define br_port_exists(dev) (dev->priv_flags & IFF_BRIDGE_PORT)
 
-- 
1.9.0

[PATCH v2 net-next 5/8] 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.
By default ports start in promiscuous mode, so this feature
is disabled.

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 fe124e5..834ca8c 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;
+
+	ASSERT_RTNL();
+
+	list_for_each_entry(p, &br->port_list, list) {
+		if (!br_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_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_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

[PATCH v2 net-next 6/8] bridge: Automatically manage port promiscuous mode.

[Vlad Yasevich]

There exist configurations where the administrator or another management
entity has the foreknowledge of all the mac addresses of end systems
that are being bridged together.

In these environments, the administrator can statically configure known
addresses in the bridge FDB and disable flooding and learning on ports.
This makes it possible to turn off promiscuous mode on the interfaces
connected to the bridge.

Here is why disabling flooding and learning allows us to control
promiscuity:
 Consider port X.  All traffic coming into this port from outside the
bridge (ingress) will be either forwarded through other ports of the
bridge (egress) or dropped.  Forwarding (egress) is defined by FDB
entries and by flooding in the event that no FDB entry exists.
In the event that flooding is disabled, only FDB entries define
the egress.  Once learning is disabled, only static FDB entries
provided by a management entity define the egress.  If we provide
information from these static FDBs to the ingress port X, then we'll
be able to accept all traffic that can be successfully forwarded and
drop all the other traffic sooner without spending CPU cycles to
process it.
 Another way to define the above is as following equations:
    ingress = egress + drop
 expanding egress
    ingress = static FDB + learned FDB + flooding + drop
 disabling flooding and learning we a left with
    ingress = static FDB + drop

By adding addresses from the static FDB entries to the MAC address
filter of an ingress port X, we fully define what the bridge can
process without dropping and can thus turn off promiscuous mode,
thus dropping packets sooner.

There have been suggestions that we may want to allow learning
and update the filters with learned addresses as well.  This
would require mac-level authentication similar to 802.1x to
prevent attacks against the hw filters as they are limited
resource.

Suggested-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
 net/bridge/br_if.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 86 insertions(+), 7 deletions(-)

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 3fefff9..e3bc5a6 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -85,6 +85,70 @@ 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_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;
+
+	/* Check if the port is already non-promisc or if it doesn't
+	 * support UNICAST filtering.  Without unicast filtering support
+	 * we'll end up re-enabling promisc mode anyway, so just check for
+	 * it here.
+	 */
+	if (!br_promisc_port(p) || !(p->dev->priv_flags & IFF_UNICAST_FLT))
+		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;
+
+	list_for_each_entry(p, &br->port_list, list) {
+		/* If the number of auto-ports is <= 1, then all other
+		 * ports will have their output configuration statically
+		 * specified through fdbs.  Since ingress on the auto-port
+		 * becomes forwarding/egress to other ports and egress
+		 * configuration is statically known, we can say that ingress
+		 * configuration of the auto-port is also statically known.
+		 * This lets us disable promiscuous mode and write this config
+		 * to hw.
+		 */
+		if (br->auto_cnt <= br_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 +158,23 @@ static void nbp_update_port_count(struct net_bridge *br)
 		if (br_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_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 +225,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 +233,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;
@@ -238,7 +317,7 @@ static struct net_bridge_port *new_nbp(struct net_bridge *br,
 	p->path_cost = port_cost(dev);
 	p->priority = 0x8000 >> BR_PORT_BITS;
 	p->port_no = index;
-	p->flags = BR_LEARNING | BR_FLOOD | BR_PROMISC;
+	p->flags = BR_LEARNING | BR_FLOOD;
 	br_init_port(p);
 	p->state = BR_STATE_DISABLED;
 	br_stp_port_timer_init(p);
@@ -367,7 +446,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;
 
-- 
1.9.0

[PATCH v2 net-next 7/8] 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>
---
 net/bridge/br_device.c  |  7 +++++++
 net/bridge/br_if.c      | 34 +++++++++++++++++++++-------------
 net/bridge/br_private.h |  1 +
 3 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 9212015..d77e2f0 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 e3bc5a6..1a3638e 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -128,24 +128,32 @@ 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;
 
 	list_for_each_entry(p, &br->port_list, list) {
-		/* If the number of auto-ports is <= 1, then all other
-		 * ports will have their output configuration statically
-		 * specified through fdbs.  Since ingress on the auto-port
-		 * becomes forwarding/egress to other ports and egress
-		 * configuration is statically known, we can say that ingress
-		 * configuration of the auto-port is also statically known.
-		 * This lets us disable promiscuous mode and write this config
-		 * to hw.
-		 */
-		if (br->auto_cnt <= br_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 the number of auto-ports is <= 1, then all other
+			 * ports will have their output configuration
+			 * statically specified through fdbs.  Since ingress
+			 * on the auto-port becomes forwarding/egress to other
+			 * ports and egress configuration is statically known,
+			 * we can say that ingress configuration of the
+			 * auto-port is also statically known.
+			 * This lets us disable promiscuous mode and write
+			 * this config to hw.
+			 */
+			if (br->auto_cnt <= br_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 00922a4..06976af 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -424,6 +424,7 @@ 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 mask);
+void br_manage_promisc(struct net_bridge *br);
 
 /* br_input.c */
 int br_handle_frame_finish(struct sk_buff *skb);
-- 
1.9.0

[PATCH v2 net-next 8/8] 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 promiscuous 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 1a3638e..33a83ea 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -131,12 +131,16 @@ static void br_port_clear_promisc(struct net_bridge_port *p)
 void br_manage_promisc(struct net_bridge *br)
 {
 	struct net_bridge_port *p;
+	bool set_all = false;
+
+	/* If vlan filtering is disabled or bridge interface is placed
+	 * into promiscuous mode, place all ports in promiscuous mode.
+	 */
+	if ((br->dev->flags & IFF_PROMISC) || !br_vlan_enabled(br))
+		set_all = true;
 
 	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 the number of auto-ports is <= 1, then all other
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 06976af..2b2286d 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -642,6 +642,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,
@@ -722,6 +726,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: [PATCH v2 net-next 1/8] bridge: Turn flag change macro into a function.

[Michael S. Tsirkin]

On Thu, May 15, 2014 at 12:56:49PM -0400, Vlad Yasevich wrote:
> Turn the flag change macro into a function to allow
> easier updates and to reduce space.
> 
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

whitespace nit below

> ---
>  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)

This second line should align "unsigned" with "struct" on the line above
it. As it is it's aligned with (  which violates the docinf style
requirement
	Descendants are always substantially shorter than the parent and
	are placed substantially to the right.
it's not substantially to the right if it's not to the right of (.

> +{
> +	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

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

[Michael S. Tsirkin]

On Thu, May 15, 2014 at 12:56:53PM -0400, Vlad Yasevich wrote:
> 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.
> By default ports start in promiscuous mode, so this feature
> is disabled.
> 
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

whitespace nit below

> ---
>  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 fe124e5..834ca8c 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;
> +
> +	ASSERT_RTNL();
> +
> +	list_for_each_entry(p, &br->port_list, list) {
> +		if (!br_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_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_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;
>  	}
> +

Is this eally necessary?

>  	fdb->added_by_user = 1;
>  
>  	fdb->used = jiffies;
> -- 
> 1.9.0

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

[Michael S. Tsirkin]

On Thu, May 15, 2014 at 12:56:50PM -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 disabled 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>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  net/bridge/br_if.c       | 24 ++++++++++++++++++++++++
>  net/bridge/br_netlink.c  |  3 +++
>  net/bridge/br_private.h  |  5 +++++
>  net/bridge/br_sysfs_if.c |  5 ++++-
>  4 files changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> index 5262b86..f7ef5f2 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_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,11 @@ 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 mask)
> +{
> +	struct net_bridge *br = p->br;
> +
> +	if (mask & BR_AUTO_MASK)
> +		nbp_update_port_count(br);
> +}
> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> index e8844d9..26edb51 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..5ce3191 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_auto_port(p) ((p)->flags & 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,7 @@ 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 mask);
>  
>  /* 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..c7fbe38 100644
> --- a/net/bridge/br_sysfs_if.c
> +++ b/net/bridge/br_sysfs_if.c
> @@ -49,7 +49,9 @@ 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;
> +
> +	flags = p->flags;
>  
>  	if (v)
>  		flags |= mask;
> @@ -58,6 +60,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, mask);
>  		br_ifinfo_notify(RTM_NEWLINK, p);
>  	}
>  	return 0;
> -- 
> 1.9.0

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

[Michael S. Tsirkin]

On Thu, May 15, 2014 at 12:56:51PM -0400, Vlad Yasevich wrote:
> 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-promiscuous mode.
> 
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>

Acked-by: Michael S. Tsirkin <mst@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..fe124e5 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;
> +	int i;
> +	int err;
> +
> +	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 5ce3191..c0a804b 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

Re: [PATCH v2 net-next 4/8] bridge: Introduce BR_PROMISC flag

[Michael S. Tsirkin]

On Thu, May 15, 2014 at 12:56:52PM -0400, Vlad Yasevich wrote:
> Introduce a BR_PROMISC per-port flag that will help us track if the
> current port is supposed to be in promiscuous mode or not.  For now,
> always start in promiscuous mode.
> 
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  net/bridge/br_if.c      | 2 +-
>  net/bridge/br_private.h | 2 ++
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> index f7ef5f2..3fefff9 100644
> --- a/net/bridge/br_if.c
> +++ b/net/bridge/br_if.c
> @@ -238,7 +238,7 @@ static struct net_bridge_port *new_nbp(struct net_bridge *br,
>  	p->path_cost = port_cost(dev);
>  	p->priority = 0x8000 >> BR_PORT_BITS;
>  	p->port_no = index;
> -	p->flags = BR_LEARNING | BR_FLOOD;
> +	p->flags = BR_LEARNING | BR_FLOOD | BR_PROMISC;
>  	br_init_port(p);
>  	p->state = BR_STATE_DISABLED;
>  	br_stp_port_timer_init(p);
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index c0a804b..00922a4 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_auto_port(p) ((p)->flags & BR_AUTO_MASK)
> +#define br_promisc_port(p) ((p)->flags & BR_PROMISC)
>  
>  #define br_port_exists(dev) (dev->priv_flags & IFF_BRIDGE_PORT)
>  
> -- 
> 1.9.0

Re: [PATCH v2 net-next 6/8] bridge: Automatically manage port promiscuous mode.

[Michael S. Tsirkin]

On Thu, May 15, 2014 at 12:56:54PM -0400, Vlad Yasevich wrote:
> There exist configurations where the administrator or another management
> entity has the foreknowledge of all the mac addresses of end systems
> that are being bridged together.
> 
> In these environments, the administrator can statically configure known
> addresses in the bridge FDB and disable flooding and learning on ports.
> This makes it possible to turn off promiscuous mode on the interfaces
> connected to the bridge.
> 
> Here is why disabling flooding and learning allows us to control
> promiscuity:
>  Consider port X.  All traffic coming into this port from outside the
> bridge (ingress) will be either forwarded through other ports of the
> bridge (egress) or dropped.  Forwarding (egress) is defined by FDB
> entries and by flooding in the event that no FDB entry exists.
> In the event that flooding is disabled, only FDB entries define
> the egress.  Once learning is disabled, only static FDB entries
> provided by a management entity define the egress.  If we provide
> information from these static FDBs to the ingress port X, then we'll
> be able to accept all traffic that can be successfully forwarded and
> drop all the other traffic sooner without spending CPU cycles to
> process it.
>  Another way to define the above is as following equations:
>     ingress = egress + drop
>  expanding egress
>     ingress = static FDB + learned FDB + flooding + drop
>  disabling flooding and learning we a left with
>     ingress = static FDB + drop
> 
> By adding addresses from the static FDB entries to the MAC address
> filter of an ingress port X, we fully define what the bridge can
> process without dropping and can thus turn off promiscuous mode,
> thus dropping packets sooner.
> 
> There have been suggestions that we may want to allow learning
> and update the filters with learned addresses as well.  This
> would require mac-level authentication similar to 802.1x to
> prevent attacks against the hw filters as they are limited
> resource.
> 
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  net/bridge/br_if.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 86 insertions(+), 7 deletions(-)
> 
> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> index 3fefff9..e3bc5a6 100644
> --- a/net/bridge/br_if.c
> +++ b/net/bridge/br_if.c
> @@ -85,6 +85,70 @@ 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_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;
> +
> +	/* Check if the port is already non-promisc or if it doesn't
> +	 * support UNICAST filtering.  Without unicast filtering support
> +	 * we'll end up re-enabling promisc mode anyway, so just check for
> +	 * it here.
> +	 */
> +	if (!br_promisc_port(p) || !(p->dev->priv_flags & IFF_UNICAST_FLT))
> +		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

s/mange/manage/

Unless you meant it in french :)

> + * 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;
> +
> +	list_for_each_entry(p, &br->port_list, list) {
> +		/* If the number of auto-ports is <= 1, then all other
> +		 * ports will have their output configuration statically
> +		 * specified through fdbs.  Since ingress on the auto-port
> +		 * becomes forwarding/egress to other ports and egress
> +		 * configuration is statically known, we can say that ingress
> +		 * configuration of the auto-port is also statically known.
> +		 * This lets us disable promiscuous mode and write this config
> +		 * to hw.
> +		 */
> +		if (br->auto_cnt <= br_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 +158,23 @@ static void nbp_update_port_count(struct net_bridge *br)
>  		if (br_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_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 +225,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 +233,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;
> @@ -238,7 +317,7 @@ static struct net_bridge_port *new_nbp(struct net_bridge *br,
>  	p->path_cost = port_cost(dev);
>  	p->priority = 0x8000 >> BR_PORT_BITS;
>  	p->port_no = index;
> -	p->flags = BR_LEARNING | BR_FLOOD | BR_PROMISC;
> +	p->flags = BR_LEARNING | BR_FLOOD;
>  	br_init_port(p);
>  	p->state = BR_STATE_DISABLED;
>  	br_stp_port_timer_init(p);
> @@ -367,7 +446,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;
>  
> -- 
> 1.9.0

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

[Michael S. Tsirkin]

On Thu, May 15, 2014 at 12:56:55PM -0400, Vlad Yasevich wrote:
> 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>


Acked-by: Michael S. Tsirkin <mst@redhat.com>

Though I would smash this one into the previous patch:
it adds about 6 lines so it's not like it makes
the patch much bigger.
OTOH after smash, we are sure bisect won't produce a broken
kernel.

> ---
>  net/bridge/br_device.c  |  7 +++++++
>  net/bridge/br_if.c      | 34 +++++++++++++++++++++-------------
>  net/bridge/br_private.h |  1 +
>  3 files changed, 29 insertions(+), 13 deletions(-)
> 
> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
> index 9212015..d77e2f0 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 e3bc5a6..1a3638e 100644
> --- a/net/bridge/br_if.c
> +++ b/net/bridge/br_if.c
> @@ -128,24 +128,32 @@ 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;
>  
>  	list_for_each_entry(p, &br->port_list, list) {
> -		/* If the number of auto-ports is <= 1, then all other
> -		 * ports will have their output configuration statically
> -		 * specified through fdbs.  Since ingress on the auto-port
> -		 * becomes forwarding/egress to other ports and egress
> -		 * configuration is statically known, we can say that ingress
> -		 * configuration of the auto-port is also statically known.
> -		 * This lets us disable promiscuous mode and write this config
> -		 * to hw.
> -		 */
> -		if (br->auto_cnt <= br_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 the number of auto-ports is <= 1, then all other
> +			 * ports will have their output configuration
> +			 * statically specified through fdbs.  Since ingress
> +			 * on the auto-port becomes forwarding/egress to other
> +			 * ports and egress configuration is statically known,
> +			 * we can say that ingress configuration of the
> +			 * auto-port is also statically known.
> +			 * This lets us disable promiscuous mode and write
> +			 * this config to hw.
> +			 */
> +			if (br->auto_cnt <= br_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 00922a4..06976af 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -424,6 +424,7 @@ 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 mask);
> +void br_manage_promisc(struct net_bridge *br);
>  
>  /* br_input.c */
>  int br_handle_frame_finish(struct sk_buff *skb);
> -- 
> 1.9.0

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

[Michael S. Tsirkin]

On Thu, May 15, 2014 at 12:56:56PM -0400, Vlad Yasevich wrote:
> If the user doesn't enable vlan filtering, we have to place all
> bridge ports in promsic mode

s/promsic/promisc/

> so that we retain the capability of
> of receiving tagged frames.

s/of of/of/

> When vlan filtering is enabled, the each port

s/the each port/each port/

> will be provided with
> necessary vlan configuration and would be able to receive tagged
> traffic without promiscuous mode set, thus allowing us to automatically
> turn promiscuity on or off depending on the configuration.
> 
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>


Acked-by: Michael S. Tsirkin <mst@redhat.com>

again I would smash this up, though it is less clear-cut here.


> ---
>  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 1a3638e..33a83ea 100644
> --- a/net/bridge/br_if.c
> +++ b/net/bridge/br_if.c
> @@ -131,12 +131,16 @@ static void br_port_clear_promisc(struct net_bridge_port *p)
>  void br_manage_promisc(struct net_bridge *br)
>  {
>  	struct net_bridge_port *p;
> +	bool set_all = false;
> +
> +	/* If vlan filtering is disabled or bridge interface is placed
> +	 * into promiscuous mode, place all ports in promiscuous mode.
> +	 */
> +	if ((br->dev->flags & IFF_PROMISC) || !br_vlan_enabled(br))
> +		set_all = true;
>  
>  	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 the number of auto-ports is <= 1, then all other
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 06976af..2b2286d 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -642,6 +642,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,
> @@ -722,6 +726,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