On 17/03/2022 11:07, Joachim Wiberg wrote:

On Thu, Mar 17, 2022 at 07:50, Mattias Forsblad [off-list ref] wrote:

quoted

This patch implements the bridge flood flags. There are three different
flags matching unicast, multicast and broadcast. When the corresponding
flag is cleared packets received on bridge ports will not be flooded
towards the bridge.

If I've not completely misunderstood things, I believe the flood and
mcast_flood flags operate on unknown unicast and multicast.  With that
in mind I think the hot path in br_input.c needs a bit more eyes.  I'll
add my own comments below.

Happy incident I saw this patch set, I have a very similar one for these
flags to the bridge itself, with the intent to improve handling of all
classes of multicast to/from the bridge itself.

+1

I'll add my comments below, yours are pretty spot on. I have one more
that's for the snipped part, I'll send that separately. :)

First please split this into 3 separate patches - one for each flag.
That would make reviewing much easier.

quoted

quoted

[snip]

diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index e0c13fcc50ed..fcb0757bfdcc 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c

@@ -109,11 +109,12 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 		/* by definition the broadcast is also a multicast address */
 		if (is_broadcast_ether_addr(eth_hdr(skb)->h_dest)) {
 			pkt_type = BR_PKT_BROADCAST;
-			local_rcv = true;
+			local_rcv = true && br_opt_get(br, BROPT_BCAST_FLOOD);

Minor comment, I believe the preferred style is more like this:

	if (br_opt_get(br, BROPT_BCAST_FLOOD))
        	local_rcv = true;

ack

quoted

 		} else {
 			pkt_type = BR_PKT_MULTICAST;
-			if (br_multicast_rcv(&brmctx, &pmctx, vlan, skb, vid))
-				goto drop;
+			if (br_opt_get(br, BROPT_MCAST_FLOOD))
+				if (br_multicast_rcv(&brmctx, &pmctx, vlan, skb, vid))
+					goto drop;

Since the BROPT_MCAST_FLOOD flag should only control uknown multicast,
we cannot bypass the call to br_multicast_rcv(), which helps with the
classifcation.  E.g., we want IGMP/MLD reports to be forwarded to all
router ports, while the mdb lookup (below) is what an tell us if we
have uknown multicast and there we can check the BROPT_MCAST_FLOOD
flag for the bridge itself.

+1

quoted

 		}
 	}
 

@@ -155,9 +156,13 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 			local_rcv = true;
 			br->dev->stats.multicast++;
 		}
+		if (!br_opt_get(br, BROPT_MCAST_FLOOD))
+			local_rcv = false;

We should never set local_rcv to false, only ever use constructs that
set it to true.  Here the PROMISC flag (above) condition would be
negated, which would be a regression.

Instead, for multicast I believe we should ensure that we reach the
else statement for unknown IP multicast, preventing mcast_hit from
being set, and instead flood unknown multicast using br_flood().

This is a bigger change that involves:

  1) dropping the mrouters_only skb flag for unknown multicast,
     keeping it only for IGMP/MLD reports
  2) extending br_flood() slightly to flood unknown multicast
     also to mcast_router ports

As I mentioned above, I have some patches, including selftests, for
forwarding known/unknown multicast using the mdb and mcast_flood +
mcast_router flags.  Maybe we should combine efforts here somehow?

Ack, sounds good! Please coordinate that between yourselves, if the mcast
flood flag will be dropped from this set or if Joachim's will be integrated.

quoted

 		break;
 	case BR_PKT_UNICAST:
 		dst = br_fdb_find_rcu(br, eth_hdr(skb)->h_dest, vid);
+		if (!br_opt_get(br, BROPT_FLOOD))
+			local_rcv = false;

Again, never set it to false, invert the check instead, like this:

		if (!dst && br_opt_get(br, BROPT_FLOOD))
			local_rcv = true;

quoted

 		break;
 	default:
 		break;

@@ -166,7 +171,7 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 	if (dst) {
 		unsigned long now = jiffies;
 
-		if (test_bit(BR_FDB_LOCAL, &dst->flags))
+		if (test_bit(BR_FDB_LOCAL, &dst->flags) && local_rcv)
 			return br_pass_frame_up(skb);

I believe this would break both the flooding of unknown multicast and
the PROMISC case.  Down here we are broadcast or known/unknown multicast
land, so the local_rcv flag should be sufficient.

quoted

 		if (now != dst->used)

@@ -190,6 +195,16 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 }
 EXPORT_SYMBOL_GPL(br_handle_frame_finish);
 
+bool br_flood_enabled(const struct net_device *dev)
+{
+	struct net_bridge *br = netdev_priv(dev);

const

quoted

+
+	return !!(br_opt_get(br, BROPT_FLOOD) ||
+		   br_opt_get(br, BROPT_MCAST_FLOOD) ||
+		   br_opt_get(br, BROPT_BCAST_FLOOD));

Minor nit, don't know what the rest of the list feels about this, but
maybe the BROPT_FLOOD option should be renamed to BR_UCAST_FLOOD or
BR_UNICAST_FLOOD?

Exactly, very good suggestion. Unfortunately we already have BR_FLOOD and can't
do anything about it, but we shouldn't follow that bad example.

Best regards
 /Joachim

Cheers,
 Nik