On Wed, Dec 11, 2024 at 11:32:38AM +0100, Jonas Gorski wrote:
> 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.
I will give a bit of background so that my answer will make more sense.
AFAICT, there are three different ways to deploy 802.1X / MAB:
1. 802.1X with static FDB entries. In this case learning can be
disabled.
2. 802.1X with dynamic FDB entries. In this case learning needs to be
enabled so that entries will be refreshed by incoming traffic.
3. MAB. In this case learning needs to be enabled so that user space
will be notified about hosts that are trying to communicate through the
bridge.
When the original patch was posted I was not aware of the last two use
cases that require learning to be enabled.
In any scenario where you have +learning +locked (regardless of +/-mab)
you need to have +no_linklocal_learn for things to work correctly, so
the potential for regressions from the original patch seems low to me.
The original patch also provides a more comprehensive solution to the
problem than marking entries learned from link local traffic with
BR_FDB_LOCKED. It applies regardless of +/-mab (i.e., it covers both
cases 2 and 3 and not only 3). That is why I prefer the original patch
over the proposed approach.
