netdev - Re: [PATCH RFC net-next 01/13] net: bridge: add control of bum flooding to bridge itself

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]

Message-ID: <d88e8591-14fd-de90-ced7-b372c8fe81e5@blackwall.org>
Date:   Tue, 12 Apr 2022 23:29:48 +0300
From:   Nikolay Aleksandrov <razor@...ckwall.org>
To:     Joachim Wiberg <troglobit@...il.com>,
        Roopa Prabhu <roopa@...dia.com>
Cc:     netdev@...r.kernel.org, bridge@...ts.linux-foundation.org,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Tobias Waldekranz <tobias@...dekranz.com>,
        Vladimir Oltean <vladimir.oltean@....com>
Subject: Re: [PATCH RFC net-next 01/13] net: bridge: add control of bum
 flooding to bridge itself

On 12/04/2022 21:27, Nikolay Aleksandrov wrote:
> On 11/04/2022 16:38, Joachim Wiberg wrote:
>> The bridge itself is also a port, but unfortunately it does not (yet)
>> have a 'struct net_bridge_port'.  However, in many cases we want to
>> treat it as a proper port so concessions have been made, e.g., NULL
>> port or host_joined attributes.
>>
>> This patch is an attempt to more of the same by adding support for
>> controlling flooding of unknown broadcast/unicast/multicast to the
>> bridge.  Something we often also want to control in an offloaded
>> switching fabric.
>>
>> Signed-off-by: Joachim Wiberg <troglobit@...il.com>
>> ---
>>  net/bridge/br_device.c  |  4 ++++
>>  net/bridge/br_input.c   | 11 ++++++++---
>>  net/bridge/br_private.h |  3 +++
>>  3 files changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
>> index 8d6bab244c4a..0aa7d21ac82c 100644
>> --- a/net/bridge/br_device.c
>> +++ b/net/bridge/br_device.c
>> @@ -526,6 +526,10 @@ void br_dev_setup(struct net_device *dev)
>>  	br->bridge_ageing_time = br->ageing_time = BR_DEFAULT_AGEING_TIME;
>>  	dev->max_mtu = ETH_MAX_MTU;
>>  
>> +	br_opt_toggle(br, BROPT_UNICAST_FLOOD, 1);
> 
> This one must be false by default. It changes current default behaviour.
> Unknown unicast is not currently passed up to the bridge if the port is

s/port/bridge/ in promisc mode

> not in promisc mode, this will change it. You'll have to make it consistent
> with promisc (e.g. one way would be for promisc always to enable unicast flood
> and it won't be possible to be disabled while promisc).
> 
>> +	br_opt_toggle(br, BROPT_MCAST_FLOOD, 1);
>> +	br_opt_toggle(br, BROPT_BCAST_FLOOD, 1);
> 
> s/1/true/ for consistency
> 
>> +
>>  	br_netfilter_rtable_init(br);
>>  	br_stp_timer_init(br);
>>  	br_multicast_init(br);
>> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
>> index 196417859c4a..d439b876bdf5 100644
>> --- a/net/bridge/br_input.c
>> +++ b/net/bridge/br_input.c
>> @@ -118,7 +118,8 @@ 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;
>> +			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))
>> @@ -161,12 +162,16 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>>  			}
>>  			mcast_hit = true;
>>  		} else {
>> -			local_rcv = true;
>> -			br->dev->stats.multicast++;
>> +			if (br_opt_get(br, BROPT_MCAST_FLOOD)) {
>> +				local_rcv = true;
>> +				br->dev->stats.multicast++;
>> +			}
>>  		}
>>  		break;
>>  	case BR_PKT_UNICAST:
>>  		dst = br_fdb_find_rcu(br, eth_hdr(skb)->h_dest, vid);
>> +		if (!dst && br_opt_get(br, BROPT_UNICAST_FLOOD))
>> +			local_rcv = true;
>>  		break;
> 
> This adds new tests for all fast paths for host traffic,
> especially the port - port communication which is the most critical one.
> Please at least move the unicast test to the "else" block of "if (dst)" later.
> 
> The other tests can be moved to host only code too, but would require bigger changes.
> Please try to keep the impact on the fast-path at minimum.
> 
>>  	default:
>>  		break;
>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>> index 18ccc3d5d296..683bd0ee4c64 100644
>> --- a/net/bridge/br_private.h
>> +++ b/net/bridge/br_private.h
>> @@ -449,6 +449,9 @@ enum net_bridge_opts {
>>  	BROPT_VLAN_BRIDGE_BINDING,
>>  	BROPT_MCAST_VLAN_SNOOPING_ENABLED,
>>  	BROPT_MST_ENABLED,
>> +	BROPT_UNICAST_FLOOD,
>> +	BROPT_MCAST_FLOOD,
>> +	BROPT_BCAST_FLOOD,
>>  };
>>  
>>  struct net_bridge {
>