| Message ID | 1435062496-14596-1-git-send-email-nikolay@cumulusnetworks.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On 15/06/23 (火) 21:28, Nikolay Aleksandrov wrote: > Add a new argument to br_fdb_delete_by_port which allows to specify a > vid to match when flushing entries and use it in nbp_vlan_delete() to > flush the dynamically learned entries of the vlan/port pair when removing > a vlan from a port. Before this patch only the local mac was being > removed and the dynamically learned ones were left to expire. > Note that the do_all argument is still respected and if specified, the > vid will be ignored. > > Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> > --- > note: There are 2 warnings, the C99 comment was already there and the 81 > char line is the prototype and I think it looks better this way. > One unclear thing is about vid 0, I see that it can be passed to > nbp_vlan_(add|delete) although the comment says it shouldn't. nbp_vlan_add() should not be called with vid 0 because vid 0 of fdb entries is internally used when vlan_filtering is disabled, and such entries are exposed to userspace as those without any vlan id. bridge's vlan_filtering is respecting 802.1Q-2011 spec, which says vid 0 cannot be used as a normal vlan id. If vlan 0 can be added, it should be a bug. 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
> On Jun 23, 2015, at 6:02 PM, Toshiaki Makita <toshiaki.makita1@gmail.com> wrote: > > On 15/06/23 (火) 21:28, Nikolay Aleksandrov wrote: >> Add a new argument to br_fdb_delete_by_port which allows to specify a >> vid to match when flushing entries and use it in nbp_vlan_delete() to >> flush the dynamically learned entries of the vlan/port pair when removing >> a vlan from a port. Before this patch only the local mac was being >> removed and the dynamically learned ones were left to expire. >> Note that the do_all argument is still respected and if specified, the >> vid will be ignored. >> >> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> >> --- >> note: There are 2 warnings, the C99 comment was already there and the 81 >> char line is the prototype and I think it looks better this way. >> One unclear thing is about vid 0, I see that it can be passed to >> nbp_vlan_(add|delete) although the comment says it shouldn't. > > nbp_vlan_add() should not be called with vid 0 because vid 0 of fdb > entries is internally used when vlan_filtering is disabled, and such > entries are exposed to userspace as those without any vlan id. > bridge's vlan_filtering is respecting 802.1Q-2011 spec, which says vid 0 > cannot be used as a normal vlan id. > If vlan 0 can be added, it should be a bug. > > Toshiaki Makita Right, that is my understanding as well. I’ll look into this and post a fix if necessary. Thanks, Nik-- 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 06/23/2015 08:28 AM, Nikolay Aleksandrov wrote: > Add a new argument to br_fdb_delete_by_port which allows to specify a > vid to match when flushing entries and use it in nbp_vlan_delete() to > flush the dynamically learned entries of the vlan/port pair when removing > a vlan from a port. Before this patch only the local mac was being > removed and the dynamically learned ones were left to expire. > Note that the do_all argument is still respected and if specified, the > vid will be ignored. > > Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> > --- > note: There are 2 warnings, the C99 comment was already there and the 81 > char line is the prototype and I think it looks better this way. > One unclear thing is about vid 0, I see that it can be passed to > nbp_vlan_(add|delete) although the comment says it shouldn't. > Adding Vlad to CC list since he has introduced the vlan filtering > infrastructure. sorry, missed this one somewhow... There is no explicit check inside the nbp_vlan_[add|del] because most checks are at higher level functions. It should be impossible to call it with vid of 0. This code looks fine to me. Acked-by: Vlad Yasevich <vyasevich@gmail.com> -vlad > > net/bridge/br_fdb.c | 7 +++++-- > net/bridge/br_if.c | 4 ++-- > net/bridge/br_private.h | 2 +- > net/bridge/br_stp_if.c | 2 +- > net/bridge/br_sysfs_if.c | 2 +- > net/bridge/br_vlan.c | 1 + > 6 files changed, 11 insertions(+), 7 deletions(-) > > diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c > index be84b7e5a3da..9e9875da0a4f 100644 > --- a/net/bridge/br_fdb.c > +++ b/net/bridge/br_fdb.c > @@ -330,9 +330,11 @@ void br_fdb_flush(struct net_bridge *br) > > /* Flush all entries referring to a specific port. > * if do_all is set also flush static entries > + * if vid is set delete all entries that match the vlan_id > */ > void br_fdb_delete_by_port(struct net_bridge *br, > const struct net_bridge_port *p, > + u16 vid, > int do_all) > { > int i; > @@ -347,8 +349,9 @@ void br_fdb_delete_by_port(struct net_bridge *br, > if (f->dst != p) > continue; > > - if (f->is_static && !do_all) > - continue; > + if (!do_all) > + if (f->is_static || (vid && f->vlan_id != vid)) > + continue; > > if (f->is_local) > fdb_delete_local(br, p, f); > diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c > index 1849d96b3c91..a538cb1199a3 100644 > --- a/net/bridge/br_if.c > +++ b/net/bridge/br_if.c > @@ -249,7 +249,7 @@ static void del_nbp(struct net_bridge_port *p) > list_del_rcu(&p->list); > > nbp_vlan_flush(p); > - br_fdb_delete_by_port(br, p, 1); > + br_fdb_delete_by_port(br, p, 0, 1); > nbp_update_port_count(br); > > netdev_upper_dev_unlink(dev, br->dev); > @@ -278,7 +278,7 @@ void br_dev_delete(struct net_device *dev, struct list_head *head) > del_nbp(p); > } > > - br_fdb_delete_by_port(br, NULL, 1); > + br_fdb_delete_by_port(br, NULL, 0, 1); > > br_vlan_flush(br); > del_timer_sync(&br->gc_timer); > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h > index 5dccced71269..8b21146b24a0 100644 > --- a/net/bridge/br_private.h > +++ b/net/bridge/br_private.h > @@ -387,7 +387,7 @@ 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); > void br_fdb_delete_by_port(struct net_bridge *br, > - const struct net_bridge_port *p, int do_all); > + const struct net_bridge_port *p, u16 vid, int do_all); > struct net_bridge_fdb_entry *__br_fdb_get(struct net_bridge *br, > const unsigned char *addr, __u16 vid); > int br_fdb_test_addr(struct net_device *dev, unsigned char *addr); > diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c > index 41146872c1b4..762d490a16f2 100644 > --- a/net/bridge/br_stp_if.c > +++ b/net/bridge/br_stp_if.c > @@ -111,7 +111,7 @@ void br_stp_disable_port(struct net_bridge_port *p) > del_timer(&p->forward_delay_timer); > del_timer(&p->hold_timer); > > - br_fdb_delete_by_port(br, p, 0); > + br_fdb_delete_by_port(br, p, 0, 0); > br_multicast_disable_port(p); > > br_configuration_update(br); > diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c > index 4905845a94e9..efe415ad842a 100644 > --- a/net/bridge/br_sysfs_if.c > +++ b/net/bridge/br_sysfs_if.c > @@ -160,7 +160,7 @@ static BRPORT_ATTR(hold_timer, S_IRUGO, show_hold_timer, NULL); > > static int store_flush(struct net_bridge_port *p, unsigned long v) > { > - br_fdb_delete_by_port(p->br, p, 0); // Don't delete local entry > + br_fdb_delete_by_port(p->br, p, 0, 0); // Don't delete local entry > return 0; > } > static BRPORT_ATTR(flush, S_IWUSR, NULL, store_flush); > diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c > index 17fc358a5432..9e1b9c1b4880 100644 > --- a/net/bridge/br_vlan.c > +++ b/net/bridge/br_vlan.c > @@ -741,6 +741,7 @@ int nbp_vlan_delete(struct net_bridge_port *port, u16 vid) > return -EINVAL; > > br_fdb_find_delete_local(port->br, port, port->dev->dev_addr, vid); > + br_fdb_delete_by_port(port->br, port, vid, 0); > > 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
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c index be84b7e5a3da..9e9875da0a4f 100644 --- a/net/bridge/br_fdb.c +++ b/net/bridge/br_fdb.c @@ -330,9 +330,11 @@ void br_fdb_flush(struct net_bridge *br) /* Flush all entries referring to a specific port. * if do_all is set also flush static entries + * if vid is set delete all entries that match the vlan_id */ void br_fdb_delete_by_port(struct net_bridge *br, const struct net_bridge_port *p, + u16 vid, int do_all) { int i; @@ -347,8 +349,9 @@ void br_fdb_delete_by_port(struct net_bridge *br, if (f->dst != p) continue; - if (f->is_static && !do_all) - continue; + if (!do_all) + if (f->is_static || (vid && f->vlan_id != vid)) + continue; if (f->is_local) fdb_delete_local(br, p, f); diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c index 1849d96b3c91..a538cb1199a3 100644 --- a/net/bridge/br_if.c +++ b/net/bridge/br_if.c @@ -249,7 +249,7 @@ static void del_nbp(struct net_bridge_port *p) list_del_rcu(&p->list); nbp_vlan_flush(p); - br_fdb_delete_by_port(br, p, 1); + br_fdb_delete_by_port(br, p, 0, 1); nbp_update_port_count(br); netdev_upper_dev_unlink(dev, br->dev); @@ -278,7 +278,7 @@ void br_dev_delete(struct net_device *dev, struct list_head *head) del_nbp(p); } - br_fdb_delete_by_port(br, NULL, 1); + br_fdb_delete_by_port(br, NULL, 0, 1); br_vlan_flush(br); del_timer_sync(&br->gc_timer); diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h index 5dccced71269..8b21146b24a0 100644 --- a/net/bridge/br_private.h +++ b/net/bridge/br_private.h @@ -387,7 +387,7 @@ 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); void br_fdb_delete_by_port(struct net_bridge *br, - const struct net_bridge_port *p, int do_all); + const struct net_bridge_port *p, u16 vid, int do_all); struct net_bridge_fdb_entry *__br_fdb_get(struct net_bridge *br, const unsigned char *addr, __u16 vid); int br_fdb_test_addr(struct net_device *dev, unsigned char *addr); diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c index 41146872c1b4..762d490a16f2 100644 --- a/net/bridge/br_stp_if.c +++ b/net/bridge/br_stp_if.c @@ -111,7 +111,7 @@ void br_stp_disable_port(struct net_bridge_port *p) del_timer(&p->forward_delay_timer); del_timer(&p->hold_timer); - br_fdb_delete_by_port(br, p, 0); + br_fdb_delete_by_port(br, p, 0, 0); br_multicast_disable_port(p); br_configuration_update(br); diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c index 4905845a94e9..efe415ad842a 100644 --- a/net/bridge/br_sysfs_if.c +++ b/net/bridge/br_sysfs_if.c @@ -160,7 +160,7 @@ static BRPORT_ATTR(hold_timer, S_IRUGO, show_hold_timer, NULL); static int store_flush(struct net_bridge_port *p, unsigned long v) { - br_fdb_delete_by_port(p->br, p, 0); // Don't delete local entry + br_fdb_delete_by_port(p->br, p, 0, 0); // Don't delete local entry return 0; } static BRPORT_ATTR(flush, S_IWUSR, NULL, store_flush); diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c index 17fc358a5432..9e1b9c1b4880 100644 --- a/net/bridge/br_vlan.c +++ b/net/bridge/br_vlan.c @@ -741,6 +741,7 @@ int nbp_vlan_delete(struct net_bridge_port *port, u16 vid) return -EINVAL; br_fdb_find_delete_local(port->br, port, port->dev->dev_addr, vid); + br_fdb_delete_by_port(port->br, port, vid, 0); return __vlan_del(pv, vid); }
Add a new argument to br_fdb_delete_by_port which allows to specify a vid to match when flushing entries and use it in nbp_vlan_delete() to flush the dynamically learned entries of the vlan/port pair when removing a vlan from a port. Before this patch only the local mac was being removed and the dynamically learned ones were left to expire. Note that the do_all argument is still respected and if specified, the vid will be ignored. Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> --- note: There are 2 warnings, the C99 comment was already there and the 81 char line is the prototype and I think it looks better this way. One unclear thing is about vid 0, I see that it can be passed to nbp_vlan_(add|delete) although the comment says it shouldn't. Adding Vlad to CC list since he has introduced the vlan filtering infrastructure. net/bridge/br_fdb.c | 7 +++++-- net/bridge/br_if.c | 4 ++-- net/bridge/br_private.h | 2 +- net/bridge/br_stp_if.c | 2 +- net/bridge/br_sysfs_if.c | 2 +- net/bridge/br_vlan.c | 1 + 6 files changed, 11 insertions(+), 7 deletions(-)