search for: br_fdb_added_by_us

...- a/net/bridge/br_fdb.c +++ b/net/bridge/br_fdb.c @@ -1056,7 +1056,7 @@ static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source, if (!(flags & NLM_F_CREATE)) return -ENOENT; - fdb = fdb_create(br, source, addr, vid, 0); + fdb = fdb_create(br, source, addr, vid, BR_FDB_ADDED_BY_USER); if (!fdb) return -ENOMEM; @@ -1069,6 +1069,8 @@ static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source, WRITE_ONCE(fdb->dst, source); modified = true; } + + set_bit(BR_FDB_ADDED_BY_USER, &fdb->flags); } if (fdb_to_nud(br, fdb) != stat...

...ld help me test the intersection of multiple bits on both sides. Do you have any in mind? > > + > > return fdb; > > } > > @@ -894,7 +940,7 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source, > > } > > if (unlikely(test_bit(BR_FDB_ADDED_BY_USER, &flags))) > > - set_bit(BR_FDB_ADDED_BY_USER, &fdb->flags); > > + fdb_set_flag_not_learned(br, fdb, BR_FDB_ADDED_BY_USER); > > Unacceptable to take hash_lock and block all learning here, eventual > consistency is ok or some other method that is much lighter...

...tion > of multiple bits on both sides. Do you have any in mind? > >>> + >>> return fdb; >>> } >>> @@ -894,7 +940,7 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source, >>> } >>> if (unlikely(test_bit(BR_FDB_ADDED_BY_USER, &flags))) >>> - set_bit(BR_FDB_ADDED_BY_USER, &fdb->flags); >>> + fdb_set_flag_not_learned(br, fdb, BR_FDB_ADDED_BY_USER); >> >> Unacceptable to take hash_lock and block all learning here, eventual >> consistency is ok or some other method tha...

