| Message ID | 1387281821-21342-2-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_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 case > 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> Acked-by: Vlad Yasevich <vyasevic@redhat.com> -vlad > --- > net/bridge/br_fdb.c | 5 ++++- > net/bridge/br_private.h | 1 + > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c > index 33e8f23..5dab230 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); > } > @@ -648,6 +650,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) { > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h > index 045d56e..91fb2c2 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; > }; > > -- 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 07:03 AM, Toshiaki Makita 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 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 case > 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. > I was looking over my patch series that adds something similar to this and noticed that you are not handing the NTF_USE case. That case was always troublesome for me as it allows for 2 different way to create the same FDB: one through br_fdb_update() and one through fdb_add_entry(). It is possible, though I haven't found any users yet, that NTF_USE may be used and in that case, bridge will create a dynamic fdb and disregard all NUD flags. In case case, add_by_user will not be set either. I think that the above is broken and plan to submit a fix shortly. Thanks -vlad > Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> > --- > net/bridge/br_fdb.c | 5 ++++- > net/bridge/br_private.h | 1 + > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c > index 33e8f23..5dab230 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); > } > @@ -648,6 +650,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) { > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h > index 045d56e..91fb2c2 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; > }; > > -- 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 01/03/2014 02:28 PM, Vlad Yasevich wrote: > On 12/17/2013 07:03 AM, Toshiaki Makita 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 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 case >> 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. >> > > I was looking over my patch series that adds something similar to this > and noticed that you are not handing the NTF_USE case. That case was > always troublesome for me as it allows for 2 different way to create > the same FDB: one through br_fdb_update() and one through fdb_add_entry(). > > It is possible, though I haven't found any users yet, that NTF_USE > may be used and in that case, bridge will create a dynamic fdb and > disregard all NUD flags. In case case, add_by_user will not be set > either. > > I think that the above is broken and plan to submit a fix shortly. Just looked again at my NTF_USE patch and while it seems ok, the whole NTF_USE usage is racy to begin with and I am really starting to question it's validity. Presently, br_fdb_update() will not update local fdb entries. Instead it will log a misleading warning... It will only let you update non-local entries. This is fine for user-created entries, but any operation on dynamically created entries will only persist until the next packet. It also races against the packet, so there is absolutely no guarantee that the values of fdb->dst and fdb->updated will be consistent.. It seems to me that the update capability of NTF_USE would actually be of more value on local or user-created fdb entries. The fdb creation capability of NTF_USE should be disabled. Thoughts? -vlad > > Thanks > -vlad > >> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> >> --- >> net/bridge/br_fdb.c | 5 ++++- >> net/bridge/br_private.h | 1 + >> 2 files changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c >> index 33e8f23..5dab230 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); >> } >> @@ -648,6 +650,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) { >> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h >> index 045d56e..91fb2c2 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; >> }; >> >> > -- 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 Fri, 2014-01-03 at 15:46 -0500, Vlad Yasevich wrote: > On 01/03/2014 02:28 PM, Vlad Yasevich wrote: > > On 12/17/2013 07:03 AM, Toshiaki Makita 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 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 case > >> 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. > >> > > > > I was looking over my patch series that adds something similar to this > > and noticed that you are not handing the NTF_USE case. That case was > > always troublesome for me as it allows for 2 different way to create > > the same FDB: one through br_fdb_update() and one through fdb_add_entry(). > > > > It is possible, though I haven't found any users yet, that NTF_USE > > may be used and in that case, bridge will create a dynamic fdb and > > disregard all NUD flags. In case case, add_by_user will not be set > > either. > > > > I think that the above is broken and plan to submit a fix shortly. > > Just looked again at my NTF_USE patch and while it seems ok, the whole > NTF_USE usage is racy to begin with and I am really starting to question > it's validity. > > Presently, br_fdb_update() will not update local fdb entries. Instead > it will log a misleading warning... It will only let you update > non-local entries. This is fine for user-created entries, but any > operation on dynamically created entries will only persist until > the next packet. It also races against the packet, so there is > absolutely no guarantee that the values of fdb->dst and fdb->updated > will be consistent.. > > It seems to me that the update capability of NTF_USE would actually be > of more value on local or user-created fdb entries. > > The fdb creation capability of NTF_USE should be disabled. > > Thoughts? I ignored NTF_USE in this patch because I regard it as emulating kernel creating entries after investigating git log. http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=0c5c2d3089068d4aa378f7a40d2b5ad9d4f52ce8 http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=292d1398983f3514a0eab13b7606df7f4730b498 So I think NTF_USE shouldn't set added_by_user. And to emulate kernel creating entries, simply calling br_fdb_update() is the right way, isn't it? 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 01/05/2014 10:26 AM, Toshiaki Makita wrote: > On Fri, 2014-01-03 at 15:46 -0500, Vlad Yasevich wrote: >> On 01/03/2014 02:28 PM, Vlad Yasevich wrote: >>> On 12/17/2013 07:03 AM, Toshiaki Makita 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 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 case >>>> 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. >>>> >>> >>> I was looking over my patch series that adds something similar to this >>> and noticed that you are not handing the NTF_USE case. That case was >>> always troublesome for me as it allows for 2 different way to create >>> the same FDB: one through br_fdb_update() and one through fdb_add_entry(). >>> >>> It is possible, though I haven't found any users yet, that NTF_USE >>> may be used and in that case, bridge will create a dynamic fdb and >>> disregard all NUD flags. In case case, add_by_user will not be set >>> either. >>> >>> I think that the above is broken and plan to submit a fix shortly. >> >> Just looked again at my NTF_USE patch and while it seems ok, the whole >> NTF_USE usage is racy to begin with and I am really starting to question >> it's validity. >> >> Presently, br_fdb_update() will not update local fdb entries. Instead >> it will log a misleading warning... It will only let you update >> non-local entries. This is fine for user-created entries, but any >> operation on dynamically created entries will only persist until >> the next packet. It also races against the packet, so there is >> absolutely no guarantee that the values of fdb->dst and fdb->updated >> will be consistent.. >> >> It seems to me that the update capability of NTF_USE would actually be >> of more value on local or user-created fdb entries. >> >> The fdb creation capability of NTF_USE should be disabled. >> >> Thoughts? > > I ignored NTF_USE in this patch because I regard it as emulating kernel > creating entries after investigating git log. > > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=0c5c2d3089068d4aa378f7a40d2b5ad9d4f52ce8 > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=292d1398983f3514a0eab13b7606df7f4730b498 > > So I think NTF_USE shouldn't set added_by_user. > And to emulate kernel creating entries, simply calling br_fdb_update() > is the right way, isn't it? You can create dynamic entries (emulating the kernel) without NTF_USE. Just set the NUD_REACHABLE. Notice that arp cache only uses NTF_USE to trigger and arp notification. The creation is still triggered via other netlink flags. The more I look at this the more I think NTF_USE should not create an entry all by itself. -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 Mon, 2014-01-06 at 06:29 -0500, Vlad Yasevich wrote: > On 01/05/2014 10:26 AM, Toshiaki Makita wrote: > > On Fri, 2014-01-03 at 15:46 -0500, Vlad Yasevich wrote: > >> On 01/03/2014 02:28 PM, Vlad Yasevich wrote: > >>> On 12/17/2013 07:03 AM, Toshiaki Makita 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 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 case > >>>> 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. > >>>> > >>> > >>> I was looking over my patch series that adds something similar to this > >>> and noticed that you are not handing the NTF_USE case. That case was > >>> always troublesome for me as it allows for 2 different way to create > >>> the same FDB: one through br_fdb_update() and one through fdb_add_entry(). > >>> > >>> It is possible, though I haven't found any users yet, that NTF_USE > >>> may be used and in that case, bridge will create a dynamic fdb and > >>> disregard all NUD flags. In case case, add_by_user will not be set > >>> either. > >>> > >>> I think that the above is broken and plan to submit a fix shortly. > >> > >> Just looked again at my NTF_USE patch and while it seems ok, the whole > >> NTF_USE usage is racy to begin with and I am really starting to question > >> it's validity. > >> > >> Presently, br_fdb_update() will not update local fdb entries. Instead > >> it will log a misleading warning... It will only let you update > >> non-local entries. This is fine for user-created entries, but any > >> operation on dynamically created entries will only persist until > >> the next packet. It also races against the packet, so there is > >> absolutely no guarantee that the values of fdb->dst and fdb->updated > >> will be consistent.. > >> > >> It seems to me that the update capability of NTF_USE would actually be > >> of more value on local or user-created fdb entries. > >> > >> The fdb creation capability of NTF_USE should be disabled. > >> > >> Thoughts? > > > > I ignored NTF_USE in this patch because I regard it as emulating kernel > > creating entries after investigating git log. > > > > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=0c5c2d3089068d4aa378f7a40d2b5ad9d4f52ce8 > > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=292d1398983f3514a0eab13b7606df7f4730b498 > > > > So I think NTF_USE shouldn't set added_by_user. > > And to emulate kernel creating entries, simply calling br_fdb_update() > > is the right way, isn't it? > > You can create dynamic entries (emulating the kernel) without NTF_USE. > Just set the NUD_REACHABLE. Notice that arp cache only uses NTF_USE > to trigger and arp notification. The creation is still triggered via > other netlink flags. > > The more I look at this the more I think NTF_USE should not create > an entry all by itself. I haven't fully understood you yet. Currently NTF_USE behaves as if the port receives a frame and it seems to work, though the ability to create entries is different from neigh subsystem. Why do you want to change the behavior? Are you worried about inconsistency of NLM-flags/NUD-state with NTF_USE between neigh and bridge? 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 01/07/2014 07:42 AM, Toshiaki Makita wrote: > On Mon, 2014-01-06 at 06:29 -0500, Vlad Yasevich wrote: >> On 01/05/2014 10:26 AM, Toshiaki Makita wrote: >>> On Fri, 2014-01-03 at 15:46 -0500, Vlad Yasevich wrote: >>>> On 01/03/2014 02:28 PM, Vlad Yasevich wrote: >>>>> On 12/17/2013 07:03 AM, Toshiaki Makita 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 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 case >>>>>> 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. >>>>>> >>>>> >>>>> I was looking over my patch series that adds something similar to this >>>>> and noticed that you are not handing the NTF_USE case. That case was >>>>> always troublesome for me as it allows for 2 different way to create >>>>> the same FDB: one through br_fdb_update() and one through fdb_add_entry(). >>>>> >>>>> It is possible, though I haven't found any users yet, that NTF_USE >>>>> may be used and in that case, bridge will create a dynamic fdb and >>>>> disregard all NUD flags. In case case, add_by_user will not be set >>>>> either. >>>>> >>>>> I think that the above is broken and plan to submit a fix shortly. >>>> >>>> Just looked again at my NTF_USE patch and while it seems ok, the whole >>>> NTF_USE usage is racy to begin with and I am really starting to question >>>> it's validity. >>>> >>>> Presently, br_fdb_update() will not update local fdb entries. Instead >>>> it will log a misleading warning... It will only let you update >>>> non-local entries. This is fine for user-created entries, but any >>>> operation on dynamically created entries will only persist until >>>> the next packet. It also races against the packet, so there is >>>> absolutely no guarantee that the values of fdb->dst and fdb->updated >>>> will be consistent.. >>>> >>>> It seems to me that the update capability of NTF_USE would actually be >>>> of more value on local or user-created fdb entries. >>>> >>>> The fdb creation capability of NTF_USE should be disabled. >>>> >>>> Thoughts? >>> >>> I ignored NTF_USE in this patch because I regard it as emulating kernel >>> creating entries after investigating git log. >>> >>> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=0c5c2d3089068d4aa378f7a40d2b5ad9d4f52ce8 >>> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=292d1398983f3514a0eab13b7606df7f4730b498 >>> >>> So I think NTF_USE shouldn't set added_by_user. >>> And to emulate kernel creating entries, simply calling br_fdb_update() >>> is the right way, isn't it? >> >> You can create dynamic entries (emulating the kernel) without NTF_USE. >> Just set the NUD_REACHABLE. Notice that arp cache only uses NTF_USE >> to trigger and arp notification. The creation is still triggered via >> other netlink flags. >> >> The more I look at this the more I think NTF_USE should not create >> an entry all by itself. > > I haven't fully understood you yet. > Currently NTF_USE behaves as if the port receives a frame and it seems > to work, though the ability to create entries is different from neigh > subsystem. > Why do you want to change the behavior? > Are you worried about inconsistency of NLM-flags/NUD-state with NTF_USE > between neigh and bridge? No, it is inconsistent with other NLM/NUD-state within bridge. As an fdb creation flag NTF_USE is confusing. It will create an entry without NLM_F_CREATE being set. It will ignore NLM_F_EXCL flag as well. It will additionally ignore any NUD-state flags that may be set in the netlink message. So it may not be doing what the user wishes. It also provides duplicate functionality. The same results are achieved by setting NLM_F_CREATE flag and NUD_REACHABLE state in the message. -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 Tue, 2014-01-07 at 09:44 -0500, Vlad Yasevich wrote: > On 01/07/2014 07:42 AM, Toshiaki Makita wrote: > > On Mon, 2014-01-06 at 06:29 -0500, Vlad Yasevich wrote: > >> On 01/05/2014 10:26 AM, Toshiaki Makita wrote: > >>> On Fri, 2014-01-03 at 15:46 -0500, Vlad Yasevich wrote: > >>>> On 01/03/2014 02:28 PM, Vlad Yasevich wrote: > >>>>> On 12/17/2013 07:03 AM, Toshiaki Makita 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 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 case > >>>>>> 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. > >>>>>> > >>>>> > >>>>> I was looking over my patch series that adds something similar to this > >>>>> and noticed that you are not handing the NTF_USE case. That case was > >>>>> always troublesome for me as it allows for 2 different way to create > >>>>> the same FDB: one through br_fdb_update() and one through fdb_add_entry(). > >>>>> > >>>>> It is possible, though I haven't found any users yet, that NTF_USE > >>>>> may be used and in that case, bridge will create a dynamic fdb and > >>>>> disregard all NUD flags. In case case, add_by_user will not be set > >>>>> either. > >>>>> > >>>>> I think that the above is broken and plan to submit a fix shortly. > >>>> > >>>> Just looked again at my NTF_USE patch and while it seems ok, the whole > >>>> NTF_USE usage is racy to begin with and I am really starting to question > >>>> it's validity. > >>>> > >>>> Presently, br_fdb_update() will not update local fdb entries. Instead > >>>> it will log a misleading warning... It will only let you update > >>>> non-local entries. This is fine for user-created entries, but any > >>>> operation on dynamically created entries will only persist until > >>>> the next packet. It also races against the packet, so there is > >>>> absolutely no guarantee that the values of fdb->dst and fdb->updated > >>>> will be consistent.. > >>>> > >>>> It seems to me that the update capability of NTF_USE would actually be > >>>> of more value on local or user-created fdb entries. > >>>> > >>>> The fdb creation capability of NTF_USE should be disabled. > >>>> > >>>> Thoughts? > >>> > >>> I ignored NTF_USE in this patch because I regard it as emulating kernel > >>> creating entries after investigating git log. > >>> > >>> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=0c5c2d3089068d4aa378f7a40d2b5ad9d4f52ce8 > >>> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=292d1398983f3514a0eab13b7606df7f4730b498 > >>> > >>> So I think NTF_USE shouldn't set added_by_user. > >>> And to emulate kernel creating entries, simply calling br_fdb_update() > >>> is the right way, isn't it? > >> > >> You can create dynamic entries (emulating the kernel) without NTF_USE. > >> Just set the NUD_REACHABLE. Notice that arp cache only uses NTF_USE > >> to trigger and arp notification. The creation is still triggered via > >> other netlink flags. > >> > >> The more I look at this the more I think NTF_USE should not create > >> an entry all by itself. > > > > I haven't fully understood you yet. > > Currently NTF_USE behaves as if the port receives a frame and it seems > > to work, though the ability to create entries is different from neigh > > subsystem. > > Why do you want to change the behavior? > > Are you worried about inconsistency of NLM-flags/NUD-state with NTF_USE > > between neigh and bridge? > > No, it is inconsistent with other NLM/NUD-state within bridge. As > an fdb creation flag NTF_USE is confusing. It will create an entry > without NLM_F_CREATE being set. It will ignore NLM_F_EXCL flag as > well. It will additionally ignore any NUD-state flags that may be set > in the netlink message. So it may not be doing what the user wishes. I don't know which of NTF-flags and NLM-flags/NUD-state should be given high priority on setting. For now, in bridge, NTF_USE masks any other flags. If this is not proper way for netlink/neighbour, I will agree with you. If not sure, I have no motivation to change existing behavior that might be expected by some users. > > It also provides duplicate functionality. The same results are achieved > by setting NLM_F_CREATE flag and NUD_REACHABLE state in the message. br_fdb_update() never updates fdb->used, which is visible by user, unlike fdb_add_entry(). If it is duplicate functionality, isn't NTF_USE itself no use? What can be achieved by changing capability of creation and update of local entries? 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 01/07/2014 11:33 AM, Toshiaki Makita wrote: > On Tue, 2014-01-07 at 09:44 -0500, Vlad Yasevich wrote: >> On 01/07/2014 07:42 AM, Toshiaki Makita wrote: >>> On Mon, 2014-01-06 at 06:29 -0500, Vlad Yasevich wrote: >>>> On 01/05/2014 10:26 AM, Toshiaki Makita wrote: >>>>> On Fri, 2014-01-03 at 15:46 -0500, Vlad Yasevich wrote: >>>>>> On 01/03/2014 02:28 PM, Vlad Yasevich wrote: >>>>>>> On 12/17/2013 07:03 AM, Toshiaki Makita 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 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 case >>>>>>>> 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. >>>>>>>> >>>>>>> >>>>>>> I was looking over my patch series that adds something similar to this >>>>>>> and noticed that you are not handing the NTF_USE case. That case was >>>>>>> always troublesome for me as it allows for 2 different way to create >>>>>>> the same FDB: one through br_fdb_update() and one through fdb_add_entry(). >>>>>>> >>>>>>> It is possible, though I haven't found any users yet, that NTF_USE >>>>>>> may be used and in that case, bridge will create a dynamic fdb and >>>>>>> disregard all NUD flags. In case case, add_by_user will not be set >>>>>>> either. >>>>>>> >>>>>>> I think that the above is broken and plan to submit a fix shortly. >>>>>> >>>>>> Just looked again at my NTF_USE patch and while it seems ok, the whole >>>>>> NTF_USE usage is racy to begin with and I am really starting to question >>>>>> it's validity. >>>>>> >>>>>> Presently, br_fdb_update() will not update local fdb entries. Instead >>>>>> it will log a misleading warning... It will only let you update >>>>>> non-local entries. This is fine for user-created entries, but any >>>>>> operation on dynamically created entries will only persist until >>>>>> the next packet. It also races against the packet, so there is >>>>>> absolutely no guarantee that the values of fdb->dst and fdb->updated >>>>>> will be consistent.. >>>>>> >>>>>> It seems to me that the update capability of NTF_USE would actually be >>>>>> of more value on local or user-created fdb entries. >>>>>> >>>>>> The fdb creation capability of NTF_USE should be disabled. >>>>>> >>>>>> Thoughts? >>>>> >>>>> I ignored NTF_USE in this patch because I regard it as emulating kernel >>>>> creating entries after investigating git log. >>>>> >>>>> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=0c5c2d3089068d4aa378f7a40d2b5ad9d4f52ce8 >>>>> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=292d1398983f3514a0eab13b7606df7f4730b498 >>>>> >>>>> So I think NTF_USE shouldn't set added_by_user. >>>>> And to emulate kernel creating entries, simply calling br_fdb_update() >>>>> is the right way, isn't it? >>>> >>>> You can create dynamic entries (emulating the kernel) without NTF_USE. >>>> Just set the NUD_REACHABLE. Notice that arp cache only uses NTF_USE >>>> to trigger and arp notification. The creation is still triggered via >>>> other netlink flags. >>>> >>>> The more I look at this the more I think NTF_USE should not create >>>> an entry all by itself. >>> >>> I haven't fully understood you yet. >>> Currently NTF_USE behaves as if the port receives a frame and it seems >>> to work, though the ability to create entries is different from neigh >>> subsystem. >>> Why do you want to change the behavior? >>> Are you worried about inconsistency of NLM-flags/NUD-state with NTF_USE >>> between neigh and bridge? >> >> No, it is inconsistent with other NLM/NUD-state within bridge. As >> an fdb creation flag NTF_USE is confusing. It will create an entry >> without NLM_F_CREATE being set. It will ignore NLM_F_EXCL flag as >> well. It will additionally ignore any NUD-state flags that may be set >> in the netlink message. So it may not be doing what the user wishes. > > I don't know which of NTF-flags and NLM-flags/NUD-state should be given > high priority on setting. For now, in bridge, NTF_USE masks any other > flags. If this is not proper way for netlink/neighbour, I will agree > with you. If not sure, I have no motivation to change existing behavior > that might be expected by some users. > >> >> It also provides duplicate functionality. The same results are achieved >> by setting NLM_F_CREATE flag and NUD_REACHABLE state in the message. > > br_fdb_update() never updates fdb->used, which is visible by user, > unlike fdb_add_entry(). Thanks for pointing this out. It looks like there are some inconsistencies in the fdb->used markings as well. > > If it is duplicate functionality, isn't NTF_USE itself no use? > What can be achieved by changing capability of creation and update of > local entries? Update of local entries gives you port redirection, but doing it under rcu. Not sure if it really makes much sense though... NTF_USE makes since for neighbor cache as it triggers an ARP and a refresh of the entry. Suppose, NTF_USE on the fdb entry should trigger a refresh as well, but causing a create has to be explicit. I think I'll just send my patch and we can continue this discussion there. -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 Tue, 2014-01-07 at 12:45 -0500, Vlad Yasevich wrote: > On 01/07/2014 11:33 AM, Toshiaki Makita wrote: > > On Tue, 2014-01-07 at 09:44 -0500, Vlad Yasevich wrote: > >> On 01/07/2014 07:42 AM, Toshiaki Makita wrote: > >>> On Mon, 2014-01-06 at 06:29 -0500, Vlad Yasevich wrote: > >>>> On 01/05/2014 10:26 AM, Toshiaki Makita wrote: > >>>>> On Fri, 2014-01-03 at 15:46 -0500, Vlad Yasevich wrote: > >>>>>> On 01/03/2014 02:28 PM, Vlad Yasevich wrote: > >>>>>>> On 12/17/2013 07:03 AM, Toshiaki Makita 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 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 case > >>>>>>>> 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. > >>>>>>>> > >>>>>>> > >>>>>>> I was looking over my patch series that adds something similar to this > >>>>>>> and noticed that you are not handing the NTF_USE case. That case was > >>>>>>> always troublesome for me as it allows for 2 different way to create > >>>>>>> the same FDB: one through br_fdb_update() and one through fdb_add_entry(). > >>>>>>> > >>>>>>> It is possible, though I haven't found any users yet, that NTF_USE > >>>>>>> may be used and in that case, bridge will create a dynamic fdb and > >>>>>>> disregard all NUD flags. In case case, add_by_user will not be set > >>>>>>> either. > >>>>>>> > >>>>>>> I think that the above is broken and plan to submit a fix shortly. > >>>>>> > >>>>>> Just looked again at my NTF_USE patch and while it seems ok, the whole > >>>>>> NTF_USE usage is racy to begin with and I am really starting to question > >>>>>> it's validity. > >>>>>> > >>>>>> Presently, br_fdb_update() will not update local fdb entries. Instead > >>>>>> it will log a misleading warning... It will only let you update > >>>>>> non-local entries. This is fine for user-created entries, but any > >>>>>> operation on dynamically created entries will only persist until > >>>>>> the next packet. It also races against the packet, so there is > >>>>>> absolutely no guarantee that the values of fdb->dst and fdb->updated > >>>>>> will be consistent.. > >>>>>> > >>>>>> It seems to me that the update capability of NTF_USE would actually be > >>>>>> of more value on local or user-created fdb entries. > >>>>>> > >>>>>> The fdb creation capability of NTF_USE should be disabled. > >>>>>> > >>>>>> Thoughts? > >>>>> > >>>>> I ignored NTF_USE in this patch because I regard it as emulating kernel > >>>>> creating entries after investigating git log. > >>>>> > >>>>> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=0c5c2d3089068d4aa378f7a40d2b5ad9d4f52ce8 > >>>>> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=292d1398983f3514a0eab13b7606df7f4730b498 > >>>>> > >>>>> So I think NTF_USE shouldn't set added_by_user. > >>>>> And to emulate kernel creating entries, simply calling br_fdb_update() > >>>>> is the right way, isn't it? > >>>> > >>>> You can create dynamic entries (emulating the kernel) without NTF_USE. > >>>> Just set the NUD_REACHABLE. Notice that arp cache only uses NTF_USE > >>>> to trigger and arp notification. The creation is still triggered via > >>>> other netlink flags. > >>>> > >>>> The more I look at this the more I think NTF_USE should not create > >>>> an entry all by itself. > >>> > >>> I haven't fully understood you yet. > >>> Currently NTF_USE behaves as if the port receives a frame and it seems > >>> to work, though the ability to create entries is different from neigh > >>> subsystem. > >>> Why do you want to change the behavior? > >>> Are you worried about inconsistency of NLM-flags/NUD-state with NTF_USE > >>> between neigh and bridge? > >> > >> No, it is inconsistent with other NLM/NUD-state within bridge. As > >> an fdb creation flag NTF_USE is confusing. It will create an entry > >> without NLM_F_CREATE being set. It will ignore NLM_F_EXCL flag as > >> well. It will additionally ignore any NUD-state flags that may be set > >> in the netlink message. So it may not be doing what the user wishes. > > > > I don't know which of NTF-flags and NLM-flags/NUD-state should be given > > high priority on setting. For now, in bridge, NTF_USE masks any other > > flags. If this is not proper way for netlink/neighbour, I will agree > > with you. If not sure, I have no motivation to change existing behavior > > that might be expected by some users. > > > >> > >> It also provides duplicate functionality. The same results are achieved > >> by setting NLM_F_CREATE flag and NUD_REACHABLE state in the message. > > > > br_fdb_update() never updates fdb->used, which is visible by user, > > unlike fdb_add_entry(). > > Thanks for pointing this out. It looks like there are some > inconsistencies in the fdb->used markings as well. > > > > > If it is duplicate functionality, isn't NTF_USE itself no use? > > What can be achieved by changing capability of creation and update of > > local entries? > > Update of local entries gives you port redirection, but doing it under > rcu. Not sure if it really makes much sense though... > > NTF_USE makes since for neighbor cache as it triggers an ARP and a > refresh of the entry. Suppose, NTF_USE on the fdb entry should > trigger a refresh as well, but causing a create has to be explicit. > > I think I'll just send my patch and we can continue this discussion > there. OK, I'll wait for your patch. 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 33e8f23..5dab230 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); } @@ -648,6 +650,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) { diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h index 045d56e..91fb2c2 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; };
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 case 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 | 5 ++++- net/bridge/br_private.h | 1 + 2 files changed, 5 insertions(+), 1 deletion(-)