Linux Ethernet Bridging - Nov 2022 - [Bridge] [RFC PATCH net-next 10/16] mlxsw: spectrum_switchdev: Add support for locked FDB notifications

On Thu, Oct 27, 2022 at 11:39:40PM +0000, Vladimir Oltean
wrote:
> On Tue, Oct 25, 2022 at 01:00:18PM +0300, Ido Schimmel wrote:
> > In Spectrum, learning happens in parallel to the security checks.
> > Therefore, regardless of the result of the security checks, a learning
> > notification will be generated by the device and polled later on by
the
> > driver.
> > 
> > Currently, the driver reacts to learning notifications by programming
> > corresponding FDB entries to the device. When a port is locked (i.e.,
> > has security checks enabled), this can no longer happen, as otherwise
> > any host will blindly gain authorization.
> > 
> > Instead, notify the learned entry as a locked entry to the bridge
driver
> > that will in turn notify it to user space, in case MAB is enabled.
User
> > space can then decide to authorize the host by clearing the
"locked"
> > flag, which will cause the entry to be programmed to the device.
> > 
> > Signed-off-by: Ido Schimmel <idosch at nvidia.com>
> > ---
> 
> So for mlxsw, the hardware/driver always gets learning notifications
> if learning is enabled (and regardless of MAB being enabled; with the
> mention that BR_PORT_MAB implies BR_LEARNING and so, with MAB, these
> notifications always come), and the driver always calls
SWITCHDEV_FDB_ADD_TO_BRIDGE,
> letting the bridge figure out if it should create a BR_FDB_LOCKED entry
> or to throw the notification away?
Yes, correct.
> 
> Hans' case is different; he needs to configure the HW differently
> (MAB is more resource intensive). I suppose at some point, in his patch
> series, he will need to also offload BR_PORT_MAB, something which you
> didn't need. Ok.
> 
> The thing is that it will become tricky to know, when adding BR_PORT_MAB
> to BR_PORT_FLAGS_HW_OFFLOAD, which drivers can offload MAB and which
> can't, without some prior knowledge. For example, Hans will need to
> patch mlxsw_sp_port_attr_br_pre_flags_set() to not reject BR_PORT_MAB,
> even if mlxsw will need to do nothing based on the flag, right?
Right. I'm quite reluctant to add the MAB flag to
BR_PORT_FLAGS_HW_OFFLOAD as part of this patchset for the simple reason
that it is not really needed. I'm not worried about someone adding it
later when it is actually needed. We will probably catch the omission
during code review. Worst case, we have a selftest that will break,
notifying us that a bug fix is needed.

On Sun, Oct 30, 2022 at 10:23:07AM +0200, Ido Schimmel
wrote:
> Right. I'm quite reluctant to add the MAB flag to
> BR_PORT_FLAGS_HW_OFFLOAD as part of this patchset for the simple reason
> that it is not really needed. I'm not worried about someone adding it
> later when it is actually needed. We will probably catch the omission
> during code review. Worst case, we have a selftest that will break,
> notifying us that a bug fix is needed.
For drivers which don't emit SWITCHDEV_FDB_ADD_TO_BRIDGE but do offload
BR_PORT_LOCKED (like mv88e6xxx), things will not work correctly on day 1
of BR_PORT_MAB because they are not told MAB is enabled, so they have no
way of rejecting it until things work properly with the offload in place.

It's the same reason for which we have BR_HAIRPIN_MODE | BR_ISOLATED |
BR_MULTICAST_TO_UNICAST in BR_PORT_FLAGS_HW_OFFLOAD, even if nobody acts
upon them.

Hi Ido,

On Mon, Oct 31, 2022 at 10:32:10AM +0200, Vladimir Oltean
wrote:
> On Sun, Oct 30, 2022 at 10:23:07AM +0200, Ido Schimmel wrote:
> > Right. I'm quite reluctant to add the MAB flag to
> > BR_PORT_FLAGS_HW_OFFLOAD as part of this patchset for the simple
reason
> > that it is not really needed. I'm not worried about someone adding
it
> > later when it is actually needed. We will probably catch the omission
> > during code review. Worst case, we have a selftest that will break,
> > notifying us that a bug fix is needed.
> 
> For drivers which don't emit SWITCHDEV_FDB_ADD_TO_BRIDGE but do offload
> BR_PORT_LOCKED (like mv88e6xxx), things will not work correctly on day 1
> of BR_PORT_MAB because they are not told MAB is enabled, so they have no
> way of rejecting it until things work properly with the offload in place.
> 
> It's the same reason for which we have BR_HAIRPIN_MODE | BR_ISOLATED |
> BR_MULTICAST_TO_UNICAST in BR_PORT_FLAGS_HW_OFFLOAD, even if nobody acts
> upon them.
Do you have any comment on this? You resent the BR_PORT_MAB patches
without even an ack that yes, mv88e6xxx will not support MAB being
enabled on a bridge port, and will not reject the configuration either,
and that's ok/intended.

Do you think this is not true? Irrelevant? The "fix" (to implement
offloading)
might come in this development cycle, or it might not.