[RFC PATCH net-next 00/16] bridge: Add MAC Authentication Bypass (MAB) support with offload

[RFC PATCH net-next 00/16] bridge: Add MAC Authentication Bypass (MAB) support with offload

[Ido Schimmel]

This patchset is based on Hans' work from [1][2]. It adds MAB support in
the bridge driver and 802.1X (with MAB) offload support in mlxsw.

Patchset overview
=================

Patch #1 adds MAB support in the bridge driver. See the commit message
for motivation and design choices.

Patch #2 adds a selftest.

Patches #3-#4 extend the switchdev interfaces to allow device drivers to
install locked FDB entries in the bridge driver. Required for MAB
offload support.

The rest of the patches add 802.1X and MAB offload support in mlxsw.
Specifically:

Patches #5-#6 add the required packet traps for 802.1X.

Patches #7-#11 are small preparations.

Patch #12 adds locked bridge port support in mlxsw.

Patches #13-#16 add mlxsw selftests.

Future work
===========

The hostapd fork by Westermo is using dynamic FDB entries to authorize
hosts [3]. Changes are required in switchdev to allow such entries to be
offloaded. Hans already indicated he is working on that [4]. It should
not necessitate any uAPI changes so I do not view it as a blocker (Hans,
please confirm).

Merge plan
==========

We need to agree on a merge plan that allows us to start submitting
patches for inclusion and finally conclude this work. In my experience,
it is best to work in small batches. I therefore propose the following
plan:

* Add MAB support in the bridge driver. This corresponds to patches
  #1-#2.

* Switchdev extensions for MAB offload together with mlxsw
  support. This corresponds to patches #3-#16. I can reduce the number
  of patches by splitting out the selftests to a separate submission.

* mv88e6xxx support. I believe the blackhole stuff is an optimization,
  so I suggest getting initial MAB offload support without that. Support
  for blackhole entries together with offload can be added in a separate
  submission.

* Switchdev extensions for dynamic FDB entries together with mv88e6xxx
  support. I can follow up with mlxsw support afterwards.

[1] https://lore.kernel.org/netdev/20221018165619.134535-1-netdev@kapio-technology.com/
[2] https://lore.kernel.org/netdev/20221004152036.7848-1-netdev@kapio-technology.com/
[3] https://github.com/westermo/hostapd/blob/bridge_driver/hostapd/hostapd_auth_deauth.sh#L11
[4] https://lore.kernel.org/netdev/a11af0d07a79adbd2ac3d242b36dec7e@kapio-technology.com/

Hans J. Schultz (3):
  bridge: Add MAC Authentication Bypass (MAB) support
  selftests: forwarding: Add MAC Authentication Bypass (MAB) test cases
  bridge: switchdev: Allow device drivers to install locked FDB entries

Ido Schimmel (13):
  bridge: switchdev: Let device drivers determine FDB offload indication
  devlink: Add packet traps for 802.1X operation
  mlxsw: spectrum_trap: Register 802.1X packet traps with devlink
  mlxsw: reg: Add Switch Port FDB Security Register
  mlxsw: spectrum: Add an API to configure security checks
  mlxsw: spectrum_switchdev: Prepare for locked FDB notifications
  mlxsw: spectrum_switchdev: Add support for locked FDB notifications
  mlxsw: spectrum_switchdev: Use extack in bridge port flag validation
  mlxsw: spectrum_switchdev: Add locked bridge port support
  selftests: devlink_lib: Split out helper
  selftests: mlxsw: Add a test for EAPOL trap
  selftests: mlxsw: Add a test for locked port trap
  selftests: mlxsw: Add a test for invalid locked bridge port
    configurations

 .../networking/devlink/devlink-trap.rst       |  13 +++
 drivers/net/ethernet/mellanox/mlxsw/reg.h     |  35 ++++++
 .../net/ethernet/mellanox/mlxsw/spectrum.c    |  22 ++++
 .../net/ethernet/mellanox/mlxsw/spectrum.h    |   5 +-
 .../mellanox/mlxsw/spectrum_switchdev.c       |  64 +++++++++--
 .../ethernet/mellanox/mlxsw/spectrum_trap.c   |  25 +++++
 drivers/net/ethernet/mellanox/mlxsw/trap.h    |   2 +
 include/linux/if_bridge.h                     |   1 +
 include/net/devlink.h                         |   9 ++
 include/net/switchdev.h                       |   1 +
 include/uapi/linux/if_link.h                  |   1 +
 include/uapi/linux/neighbour.h                |   8 +-
 net/bridge/br.c                               |   5 +-
 net/bridge/br_fdb.c                           |  46 +++++++-
 net/bridge/br_input.c                         |  15 ++-
 net/bridge/br_netlink.c                       |  13 ++-
 net/bridge/br_private.h                       |   5 +-
 net/bridge/br_switchdev.c                     |   1 +
 net/core/devlink.c                            |   3 +
 net/core/rtnetlink.c                          |   5 +
 .../drivers/net/mlxsw/devlink_trap_control.sh |  22 ++++
 .../net/mlxsw/devlink_trap_l2_drops.sh        | 105 ++++++++++++++++++
 .../selftests/drivers/net/mlxsw/rtnetlink.sh  |  31 ++++++
 .../net/forwarding/bridge_locked_port.sh      | 101 ++++++++++++++++-
 .../selftests/net/forwarding/devlink_lib.sh   |  19 ++--
 tools/testing/selftests/net/forwarding/lib.sh |   8 ++
 26 files changed, 535 insertions(+), 30 deletions(-)

-- 
2.37.3

[RFC PATCH net-next 01/16] bridge: Add MAC Authentication Bypass (MAB) support

[Ido Schimmel]

From: "Hans J. Schultz" <netdev@kapio-technology.com>

Hosts that support 802.1X authentication are able to authenticate
themselves by exchanging EAPOL frames with an authenticator (Ethernet
bridge, in this case) and an authentication server. Access to the
network is only granted by the authenticator to successfully
authenticated hosts.

The above is implemented in the bridge using the "locked" bridge port
option. When enabled, link-local frames (e.g., EAPOL) can be locally
received by the bridge, but all other frames are dropped unless the host
is authenticated. That is, unless the user space control plane installed
an FDB entry according to which the source address of the frame is
located behind the locked ingress port. The entry can be dynamic, in
which case learning needs to be enabled so that the entry will be
refreshed by incoming traffic.

There are deployments in which not all the devices connected to the
authenticator (the bridge) support 802.1X. Such devices can include
printers and cameras. One option to support such deployments is to
unlock the bridge ports connecting these devices, but a slightly more
secure option is to use MAB. When MAB is enabled, the MAC address of the
connected device is used as the user name and password for the
authentication.

For MAB to work, the user space control plane needs to be notified about
MAC addresses that are trying to gain access so that they will be
compared against an allow list. This can be implemented via the regular
learning process with the following differences:

1. Learned FDB entries are installed with a new "locked" flag indicating
   that the entry cannot be used to authenticate the device. The flag
   cannot be set by user space, but user space can clear the flag by
   replacing the entry, thereby authenticating the device.

2. FDB entries cannot roam to locked ports to prevent unauthenticated
   devices from disrupting traffic destined to already authenticated
   devices.

Enable this behavior using a new bridge port option called "mab". It can
only be enabled on a bridge port that is both locked and has learning
enabled. A new option is added because there are pure 802.1X deployments
that are not interested in notifications about "locked" FDB entries.

Signed-off-by: Hans J. Schultz <netdev@kapio-technology.com>
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---

Notes:
    Changes made by me:
    
     * Reword commit message.
     * Reword comment regarding 'NTF_EXT_LOCKED'.
     * Use extack in br_fdb_add().
     * Forbid MAB when learning is disabled.

 include/linux/if_bridge.h      |  1 +
 include/uapi/linux/if_link.h   |  1 +
 include/uapi/linux/neighbour.h |  8 +++++++-
 net/bridge/br_fdb.c            | 24 ++++++++++++++++++++++++
 net/bridge/br_input.c          | 15 +++++++++++++--
 net/bridge/br_netlink.c        | 13 ++++++++++++-
 net/bridge/br_private.h        |  3 ++-
 net/core/rtnetlink.c           |  5 +++++
 8 files changed, 65 insertions(+), 5 deletions(-)

diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
index d62ef428e3aa..1668ac4d7adc 100644
--- a/include/linux/if_bridge.h
+++ b/include/linux/if_bridge.h
@@ -59,6 +59,7 @@ struct br_ip_list {
 #define BR_MRP_LOST_IN_CONT	BIT(19)
 #define BR_TX_FWD_OFFLOAD	BIT(20)
 #define BR_PORT_LOCKED		BIT(21)
+#define BR_PORT_MAB		BIT(22)
 
 #define BR_DEFAULT_AGEING_TIME	(300 * HZ)
 
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 5e7a1041df3a..d92b3f79eba3 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -561,6 +561,7 @@ enum {
 	IFLA_BRPORT_MCAST_EHT_HOSTS_LIMIT,
 	IFLA_BRPORT_MCAST_EHT_HOSTS_CNT,
 	IFLA_BRPORT_LOCKED,
+	IFLA_BRPORT_MAB,
 	__IFLA_BRPORT_MAX
 };
 #define IFLA_BRPORT_MAX (__IFLA_BRPORT_MAX - 1)
diff --git a/include/uapi/linux/neighbour.h b/include/uapi/linux/neighbour.h
index a998bf761635..5e67a7eaf4a7 100644
--- a/include/uapi/linux/neighbour.h
+++ b/include/uapi/linux/neighbour.h
@@ -52,7 +52,8 @@ enum {
 #define NTF_STICKY	(1 << 6)
 #define NTF_ROUTER	(1 << 7)
 /* Extended flags under NDA_FLAGS_EXT: */
-#define NTF_EXT_MANAGED	(1 << 0)
+#define NTF_EXT_MANAGED		(1 << 0)
+#define NTF_EXT_LOCKED		(1 << 1)
 
 /*
  *	Neighbor Cache Entry States.
@@ -86,6 +87,11 @@ enum {
  * NTF_EXT_MANAGED flagged neigbor entries are managed by the kernel on behalf
  * of a user space control plane, and automatically refreshed so that (if
  * possible) they remain in NUD_REACHABLE state.
+ *
+ * NTF_EXT_LOCKED flagged bridge FDB entries are entries generated by the
+ * bridge in response to a host trying to communicate via a locked bridge port
+ * with MAB enabled. Their purpose is to notify user space that a host requires
+ * authentication.
  */
 
 struct nda_cacheinfo {
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index e7f4fccb6adb..3b83af4458b8 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -105,6 +105,7 @@ static int fdb_fill_info(struct sk_buff *skb, const struct net_bridge *br,
 	struct nda_cacheinfo ci;
 	struct nlmsghdr *nlh;
 	struct ndmsg *ndm;
+	u32 ext_flags = 0;
 
 	nlh = nlmsg_put(skb, portid, seq, type, sizeof(*ndm), flags);
 	if (nlh == NULL)
@@ -125,11 +126,16 @@ static int fdb_fill_info(struct sk_buff *skb, const struct net_bridge *br,
 		ndm->ndm_flags |= NTF_EXT_LEARNED;
 	if (test_bit(BR_FDB_STICKY, &fdb->flags))
 		ndm->ndm_flags |= NTF_STICKY;
+	if (test_bit(BR_FDB_LOCKED, &fdb->flags))
+		ext_flags |= NTF_EXT_LOCKED;
 
 	if (nla_put(skb, NDA_LLADDR, ETH_ALEN, &fdb->key.addr))
 		goto nla_put_failure;
 	if (nla_put_u32(skb, NDA_MASTER, br->dev->ifindex))
 		goto nla_put_failure;
+	if (nla_put_u32(skb, NDA_FLAGS_EXT, ext_flags))
+		goto nla_put_failure;
+
 	ci.ndm_used	 = jiffies_to_clock_t(now - fdb->used);
 	ci.ndm_confirmed = 0;
 	ci.ndm_updated	 = jiffies_to_clock_t(now - fdb->updated);
@@ -171,6 +177,7 @@ static inline size_t fdb_nlmsg_size(void)
 	return NLMSG_ALIGN(sizeof(struct ndmsg))
 		+ nla_total_size(ETH_ALEN) /* NDA_LLADDR */
 		+ nla_total_size(sizeof(u32)) /* NDA_MASTER */
+		+ nla_total_size(sizeof(u32)) /* NDA_FLAGS_EXT */
 		+ nla_total_size(sizeof(u16)) /* NDA_VLAN */
 		+ nla_total_size(sizeof(struct nda_cacheinfo))
 		+ nla_total_size(0) /* NDA_FDB_EXT_ATTRS */
@@ -879,6 +886,11 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
 						      &fdb->flags)))
 					clear_bit(BR_FDB_ADDED_BY_EXT_LEARN,
 						  &fdb->flags);
+				/* Clear locked flag when roaming to an
+				 * unlocked port.
+				 */
+				if (unlikely(test_bit(BR_FDB_LOCKED, &fdb->flags)))
+					clear_bit(BR_FDB_LOCKED, &fdb->flags);
 			}
 
 			if (unlikely(test_bit(BR_FDB_ADDED_BY_USER, &flags)))
@@ -1082,6 +1094,9 @@ static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source,
 		modified = true;
 	}
 
+	if (test_and_clear_bit(BR_FDB_LOCKED, &fdb->flags))
+		modified = true;
+
 	if (fdb_handle_notify(fdb, notify))
 		modified = true;
 
@@ -1150,6 +1165,7 @@ int br_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
 	struct net_bridge_port *p = NULL;
 	struct net_bridge_vlan *v;
 	struct net_bridge *br = NULL;
+	u32 ext_flags = 0;
 	int err = 0;
 
 	trace_br_fdb_add(ndm, dev, addr, vid, nlh_flags);
