Message ID | 1387281821-21342-8-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: > 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: > - We change the dst of a local entry when the same address is found. > This occurs in the case kernel has inserted the same address for another > port but has failed due to dup. We can regard changing dst as deleting > old one and inserting new one that should have been added by the dup > port, so we can always set its added_by_user to 0 in fdb_delete_local(). I disagree. What happens if the user tries add a duplicate fdb with the local bit set? That is permitted and in fact a default because in iproute right now. That fdb should persist until the port is removed or user removes the fdb. added_by_user flag should only be changed in the netlink code since the user has full control of it. -vlad > > - This is a slight change in behavior where the bridge device can receive > the traffic to the old address during the short window between calling > del_nbp() and br_stp_recalculate_bridge_id() in br_del_if(). However, > it is not a problem because we still have the address on the bridge device. > > 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 817f138..bd43cb1 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); > -- 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:12 -0500, Vlad Yasevich wrote: > On 12/17/2013 07:03 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: > > - We change the dst of a local entry when the same address is found. > > This occurs in the case kernel has inserted the same address for another > > port but has failed due to dup. We can regard changing dst as deleting > > old one and inserting new one that should have been added by the dup > > port, so we can always set its added_by_user to 0 in fdb_delete_local(). > > I disagree. What happens if the user tries add a duplicate fdb with > the local bit set? If the user add a dup local entry, the existent entry will be overwritten and its add_by_user is set to 1 (if !NLM_F_EXCL). The user never fails to add an entry due to dup in !NLM_F_EXCL case. > That is permitted and in fact a default because in > iproute right now. That fdb should persist until the port is removed or > user removes the fdb. > > added_by_user flag should only be changed in the netlink code since the > user has full control of it. Maybe my changelog is misleading. 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()/nbp_vlan_delete() doesn't call fdb_delete_local() for user-added entry. So it is safe to set added_by_user to 0 in fdb_delete_local(). will reword the changelog. 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
On 12/17/2013 09:27 PM, Toshiaki Makita wrote: > On Tue, 2013-12-17 at 14:12 -0500, Vlad Yasevich wrote: >> On 12/17/2013 07:03 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: >>> - We change the dst of a local entry when the same address is found. >>> This occurs in the case kernel has inserted the same address for another >>> port but has failed due to dup. We can regard changing dst as deleting >>> old one and inserting new one that should have been added by the dup >>> port, so we can always set its added_by_user to 0 in fdb_delete_local(). >> >> I disagree. What happens if the user tries add a duplicate fdb with >> the local bit set? > > If the user add a dup local entry, the existent entry will be > overwritten and its add_by_user is set to 1 (if !NLM_F_EXCL). > The user never fails to add an entry due to dup in !NLM_F_EXCL case. You are right. This is actually a very interesting situation. User may over-write the current entry on add, but a delete will remove the entry instead of restoring original configuration. I wonder if this was done on purpose... > >> That is permitted and in fact a default because in >> iproute right now. That fdb should persist until the port is removed or >> user removes the fdb. >> >> added_by_user flag should only be changed in the netlink code since the >> user has full control of it. > > Maybe my changelog is misleading. > > 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()/nbp_vlan_delete() doesn't call fdb_delete_local() > for user-added entry. > > So it is safe to set added_by_user to 0 in fdb_delete_local(). > > will reword the changelog. Ok. Thanks for clearing this up. Looking at patch 6 made it a bit more clear. Yes, updating the changelog makes sense since I don't see this patch introducing the the "change in behavior" you note in the log. -vlad > > 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
On Wed, 2013-12-18 at 12:50 -0500, Vlad Yasevich wrote: > On 12/17/2013 09:27 PM, Toshiaki Makita wrote: > > On Tue, 2013-12-17 at 14:12 -0500, Vlad Yasevich wrote: > >> On 12/17/2013 07:03 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: > >>> - We change the dst of a local entry when the same address is found. > >>> This occurs in the case kernel has inserted the same address for another > >>> port but has failed due to dup. We can regard changing dst as deleting > >>> old one and inserting new one that should have been added by the dup > >>> port, so we can always set its added_by_user to 0 in fdb_delete_local(). > >> > >> I disagree. What happens if the user tries add a duplicate fdb with > >> the local bit set? > > > > If the user add a dup local entry, the existent entry will be > > overwritten and its add_by_user is set to 1 (if !NLM_F_EXCL). > > The user never fails to add an entry due to dup in !NLM_F_EXCL case. > > You are right. This is actually a very interesting situation. User may > over-write the current entry on add, but a delete will remove the entry > instead of restoring original configuration. I wonder if this was done > on purpose... > > > > >> That is permitted and in fact a default because in > >> iproute right now. That fdb should persist until the port is removed or > >> user removes the fdb. > >> > >> added_by_user flag should only be changed in the netlink code since the > >> user has full control of it. > > > > Maybe my changelog is misleading. > > > > 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()/nbp_vlan_delete() doesn't call fdb_delete_local() > > for user-added entry. > > > > So it is safe to set added_by_user to 0 in fdb_delete_local(). > > > > will reword the changelog. > > Ok. Thanks for clearing this up. Looking at patch 6 made it a bit > more clear. Yes, updating the changelog makes sense since I don't see > this patch introducing the the "change in behavior" you note in the > log. This patch actually introduces the behavior change because br_fdb_delete_by_port() starts to use fdb_delete_local(). Without this patch, del_nbp() never delay the fdb deleting. Sorry for my confusing logs. 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 817f138..bd43cb1 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);
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: - We change the dst of a local entry when the same address is found. This occurs in the case kernel has inserted the same address for another port but has failed due to dup. We can regard changing dst as deleting old one and inserting new one that should have been added by the dup port, so we can always set its added_by_user to 0 in fdb_delete_local(). - This is a slight change in behavior where the bridge device can receive the traffic to the old address during the short window between calling del_nbp() and br_stp_recalculate_bridge_id() in br_del_if(). However, it is not a problem because we still have the address on the bridge device. Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> --- net/bridge/br_fdb.c | 25 ++++++------------------- 1 file changed, 6 insertions(+), 19 deletions(-)