| 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 >
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(-)