Date: Tue, 16 May 2023 11:56:41 +0300
From: Nikolay Aleksandrov <razor@...ckwall.org>
To: Johannes Nixdorf <jnixdorf-oss@....de>
Cc: netdev@...r.kernel.org, bridge@...ts.linux-foundation.org,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Roopa Prabhu <roopa@...dia.com>, Ido Schimmel <idosch@...dia.com>,
 Vladimir Oltean <vladimir.oltean@....com>
Subject: 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:
>> On 15/05/2023 11:50, 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_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 <jnixdorf-oss@....de>
>>> ---
>>>  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.

>> 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.

Powered by blists - more mailing lists