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

[Jonas Gorski]

Am Mi., 11. Dez. 2024 um 09:42 Uhr schrieb Ido Schimmel <idosch@xxxxxxxxxx>:
>
> On Tue, Dec 10, 2024 at 04:28:54PM +0100, Jonas Gorski wrote:
> > Thanks for the pointer. Reading the discussion, it seems this was
> > before the explicit BR_PORT_MAB option and locked learning support, so
> > there was some ambiguity around whether learning on locked ports is
> > desired or not, and this was needed(?) for the out-of-tree(?) MAB
> > implementation.
>
> There is a use case for learning on a locked port even without MAB. If
> user space is granting access via dynamic FDB entires, then you need
> learning enabled to refresh these entries.

AFAICT this would still work with my patch, as long learning is
enabled for the port. The difference would be that new dynamic entries
won't be created anymore from link local learning, so userspace would
now have to add them themselves. But any existing dynamic entries will
be refreshed via the normal input paths.

Though I see that this would break offloading these, since USER
dynamic entries are ignored in br_switchdev_fdb_notify() since
927cdea5d209 ("net: bridge: switchdev: don't notify FDB entries with
"master dynamic""). Side note, br_switchdev_fdb_replay() seems to
still pass them on. Do I miss something or shouldn't replay also need
to ignore/skip them?

> > But now that we do have an explicit flag for MAB, maybe this should be
> > revisited? Especially since with BR_PORT_MAB enabled, entries are
> > supposed to be learned as locked. But link local learned entries are
> > still learned unlocked. So no_linklocal_learn still needs to be
> > enabled for +locked, +learning, +mab.
>
> I mentioned this in the man page and added "no_linklocal_learn" to
> iproute2, but looks like it is not enough. You can try reposting the
> original patch (skip learning from link-local frames on a locked port)
> with a Fixes tag and see how it goes. I think it is unfortunate to
> change the behavior when there is already a dedicated knob for what you
> want to achieve, but I suspect the change will not introduce regressions
> so maybe people will find it acceptable.

Absolutely not your fault; my reference was the original cover letters
for BR_PORT_LOCKED and BR_PORT_MAB and reading br_input.c where the
flags are handled (not even looking at if_link.h's doc comments). And
there the constraint/side effect isn't mentioned anywhere, so I
assumed it was unintentional. And I never looked at any man pages,
just used bridge link help to find out what the arguments are to
(un)set those port flags. So I looked everywhere except where this
constraint is pointed out.

Anyway, I understand your concern about already having a knob to avoid
the issue, my concern here is that the knob isn't quite obvious, and
that you do need an additional knob to have a "secure" default. So
IMHO it's easy to miss as an inexperienced user. Though at least in
the !MAB case, disabling learning on the port is also enough to avoid
that (and keeps learning via link local enabled for unlocked ports).

At least in the case of having enabled BR_PORT_MAB, I would consider
it a bug that the entries learned via link local traffic aren't marked
as BR_FDB_LOCKED. If you agree, I can send in a reduced patch for
that, so that the entries are initially locked regardless the source
of learning.

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