On Mon, Jun 19, 2023 at 09:14:42AM +0200, Johannes Nixdorf wrote: > A malicious actor behind one bridge port may spam the kernel with packets > with a random source MAC address, each of which will create an FDB entry, > each of which is a dynamic allocation in the kernel. > > There are roughly 2^48 different MAC addresses, further limited by the > rhashtable they are stored in to 2^31. Each entry is of the type struct > net_bridge_fdb_entry, which is currently 128 bytes big. This means the > maximum amount of memory allocated for FDB entries is 2^31 * 128B = > 256GiB, which is too much for most computers. > > Mitigate this by adding a bridge netlink setting > IFLA_BR_FDB_MAX_LEARNED_ENTRIES, which, if nonzero, limits the amount > of learned entries to a user specified maximum. > > For backwards compatibility the default setting of 0 disables the limit. > > User-added entries by netlink or from bridge or bridge port addresses > are never blocked and do not count towards that limit. > > All changes to fdb_n_entries are under br->hash_lock, which means we do > not need additional locking. The call paths are (✓ denotes that > br->hash_lock is taken around the next call): > > - fdb_delete <-+- fdb_delete_local <-+- br_fdb_changeaddr ✓ > | +- br_fdb_change_mac_address ✓ > | +- br_fdb_delete_by_port ✓ > +- br_fdb_find_delete_local ✓ > +- fdb_add_local <-+- br_fdb_changeaddr ✓ > | +- br_fdb_change_mac_address ✓ > | +- br_fdb_add_local ✓ > +- br_fdb_cleanup ✓ > +- br_fdb_flush ✓ > +- br_fdb_delete_by_port ✓ > +- fdb_delete_by_addr_and_port <--- __br_fdb_delete ✓ > +- br_fdb_external_learn_del ✓ > - fdb_create <-+- fdb_add_local <-+- br_fdb_changeaddr ✓ > | +- br_fdb_change_mac_address ✓ > | +- br_fdb_add_local ✓ > +- br_fdb_update ✓ > +- fdb_add_entry <--- __br_fdb_add ✓ > +- br_fdb_external_learn_add ✓ > > The flags that imply 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-...@avm.de> > > --- > > Changes since v1: > - Do not initialize fdb_*_entries to 0. (from review) > - Do not skip decrementing on 0. (from review) > - Moved the counters to a conditional hole in struct net_bridge to > avoid growing the struct. (from review, it still grows the struct as > there are 2 32-bit values) > - Add IFLA_BR_FDB_CUR_LEARNED_ENTRIES (from review) > - Fix br_get_size() > - Only limit learned entries, rename to > *_(CUR|MAX)_LEARNED_ENTRIES. (from review) > > Obsolete v1 review comments: > - Return better errors to users: Due to limiting the limit to > automatically created entries, netlink fdb add requests and changing > bridge ports are never rejected, so they do not yet need a more > friendly error returned. > > include/uapi/linux/if_link.h | 2 ++ > net/bridge/br_fdb.c | 67 +++++++++++++++++++++++++++++++++--- > net/bridge/br_netlink.c | 13 ++++++- > net/bridge/br_private.h | 6 ++++
To minimize the number of changes per patch and make review easier, try to first maintain the count and the maximum and then in a separate patch expose them via netlink. See b57e8d870d52 and a1aee20d5db2, for example. Merge commit is cb3086cee656. > 4 files changed, 83 insertions(+), 5 deletions(-) > > diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h > index 4ac1000b0ef2..165b9014379b 100644 > --- a/include/uapi/linux/if_link.h > +++ b/include/uapi/linux/if_link.h > @@ -510,6 +510,8 @@ enum { > IFLA_BR_VLAN_STATS_PER_PORT, > IFLA_BR_MULTI_BOOLOPT, > IFLA_BR_MCAST_QUERIER_STATE, > + IFLA_BR_FDB_CUR_LEARNED_ENTRIES, > + IFLA_BR_FDB_MAX_LEARNED_ENTRIES, > __IFLA_BR_MAX, > }; > > diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c > index ac1dc8723b9c..bc61d1fd5fcf 100644 > --- a/net/bridge/br_fdb.c > +++ b/net/bridge/br_fdb.c > @@ -301,6 +301,38 @@ static void fdb_add_hw_addr(struct net_bridge *br, const > unsigned char *addr) > } > } > > +/* Set a FDB flag that implies the entry was not learned, and account > + * for changes in the learned status. > + */ > +static void __fdb_set_flag_not_learned(struct net_bridge *br, > + struct net_bridge_fdb_entry *fdb, > + long nr) > +{ > + WARN_ON_ONCE(!(BIT(nr) & BR_FDB_NOT_LEARNED_MASK)); > + > + /* learned before, but we set a flag that implies it's manually added */ > + if (!(fdb->flags & BR_FDB_NOT_LEARNED_MASK)) > + br->fdb_cur_learned_entries--; > + set_bit(nr, &fdb->flags); > +} > + > +/* Set a FDB flag that implies the entry was not learned, and account > + * for changes in the learned status. > + * > + * This function takes a lock, so ensure it is not called in the fast > + * path. > + */ > +static void fdb_set_flag_not_learned(struct net_bridge *br, > + struct net_bridge_fdb_entry *fdb, > + long nr) > +{ > + spin_lock_bh(&br->hash_lock); > + > + __fdb_set_flag_not_learned(br, fdb, nr); > + > + spin_unlock_bh(&br->hash_lock); > +} > + > /* When a static FDB entry is deleted, the HW address from that entry is > * also removed from the bridge private HW address list and updates all > * the ports with needed information. > @@ -321,6 +353,8 @@ static void fdb_del_hw_addr(struct net_bridge *br, const > unsigned char *addr) > static void fdb_delete(struct net_bridge *br, struct net_bridge_fdb_entry *f, > bool swdev_notify) > { > + bool learned = !(f->flags & BR_FDB_NOT_LEARNED_MASK); > + > trace_fdb_delete(br, f); > > if (test_bit(BR_FDB_STATIC, &f->flags)) > @@ -329,11 +363,16 @@ static void fdb_delete(struct net_bridge *br, struct > net_bridge_fdb_entry *f, > hlist_del_init_rcu(&f->fdb_node); > rhashtable_remove_fast(&br->fdb_hash_tbl, &f->rhnode, > br_fdb_rht_params); > + br->fdb_cur_learned_entries -= 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_bridge *br, > { > struct net_bridge_fdb_entry *fdb; > int err; > + bool learned = !(flags & BR_FDB_NOT_LEARNED_MASK); https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html#local-variable-ordering-reverse-xmas-tree-rcs > + > + if (unlikely(learned && br->fdb_max_learned_entries && > + br->fdb_cur_learned_entries >= > br->fdb_max_learned_entries)) This function is not run with RTNL held and so 'fdb_max_learned_entries' can be updated concurrently from user space. You will need READ_ONCE() / WRITE_ONCE() to silence KCSAN. > + return NULL; > > fdb = kmem_cache_alloc(br_fdb_cache, GFP_ATOMIC); > if (!fdb) > @@ -409,6 +453,8 @@ static struct net_bridge_fdb_entry *fdb_create(struct > net_bridge *br, > > hlist_add_head_rcu(&fdb->fdb_node, &br->fdb_list); > > + br->fdb_cur_learned_entries += learned; Don't remember seeing bool being added to a u32... > + > 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); > if (unlikely(fdb_modified)) { > trace_br_fdb_update(br, source, addr, vid, > flags); > fdb_notify(br, fdb, RTM_NEWNEIGH, true); > @@ -1070,6 +1116,8 @@ static int fdb_add_entry(struct net_bridge *br, struct > net_bridge_port *source, > modified = true; > } > > + if (!(fdb->flags & BR_FDB_NOT_LEARNED_MASK)) > + br->fdb_cur_learned_entries--; > set_bit(BR_FDB_ADDED_BY_USER, &fdb->flags); > } > > @@ -1440,10 +1488,10 @@ int br_fdb_external_learn_add(struct net_bridge *br, > struct net_bridge_port *p, > } > > if (swdev_notify) > - set_bit(BR_FDB_ADDED_BY_USER, &fdb->flags); > + __fdb_set_flag_not_learned(br, fdb, > BR_FDB_ADDED_BY_USER); > > if (!p) > - set_bit(BR_FDB_LOCAL, &fdb->flags); > + __fdb_set_flag_not_learned(br, fdb, BR_FDB_LOCAL); > > if (modified) > fdb_notify(br, fdb, RTM_NEWNEIGH, swdev_notify); > @@ -1508,3 +1556,14 @@ void br_fdb_clear_offload(const struct net_device > *dev, u16 vid) > spin_unlock_bh(&p->br->hash_lock); > } > EXPORT_SYMBOL_GPL(br_fdb_clear_offload); > + > +u32 br_fdb_get_cur_learned_entries(struct net_bridge *br) > +{ > + u32 ret; > + > + spin_lock_bh(&br->hash_lock); > + ret = br->fdb_cur_learned_entries; I believe that for MDB Nik asked not to take a spin lock for each dump and we used READ_ONCE() / WRITE_ONCE() instead. > + spin_unlock_bh(&br->hash_lock); > + > + return ret; > +} > diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c > index 05c5863d2e20..954c468d52ec 100644 > --- a/net/bridge/br_netlink.c > +++ b/net/bridge/br_netlink.c > @@ -1527,6 +1527,12 @@ static int br_changelink(struct net_device *brdev, > struct nlattr *tb[], > return err; > } > > + if (data[IFLA_BR_FDB_MAX_LEARNED_ENTRIES]) { Please add this attribute to 'br_policy' and in a separate patch convert the policy to use 'strict_start_type' so that future submissions will be forced to update the policy. See c00041cf1cb82 for example. I suggest writing a selftest to verify the expected behavior and to make sure this feature does not regress. There are a lot of examples under tools/testing/selftests/net/ and tools/testing/selftests/net/forwarding in particular. You can post another version before writing a test, but mark it as RFC so that it doesn't get applied by mistake. Thanks > + u32 val = nla_get_u32(data[IFLA_BR_FDB_MAX_LEARNED_ENTRIES]); > + > + br->fdb_max_learned_entries = val; > + } > + > return 0; > } > > @@ -1581,6 +1587,8 @@ static size_t br_get_size(const struct net_device > *brdev) > nla_total_size_64bit(sizeof(u64)) + /* > IFLA_BR_TOPOLOGY_CHANGE_TIMER */ > nla_total_size_64bit(sizeof(u64)) + /* IFLA_BR_GC_TIMER */ > nla_total_size(ETH_ALEN) + /* IFLA_BR_GROUP_ADDR */ > + nla_total_size(sizeof(u32)) + /* > IFLA_BR_FDB_CUR_LEARNED_ENTRIES */ > + nla_total_size(sizeof(u32)) + /* > IFLA_BR_FDB_MAX_LEARNED_ENTRIES */ > #ifdef CONFIG_BRIDGE_IGMP_SNOOPING > nla_total_size(sizeof(u8)) + /* IFLA_BR_MCAST_ROUTER */ > nla_total_size(sizeof(u8)) + /* IFLA_BR_MCAST_SNOOPING */ > @@ -1620,6 +1628,7 @@ static int br_fill_info(struct sk_buff *skb, const > struct net_device *brdev) > u32 stp_enabled = br->stp_enabled; > u16 priority = (br->bridge_id.prio[0] << 8) | br->bridge_id.prio[1]; > u8 vlan_enabled = br_vlan_enabled(br->dev); > + u32 fdb_cur_learned_entries = br_fdb_get_cur_learned_entries(br); > struct br_boolopt_multi bm; > u64 clockval; > > @@ -1656,7 +1665,9 @@ static int br_fill_info(struct sk_buff *skb, const > struct net_device *brdev) > nla_put_u8(skb, IFLA_BR_TOPOLOGY_CHANGE_DETECTED, > br->topology_change_detected) || > nla_put(skb, IFLA_BR_GROUP_ADDR, ETH_ALEN, br->group_addr) || > - nla_put(skb, IFLA_BR_MULTI_BOOLOPT, sizeof(bm), &bm)) > + nla_put(skb, IFLA_BR_MULTI_BOOLOPT, sizeof(bm), &bm) || > + nla_put_u32(skb, IFLA_BR_FDB_CUR_LEARNED_ENTRIES, > fdb_cur_learned_entries) || > + nla_put_u32(skb, IFLA_BR_FDB_MAX_LEARNED_ENTRIES, > br->fdb_max_learned_entries)) > return -EMSGSIZE; > > #ifdef CONFIG_BRIDGE_VLAN_FILTERING > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h > index 2119729ded2b..df079191479e 100644 > --- a/net/bridge/br_private.h > +++ b/net/bridge/br_private.h > @@ -275,6 +275,8 @@ enum { > BR_FDB_LOCKED, > }; > > +#define BR_FDB_NOT_LEARNED_MASK (BIT(BR_FDB_LOCAL) | > BIT(BR_FDB_ADDED_BY_USER)) > + > struct net_bridge_fdb_key { > mac_addr addr; > u16 vlan_id; > @@ -553,6 +555,9 @@ struct net_bridge { > struct kobject *ifobj; > u32 auto_cnt; > > + u32 fdb_max_learned_entries; > + u32 fdb_cur_learned_entries; > + > #ifdef CONFIG_NET_SWITCHDEV > /* Counter used to make sure that hardware domains get unique > * identifiers in case a bridge spans multiple switchdev instances. > @@ -838,6 +843,7 @@ int br_fdb_external_learn_del(struct net_bridge *br, > struct net_bridge_port *p, > bool swdev_notify); > void br_fdb_offloaded_set(struct net_bridge *br, struct net_bridge_port *p, > const unsigned char *addr, u16 vid, bool offloaded); > +u32 br_fdb_get_cur_learned_entries(struct net_bridge *br); > > /* br_forward.c */ > enum br_pkt_type { > -- > 2.40.1 >