| Message ID | 1387281821-21342-9-git-send-email-makita.toshiaki@lab.ntt.co.jp |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On 12/17/2013 07:03 AM, Toshiaki Makita wrote: > 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> Note: All these special cases are begging for something in the fdb to track if this fdb refers to multiple ports or not. May be a list of pointers to ports? Then we can simply check to see if the list is not empty and assign to the first one in the list.. -vlad > --- > 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 bd43cb1..38cd29d 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; > @@ -766,8 +783,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 868c059..14c74c2 100644 > --- a/net/bridge/br_private.h > +++ b/net/bridge/br_private.h > @@ -379,6 +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_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); > @@ -393,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..24db71d 100644 > --- a/net/bridge/br_vlan.c > +++ b/net/bridge/br_vlan.c > @@ -296,9 +296,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; > @@ -399,9 +397,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); > } > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2013-12-17 at 14:34 -0500, Vlad Yasevich wrote: > On 12/17/2013 07:03 AM, Toshiaki Makita wrote: > > 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> > > Note: All these special cases are begging for something in the fdb > to track if this fdb refers to multiple ports or not. May be > a list of pointers to ports? Then we can simply check to see if > the list is not empty and assign to the first one in the list.. Maybe the list will simplify fdb_delete_local(). The difficult point is maintaining the list properly. I will think about it more deeply. Thanks, Toshiaki Makita -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c index bd43cb1..38cd29d 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; @@ -766,8 +783,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 868c059..14c74c2 100644 --- a/net/bridge/br_private.h +++ b/net/bridge/br_private.h @@ -379,6 +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_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); @@ -393,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..24db71d 100644 --- a/net/bridge/br_vlan.c +++ b/net/bridge/br_vlan.c @@ -296,9 +296,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; @@ -399,9 +397,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); }
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 | 20 ++++++++++++++++++-- net/bridge/br_private.h | 4 +++- net/bridge/br_vlan.c | 8 ++------ 3 files changed, 23 insertions(+), 9 deletions(-)