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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 17 Mar 2022 13:42:55 +0200
From:   Nikolay Aleksandrov <razor@...ckwall.org>
To:     Mattias Forsblad <mattias.forsblad@...il.com>,
        Joachim Wiberg <troglobit@...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>,
        Vladimir Oltean <olteanv@...il.com>
Subject: Re: [PATCH 2/5] net: bridge: Implement bridge flood flag

On 17/03/2022 13:39, Mattias Forsblad wrote:
> On 2022-03-17 10: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.
>>
>>> [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.
> 
> The original flag was name was local_receive to separate it from being
> mistaken for the flood unknown flags. However the comment I've got was
> to align it with the existing (port) flags. These flags have nothing to do with
> the port flood unknown flags. Imagine the setup below:
> 
>            vlan1
>              |
>             br0             br1
>            /   \           /   \
>          swp1 swp2       swp3 swp4
> 
> We want to have swp1/2 as member of a normal vlan filtering bridge br0 /w learning on. 
> On br1 we want to just forward packets between swp3/4 and disable learning. 
> Additional we don't want this traffic to impact the CPU. 
> If we disable learning on swp3/4 all traffic will be unknown and if we also 
> have flood unknown on the CPU-port because of requirements for br0 it will
> impact the traffic to br1. Thus we want to restrict traffic between swp3/4<->CPU port
> with the help of the PVT.
> 
> /Mattias

The feedback was correct and we all assumed unknown traffic control.
If you don't want any local receive then use filtering rules. Don't add unnecessary flags.