[PATCH net 0/7] bridge: Fix corner case problems around local fdb entries

[PATCH net 0/7] bridge: Fix corner case problems around local fdb entries

[Toshiaki Makita]

There are so many corner cases that are not handled properly around local
fdb entries.
- We might fail to delete the old entry and might delete an arbitrary local
  entry when changing mac address of a bridge port.
- We always fail to delete the old entry when changing mac address of the
  bridge device.
- We might incorrectly delete a necessary entry when detaching a bridge port.
- We might incorrectly delete a necessary entry when deleting a vlan.
and so on.

This is a patch series to fix these issues.

Toshiaki Makita (7):
  bridge: Fix the way finding the old local fdb entry in
    br_fdb_changeaddr
  bridge: Fix the way finding the old local fdb entry in
    br_fdb_change_mac_address
  bridge: Change local fdb entries whenever mac address of bridge device
    changes
  bridge: Fix the way checking if a local fdb entry can be deleted
  bridge: Properly check if local fdb entry can be deleted in
    br_fdb_change_mac_address
  bridge: Properly check if local fdb entry can be deleted in
    br_fdb_delete_by_port
  bridge: Properly check if local fdb entry can be deleted when deleting
    vlan

 net/bridge/br_device.c  |   3 +-
 net/bridge/br_fdb.c     | 144 ++++++++++++++++++++++--------------------------
 net/bridge/br_if.c      |   1 +
 net/bridge/br_notify.c  |   9 ++-
 net/bridge/br_private.h |  27 ++++++++-
 net/bridge/br_stp_if.c  |   2 +
 net/bridge/br_vlan.c    |  23 +++++++-
 7 files changed, 120 insertions(+), 89 deletions(-)

-- 
1.8.1.2

[PATCH net 1/7] bridge: Fix the way finding the old local fdb entry in br_fdb_changeaddr

[Toshiaki Makita]

br_fdb_changeaddr() assumes that there is at most one local entry per port
per vlan. It used to be true, but since commit 36fd2b63e3b4 ("bridge: allow
creating/deleting fdb entries via netlink"), it has been not.
Therefore, the function might fail to search a correct previous address
to be deleted and delete an arbitrary local entry if user has added local
entries manually.

Example of problematic case:
  ip link set eth0 address ee:ff:12:34:56:78
  brctl addif br0 eth0
  bridge fdb add 12:34:56:78:90:ab dev eth0 master
  ip link set eth0 address aa:bb:cc:dd:ee:ff
Then, the address 12:34:56:78:90:ab might be deleted instead of
ee:ff:12:34:56:78, the original mac address of eth0.

To fix this, remember the mac addresses of bridge ports and use them to
find old fdb entries when changing their mac addresses.
It is no longer necessary to seek all fdb hash chains, as we can call
fdb_find() with the address to get the corresponding entry.

This approach also has secondary effects that we will come to be able to
share the code between br_fdb_changeaddr() and br_fdb_change_mac_address(),
and prevent unnecessary bridge id recalculation in br_device_event().

Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 net/bridge/br_fdb.c     | 89 ++++++++++++++++++++++++++++---------------------
 net/bridge/br_if.c      |  1 +
 net/bridge/br_notify.c  |  9 +++--
 net/bridge/br_private.h |  1 +
 4 files changed, 59 insertions(+), 41 deletions(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 33e8f23..d29e184 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -27,6 +27,9 @@
 #include "br_private.h"
 
 static struct kmem_cache *br_fdb_cache __read_mostly;
+static struct net_bridge_fdb_entry *fdb_find(struct hlist_head *head,
+					     const unsigned char *addr,
+					     __u16 vid);
 static int fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
 		      const unsigned char *addr, u16 vid);
 static void fdb_notify(struct net_bridge *br,
@@ -89,54 +92,64 @@ static void fdb_delete(struct net_bridge *br, struct net_bridge_fdb_entry *f)
 	call_rcu(&f->rcu, fdb_rcu_free);
 }
 
+/* Delete a local entry if no other port had the same address. */
+static void fdb_delete_local(struct net_bridge *br,
+			     const struct net_bridge_port *p,
+			     const unsigned char *addr, u16 vid)
+{
+	struct hlist_head *head = &br->hash[br_mac_hash(addr, vid)];
+	struct net_bridge_fdb_entry *f;
+	struct net_bridge_port *op;
+
+	f = fdb_find(head, addr, vid);
+	if (!f || !f->is_local || f->dst != p)
+		return;
+
+	/* Maybe another port has same hw addr? */
+	list_for_each_entry(op, &br->port_list, list) {
+		if (op != p && ether_addr_equal(op->dev->dev_addr, addr) &&
+		    nbp_vlan_find(op, vid)) {
+			f->dst = op;
+			return;
+		}
+	}
+
+	fdb_delete(br, f);
+}
+
 void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
 {
 	struct net_bridge *br = p->br;
-	bool no_vlan = (nbp_get_vlan_info(p) == NULL) ? true : false;
-	int i;
+	struct net_port_vlans *pv;
+	const unsigned char *oldaddr = p->prev_addr.addr;
+	u16 vid;
+
+	ASSERT_RTNL();
 
 	spin_lock_bh(&br->hash_lock);
 
-	/* Search all chains since old address/hash is unknown */
-	for (i = 0; i < BR_HASH_SIZE; i++) {
-		struct hlist_node *h;
-		hlist_for_each(h, &br->hash[i]) {
-			struct net_bridge_fdb_entry *f;
+	/* Delete old one, may fail if corresponding entry was associated
+	 * with another port or user deleted it.
+	 */
+	fdb_delete_local(br, p, oldaddr, 0);
 
-			f = hlist_entry(h, struct net_bridge_fdb_entry, hlist);
-			if (f->dst == p && f->is_local) {
-				/* maybe another port has same hw addr? */
-				struct net_bridge_port *op;
-				u16 vid = f->vlan_id;
-				list_for_each_entry(op, &br->port_list, list) {
-					if (op != p &&
-					    ether_addr_equal(op->dev->dev_addr,
-							     f->addr.addr) &&
-					    nbp_vlan_find(op, vid)) {
-						f->dst = op;
-						goto insert;
-					}
-				}
+	/* Insert new address, may fail if invalid address or dup. */
+	fdb_insert(br, p, newaddr, 0);
 
-				/* delete old one */
-				fdb_delete(br, f);
-insert:
-				/* insert new address,  may fail if invalid
-				 * address or dup.
-				 */
-				fdb_insert(br, p, newaddr, vid);
-
-				/* if this port has no vlan information
-				 * configured, we can safely be done at
-				 * this point.
-				 */
-				if (no_vlan)
-					goto done;
-			}
-		}
+	/* Now remove and add entries for every VLAN configured on the
+	 * port.  This function runs under RTNL so the bitmap will not
+	 * change from under us.
+	 */
+	pv = nbp_get_vlan_info(p);
+	if (!pv)
+		goto out;
+
+	for_each_set_bit(vid, pv->vlan_bitmap, VLAN_N_VID) {
+		fdb_delete_local(br, p, oldaddr, vid);
+		fdb_insert(br, p, newaddr, vid);
 	}
 
-done:
+out:
 	spin_unlock_bh(&br->hash_lock);
 }
 
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 4bf02ad..346b2b2 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -406,6 +406,7 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
 
 	if (br_fdb_insert(br, p, dev->dev_addr, 0))
 		netdev_err(dev, "failed insert local address bridge forwarding table\n");
+	memcpy(p->prev_addr.addr, dev->dev_addr, ETH_ALEN);
 
 	kobject_uevent(&p->kobj, KOBJ_ADD);
 
diff --git a/net/bridge/br_notify.c b/net/bridge/br_notify.c
index 2998dd1..3d3f7a1 100644
--- a/net/bridge/br_notify.c
+++ b/net/bridge/br_notify.c
@@ -34,7 +34,7 @@ static int br_device_event(struct notifier_block *unused, unsigned long event, v
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
 	struct net_bridge_port *p;
 	struct net_bridge *br;
-	bool changed_addr;
+	bool changed_addr = false;
 	int err;
 
 	/* register of bridge completed, add sysfs entries */
@@ -57,8 +57,11 @@ static int br_device_event(struct notifier_block *unused, unsigned long event, v
 
 	case NETDEV_CHANGEADDR:
 		spin_lock_bh(&br->lock);
-		br_fdb_changeaddr(p, dev->dev_addr);
-		changed_addr = br_stp_recalculate_bridge_id(br);
+		if (!ether_addr_equal(dev->dev_addr, p->prev_addr.addr)) {
+			br_fdb_changeaddr(p, dev->dev_addr);
+			memcpy(p->prev_addr.addr, dev->dev_addr, ETH_ALEN);
+			changed_addr = br_stp_recalculate_bridge_id(br);
+		}
 		spin_unlock_bh(&br->lock);
 
 		if (changed_addr)
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 229d820..0902658 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -195,6 +195,7 @@ struct net_bridge_port
 #ifdef CONFIG_BRIDGE_VLAN_FILTERING
 	struct net_port_vlans __rcu	*vlan_info;
 #endif
+	mac_addr			prev_addr;
 };
 
 #define br_port_exists(dev) (dev->priv_flags & IFF_BRIDGE_PORT)
-- 
1.8.1.2

[PATCH net 2/7] bridge: Fix the way finding the old local fdb entry in br_fdb_change_mac_address

[Toshiaki Makita]

We have been always failed to delete the old entry at
br_fdb_change_mac_address() because br_set_mac_address() updates
dev->dev_addr before calling br_fdb_change_mac_address() and
br_fdb_change_mac_address() uses dev->dev_addr to find the old entry.

That update of dev_addr is completely unnecessary because the same work
is done in br_stp_change_bridge_id() which is called right away after
calling br_fdb_change_mac_address().

Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 net/bridge/br_device.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index f00cfd2..d967773 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -187,8 +187,8 @@ static int br_set_mac_address(struct net_device *dev, void *p)
 
 	spin_lock_bh(&br->lock);
 	if (!ether_addr_equal(dev->dev_addr, addr->sa_data)) {
-		memcpy(dev->dev_addr, addr->sa_data, ETH_ALEN);
 		br_fdb_change_mac_address(br, addr->sa_data);
+		/* Mac address will be changed in br_stp_change_bridge_id(). */
 		br_stp_change_bridge_id(br, addr->sa_data);
 	}
 	spin_unlock_bh(&br->lock);
-- 
1.8.1.2

[PATCH net 3/7] bridge: Change local fdb entries whenever mac address of bridge device changes

[Toshiaki Makita]

Vlan code may need fdb change when changing mac address of bridge device
even if it is caused by the mac address changing of a bridge port.

Example configuration:
  ip link set eth0 address 12:34:56:78:90:ab
  ip link set eth1 address aa:bb:cc:dd:ee:ff
  brctl addif br0 eth0
  brctl addif br0 eth1 # br0 will have mac address 12:34:56:78:90:ab
  bridge vlan add dev br0 vid 10 self
  bridge vlan add dev eth0 vid 10
We will have fdb entry such that f->dst == NULL, f->vlan_id == 10 and
f->addr == 12:34:56:78:90:ab at this time.
Next, change the mac address of eth0 to greater value.
  ip link set eth0 address ee:ff:12:34:56:78
Then, mac address of br0 will be recalculated and set to aa:bb:cc:dd:ee:ff.
However, an entry aa:bb:cc:dd:ee:ff will not be created and we will be not
able to communicate using br0 on vlan 10.

Address this issue by deleting and adding local entries whenever
changing the mac address of the bridge device.

If there already exists an entry that has the same address, for example,
in case that br_fdb_changeaddr() has already inserted it,
br_fdb_change_mac_address() will simply fail to insert it and no
duplicated entry will be made, as it was.

Note that br_add_if() calls br_stp_recalculate_bridge_id() before
br_fdb_insert() and an entry whose dst == NULL will be created in this case.
Though such an entry wouldn't be created in this function without this
patch, I think this behavior will not be harmful because we already have
such entries with vlan_filtering enabled or setting mac address of the
bridge device manually.

Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
It seems to be not difficult to change the code not to create an entry whose
dst == NULL at br_add_if().  It can be acheived by calling br_fdb_insert()
before br_stp_recalculate_bridge_id() (and actually br_device_event() changes
a fdb entry in such a way).
If it is desirable, I will do that change.

 net/bridge/br_device.c | 1 -
 net/bridge/br_stp_if.c | 2 ++
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index d967773..1cbdbf1 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -187,7 +187,6 @@ static int br_set_mac_address(struct net_device *dev, void *p)
 
 	spin_lock_bh(&br->lock);
 	if (!ether_addr_equal(dev->dev_addr, addr->sa_data)) {
-		br_fdb_change_mac_address(br, addr->sa_data);
 		/* Mac address will be changed in br_stp_change_bridge_id(). */
 		br_stp_change_bridge_id(br, addr->sa_data);
 	}
diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
index 656a6f3..189ba1e 100644
--- a/net/bridge/br_stp_if.c
+++ b/net/bridge/br_stp_if.c
@@ -194,6 +194,8 @@ void br_stp_change_bridge_id(struct net_bridge *br, const unsigned char *addr)
 
 	wasroot = br_is_root_bridge(br);
 
+	br_fdb_change_mac_address(br, addr);
+
 	memcpy(oldaddr, br->bridge_id.addr, ETH_ALEN);
 	memcpy(br->bridge_id.addr, addr, ETH_ALEN);
 	memcpy(br->dev->dev_addr, addr, ETH_ALEN);
-- 
1.8.1.2

[PATCH net 4/7] bridge: Fix the way checking if a local fdb entry can be deleted

[Toshiaki Makita]

We should take into account the followings when deleting a local fdb entry.

- nbp_vlan_find() can be used only when vid != 0 to check if an entry is
  deletable, because a fdb entry with vid 0 can exist at any time but
  nbp_vlan_find() always return false with vid 0.

  Example of problematic case:
    ip link set eth0 address 12:34:56:78:90:ab
    ip link set eth1 address 12:34:56:78:90:ab
    brctl addif br0 eth0
    brctl addif br0 eth1
    ip link set eth0 address aa:bb:cc:dd:ee:ff
  Then, the fdb entry 12:34:56:78:90:ab will be deleted even though the
  bridge port eth1 still has that address.

- The port to which the bridge device is attached might needs a local entry
  if its mac address is set manually.

  Example of problematic case:
    ip link set eth0 address 12:34:56:78:90:ab
    brctl addif br0 eth0
    ip link set br0 address 12:34:56:78:90:ab
    ip link set eth0 address aa:bb:cc:dd:ee:ff
  Then, the fdb still must have the entry 12:34:56:78:90:ab, but it will be
  deleted.

We can use br->dev->addr_assign_type to check if the address is manually
set or not, but I propose another approach.

Since we delete and insert local entries whenever changing mac address
of the bridge device, we can change dst of the entry to NULL regardless of
addr_assign_type when deleting an entry associated with a certain port,
and if it is found to be unnecessary later, then delete it.
That is, if changing mac address of a port, the entry might be changed
to its dst being NULL first, but is eventually deleted when recalculating
and changing bridge id.

This approach is useful when we want to share the code with deleting
vlan in which the bridge device might want such an entry regardless of
addr_assign_type, and makes things easy because we don't have to consider
if mac address of the bridge device will be changed or not at
fdb_delete_local().

Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 net/bridge/br_fdb.c     |  9 ++++++++-
 net/bridge/br_private.h |  6 ++++++
 net/bridge/br_vlan.c    | 19 +++++++++++++++++++
 3 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index d29e184..2c79b1b 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -108,12 +108,19 @@ static void fdb_delete_local(struct net_bridge *br,
 	/* Maybe another port has same hw addr? */
 	list_for_each_entry(op, &br->port_list, list) {
 		if (op != p && ether_addr_equal(op->dev->dev_addr, addr) &&
-		    nbp_vlan_find(op, vid)) {
+		    (!vid || nbp_vlan_find(op, vid))) {
 			f->dst = op;
 			return;
 		}
 	}
 
+	/* Maybe bridge device has same hw addr? */
+	if (p && ether_addr_equal(br->dev->dev_addr, addr) &&
+	    (!vid || br_vlan_find(br, vid))) {
+		f->dst = NULL;
+		return;
+	}
+
 	fdb_delete(br, f);
 }
 
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 0902658..673ef9d 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -584,6 +584,7 @@ struct sk_buff *br_handle_vlan(struct net_bridge *br,
 int br_vlan_add(struct net_bridge *br, u16 vid, u16 flags);
 int br_vlan_delete(struct net_bridge *br, u16 vid);
 void br_vlan_flush(struct net_bridge *br);
+bool br_vlan_find(struct net_bridge *br, u16 vid);
 int br_vlan_filter_toggle(struct net_bridge *br, unsigned long val);
 int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags);
 int nbp_vlan_delete(struct net_bridge_port *port, u16 vid);
@@ -665,6 +666,11 @@ static inline void br_vlan_flush(struct net_bridge *br)
 {
 }
 
+static inline bool br_vlan_find(struct net_bridge *br, u16 vid)
+{
+	return false;
+}
+
 static inline int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags)
 {
 	return -EOPNOTSUPP;
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index af5ebd1..f87ab88f 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -316,6 +316,25 @@ void br_vlan_flush(struct net_bridge *br)
 	__vlan_flush(pv);
 }
 
+bool br_vlan_find(struct net_bridge *br, u16 vid)
+{
+	struct net_port_vlans *pv;
+	bool found = false;
+
+	rcu_read_lock();
+	pv = rcu_dereference(br->vlan_info);
+
+	if (!pv)
+		goto out;
+
+	if (test_bit(vid, pv->vlan_bitmap))
+		found = true;
+
+out:
+	rcu_read_unlock();
+	return found;
+}
+
 int br_vlan_filter_toggle(struct net_bridge *br, unsigned long val)
 {
 	if (!rtnl_trylock())
-- 
1.8.1.2

[PATCH net 5/7] bridge: Properly check if local fdb entry can be deleted in br_fdb_change_mac_address

[Toshiaki Makita]

br_fdb_change_mac_address() doesn't check if the local entry has the
same address as any of bridge ports.
Although I'm not sure when it is beneficial, current implementation allow
the bridge device to receive any mac address of its ports.
To preserve this behavior, we have to check if the mac address of the
entry we are deleting is identical to that of any port.

With that check, as the logic of changing local entries is almost the
same as br_fdb_changeaddr(), create a common function
br_fdb_change_locals() and call it from br_fdb_changeaddr() and
br_fdb_change_mac_address().
Now these two functions have only one line, make them inline functions.

Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 net/bridge/br_fdb.c     | 37 ++++---------------------------------
 net/bridge/br_private.h | 17 +++++++++++++++--
 2 files changed, 19 insertions(+), 35 deletions(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 2c79b1b..d39b525 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -124,11 +124,11 @@ static void fdb_delete_local(struct net_bridge *br,
 	fdb_delete(br, f);
 }
 
-void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
+void br_fdb_change_locals(struct net_bridge *br, struct net_bridge_port *p,
+			  const unsigned char *oldaddr,
+			  const unsigned char *newaddr)
 {
-	struct net_bridge *br = p->br;
 	struct net_port_vlans *pv;
-	const unsigned char *oldaddr = p->prev_addr.addr;
 	u16 vid;
 
 	ASSERT_RTNL();
@@ -147,7 +147,7 @@ void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
 	 * port.  This function runs under RTNL so the bitmap will not
 	 * change from under us.
 	 */
-	pv = nbp_get_vlan_info(p);
+	pv = p ? nbp_get_vlan_info(p) : br_get_vlan_info(br);
 	if (!pv)
 		goto out;
 
@@ -160,35 +160,6 @@ out:
 	spin_unlock_bh(&br->hash_lock);
 }
 
-void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
-{
-	struct net_bridge_fdb_entry *f;
-	struct net_port_vlans *pv;
-	u16 vid = 0;
-
-	/* If old entry was unassociated with any port, then delete it. */
-	f = __br_fdb_get(br, br->dev->dev_addr, 0);
-	if (f && f->is_local && !f->dst)
-		fdb_delete(br, f);
-
-	fdb_insert(br, NULL, newaddr, 0);
-
-	/* Now remove and add entries for every VLAN configured on the
-	 * bridge.  This function runs under RTNL so the bitmap will not
-	 * change from under us.
-	 */
-	pv = br_get_vlan_info(br);
-	if (!pv)
-		return;
-
-	for_each_set_bit_from(vid, pv->vlan_bitmap, VLAN_N_VID) {
-		f = __br_fdb_get(br, br->dev->dev_addr, vid);
-		if (f && f->is_local && !f->dst)
-			fdb_delete(br, f);
-		fdb_insert(br, NULL, newaddr, vid);
-	}
-}
-
 void br_fdb_cleanup(unsigned long _data)
 {
 	struct net_bridge *br = (struct net_bridge *)_data;
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 673ef9d..50f0b0d 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -379,8 +379,9 @@ static inline void br_netpoll_disable(struct net_bridge_port *p)
 int br_fdb_init(void);
 void br_fdb_fini(void);
 void br_fdb_flush(struct net_bridge *br);
-void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr);
-void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr);
+void br_fdb_change_locals(struct net_bridge *br, struct net_bridge_port *p,
+			  const unsigned char *oldaddr,
+			  const unsigned char *newaddr);
 void br_fdb_cleanup(unsigned long arg);
 void br_fdb_delete_by_port(struct net_bridge *br,
 			   const struct net_bridge_port *p, int do_all);
@@ -402,6 +403,18 @@ int br_fdb_add(struct ndmsg *nlh, struct nlattr *tb[], struct net_device *dev,
 int br_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb,
 		struct net_device *dev, int idx);
 
+static inline void br_fdb_changeaddr(struct net_bridge_port *p,
+				     const unsigned char *newaddr)
+{
+	br_fdb_change_locals(p->br, p, p->prev_addr.addr, newaddr);
+}
+
+static inline void br_fdb_change_mac_address(struct net_bridge *br,
+					     const u8 *newaddr)
+{
+	br_fdb_change_locals(br, NULL, br->dev->dev_addr, newaddr);
+}
+
 /* br_forward.c */
 void br_deliver(const struct net_bridge_port *to, struct sk_buff *skb);
 int br_dev_queue_push_xmit(struct sk_buff *skb);
-- 
1.8.1.2

[PATCH net 6/7] bridge: Properly check if local fdb entry can be deleted in br_fdb_delete_by_port

[Toshiaki Makita]

br_fdb_delete_by_port() doesn't care about vlan and mac addresses of the
bridge device.

We can use the same logic as other codes to check if a local entry can
be deleted.
To share the code, create __fdb_delete_local() and call it from
fdb_delete_local() and br_fdb_delete_by_port().

Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 net/bridge/br_fdb.c | 52 ++++++++++++++++++++++++----------------------------
 1 file changed, 24 insertions(+), 28 deletions(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index d39b525..d87d4cd 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -92,19 +92,14 @@ static void fdb_delete(struct net_bridge *br, struct net_bridge_fdb_entry *f)
 	call_rcu(&f->rcu, fdb_rcu_free);
 }
 
-/* Delete a local entry if no other port had the same address. */
-static void fdb_delete_local(struct net_bridge *br,
-			     const struct net_bridge_port *p,
-			     const unsigned char *addr, u16 vid)
+static void __fdb_delete_local(struct net_bridge *br,
+			       const struct net_bridge_port *p,
+			       struct net_bridge_fdb_entry *f)
 {
-	struct hlist_head *head = &br->hash[br_mac_hash(addr, vid)];
-	struct net_bridge_fdb_entry *f;
+	const unsigned char *addr = f->addr.addr;
+	u16 vid = f->vlan_id;
 	struct net_bridge_port *op;
 
-	f = fdb_find(head, addr, vid);
-	if (!f || !f->is_local || f->dst != p)
-		return;
-
 	/* Maybe another port has same hw addr? */
 	list_for_each_entry(op, &br->port_list, list) {
 		if (op != p && ether_addr_equal(op->dev->dev_addr, addr) &&
@@ -124,6 +119,21 @@ static void fdb_delete_local(struct net_bridge *br,
 	fdb_delete(br, f);
 }
 
+/* Delete a local entry if no other port had the same address. */
+static void fdb_delete_local(struct net_bridge *br,
+			     const struct net_bridge_port *p,
+			     const unsigned char *addr, u16 vid)
+{
+	struct hlist_head *head = &br->hash[br_mac_hash(addr, vid)];
+	struct net_bridge_fdb_entry *f;
+
+	f = fdb_find(head, addr, vid);
+	if (!f || !f->is_local || f->dst != p)
+		return;
+
+	__fdb_delete_local(br, p, f);
+}
+
 void br_fdb_change_locals(struct net_bridge *br, struct net_bridge_port *p,
 			  const unsigned char *oldaddr,
 			  const unsigned char *newaddr)
@@ -226,25 +236,11 @@ void br_fdb_delete_by_port(struct net_bridge *br,
 
 			if (f->is_static && !do_all)
 				continue;
-			/*
-			 * if multiple ports all have the same device address
-			 * then when one port is deleted, assign
-			 * the local entry to other port
-			 */
-			if (f->is_local) {
-				struct net_bridge_port *op;
-				list_for_each_entry(op, &br->port_list, list) {
-					if (op != p &&
-					    ether_addr_equal(op->dev->dev_addr,
-							     f->addr.addr)) {
-						f->dst = op;
-						goto skip_delete;
-					}
-				}
-			}
 
-			fdb_delete(br, f);
-		skip_delete: ;
+			if (f->is_local)
+				__fdb_delete_local(br, p, f);
+			else
+				fdb_delete(br, f);
 		}
 	}
 	spin_unlock_bh(&br->hash_lock);
-- 
1.8.1.2

[PATCH net 7/7] bridge: Properly check if local fdb entry can be deleted when deleting vlan

[Toshiaki Makita]

Vlan codes unconditionally delete local fdb entries.
We should consider the possibility that other ports have the same
address and vlan.

Example of problematic case:
  ip link set eth0 address 12:34:56:78:90:ab
  ip link set eth1 address aa:bb:cc:dd:ee:ff
  brctl addif br0 eth0
  brctl addif br0 eth1 # br0 will have mac address 12:34:56:78:90:ab
  bridge vlan add dev eth0 vid 10
  bridge vlan add dev eth1 vid 10
  bridge vlan add dev br0 vid 10 self
We will have fdb entry such that f->dst == eth0, f->vlan_id == 10 and
f->addr == 12:34:56:78:90:ab at this time.
Next, delete eth0 vlan 10.
  bridge vlan del dev eth0 vid 10
In this case, we still need the entry for br0, but it will be deleted.

Note that br0 needs the entry even though its mac address is not set
manually. To delete the entry with proper condition checking,
fdb_delete_local() is suitable to use.

Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 net/bridge/br_fdb.c     | 9 ++++-----
 net/bridge/br_private.h | 3 ++-
 net/bridge/br_vlan.c    | 4 ++--
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index d87d4cd..2688ea8 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -120,9 +120,8 @@ static void __fdb_delete_local(struct net_bridge *br,
 }
 
 /* Delete a local entry if no other port had the same address. */
-static void fdb_delete_local(struct net_bridge *br,
-			     const struct net_bridge_port *p,
-			     const unsigned char *addr, u16 vid)
+void fdb_delete_local(struct net_bridge *br, const struct net_bridge_port *p,
+		      const unsigned char *addr, u16 vid)
 {
 	struct hlist_head *head = &br->hash[br_mac_hash(addr, vid)];
 	struct net_bridge_fdb_entry *f;
@@ -737,8 +736,8 @@ out:
 	return err;
 }
 
-int fdb_delete_by_addr(struct net_bridge *br, const u8 *addr,
-		       u16 vlan)
+static int fdb_delete_by_addr(struct net_bridge *br, const u8 *addr,
+			      u16 vlan)
 {
 	struct hlist_head *head = &br->hash[br_mac_hash(addr, vlan)];
 	struct net_bridge_fdb_entry *fdb;
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 50f0b0d..1763908 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -379,6 +379,8 @@ static inline void br_netpoll_disable(struct net_bridge_port *p)
 int br_fdb_init(void);
 void br_fdb_fini(void);
 void br_fdb_flush(struct net_bridge *br);
+void fdb_delete_local(struct net_bridge *br, const struct net_bridge_port *p,
+		      const unsigned char *addr, u16 vid);
 void br_fdb_change_locals(struct net_bridge *br, struct net_bridge_port *p,
 			  const unsigned char *oldaddr,
 			  const unsigned char *newaddr);
@@ -394,7 +396,6 @@ int br_fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
 		  const unsigned char *addr, u16 vid);
 void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
 		   const unsigned char *addr, u16 vid);
-int fdb_delete_by_addr(struct net_bridge *br, const u8 *addr, u16 vid);
 
 int br_fdb_delete(struct ndmsg *ndm, struct nlattr *tb[],
 		  struct net_device *dev, const unsigned char *addr);
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index f87ab88f..e4d5c7f 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -297,7 +297,7 @@ int br_vlan_delete(struct net_bridge *br, u16 vid)
 		return -EINVAL;
 
 	spin_lock_bh(&br->hash_lock);
-	fdb_delete_by_addr(br, br->dev->dev_addr, vid);
+	fdb_delete_local(br, NULL, br->dev->dev_addr, vid);
 	spin_unlock_bh(&br->hash_lock);
 
 	__vlan_del(pv, vid);
@@ -400,7 +400,7 @@ int nbp_vlan_delete(struct net_bridge_port *port, u16 vid)
 		return -EINVAL;
 
 	spin_lock_bh(&port->br->hash_lock);
-	fdb_delete_by_addr(port->br, port->dev->dev_addr, vid);
+	fdb_delete_local(port->br, port, port->dev->dev_addr, vid);
 	spin_unlock_bh(&port->br->hash_lock);
 
 	return __vlan_del(pv, vid);
-- 
1.8.1.2

Re: [PATCH net 3/7] bridge: Change local fdb entries whenever mac address of bridge device changes

[Vlad Yasevich]

On 12/02/2013 01:40 AM, Toshiaki Makita wrote:
> Vlan code may need fdb change when changing mac address of bridge device
> even if it is caused by the mac address changing of a bridge port.
> 
> Example configuration:
>   ip link set eth0 address 12:34:56:78:90:ab
>   ip link set eth1 address aa:bb:cc:dd:ee:ff
>   brctl addif br0 eth0
>   brctl addif br0 eth1 # br0 will have mac address 12:34:56:78:90:ab
>   bridge vlan add dev br0 vid 10 self
>   bridge vlan add dev eth0 vid 10
> We will have fdb entry such that f->dst == NULL, f->vlan_id == 10 and
> f->addr == 12:34:56:78:90:ab at this time.
> Next, change the mac address of eth0 to greater value.
>   ip link set eth0 address ee:ff:12:34:56:78
> Then, mac address of br0 will be recalculated and set to aa:bb:cc:dd:ee:ff.
> However, an entry aa:bb:cc:dd:ee:ff will not be created and we will be not
> able to communicate using br0 on vlan 10.
> 
> Address this issue by deleting and adding local entries whenever
> changing the mac address of the bridge device.
> 
> If there already exists an entry that has the same address, for example,
> in case that br_fdb_changeaddr() has already inserted it,
> br_fdb_change_mac_address() will simply fail to insert it and no
> duplicated entry will be made, as it was.
> 
> Note that br_add_if() calls br_stp_recalculate_bridge_id() before
> br_fdb_insert() and an entry whose dst == NULL will be created in this case.
> Though such an entry wouldn't be created in this function without this
> patch, I think this behavior will not be harmful because we already have
> such entries with vlan_filtering enabled or setting mac address of the
> bridge device manually.
> 
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> ---
> It seems to be not difficult to change the code not to create an entry whose
> dst == NULL at br_add_if().  It can be acheived by calling br_fdb_insert()
> before br_stp_recalculate_bridge_id() (and actually br_device_event() changes
> a fdb entry in such a way).
> If it is desirable, I will do that change.

Yes, I think this is desirable.  An fdb where dst == NULL, is typically
only created when the mac address of the bridge device itself is set by
user.  When the bridge carries the mac of one of the ports, we don't
need that dst entry.

-vlad

> 
>  net/bridge/br_device.c | 1 -
>  net/bridge/br_stp_if.c | 2 ++
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
> index d967773..1cbdbf1 100644
> --- a/net/bridge/br_device.c
> +++ b/net/bridge/br_device.c
> @@ -187,7 +187,6 @@ static int br_set_mac_address(struct net_device *dev, void *p)
>  
>  	spin_lock_bh(&br->lock);
>  	if (!ether_addr_equal(dev->dev_addr, addr->sa_data)) {
> -		br_fdb_change_mac_address(br, addr->sa_data);
>  		/* Mac address will be changed in br_stp_change_bridge_id(). */
>  		br_stp_change_bridge_id(br, addr->sa_data);
>  	}
> diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
> index 656a6f3..189ba1e 100644
> --- a/net/bridge/br_stp_if.c
> +++ b/net/bridge/br_stp_if.c
> @@ -194,6 +194,8 @@ void br_stp_change_bridge_id(struct net_bridge *br, const unsigned char *addr)
>  
>  	wasroot = br_is_root_bridge(br);
>  
> +	br_fdb_change_mac_address(br, addr);
> +
>  	memcpy(oldaddr, br->bridge_id.addr, ETH_ALEN);
>  	memcpy(br->bridge_id.addr, addr, ETH_ALEN);
>  	memcpy(br->dev->dev_addr, addr, ETH_ALEN);
> 

Re: [PATCH net 4/7] bridge: Fix the way checking if a local fdb entry can be deleted

[Vlad Yasevich]

On 12/02/2013 01:40 AM, Toshiaki Makita wrote:
> We should take into account the followings when deleting a local fdb entry.
> 
> - nbp_vlan_find() can be used only when vid != 0 to check if an entry is
>   deletable, because a fdb entry with vid 0 can exist at any time but
>   nbp_vlan_find() always return false with vid 0.
> 
>   Example of problematic case:
>     ip link set eth0 address 12:34:56:78:90:ab
>     ip link set eth1 address 12:34:56:78:90:ab
>     brctl addif br0 eth0
>     brctl addif br0 eth1
>     ip link set eth0 address aa:bb:cc:dd:ee:ff
>   Then, the fdb entry 12:34:56:78:90:ab will be deleted even though the
>   bridge port eth1 still has that address.
> 
> - The port to which the bridge device is attached might needs a local entry
>   if its mac address is set manually.
> 
>   Example of problematic case:
>     ip link set eth0 address 12:34:56:78:90:ab
>     brctl addif br0 eth0
>     ip link set br0 address 12:34:56:78:90:ab
>     ip link set eth0 address aa:bb:cc:dd:ee:ff
>   Then, the fdb still must have the entry 12:34:56:78:90:ab, but it will be
>   deleted.
> 
> We can use br->dev->addr_assign_type to check if the address is manually
> set or not, but I propose another approach.
> 
> Since we delete and insert local entries whenever changing mac address
> of the bridge device, we can change dst of the entry to NULL regardless of
> addr_assign_type when deleting an entry associated with a certain port,
> and if it is found to be unnecessary later, then delete it.
> That is, if changing mac address of a port, the entry might be changed
> to its dst being NULL first, but is eventually deleted when recalculating
> and changing bridge id.
> 
> This approach is useful when we want to share the code with deleting
> vlan in which the bridge device might want such an entry regardless of
> addr_assign_type, and makes things easy because we don't have to consider
> if mac address of the bridge device will be changed or not at
> fdb_delete_local().

This is a nifty approach, but it does have one side-effect that I am not
sure is correct.  In the case where bridge mac address is not manually
set, a fdb entry for the removed address survives past the
synchronize_net() call.  This would result in a behavioral change where
packets that always used to flood, would now sometimes be delivered
only to the bridge.

-vlad


> 
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> ---
>  net/bridge/br_fdb.c     |  9 ++++++++-
>  net/bridge/br_private.h |  6 ++++++
>  net/bridge/br_vlan.c    | 19 +++++++++++++++++++
>  3 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index d29e184..2c79b1b 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -108,12 +108,19 @@ static void fdb_delete_local(struct net_bridge *br,
>  	/* Maybe another port has same hw addr? */
>  	list_for_each_entry(op, &br->port_list, list) {
>  		if (op != p && ether_addr_equal(op->dev->dev_addr, addr) &&
> -		    nbp_vlan_find(op, vid)) {
> +		    (!vid || nbp_vlan_find(op, vid))) {
>  			f->dst = op;
>  			return;
>  		}
>  	}
>  
> +	/* Maybe bridge device has same hw addr? */
> +	if (p && ether_addr_equal(br->dev->dev_addr, addr) &&
> +	    (!vid || br_vlan_find(br, vid))) {
> +		f->dst = NULL;
> +		return;
> +	}
> +
>  	fdb_delete(br, f);
>  }
>  
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 0902658..673ef9d 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -584,6 +584,7 @@ struct sk_buff *br_handle_vlan(struct net_bridge *br,
>  int br_vlan_add(struct net_bridge *br, u16 vid, u16 flags);
>  int br_vlan_delete(struct net_bridge *br, u16 vid);
>  void br_vlan_flush(struct net_bridge *br);
> +bool br_vlan_find(struct net_bridge *br, u16 vid);
>  int br_vlan_filter_toggle(struct net_bridge *br, unsigned long val);
>  int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags);
>  int nbp_vlan_delete(struct net_bridge_port *port, u16 vid);
> @@ -665,6 +666,11 @@ static inline void br_vlan_flush(struct net_bridge *br)
>  {
>  }
>  
> +static inline bool br_vlan_find(struct net_bridge *br, u16 vid)
> +{
> +	return false;
> +}
> +
>  static inline int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags)
>  {
>  	return -EOPNOTSUPP;
> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> index af5ebd1..f87ab88f 100644
> --- a/net/bridge/br_vlan.c
> +++ b/net/bridge/br_vlan.c
> @@ -316,6 +316,25 @@ void br_vlan_flush(struct net_bridge *br)
>  	__vlan_flush(pv);
>  }
>  
> +bool br_vlan_find(struct net_bridge *br, u16 vid)
> +{
> +	struct net_port_vlans *pv;
> +	bool found = false;
> +
> +	rcu_read_lock();
> +	pv = rcu_dereference(br->vlan_info);
> +
> +	if (!pv)
> +		goto out;
> +
> +	if (test_bit(vid, pv->vlan_bitmap))
> +		found = true;
> +
> +out:
> +	rcu_read_unlock();
> +	return found;
> +}
> +
>  int br_vlan_filter_toggle(struct net_bridge *br, unsigned long val)
>  {
>  	if (!rtnl_trylock())
> 

Re: [PATCH net 1/7] bridge: Fix the way finding the old local fdb entry in br_fdb_changeaddr

[Stephen Hemminger]

On Mon,  2 Dec 2013 15:40:32 +0900
Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> wrote:

> br_fdb_changeaddr() assumes that there is at most one local entry per port
> per vlan. It used to be true, but since commit 36fd2b63e3b4 ("bridge: allow
> creating/deleting fdb entries via netlink"), it has been not.
> Therefore, the function might fail to search a correct previous address
> to be deleted and delete an arbitrary local entry if user has added local
> entries manually.
> 
> Example of problematic case:
>   ip link set eth0 address ee:ff:12:34:56:78
>   brctl addif br0 eth0
>   bridge fdb add 12:34:56:78:90:ab dev eth0 master
>   ip link set eth0 address aa:bb:cc:dd:ee:ff
> Then, the address 12:34:56:78:90:ab might be deleted instead of
> ee:ff:12:34:56:78, the original mac address of eth0.
> 
> To fix this, remember the mac addresses of bridge ports and use them to
> find old fdb entries when changing their mac addresses.
> It is no longer necessary to seek all fdb hash chains, as we can call
> fdb_find() with the address to get the corresponding entry.
> 
> This approach also has secondary effects that we will come to be able to
> share the code between br_fdb_changeaddr() and br_fdb_change_mac_address(),
> and prevent unnecessary bridge id recalculation in br_device_event().
> 
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> ---
>  net/bridge/br_fdb.c     | 89 ++++++++++++++++++++++++++++---------------------
>  net/bridge/br_if.c      |  1 +
>  net/bridge/br_notify.c  |  9 +++--
>  net/bridge/br_private.h |  1 +
>  4 files changed, 59 insertions(+), 41 deletions(-)
> 
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index 33e8f23..d29e184 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -27,6 +27,9 @@
>  #include "br_private.h"
>  
>  static struct kmem_cache *br_fdb_cache __read_mostly;
> +static struct net_bridge_fdb_entry *fdb_find(struct hlist_head *head,
> +					     const unsigned char *addr,
> +					     __u16 vid);
>  static int fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
>  		      const unsigned char *addr, u16 vid);
>  static void fdb_notify(struct net_bridge *br,
> @@ -89,54 +92,64 @@ static void fdb_delete(struct net_bridge *br, struct net_bridge_fdb_entry *f)
>  	call_rcu(&f->rcu, fdb_rcu_free);
>  }
>  
> +/* Delete a local entry if no other port had the same address. */
> +static void fdb_delete_local(struct net_bridge *br,
> +			     const struct net_bridge_port *p,
> +			     const unsigned char *addr, u16 vid)
> +{
> +	struct hlist_head *head = &br->hash[br_mac_hash(addr, vid)];
> +	struct net_bridge_fdb_entry *f;
> +	struct net_bridge_port *op;
> +
> +	f = fdb_find(head, addr, vid);
> +	if (!f || !f->is_local || f->dst != p)
> +		return;
> +
> +	/* Maybe another port has same hw addr? */
> +	list_for_each_entry(op, &br->port_list, list) {
> +		if (op != p && ether_addr_equal(op->dev->dev_addr, addr) &&
> +		    nbp_vlan_find(op, vid)) {
> +			f->dst = op;
> +			return;
> +		}
> +	}
> +
> +	fdb_delete(br, f);
> +}
> +
>  void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
>  {
>  	struct net_bridge *br = p->br;
> -	bool no_vlan = (nbp_get_vlan_info(p) == NULL) ? true : false;
> -	int i;
> +	struct net_port_vlans *pv;
> +	const unsigned char *oldaddr = p->prev_addr.addr;
> +	u16 vid;
> +
> +	ASSERT_RTNL();
>  
>  	spin_lock_bh(&br->hash_lock);
>  
> -	/* Search all chains since old address/hash is unknown */
> -	for (i = 0; i < BR_HASH_SIZE; i++) {
> -		struct hlist_node *h;
> -		hlist_for_each(h, &br->hash[i]) {
> -			struct net_bridge_fdb_entry *f;
> +	/* Delete old one, may fail if corresponding entry was associated
> +	 * with another port or user deleted it.
> +	 */
> +	fdb_delete_local(br, p, oldaddr, 0);
>  
> -			f = hlist_entry(h, struct net_bridge_fdb_entry, hlist);
> -			if (f->dst == p && f->is_local) {
> -				/* maybe another port has same hw addr? */
> -				struct net_bridge_port *op;
> -				u16 vid = f->vlan_id;
> -				list_for_each_entry(op, &br->port_list, list) {
> -					if (op != p &&
> -					    ether_addr_equal(op->dev->dev_addr,
> -							     f->addr.addr) &&
> -					    nbp_vlan_find(op, vid)) {
> -						f->dst = op;
> -						goto insert;
> -					}
> -				}
> +	/* Insert new address, may fail if invalid address or dup. */
> +	fdb_insert(br, p, newaddr, 0);
>  
> -				/* delete old one */
> -				fdb_delete(br, f);
> -insert:
> -				/* insert new address,  may fail if invalid
> -				 * address or dup.
> -				 */
> -				fdb_insert(br, p, newaddr, vid);
> -
> -				/* if this port has no vlan information
> -				 * configured, we can safely be done at
> -				 * this point.
> -				 */
> -				if (no_vlan)
> -					goto done;
> -			}
> -		}
> +	/* Now remove and add entries for every VLAN configured on the
> +	 * port.  This function runs under RTNL so the bitmap will not
> +	 * change from under us.
> +	 */
> +	pv = nbp_get_vlan_info(p);
> +	if (!pv)
> +		goto out;
> +
> +	for_each_set_bit(vid, pv->vlan_bitmap, VLAN_N_VID) {
> +		fdb_delete_local(br, p, oldaddr, vid);
> +		fdb_insert(br, p, newaddr, vid);
>  	}
>  
> -done:
> +out:
>  	spin_unlock_bh(&br->hash_lock);
>  }
>  
> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> index 4bf02ad..346b2b2 100644
> --- a/net/bridge/br_if.c
> +++ b/net/bridge/br_if.c
> @@ -406,6 +406,7 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
>  
>  	if (br_fdb_insert(br, p, dev->dev_addr, 0))
>  		netdev_err(dev, "failed insert local address bridge forwarding table\n");
> +	memcpy(p->prev_addr.addr, dev->dev_addr, ETH_ALEN);
>  
>  	kobject_uevent(&p->kobj, KOBJ_ADD);
>  
> diff --git a/net/bridge/br_notify.c b/net/bridge/br_notify.c
> index 2998dd1..3d3f7a1 100644
> --- a/net/bridge/br_notify.c
> +++ b/net/bridge/br_notify.c
> @@ -34,7 +34,7 @@ static int br_device_event(struct notifier_block *unused, unsigned long event, v
>  	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
>  	struct net_bridge_port *p;
>  	struct net_bridge *br;
> -	bool changed_addr;
> +	bool changed_addr = false;
>  	int err;
>  
>  	/* register of bridge completed, add sysfs entries */
> @@ -57,8 +57,11 @@ static int br_device_event(struct notifier_block *unused, unsigned long event, v
>  
>  	case NETDEV_CHANGEADDR:
>  		spin_lock_bh(&br->lock);
> -		br_fdb_changeaddr(p, dev->dev_addr);
> -		changed_addr = br_stp_recalculate_bridge_id(br);
> +		if (!ether_addr_equal(dev->dev_addr, p->prev_addr.addr)) {
> +			br_fdb_changeaddr(p, dev->dev_addr);
> +			memcpy(p->prev_addr.addr, dev->dev_addr, ETH_ALEN);
> +			changed_addr = br_stp_recalculate_bridge_id(br);
> +		}
>  		spin_unlock_bh(&br->lock);
>  
>  		if (changed_addr)
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 229d820..0902658 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -195,6 +195,7 @@ struct net_bridge_port
>  #ifdef CONFIG_BRIDGE_VLAN_FILTERING
>  	struct net_port_vlans __rcu	*vlan_info;
>  #endif
> +	mac_addr			prev_addr;
>  };

There must be a better way without all this extra book keeping which risks
getting out of sync.  IT seems to me that NETDEV_CHANGEADDR should not
be notified (from core) if old == new address.

Re: [PATCH net 3/7] bridge: Change local fdb entries whenever mac address of bridge device changes

[Toshiaki Makita]

On Mon, 2013-12-02 at 10:43 -0500, Vlad Yasevich wrote:
> On 12/02/2013 01:40 AM, Toshiaki Makita wrote:
> > Vlan code may need fdb change when changing mac address of bridge device
> > even if it is caused by the mac address changing of a bridge port.
...
> > ---
> > It seems to be not difficult to change the code not to create an entry whose
> > dst == NULL at br_add_if().  It can be acheived by calling br_fdb_insert()
> > before br_stp_recalculate_bridge_id() (and actually br_device_event() changes
> > a fdb entry in such a way).
> > If it is desirable, I will do that change.
> 
> Yes, I think this is desirable.  An fdb where dst == NULL, is typically
> only created when the mac address of the bridge device itself is set by
> user.  When the bridge carries the mac of one of the ports, we don't
> need that dst entry.

OK, I will rework it.

Thanks,
Toshiaki Makita

> 
> -vlad

Re: [PATCH net 4/7] bridge: Fix the way checking if a local fdb entry can be deleted

[Toshiaki Makita]

On Mon, 2013-12-02 at 12:07 -0500, Vlad Yasevich wrote:
> On 12/02/2013 01:40 AM, Toshiaki Makita wrote:
> > We should take into account the followings when deleting a local fdb entry.
> > 
> > - nbp_vlan_find() can be used only when vid != 0 to check if an entry is
> >   deletable, because a fdb entry with vid 0 can exist at any time but
> >   nbp_vlan_find() always return false with vid 0.
> > 
> >   Example of problematic case:
> >     ip link set eth0 address 12:34:56:78:90:ab
> >     ip link set eth1 address 12:34:56:78:90:ab
> >     brctl addif br0 eth0
> >     brctl addif br0 eth1
> >     ip link set eth0 address aa:bb:cc:dd:ee:ff
> >   Then, the fdb entry 12:34:56:78:90:ab will be deleted even though the
> >   bridge port eth1 still has that address.
> > 
> > - The port to which the bridge device is attached might needs a local entry
> >   if its mac address is set manually.
> > 
> >   Example of problematic case:
> >     ip link set eth0 address 12:34:56:78:90:ab
> >     brctl addif br0 eth0
> >     ip link set br0 address 12:34:56:78:90:ab
> >     ip link set eth0 address aa:bb:cc:dd:ee:ff
> >   Then, the fdb still must have the entry 12:34:56:78:90:ab, but it will be
> >   deleted.
> > 
> > We can use br->dev->addr_assign_type to check if the address is manually
> > set or not, but I propose another approach.
> > 
> > Since we delete and insert local entries whenever changing mac address
> > of the bridge device, we can change dst of the entry to NULL regardless of
> > addr_assign_type when deleting an entry associated with a certain port,
> > and if it is found to be unnecessary later, then delete it.
> > That is, if changing mac address of a port, the entry might be changed
> > to its dst being NULL first, but is eventually deleted when recalculating
> > and changing bridge id.
> > 
> > This approach is useful when we want to share the code with deleting
> > vlan in which the bridge device might want such an entry regardless of
> > addr_assign_type, and makes things easy because we don't have to consider
> > if mac address of the bridge device will be changed or not at
> > fdb_delete_local().
> 
> This is a nifty approach, but it does have one side-effect that I am not
> sure is correct.  In the case where bridge mac address is not manually
> set, a fdb entry for the removed address survives past the
> synchronize_net() call.  This would result in a behavioral change where
> packets that always used to flood, would now sometimes be delivered
> only to the bridge.

I think no unnecessary entry will survive in any case.
It will survive only if the bridge device remains to have the same mac
address, because we check if the entry is necessary or not whenever the
address of the bridge device is changed (assuming patch 3/7 is applied).

Further analysis:
The function fdb_delete_local() (and __fdb_delete_local()) will called
by br_fdb_changeaddr(), br_fdb_change_mac_address(),
br_fdb_delete_by_port() (with do_all==1), br_vlan_delete(), and
nbp_vlan_delete() after all of this patch series are applied.
So, we have to consider only these 5 functions.

br_fdb_changeaddr():
It is called by br_device_event().
br_device_event() calls br_stp_recalculate_bridge_id() right after
calling br_fdb_changeaddr(), so if the address of bridge device should
be changed, the fdb entry will immediately reflect it and be deleted.

br_fdb_change_mac_address():
In this case, fdb_delete_local() is called with p==NULL, so it will not
be affected by this change.

br_fdb_delete_by_port() with do_all==1 and p!=NULL:
It is called by del_nbp() and del_nbp() is called by br_del_if() and
br_dev_delete().
br_del_if() calls br_stp_recalculate_bridge_id() right after calling
del_nbp(), so if the address of bridge device should be changed, the fdb
entry will immediately reflect it and be deleted.
br_dev_delete() calls br_fdb_delete_by_port() with p==NULL right after
calling del_nbp(), so all fdb entry will be immediately deleted.

br_vlan_delete():
In this case, fdb_delete_local() is called with p==NULL, so it will not
be affected by this change.

nbp_vlan_delete():
In this case, the address of bridge device will not be changed, and if
the bridge device has the same vlan, it survives. This is very my
intended behavior.

Thanks,
Toshiaki Makita

> 
> -vlad

Re: [PATCH net 1/7] bridge: Fix the way finding the old local fdb entry in br_fdb_changeaddr

[Toshiaki Makita]

On Mon, 2013-12-02 at 18:09 -0800, Stephen Hemminger wrote:
> On Mon,  2 Dec 2013 15:40:32 +0900
> Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> wrote:
> 
> > br_fdb_changeaddr() assumes that there is at most one local entry per port
> > per vlan. It used to be true, but since commit 36fd2b63e3b4 ("bridge: allow
> > creating/deleting fdb entries via netlink"), it has been not.
> > Therefore, the function might fail to search a correct previous address
> > to be deleted and delete an arbitrary local entry if user has added local
> > entries manually.
...
> > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> > index 229d820..0902658 100644
> > --- a/net/bridge/br_private.h
> > +++ b/net/bridge/br_private.h
> > @@ -195,6 +195,7 @@ struct net_bridge_port
> >  #ifdef CONFIG_BRIDGE_VLAN_FILTERING
> >  	struct net_port_vlans __rcu	*vlan_info;
> >  #endif
> > +	mac_addr			prev_addr;
> >  };
> 
> There must be a better way without all this extra book keeping which risks
> getting out of sync.

Another approach I can conceive is:
- Make fdb entries be able to be differentiated from those user added
  by introducing a flag in struct net_bridge_fdb_entry. 

Though this approach will be less efficient, it doesn't require to track
address changes. It will need to set a flag only when adding fdb
entries.

How do you think about it?

> IT seems to me that NETDEV_CHANGEADDR should not
> be notified (from core) if old == new address.

I'm afraid that user space will be affected if doing so because
br_device_event() notifies RTM_NEWLINK when receiving NETDEV_CHANGEADDR
and another subsystem might do that though I'm not sure.
So I think that checking old==new should be done in bridge code.

Thanks,
Toshiaki Makita

Re: [PATCH net 4/7] bridge: Fix the way checking if a local fdb entry can be deleted

[Vlad Yasevich]

On 12/03/2013 07:45 AM, Toshiaki Makita wrote:
> On Mon, 2013-12-02 at 12:07 -0500, Vlad Yasevich wrote:
>> On 12/02/2013 01:40 AM, Toshiaki Makita wrote:
>>> We should take into account the followings when deleting a local fdb entry.
>>>
>>> - nbp_vlan_find() can be used only when vid != 0 to check if an entry is
>>>   deletable, because a fdb entry with vid 0 can exist at any time but
>>>   nbp_vlan_find() always return false with vid 0.
>>>
>>>   Example of problematic case:
>>>     ip link set eth0 address 12:34:56:78:90:ab
>>>     ip link set eth1 address 12:34:56:78:90:ab
>>>     brctl addif br0 eth0
>>>     brctl addif br0 eth1
>>>     ip link set eth0 address aa:bb:cc:dd:ee:ff
>>>   Then, the fdb entry 12:34:56:78:90:ab will be deleted even though the
>>>   bridge port eth1 still has that address.
>>>
>>> - The port to which the bridge device is attached might needs a local entry
>>>   if its mac address is set manually.
>>>
>>>   Example of problematic case:
>>>     ip link set eth0 address 12:34:56:78:90:ab
>>>     brctl addif br0 eth0
>>>     ip link set br0 address 12:34:56:78:90:ab
>>>     ip link set eth0 address aa:bb:cc:dd:ee:ff
>>>   Then, the fdb still must have the entry 12:34:56:78:90:ab, but it will be
>>>   deleted.
>>>
>>> We can use br->dev->addr_assign_type to check if the address is manually
>>> set or not, but I propose another approach.
>>>
>>> Since we delete and insert local entries whenever changing mac address
>>> of the bridge device, we can change dst of the entry to NULL regardless of
>>> addr_assign_type when deleting an entry associated with a certain port,
>>> and if it is found to be unnecessary later, then delete it.
>>> That is, if changing mac address of a port, the entry might be changed
>>> to its dst being NULL first, but is eventually deleted when recalculating
>>> and changing bridge id.
>>>
>>> This approach is useful when we want to share the code with deleting
>>> vlan in which the bridge device might want such an entry regardless of
>>> addr_assign_type, and makes things easy because we don't have to consider
>>> if mac address of the bridge device will be changed or not at
>>> fdb_delete_local().
>>
>> This is a nifty approach, but it does have one side-effect that I am not
>> sure is correct.  In the case where bridge mac address is not manually
>> set, a fdb entry for the removed address survives past the
>> synchronize_net() call.  This would result in a behavioral change where
>> packets that always used to flood, would now sometimes be delivered
>> only to the bridge.
> 
> I think no unnecessary entry will survive in any case.
> It will survive only if the bridge device remains to have the same mac
> address, because we check if the entry is necessary or not whenever the
> address of the bridge device is changed (assuming patch 3/7 is applied).
> 
> Further analysis:
> The function fdb_delete_local() (and __fdb_delete_local()) will called
> by br_fdb_changeaddr(), br_fdb_change_mac_address(),
> br_fdb_delete_by_port() (with do_all==1), br_vlan_delete(), and
> nbp_vlan_delete() after all of this patch series are applied.
> So, we have to consider only these 5 functions.
> 
> br_fdb_changeaddr():
> It is called by br_device_event().
> br_device_event() calls br_stp_recalculate_bridge_id() right after
> calling br_fdb_changeaddr(), so if the address of bridge device should
> be changed, the fdb entry will immediately reflect it and be deleted.

This doesn't happen until later.  Currently nbp_del() will remove all
fdb entries for a given port.  With this patch, the local fdb entry for
the port will survive the removal process and the fdb->dst will be set
to NULL.
The port is now removed from the list, rx_handler is unregistered and we
push call synchronize_net() trying flush all packets currently in rcu
section.  Once this completes, the port and all the fdbs for it should
be removed, but now they are not. We have to wait for br_del_if() to
call the notifier call chain to remove the the fdb entry.  Any packet
arriving at the bridge for the mac address of the port that just got
removed will now be handed over to the bridge, instead of being flooded.
This is a change in behavior.

-vlad

> 
> br_fdb_change_mac_address():
> In this case, fdb_delete_local() is called with p==NULL, so it will not
> be affected by this change.
> 
> br_fdb_delete_by_port() with do_all==1 and p!=NULL:
> It is called by del_nbp() and del_nbp() is called by br_del_if() and
> br_dev_delete().
> br_del_if() calls br_stp_recalculate_bridge_id() right after calling
> del_nbp(), so if the address of bridge device should be changed, the fdb
> entry will immediately reflect it and be deleted.
> br_dev_delete() calls br_fdb_delete_by_port() with p==NULL right after
> calling del_nbp(), so all fdb entry will be immediately deleted.
> 
> br_vlan_delete():
> In this case, fdb_delete_local() is called with p==NULL, so it will not
> be affected by this change.
> 
> nbp_vlan_delete():
> In this case, the address of bridge device will not be changed, and if
> the bridge device has the same vlan, it survives. This is very my
> intended behavior.
> 
> Thanks,
> Toshiaki Makita
> 
>>
>> -vlad
> 
> 

Re: [PATCH net 1/7] bridge: Fix the way finding the old local fdb entry in br_fdb_changeaddr

[Stephen Hemminger]

On Tue, 03 Dec 2013 21:55:26 +0900
Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> wrote:

> Another approach I can conceive is:
> - Make fdb entries be able to be differentiated from those user added
>   by introducing a flag in struct net_bridge_fdb_entry. 

Yes, this makes more sense; and it would be useful for administration display commands.

Re: [PATCH net 1/7] bridge: Fix the way finding the old local fdb entry in br_fdb_changeaddr

[Toshiaki Makita]

On Tue, 2013-12-03 at 08:50 -0800, Stephen Hemminger wrote:
> On Tue, 03 Dec 2013 21:55:26 +0900
> Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> wrote:
> 
> > Another approach I can conceive is:
> > - Make fdb entries be able to be differentiated from those user added
> >   by introducing a flag in struct net_bridge_fdb_entry. 
> 
> Yes, this makes more sense; and it would be useful for administration display commands.

Thanks, I will change and resend this.

Toshiaki Makita

Re: [PATCH net 4/7] bridge: Fix the way checking if a local fdb entry can be deleted

[Toshiaki Makita]

On Tue, 2013-12-03 at 10:41 -0500, Vlad Yasevich wrote:
> On 12/03/2013 07:45 AM, Toshiaki Makita wrote:
> > On Mon, 2013-12-02 at 12:07 -0500, Vlad Yasevich wrote:
> >> On 12/02/2013 01:40 AM, Toshiaki Makita wrote:
> >>> We should take into account the followings when deleting a local fdb entry.
> >>>
> >>> - nbp_vlan_find() can be used only when vid != 0 to check if an entry is
> >>>   deletable, because a fdb entry with vid 0 can exist at any time but
> >>>   nbp_vlan_find() always return false with vid 0.
> >>>
> >>>   Example of problematic case:
> >>>     ip link set eth0 address 12:34:56:78:90:ab
> >>>     ip link set eth1 address 12:34:56:78:90:ab
> >>>     brctl addif br0 eth0
> >>>     brctl addif br0 eth1
> >>>     ip link set eth0 address aa:bb:cc:dd:ee:ff
> >>>   Then, the fdb entry 12:34:56:78:90:ab will be deleted even though the
> >>>   bridge port eth1 still has that address.
> >>>
> >>> - The port to which the bridge device is attached might needs a local entry
> >>>   if its mac address is set manually.
> >>>
> >>>   Example of problematic case:
> >>>     ip link set eth0 address 12:34:56:78:90:ab
> >>>     brctl addif br0 eth0
> >>>     ip link set br0 address 12:34:56:78:90:ab
> >>>     ip link set eth0 address aa:bb:cc:dd:ee:ff
> >>>   Then, the fdb still must have the entry 12:34:56:78:90:ab, but it will be
> >>>   deleted.
> >>>
> >>> We can use br->dev->addr_assign_type to check if the address is manually
> >>> set or not, but I propose another approach.
> >>>
> >>> Since we delete and insert local entries whenever changing mac address
> >>> of the bridge device, we can change dst of the entry to NULL regardless of
> >>> addr_assign_type when deleting an entry associated with a certain port,
> >>> and if it is found to be unnecessary later, then delete it.
> >>> That is, if changing mac address of a port, the entry might be changed
> >>> to its dst being NULL first, but is eventually deleted when recalculating
> >>> and changing bridge id.
> >>>
> >>> This approach is useful when we want to share the code with deleting
> >>> vlan in which the bridge device might want such an entry regardless of
> >>> addr_assign_type, and makes things easy because we don't have to consider
> >>> if mac address of the bridge device will be changed or not at
> >>> fdb_delete_local().
> >>
> >> This is a nifty approach, but it does have one side-effect that I am not
> >> sure is correct.  In the case where bridge mac address is not manually
> >> set, a fdb entry for the removed address survives past the
> >> synchronize_net() call.  This would result in a behavioral change where
> >> packets that always used to flood, would now sometimes be delivered
> >> only to the bridge.
> > 
> > I think no unnecessary entry will survive in any case.
> > It will survive only if the bridge device remains to have the same mac
> > address, because we check if the entry is necessary or not whenever the
> > address of the bridge device is changed (assuming patch 3/7 is applied).
> > 
> > Further analysis:
> > The function fdb_delete_local() (and __fdb_delete_local()) will called
> > by br_fdb_changeaddr(), br_fdb_change_mac_address(),
> > br_fdb_delete_by_port() (with do_all==1), br_vlan_delete(), and
> > nbp_vlan_delete() after all of this patch series are applied.
> > So, we have to consider only these 5 functions.
> > 
> > br_fdb_changeaddr():
> > It is called by br_device_event().
> > br_device_event() calls br_stp_recalculate_bridge_id() right after
> > calling br_fdb_changeaddr(), so if the address of bridge device should
> > be changed, the fdb entry will immediately reflect it and be deleted.
> 
> This doesn't happen until later.  Currently nbp_del() will remove all
> fdb entries for a given port.  With this patch, the local fdb entry for
> the port will survive the removal process and the fdb->dst will be set
> to NULL.
> The port is now removed from the list, rx_handler is unregistered and we
> push call synchronize_net() trying flush all packets currently in rcu
> section.  Once this completes, the port and all the fdbs for it should
> be removed, but now they are not. We have to wait for br_del_if() to
> call the notifier call chain to remove the the fdb entry.  Any packet
> arriving at the bridge for the mac address of the port that just got
> removed will now be handed over to the bridge, instead of being flooded.
> This is a change in behavior.

I see. Indeed this is a change in behavior.

But the bridge device still has the mac address after synchronize_net(),
and it will be actually changed in br_stp_recalculate_bridge_id().
Though that port has gone, as the bridge still uses the address, I'm
thinking the corresponding entry should exist.

Thanks,
Toshiaki Makita

> 
> -vlad

Re: [PATCH net 4/7] bridge: Fix the way checking if a local fdb entry can be deleted

[Vlad Yasevich]

On 12/04/2013 03:29 AM, Toshiaki Makita wrote:
> On Tue, 2013-12-03 at 10:41 -0500, Vlad Yasevich wrote:
>> On 12/03/2013 07:45 AM, Toshiaki Makita wrote:
>>> On Mon, 2013-12-02 at 12:07 -0500, Vlad Yasevich wrote:
>>>> On 12/02/2013 01:40 AM, Toshiaki Makita wrote:
>>>>> We should take into account the followings when deleting a local fdb entry.
>>>>>
>>>>> - nbp_vlan_find() can be used only when vid != 0 to check if an entry is
>>>>>   deletable, because a fdb entry with vid 0 can exist at any time but
>>>>>   nbp_vlan_find() always return false with vid 0.
>>>>>
>>>>>   Example of problematic case:
>>>>>     ip link set eth0 address 12:34:56:78:90:ab
>>>>>     ip link set eth1 address 12:34:56:78:90:ab
>>>>>     brctl addif br0 eth0
>>>>>     brctl addif br0 eth1
>>>>>     ip link set eth0 address aa:bb:cc:dd:ee:ff
>>>>>   Then, the fdb entry 12:34:56:78:90:ab will be deleted even though the
>>>>>   bridge port eth1 still has that address.
>>>>>
>>>>> - The port to which the bridge device is attached might needs a local entry
>>>>>   if its mac address is set manually.
>>>>>
>>>>>   Example of problematic case:
>>>>>     ip link set eth0 address 12:34:56:78:90:ab
>>>>>     brctl addif br0 eth0
>>>>>     ip link set br0 address 12:34:56:78:90:ab
>>>>>     ip link set eth0 address aa:bb:cc:dd:ee:ff
>>>>>   Then, the fdb still must have the entry 12:34:56:78:90:ab, but it will be
>>>>>   deleted.
>>>>>
>>>>> We can use br->dev->addr_assign_type to check if the address is manually
>>>>> set or not, but I propose another approach.
>>>>>
>>>>> Since we delete and insert local entries whenever changing mac address
>>>>> of the bridge device, we can change dst of the entry to NULL regardless of
>>>>> addr_assign_type when deleting an entry associated with a certain port,
>>>>> and if it is found to be unnecessary later, then delete it.
>>>>> That is, if changing mac address of a port, the entry might be changed
>>>>> to its dst being NULL first, but is eventually deleted when recalculating
>>>>> and changing bridge id.
>>>>>
>>>>> This approach is useful when we want to share the code with deleting
>>>>> vlan in which the bridge device might want such an entry regardless of
>>>>> addr_assign_type, and makes things easy because we don't have to consider
>>>>> if mac address of the bridge device will be changed or not at
>>>>> fdb_delete_local().
>>>>
>>>> This is a nifty approach, but it does have one side-effect that I am not
>>>> sure is correct.  In the case where bridge mac address is not manually
>>>> set, a fdb entry for the removed address survives past the
>>>> synchronize_net() call.  This would result in a behavioral change where
>>>> packets that always used to flood, would now sometimes be delivered
>>>> only to the bridge.
>>>
>>> I think no unnecessary entry will survive in any case.
>>> It will survive only if the bridge device remains to have the same mac
>>> address, because we check if the entry is necessary or not whenever the
>>> address of the bridge device is changed (assuming patch 3/7 is applied).
>>>
>>> Further analysis:
>>> The function fdb_delete_local() (and __fdb_delete_local()) will called
>>> by br_fdb_changeaddr(), br_fdb_change_mac_address(),
>>> br_fdb_delete_by_port() (with do_all==1), br_vlan_delete(), and
>>> nbp_vlan_delete() after all of this patch series are applied.
>>> So, we have to consider only these 5 functions.
>>>
>>> br_fdb_changeaddr():
>>> It is called by br_device_event().
>>> br_device_event() calls br_stp_recalculate_bridge_id() right after
>>> calling br_fdb_changeaddr(), so if the address of bridge device should
>>> be changed, the fdb entry will immediately reflect it and be deleted.
>>
>> This doesn't happen until later.  Currently nbp_del() will remove all
>> fdb entries for a given port.  With this patch, the local fdb entry for
>> the port will survive the removal process and the fdb->dst will be set
>> to NULL.
>> The port is now removed from the list, rx_handler is unregistered and we
>> push call synchronize_net() trying flush all packets currently in rcu
>> section.  Once this completes, the port and all the fdbs for it should
>> be removed, but now they are not. We have to wait for br_del_if() to
>> call the notifier call chain to remove the the fdb entry.  Any packet
>> arriving at the bridge for the mac address of the port that just got
>> removed will now be handed over to the bridge, instead of being flooded.
>> This is a change in behavior.
> 
> I see. Indeed this is a change in behavior.
> 
> But the bridge device still has the mac address after synchronize_net(),
> and it will be actually changed in br_stp_recalculate_bridge_id().
> Though that port has gone, as the bridge still uses the address, I'm
> thinking the corresponding entry should exist.

You are right in that the address is still assigned to bridge.  This is
the same exact state the we have when adding a port to the bridge.
>From br_add_if():
        err = netdev_rx_handler_register(dev, br_handle_frame, p);
        ...
        list_add_rcu(&p->list, &br->port_list);
        ....
        changed_addr = br_stp_recalculate_bridge_id(br);
        ....
        if (br_fdb_insert(br, p, dev->dev_addr, 0))

So, we add the the port to the list, set the bridge mac address and then
add the local fdb.  So, we have a short window where a mac address
assigned to the bridge does not have an associated fdb.

As I stated before, I am not sure if the side-effect you are introducing
is OK or not.  It doesn't appear to be a problem from the fdb inspection
side of things since we are holding the rtnl here.
It might cause an issue if the a host behind a port might start using
the address while the fdb is still alive.

Now the way I see it, you have 2 options:
  1) Add some text to the commit log to explain the new state
  2) Add a check for addr_type to the if statement below

> +	/* Maybe bridge device has same hw addr? */
> +	if (p && ether_addr_equal(br->dev->dev_addr, addr) &&
> +	    (!vid || br_vlan_find(br, vid))) {
> +		f->dst = NULL;
> +		return;
> +	}
> +

-vlad

> 
> Thanks,
> Toshiaki Makita
> 
>>
>> -vlad
> 
> 

Re: [PATCH net 4/7] bridge: Fix the way checking if a local fdb entry can be deleted

[Toshiaki Makita]

On Wed, 2013-12-04 at 11:40 -0500, Vlad Yasevich wrote:
> On 12/04/2013 03:29 AM, Toshiaki Makita wrote:
> > On Tue, 2013-12-03 at 10:41 -0500, Vlad Yasevich wrote:
> >> On 12/03/2013 07:45 AM, Toshiaki Makita wrote:
> >>> On Mon, 2013-12-02 at 12:07 -0500, Vlad Yasevich wrote:
> >>>> On 12/02/2013 01:40 AM, Toshiaki Makita wrote:
> >>>>> We should take into account the followings when deleting a local fdb entry.
> >>>>>
> >>>>> - nbp_vlan_find() can be used only when vid != 0 to check if an entry is
> >>>>>   deletable, because a fdb entry with vid 0 can exist at any time but
> >>>>>   nbp_vlan_find() always return false with vid 0.
> >>>>>
> >>>>>   Example of problematic case:
> >>>>>     ip link set eth0 address 12:34:56:78:90:ab
> >>>>>     ip link set eth1 address 12:34:56:78:90:ab
> >>>>>     brctl addif br0 eth0
> >>>>>     brctl addif br0 eth1
> >>>>>     ip link set eth0 address aa:bb:cc:dd:ee:ff
> >>>>>   Then, the fdb entry 12:34:56:78:90:ab will be deleted even though the
> >>>>>   bridge port eth1 still has that address.
> >>>>>
> >>>>> - The port to which the bridge device is attached might needs a local entry
> >>>>>   if its mac address is set manually.
> >>>>>
> >>>>>   Example of problematic case:
> >>>>>     ip link set eth0 address 12:34:56:78:90:ab
> >>>>>     brctl addif br0 eth0
> >>>>>     ip link set br0 address 12:34:56:78:90:ab
> >>>>>     ip link set eth0 address aa:bb:cc:dd:ee:ff
> >>>>>   Then, the fdb still must have the entry 12:34:56:78:90:ab, but it will be
> >>>>>   deleted.
> >>>>>
> >>>>> We can use br->dev->addr_assign_type to check if the address is manually
> >>>>> set or not, but I propose another approach.
> >>>>>
> >>>>> Since we delete and insert local entries whenever changing mac address
> >>>>> of the bridge device, we can change dst of the entry to NULL regardless of
> >>>>> addr_assign_type when deleting an entry associated with a certain port,
> >>>>> and if it is found to be unnecessary later, then delete it.
> >>>>> That is, if changing mac address of a port, the entry might be changed
> >>>>> to its dst being NULL first, but is eventually deleted when recalculating
> >>>>> and changing bridge id.
> >>>>>
> >>>>> This approach is useful when we want to share the code with deleting
> >>>>> vlan in which the bridge device might want such an entry regardless of
> >>>>> addr_assign_type, and makes things easy because we don't have to consider
> >>>>> if mac address of the bridge device will be changed or not at
> >>>>> fdb_delete_local().
> >>>>
> >>>> This is a nifty approach, but it does have one side-effect that I am not
> >>>> sure is correct.  In the case where bridge mac address is not manually
> >>>> set, a fdb entry for the removed address survives past the
> >>>> synchronize_net() call.  This would result in a behavioral change where
> >>>> packets that always used to flood, would now sometimes be delivered
> >>>> only to the bridge.
> >>>
> >>> I think no unnecessary entry will survive in any case.
> >>> It will survive only if the bridge device remains to have the same mac
> >>> address, because we check if the entry is necessary or not whenever the
> >>> address of the bridge device is changed (assuming patch 3/7 is applied).
> >>>
> >>> Further analysis:
> >>> The function fdb_delete_local() (and __fdb_delete_local()) will called
> >>> by br_fdb_changeaddr(), br_fdb_change_mac_address(),
> >>> br_fdb_delete_by_port() (with do_all==1), br_vlan_delete(), and
> >>> nbp_vlan_delete() after all of this patch series are applied.
> >>> So, we have to consider only these 5 functions.
> >>>
> >>> br_fdb_changeaddr():
> >>> It is called by br_device_event().
> >>> br_device_event() calls br_stp_recalculate_bridge_id() right after
> >>> calling br_fdb_changeaddr(), so if the address of bridge device should
> >>> be changed, the fdb entry will immediately reflect it and be deleted.
> >>
> >> This doesn't happen until later.  Currently nbp_del() will remove all
> >> fdb entries for a given port.  With this patch, the local fdb entry for
> >> the port will survive the removal process and the fdb->dst will be set
> >> to NULL.
> >> The port is now removed from the list, rx_handler is unregistered and we
> >> push call synchronize_net() trying flush all packets currently in rcu
> >> section.  Once this completes, the port and all the fdbs for it should
> >> be removed, but now they are not. We have to wait for br_del_if() to
> >> call the notifier call chain to remove the the fdb entry.  Any packet
> >> arriving at the bridge for the mac address of the port that just got
> >> removed will now be handed over to the bridge, instead of being flooded.
> >> This is a change in behavior.
> > 
> > I see. Indeed this is a change in behavior.
> > 
> > But the bridge device still has the mac address after synchronize_net(),
> > and it will be actually changed in br_stp_recalculate_bridge_id().
> > Though that port has gone, as the bridge still uses the address, I'm
> > thinking the corresponding entry should exist.
> 
> You are right in that the address is still assigned to bridge.  This is
> the same exact state the we have when adding a port to the bridge.
> >From br_add_if():
>         err = netdev_rx_handler_register(dev, br_handle_frame, p);
>         ...
>         list_add_rcu(&p->list, &br->port_list);
>         ....
>         changed_addr = br_stp_recalculate_bridge_id(br);
>         ....
>         if (br_fdb_insert(br, p, dev->dev_addr, 0))
> 
> So, we add the the port to the list, set the bridge mac address and then
> add the local fdb.  So, we have a short window where a mac address
> assigned to the bridge does not have an associated fdb.
> 
> As I stated before, I am not sure if the side-effect you are introducing
> is OK or not.  It doesn't appear to be a problem from the fdb inspection
> side of things since we are holding the rtnl here.
> It might cause an issue if the a host behind a port might start using
> the address while the fdb is still alive.
> 
> Now the way I see it, you have 2 options:
>   1) Add some text to the commit log to explain the new state
>   2) Add a check for addr_type to the if statement below
> 
> > +	/* Maybe bridge device has same hw addr? */
> > +	if (p && ether_addr_equal(br->dev->dev_addr, addr) &&
> > +	    (!vid || br_vlan_find(br, vid))) {
> > +		f->dst = NULL;
> > +		return;
> > +	}
> > +

I want to go for the former unless the change is absolutely not
accepted.
Introducing a check for addr_assign_type seems to be risky to me because
it might considerably make conditions complicated.

The check is not a simple check but a part of prediction of address
changing of the bridge device. We have to know how the bridge id is
calculated by other codes to implement fdb_delete_local().

For now, if fdb_delete_local() is called by br_fdb_delete_by_port(), we
will be able to expect the address change.
But once we try to use the function from *_vlan_delete(), we have to
assume that the address will not change. We will need to use another
function or introduce another argument for the function.
For the future, the logic of address changing might be modified. Fdb
code will have to reflect it properly as well.

I want to make the logic based on just current states. The last port
among those having the same address and vlan is simply responsible to
delete the entry.
I believe simplification of the logic will help us reduce unhandled
corner cases.

Anyway, thank you for your kind analysis and advice.
I will try to make proper changelog.

Thanks,
Toshiaki Makita