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

On 19/09/2024 14:13, Thomas Martitz wrote:

Am 19.09.24 um 12:33 schrieb Nikolay Aleksandrov:

quoted

On 19/09/2024 11:58, Thomas Martitz wrote:

quoted

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

quoted

Cheers,
  Nik