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

[Ido Schimmel <idosch@nvidia.com>]

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.

> 
> Ofc enabling MAB and then unlocking the locked entries hosts that
> successfully authenticated should still work for 2, as long as the host
> sent something other than link local traffic to create a (locked)
> dynamic entry AFAIU.
> 
> 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".

>  }
>  
>  /* note: already called with rcu_read_lock */
> 
> which just makes sure that when MAB is enabled, link local learned
> entries are also locked. This relies on br_fdb_update() ignoring most
> flags for existing entries, not sure if this is a good idea though.
> 
> 
> 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
>