Message-ID: <298aa961-5827-4fa9-bfdc-66267d08198c@blackwall.org>
Date: Sun, 22 Sep 2024 21:22:28 +0300
From: Nikolay Aleksandrov <razor@...ckwall.org>
To: Thomas Martitz <tmartitz-oss@....de>, Roopa Prabhu <roopa@...dia.com>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>
Cc: Johannes Nixdorf <jnixdorf-oss@....de>, bridge@...ts.linux.dev,
 netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH net-next] net: bridge: drop packets with a local
 source

On 9/20/24 16:28, Thomas Martitz wrote:
> Am 20.09.24 um 08:42 schrieb Nikolay Aleksandrov:
>> 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@....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.
>>
> 
> Alright. I'll approach this. It may take a little while because the LPC
> talks are so amazing that I don't want to miss anything.
> 
> I'm currently considering a per-bridge flag because that's fits our use
> case. A per-port flag would also work, though, and may fit the code
> there better because it's already checking for other port flags
> (BR_PORT_LOCKED, BR_LEARNING). Do you have a preference?
> 

Hi,
Sorry for the delayed response, but I was traveling over the weekend
and I got some more time to think about this. There is a more subtle
problem with this change - you're introducing packet filtering based on
fdb flags in the bridge, but it's not the bridge's job to filter
packets. We have filtering subsystems - netfilter, tc or ebpf, if they
lack some functionality you need to achieve this, then extend them.
Just because it's easy to hard-code this packet filter in the bridge
doesn't make it right, use the right subsystem if you want to filter.
For example you can extend nft's bridge matching capabilities.
More below.

> But on the performance topic: In our environment (home routers for
> end-users) the bridge ports are always in BR_LEARNING mode (and this is
> the default port mode). In this mode, I don't actually introduce an
> additional lookup. br_fdb_update() is currenty always called for the source
> MAC and in that function there is already the check for BR_FDB_LOCAL and
> the warning. I basically only added the drop as a result of that test. So
> when you are worried about an additional lookup, are you considering
> scenarios where BR_LEARNING is not set on the ports? I do wonder how
> common these are, I currently don't have a good feeling for that. I hope
> you can expand a bit on that and enlighten me.
> 
> If you prefer, I could also make a patch that limits drop to BR_LEARNING
> mode. I could extend br_fdb_update() to return an indication and make
> the drop conditional on that (after the existing call). Something
> like the below pseudo-code:
> 
> 	if (p->flags & BR_LEARNING) {
> 		if (br_fdb_update(br, p, eth_hdr(skb)->h_source, vid, 0) & BR_FDB_LOCAL)
> 			goto drop;
> 	}
> 

Oh no, this is worse.. definitely not.

> Although that would risk breaking existing weird set-ups. So unless you
> signal preference for this I will not persue that any further.
> 
> 
>>> 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.
> 
> I'm really trying to be open-minded about other possible ways, but I'm struggling.
> 
> For one, you know we're making a firmware for our home routers. We control all the
> code, from boot-loader to kernel to user space, so it's often both easier and more 
> reliable to make small modifications to the kernel than to come up with complex
> user space. In other words, we don't have any eBPF tooling in place currently and
> that would be a major disruption to our workflow. We don't even use LLVM, just GCC
> everywhere. I'd have to justify introducing all the eBPF tooling and processes (in
> order to avoid having a small patch to kernel) to my colleagues and my manager. I
> don't think that'd work out well. I'm pretty sure other companies in our field are
> in the same situation.

That really is your problem, it doesn't change the fact it can be solved
using eBPF or netfilter. I'm sorry but this is not an argument for this
mailing list or for accepting a patch. It really is a pretty simple
solution - take ipmonitor (from iproute2/ip), strip all and just look
for NEWLINK then act on master changes: on new master add port/mac to
table and vice-versa. What's so complex? You can also do it with
netfilter and nftables, just update a matching nft table on master
changes. Moreover these events are not usually many. In fact since you
control user-space entirely I'd add it to the enslave/release pieces in
whatever network management tools you're using, so when an interface is
enslaved its mac is added to that filter and removed when it's released,
then you won't need to have a constantly running process to monitor,
even simpler.

Actually it took me about 15 minutes to get a working solution to this
problem just by reusing the ipmonitor and iproute2 netlink code with a
nft table hooked on port's ingress. It is that simple, but I'd prefer to
do it in the network manager on port add/del and avoid monitoring
altogether.

> 
> Furthermore, from what I understand, an eBPF filter would not perform as good
> (performance also matters for us!) because there is no hook at this point. I'd need
> to hook earlier, perhaps using XDP (?), and that might have to process many more
> packets than those that enter the bridge. On the user space side, I'd need to have
> a daemon that update bpf maps or something like that to keep the list updated. I'm
> new to eBPF, so sorry if it seems more complex to me than it is.

It will process the same amount of packets that the bridge would.

> 
> For netfilter, I looked into that also, but the NF_BR_LOCAL_IN hook is too late. One
> of the biggest problems we try to solve is that looping IGMP packets enter the bridge
> and acually refresh MDBs that should normally timeout (we send JOINs for the addresses
> out but the MDB should only refresh when JOINs from other systems are received). Then,
> even if the filter location would fit, I'd effectively just re-implement the bridge's
> FDB lookup which rings bells that it's not an effective approach.
> 
> So both alternatives you projected are not a good fit to the actual problem and may
> require vastly more complex user space.

Is that all the research? You read 2 minutes of webpages and diagonally
scanned some source code, did you see the other bridge netfilter
hooks? You can extend netfilter and match in any of them if you insist
on having a kernel solution. For example match in NF_BR_PRE_ROUTING.
You can extend nft's bridge support and match anything you need.

> 
> So I did consider alternatives, however making the check that's already there also
> drop the packets, is the most effective solution to our problems from my point of
> view.
> 

Again just because it's easy to hardcode the filter there, doesn't make
it right. If you want to filter then either extend one of the filtering
subsystems or do a hybrid solution.

> 
>>
>>> 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.
> 
> I understand that. I hope that new options are still possible if they're justified.
> 

This is not justified because the bridge is not about packet filtering
and hardcoding a packet filter in it is not ok.

Cheers,
 Nik

>>
>>> 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.
> 
> Too bad. I hope we get a chance on another conference. I first need to convince
> my managers that this trip was useful use of the company's resources, though!
> 
> Best regards,
> Thomas Martitz
> 
>>
>> Cheers,
>>   Nik
>>
>>> Best regards.
>>>
>>>
>>>> Cheers,
>>>>    Nik
>>>>
>>>
>>
>>
> 

Powered by blists - more mailing lists