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
