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

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index c77591e63841..e33b2583e129 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)) {
-- 
2.46.0

On Wed, Sep 11, 2024 at 02:58:17PM +0200, 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.

Does 802.1d say anything about this?

Quoting the standard gives you a strong case for getting the patch
merged.

	Andrew

Hello Andrew,

Am 11.09.24 um 18:33 schrieb Andrew Lunn:
> On Wed, Sep 11, 2024 at 02:58:17PM +0200, 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.
> Does 802.1d say anything about this?
>
> Quoting the standard gives you a strong case for getting the patch
> merged.

I have 802.1q, the successor to 802.1d, at hand. It's a large document 
and I haven't read
all the details yet. From a coarse reading I get the impression that a 
bridge entity could
chose to filter such frames (based on "Filter Database" information 
which translates to our
BR_FDB_LOCAL flag), or forward to potential ports.

If we say we still want to forward these frames, that would probably be 
OK. The main
purpose of the patch is to avoid local processing of IGMP and other 
stuff. So at a minimum
we must avoid passing the frame up the stack and IGMP/MLD snooping code 
in addition
to the current handling (which is limited to avoiding to update the FDB).

However, I have a hard time to imagine legitimate cases of forwarding a 
frame again
that we obviously send out earlier. This is not helpful to our overall 
goal to stop storms when
customers manage to make loops in their networks (remember that we build 
routers
for end-user homes and loops are a constant source of problem reports).

Dropping on ingress is, in my opinion, also the better response to MAC 
spoofers on the network.

Best regards.