@@ -1178,6 +1194,14 @@ int br_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
 		vg = nbp_vlan_group(p);
 	}
 
+	if (tb[NDA_FLAGS_EXT])
+		ext_flags = nla_get_u32(tb[NDA_FLAGS_EXT]);
+
+	if (ext_flags & NTF_EXT_LOCKED) {
+		NL_SET_ERR_MSG_MOD(extack, "Cannot add FDB entry with \"locked\" flag set");
+		return -EINVAL;
+	}
+
 	if (tb[NDA_FDB_EXT_ATTRS]) {
 		attr = tb[NDA_FDB_EXT_ATTRS];
 		err = nla_parse_nested(nfea_tb, NFEA_MAX, attr,
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 68b3e850bcb9..068fced7693c 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -109,9 +109,20 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 		struct net_bridge_fdb_entry *fdb_src =
 			br_fdb_find_rcu(br, eth_hdr(skb)->h_source, vid);
 
-		if (!fdb_src || READ_ONCE(fdb_src->dst) != p ||
-		    test_bit(BR_FDB_LOCAL, &fdb_src->flags))
+		if (!fdb_src) {
+			unsigned long flags = 0;
+
+			if (p->flags & BR_PORT_MAB) {
+				__set_bit(BR_FDB_LOCKED, &flags);
+				br_fdb_update(br, p, eth_hdr(skb)->h_source,
+					      vid, flags);
+			}
 			goto drop;
+		} else if (READ_ONCE(fdb_src->dst) != p ||
+			   test_bit(BR_FDB_LOCAL, &fdb_src->flags) ||
+			   test_bit(BR_FDB_LOCKED, &fdb_src->flags)) {
+			goto drop;
+		}
 	}
 
 	nbp_switchdev_frame_mark(p, skb);
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index 5aeb3646e74c..bbc82c70b091 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -188,6 +188,7 @@ static inline size_t br_port_info_size(void)
 		+ nla_total_size(1)	/* IFLA_BRPORT_NEIGH_SUPPRESS */
 		+ nla_total_size(1)	/* IFLA_BRPORT_ISOLATED */
 		+ nla_total_size(1)	/* IFLA_BRPORT_LOCKED */
+		+ nla_total_size(1)	/* IFLA_BRPORT_MAB */
 		+ nla_total_size(sizeof(struct ifla_bridge_id))	/* IFLA_BRPORT_ROOT_ID */
 		+ nla_total_size(sizeof(struct ifla_bridge_id))	/* IFLA_BRPORT_BRIDGE_ID */
 		+ nla_total_size(sizeof(u16))	/* IFLA_BRPORT_DESIGNATED_PORT */
@@ -274,7 +275,8 @@ static int br_port_fill_attrs(struct sk_buff *skb,
 	    nla_put_u8(skb, IFLA_BRPORT_MRP_IN_OPEN,
 		       !!(p->flags & BR_MRP_LOST_IN_CONT)) ||
 	    nla_put_u8(skb, IFLA_BRPORT_ISOLATED, !!(p->flags & BR_ISOLATED)) ||
-	    nla_put_u8(skb, IFLA_BRPORT_LOCKED, !!(p->flags & BR_PORT_LOCKED)))
+	    nla_put_u8(skb, IFLA_BRPORT_LOCKED, !!(p->flags & BR_PORT_LOCKED)) ||
+	    nla_put_u8(skb, IFLA_BRPORT_MAB, !!(p->flags & BR_PORT_MAB)))
 		return -EMSGSIZE;
 
 	timerval = br_timer_value(&p->message_age_timer);
@@ -876,6 +878,7 @@ static const struct nla_policy br_port_policy[IFLA_BRPORT_MAX + 1] = {
 	[IFLA_BRPORT_NEIGH_SUPPRESS] = { .type = NLA_U8 },
 	[IFLA_BRPORT_ISOLATED]	= { .type = NLA_U8 },
 	[IFLA_BRPORT_LOCKED] = { .type = NLA_U8 },
+	[IFLA_BRPORT_MAB] = { .type = NLA_U8 },
 	[IFLA_BRPORT_BACKUP_PORT] = { .type = NLA_U32 },
 	[IFLA_BRPORT_MCAST_EHT_HOSTS_LIMIT] = { .type = NLA_U32 },
 };
