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 for Android: free password hash cracker in your pocket

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

Date:   Wed, 2 Mar 2022 07:27:25 +0100
From:   Mattias Forsblad <mattias.forsblad@...il.com>
To:     Nikolay Aleksandrov <razor@...ckwall.org>,
        Ido Schimmel <idosch@...sch.org>
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 2022-03-01 23:36, Nikolay Aleksandrov wrote:
> 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.
> 

I'll split the patch, thanks!

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

Ok, I wasn't aware of that. I'll drop that part, thanks!

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

Soo, if I understand you correctly, you want to have three different options?
local_receive_unicast
local_receive_multicast
local_receive_broadcast

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