Re: [PATCH net-next 1/2] bridge: Add a limit on FDB entries

On 16/05/2023 11:53, Johannes Nixdorf wrote:

On Tue, May 16, 2023 at 11:38:11AM +0300, Nikolay Aleksandrov wrote:

quoted

On 15/05/2023 11:50, Johannes Nixdorf wrote:

quoted

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_ENTRIES,
which, if nonzero, limits the amount of entries to a user specified
maximum.

For backwards compatibility the default setting of 0 disables the 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 ✓

Signed-off-by: Johannes Nixdorf <redacted>
---
 include/uapi/linux/if_link.h | 1 +
 net/bridge/br_device.c       | 2 ++
 net/bridge/br_fdb.c          | 6 ++++++
 net/bridge/br_netlink.c      | 9 ++++++++-
 net/bridge/br_private.h      | 2 ++
 5 files changed, 19 insertions(+), 1 deletion(-)

I completely missed the fact that you don't deal with the situation where you already have fdbs created
and a limit is set later, then it would be useless because it will start counting from 0 even though
there are already entries.

This should not be an issue. The accounting starts with the bridge
creation and is never suspended, so if the user sets a limit later we
do not restart counting at 0.

The only corner case I can see there is if the user sets a new limit
lower than the current number of FDB entries. In that case the code
currently leaves the bridge in a state where the limit is violated,
but refuses new FDB entries until the total is back below the limit. The
alternative of cleaning out old FDB entries until their number is under
the limit again seems to be more error prone to me as well, so I'd rather
leave it that way.

Ah, good. That's ok then.

quoted

Also another issue that came to mind is that you don't deal with fdb_create()
for "special" entries, i.e. when adding a port. Currently it will print an error, but you should revisit
all callers and see where it might be a problem.

I'll have a look again, also to see whether only counting dynamic
entries created as a reaction to observed packets might be a viable
alternative. If the user creates the entries by adding a port or manually
via netlink I see no reason to restrict them to the same limit.

Hmm.. perhaps we can add a flag mask of entries to count. Initially it can be
only dynamic entries. We should include more people in this discussion (+CC Ido and Vladimir).
Switchdev folks might have more specific requirements and restrictions, so it'd be nice to get
their input as well.