Re: [RFC PATCH net-next 04/16] bridge: switchdev: Allow device drivers to install locked FDB entries

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Tue, Oct 25, 2022 at 01:00:12PM +0300, Ido Schimmel wrote:
> From: "Hans J. Schultz" <netdev@kapio-technology.com>
> 
> When the bridge is offloaded to hardware, FDB entries are learned and
> aged-out by the hardware. Some device drivers synchronize the hardware
> and software FDBs by generating switchdev events towards the bridge.
> 
> When a port is locked, the hardware must not learn autonomously, as
> otherwise any host will blindly gain authorization. Instead, the
> hardware should generate events regarding hosts that are trying to gain
> authorization and their MAC addresses should be notified by the device
> driver as locked FDB entries towards the bridge driver.
> 
> Allow device drivers to notify the bridge driver about such entries by
> extending the 'switchdev_notifier_fdb_info' structure with the 'locked'
> bit. The bit can only be set by device drivers and not by the bridge
> driver.

What prevents a BR_FDB_LOCKED entry learned by the software bridge in
br_handle_frame_finish() from being notified to switchdev (as non-BR_FDB_LOCKED,
since this is what br_switchdev_fdb_notify() currently hardcodes)?

I think it would be good to reinstate some of the checks in
br_switchdev_fdb_notify() like the one removed in commit 2c4eca3ef716
("net: bridge: switchdev: include local flag in FDB notifications"):

	if (test_bit(BR_FDB_LOCKED, &fdb->flags))
		return;

at least until we need something more complex and somebody on the
switchdev chain wants to snoop these addresses for some incredibly odd
reason.

> Prevent a locked entry from being installed if MAB is not enabled on the
> bridge port. By placing this check in the bridge driver we avoid the
> need to reflect the 'BR_PORT_MAB' flag to device drivers.

So how does the device driver know whether to emit the SWITCHDEV_FDB_ADD_TO_BRIDGE
or not, if we don't pass the BR_PORT_MAB bit to it?

> If an entry already exists in the bridge driver, reject the locked entry
> if the current entry does not have the "locked" flag set or if it points
> to a different port. The same semantics are implemented in the software
> data path.
> 
> Signed-off-by: Hans J. Schultz <netdev@kapio-technology.com>
> Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> ---
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 4ce8b8e5ae0b..4c4fda930068 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -811,7 +811,7 @@ int br_fdb_sync_static(struct net_bridge *br, struct net_bridge_port *p);
>  void br_fdb_unsync_static(struct net_bridge *br, struct net_bridge_port *p);
>  int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
>  			      const unsigned char *addr, u16 vid,
> -			      bool swdev_notify);
> +			      bool locked, bool swdev_notify);
>  int br_fdb_external_learn_del(struct net_bridge *br, struct net_bridge_port *p,
>  			      const unsigned char *addr, u16 vid,
>  			      bool swdev_notify);
> diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
> index 8f3d76c751dd..6afd4f241474 100644
> --- a/net/bridge/br_switchdev.c
> +++ b/net/bridge/br_switchdev.c
> @@ -136,6 +136,7 @@ static void br_switchdev_fdb_populate(struct net_bridge *br,
>  	item->added_by_user = test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags);
>  	item->offloaded = test_bit(BR_FDB_OFFLOADED, &fdb->flags);
>  	item->is_local = test_bit(BR_FDB_LOCAL, &fdb->flags);
> +	item->locked = 0;

0 or false? A matter of preference, I presume. Anyway, this will only be
correct with the extra check mentioned above. Otherwise, a LOCKED entry
may be presented as non-LOCKED to switchdev, with potentially unforeseen
consequences.

>  	item->info.dev = (!p || item->is_local) ? br->dev : p->dev;
>  	item->info.ctx = ctx;
>  }
> -- 
> 2.37.3
>