Re: [PATCH RFC] net: bridge: handle ports in locked mode for ll learning

[Jonas Gorski]

On 12.12.24 13:25, Ido Schimmel wrote:
> On Thu, Dec 12, 2024 at 10:50:10AM +0100, Jonas Gorski wrote:
> > The original patch (just disabling LL learning if port is locked) has
> > the same issue as mine, it will indirectly break switchdev offloading
> > for case 2 when not using MAB (the kernel feature).
> >
> > Once we disable creating dynamic entries in the kernel, userspace needs
> > to create them,
>
> But the whole point of the "locked" feature is to defer the installation
> of FDB entries to user space so that the control plane will be able to
> decide which hosts can communicate through the bridge. Having the kernel
> auto-populate the FDB based on incoming packets defeats this purpose,
> which is why the man page mentions the "no_linklocal_learn" option and
> why I think there is a very low risk of regressions from the original
> patch.
>
> > and userspace dynamic entries have the user bit set, which makes them
> > get ignored by switchdev.
>
> The second use case never worked correctly in the offload case. It is
> not a regression.

Ah, I just noticed I incorrectly assumed that unlocking an entry just 
means removing the locked flag, but any change from user space also 
implicitly adds the user bit. So they would never get offloaded.

So there currently is no way to get offloaded dynamic entries with 
locked ports, regardless of MAB or not. And implementing that is 
definitely not a oneliner.

> > FWIW, my proposed change/fix would be:
> >
> > diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> > index ceaa5a89b947..41b69ea300bf 100644
> > --- a/net/bridge/br_input.c
> > +++ b/net/bridge/br_input.c
> > @@ -238,7 +238,8 @@ static void __br_handle_local_finish(struct sk_buff *skb)
> >   	    nbp_state_should_learn(p) &&
> >   	    !br_opt_get(p->br, BROPT_NO_LL_LEARN) &&
> >   	    br_should_learn(p, skb, &vid))
> > -		br_fdb_update(p->br, p, eth_hdr(skb)->h_source, vid, 0);
> > +		br_fdb_update(p->br, p, eth_hdr(skb)->h_source, vid,
> > +			      p->flags & BR_PORT_MAB ? BIT(BR_FDB_LOCKED) : 0);
>
> IIUC, this will potentially roam FDB entries to unauthorized ports,
> unlike the implementation in br_handle_frame_finish(). I documented it
> in commit a35ec8e38cdd ("bridge: Add MAC Authentication Bypass (MAB)
> support") in "1. Roaming".
Good point, missed that br_fdb_update() will automatically clear the 
locked flag on roaming regardless of flags including it.

So disregard that idea as well, and we would need to do what I 
originally proposed to allow locked learning via link local traffic.

But since there is no difference between kernel entries that userspace 
unlocked and dynamic entries userspace added as I now found out, it also 
isn't needed at all. Userspace can just add dynamic entries if needed.

So in conclusion, I agree with the original patch. Shall I resend it? 
Should I extend the commit message?

Best Regards,
Jonas

--
BISDN GmbH
Körnerstraße 7-10
10785 Berlin
Germany


Phone: 
+49-30-6108-1-6100


Managing Directors: 
Dr.-Ing. Hagen Woesner, Andreas 
Köpsel


Commercial register: 
Amtsgericht Berlin-Charlottenburg HRB 141569 
B
VAT ID No: DE283257294