| Message ID | 20240919085803.105430-1-tmartitz-oss@avm.de (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC,net-next] net: bridge: drop packets with a local source | expand |
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. Cheers, Nik
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 >
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 >> >
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@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. > 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? 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; } 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. 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. 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. 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. > >> 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. > >> 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 >>> >> > >
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@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. >> > > 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 >>>> >>> >> >> >
Am 22.09.24 um 20:22 schrieb Nikolay Aleksandrov: > 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@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. >>> >> >> 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. Hi, Alright, I understand that you basically object to the whole idea of filtering in the bridge code directly (based on fdb flags). While that makes some sense, I found that basically the same filter that I already exists for mac802.11 use cases: } else if (READ_ONCE(fdb_src->dst) != p || test_bit(BR_FDB_LOCAL, &fdb_src->flags)) { <-- drop if local source on ingress /* FDB mismatch. Drop the packet without roaming. */ goto drop; In fact, this very code motivated me because I'm just adding one more condition to an existing drop mechanism after all. In this area there are also further drops based on fdb flags. Anyway, dropping isn't actually my main intent, although it's a welcome side effect because it immediately stops loops. What I'm after most is avoiding local proccessing, both in the IGMP/MLD snooping code and up in the stack. In my opinion, it would be good if the bridge code can be more resilient against loops (and spoofers) by not processing its own packets as if it came from somebody else. My main issue is: the IGMP/MLD snooping code becomes convinced that there are subscribers on the network even if here aren't, just by processing IGMP/MLD joins that were send out a moment ago. That said, we could still decide to forward these packets and not filter them completely. And I still think that should also be the default, especially if we block only local processing but not forwarding. You don't feel this robustness is not necessary (or consider the performance impact too high) then I accept that and withdraw my proposal. I just thought it would be a useful addition to the bridge's out-of-the-box stability. All that said, I'll explore a netfilter solution (see below) to avoid maintaing out-of-tree patches. > > >> 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. Impressive! > >> >> 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. If we add vlan devices the tagged packets that don't enter the bridge would still be processed by eBPF. On top, we have also a custom hook that may consume packets for other reasons before they enter the bridge. But that's not your problem. > >> >> 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. Thank you very much for this! I literally looked for NF_HOOK in br_input.c to find suitable entry points and the NF_BR_PRE_ROUTING hook didn't occur to me (it's handled differently). I really should have looked more carefully over the entire file. Also, I should have known it anyway, I'm working with the bridge code for quite a long time already and considered myself experienced in this area (have to reconsider this now...). So sorry for my incomplete research, NF_BR_PRE_ROUTING seems like a nice fit actually. I'll explore this further, and assuming this works, we can drop my proposal altogether. From a first look it should work, altough we wouldn't be able to just block out local processing (can just have drop or not) if need arises and we have to re-implement MAC lookup. I have to apologize for wasting your time but at least I have lerned a lesson. Best regards.
On 9/23/24 10:26, Thomas Martitz wrote: > Am 22.09.24 um 20:22 schrieb Nikolay Aleksandrov: >> 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@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. >>>> >>> >>> 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. > > Hi, > > Alright, I understand that you basically object to the whole idea of > filtering in the bridge code directly (based on fdb flags). While that > makes some sense, I found that basically the same filter that I already > exists for mac802.11 use cases: > > } else if (READ_ONCE(fdb_src->dst) != p || > test_bit(BR_FDB_LOCAL, &fdb_src->flags)) { <-- drop if local source on ingress > /* FDB mismatch. Drop the packet without roaming. */ > goto drop; > > In fact, this very code motivated me because I'm just adding one more > condition to an existing drop mechanism after all. In this area there > are also further drops based on fdb flags. > I was expecting you'd bring this code up :) but you've also taken it out of context. It is a part of a larger feature which also creates locked fdbs, and enforces certain policies which give user-space a chance to authenticate user macs, i.e. MAC Authentication Bypass. Creating and managing such fdbs can only be done from the bridge but also with the help of user-space. > Anyway, dropping isn't actually my main intent, although it's a welcome > side effect because it immediately stops loops. What I'm after most is > avoiding local proccessing, both in the IGMP/MLD snooping code and up in > the stack. In my opinion, it would be good if the bridge code can be more > resilient against loops (and spoofers) by not processing its own packets > as if it came from somebody else. My main issue is: the IGMP/MLD snooping > code becomes convinced that there are subscribers on the network even if > here aren't, just by processing IGMP/MLD joins that were send out a moment > ago. That said, we could still decide to forward these packets and not > filter them completely. > > And I still think that should also be the default, especially if we block > only local processing but not forwarding. You don't feel this robustness > is not necessary (or consider the performance impact too high) then I > accept that and withdraw my proposal. I just thought it would be a useful > addition to the bridge's out-of-the-box stability. > You can decide to filter, I don't mind, but do it within the filtering subsystems and not in the bridge. This is better for everyone - interested parties would take the performance hit, but also you get more flexibility and matching opportunities, tomorrow you might decide to have more complex criteria. > All that said, I'll explore a netfilter solution (see below) to avoid > maintaing out-of-tree patches. > > >> >> >>> 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. > > > Impressive! > > >> >>> >>> 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. > > > If we add vlan devices the tagged packets that don't enter the bridge would still be > processed by eBPF. > This processing could come down to a simple conditional statement depending on how much vlan filtering you need. You wouldn't notice its impact if implemented properly. > On top, we have also a custom hook that may consume packets for other reasons before > they enter the bridge. But that's not your problem. > > >> >>> >>> 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. > > > Thank you very much for this! I literally looked for NF_HOOK in br_input.c > to find suitable entry points and the NF_BR_PRE_ROUTING hook didn't occur > to me (it's handled differently). I really should have looked more carefully > over the entire file. > > Also, I should have known it anyway, I'm working with the bridge code for > quite a long time already and considered myself experienced in this area > (have to reconsider this now...). > > So sorry for my incomplete research, NF_BR_PRE_ROUTING seems like a nice > fit actually. I'll explore this further, and assuming this works, we can > drop my proposal altogether. From a first look it should work, altough we > wouldn't be able to just block out local processing (can just have drop or > not) if need arises and we have to re-implement MAC lookup. > > I have to apologize for wasting your time but at least I have lerned a lesson. > > Best regards. No need to apologize, it's good to have these discussions and to make things clearer. Cheers, Nik
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c index ad7a42b505ef..f97203c56394 100644 --- a/net/bridge/br_fdb.c +++ b/net/bridge/br_fdb.c @@ -900,9 +900,7 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source, if (likely(fdb)) { /* attempt to update an entry for a local interface */ if (unlikely(test_bit(BR_FDB_LOCAL, &fdb->flags))) { - if (net_ratelimit()) - br_warn(br, "received packet on %s with own address as source address (addr:%pM, vlan:%u)\n", - source->dev->name, addr, vid); + return; } else { unsigned long now = jiffies; bool fdb_modified = false; diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c index ceaa5a89b947..06db92d03dd3 100644 --- a/net/bridge/br_input.c +++ b/net/bridge/br_input.c @@ -77,7 +77,7 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb { struct net_bridge_port *p = br_port_get_rcu(skb->dev); enum br_pkt_type pkt_type = BR_PKT_UNICAST; - struct net_bridge_fdb_entry *dst = NULL; + struct net_bridge_fdb_entry *fdb_src, *dst = NULL; struct net_bridge_mcast_port *pmctx; struct net_bridge_mdb_entry *mdst; bool local_rcv, mcast_hit = false; @@ -108,10 +108,14 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb &state, &vlan)) goto out; - if (p->flags & BR_PORT_LOCKED) { - struct net_bridge_fdb_entry *fdb_src = - br_fdb_find_rcu(br, eth_hdr(skb)->h_source, vid); - + fdb_src = br_fdb_find_rcu(br, eth_hdr(skb)->h_source, vid); + if (fdb_src && test_bit(BR_FDB_LOCAL, &fdb_src->flags)) { + /* Spoofer or short-curcuit on the network. Drop the packet. */ + if (net_ratelimit()) + br_warn(br, "received packet on %s with own address as source address (addr:%pM, vlan:%u)\n", + p->dev->name, eth_hdr(skb)->h_source, vid); + goto drop; + } else if (p->flags & BR_PORT_LOCKED) { if (!fdb_src) { /* FDB miss. Create locked FDB entry if MAB is enabled * and drop the packet. @@ -120,8 +124,7 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb br_fdb_update(br, p, eth_hdr(skb)->h_source, vid, BIT(BR_FDB_LOCKED)); goto drop; - } else if (READ_ONCE(fdb_src->dst) != p || - test_bit(BR_FDB_LOCAL, &fdb_src->flags)) { + } else if (READ_ONCE(fdb_src->dst) != p) { /* FDB mismatch. Drop the packet without roaming. */ goto drop; } else if (test_bit(BR_FDB_LOCKED, &fdb_src->flags)) {
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(-)