Date:   Thu, 17 Mar 2022 12:30:51 +0200
From:   Nikolay Aleksandrov <razor@...ckwall.org>
To:     Joachim Wiberg <troglobit@...il.com>,
        Mattias Forsblad <mattias.forsblad@...il.com>,
        netdev@...r.kernel.org
Cc:     "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Roopa Prabhu <roopa@...dia.com>,
        Tobias Waldekranz <tobias@...dekranz.com>
Subject: Re: [PATCH 2/5] net: bridge: Implement bridge flood flag

On 17/03/2022 11:07, Joachim Wiberg wrote:
> On Thu, Mar 17, 2022 at 07:50, Mattias Forsblad <mattias.forsblad@...il.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.

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

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

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

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

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

const

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

Powered by blists - more mailing lists