Re: [PATCH 1/3] net: bridge: Implement bridge flag local_receive

Mattias Forsblad
2022-03-02T06:27:37Z

On 2022-03-01 23:36, Nikolay Aleksandrov wrote:
> On 1 March 2022 17:43:27 CET, Ido Schimmel <idosch@idosch.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@gmail.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;
>