* [PATCH v3 net 1/9] bridge: Fix the way to find old local fdb entries in br_fdb_changeaddr
2014-02-07 7:48 [PATCH v3 net 0/9] bridge: Fix corner case problems around local fdb entries Toshiaki Makita
@ 2014-02-07 7:48 ` Toshiaki Makita
2014-02-07 7:48 ` [PATCH v3 net 2/9] bridge: Fix the way to insert new " Toshiaki Makita
` (8 subsequent siblings)
9 siblings, 0 replies; 15+ messages in thread
From: Toshiaki Makita @ 2014-02-07 7:48 UTC (permalink / raw)
To: David S . Miller, Stephen Hemminger, Vlad Yasevich, netdev
Cc: 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 not been so.
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.
Address this issue by introducing a new flag, added_by_user, to struct
net_bridge_fdb_entry.
Note that br_fdb_delete_by_port() has to set added_by_user to 0 in cases
like:
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
bridge fdb add aa:bb:cc:dd:ee:ff dev eth0 master
brctl addif br0 eth1
brctl delif br0 eth0
In this case, kernel should delete the user-added entry aa:bb:cc:dd:ee:ff,
but it also should have been added by "brctl addif br0 eth1" originally,
so we don't delete it and treat it a new kernel-created entry.
Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
net/bridge/br_fdb.c | 16 ++++++++++++----
net/bridge/br_input.c | 4 ++--
net/bridge/br_private.h | 3 ++-
3 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index c5f5a4a..ce54119 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -104,7 +104,7 @@ void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
struct net_bridge_fdb_entry *f;
f = hlist_entry(h, struct net_bridge_fdb_entry, hlist);
- if (f->dst == p && f->is_local) {
+ if (f->dst == p && f->is_local && !f->added_by_user) {
/* maybe another port has same hw addr? */
struct net_bridge_port *op;
u16 vid = f->vlan_id;
@@ -247,6 +247,7 @@ void br_fdb_delete_by_port(struct net_bridge *br,
ether_addr_equal(op->dev->dev_addr,
f->addr.addr)) {
f->dst = op;
+ f->added_by_user = 0;
goto skip_delete;
}
}
@@ -397,6 +398,7 @@ static struct net_bridge_fdb_entry *fdb_create(struct hlist_head *head,
fdb->vlan_id = vid;
fdb->is_local = 0;
fdb->is_static = 0;
+ fdb->added_by_user = 0;
fdb->updated = fdb->used = jiffies;
hlist_add_head_rcu(&fdb->hlist, head);
}
@@ -447,7 +449,7 @@ int br_fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
}
void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
- const unsigned char *addr, u16 vid)
+ const unsigned char *addr, u16 vid, bool added_by_user)
{
struct hlist_head *head = &br->hash[br_mac_hash(addr, vid)];
struct net_bridge_fdb_entry *fdb;
@@ -473,13 +475,18 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
/* fastpath: update of existing entry */
fdb->dst = source;
fdb->updated = jiffies;
+ if (unlikely(added_by_user))
+ fdb->added_by_user = 1;
}
} else {
spin_lock(&br->hash_lock);
if (likely(!fdb_find(head, addr, vid))) {
fdb = fdb_create(head, source, addr, vid);
- if (fdb)
+ if (fdb) {
+ if (unlikely(added_by_user))
+ fdb->added_by_user = 1;
fdb_notify(br, fdb, RTM_NEWNEIGH);
+ }
}
/* else we lose race and someone else inserts
* it first, don't bother updating
@@ -647,6 +654,7 @@ static int fdb_add_entry(struct net_bridge_port *source, const __u8 *addr,
modified = true;
}
+ fdb->added_by_user = 1;
fdb->used = jiffies;
if (modified) {
@@ -664,7 +672,7 @@ static int __br_fdb_add(struct ndmsg *ndm, struct net_bridge_port *p,
if (ndm->ndm_flags & NTF_USE) {
rcu_read_lock();
- br_fdb_update(p->br, p, addr, vid);
+ br_fdb_update(p->br, p, addr, vid, true);
rcu_read_unlock();
} else {
spin_lock_bh(&p->br->hash_lock);
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index bf8dc7d..28d5446 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -77,7 +77,7 @@ int br_handle_frame_finish(struct sk_buff *skb)
/* insert into forwarding database after filtering to avoid spoofing */
br = p->br;
if (p->flags & BR_LEARNING)
- br_fdb_update(br, p, eth_hdr(skb)->h_source, vid);
+ br_fdb_update(br, p, eth_hdr(skb)->h_source, vid, false);
if (!is_broadcast_ether_addr(dest) && is_multicast_ether_addr(dest) &&
br_multicast_rcv(br, p, skb, vid))
@@ -148,7 +148,7 @@ static int br_handle_local_finish(struct sk_buff *skb)
br_vlan_get_tag(skb, &vid);
if (p->flags & BR_LEARNING)
- br_fdb_update(p->br, p, eth_hdr(skb)->h_source, vid);
+ br_fdb_update(p->br, p, eth_hdr(skb)->h_source, vid, false);
return 0; /* process further */
}
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index fcd1233..939a59e 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -104,6 +104,7 @@ struct net_bridge_fdb_entry
mac_addr addr;
unsigned char is_local;
unsigned char is_static;
+ unsigned char added_by_user;
__u16 vlan_id;
};
@@ -383,7 +384,7 @@ int br_fdb_fillbuf(struct net_bridge *br, void *buf, unsigned long count,
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);
+ const unsigned char *addr, u16 vid, bool added_by_user);
int fdb_delete_by_addr(struct net_bridge *br, const u8 *addr, u16 vid);
int br_fdb_delete(struct ndmsg *ndm, struct nlattr *tb[],
--
1.8.1.2
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v3 net 2/9] bridge: Fix the way to insert new local fdb entries in br_fdb_changeaddr
2014-02-07 7:48 [PATCH v3 net 0/9] bridge: Fix corner case problems around local fdb entries Toshiaki Makita
2014-02-07 7:48 ` [PATCH v3 net 1/9] bridge: Fix the way to find old local fdb entries in br_fdb_changeaddr Toshiaki Makita
@ 2014-02-07 7:48 ` Toshiaki Makita
2014-02-07 16:31 ` Stephen Hemminger
2014-02-07 7:48 ` [PATCH v3 net 3/9] bridge: Fix the way to find old local fdb entries in br_fdb_change_mac_address Toshiaki Makita
` (7 subsequent siblings)
9 siblings, 1 reply; 15+ messages in thread
From: Toshiaki Makita @ 2014-02-07 7:48 UTC (permalink / raw)
To: David S . Miller, Stephen Hemminger, Vlad Yasevich, netdev
Cc: Toshiaki Makita
Since commit bc9a25d21ef8 ("bridge: Add vlan support for local fdb entries"),
br_fdb_changeaddr() has inserted a new local fdb entry only if it can
find old one. But if we have two ports where they have the same address
or user has deleted a local entry, there will be no entry for one of the
ports.
Example of problematic case:
ip link set eth0 address aa:bb:cc:dd:ee:ff
ip link set eth1 address aa:bb:cc:dd:ee:ff
brctl addif br0 eth0
brctl addif br0 eth1 # eth1 will not have a local entry due to dup.
ip link set eth1 address 12:34:56:78:90:ab
Then, the new entry for the address 12:34:56:78:90:ab will not be
created, and the bridge device will not be able to communicate.
Insert new entries regardless of whether we can find old entries or not.
Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
Acked-by: Vlad Yasevich <vyasevic@redhat.com>
---
net/bridge/br_fdb.c | 29 ++++++++++++++++++++---------
1 file changed, 20 insertions(+), 9 deletions(-)
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index ce54119..96ab1d1 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -92,8 +92,10 @@ static void fdb_delete(struct net_bridge *br, struct net_bridge_fdb_entry *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;
+ struct net_port_vlans *pv = nbp_get_vlan_info(p);
+ bool no_vlan = !pv;
int i;
+ u16 vid;
spin_lock_bh(&br->hash_lock);
@@ -114,28 +116,37 @@ void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
f->addr.addr) &&
nbp_vlan_find(op, vid)) {
f->dst = op;
- goto insert;
+ goto skip_delete;
}
}
/* delete old one */
fdb_delete(br, f);
-insert:
- /* insert new address, may fail if invalid
- * address or dup.
- */
- fdb_insert(br, p, newaddr, vid);
-
+skip_delete:
/* if this port has no vlan information
* configured, we can safely be done at
* this point.
*/
if (no_vlan)
- goto done;
+ goto insert;
}
}
}
+insert:
+ /* insert new address, may fail if invalid address or dup. */
+ fdb_insert(br, p, newaddr, 0);
+
+ if (no_vlan)
+ goto done;
+
+ /* Now add entries for every VLAN configured on the port.
+ * This function runs under RTNL so the bitmap will not change
+ * from under us.
+ */
+ for_each_set_bit(vid, pv->vlan_bitmap, VLAN_N_VID)
+ fdb_insert(br, p, newaddr, vid);
+
done:
spin_unlock_bh(&br->hash_lock);
}
--
1.8.1.2
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v3 net 2/9] bridge: Fix the way to insert new local fdb entries in br_fdb_changeaddr
2014-02-07 7:48 ` [PATCH v3 net 2/9] bridge: Fix the way to insert new " Toshiaki Makita
@ 2014-02-07 16:31 ` Stephen Hemminger
2014-02-08 2:43 ` Toshiaki Makita
0 siblings, 1 reply; 15+ messages in thread
From: Stephen Hemminger @ 2014-02-07 16:31 UTC (permalink / raw)
To: Toshiaki Makita; +Cc: David S . Miller, Vlad Yasevich, netdev
On Fri, 7 Feb 2014 16:48:19 +0900
Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> wrote:
> Since commit bc9a25d21ef8 ("bridge: Add vlan support for local fdb entries"),
> br_fdb_changeaddr() has inserted a new local fdb entry only if it can
> find old one. But if we have two ports where they have the same address
> or user has deleted a local entry, there will be no entry for one of the
> ports.
>
> Example of problematic case:
> ip link set eth0 address aa:bb:cc:dd:ee:ff
> ip link set eth1 address aa:bb:cc:dd:ee:ff
> brctl addif br0 eth0
> brctl addif br0 eth1 # eth1 will not have a local entry due to dup.
I think the second addif should fail, it doesn't seem valid to have
two interfaces on same bridge with same address. Most hardware switches
would disable the port in that case.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v3 net 2/9] bridge: Fix the way to insert new local fdb entries in br_fdb_changeaddr
2014-02-07 16:31 ` Stephen Hemminger
@ 2014-02-08 2:43 ` Toshiaki Makita
0 siblings, 0 replies; 15+ messages in thread
From: Toshiaki Makita @ 2014-02-08 2:43 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Toshiaki Makita, David S . Miller, Vlad Yasevich, netdev
On Fri, 2014-02-07 at 09:31 -0700, Stephen Hemminger wrote:
> On Fri, 7 Feb 2014 16:48:19 +0900
> Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> wrote:
>
> > Since commit bc9a25d21ef8 ("bridge: Add vlan support for local fdb entries"),
> > br_fdb_changeaddr() has inserted a new local fdb entry only if it can
> > find old one. But if we have two ports where they have the same address
> > or user has deleted a local entry, there will be no entry for one of the
> > ports.
> >
> > Example of problematic case:
> > ip link set eth0 address aa:bb:cc:dd:ee:ff
> > ip link set eth1 address aa:bb:cc:dd:ee:ff
> > brctl addif br0 eth0
> > brctl addif br0 eth1 # eth1 will not have a local entry due to dup.
>
> I think the second addif should fail, it doesn't seem valid to have
> two interfaces on same bridge with same address. Most hardware switches
> would disable the port in that case.
Thank you for your comment, but I don't think so for several reasons.
- From other network elements on the same network, bridge ports don't
appear to have a mac address, but the bridge appears to have several mac
addresses that can reach to the bridge. The duplicated address is simply
seen as one of those addresses. I don't think it is a problem.
- This operation (add a port that has duplicated address) has allowed
for several years, and it is obviously intended, as commented in
fdb_insert().
417 /* it is okay to have multiple ports with same
418 * address, just use the first one.
419 */
- Hardware switches usually have one mac address per one switch. Their
ports don't have mac addresses. It is not reasonable to compare hardware
switches.
Thanks,
Toshiaki Makita
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 net 3/9] bridge: Fix the way to find old local fdb entries in br_fdb_change_mac_address
2014-02-07 7:48 [PATCH v3 net 0/9] bridge: Fix corner case problems around local fdb entries Toshiaki Makita
2014-02-07 7:48 ` [PATCH v3 net 1/9] bridge: Fix the way to find old local fdb entries in br_fdb_changeaddr Toshiaki Makita
2014-02-07 7:48 ` [PATCH v3 net 2/9] bridge: Fix the way to insert new " Toshiaki Makita
@ 2014-02-07 7:48 ` Toshiaki Makita
2014-02-07 7:48 ` [PATCH v3 net 4/9] bridge: Change local fdb entries whenever mac address of bridge device changes Toshiaki Makita
` (6 subsequent siblings)
9 siblings, 0 replies; 15+ messages in thread
From: Toshiaki Makita @ 2014-02-07 7:48 UTC (permalink / raw)
To: David S . Miller, Stephen Hemminger, Vlad Yasevich, netdev
Cc: 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>
Acked-by: Vlad Yasevich <vyasevic@redhat.com>
---
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 e4401a5..5f655b6 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
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v3 net 4/9] bridge: Change local fdb entries whenever mac address of bridge device changes
2014-02-07 7:48 [PATCH v3 net 0/9] bridge: Fix corner case problems around local fdb entries Toshiaki Makita
` (2 preceding siblings ...)
2014-02-07 7:48 ` [PATCH v3 net 3/9] bridge: Fix the way to find old local fdb entries in br_fdb_change_mac_address Toshiaki Makita
@ 2014-02-07 7:48 ` Toshiaki Makita
2014-02-07 7:48 ` [PATCH v3 net 5/9] bridge: Fix the way to check if a local fdb entry can be deleted Toshiaki Makita
` (5 subsequent siblings)
9 siblings, 0 replies; 15+ messages in thread
From: Toshiaki Makita @ 2014-02-07 7:48 UTC (permalink / raw)
To: David S . Miller, Stephen Hemminger, Vlad Yasevich, netdev
Cc: 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.
This approach also needs br_add_if() to call br_fdb_insert() before
br_stp_recalculate_bridge_id() so that we don't create an entry whose
dst == NULL in this function to preserve previous behavior.
Note that this is a slight change in behavior where the bridge device can
receive the traffic to the new address before calling
br_stp_recalculate_bridge_id() in br_add_if().
However, it is not a problem because we have already the address on the
new port and such a way to insert new one before recalculating bridge id
is taken in br_device_event() as well.
Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
Acked-by: Vlad Yasevich <vyasevic@redhat.com>
---
net/bridge/br_device.c | 1 -
net/bridge/br_if.c | 6 +++---
net/bridge/br_stp_if.c | 2 ++
3 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 5f655b6..ac48940 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_if.c b/net/bridge/br_if.c
index cffe1d6..54d207d 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -389,6 +389,9 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
if (br->dev->needed_headroom < dev->needed_headroom)
br->dev->needed_headroom = dev->needed_headroom;
+ if (br_fdb_insert(br, p, dev->dev_addr, 0))
+ netdev_err(dev, "failed insert local address bridge forwarding table\n");
+
spin_lock_bh(&br->lock);
changed_addr = br_stp_recalculate_bridge_id(br);
@@ -404,9 +407,6 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
dev_set_mtu(br->dev, br_min_mtu(br));
- if (br_fdb_insert(br, p, dev->dev_addr, 0))
- netdev_err(dev, "failed insert local address bridge forwarding table\n");
-
kobject_uevent(&p->kobj, KOBJ_ADD);
return 0;
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v3 net 5/9] bridge: Fix the way to check if a local fdb entry can be deleted
2014-02-07 7:48 [PATCH v3 net 0/9] bridge: Fix corner case problems around local fdb entries Toshiaki Makita
` (3 preceding siblings ...)
2014-02-07 7:48 ` [PATCH v3 net 4/9] bridge: Change local fdb entries whenever mac address of bridge device changes Toshiaki Makita
@ 2014-02-07 7:48 ` Toshiaki Makita
2014-02-10 17:22 ` Vlad Yasevich
2014-02-07 7:48 ` [PATCH v3 net 6/9] bridge: Properly check if local fdb entry can be deleted in br_fdb_change_mac_address Toshiaki Makita
` (4 subsequent siblings)
9 siblings, 1 reply; 15+ messages in thread
From: Toshiaki Makita @ 2014-02-07 7:48 UTC (permalink / raw)
To: David S . Miller, Stephen Hemminger, Vlad Yasevich, netdev
Cc: 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 while
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 especially 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 the time we
delete a local entry of a port, which means fdb code will not be bothered
even if the bridge id calculating logic is changed in the future.
Also, this change reduces inconsistent state, where frames whose dst is the
mac address of the bridge, can't reach the bridge because of premature fdb
entry deletion. This change reduces the possibility that the bridge device
replies unreachable mac address to arp requests, which could occur during
the short window between calling del_nbp() and br_stp_recalculate_bridge_id()
in br_del_if(). This will effective after br_fdb_delete_by_port() starts to
use the same code by following patch.
Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
net/bridge/br_fdb.c | 10 +++++++++-
net/bridge/br_private.h | 6 ++++++
net/bridge/br_vlan.c | 19 +++++++++++++++++++
3 files changed, 34 insertions(+), 1 deletion(-)
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 96ab1d1..b4005f5 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -114,12 +114,20 @@ void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
if (op != p &&
ether_addr_equal(op->dev->dev_addr,
f->addr.addr) &&
- nbp_vlan_find(op, vid)) {
+ (!vid || nbp_vlan_find(op, vid))) {
f->dst = op;
goto skip_delete;
}
}
+ /* maybe bridge device has same hw addr? */
+ if (ether_addr_equal(br->dev->dev_addr,
+ f->addr.addr) &&
+ (!vid || br_vlan_find(br, vid))) {
+ f->dst = NULL;
+ goto skip_delete;
+ }
+
/* delete old one */
fdb_delete(br, f);
skip_delete:
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 939a59e..f91e1d9 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -585,6 +585,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);
@@ -666,6 +667,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 4ca4d0a..233ec1c 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -295,6 +295,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
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v3 net 5/9] bridge: Fix the way to check if a local fdb entry can be deleted
2014-02-07 7:48 ` [PATCH v3 net 5/9] bridge: Fix the way to check if a local fdb entry can be deleted Toshiaki Makita
@ 2014-02-10 17:22 ` Vlad Yasevich
0 siblings, 0 replies; 15+ messages in thread
From: Vlad Yasevich @ 2014-02-10 17:22 UTC (permalink / raw)
To: Toshiaki Makita, David S . Miller, Stephen Hemminger, netdev
On 02/07/2014 02:48 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 while
> 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 especially 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 the time we
> delete a local entry of a port, which means fdb code will not be bothered
> even if the bridge id calculating logic is changed in the future.
>
> Also, this change reduces inconsistent state, where frames whose dst is the
> mac address of the bridge, can't reach the bridge because of premature fdb
> entry deletion. This change reduces the possibility that the bridge device
> replies unreachable mac address to arp requests, which could occur during
> the short window between calling del_nbp() and br_stp_recalculate_bridge_id()
> in br_del_if(). This will effective after br_fdb_delete_by_port() starts to
> use the same code by following patch.
>
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
Acked-by: Vlad Yasevich <vyasevic@redhat.com>
-vlad
> ---
> net/bridge/br_fdb.c | 10 +++++++++-
> net/bridge/br_private.h | 6 ++++++
> net/bridge/br_vlan.c | 19 +++++++++++++++++++
> 3 files changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index 96ab1d1..b4005f5 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -114,12 +114,20 @@ void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
> if (op != p &&
> ether_addr_equal(op->dev->dev_addr,
> f->addr.addr) &&
> - nbp_vlan_find(op, vid)) {
> + (!vid || nbp_vlan_find(op, vid))) {
> f->dst = op;
> goto skip_delete;
> }
> }
>
> + /* maybe bridge device has same hw addr? */
> + if (ether_addr_equal(br->dev->dev_addr,
> + f->addr.addr) &&
> + (!vid || br_vlan_find(br, vid))) {
> + f->dst = NULL;
> + goto skip_delete;
> + }
> +
> /* delete old one */
> fdb_delete(br, f);
> skip_delete:
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 939a59e..f91e1d9 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -585,6 +585,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);
> @@ -666,6 +667,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 4ca4d0a..233ec1c 100644
> --- a/net/bridge/br_vlan.c
> +++ b/net/bridge/br_vlan.c
> @@ -295,6 +295,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())
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 net 6/9] bridge: Properly check if local fdb entry can be deleted in br_fdb_change_mac_address
2014-02-07 7:48 [PATCH v3 net 0/9] bridge: Fix corner case problems around local fdb entries Toshiaki Makita
` (4 preceding siblings ...)
2014-02-07 7:48 ` [PATCH v3 net 5/9] bridge: Fix the way to check if a local fdb entry can be deleted Toshiaki Makita
@ 2014-02-07 7:48 ` Toshiaki Makita
2014-02-07 7:48 ` [PATCH v3 net 7/9] bridge: Properly check if local fdb entry can be deleted in br_fdb_delete_by_port Toshiaki Makita
` (3 subsequent siblings)
9 siblings, 0 replies; 15+ messages in thread
From: Toshiaki Makita @ 2014-02-07 7:48 UTC (permalink / raw)
To: David S . Miller, Stephen Hemminger, Vlad Yasevich, netdev
Cc: 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 being deleted is identical to that of any port.
As this check is almost the same as that in br_fdb_changeaddr(), create
a common function fdb_delete_local() and call it from
br_fdb_changeadddr() and br_fdb_change_mac_address().
Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
Acked-by: Vlad Yasevich <vyasevic@redhat.com>
---
net/bridge/br_fdb.c | 57 ++++++++++++++++++++++++++++++-----------------------
1 file changed, 32 insertions(+), 25 deletions(-)
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index b4005f5..15bf13d 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -89,6 +89,34 @@ 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,
+ struct net_bridge_fdb_entry *f)
+{
+ const unsigned char *addr = f->addr.addr;
+ u16 vid = f->vlan_id;
+ struct net_bridge_port *op;
+
+ /* 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) &&
+ (!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);
+}
+
void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
{
struct net_bridge *br = p->br;
@@ -107,30 +135,9 @@ void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
f = hlist_entry(h, struct net_bridge_fdb_entry, hlist);
if (f->dst == p && f->is_local && !f->added_by_user) {
- /* 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) &&
- (!vid || nbp_vlan_find(op, vid))) {
- f->dst = op;
- goto skip_delete;
- }
- }
-
- /* maybe bridge device has same hw addr? */
- if (ether_addr_equal(br->dev->dev_addr,
- f->addr.addr) &&
- (!vid || br_vlan_find(br, vid))) {
- f->dst = NULL;
- goto skip_delete;
- }
-
/* delete old one */
- fdb_delete(br, f);
-skip_delete:
+ fdb_delete_local(br, p, f);
+
/* if this port has no vlan information
* configured, we can safely be done at
* this point.
@@ -168,7 +175,7 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
/* 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_delete_local(br, NULL, f);
fdb_insert(br, NULL, newaddr, 0);
@@ -183,7 +190,7 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
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_delete_local(br, NULL, f);
fdb_insert(br, NULL, newaddr, vid);
}
}
--
1.8.1.2
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v3 net 7/9] bridge: Properly check if local fdb entry can be deleted in br_fdb_delete_by_port
2014-02-07 7:48 [PATCH v3 net 0/9] bridge: Fix corner case problems around local fdb entries Toshiaki Makita
` (5 preceding siblings ...)
2014-02-07 7:48 ` [PATCH v3 net 6/9] bridge: Properly check if local fdb entry can be deleted in br_fdb_change_mac_address Toshiaki Makita
@ 2014-02-07 7:48 ` Toshiaki Makita
2014-02-10 17:37 ` Vlad Yasevich
2014-02-07 7:48 ` [PATCH v3 net 8/9] bridge: Properly check if local fdb entry can be deleted when deleting vlan Toshiaki Makita
` (2 subsequent siblings)
9 siblings, 1 reply; 15+ messages in thread
From: Toshiaki Makita @ 2014-02-07 7:48 UTC (permalink / raw)
To: David S . Miller, Stephen Hemminger, Vlad Yasevich, netdev
Cc: Toshiaki Makita
br_fdb_delete_by_port() doesn't care about vlan and mac address of the
bridge device.
As the check is almost the same as mac address changing, slightly modify
fdb_delete_local() and use it.
Note that we can always set added_by_user to 0 in fdb_delete_local() because
- br_fdb_delete_by_port() calls fdb_delete_local() for local entries
regardless of its added_by_user. In this case, we have to check if another
port has the same address and vlan, and if found, we have to create the
entry (by changing dst). This is kernel-added entry, not user-added.
- br_fdb_changeaddr() doesn't call fdb_delete_local() for user-added entry.
Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
net/bridge/br_fdb.c | 25 ++++++-------------------
1 file changed, 6 insertions(+), 19 deletions(-)
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 15bf13d..43e54d1 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -103,6 +103,7 @@ static void fdb_delete_local(struct net_bridge *br,
if (op != p && ether_addr_equal(op->dev->dev_addr, addr) &&
(!vid || nbp_vlan_find(op, vid))) {
f->dst = op;
+ f->added_by_user = 0;
return;
}
}
@@ -111,6 +112,7 @@ static void fdb_delete_local(struct net_bridge *br,
if (p && ether_addr_equal(br->dev->dev_addr, addr) &&
(!vid || br_vlan_find(br, vid))) {
f->dst = NULL;
+ f->added_by_user = 0;
return;
}
@@ -261,26 +263,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;
- f->added_by_user = 0;
- 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
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v3 net 7/9] bridge: Properly check if local fdb entry can be deleted in br_fdb_delete_by_port
2014-02-07 7:48 ` [PATCH v3 net 7/9] bridge: Properly check if local fdb entry can be deleted in br_fdb_delete_by_port Toshiaki Makita
@ 2014-02-10 17:37 ` Vlad Yasevich
0 siblings, 0 replies; 15+ messages in thread
From: Vlad Yasevich @ 2014-02-10 17:37 UTC (permalink / raw)
To: Toshiaki Makita, David S . Miller, Stephen Hemminger, netdev
On 02/07/2014 02:48 AM, Toshiaki Makita wrote:
> br_fdb_delete_by_port() doesn't care about vlan and mac address of the
> bridge device.
>
> As the check is almost the same as mac address changing, slightly modify
> fdb_delete_local() and use it.
>
> Note that we can always set added_by_user to 0 in fdb_delete_local() because
> - br_fdb_delete_by_port() calls fdb_delete_local() for local entries
> regardless of its added_by_user. In this case, we have to check if another
> port has the same address and vlan, and if found, we have to create the
> entry (by changing dst). This is kernel-added entry, not user-added.
> - br_fdb_changeaddr() doesn't call fdb_delete_local() for user-added entry.
>
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
Acked-by: Vlad Yasevich <vyasevic@redhat.com>
-vlad
> ---
> net/bridge/br_fdb.c | 25 ++++++-------------------
> 1 file changed, 6 insertions(+), 19 deletions(-)
>
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index 15bf13d..43e54d1 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -103,6 +103,7 @@ static void fdb_delete_local(struct net_bridge *br,
> if (op != p && ether_addr_equal(op->dev->dev_addr, addr) &&
> (!vid || nbp_vlan_find(op, vid))) {
> f->dst = op;
> + f->added_by_user = 0;
> return;
> }
> }
> @@ -111,6 +112,7 @@ static void fdb_delete_local(struct net_bridge *br,
> if (p && ether_addr_equal(br->dev->dev_addr, addr) &&
> (!vid || br_vlan_find(br, vid))) {
> f->dst = NULL;
> + f->added_by_user = 0;
> return;
> }
>
> @@ -261,26 +263,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;
> - f->added_by_user = 0;
> - 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);
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 net 8/9] bridge: Properly check if local fdb entry can be deleted when deleting vlan
2014-02-07 7:48 [PATCH v3 net 0/9] bridge: Fix corner case problems around local fdb entries Toshiaki Makita
` (6 preceding siblings ...)
2014-02-07 7:48 ` [PATCH v3 net 7/9] bridge: Properly check if local fdb entry can be deleted in br_fdb_delete_by_port Toshiaki Makita
@ 2014-02-07 7:48 ` Toshiaki Makita
2014-02-07 7:48 ` [PATCH v3 net 9/9] bridge: Prevent possible race condition in br_fdb_change_mac_address Toshiaki Makita
2014-02-10 22:36 ` [PATCH v3 net 0/9] bridge: Fix corner case problems around local fdb entries David Miller
9 siblings, 0 replies; 15+ messages in thread
From: Toshiaki Makita @ 2014-02-07 7:48 UTC (permalink / raw)
To: David S . Miller, Stephen Hemminger, Vlad Yasevich, netdev
Cc: 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>
Acked-by: Vlad Yasevich <vyasevic@redhat.com>
---
net/bridge/br_fdb.c | 20 ++++++++++++++++++--
net/bridge/br_private.h | 4 +++-
net/bridge/br_vlan.c | 8 ++------
3 files changed, 23 insertions(+), 9 deletions(-)
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 43e54d1..0426dff 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,
@@ -119,6 +122,20 @@ static void fdb_delete_local(struct net_bridge *br,
fdb_delete(br, f);
}
+void br_fdb_find_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;
+
+ spin_lock_bh(&br->hash_lock);
+ f = fdb_find(head, addr, vid);
+ if (f && f->is_local && !f->added_by_user && f->dst == p)
+ fdb_delete_local(br, p, f);
+ spin_unlock_bh(&br->hash_lock);
+}
+
void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
{
struct net_bridge *br = p->br;
@@ -770,8 +787,7 @@ 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 f91e1d9..3ba11bc 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -371,6 +371,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_find_delete_local(struct net_bridge *br,
+ const struct net_bridge_port *p,
+ const unsigned char *addr, u16 vid);
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_cleanup(unsigned long arg);
@@ -385,7 +388,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, bool added_by_user);
-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 233ec1c..8249ca7 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -275,9 +275,7 @@ int br_vlan_delete(struct net_bridge *br, u16 vid)
if (!pv)
return -EINVAL;
- spin_lock_bh(&br->hash_lock);
- fdb_delete_by_addr(br, br->dev->dev_addr, vid);
- spin_unlock_bh(&br->hash_lock);
+ br_fdb_find_delete_local(br, NULL, br->dev->dev_addr, vid);
__vlan_del(pv, vid);
return 0;
@@ -378,9 +376,7 @@ int nbp_vlan_delete(struct net_bridge_port *port, u16 vid)
if (!pv)
return -EINVAL;
- spin_lock_bh(&port->br->hash_lock);
- fdb_delete_by_addr(port->br, port->dev->dev_addr, vid);
- spin_unlock_bh(&port->br->hash_lock);
+ br_fdb_find_delete_local(port->br, port, port->dev->dev_addr, vid);
return __vlan_del(pv, vid);
}
--
1.8.1.2
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v3 net 9/9] bridge: Prevent possible race condition in br_fdb_change_mac_address
2014-02-07 7:48 [PATCH v3 net 0/9] bridge: Fix corner case problems around local fdb entries Toshiaki Makita
` (7 preceding siblings ...)
2014-02-07 7:48 ` [PATCH v3 net 8/9] bridge: Properly check if local fdb entry can be deleted when deleting vlan Toshiaki Makita
@ 2014-02-07 7:48 ` Toshiaki Makita
2014-02-10 22:36 ` [PATCH v3 net 0/9] bridge: Fix corner case problems around local fdb entries David Miller
9 siblings, 0 replies; 15+ messages in thread
From: Toshiaki Makita @ 2014-02-07 7:48 UTC (permalink / raw)
To: David S . Miller, Stephen Hemminger, Vlad Yasevich, netdev
Cc: Toshiaki Makita
br_fdb_change_mac_address() calls fdb_insert()/fdb_delete() without
br->hash_lock.
These hash list updates are racy with br_fdb_update()/br_fdb_cleanup().
Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
Acked-by: Vlad Yasevich <vyasevic@redhat.com>
---
net/bridge/br_fdb.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 0426dff..9203d5a 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -191,6 +191,8 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
struct net_port_vlans *pv;
u16 vid = 0;
+ spin_lock_bh(&br->hash_lock);
+
/* 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)
@@ -204,7 +206,7 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
*/
pv = br_get_vlan_info(br);
if (!pv)
- return;
+ goto out;
for_each_set_bit_from(vid, pv->vlan_bitmap, VLAN_N_VID) {
f = __br_fdb_get(br, br->dev->dev_addr, vid);
@@ -212,6 +214,8 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
fdb_delete_local(br, NULL, f);
fdb_insert(br, NULL, newaddr, vid);
}
+out:
+ spin_unlock_bh(&br->hash_lock);
}
void br_fdb_cleanup(unsigned long _data)
--
1.8.1.2
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v3 net 0/9] bridge: Fix corner case problems around local fdb entries
2014-02-07 7:48 [PATCH v3 net 0/9] bridge: Fix corner case problems around local fdb entries Toshiaki Makita
` (8 preceding siblings ...)
2014-02-07 7:48 ` [PATCH v3 net 9/9] bridge: Prevent possible race condition in br_fdb_change_mac_address Toshiaki Makita
@ 2014-02-10 22:36 ` David Miller
9 siblings, 0 replies; 15+ messages in thread
From: David Miller @ 2014-02-10 22:36 UTC (permalink / raw)
To: makita.toshiaki; +Cc: stephen, vyasevic, netdev
From: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
Date: Fri, 7 Feb 2014 16:48:17 +0900
> 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.
Series applied, thanks.
^ permalink raw reply [flat|nested] 15+ messages in thread