@@ -943,6 +946,14 @@ static int br_setport(struct net_bridge_port *p, struct nlattr *tb[],
 	br_set_port_flag(p, tb, IFLA_BRPORT_NEIGH_SUPPRESS, BR_NEIGH_SUPPRESS);
 	br_set_port_flag(p, tb, IFLA_BRPORT_ISOLATED, BR_ISOLATED);
 	br_set_port_flag(p, tb, IFLA_BRPORT_LOCKED, BR_PORT_LOCKED);
+	br_set_port_flag(p, tb, IFLA_BRPORT_MAB, BR_PORT_MAB);
+
+	if ((p->flags & BR_PORT_MAB) &&
+	    (!(p->flags & BR_PORT_LOCKED) || !(p->flags & BR_LEARNING))) {
+		NL_SET_ERR_MSG(extack, "MAB can only be enabled on a locked port with learning enabled");
+		p->flags = old_flags;
+		return -EINVAL;
+	}
 
 	changed_mask = old_flags ^ p->flags;
 
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 06e5f6faa431..4ce8b8e5ae0b 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -251,7 +251,8 @@ enum {
 	BR_FDB_ADDED_BY_EXT_LEARN,
 	BR_FDB_OFFLOADED,
 	BR_FDB_NOTIFY,
-	BR_FDB_NOTIFY_INACTIVE
+	BR_FDB_NOTIFY_INACTIVE,
+	BR_FDB_LOCKED,
 };
 
 struct net_bridge_fdb_key {
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 74864dc46a7e..d6e4d2854edb 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -4045,6 +4045,11 @@ int ndo_dflt_fdb_add(struct ndmsg *ndm,
 		return err;
 	}
 
+	if (tb[NDA_FLAGS_EXT]) {
+		netdev_info(dev, "invalid flags given to default FDB implementation\n");
+		return err;
+	}
+
 	if (vid) {
 		netdev_info(dev, "vlans aren't supported yet for dev_uc|mc_add()\n");
 		return err;
-- 
2.37.3

Re: [RFC PATCH net-next 01/16] bridge: Add MAC Authentication Bypass (MAB) support

[Nikolay Aleksandrov]

On 25/10/2022 13:00, Ido Schimmel wrote:
> From: "Hans J. Schultz" <netdev@kapio-technology.com>
> 
> Hosts that support 802.1X authentication are able to authenticate
> themselves by exchanging EAPOL frames with an authenticator (Ethernet
> bridge, in this case) and an authentication server. Access to the
> network is only granted by the authenticator to successfully
> authenticated hosts.
> 
> The above is implemented in the bridge using the "locked" bridge port
> option. When enabled, link-local frames (e.g., EAPOL) can be locally
> received by the bridge, but all other frames are dropped unless the host
> is authenticated. That is, unless the user space control plane installed
> an FDB entry according to which the source address of the frame is
> located behind the locked ingress port. The entry can be dynamic, in
> which case learning needs to be enabled so that the entry will be
> refreshed by incoming traffic.
> 
> There are deployments in which not all the devices connected to the
> authenticator (the bridge) support 802.1X. Such devices can include
> printers and cameras. One option to support such deployments is to
> unlock the bridge ports connecting these devices, but a slightly more
> secure option is to use MAB. When MAB is enabled, the MAC address of the
> connected device is used as the user name and password for the
> authentication.
> 
> For MAB to work, the user space control plane needs to be notified about
> MAC addresses that are trying to gain access so that they will be
> compared against an allow list. This can be implemented via the regular
> learning process with the following differences:
> 
> 1. Learned FDB entries are installed with a new "locked" flag indicating
>    that the entry cannot be used to authenticate the device. The flag
>    cannot be set by user space, but user space can clear the flag by
>    replacing the entry, thereby authenticating the device.
> 
> 2. FDB entries cannot roam to locked ports to prevent unauthenticated
>    devices from disrupting traffic destined to already authenticated
>    devices.
> 
> Enable this behavior using a new bridge port option called "mab". It can
> only be enabled on a bridge port that is both locked and has learning
> enabled. A new option is added because there are pure 802.1X deployments
> that are not interested in notifications about "locked" FDB entries.
> 
> Signed-off-by: Hans J. Schultz <netdev@kapio-technology.com>
> Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> ---
> 
> Notes:
>     Changes made by me:
>     
>      * Reword commit message.
>      * Reword comment regarding 'NTF_EXT_LOCKED'.
>      * Use extack in br_fdb_add().
>      * Forbid MAB when learning is disabled.
> 
>  include/linux/if_bridge.h      |  1 +
>  include/uapi/linux/if_link.h   |  1 +
>  include/uapi/linux/neighbour.h |  8 +++++++-
>  net/bridge/br_fdb.c            | 24 ++++++++++++++++++++++++
>  net/bridge/br_input.c          | 15 +++++++++++++--
>  net/bridge/br_netlink.c        | 13 ++++++++++++-
>  net/bridge/br_private.h        |  3 ++-
>  net/core/rtnetlink.c           |  5 +++++
>  8 files changed, 65 insertions(+), 5 deletions(-)
> 

Thanks for finalizing this, the patch looks good to me.
Acked-by: Nikolay Aleksandrov <razor@blackwall.org>

Thanks,
 Nik

Re: [RFC PATCH net-next 01/16] bridge: Add MAC Authentication Bypass (MAB) support

[Vladimir Oltean]

Hi Ido,

Thanks for the commit message. It is very good.

On Tue, Oct 25, 2022 at 01:00:09PM +0300, Ido Schimmel wrote:
> From: "Hans J. Schultz" <netdev@kapio-technology.com>
> 
> Hosts that support 802.1X authentication are able to authenticate
> themselves by exchanging EAPOL frames with an authenticator (Ethernet
> bridge, in this case) and an authentication server. Access to the
> network is only granted by the authenticator to successfully
> authenticated hosts.
> 
> The above is implemented in the bridge using the "locked" bridge port
> option. When enabled, link-local frames (e.g., EAPOL) can be locally
> received by the bridge, but all other frames are dropped unless the host
> is authenticated. That is, unless the user space control plane installed
> an FDB entry according to which the source address of the frame is
> located behind the locked ingress port. The entry can be dynamic, in
> which case learning needs to be enabled so that the entry will be
> refreshed by incoming traffic.
> 
> There are deployments in which not all the devices connected to the
> authenticator (the bridge) support 802.1X. Such devices can include
> printers and cameras. One option to support such deployments is to
> unlock the bridge ports connecting these devices, but a slightly more
> secure option is to use MAB. When MAB is enabled, the MAC address of the
> connected device is used as the user name and password for the
> authentication.
> 
> For MAB to work, the user space control plane needs to be notified about
> MAC addresses that are trying to gain access so that they will be
> compared against an allow list. This can be implemented via the regular
> learning process with the following differences:
> 
> 1. Learned FDB entries are installed with a new "locked" flag indicating
>    that the entry cannot be used to authenticate the device. The flag
>    cannot be set by user space, but user space can clear the flag by
>    replacing the entry, thereby authenticating the device.
> 
> 2. FDB entries cannot roam to locked ports to prevent unauthenticated
>    devices from disrupting traffic destined to already authenticated
>    devices.

The behavior described in (2) has nothing to do with locked FDB entries
or MAB (what is described in this paragraph), it applies to all of them,
no? The code was already there:

	if (p->flags & BR_PORT_LOCKED)
		if (!fdb_src || READ_ONCE(fdb_src->dst) != p ||
		    test_bit(BR_FDB_LOCAL, &fdb_src->flags))
			goto drop;

I think you mean to say: the above already holds true, but the relevant
implication here is that locked FDB entries will not be created if the
MAC address is present in the FDB on any other port?

I think some part of this comment should also go to the convoluted
BR_PORT_LOCKED block from br_handle_frame_finish()?

I was going to ask if we should bother to add code to prohibit packets
from being forwarded to an FDB entry that was learned as LOCKED, since
that FDB entry is more of a "ghost" and not something fully committed?

But with the "never roam to locked port" policy, I don't think there is
any practical risk that the extra code would mitigate. Assume that a
"snooper" wants to get the traffic destined for a MAC DA X, so it creates
a LOCKED FDB entry. It has to time itself just right, 5 minutes after
the station it wants to intercept has gone silent (or before the station
said anything). Anyone thinking it's talking to X now talks to the snooper.
But this attack vector is bounded in time. As long as X says anything,
the LOCKED FDB entry moves towards X, and the LOCKED flag gets cleared.

> 
> Enable this behavior using a new bridge port option called "mab". It can
> only be enabled on a bridge port that is both locked and has learning
> enabled. A new option is added because there are pure 802.1X deployments
> that are not interested in notifications about "locked" FDB entries.
> 
> Signed-off-by: Hans J. Schultz <netdev@kapio-technology.com>
> Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> ---
> 
> Notes:
>     Changes made by me:
>     
>      * Reword commit message.
>      * Reword comment regarding 'NTF_EXT_LOCKED'.
>      * Use extack in br_fdb_add().
>      * Forbid MAB when learning is disabled.

Forbidding MAB when learning is disabled makes sense to me, since it
means accepting that MAB is a form of learning (as the implementation
also shows; all other callers of br_fdb_update() are guarded by a
port learning check). I believe this will also make life easier with
offloading drivers. Thanks.

>  include/linux/if_bridge.h      |  1 +
>  include/uapi/linux/if_link.h   |  1 +
>  include/uapi/linux/neighbour.h |  8 +++++++-
>  net/bridge/br_fdb.c            | 24 ++++++++++++++++++++++++
>  net/bridge/br_input.c          | 15 +++++++++++++--
>  net/bridge/br_netlink.c        | 13 ++++++++++++-
>  net/bridge/br_private.h        |  3 ++-
>  net/core/rtnetlink.c           |  5 +++++
>  8 files changed, 65 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
> index d62ef428e3aa..1668ac4d7adc 100644
> --- a/include/linux/if_bridge.h
> +++ b/include/linux/if_bridge.h
> @@ -59,6 +59,7 @@ struct br_ip_list {
>  #define BR_MRP_LOST_IN_CONT	BIT(19)
>  #define BR_TX_FWD_OFFLOAD	BIT(20)
>  #define BR_PORT_LOCKED		BIT(21)
> +#define BR_PORT_MAB		BIT(22)

Question about unsetting BR_PORT_MAB using IFLA_BRPORT_MAB: should this
operation flush BR_FDB_LOCKED entries on the port?

> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> index 68b3e850bcb9..068fced7693c 100644
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -109,9 +109,20 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>  		struct net_bridge_fdb_entry *fdb_src =
>  			br_fdb_find_rcu(br, eth_hdr(skb)->h_source, vid);
>  
> -		if (!fdb_src || READ_ONCE(fdb_src->dst) != p ||
> -		    test_bit(BR_FDB_LOCAL, &fdb_src->flags))
> +		if (!fdb_src) {
> +			unsigned long flags = 0;
> +
> +			if (p->flags & BR_PORT_MAB) {
> +				__set_bit(BR_FDB_LOCKED, &flags);
> +				br_fdb_update(br, p, eth_hdr(skb)->h_source,
> +					      vid, flags);
> +			}
>  			goto drop;
> +		} else if (READ_ONCE(fdb_src->dst) != p ||
> +			   test_bit(BR_FDB_LOCAL, &fdb_src->flags) ||
> +			   test_bit(BR_FDB_LOCKED, &fdb_src->flags)) {

Minor nitpick: shouldn't br_fdb_update() also be called when the packet
matched on a BR_FDB_LOCKED entry on port p, so as to refresh it, if the
station is persistent? Currently I believe the FDB entry will expire
within 5 minutes of the first packet, regardless of subsequent traffic.

> +			goto drop;
> +		}
>  	}
>  
>  	nbp_switchdev_frame_mark(p, skb);

Re: [RFC PATCH net-next 01/16] bridge: Add MAC Authentication Bypass (MAB) support

[netdev]

On 2022-10-28 00:58, Vladimir Oltean wrote:

> I was going to ask if we should bother to add code to prohibit packets
> from being forwarded to an FDB entry that was learned as LOCKED, since
> that FDB entry is more of a "ghost" and not something fully committed?

I think that it is a security flaw if there is any forwarding to 
BR_FDB_LOCKED
entries. I can imagine a host behind a locked port with no credentials,
that gets a BR_FDB_LOCKED entry and has a friend on another non-locked 
port
who can now communicate uni-directional to the host with the 
BR_FDB_LOCKED
entry. It should not be too hard to create a scheme using UDP packets or
other for that.

Re: [RFC PATCH net-next 01/16] bridge: Add MAC Authentication Bypass (MAB) support

[Ido Schimmel]

On Fri, Oct 28, 2022 at 09:45:52AM +0200, netdev@kapio-technology.com wrote:
> On 2022-10-28 00:58, Vladimir Oltean wrote:
> 
> > I was going to ask if we should bother to add code to prohibit packets
> > from being forwarded to an FDB entry that was learned as LOCKED, since
> > that FDB entry is more of a "ghost" and not something fully committed?
> 
> I think that it is a security flaw if there is any forwarding to
> BR_FDB_LOCKED
> entries. I can imagine a host behind a locked port with no credentials,
> that gets a BR_FDB_LOCKED entry and has a friend on another non-locked port
> who can now communicate uni-directional to the host with the BR_FDB_LOCKED
> entry. It should not be too hard to create a scheme using UDP packets or
> other for that.

User space knows that the MAC is not authorized (otherwise it would have
cleared the "locked" flag) and can choose to mitigate this corner case
(or not) by shutting down the port, installing flower filters or doing
something else entirely. I think it is best to defer such policy
decisions to user space instead of overloading the "locked" flag with
more meaning which will likely result in more checks in the fast path
for a corner case of a use case that is quite obscure to begin with.

Re: [RFC PATCH net-next 01/16] bridge: Add MAC Authentication Bypass (MAB) support

[Ido Schimmel]

On Thu, Oct 27, 2022 at 10:58:32PM +0000, Vladimir Oltean wrote:
> Hi Ido,
> 
> Thanks for the commit message. It is very good.
> 
> On Tue, Oct 25, 2022 at 01:00:09PM +0300, Ido Schimmel wrote:
> > From: "Hans J. Schultz" <netdev@kapio-technology.com>
> > 
> > Hosts that support 802.1X authentication are able to authenticate
> > themselves by exchanging EAPOL frames with an authenticator (Ethernet
> > bridge, in this case) and an authentication server. Access to the
> > network is only granted by the authenticator to successfully
> > authenticated hosts.
> > 
> > The above is implemented in the bridge using the "locked" bridge port
> > option. When enabled, link-local frames (e.g., EAPOL) can be locally
> > received by the bridge, but all other frames are dropped unless the host
> > is authenticated. That is, unless the user space control plane installed
> > an FDB entry according to which the source address of the frame is
> > located behind the locked ingress port. The entry can be dynamic, in
> > which case learning needs to be enabled so that the entry will be
> > refreshed by incoming traffic.
> > 
> > There are deployments in which not all the devices connected to the
> > authenticator (the bridge) support 802.1X. Such devices can include
> > printers and cameras. One option to support such deployments is to
> > unlock the bridge ports connecting these devices, but a slightly more
> > secure option is to use MAB. When MAB is enabled, the MAC address of the
> > connected device is used as the user name and password for the
> > authentication.
> > 
> > For MAB to work, the user space control plane needs to be notified about
> > MAC addresses that are trying to gain access so that they will be
> > compared against an allow list. This can be implemented via the regular
> > learning process with the following differences:
> > 
> > 1. Learned FDB entries are installed with a new "locked" flag indicating
> >    that the entry cannot be used to authenticate the device. The flag
> >    cannot be set by user space, but user space can clear the flag by
> >    replacing the entry, thereby authenticating the device.
> > 
> > 2. FDB entries cannot roam to locked ports to prevent unauthenticated
> >    devices from disrupting traffic destined to already authenticated
> >    devices.
> 
> The behavior described in (2) has nothing to do with locked FDB entries
> or MAB (what is described in this paragraph), it applies to all of them,
> no? The code was already there:
> 
> 	if (p->flags & BR_PORT_LOCKED)
> 		if (!fdb_src || READ_ONCE(fdb_src->dst) != p ||
> 		    test_bit(BR_FDB_LOCAL, &fdb_src->flags))
> 			goto drop;
> 
> I think you mean to say: the above already holds true, but the relevant
> implication here is that locked FDB entries will not be created if the
> MAC address is present in the FDB on any other port?

Yes, will reword to make this clearer.

> 
> I think some part of this comment should also go to the convoluted
> BR_PORT_LOCKED block from br_handle_frame_finish()?

Sure, will add a comment.

> 
> I was going to ask if we should bother to add code to prohibit packets
> from being forwarded to an FDB entry that was learned as LOCKED, since
> that FDB entry is more of a "ghost" and not something fully committed?
> 
> But with the "never roam to locked port" policy, I don't think there is
> any practical risk that the extra code would mitigate. Assume that a
> "snooper" wants to get the traffic destined for a MAC DA X, so it creates
> a LOCKED FDB entry. It has to time itself just right, 5 minutes after
> the station it wants to intercept has gone silent (or before the station
> said anything). Anyone thinking it's talking to X now talks to the snooper.
> But this attack vector is bounded in time. As long as X says anything,
> the LOCKED FDB entry moves towards X, and the LOCKED flag gets cleared.

I think it is best if we keep the semantic of the "locked" flag as the
"entry cannot be used to authenticate the device" and let the MAB user
space daemon worry about the rest. For example, if a MAC address that is
not in the allow list appears behind a locked port, user space can
decide to shutdown the port or install a flower filter that drops
packets destined to this MAC DA.

> 
> > 
> > Enable this behavior using a new bridge port option called "mab". It can
> > only be enabled on a bridge port that is both locked and has learning
> > enabled. A new option is added because there are pure 802.1X deployments
> > that are not interested in notifications about "locked" FDB entries.
> > 
> > Signed-off-by: Hans J. Schultz <netdev@kapio-technology.com>
> > Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> > ---
> > 
> > Notes:
> >     Changes made by me:
> >     
> >      * Reword commit message.
> >      * Reword comment regarding 'NTF_EXT_LOCKED'.
> >      * Use extack in br_fdb_add().
> >      * Forbid MAB when learning is disabled.
> 
> Forbidding MAB when learning is disabled makes sense to me, since it
> means accepting that MAB is a form of learning (as the implementation
> also shows; all other callers of br_fdb_update() are guarded by a
> port learning check). I believe this will also make life easier with
> offloading drivers. Thanks.
> 
> >  include/linux/if_bridge.h      |  1 +
> >  include/uapi/linux/if_link.h   |  1 +
> >  include/uapi/linux/neighbour.h |  8 +++++++-
> >  net/bridge/br_fdb.c            | 24 ++++++++++++++++++++++++
> >  net/bridge/br_input.c          | 15 +++++++++++++--
> >  net/bridge/br_netlink.c        | 13 ++++++++++++-
> >  net/bridge/br_private.h        |  3 ++-
> >  net/core/rtnetlink.c           |  5 +++++
> >  8 files changed, 65 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
> > index d62ef428e3aa..1668ac4d7adc 100644
> > --- a/include/linux/if_bridge.h
> > +++ b/include/linux/if_bridge.h
> > @@ -59,6 +59,7 @@ struct br_ip_list {
> >  #define BR_MRP_LOST_IN_CONT	BIT(19)
> >  #define BR_TX_FWD_OFFLOAD	BIT(20)
> >  #define BR_PORT_LOCKED		BIT(21)
> > +#define BR_PORT_MAB		BIT(22)
> 
> Question about unsetting BR_PORT_MAB using IFLA_BRPORT_MAB: should this
> operation flush BR_FDB_LOCKED entries on the port?

Good point. Will try to do that using br_fdb_flush() and add a test
case.

> 
> > diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> > index 68b3e850bcb9..068fced7693c 100644
> > --- a/net/bridge/br_input.c
> > +++ b/net/bridge/br_input.c
> > @@ -109,9 +109,20 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
> >  		struct net_bridge_fdb_entry *fdb_src =
> >  			br_fdb_find_rcu(br, eth_hdr(skb)->h_source, vid);
> >  
> > -		if (!fdb_src || READ_ONCE(fdb_src->dst) != p ||
> > -		    test_bit(BR_FDB_LOCAL, &fdb_src->flags))
> > +		if (!fdb_src) {
> > +			unsigned long flags = 0;
> > +
> > +			if (p->flags & BR_PORT_MAB) {
> > +				__set_bit(BR_FDB_LOCKED, &flags);
> > +				br_fdb_update(br, p, eth_hdr(skb)->h_source,
> > +					      vid, flags);
> > +			}
> >  			goto drop;
> > +		} else if (READ_ONCE(fdb_src->dst) != p ||
> > +			   test_bit(BR_FDB_LOCAL, &fdb_src->flags) ||
> > +			   test_bit(BR_FDB_LOCKED, &fdb_src->flags)) {
> 
> Minor nitpick: shouldn't br_fdb_update() also be called when the packet
> matched on a BR_FDB_LOCKED entry on port p, so as to refresh it, if the
> station is persistent? Currently I believe the FDB entry will expire
> within 5 minutes of the first packet, regardless of subsequent traffic.

Makes sense. Will add.

Thanks!

> 
> > +			goto drop;
> > +		}
> >  	}
> >  
> >  	nbp_switchdev_frame_mark(p, skb);

Re: [RFC PATCH net-next 01/16] bridge: Add MAC Authentication Bypass (MAB) support

[netdev]

On 2022-10-25 12:00, Ido Schimmel wrote:
> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> index 5aeb3646e74c..bbc82c70b091 100644
> --- a/net/bridge/br_netlink.c
> +++ b/net/bridge/br_netlink.c
> @@ -188,6 +188,7 @@ static inline size_t br_port_info_size(void)
>  		+ nla_total_size(1)	/* IFLA_BRPORT_NEIGH_SUPPRESS */
>  		+ nla_total_size(1)	/* IFLA_BRPORT_ISOLATED */
>  		+ nla_total_size(1)	/* IFLA_BRPORT_LOCKED */
> +		+ nla_total_size(1)	/* IFLA_BRPORT_MAB */
>  		+ nla_total_size(sizeof(struct ifla_bridge_id))	/* 
> IFLA_BRPORT_ROOT_ID */
>  		+ nla_total_size(sizeof(struct ifla_bridge_id))	/* 
> IFLA_BRPORT_BRIDGE_ID */
>  		+ nla_total_size(sizeof(u16))	/* IFLA_BRPORT_DESIGNATED_PORT */
> @@ -274,7 +275,8 @@ static int br_port_fill_attrs(struct sk_buff *skb,
>  	    nla_put_u8(skb, IFLA_BRPORT_MRP_IN_OPEN,
>  		       !!(p->flags & BR_MRP_LOST_IN_CONT)) ||
>  	    nla_put_u8(skb, IFLA_BRPORT_ISOLATED, !!(p->flags & BR_ISOLATED)) 
> ||
> -	    nla_put_u8(skb, IFLA_BRPORT_LOCKED, !!(p->flags & 
> BR_PORT_LOCKED)))
> +	    nla_put_u8(skb, IFLA_BRPORT_LOCKED, !!(p->flags & 
> BR_PORT_LOCKED)) ||
> +	    nla_put_u8(skb, IFLA_BRPORT_MAB, !!(p->flags & BR_PORT_MAB)))
>  		return -EMSGSIZE;
> 
>  	timerval = br_timer_value(&p->message_age_timer);
> @@ -876,6 +878,7 @@ static const struct nla_policy
> br_port_policy[IFLA_BRPORT_MAX + 1] = {
>  	[IFLA_BRPORT_NEIGH_SUPPRESS] = { .type = NLA_U8 },
>  	[IFLA_BRPORT_ISOLATED]	= { .type = NLA_U8 },
>  	[IFLA_BRPORT_LOCKED] = { .type = NLA_U8 },
> +	[IFLA_BRPORT_MAB] = { .type = NLA_U8 },
>  	[IFLA_BRPORT_BACKUP_PORT] = { .type = NLA_U32 },
>  	[IFLA_BRPORT_MCAST_EHT_HOSTS_LIMIT] = { .type = NLA_U32 },
>  };
> @@ -943,6 +946,14 @@ static int br_setport(struct net_bridge_port *p,
> struct nlattr *tb[],
>  	br_set_port_flag(p, tb, IFLA_BRPORT_NEIGH_SUPPRESS, 
> BR_NEIGH_SUPPRESS);
>  	br_set_port_flag(p, tb, IFLA_BRPORT_ISOLATED, BR_ISOLATED);
>  	br_set_port_flag(p, tb, IFLA_BRPORT_LOCKED, BR_PORT_LOCKED);
> +	br_set_port_flag(p, tb, IFLA_BRPORT_MAB, BR_PORT_MAB);
> +
> +	if ((p->flags & BR_PORT_MAB) &&
> +	    (!(p->flags & BR_PORT_LOCKED) || !(p->flags & BR_LEARNING))) {
> +		NL_SET_ERR_MSG(extack, "MAB can only be enabled on a locked port
> with learning enabled");

It's a bit odd to get this message when turning off learning on a port 
with MAB on, e.g....

# bridge link set dev a2 learning off
Error: MAB can only be enabled on a locked port with learning enabled.

Re: [RFC PATCH net-next 01/16] bridge: Add MAC Authentication Bypass (MAB) support

[Ido Schimmel]

On Sun, Oct 30, 2022 at 11:09:31PM +0100, netdev@kapio-technology.com wrote:
> On 2022-10-25 12:00, Ido Schimmel wrote:
> > @@ -943,6 +946,14 @@ static int br_setport(struct net_bridge_port *p,
> > struct nlattr *tb[],
> >  	br_set_port_flag(p, tb, IFLA_BRPORT_NEIGH_SUPPRESS,
> > BR_NEIGH_SUPPRESS);
> >  	br_set_port_flag(p, tb, IFLA_BRPORT_ISOLATED, BR_ISOLATED);
> >  	br_set_port_flag(p, tb, IFLA_BRPORT_LOCKED, BR_PORT_LOCKED);
> > +	br_set_port_flag(p, tb, IFLA_BRPORT_MAB, BR_PORT_MAB);
> > +
> > +	if ((p->flags & BR_PORT_MAB) &&
> > +	    (!(p->flags & BR_PORT_LOCKED) || !(p->flags & BR_LEARNING))) {
> > +		NL_SET_ERR_MSG(extack, "MAB can only be enabled on a locked port
> > with learning enabled");
> 
> It's a bit odd to get this message when turning off learning on a port with
> MAB on, e.g....
> 
> # bridge link set dev a2 learning off
> Error: MAB can only be enabled on a locked port with learning enabled.

It's better if you suggest something else. How about:

"Bridge port must be locked and have learning enabled when MAB is enabled"

?

Re: [RFC PATCH net-next 01/16] bridge: Add MAC Authentication Bypass (MAB) support

[netdev]

On 2022-10-31 15:43, Ido Schimmel wrote:
> On Sun, Oct 30, 2022 at 11:09:31PM +0100, netdev@kapio-technology.com 
> wrote:
>> On 2022-10-25 12:00, Ido Schimmel wrote:
>> > @@ -943,6 +946,14 @@ static int br_setport(struct net_bridge_port *p,
>> > struct nlattr *tb[],
>> >  	br_set_port_flag(p, tb, IFLA_BRPORT_NEIGH_SUPPRESS,
>> > BR_NEIGH_SUPPRESS);
>> >  	br_set_port_flag(p, tb, IFLA_BRPORT_ISOLATED, BR_ISOLATED);
>> >  	br_set_port_flag(p, tb, IFLA_BRPORT_LOCKED, BR_PORT_LOCKED);
>> > +	br_set_port_flag(p, tb, IFLA_BRPORT_MAB, BR_PORT_MAB);
>> > +
>> > +	if ((p->flags & BR_PORT_MAB) &&
>> > +	    (!(p->flags & BR_PORT_LOCKED) || !(p->flags & BR_LEARNING))) {
>> > +		NL_SET_ERR_MSG(extack, "MAB can only be enabled on a locked port
>> > with learning enabled");
>> 
>> It's a bit odd to get this message when turning off learning on a port 
>> with
>> MAB on, e.g....
>> 
>> # bridge link set dev a2 learning off
>> Error: MAB can only be enabled on a locked port with learning enabled.
> 
> It's better if you suggest something else. How about:
> 
> "Bridge port must be locked and have learning enabled when MAB is 
> enabled"
> 
> ?

Yes, I think that is better in case it should not be split into more 
than one
message. At least it is not bound to a specific action.

[RFC PATCH net-next 02/16] selftests: forwarding: Add MAC Authentication Bypass (MAB) test cases

[Ido Schimmel]

From: "Hans J. Schultz" <netdev@kapio-technology.com>

Add three test cases to verify MAB functionality:

* Verify that a locked FDB entry can be generated by the bridge,
  preventing a host from communicating via the bridge. Test that user
  space can clear the "locked" flag by replacing the entry, thereby
  authenticating the host and allowing it to communicate via the bridge.

* Test that an entry cannot roam to a locked port, but that it can roam
  to an unlocked port.

* Test that MAB can only be enabled on a port that is both locked and
  has learning enabled.

Signed-off-by: Hans J. Schultz <netdev@kapio-technology.com>
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---

Notes:
    Changes made by me:
    
     * Reword commit message.
     * Remove blackhole tests as they are not relevant for this series.
     * Add configuration test case.
     * Add a few additional test cases.

 .../net/forwarding/bridge_locked_port.sh      | 101 +++++++++++++++++-
 tools/testing/selftests/net/forwarding/lib.sh |   8 ++
 2 files changed, 108 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/forwarding/bridge_locked_port.sh b/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
index 5b02b6b60ce7..0ad90007f96b 100755
--- a/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
+++ b/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
@@ -1,7 +1,15 @@
 #!/bin/bash
 # SPDX-License-Identifier: GPL-2.0
 
-ALL_TESTS="locked_port_ipv4 locked_port_ipv6 locked_port_vlan"
+ALL_TESTS="
+	locked_port_ipv4
+	locked_port_ipv6
+	locked_port_vlan
+	locked_port_mab
+	locked_port_mab_roam
+	locked_port_mab_config
+"
+
 NUM_NETIFS=4
 CHECK_TC="no"
 source lib.sh
@@ -166,6 +174,97 @@ locked_port_ipv6()
 	log_test "Locked port ipv6"
 }
 
+locked_port_mab()
+{
+	RET=0
+	check_port_mab_support || return 0
+
+	ping_do $h1 192.0.2.2
+	check_err $? "Ping did not work before locking port"
+
+	bridge link set dev $swp1 learning on locked on
+
+	ping_do $h1 192.0.2.2
+	check_fail $? "Ping worked on a locked port without an FDB entry"
+
+	bridge fdb get `mac_get $h1` br br0 vlan 1 &> /dev/null
+	check_fail $? "FDB entry created before enabling MAB"
+
+	bridge link set dev $swp1 learning on locked on mab on
+
+	ping_do $h1 192.0.2.2
+	check_fail $? "Ping worked on MAB enabled port without an FDB entry"
+
+	bridge fdb get `mac_get $h1` br br0 vlan 1 | grep "dev $swp1" | grep -q "locked"
+	check_err $? "Locked FDB entry not created"
+
+	bridge fdb replace `mac_get $h1` dev $swp1 master static
+
+	ping_do $h1 192.0.2.2
+	check_err $? "Ping did not work after replacing FDB entry"
+
+	bridge fdb get `mac_get $h1` br br0 vlan 1 | grep "dev $swp1" | grep -q "locked"
+	check_fail $? "FDB entry marked as locked after replacement"
+
+	bridge fdb del `mac_get $h1` dev $swp1 master
+	bridge link set dev $swp1 learning off locked off mab off
+
+	log_test "Locked port MAB"
+}
+
+# Check that entries cannot roam to a locked port, but that entries can roam
+# to an unlocked port.
+locked_port_mab_roam()
+{
+	local mac=a0:b0:c0:c0:b0:a0
+
+	RET=0
+	check_port_mab_support || return 0
+
+	bridge link set dev $swp1 learning on locked on mab on
+
+	$MZ $h1 -q -c 5 -d 100msec -t udp -a $mac -b rand
+	bridge fdb get $mac br br0 vlan 1 | grep "dev $swp1" | grep -q "locked"
+	check_err $? "No locked entry on first injection"
+
+	$MZ $h2 -q -c 5 -d 100msec -t udp -a $mac -b rand
+	bridge fdb get $mac br br0 vlan 1 | grep -q "dev $swp2"
+	check_err $? "Entry did not roam to an unlocked port"
+
+	bridge fdb get $mac br br0 vlan 1 | grep -q "locked"
+	check_fail $? "Entry roamed with locked flag on"
+
+	$MZ $h1 -q -c 5 -d 100msec -t udp -a $mac -b rand
+	bridge fdb get $mac br br0 vlan 1 | grep -q "dev $swp1"
+	check_fail $? "Entry roamed back to locked port"
+
+	bridge fdb del $mac vlan 1 dev $swp2 master
+	bridge link set dev $swp1 learning off locked off mab off
+
+	log_test "Locked port MAB roam"
+}
+
+# Check that MAB can only be enabled on a port that is both locked and has
+# learning enabled.
+locked_port_mab_config()
+{
+	RET=0
+	check_port_mab_support || return 0
+
+	bridge link set dev $swp1 learning on locked off mab on &> /dev/null
+	check_fail $? "MAB enabled while port is unlocked"
+
+	bridge link set dev $swp1 learning off locked on mab on &> /dev/null
+	check_fail $? "MAB enabled while port has learning disabled"
+
+	bridge link set dev $swp1 learning on locked on mab on
+	check_err $? "Failed to enable MAB when port is locked and has learning enabled"
+
+	bridge link set dev $swp1 learning off locked off mab off
+
+	log_test "Locked port MAB configuration"
+}
+
 trap cleanup EXIT
 
 setup_prepare
diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
index 3ffb9d6c0950..1c4f866de7d7 100755
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -137,6 +137,14 @@ check_locked_port_support()
 	fi
 }
 
+check_port_mab_support()
+{
+	if ! bridge -d link show | grep -q "mab"; then
+		echo "SKIP: iproute2 too old; MacAuth feature not supported."
+		return $ksft_skip
+	fi
+}
+
 if [[ "$(id -u)" -ne 0 ]]; then
 	echo "SKIP: need root privileges"
 	exit $ksft_skip
-- 
2.37.3

[RFC PATCH net-next 03/16] bridge: switchdev: Let device drivers determine FDB offload indication

[Ido Schimmel]

Currently, FDB entries that are notified to the bridge via
'SWITCHDEV_FDB_ADD_TO_BRIDGE' are always marked as offloaded. With MAB
enabled, this will no longer be universally true. Device drivers will
report locked FDB entries to the bridge to let it know that the
corresponding hosts required authorization, but it does not mean that
these entries are necessarily programmed in the underlying hardware.

Solve this by determining the offload indication based of the
'offloaded' bit in the FDB notification.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---

Notes:
    Needs auditing to see which device drivers are not setting this bit.

 net/bridge/br.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/bridge/br.c b/net/bridge/br.c
index 96e91d69a9a8..145999b8c355 100644
--- a/net/bridge/br.c
+++ b/net/bridge/br.c
@@ -172,7 +172,7 @@ static int br_switchdev_event(struct notifier_block *unused,
 			break;
 		}
 		br_fdb_offloaded_set(br, p, fdb_info->addr,
-				     fdb_info->vid, true);
+				     fdb_info->vid, fdb_info->offloaded);
 		break;
 	case SWITCHDEV_FDB_DEL_TO_BRIDGE:
 		fdb_info = ptr;
-- 
2.37.3

Re: [RFC PATCH net-next 03/16] bridge: switchdev: Let device drivers determine FDB offload indication

[Vladimir Oltean]

On Tue, Oct 25, 2022 at 01:00:11PM +0300, Ido Schimmel wrote:
> Currently, FDB entries that are notified to the bridge via
> 'SWITCHDEV_FDB_ADD_TO_BRIDGE' are always marked as offloaded. With MAB
> enabled, this will no longer be universally true. Device drivers will
> report locked FDB entries to the bridge to let it know that the
> corresponding hosts required authorization, but it does not mean that
> these entries are necessarily programmed in the underlying hardware.
> 
> Solve this by determining the offload indication based of the
> 'offloaded' bit in the FDB notification.
> 
> Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> ---
> 
> Notes:
>     Needs auditing to see which device drivers are not setting this bit.
> 
>  net/bridge/br.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/bridge/br.c b/net/bridge/br.c
> index 96e91d69a9a8..145999b8c355 100644
> --- a/net/bridge/br.c
> +++ b/net/bridge/br.c
> @@ -172,7 +172,7 @@ static int br_switchdev_event(struct notifier_block *unused,
>  			break;
>  		}
>  		br_fdb_offloaded_set(br, p, fdb_info->addr,
> -				     fdb_info->vid, true);
> +				     fdb_info->vid, fdb_info->offloaded);

ofdpa_port_fdb_learn_work() doesn't set info->offloaded on
SWITCHDEV_FDB_ADD_TO_BRIDGE, the rest do.

>  		break;
>  	case SWITCHDEV_FDB_DEL_TO_BRIDGE:
>  		fdb_info = ptr;
> -- 
> 2.37.3
>

Re: [RFC PATCH net-next 03/16] bridge: switchdev: Let device drivers determine FDB offload indication

[Ido Schimmel]

On Thu, Oct 27, 2022 at 11:10:41PM +0000, Vladimir Oltean wrote:
> On Tue, Oct 25, 2022 at 01:00:11PM +0300, Ido Schimmel wrote:
> > Currently, FDB entries that are notified to the bridge via
> > 'SWITCHDEV_FDB_ADD_TO_BRIDGE' are always marked as offloaded. With MAB
> > enabled, this will no longer be universally true. Device drivers will
> > report locked FDB entries to the bridge to let it know that the
> > corresponding hosts required authorization, but it does not mean that
> > these entries are necessarily programmed in the underlying hardware.
> > 
> > Solve this by determining the offload indication based of the
> > 'offloaded' bit in the FDB notification.
> > 
> > Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> > ---
> > 
> > Notes:
> >     Needs auditing to see which device drivers are not setting this bit.
> > 
> >  net/bridge/br.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/net/bridge/br.c b/net/bridge/br.c
> > index 96e91d69a9a8..145999b8c355 100644
> > --- a/net/bridge/br.c
> > +++ b/net/bridge/br.c
> > @@ -172,7 +172,7 @@ static int br_switchdev_event(struct notifier_block *unused,
> >  			break;
> >  		}
> >  		br_fdb_offloaded_set(br, p, fdb_info->addr,
> > -				     fdb_info->vid, true);
> > +				     fdb_info->vid, fdb_info->offloaded);
> 
> ofdpa_port_fdb_learn_work() doesn't set info->offloaded on
> SWITCHDEV_FDB_ADD_TO_BRIDGE, the rest do.

Double-checked and this is the only one missing. Will send a patch.
Thanks

> 
> >  		break;
> >  	case SWITCHDEV_FDB_DEL_TO_BRIDGE:
> >  		fdb_info = ptr;
> > -- 
> > 2.37.3
> >

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

[Ido Schimmel]

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.

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.

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>
---

Notes:
    Changes made by me:
    
     * Reword commit message.
     * Forbid locked entries when MAB is not enabled.
     * Forbid roaming of locked entries.
     * Avoid setting 'locked' bit towards device drivers.

 include/net/switchdev.h   |  1 +
 net/bridge/br.c           |  3 ++-
 net/bridge/br_fdb.c       | 22 ++++++++++++++++++++--
 net/bridge/br_private.h   |  2 +-
 net/bridge/br_switchdev.c |  1 +
 5 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index 7dcdc97c0bc3..ca0312b78294 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -248,6 +248,7 @@ struct switchdev_notifier_fdb_info {
 	u16 vid;
 	u8 added_by_user:1,
 	   is_local:1,
+	   locked:1,
 	   offloaded:1;
 };
 
diff --git a/net/bridge/br.c b/net/bridge/br.c
index 145999b8c355..4f5098d33a46 100644
--- a/net/bridge/br.c
+++ b/net/bridge/br.c
@@ -166,7 +166,8 @@ static int br_switchdev_event(struct notifier_block *unused,
 	case SWITCHDEV_FDB_ADD_TO_BRIDGE:
 		fdb_info = ptr;
 		err = br_fdb_external_learn_add(br, p, fdb_info->addr,
-						fdb_info->vid, false);
+						fdb_info->vid,
+						fdb_info->locked, false);
 		if (err) {
 			err = notifier_from_errno(err);
 			break;
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 3b83af4458b8..e69a872bfc1d 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -1139,7 +1139,7 @@ static int __br_fdb_add(struct ndmsg *ndm, struct net_bridge *br,
 					   "FDB entry towards bridge must be permanent");
 			return -EINVAL;
 		}
-		err = br_fdb_external_learn_add(br, p, addr, vid, true);
+		err = br_fdb_external_learn_add(br, p, addr, vid, false, true);
 	} else {
 		spin_lock_bh(&br->hash_lock);
 		err = fdb_add_entry(br, p, addr, ndm, nlh_flags, vid, nfea_tb);
@@ -1377,7 +1377,7 @@ 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,
+			      const unsigned char *addr, u16 vid, bool locked,
 			      bool swdev_notify)
 {
 	struct net_bridge_fdb_entry *fdb;
@@ -1386,6 +1386,9 @@ int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
 
 	trace_br_fdb_external_learn_add(br, p, addr, vid);
 
+	if (locked && (!p || !(p->flags & BR_PORT_MAB)))
+		return -EINVAL;
+
 	spin_lock_bh(&br->hash_lock);
 
 	fdb = br_fdb_find(br, addr, vid);
@@ -1398,6 +1401,9 @@ int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
 		if (!p)
 			flags |= BIT(BR_FDB_LOCAL);
 
+		if (locked)
+			flags |= BIT(BR_FDB_LOCKED);
+
 		fdb = fdb_create(br, p, addr, vid, flags);
 		if (!fdb) {
 			err = -ENOMEM;
@@ -1405,6 +1411,13 @@ int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
 		}
 		fdb_notify(br, fdb, RTM_NEWNEIGH, swdev_notify);
 	} else {
+		if (locked &&
+		    (!test_bit(BR_FDB_LOCKED, &fdb->flags) ||
+		     READ_ONCE(fdb->dst) != p)) {
+			err = -EINVAL;
+			goto err_unlock;
+		}
+
 		fdb->updated = jiffies;
 
 		if (READ_ONCE(fdb->dst) != p) {
@@ -1421,6 +1434,11 @@ int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
 			modified = true;
 		}
 
+		if (locked != test_bit(BR_FDB_LOCKED, &fdb->flags)) {
+			change_bit(BR_FDB_LOCKED, &fdb->flags);
+			modified = true;
+		}
+
 		if (swdev_notify)
 			set_bit(BR_FDB_ADDED_BY_USER, &fdb->flags);
 
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;
 	item->info.dev = (!p || item->is_local) ? br->dev : p->dev;
 	item->info.ctx = ctx;
 }
-- 
2.37.3

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

[Nikolay Aleksandrov]

On 25/10/2022 13:00, 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.
> 
> 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.
> 
> 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>
> ---
> 
> Notes:
>     Changes made by me:
>     
>      * Reword commit message.
>      * Forbid locked entries when MAB is not enabled.
>      * Forbid roaming of locked entries.
>      * Avoid setting 'locked' bit towards device drivers.
> 
>  include/net/switchdev.h   |  1 +
>  net/bridge/br.c           |  3 ++-
>  net/bridge/br_fdb.c       | 22 ++++++++++++++++++++--
>  net/bridge/br_private.h   |  2 +-
>  net/bridge/br_switchdev.c |  1 +
>  5 files changed, 25 insertions(+), 4 deletions(-)
> 

Acked-by: Nikolay Aleksandrov <razor@blackwall.org>

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

[Vladimir Oltean]

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
>

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

[Ido Schimmel]

On Thu, Oct 27, 2022 at 11:27:48PM +0000, Vladimir Oltean wrote:
> 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.

Good idea, will add a check in br_switchdev_fdb_notify().

> 
> > 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?

At least for Spectrum, no special configuration is required for MAB
compared to a locked port with learning enabled. Learning notifications
will always be generated by the device and the driver will report them
as "locked" entries to the bridge driver, which will decide whether to
install them or not based on the 'BR_PORT_MAB' flag.

Once we have a driver that needs to differentiate between a locked port
with learning enabled and a port with MAB enabled, we can start passing
'BR_PORT_MAB' to drivers. Should be an easy change.

> 
> > 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.

Will change to false.

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

[RFC PATCH net-next 05/16] devlink: Add packet traps for 802.1X operation

[Ido Schimmel]

Add packet traps for 802.1X operation. The "eapol" control trap is used
to trap EAPOL packets and is required for the correct operation of the
control plane. The "locked_port" drop trap can be enabled to gain
visibility into packets that were dropped by the device due to the
locked bridge port check.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 Documentation/networking/devlink/devlink-trap.rst | 13 +++++++++++++
 include/net/devlink.h                             |  9 +++++++++
 net/core/devlink.c                                |  3 +++
 3 files changed, 25 insertions(+)

diff --git a/Documentation/networking/devlink/devlink-trap.rst b/Documentation/networking/devlink/devlink-trap.rst
index 90d1381b88de..2c14dfe69b3a 100644
--- a/Documentation/networking/devlink/devlink-trap.rst
+++ b/Documentation/networking/devlink/devlink-trap.rst
@@ -485,6 +485,16 @@ be added to the following table:
      - Traps incoming packets that the device decided to drop because
        the destination MAC is not configured in the MAC table and
        the interface is not in promiscuous mode
+   * - ``eapol``
+     - ``control``
+     - Traps "Extensible Authentication Protocol over LAN" (EAPOL) packets
+       specified in IEEE 802.1X
+   * - ``locked_port``
+     - ``drop``
+     - Traps packets that the device decided to drop because they failed the
+       locked bridge port check. That is, packets that were received via a
+       locked port and whose {SMAC, VID} does not correspond to an FDB entry
+       pointing to the port
 
 Driver-specific Packet Traps
 ============================
@@ -589,6 +599,9 @@ narrow. The description of these groups must be added to the following table:
    * - ``parser_error_drops``
      - Contains packet traps for packets that were marked by the device during
        parsing as erroneous
+   * - ``eapol``
+     - Contains packet traps for "Extensible Authentication Protocol over LAN"
+       (EAPOL) packets specified in IEEE 802.1X
 
 Packet Trap Policers
 ====================
diff --git a/include/net/devlink.h b/include/net/devlink.h
index ba6b8b094943..afd9999a9b2a 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -885,6 +885,8 @@ enum devlink_trap_generic_id {
 	DEVLINK_TRAP_GENERIC_ID_ESP_PARSING,
 	DEVLINK_TRAP_GENERIC_ID_BLACKHOLE_NEXTHOP,
 	DEVLINK_TRAP_GENERIC_ID_DMAC_FILTER,
+	DEVLINK_TRAP_GENERIC_ID_EAPOL,
+	DEVLINK_TRAP_GENERIC_ID_LOCKED_PORT,
 
 	/* Add new generic trap IDs above */
 	__DEVLINK_TRAP_GENERIC_ID_MAX,
@@ -921,6 +923,7 @@ enum devlink_trap_group_generic_id {
 	DEVLINK_TRAP_GROUP_GENERIC_ID_ACL_SAMPLE,
 	DEVLINK_TRAP_GROUP_GENERIC_ID_ACL_TRAP,
 	DEVLINK_TRAP_GROUP_GENERIC_ID_PARSER_ERROR_DROPS,
+	DEVLINK_TRAP_GROUP_GENERIC_ID_EAPOL,
 
 	/* Add new generic trap group IDs above */
 	__DEVLINK_TRAP_GROUP_GENERIC_ID_MAX,
@@ -1112,6 +1115,10 @@ enum devlink_trap_group_generic_id {
 	"blackhole_nexthop"
 #define DEVLINK_TRAP_GENERIC_NAME_DMAC_FILTER \
 	"dmac_filter"
+#define DEVLINK_TRAP_GENERIC_NAME_EAPOL \
+	"eapol"
+#define DEVLINK_TRAP_GENERIC_NAME_LOCKED_PORT \
+	"locked_port"
 
 #define DEVLINK_TRAP_GROUP_GENERIC_NAME_L2_DROPS \
 	"l2_drops"
@@ -1165,6 +1172,8 @@ enum devlink_trap_group_generic_id {
 	"acl_trap"
 #define DEVLINK_TRAP_GROUP_GENERIC_NAME_PARSER_ERROR_DROPS \
 	"parser_error_drops"
+#define DEVLINK_TRAP_GROUP_GENERIC_NAME_EAPOL \
+	"eapol"
 
 #define DEVLINK_TRAP_GENERIC(_type, _init_action, _id, _group_id,	      \
 			     _metadata_cap)				      \
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 89baa7c0938b..dc54182f2e57 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -11624,6 +11624,8 @@ static const struct devlink_trap devlink_trap_generic[] = {
 	DEVLINK_TRAP(ESP_PARSING, DROP),
 	DEVLINK_TRAP(BLACKHOLE_NEXTHOP, DROP),
 	DEVLINK_TRAP(DMAC_FILTER, DROP),
+	DEVLINK_TRAP(EAPOL, CONTROL),
+	DEVLINK_TRAP(LOCKED_PORT, DROP),
 };
 
 #define DEVLINK_TRAP_GROUP(_id)						      \
@@ -11659,6 +11661,7 @@ static const struct devlink_trap_group devlink_trap_group_generic[] = {
 	DEVLINK_TRAP_GROUP(ACL_SAMPLE),
 	DEVLINK_TRAP_GROUP(ACL_TRAP),
 	DEVLINK_TRAP_GROUP(PARSER_ERROR_DROPS),
+	DEVLINK_TRAP_GROUP(EAPOL),
 };
 
 static int devlink_trap_generic_verify(const struct devlink_trap *trap)
-- 
2.37.3

[RFC PATCH net-next 06/16] mlxsw: spectrum_trap: Register 802.1X packet traps with devlink

[Ido Schimmel]

Register the previously added packet traps with devlink. This allows
user space to tune their policers and in the case of the locked port
trap, user space can set its action to "trap" in order to gain
visibility into packets that were discarded by the device due to the
locked port check failure.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlxsw/reg.h     |  1 +
 .../ethernet/mellanox/mlxsw/spectrum_trap.c   | 25 +++++++++++++++++++
 drivers/net/ethernet/mellanox/mlxsw/trap.h    |  2 ++
 3 files changed, 28 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/reg.h b/drivers/net/ethernet/mellanox/mlxsw/reg.h
index b74f30ec629a..7240af45ade5 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/reg.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/reg.h
@@ -6316,6 +6316,7 @@ enum mlxsw_reg_htgt_trap_group {
 	MLXSW_REG_HTGT_TRAP_GROUP_SP_TUNNEL_DISCARDS,
 	MLXSW_REG_HTGT_TRAP_GROUP_SP_ACL_DISCARDS,
 	MLXSW_REG_HTGT_TRAP_GROUP_SP_BUFFER_DISCARDS,
+	MLXSW_REG_HTGT_TRAP_GROUP_SP_EAPOL,
 
 	__MLXSW_REG_HTGT_TRAP_GROUP_MAX,
 	MLXSW_REG_HTGT_TRAP_GROUP_MAX = __MLXSW_REG_HTGT_TRAP_GROUP_MAX - 1
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_trap.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_trap.c
index f4bfdb6dab9c..899c954e0e5f 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_trap.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_trap.c
@@ -510,6 +510,9 @@ mlxsw_sp_trap_policer_items_arr[] = {
 	{
 		.policer = MLXSW_SP_TRAP_POLICER(20, 10240, 4096),
 	},
+	{
+		.policer = MLXSW_SP_TRAP_POLICER(21, 128, 128),
+	},
 };
 
 static const struct mlxsw_sp_trap_group_item mlxsw_sp_trap_group_items_arr[] = {
@@ -628,6 +631,11 @@ static const struct mlxsw_sp_trap_group_item mlxsw_sp_trap_group_items_arr[] = {
 		.hw_group_id = MLXSW_REG_HTGT_TRAP_GROUP_SP_FLOW_LOGGING,
 		.priority = 4,
 	},
+	{
+		.group = DEVLINK_TRAP_GROUP_GENERIC(EAPOL, 21),
+		.hw_group_id = MLXSW_REG_HTGT_TRAP_GROUP_SP_EAPOL,
+		.priority = 5,
+	},
 };
 
 static const struct mlxsw_sp_trap_item mlxsw_sp_trap_items_arr[] = {
@@ -1160,6 +1168,23 @@ static const struct mlxsw_sp_trap_item mlxsw_sp_trap_items_arr[] = {
 			MLXSW_SP_RXL_DISCARD(ROUTER3, L3_DISCARDS),
 		},
 	},
+	{
+		.trap = MLXSW_SP_TRAP_CONTROL(EAPOL, EAPOL, TRAP),
+		.listeners_arr = {
+			MLXSW_SP_RXL_NO_MARK(EAPOL, EAPOL, TRAP_TO_CPU, true),
+		},
+	},
+	{
+		.trap = MLXSW_SP_TRAP_DROP(LOCKED_PORT, L2_DROPS),
+		.listeners_arr = {
+			MLXSW_RXL_DIS(mlxsw_sp_rx_drop_listener, FDB_MISS,
+				      TRAP_EXCEPTION_TO_CPU, false,
+				      SP_L2_DISCARDS, DISCARD, SP_L2_DISCARDS),
+			MLXSW_RXL_DIS(mlxsw_sp_rx_drop_listener, FDB_MISMATCH,
+				      TRAP_EXCEPTION_TO_CPU, false,
+				      SP_L2_DISCARDS, DISCARD, SP_L2_DISCARDS),
+		},
+	},
 };
 
 static struct mlxsw_sp_trap_policer_item *
diff --git a/drivers/net/ethernet/mellanox/mlxsw/trap.h b/drivers/net/ethernet/mellanox/mlxsw/trap.h
index 8da169663bda..83477c8e6971 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/trap.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/trap.h
@@ -25,6 +25,8 @@ enum {
 	MLXSW_TRAP_ID_IGMP_V2_LEAVE = 0x33,
 	MLXSW_TRAP_ID_IGMP_V3_REPORT = 0x34,
 	MLXSW_TRAP_ID_PKT_SAMPLE = 0x38,
+	MLXSW_TRAP_ID_FDB_MISS = 0x3A,
+	MLXSW_TRAP_ID_FDB_MISMATCH = 0x3B,
 	MLXSW_TRAP_ID_FID_MISS = 0x3D,
 	MLXSW_TRAP_ID_DECAP_ECN0 = 0x40,
 	MLXSW_TRAP_ID_MTUERROR = 0x52,
-- 
2.37.3

[RFC PATCH net-next 07/16] mlxsw: reg: Add Switch Port FDB Security Register

[Ido Schimmel]

Add the Switch Port FDB Security Register (SPFSR) that allows enabling
and disabling security checks on a given local port. In Linux terms, it
allows locking / unlocking a port.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlxsw/reg.h | 34 +++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/reg.h b/drivers/net/ethernet/mellanox/mlxsw/reg.h
index 7240af45ade5..f2d6f8654e04 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/reg.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/reg.h
@@ -2046,6 +2046,39 @@ static inline void mlxsw_reg_spvmlr_pack(char *payload, u16 local_port,
 	}
 }
 
+/* SPFSR - Switch Port FDB Security Register
+ * -----------------------------------------
+ * Configures the security mode per port.
+ */
+#define MLXSW_REG_SPFSR_ID 0x2023
+#define MLXSW_REG_SPFSR_LEN 0x08
+
+MLXSW_REG_DEFINE(spfsr, MLXSW_REG_SPFSR_ID, MLXSW_REG_SPFSR_LEN);
+
+/* reg_spfsr_local_port
+ * Local port.
+ * Access: Index
+ *
+ * Note: not supported for CPU port.
+ */
+MLXSW_ITEM32_LP(reg, spfsr, 0x00, 16, 0x00, 12);
+
+/* reg_spfsr_security
+ * Security checks.
+ * 0: disabled (default)
+ * 1: enabled
+ * Access: RW
+ */
+MLXSW_ITEM32(reg, spfsr, security, 0x04, 31, 1);
+
+static inline void mlxsw_reg_spfsr_pack(char *payload, u16 local_port,
+					bool security)
+{
+	MLXSW_REG_ZERO(spfsr, payload);
+	mlxsw_reg_spfsr_local_port_set(payload, local_port);
+	mlxsw_reg_spfsr_security_set(payload, security);
+}
+
 /* SPVC - Switch Port VLAN Classification Register
  * -----------------------------------------------
  * Configures the port to identify packets as untagged / single tagged /
@@ -12762,6 +12795,7 @@ static const struct mlxsw_reg_info *mlxsw_reg_infos[] = {
 	MLXSW_REG(svpe),
 	MLXSW_REG(sfmr),
 	MLXSW_REG(spvmlr),
+	MLXSW_REG(spfsr),
 	MLXSW_REG(spvc),
 	MLXSW_REG(spevet),
 	MLXSW_REG(smpe),
-- 
2.37.3

[RFC PATCH net-next 08/16] mlxsw: spectrum: Add an API to configure security checks

[Ido Schimmel]

Add an API to enable or disable security checks on a local port. It will
be used by subsequent patches when the 'BR_PORT_LOCKED' flag is toggled.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlxsw/spectrum.c | 18 ++++++++++++++++++
 drivers/net/ethernet/mellanox/mlxsw/spectrum.h |  5 ++++-
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
index 5bcf5bceff71..10f438bc83dd 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
@@ -466,6 +466,24 @@ int mlxsw_sp_port_vid_learning_set(struct mlxsw_sp_port *mlxsw_sp_port, u16 vid,
 	return err;
 }
 
+int mlxsw_sp_port_security_set(struct mlxsw_sp_port *mlxsw_sp_port, bool enable)
+{
+	struct mlxsw_sp *mlxsw_sp = mlxsw_sp_port->mlxsw_sp;
+	char spfsr_pl[MLXSW_REG_SPFSR_LEN];
+	int err;
+
+	if (mlxsw_sp_port->security == enable)
+		return 0;
+
+	mlxsw_reg_spfsr_pack(spfsr_pl, mlxsw_sp_port->local_port, enable);
+	err = mlxsw_reg_write(mlxsw_sp->core, MLXSW_REG(spfsr), spfsr_pl);
+	if (err)
+		return err;
+
+	mlxsw_sp_port->security = enable;
+	return 0;
+}
+
 int mlxsw_sp_ethtype_to_sver_type(u16 ethtype, u8 *p_sver_type)
 {
 	switch (ethtype) {
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
index c8ff2a6d7e90..bbc73324451d 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
@@ -321,7 +321,8 @@ struct mlxsw_sp_port {
 	struct mlxsw_sp *mlxsw_sp;
 	u16 local_port;
 	u8 lagged:1,
-	   split:1;
+	   split:1,
+	   security:1;
 	u16 pvid;
 	u16 lag_id;
 	struct {
@@ -687,6 +688,8 @@ int mlxsw_sp_port_vid_stp_set(struct mlxsw_sp_port *mlxsw_sp_port, u16 vid,
 int mlxsw_sp_port_vp_mode_set(struct mlxsw_sp_port *mlxsw_sp_port, bool enable);
 int mlxsw_sp_port_vid_learning_set(struct mlxsw_sp_port *mlxsw_sp_port, u16 vid,
 				   bool learn_enable);
+int mlxsw_sp_port_security_set(struct mlxsw_sp_port *mlxsw_sp_port,
+			       bool enable);
 int mlxsw_sp_ethtype_to_sver_type(u16 ethtype, u8 *p_sver_type);
 int mlxsw_sp_port_egress_ethtype_set(struct mlxsw_sp_port *mlxsw_sp_port,
 				     u16 ethtype);
-- 
2.37.3

[RFC PATCH net-next 09/16] mlxsw: spectrum_switchdev: Prepare for locked FDB notifications

[Ido Schimmel]

Subsequent patches will need to report locked FDB entries to the bridge
driver. Prepare for that by adding a 'locked' argument to
mlxsw_sp_fdb_call_notifiers() according to which the 'locked' bit is set
in the FDB notification info. For now, always pass 'false'.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 .../mellanox/mlxsw/spectrum_switchdev.c       | 21 ++++++++++++-------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
index 4efccd942fb8..0fbefa43f9b1 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
@@ -2888,13 +2888,14 @@ static void mlxsw_sp_fdb_nve_call_notifiers(struct net_device *dev,
 static void
 mlxsw_sp_fdb_call_notifiers(enum switchdev_notifier_type type,
 			    const char *mac, u16 vid,
-			    struct net_device *dev, bool offloaded)
+			    struct net_device *dev, bool offloaded, bool locked)
 {
 	struct switchdev_notifier_fdb_info info = {};
 
 	info.addr = mac;
 	info.vid = vid;
 	info.offloaded = offloaded;
+	info.locked = locked;
 	call_switchdev_notifiers(type, dev, &info.info, NULL);
 }
 
@@ -2952,7 +2953,8 @@ static void mlxsw_sp_fdb_notify_mac_process(struct mlxsw_sp *mlxsw_sp,
 	if (!do_notification)
 		return;
 	type = adding ? SWITCHDEV_FDB_ADD_TO_BRIDGE : SWITCHDEV_FDB_DEL_TO_BRIDGE;
-	mlxsw_sp_fdb_call_notifiers(type, mac, vid, bridge_port->dev, adding);
+	mlxsw_sp_fdb_call_notifiers(type, mac, vid, bridge_port->dev, adding,
+				    false);
 
 	return;
 
@@ -3015,7 +3017,8 @@ static void mlxsw_sp_fdb_notify_mac_lag_process(struct mlxsw_sp *mlxsw_sp,
 	if (!do_notification)
 		return;
 	type = adding ? SWITCHDEV_FDB_ADD_TO_BRIDGE : SWITCHDEV_FDB_DEL_TO_BRIDGE;
-	mlxsw_sp_fdb_call_notifiers(type, mac, vid, bridge_port->dev, adding);
+	mlxsw_sp_fdb_call_notifiers(type, mac, vid, bridge_port->dev, adding,
+				    false);
 
 	return;
 
@@ -3122,7 +3125,7 @@ static void mlxsw_sp_fdb_notify_mac_uc_tunnel_process(struct mlxsw_sp *mlxsw_sp,
 
 	type = adding ? SWITCHDEV_FDB_ADD_TO_BRIDGE :
 			SWITCHDEV_FDB_DEL_TO_BRIDGE;
-	mlxsw_sp_fdb_call_notifiers(type, mac, vid, nve_dev, adding);
+	mlxsw_sp_fdb_call_notifiers(type, mac, vid, nve_dev, adding, false);
 
 	mlxsw_sp_fid_put(fid);
 
@@ -3264,7 +3267,7 @@ mlxsw_sp_switchdev_bridge_vxlan_fdb_event(struct mlxsw_sp *mlxsw_sp,
 					 &vxlan_fdb_info.info, NULL);
 		mlxsw_sp_fdb_call_notifiers(SWITCHDEV_FDB_OFFLOADED,
 					    vxlan_fdb_info.eth_addr,
-					    fdb_info->vid, dev, true);
+					    fdb_info->vid, dev, true, false);
 		break;
 	case SWITCHDEV_FDB_DEL_TO_DEVICE:
 		err = mlxsw_sp_port_fdb_tunnel_uc_op(mlxsw_sp,
@@ -3359,7 +3362,7 @@ static void mlxsw_sp_switchdev_bridge_fdb_event_work(struct work_struct *work)
 			break;
 		mlxsw_sp_fdb_call_notifiers(SWITCHDEV_FDB_OFFLOADED,
 					    fdb_info->addr,
-					    fdb_info->vid, dev, true);
+					    fdb_info->vid, dev, true, false);
 		break;
 	case SWITCHDEV_FDB_DEL_TO_DEVICE:
 		fdb_info = &switchdev_work->fdb_info;
@@ -3443,7 +3446,8 @@ mlxsw_sp_switchdev_vxlan_fdb_add(struct mlxsw_sp *mlxsw_sp,
 	call_switchdev_notifiers(SWITCHDEV_VXLAN_FDB_OFFLOADED, dev,
 				 &vxlan_fdb_info->info, NULL);
 	mlxsw_sp_fdb_call_notifiers(SWITCHDEV_FDB_OFFLOADED,
-				    vxlan_fdb_info->eth_addr, vid, dev, true);
+				    vxlan_fdb_info->eth_addr, vid, dev, true,
+				    false);
 
 	mlxsw_sp_fid_put(fid);
 
@@ -3493,7 +3497,8 @@ mlxsw_sp_switchdev_vxlan_fdb_del(struct mlxsw_sp *mlxsw_sp,
 				       false, false);
 	vid = bridge_device->ops->fid_vid(bridge_device, fid);
 	mlxsw_sp_fdb_call_notifiers(SWITCHDEV_FDB_OFFLOADED,
-				    vxlan_fdb_info->eth_addr, vid, dev, false);
+				    vxlan_fdb_info->eth_addr, vid, dev, false,
+				    false);
 
 	mlxsw_sp_fid_put(fid);
 }
-- 
2.37.3

[RFC PATCH net-next 10/16] mlxsw: spectrum_switchdev: Add support for locked FDB notifications

[Ido Schimmel]

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(+)

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);
-- 
2.37.3

Re: [RFC PATCH net-next 10/16] mlxsw: spectrum_switchdev: Add support for locked FDB notifications

[Vladimir Oltean]

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?

Re: [RFC PATCH net-next 10/16] mlxsw: spectrum_switchdev: Add support for locked FDB notifications

[Ido Schimmel]

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.

Re: [RFC PATCH net-next 10/16] mlxsw: spectrum_switchdev: Add support for locked FDB notifications

[Vladimir Oltean]

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.

Re: [RFC PATCH net-next 10/16] mlxsw: spectrum_switchdev: Add support for locked FDB notifications

[Vladimir Oltean]

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.

Re: [RFC PATCH net-next 10/16] mlxsw: spectrum_switchdev: Add support for locked FDB notifications

[Ido Schimmel]

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)

Re: [RFC PATCH net-next 10/16] mlxsw: spectrum_switchdev: Add support for locked FDB notifications

[Vladimir Oltean]

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.

[RFC PATCH net-next 11/16] mlxsw: spectrum_switchdev: Use extack in bridge port flag validation

[Ido Schimmel]

Propagate extack to mlxsw_sp_port_attr_br_pre_flags_set() in order to
communicate error messages related to bridge port flag validation.

Example:

 # bridge link set dev swp1 locked on
 Error: mlxsw_spectrum: Unsupported bridge port flag.

More error messages will be added in subsequent patches.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 .../net/ethernet/mellanox/mlxsw/spectrum_switchdev.c   | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
index f336be77019f..db149af7c888 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
@@ -782,10 +782,13 @@ mlxsw_sp_bridge_port_learning_set(struct mlxsw_sp_port *mlxsw_sp_port,
 
 static int
 mlxsw_sp_port_attr_br_pre_flags_set(struct mlxsw_sp_port *mlxsw_sp_port,
-				    struct switchdev_brport_flags flags)
+				    struct switchdev_brport_flags flags,
+				    struct netlink_ext_ack *extack)
 {
-	if (flags.mask & ~(BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD))
+	if (flags.mask & ~(BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD)) {
+		NL_SET_ERR_MSG_MOD(extack, "Unsupported bridge port flag");
 		return -EINVAL;
+	}
 
 	return 0;
 }
@@ -1186,7 +1189,8 @@ static int mlxsw_sp_port_attr_set(struct net_device *dev, const void *ctx,
 		break;
 	case SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS:
 		err = mlxsw_sp_port_attr_br_pre_flags_set(mlxsw_sp_port,
-							  attr->u.brport_flags);
+							  attr->u.brport_flags,
+							  extack);
 		break;
 	case SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS:
 		err = mlxsw_sp_port_attr_br_flags_set(mlxsw_sp_port,
-- 
2.37.3

[RFC PATCH net-next 12/16] mlxsw: spectrum_switchdev: Add locked bridge port support

[Ido Schimmel]

Add locked bridge port support by reacting to changes in the
'BR_PORT_LOCKED' flag. When set, enable security checks on the local
port via the previously added SPFSR register.

When security checks are enabled, an incoming packet will trigger an FDB
lookup with the packet's source MAC and the FID it was classified to. If
an FDB entry was not found or was found to be pointing to a different
port, the packet will be dropped. Such packets increment the
"discard_ingress_general" ethtool counter. For added visibility, user
space can trap such packets to the CPU by enabling the "locked_port"
trap. Example:

 # devlink trap set pci/0000:06:00.0 trap locked_port action trap

Unlike other configurations done via bridge port flags (e.g., learning,
flooding), security checks are enabled in the device on a per-port basis
and not on a per-{port, VLAN} basis. As such, scenarios where user space
can configure different locking settings for different VLANs configured
on a port need to be vetoed. To that end, veto the following scenarios:

1. Locking is set on a bridge port that is a VLAN upper

2. Locking is set on a bridge port that has VLAN uppers

3. VLAN upper is configured on a locked bridge port

Examples:

 # bridge link set dev swp1.10 locked on
 Error: mlxsw_spectrum: Locked flag cannot be set on a VLAN upper.

 # ip link add link swp1 name swp1.10 type vlan id 10
 # bridge link set dev swp1 locked on
 Error: mlxsw_spectrum: Locked flag cannot be set on a bridge port that has VLAN uppers.

 # bridge link set dev swp1 locked on
 # ip link add link swp1 name swp1.10 type vlan id 10
 Error: mlxsw_spectrum: VLAN uppers are not supported on a locked port.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 .../net/ethernet/mellanox/mlxsw/spectrum.c    |  4 ++++
 .../mellanox/mlxsw/spectrum_switchdev.c       | 23 ++++++++++++++++++-
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
index 10f438bc83dd..7ca96a4937b9 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
@@ -4772,6 +4772,10 @@ static int mlxsw_sp_netdevice_port_upper_event(struct net_device *lower_dev,
 			NL_SET_ERR_MSG_MOD(extack, "VLAN uppers are only supported with 802.1q VLAN protocol");
 			return -EOPNOTSUPP;
 		}
+		if (is_vlan_dev(upper_dev) && mlxsw_sp_port->security) {
+			NL_SET_ERR_MSG_MOD(extack, "VLAN uppers are not supported on a locked port");
+			return -EOPNOTSUPP;
+		}
 		break;
 	case NETDEV_CHANGEUPPER:
 		upper_dev = info->upper_dev;
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
index db149af7c888..70b4f8c4b038 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
@@ -782,14 +782,26 @@ mlxsw_sp_bridge_port_learning_set(struct mlxsw_sp_port *mlxsw_sp_port,
 
 static int
 mlxsw_sp_port_attr_br_pre_flags_set(struct mlxsw_sp_port *mlxsw_sp_port,
+				    const struct net_device *orig_dev,
 				    struct switchdev_brport_flags flags,
 				    struct netlink_ext_ack *extack)
 {
-	if (flags.mask & ~(BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD)) {
+	if (flags.mask & ~(BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD |
+			   BR_PORT_LOCKED)) {
 		NL_SET_ERR_MSG_MOD(extack, "Unsupported bridge port flag");
 		return -EINVAL;
 	}
 
+	if ((flags.mask & BR_PORT_LOCKED) && is_vlan_dev(orig_dev)) {
+		NL_SET_ERR_MSG_MOD(extack, "Locked flag cannot be set on a VLAN upper");
+		return -EINVAL;
+	}
+
+	if ((flags.mask & BR_PORT_LOCKED) && vlan_uses_dev(orig_dev)) {
+		NL_SET_ERR_MSG_MOD(extack, "Locked flag cannot be set on a bridge port that has VLAN uppers");
+		return -EINVAL;
+	}
+
 	return 0;
 }
 
@@ -822,6 +834,13 @@ static int mlxsw_sp_port_attr_br_flags_set(struct mlxsw_sp_port *mlxsw_sp_port,
 			return err;
 	}
 
+	if (flags.mask & BR_PORT_LOCKED) {
+		err = mlxsw_sp_port_security_set(mlxsw_sp_port,
+						 flags.val & BR_PORT_LOCKED);
+		if (err)
+			return err;
+	}
+
 	if (bridge_port->bridge_device->multicast_enabled)
 		goto out;
 
@@ -1189,6 +1208,7 @@ static int mlxsw_sp_port_attr_set(struct net_device *dev, const void *ctx,
 		break;
 	case SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS:
 		err = mlxsw_sp_port_attr_br_pre_flags_set(mlxsw_sp_port,
+							  attr->orig_dev,
 							  attr->u.brport_flags,
 							  extack);
 		break;
@@ -2787,6 +2807,7 @@ void mlxsw_sp_port_bridge_leave(struct mlxsw_sp_port *mlxsw_sp_port,
 
 	bridge_device->ops->port_leave(bridge_device, bridge_port,
 				       mlxsw_sp_port);
+	mlxsw_sp_port_security_set(mlxsw_sp_port, false);
 	mlxsw_sp_bridge_port_put(mlxsw_sp->bridge, bridge_port);
 }
 
-- 
2.37.3

[RFC PATCH net-next 13/16] selftests: devlink_lib: Split out helper

[Ido Schimmel]

Merely checking whether a trap counter incremented or not without
logging a test result is useful on its own. Split this functionality to
a helper which will be used by subsequent patches.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 .../selftests/net/forwarding/devlink_lib.sh   | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/net/forwarding/devlink_lib.sh b/tools/testing/selftests/net/forwarding/devlink_lib.sh
index 601990c6881b..f1de525cfa55 100644
--- a/tools/testing/selftests/net/forwarding/devlink_lib.sh
+++ b/tools/testing/selftests/net/forwarding/devlink_lib.sh
@@ -503,25 +503,30 @@ devlink_trap_drop_cleanup()
 	tc filter del dev $dev egress protocol $proto pref $pref handle $handle flower
 }
 
-devlink_trap_stats_test()
+devlink_trap_stats_check()
 {
-	local test_name=$1; shift
 	local trap_name=$1; shift
 	local send_one="$@"
 	local t0_packets
 	local t1_packets
 
-	RET=0
-
 	t0_packets=$(devlink_trap_rx_packets_get $trap_name)
 
 	$send_one && sleep 1
 
 	t1_packets=$(devlink_trap_rx_packets_get $trap_name)
 
-	if [[ $t1_packets -eq $t0_packets ]]; then
-		check_err 1 "Trap stats did not increase"
-	fi
+	[[ $t1_packets -ne $t0_packets ]]
+}
+
+devlink_trap_stats_test()
+{
+	local test_name=$1; shift
+
+	RET=0
+
+	devlink_trap_stats_check "$@"
+	check_err $? "Trap stats did not increase"
 
 	log_test "$test_name"
 }
-- 
2.37.3

[RFC PATCH net-next 14/16] selftests: mlxsw: Add a test for EAPOL trap

[Ido Schimmel]

Test that packets with a destination MAC of 01:80:C2:00:00:03 trigger
the "eapol" packet trap.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 .../drivers/net/mlxsw/devlink_trap_control.sh | 22 +++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/tools/testing/selftests/drivers/net/mlxsw/devlink_trap_control.sh b/tools/testing/selftests/drivers/net/mlxsw/devlink_trap_control.sh
index d3a891d421ab..64153bbf95df 100755
--- a/tools/testing/selftests/drivers/net/mlxsw/devlink_trap_control.sh
+++ b/tools/testing/selftests/drivers/net/mlxsw/devlink_trap_control.sh
@@ -83,6 +83,7 @@ ALL_TESTS="
 	ptp_general_test
 	flow_action_sample_test
 	flow_action_trap_test
+	eapol_test
 "
 NUM_NETIFS=4
 source $lib_dir/lib.sh
@@ -677,6 +678,27 @@ flow_action_trap_test()
 	tc qdisc del dev $rp1 clsact
 }
 
+eapol_payload_get()
+{
+	local source_mac=$1; shift
+	local p
+
+	p=$(:
+		)"01:80:C2:00:00:03:"$(       : ETH daddr
+		)"$source_mac:"$(             : ETH saddr
+		)"88:8E:"$(                   : ETH type
+		)
+	echo $p
+}
+
+eapol_test()
+{
+	local h1mac=$(mac_get $h1)
+
+	devlink_trap_stats_test "EAPOL" "eapol" $MZ $h1 -c 1 \
+		$(eapol_payload_get $h1mac) -p 100 -q
+}
+
 trap cleanup EXIT
 
 setup_prepare
-- 
2.37.3

[RFC PATCH net-next 15/16] selftests: mlxsw: Add a test for locked port trap

[Ido Schimmel]

Test that packets received via a locked bridge port whose {SMAC, VID}
does not appear in the bridge's FDB or appears with a different port,
trigger the "locked_port" packet trap.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 .../net/mlxsw/devlink_trap_l2_drops.sh        | 105 ++++++++++++++++++
 1 file changed, 105 insertions(+)

diff --git a/tools/testing/selftests/drivers/net/mlxsw/devlink_trap_l2_drops.sh b/tools/testing/selftests/drivers/net/mlxsw/devlink_trap_l2_drops.sh
index a4c2812e9807..8d4b2c6265b3 100755
--- a/tools/testing/selftests/drivers/net/mlxsw/devlink_trap_l2_drops.sh
+++ b/tools/testing/selftests/drivers/net/mlxsw/devlink_trap_l2_drops.sh
@@ -14,6 +14,7 @@ ALL_TESTS="
 	ingress_stp_filter_test
 	port_list_is_empty_test
 	port_loopback_filter_test
+	locked_port_test
 "
 NUM_NETIFS=4
 source $lib_dir/tc_common.sh
@@ -420,6 +421,110 @@ port_loopback_filter_test()
 	port_loopback_filter_uc_test
 }
 
+locked_port_miss_test()
+{
+	local trap_name="locked_port"
+	local smac=00:11:22:33:44:55
+
+	bridge link set dev $swp1 learning off
+	bridge link set dev $swp1 locked on
+
+	RET=0
+
+	devlink_trap_stats_check $trap_name $MZ $h1 -c 1 \
+		-a $smac -b $(mac_get $h2) -A 192.0.2.1 -B 192.0.2.2 -p 100 -q
+	check_fail $? "Trap stats increased before setting action to \"trap\""
+
+	devlink_trap_action_set $trap_name "trap"
+
+	devlink_trap_stats_check $trap_name $MZ $h1 -c 1 \
+		-a $smac -b $(mac_get $h2) -A 192.0.2.1 -B 192.0.2.2 -p 100 -q
+	check_err $? "Trap stats did not increase when should"
+
+	devlink_trap_action_set $trap_name "drop"
+
+	devlink_trap_stats_check $trap_name $MZ $h1 -c 1 \
+		-a $smac -b $(mac_get $h2) -A 192.0.2.1 -B 192.0.2.2 -p 100 -q
+	check_fail $? "Trap stats increased after setting action to \"drop\""
+
+	devlink_trap_action_set $trap_name "trap"
+
+	bridge fdb replace $smac dev $swp1 master static vlan 1
+
+	devlink_trap_stats_check $trap_name $MZ $h1 -c 1 \
+		-a $smac -b $(mac_get $h2) -A 192.0.2.1 -B 192.0.2.2 -p 100 -q
+	check_fail $? "Trap stats increased after adding an FDB entry"
+
+	bridge fdb del $smac dev $swp1 master static vlan 1
+	bridge link set dev $swp1 locked off
+
+	devlink_trap_stats_check $trap_name $MZ $h1 -c 1 \
+		-a $smac -b $(mac_get $h2) -A 192.0.2.1 -B 192.0.2.2 -p 100 -q
+	check_fail $? "Trap stats increased after unlocking port"
+
+	log_test "Locked port - FDB miss"
+
+	devlink_trap_action_set $trap_name "drop"
+	bridge link set dev $swp1 learning on
+}
+
+locked_port_mismatch_test()
+{
+	local trap_name="locked_port"
+	local smac=00:11:22:33:44:55
+
+	bridge link set dev $swp1 learning off
+	bridge link set dev $swp1 locked on
+
+	RET=0
+
+	bridge fdb replace $smac dev $swp2 master static vlan 1
+
+	devlink_trap_stats_check $trap_name $MZ $h1 -c 1 \
+		-a $smac -b $(mac_get $h2) -A 192.0.2.1 -B 192.0.2.2 -p 100 -q
+	check_fail $? "Trap stats increased before setting action to \"trap\""
+
+	devlink_trap_action_set $trap_name "trap"
+
+	devlink_trap_stats_check $trap_name $MZ $h1 -c 1 \
+		-a $smac -b $(mac_get $h2) -A 192.0.2.1 -B 192.0.2.2 -p 100 -q
+	check_err $? "Trap stats did not increase when should"
+
+	devlink_trap_action_set $trap_name "drop"
+
+	devlink_trap_stats_check $trap_name $MZ $h1 -c 1 \
+		-a $smac -b $(mac_get $h2) -A 192.0.2.1 -B 192.0.2.2 -p 100 -q
+	check_fail $? "Trap stats increased after setting action to \"drop\""
+
+	devlink_trap_action_set $trap_name "trap"
+	bridge link set dev $swp1 locked off
+
+	devlink_trap_stats_check $trap_name $MZ $h1 -c 1 \
+		-a $smac -b $(mac_get $h2) -A 192.0.2.1 -B 192.0.2.2 -p 100 -q
+	check_fail $? "Trap stats increased after unlocking port"
+
+	bridge link set dev $swp1 locked on
+	bridge fdb replace $smac dev $swp1 master static vlan 1
+
+	devlink_trap_stats_check $trap_name $MZ $h1 -c 1 \
+		-a $smac -b $(mac_get $h2) -A 192.0.2.1 -B 192.0.2.2 -p 100 -q
+	check_fail $? "Trap stats increased after replacing an FDB entry"
+
+	bridge fdb del $smac dev $swp1 master static vlan 1
+	devlink_trap_action_set $trap_name "drop"
+
+	log_test "Locked port - FDB mismatch"
+
+	bridge link set dev $swp1 locked off
+	bridge link set dev $swp1 learning on
+}
+
+locked_port_test()
+{
+	locked_port_miss_test
+	locked_port_mismatch_test
+}
+
 trap cleanup EXIT
 
 setup_prepare
-- 
2.37.3

[RFC PATCH net-next 16/16] selftests: mlxsw: Add a test for invalid locked bridge port configurations

[Ido Schimmel]

Test that locked bridge port configurations that are not supported by
mlxsw are rejected.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 .../selftests/drivers/net/mlxsw/rtnetlink.sh  | 31 +++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/tools/testing/selftests/drivers/net/mlxsw/rtnetlink.sh b/tools/testing/selftests/drivers/net/mlxsw/rtnetlink.sh
index 04f03ae9d8fb..5e89657857c7 100755
--- a/tools/testing/selftests/drivers/net/mlxsw/rtnetlink.sh
+++ b/tools/testing/selftests/drivers/net/mlxsw/rtnetlink.sh
@@ -34,6 +34,7 @@ ALL_TESTS="
 	nexthop_obj_bucket_offload_test
 	nexthop_obj_blackhole_offload_test
 	nexthop_obj_route_offload_test
+	bridge_locked_port_test
 	devlink_reload_test
 "
 NUM_NETIFS=2
@@ -917,6 +918,36 @@ nexthop_obj_route_offload_test()
 	simple_if_fini $swp1 192.0.2.1/24 2001:db8:1::1/64
 }
 
+bridge_locked_port_test()
+{
+	RET=0
+
+	ip link add name br1 up type bridge vlan_filtering 0
+
+	ip link add link $swp1 name $swp1.10 type vlan id 10
+	ip link set dev $swp1.10 master br1
+
+	bridge link set dev $swp1.10 locked on
+	check_fail $? "managed to set locked flag on a VLAN upper"
+
+	ip link set dev $swp1.10 nomaster
+	ip link set dev $swp1 master br1
+
+	bridge link set dev $swp1 locked on
+	check_fail $? "managed to set locked flag on a bridge port that has a VLAN upper"
+
+	ip link del dev $swp1.10
+	bridge link set dev $swp1 locked on
+
+	ip link add link $swp1 name $swp1.10 type vlan id 10
+	check_fail $? "managed to configure a VLAN upper on a locked port"
+
+	log_test "bridge locked port"
+
+	ip link del dev $swp1.10 &> /dev/null
+	ip link del dev br1
+}
+
 devlink_reload_test()
 {
 	# Test that after executing all the above configuration tests, a
-- 
2.37.3

Re: [RFC PATCH net-next 00/16] bridge: Add MAC Authentication Bypass (MAB) support with offload

[netdev]

On 2022-10-25 12:00, Ido Schimmel wrote:
> 
> Future work
> ===========
> 
> The hostapd fork by Westermo is using dynamic FDB entries to authorize
> hosts [3]. Changes are required in switchdev to allow such entries to 
> be
> offloaded. Hans already indicated he is working on that [4]. It should
> not necessitate any uAPI changes so I do not view it as a blocker 
> (Hans,
> please confirm).

The dynamic ATU patchset will do changes in the switchdev and DSA layers
and in the driver of course, so I suppose that confirms what you think
(e.g. no changes in include/uapi), but it requires the fdb_flags towards
drivers patches of course and MAB driver changes, that are part of my
v8 patchset.

Re: [RFC PATCH net-next 00/16] bridge: Add MAC Authentication Bypass (MAB) support with offload

[Ido Schimmel]

On Tue, Oct 25, 2022 at 04:09:07PM +0200, netdev@kapio-technology.com wrote:
> On 2022-10-25 12:00, Ido Schimmel wrote:
> > 
> > Future work
> > ===========
> > 
> > The hostapd fork by Westermo is using dynamic FDB entries to authorize
> > hosts [3]. Changes are required in switchdev to allow such entries to be
> > offloaded. Hans already indicated he is working on that [4]. It should
> > not necessitate any uAPI changes so I do not view it as a blocker (Hans,
> > please confirm).
> 
> The dynamic ATU patchset will do changes in the switchdev and DSA layers
> and in the driver of course, so I suppose that confirms what you think
> (e.g. no changes in include/uapi), but it requires the fdb_flags towards
> drivers patches of course and MAB driver changes, that are part of my
> v8 patchset.

Any comments on the merge plan and patches #1-#4?

Re: [RFC PATCH net-next 00/16] bridge: Add MAC Authentication Bypass (MAB) support with offload

[Vladimir Oltean]

On Tue, Oct 25, 2022 at 01:00:08PM +0300, Ido Schimmel wrote:
> Merge plan
> ==========
> 
> We need to agree on a merge plan that allows us to start submitting
> patches for inclusion and finally conclude this work. In my experience,
> it is best to work in small batches. I therefore propose the following
> plan:
> 
> * Add MAB support in the bridge driver. This corresponds to patches
>   #1-#2.

Yes, exactly, let's keep the software implementation its own patch set,
together with its selftest.

> * Switchdev extensions for MAB offload together with mlxsw
>   support. This corresponds to patches #3-#16. I can reduce the number
>   of patches by splitting out the selftests to a separate submission.

I don't think that further splitting of mlxsw will be necessary, the
threshold is 15 patches, and the current 16-2 = 14.

IMO patches 3-4 should go with mlxsw being the first user.

> * mv88e6xxx support. I believe the blackhole stuff is an optimization,
>   so I suggest getting initial MAB offload support without that. Support
>   for blackhole entries together with offload can be added in a separate
>   submission.
> 
> * Switchdev extensions for dynamic FDB entries together with mv88e6xxx
>   support. I can follow up with mlxsw support afterwards.

The last 2 can also happen in parallel, if somebody else comes to it.

Looks great, thanks for doing this!

Re: [RFC PATCH net-next 00/16] bridge: Add MAC Authentication Bypass (MAB) support with offload

[netdev]

On 2022-10-25 12:00, Ido Schimmel wrote:
> Merge plan
> ==========
> 
> We need to agree on a merge plan that allows us to start submitting
> patches for inclusion and finally conclude this work. In my experience,
> it is best to work in small batches. I therefore propose the following
> plan:
> 
> * Add MAB support in the bridge driver. This corresponds to patches
>   #1-#2.
> 
> * Switchdev extensions for MAB offload together with mlxsw
>   support. This corresponds to patches #3-#16. I can reduce the number
>   of patches by splitting out the selftests to a separate submission.
> 
> * mv88e6xxx support. I believe the blackhole stuff is an optimization,
>   so I suggest getting initial MAB offload support without that. 
> Support
>   for blackhole entries together with offload can be added in a 
> separate
>   submission.

As I understand for the mv88e6xxx support, we will be sending 
SWITCHDEV_FDB_ADD_TO_BRIDGE
events from the driver to the bridge without installing entries in the 
driver.
Just to note, that will of course imply that the bridge FDB will be out 
of sync with the
FDB in the driver (ATU).

> 
> * Switchdev extensions for dynamic FDB entries together with mv88e6xxx
>   support. I can follow up with mlxsw support afterwards.
> 
> [1] 
> https://lore.kernel.org/netdev/20221018165619.134535-1-netdev@kapio-technology.com/
> [2] 
> https://lore.kernel.org/netdev/20221004152036.7848-1-netdev@kapio-technology.com/
> [3] 
> https://github.com/westermo/hostapd/blob/bridge_driver/hostapd/hostapd_auth_deauth.sh#L11
> [4] 
> https://lore.kernel.org/netdev/a11af0d07a79adbd2ac3d242b36dec7e@kapio-technology.com/

Re: [RFC PATCH net-next 00/16] bridge: Add MAC Authentication Bypass (MAB) support with offload

[Ido Schimmel]

On Sun, Nov 06, 2022 at 01:04:36PM +0100, netdev@kapio-technology.com wrote:
> On 2022-10-25 12:00, Ido Schimmel wrote:
> > Merge plan
> > ==========
> > 
> > We need to agree on a merge plan that allows us to start submitting
> > patches for inclusion and finally conclude this work. In my experience,
> > it is best to work in small batches. I therefore propose the following
> > plan:
> > 
> > * Add MAB support in the bridge driver. This corresponds to patches
> >   #1-#2.
> > 
> > * Switchdev extensions for MAB offload together with mlxsw
> >   support. This corresponds to patches #3-#16. I can reduce the number
> >   of patches by splitting out the selftests to a separate submission.
> > 
> > * mv88e6xxx support. I believe the blackhole stuff is an optimization,
> >   so I suggest getting initial MAB offload support without that. Support
> >   for blackhole entries together with offload can be added in a separate
> >   submission.
> 
> As I understand for the mv88e6xxx support, we will be sending
> SWITCHDEV_FDB_ADD_TO_BRIDGE
> events from the driver to the bridge without installing entries in the
> driver.
> Just to note, that will of course imply that the bridge FDB will be out of
> sync with the
> FDB in the driver (ATU).

Stated explicitly here:
https://lore.kernel.org/netdev/20221025100024.1287157-4-idosch@nvidia.com/

Don't see a way around it and it's not critical IMO. The entries will
not appear with the "offload" flag so user space knows they are not in
hardware. Once the "blackhole" flag is supported, user space can replace
such entries and set the "blackhole" flag, which will result in the
entries being programmed to hardware (assuming hardware/driver support).

I plan to submit the offload patches later this week.