On Wed, Apr 12, 2023 at 07:24:07PM +0300, Vladimir Oltean wrote: > I'll send v2 with BR_FDB_ADDED_BY_EXT_LEARN not prevented from being > notified from switchdev. > > Unless you have any objection, I won't send v2 like this: > > if (test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags) && > !test_bit(BR_FDB_STATIC, &fdb->flags) && > !test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags)) > return; > > but like this: > > /* Entries with just the BR_FDB_ADDED_BY_USER flag set were created > * u...

..."hey, did you know you were never using the bridge fdb flags correctly?" is starting to become a bit of a meme. I'll send v2 with BR_FDB_ADDED_BY_EXT_LEARN not prevented from being notified from switchdev. Unless you have any objection, I won't send v2 like this: if (test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags) && !test_bit(BR_FDB_STATIC, &fdb->flags) && !test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags)) return; but like this: /* Entries with just the BR_FDB_ADDED_BY_USER flag set were created * using 'bridge fdb add ... master dyna...

...an entry does not come from learning (BR_FDB_NOT_LEARNED_MASK) are now only set or cleared under br->hash_lock as well, and when the boolean value of (fdb->flags & BR_FDB_NOT_LEARNED_MASK) changes the accounting is updated. This introduces one additional locked update in br_fdb_update if BR_FDB_ADDED_BY_USER was set. This is only the case when creating a new entry via netlink, and never in the packet handling fast path. Signed-off-by: Johannes Nixdorf <jnixdorf-oss at avm.de> --- Changes since v1: - Do not initialize fdb_*_entries to 0. (from review) - Do not skip decrementing on 0. (from...

...efault is disabling the limit (0). Without any limit a malicious actor may OOM a kernel by spamming packets with changing MAC addresses on their bridge port, so allow the bridge creator to limit the number of entries. Currently the manual entries are identified by the bridge flags BR_FDB_LOCAL or BR_FDB_ADDED_BY_USER, and changes to those flags are protected under a lock. This means the limit also applies to entries created with BR_FDB_ADDED_BY_EXT_LEARN but none of the other two, e.g. ones added by SWITCHDEV_FDB_ADD_TO_BRIDGE. v1: https://lore.kernel.org/netdev/20230515085046.4457-1-jnixdorf-oss at avm.de/...

There is a structural problem in switchdev, where the flag bits in struct switchdev_notifier_fdb_info (added_by_user, is_local etc) only represent a simplified / denatured view of what's in struct net_bridge_fdb_entry :: flags (BR_FDB_ADDED_BY_USER, BR_FDB_LOCAL etc). Each time we want to pass more information about struct net_bridge_fdb_entry :: flags to struct switchdev_notifier_fdb_info (here, BR_FDB_STATIC), we find that FDB entries were already notified to switchdev with no regard to this flag, and thus, switchdev drivers had no indica...

On Wed, Apr 12, 2023 at 05:27:33PM +0300, Vladimir Oltean wrote: > How are extern_learn FDB entries processed by spectrum's > SWITCHDEV_FDB_ADD_TO_DEVICE handler? No different than "BR_FDB_STATIC", which is a bug I'm aware of and intend to fix in net-next when I get the time (together with all the other combinations enabled by the bridge). Entry has ageing disabled, but

...023 at 11:49:51PM +0300, Vladimir Oltean wrote: > There is a structural problem in switchdev, where the flag bits in > struct switchdev_notifier_fdb_info (added_by_user, is_local etc) only > represent a simplified / denatured view of what's in struct > net_bridge_fdb_entry :: flags (BR_FDB_ADDED_BY_USER, BR_FDB_LOCAL etc). > Each time we want to pass more information about struct > net_bridge_fdb_entry :: flags to struct switchdev_notifier_fdb_info > (here, BR_FDB_STATIC), we find that FDB entries were already notified to > switchdev with no regard to this flag, and thus, switchdev d...

...ed:1; }; diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c index de18e9c1d7a7..9707d3fdb396 100644 --- a/net/bridge/br_switchdev.c +++ b/net/bridge/br_switchdev.c @@ -134,6 +134,7 @@ static void br_switchdev_fdb_populate(struct net_bridge *br, item->added_by_user = test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags); item->offloaded = test_bit(BR_FDB_OFFLOADED, &fdb->flags); item->is_local = test_bit(BR_FDB_LOCAL, &fdb->flags); + item->is_dyn = !test_bit(BR_FDB_STATIC, &fdb->flags); item->locked = false; item->info.dev = (!p || item->is_loca...

On Thu, Mar 30, 2023 at 18:07, Vladimir Oltean <olteanv at gmail.com> wrote: > As a bug fix, stop reporting to switchdev those FDB entries with > BR_FDB_ADDED_BY_USER && !BR_FDB_STATIC. Then, after "net" is merged into > "net-next" next Thursday (the ship has sailed for today), add "bool static" It is probably too late today (now I have a Debian based VM that can do the selftests), but with this bug fix I have 1) not s...

On Thu, Apr 06, 2023 at 05:17:46PM +0200, Hans Schultz wrote: > On Thu, Mar 30, 2023 at 18:07, Vladimir Oltean <olteanv at gmail.com> wrote: > > As a bug fix, stop reporting to switchdev those FDB entries with > > BR_FDB_ADDED_BY_USER && !BR_FDB_STATIC. Then, after "net" is merged into > > "net-next" next Thursday (the ship has sailed for today), add "bool static" > > It is probably too late today (now I have a Debian based VM that can do > the selftests), but with this bug...

...gt; If you have a suggestion, feel free. Didn't I explain what I would do from the first reply on this thread? https://patchwork.kernel.org/project/netdevbpf/patch/20230318141010.513424-3-netdev at kapio-technology.com/#25270613 As a bug fix, stop reporting to switchdev those FDB entries with BR_FDB_ADDED_BY_USER && !BR_FDB_STATIC. Then, after "net" is merged into "net-next" next Thursday (the ship has sailed for today), add "bool static" to the switchdev notifier info, and make all switchdev drivers (everywhere where a SWITCHDEV_FDB_ADD_TO_DEVICE handler appears) ign...

...net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c > index 7eb6fd5bb917..60c05a00a1df 100644 > --- a/net/bridge/br_switchdev.c > +++ b/net/bridge/br_switchdev.c > @@ -136,6 +136,7 @@ static void br_switchdev_fdb_populate(struct net_bridge *br, > item->added_by_user = test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags); > item->offloaded = test_bit(BR_FDB_OFFLOADED, &fdb->flags); > item->is_local = test_bit(BR_FDB_LOCAL, &fdb->flags); > + item->is_dyn = !test_bit(BR_FDB_STATIC, &fdb->flags); Why reverse logic? Why not just name this "is_stati...

...t/bridge/br_switchdev.c >> index 7eb6fd5bb917..60c05a00a1df 100644 >> --- a/net/bridge/br_switchdev.c >> +++ b/net/bridge/br_switchdev.c >> @@ -136,6 +136,7 @@ static void br_switchdev_fdb_populate(struct >> net_bridge *br, >> item->added_by_user = test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags); >> item->offloaded = test_bit(BR_FDB_OFFLOADED, &fdb->flags); >> item->is_local = test_bit(BR_FDB_LOCAL, &fdb->flags); >> + item->is_dyn = !test_bit(BR_FDB_STATIC, &fdb->flags); > > Why reverse logic? Why not just na...

On Thu, Mar 30, 2023 at 16:09, Vladimir Oltean <olteanv at gmail.com> wrote: > On Thu, Mar 30, 2023 at 02:59:04PM +0200, Hans Schultz wrote: >> On Thu, Mar 30, 2023 at 15:43, Vladimir Oltean <olteanv at gmail.com> wrote: >> > On Tue, Mar 28, 2023 at 09:45:26PM +0200, Hans Schultz wrote: >> >> So the solution would be to not let the DSA layer send the