netdev - Re: [PATCH 1/3] net: bridge: Implement bridge flag local

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:   Tue, 01 Mar 2022 23:36:43 +0100
From:   Nikolay Aleksandrov <razor@...ckwall.org>
To:     Ido Schimmel <idosch@...sch.org>,
        Mattias Forsblad <mattias.forsblad@...il.com>
CC:     netdev@...r.kernel.org, "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>,
        Mattias Forsblad <mattias.forsblad+netdev@...il.com>
Subject: Re: [PATCH 1/3] net: bridge: Implement bridge flag local_receive

On 1 March 2022 17:43:27 CET, Ido Schimmel <idosch@...sch.org> wrote:
>On Tue, Mar 01, 2022 at 01:31:02PM +0100, Mattias Forsblad wrote:
>> This patch implements the bridge flag local_receive. When this
>> flag is cleared packets received on bridge ports will not be forwarded up.
>> This makes is possible to only forward traffic between the port members
>> of the bridge.
>> 
>> Signed-off-by: Mattias Forsblad <mattias.forsblad+netdev@...il.com>
>> ---
>>  include/linux/if_bridge.h      |  6 ++++++
>>  include/net/switchdev.h        |  2 ++
>
>Nik might ask you to split the offload part from the bridge
>implementation. Please wait for his feedback as he might be AFK right
>now
>

Indeed, I'm traveling and won't have pc access until end of week (Sun). 
I'll try to review the patches through my phoneas much as I can.
Ack on the split.

>>  include/uapi/linux/if_bridge.h |  1 +
>>  include/uapi/linux/if_link.h   |  1 +
>>  net/bridge/br.c                | 18 ++++++++++++++++++
>>  net/bridge/br_device.c         |  1 +
>>  net/bridge/br_input.c          |  3 +++
>>  net/bridge/br_ioctl.c          |  1 +
>>  net/bridge/br_netlink.c        | 14 +++++++++++++-
>>  net/bridge/br_private.h        |  2 ++
>>  net/bridge/br_sysfs_br.c       | 23 +++++++++++++++++++++++
>
>I believe the bridge doesn't implement sysfs for new attributes
>

Right, no new sysfs please.

>>  net/bridge/br_vlan.c           |  8 ++++++++
>>  12 files changed, 79 insertions(+), 1 deletion(-)
>
>[...]
>
>> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
>> index e0c13fcc50ed..5864b61157d3 100644
>> --- a/net/bridge/br_input.c
>> +++ b/net/bridge/br_input.c
>> @@ -163,6 +163,9 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>>  		break;
>>  	}
>>  
>> +	if (local_rcv && !br_opt_get(br, BROPT_LOCAL_RECEIVE))
>> +		local_rcv = false;
>> +
>
>I don't think the description in the commit message is accurate:
>"packets received on bridge ports will not be forwarded up". From the
>code it seems that if packets hit a local FDB entry, then they will be
>"forwarded up". Instead, it seems that packets will not be flooded
>towards the bridge. In which case, why not maintain the same granularity
>we have for the rest of the ports and split this into unicast /
>multicast / broadcast?
>

Exactly my first thought - why not implement the same control for the bridge?
Also try to minimize the fast-path hit, you can keep the needed changes 
localized only to the cases where they are needed.
I'll send a few more comments in a reply to the patch.

>BTW, while the patch honors local FDB entries, it overrides host MDB
>entries which seems wrong / inconsistent.
>
>>  	if (dst) {
>>  		unsigned long now = jiffies;