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@xxxxxx> >>> --- >>> 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 >> >
