Ethernet Bridge development
 help / color / mirror / Atom feed
From: Thomas Martitz <tmartitz-oss@avm.de>
To: Nikolay Aleksandrov <razor@blackwall.org>,
	Roopa Prabhu <roopa@nvidia.com>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>
Cc: Johannes Nixdorf <jnixdorf-oss@avm.de>,
	bridge@lists.linux.dev, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH net-next] net: bridge: drop packets with a local source
Date: Thu, 19 Sep 2024 13:13:03 +0200	[thread overview]
Message-ID: <7aa4c66e-d0dc-452f-aebd-eb02a1b15a44@avm.de> (raw)
In-Reply-To: <934bf1f6-3f1c-4de4-be91-ba1913d1cb0e@blackwall.org>

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.

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.

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.

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.

Best regards.


> Cheers,
>   Nik
> 


  reply	other threads:[~2024-09-19 11:13 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-09-19  8:58 [RFC PATCH net-next] net: bridge: drop packets with a local source Thomas Martitz
2024-09-19 10:33 ` Nikolay Aleksandrov
2024-09-19 11:13   ` Thomas Martitz [this message]
2024-09-20  6:42     ` Nikolay Aleksandrov
2024-09-20 13:28       ` Thomas Martitz
2024-09-22 18:22         ` Nikolay Aleksandrov
2024-09-23  7:26           ` Thomas Martitz
2024-09-23 13:03             ` Nikolay Aleksandrov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7aa4c66e-d0dc-452f-aebd-eb02a1b15a44@avm.de \
    --to=tmartitz-oss@avm.de \
    --cc=bridge@lists.linux.dev \
    --cc=davem@davemloft.net \
    --cc=edumazet@google.com \
    --cc=jnixdorf-oss@avm.de \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    --cc=razor@blackwall.org \
    --cc=roopa@nvidia.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox