| Message ID | 20221025100024.1287157-11-idosch@nvidia.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | bridge: Add MAC Authentication Bypass (MAB) support with offload | expand |
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@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? 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?
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@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.
On Thu, Nov 03, 2022 at 10:31:52PM +0000, Vladimir Oltean wrote: > 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? Sorry, forgot to reply... I added a patch (see below) to the offload set. If the bridge patches are accepted and we have disagreements on the offload part I can always split out this patch and send it separately so that mv88e6xxx rejects MAB in 6.2. commit ebdd7363f8c1802af63c35f74d6922b727617a7d Author: Ido Schimmel <idosch@nvidia.com> Date: Mon Oct 31 19:36:36 2022 +0200 bridge: switchdev: Reflect MAB bridge port flag to device drivers Reflect the 'BR_PORT_MAB' flag to device drivers so that: * Drivers that support MAB could act upon the flag being toggled. * Drivers that do not support MAB will prevent MAB from being enabled. Signed-off-by: Ido Schimmel <idosch@nvidia.com> Notes: v1: * New patch. diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c index 8a0abe35137d..7eb6fd5bb917 100644 --- a/net/bridge/br_switchdev.c +++ b/net/bridge/br_switchdev.c @@ -71,7 +71,7 @@ bool nbp_switchdev_allowed_egress(const struct net_bridge_port *p, } /* Flags that can be offloaded to hardware */ -#define BR_PORT_FLAGS_HW_OFFLOAD (BR_LEARNING | BR_FLOOD | \ +#define BR_PORT_FLAGS_HW_OFFLOAD (BR_LEARNING | BR_FLOOD | BR_PORT_MAB | \ BR_MCAST_FLOOD | BR_BCAST_FLOOD | BR_PORT_LOCKED | \ BR_HAIRPIN_MODE | BR_ISOLATED | BR_MULTICAST_TO_UNICAST)
On Fri, Nov 04, 2022 at 12:54:39AM +0200, Ido Schimmel wrote: > Sorry, forgot to reply... I added a patch (see below) to the offload > set. If the bridge patches are accepted and we have disagreements on the > offload part I can always split out this patch and send it separately so > that mv88e6xxx rejects MAB in 6.2. > > commit ebdd7363f8c1802af63c35f74d6922b727617a7d > Author: Ido Schimmel <idosch@nvidia.com> > Date: Mon Oct 31 19:36:36 2022 +0200 > > bridge: switchdev: Reflect MAB bridge port flag to device drivers > > Reflect the 'BR_PORT_MAB' flag to device drivers so that: > > * Drivers that support MAB could act upon the flag being toggled. > * Drivers that do not support MAB will prevent MAB from being enabled. > > Signed-off-by: Ido Schimmel <idosch@nvidia.com> > > Notes: > v1: > * New patch. > > diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c > index 8a0abe35137d..7eb6fd5bb917 100644 > --- a/net/bridge/br_switchdev.c > +++ b/net/bridge/br_switchdev.c > @@ -71,7 +71,7 @@ bool nbp_switchdev_allowed_egress(const struct net_bridge_port *p, > } > > /* Flags that can be offloaded to hardware */ > -#define BR_PORT_FLAGS_HW_OFFLOAD (BR_LEARNING | BR_FLOOD | \ > +#define BR_PORT_FLAGS_HW_OFFLOAD (BR_LEARNING | BR_FLOOD | BR_PORT_MAB | \ > BR_MCAST_FLOOD | BR_BCAST_FLOOD | BR_PORT_LOCKED | \ > BR_HAIRPIN_MODE | BR_ISOLATED | BR_MULTICAST_TO_UNICAST) Yeah, ok, normally the feature would be gated until it really works on existing offloading drivers, but I suppose a compromise from 100% correctness could be made if you say you're going to send the offload bits right away.
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c index 0fbefa43f9b1..f336be77019f 100644 --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c @@ -2942,6 +2942,12 @@ static void mlxsw_sp_fdb_notify_mac_process(struct mlxsw_sp *mlxsw_sp, vid = bridge_device->vlan_enabled ? mlxsw_sp_port_vlan->vid : 0; evid = mlxsw_sp_port_vlan->vid; + if (adding && mlxsw_sp_port->security) { + mlxsw_sp_fdb_call_notifiers(SWITCHDEV_FDB_ADD_TO_BRIDGE, mac, + vid, bridge_port->dev, false, true); + return; + } + do_fdb_op: err = mlxsw_sp_port_fdb_uc_op(mlxsw_sp, local_port, mac, fid, evid, adding, true); @@ -3006,6 +3012,12 @@ static void mlxsw_sp_fdb_notify_mac_lag_process(struct mlxsw_sp *mlxsw_sp, vid = bridge_device->vlan_enabled ? mlxsw_sp_port_vlan->vid : 0; lag_vid = mlxsw_sp_port_vlan->vid; + if (adding && mlxsw_sp_port->security) { + mlxsw_sp_fdb_call_notifiers(SWITCHDEV_FDB_ADD_TO_BRIDGE, mac, + vid, bridge_port->dev, false, true); + return; + } + do_fdb_op: err = mlxsw_sp_port_fdb_uc_lag_op(mlxsw_sp, lag_id, mac, fid, lag_vid, adding, true);
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@nvidia.com> --- .../net/ethernet/mellanox/mlxsw/spectrum_switchdev.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)