Re: [RFC PATCH net-next] net: bridge: drop packets with a local source

[Nikolay Aleksandrov <razor@blackwall.org>]

On 19/09/2024 14:13, Thomas Martitz wrote:
> Am 19.09.24 um 12:33 schrieb Nikolay Aleksandrov:
>> On 19/09/2024 11:58, Thomas Martitz wrote:
>>> Currently, there is only a warning if a packet enters the bridge
>>> that has the bridge's or one port's MAC address as source.
>>>
>>> Clearly this indicates a network loop (or even spoofing) so we
>>> generally do not want to process the packet. Therefore, move the check
>>> already done for 802.1x scenarios up and do it unconditionally.
>>>
>>> For example, a common scenario we see in the field:
>>> In a accidental network loop scenario, if an IGMP join
>>> loops back to us, it would cause mdb entries to stay indefinitely
>>> even if there's no actual join from the outside. Therefore
>>> this change can effectively prevent multicast storms, at least
>>> for simple loops.
>>>
>>> Signed-off-by: Thomas Martitz <tmartitz-oss@avm.de>
>>> ---
>>>   net/bridge/br_fdb.c   |  4 +---
>>>   net/bridge/br_input.c | 17 ++++++++++-------
>>>   2 files changed, 11 insertions(+), 10 deletions(-)
>>>
>>
>> Absolutely not, I'm sorry but we're not all going to take a performance hit
>> of an additional lookup because you want to filter src address. You can filter
>> it in many ways that won't affect others and don't require kernel changes
>> (ebpf, netfilter etc). To a lesser extent there is also the issue where we might
>> break some (admittedly weird) setup.
>>
> 
> Hello Nikolay,
> 
> thanks for taking a look at the patch. I expected concerns, therefore the RFC state.
> 
> So I understand that performance is your main concern. Some users might
> be willing to pay for that cost, however, in exchange for increased
> system robustness. May I suggest per-bridge or even per-port flags to
> opt-in to this behavior? We'd set this from our userspace. This would
> also address the concern to not break weird, existing setups.
> 

That is the usual way these things are added, as opt-in. A flag sounds good
to me, if you're going to make it per-bridge take a look at the bridge bool
opts, they were added for such cases.

> This would be analogous to the check added for MAB in 2022
> (commit a35ec8e38cdd "bridge: Add MAC Authentication Bypass (MAB) support").
> 
> While there are maybe other methods, only in the bridge code I may
> access the resulting FDB to test for the BR_FDB_LOCAL flag. There's
> typically not only a single MAC adress to check for, but such a local
> FDB is maintained for the enslaved port's MACs as well. Replicating
> the check outside of the bridge receive code would be orders more
> complex. For example, you need to update the filter each time a port is
> added or removed from the bridge.
> 

That is not entirely true, you can make a solution that dynamically compares
the mac addresses of net devices with src mac of incoming frames, you may need
to keep a list of the ports themselves or use ebpf though. It isn't complicated
at all, you just need to keep that list updated when adding/removing ports
you can even do it with a simple ip monitor and a bash script as a poc, there's nothing
complicated about it and we won't have to maintain another bridge option forever.

> Since a very similar check exists already using a per-port opt-in flag,
> would a similar approach acceptable for you? If yes, I'd send a
> follow-up shortly.
> 

Yeah, that would work although I try to limit the new options as the bridge
has already too many options.

> PS: I haven't spottet you, but in case you're at LPC in Vienna we can
> chat in person about it, I'm here.
> 

That would've been nice, but unfortunately I couldn't make it this year.

Cheers,
 Nik

> Best regards.
> 
> 
>> Cheers,
>>   Nik
>>
>