Re: [PATCH 2/5] net: bridge: Implement bridge flood flag

[Joachim Wiberg <troglobit@gmail.com>]

On Thu, Mar 17, 2022 at 07:50, Mattias Forsblad <mattias.forsblad@gmail.com> wrote:
> 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.

> [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;

>  		} 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.

>  		}
>  	}
>  
> @@ -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?

>  		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;

>  		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.

>  		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);
> +
> +	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?

Best regards
 /Joachim