search for: br_fdb_local

...ng v2, this seemed difficult because we want to test multiple bits and increment a counter, but remembering that clear_bit is never called for the bits I care about I came up with the following approach: a) Add a new flag BR_FDB_DYNAMIC_LEARNED, which is set to 1 iff BR_FDB_ADDED_BY_USER or BR_FDB_LOCAL are set in br_create. Every time BR_FDB_ADDED_BY_USER or BR_FDB_LOCAL is set, also clear BR_FDB_DYNAMIC_LEARNED, and decrement the count if it was 1 before. This solves the problem of testing two bits at once, and would not have been possible if we had a code path that could cle...

...ecause we want to test > multiple bits and increment a counter, but remembering that clear_bit > is never called for the bits I care about I came up with the following > approach: > > a) Add a new flag BR_FDB_DYNAMIC_LEARNED, which is set to 1 iff > BR_FDB_ADDED_BY_USER or BR_FDB_LOCAL are set in br_create. > Every time BR_FDB_ADDED_BY_USER or BR_FDB_LOCAL is set, also clear > BR_FDB_DYNAMIC_LEARNED, and decrement the count if it was 1 before. > This solves the problem of testing two bits at once, and would not > have been possible if we had a code...

...-= learned; fdb_notify(br, f, RTM_DELNEIGH, swdev_notify); call_rcu(&f->rcu, fdb_rcu_free); } -/* Delete a local entry if no other port had the same address. */ +/* Delete a local entry if no other port had the same address. + * + * This function should only be called on entries with BR_FDB_LOCAL set, + * so clear_bit never removes the last bit in BR_FDB_NOT_LEARNED_MASK. + */ static void fdb_delete_local(struct net_bridge *br, const struct net_bridge_port *p, struct net_bridge_fdb_entry *f) @@ -390,6 +429,11 @@ static struct net_bridge_fdb_entry *fdb_create(struct net_br...

...dev.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_local) ? br->dev : p->dev; item->info.ctx = ctx; -- 2.34.1

...kernel config default 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-jn...

...ge/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_static" and leave any further interpretations up to the consumer? > item->locked = false; > item->info.dev = (!p || item->is_loca...

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 indication whether the...

...@@ -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_static" and leave any > further interpretations up to the consumer? > My reasoning for this is that the common case is to have...

...0, 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 drivers had >...