[PATCH v4 net-next 0/6] Extend locked port feature with FDB locked flag (MAC-Auth/MAB)

[PATCH v4 net-next 0/6] Extend locked port feature with FDB locked flag (MAC-Auth/MAB)

[Hans Schultz]

This patch set extends the locked port feature for devices
that are behind a locked port, but do not have the ability to
authorize themselves as a supplicant using IEEE 802.1X.
Such devices can be printers, meters or anything related to
fixed installations. Instead of 802.1X authorization, devices
can get access based on their MAC addresses being whitelisted.

For an authorization daemon to detect that a device is trying
to get access through a locked port, the bridge will add the
MAC address of the device to the FDB with a locked flag to it.
Thus the authorization daemon can catch the FDB add event and
check if the MAC address is in the whitelist and if so replace
the FDB entry without the locked flag enabled, and thus open
the port for the device.

This feature is known as MAC-Auth or MAC Authentication Bypass
(MAB) in Cisco terminology, where the full MAB concept involves
additional Cisco infrastructure for authorization. There is no
real authentication process, as the MAC address of the device
is the only input the authorization daemon, in the general
case, has to base the decision if to unlock the port or not.

With this patch set, an implementation of the offloaded case is
supplied for the mv88e6xxx driver. When a packet ingresses on
a locked port, an ATU miss violation event will occur. When
handling such ATU miss violation interrupts, the MAC address of
the device is added to the FDB with a zero destination port
vector (DPV) and the MAC address is communicated through the
switchdev layer to the bridge, so that a FDB entry with the
locked flag enabled can be added.

Log:
	v3:	Added timers and lists in the driver (mv88e6xxx)
		to keep track of and remove locked entries.

	v4:	Leave out enforcing a limit to the number of
		locked entries in the bridge.
		Removed the timers in the driver and use the
		worker only. Add locked FDB flag to all drivers
		using port_fdb_add() from the dsa api and let
		all drivers ignore entries with this flag set.
		Change how to get the ageing timeout of locked
		entries. See global1_atu.c and switchdev.c.
		Use struct mv88e6xxx_port for locked entries
		variables instead of struct dsa_port.

Hans Schultz (6):
  net: bridge: add locked entry fdb flag to extend locked port feature
  net: switchdev: add support for offloading of fdb locked flag
  drivers: net: dsa: add locked fdb entry flag to drivers
  net: dsa: mv88e6xxx: allow reading FID when handling ATU violations
  net: dsa: mv88e6xxx: mac-auth/MAB implementation
  selftests: forwarding: add test of MAC-Auth Bypass to locked port
    tests

 drivers/net/dsa/b53/b53_common.c              |   5 +
 drivers/net/dsa/b53/b53_priv.h                |   1 +
 drivers/net/dsa/hirschmann/hellcreek.c        |   5 +
 drivers/net/dsa/lan9303-core.c                |   5 +
 drivers/net/dsa/lantiq_gswip.c                |   5 +
 drivers/net/dsa/microchip/ksz9477.c           |   5 +
 drivers/net/dsa/mt7530.c                      |   5 +
 drivers/net/dsa/mv88e6xxx/Makefile            |   1 +
 drivers/net/dsa/mv88e6xxx/chip.c              |  54 +++-
 drivers/net/dsa/mv88e6xxx/chip.h              |  15 +
 drivers/net/dsa/mv88e6xxx/global1.h           |   1 +
 drivers/net/dsa/mv88e6xxx/global1_atu.c       |  32 +-
 drivers/net/dsa/mv88e6xxx/port.c              |  30 +-
 drivers/net/dsa/mv88e6xxx/port.h              |   2 +
 drivers/net/dsa/mv88e6xxx/switchdev.c         | 280 ++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/switchdev.h         |  37 +++
 drivers/net/dsa/ocelot/felix.c                |   5 +
 drivers/net/dsa/qca8k.c                       |   5 +
 drivers/net/dsa/sja1105/sja1105_main.c        |   5 +
 include/net/dsa.h                             |   7 +
 include/net/switchdev.h                       |   1 +
 include/uapi/linux/neighbour.h                |   1 +
 net/bridge/br.c                               |   3 +-
 net/bridge/br_fdb.c                           |  19 +-
 net/bridge/br_input.c                         |  10 +-
 net/bridge/br_private.h                       |   5 +-
 net/bridge/br_switchdev.c                     |   1 +
 net/dsa/dsa_priv.h                            |   4 +-
 net/dsa/port.c                                |   7 +-
 net/dsa/slave.c                               |   4 +-
 net/dsa/switch.c                              |  10 +-
 .../net/forwarding/bridge_locked_port.sh      |  30 +-
 32 files changed, 566 insertions(+), 34 deletions(-)
 create mode 100644 drivers/net/dsa/mv88e6xxx/switchdev.c
 create mode 100644 drivers/net/dsa/mv88e6xxx/switchdev.h

-- 
2.30.2

[PATCH v4 net-next 1/6] net: bridge: add locked entry fdb flag to extend locked port feature

[Hans Schultz]

Add an intermediate state for clients behind a locked port to allow for
possible opening of the port for said clients. The clients mac address
will be added with the locked flag set, denying access through the port
for the mac address, but also creating a new FDB add event giving
userspace daemons the ability to unlock the mac address. This feature
corresponds to the Mac-Auth and MAC Authentication Bypass (MAB) named
features. The latter defined by Cisco.

Only the kernel can set this FDB entry flag, while userspace can read
the flag and remove it by replacing or deleting the FDB entry.

Signed-off-by: Hans Schultz <netdev@kapio-technology.com>
---
 include/uapi/linux/neighbour.h |  1 +
 net/bridge/br_fdb.c            | 12 ++++++++++++
 net/bridge/br_input.c          | 10 +++++++++-
 net/bridge/br_private.h        |  3 ++-
 4 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/neighbour.h b/include/uapi/linux/neighbour.h
index 39c565e460c7..76d65b481086 100644
--- a/include/uapi/linux/neighbour.h
+++ b/include/uapi/linux/neighbour.h
@@ -53,6 +53,7 @@ enum {
 #define NTF_ROUTER	(1 << 7)
 /* Extended flags under NDA_FLAGS_EXT: */
 #define NTF_EXT_MANAGED	(1 << 0)
+#define NTF_EXT_LOCKED	(1 << 1)
 
 /*
  *	Neighbor Cache Entry States.
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index e7f4fccb6adb..ee9064a536ae 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_ENTRY_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 */
@@ -1082,6 +1089,11 @@ static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source,
 		modified = true;
 	}
 
+	if (test_bit(BR_FDB_ENTRY_LOCKED, &fdb->flags)) {
+		clear_bit(BR_FDB_ENTRY_LOCKED, &fdb->flags);
+		modified = true;
+	}
+
 	if (fdb_handle_notify(fdb, notify))
 		modified = true;
 
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 68b3e850bcb9..3d15548cfda6 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -110,8 +110,16 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 			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))
+		    test_bit(BR_FDB_LOCAL, &fdb_src->flags) ||
+		    test_bit(BR_FDB_ENTRY_LOCKED, &fdb_src->flags)) {
+			if (!fdb_src) {
+				unsigned long flags = 0;
+
+				__set_bit(BR_FDB_ENTRY_LOCKED, &flags);
+				br_fdb_update(br, p, eth_hdr(skb)->h_source, vid, flags);
+			}
 			goto drop;
+		}
 	}
 
 	nbp_switchdev_frame_mark(p, skb);
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 06e5f6faa431..47a3598d25c8 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_ENTRY_LOCKED,
 };
 
 struct net_bridge_fdb_key {
-- 
2.30.2

[PATCH v4 net-next 2/6] net: switchdev: add support for offloading of fdb locked flag

[Hans Schultz]

Used for Mac-auth/MAB feature in the offloaded case.
Send flag through switchdev to driver.

Signed-off-by: Hans Schultz <netdev@kapio-technology.com>
---
 include/net/dsa.h         | 6 ++++++
 include/net/switchdev.h   | 1 +
 net/bridge/br.c           | 3 ++-
 net/bridge/br_fdb.c       | 7 +++++--
 net/bridge/br_private.h   | 2 +-
 net/bridge/br_switchdev.c | 1 +
 net/dsa/dsa_priv.h        | 4 +++-
 net/dsa/port.c            | 7 ++++++-
 net/dsa/slave.c           | 4 +++-
 net/dsa/switch.c          | 6 +++---
 10 files changed, 31 insertions(+), 10 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 14f07275852b..a5a843b2d67d 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -330,6 +330,12 @@ struct dsa_port {
 	/* List of VLANs that CPU and DSA ports are members of. */
 	struct mutex		vlans_lock;
 	struct list_head	vlans;
+
+	/* List and maintenance of locked ATU entries */
+	struct mutex		locked_entries_list_lock;
+	struct list_head	atu_locked_entries_list;
+	atomic_t		atu_locked_entry_cnt;
+	struct delayed_work	atu_work;
 };
 
 /* TODO: ideally DSA ports would have a single dp->link_dp member,
diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index aa0171d5786d..9f83c835ee62 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -245,6 +245,7 @@ struct switchdev_notifier_fdb_info {
 	u16 vid;
 	u8 added_by_user:1,
 	   is_local:1,
+	   is_locked:1,
 	   offloaded:1;
 };
 
diff --git a/net/bridge/br.c b/net/bridge/br.c
index 96e91d69a9a8..fe0a4741fcda 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, false,
+						fdb_info->is_locked);
 		if (err) {
 			err = notifier_from_errno(err);
 			break;
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index ee9064a536ae..32ebb18050b9 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -1136,7 +1136,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, true, false);
 	} else {
 		spin_lock_bh(&br->hash_lock);
 		err = fdb_add_entry(br, p, addr, ndm, nlh_flags, vid, nfea_tb);
@@ -1366,7 +1366,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,
-			      bool swdev_notify)
+			      bool swdev_notify, bool locked)
 {
 	struct net_bridge_fdb_entry *fdb;
 	bool modified = false;
@@ -1386,6 +1386,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_ENTRY_LOCKED);
+
 		fdb = fdb_create(br, p, addr, vid, flags);
 		if (!fdb) {
 			err = -ENOMEM;
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 47a3598d25c8..9082451b4d40 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 swdev_notify, bool locked);
 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..85e566b856e1 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->is_locked = test_bit(BR_FDB_ENTRY_LOCKED, &fdb->flags);
 	item->info.dev = (!p || item->is_local) ? br->dev : p->dev;
 	item->info.ctx = ctx;
 }
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index d9722e49864b..42f47a94b0f0 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -65,6 +65,7 @@ struct dsa_notifier_fdb_info {
 	const struct dsa_port *dp;
 	const unsigned char *addr;
 	u16 vid;
+	bool is_locked;
 	struct dsa_db db;
 };
 
@@ -131,6 +132,7 @@ struct dsa_switchdev_event_work {
 	unsigned char addr[ETH_ALEN];
 	u16 vid;
 	bool host_addr;
+	bool is_locked;
 };
 
 enum dsa_standalone_event {
@@ -232,7 +234,7 @@ int dsa_port_vlan_msti(struct dsa_port *dp,
 		       const struct switchdev_vlan_msti *msti);
 int dsa_port_mtu_change(struct dsa_port *dp, int new_mtu);
 int dsa_port_fdb_add(struct dsa_port *dp, const unsigned char *addr,
-		     u16 vid);
+		     u16 vid, bool is_locked);
 int dsa_port_fdb_del(struct dsa_port *dp, const unsigned char *addr,
 		     u16 vid);
 int dsa_port_standalone_host_fdb_add(struct dsa_port *dp,
diff --git a/net/dsa/port.c b/net/dsa/port.c
index 3738f2d40a0b..8bdac9aabe5d 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -35,6 +35,7 @@ static void dsa_port_notify_bridge_fdb_flush(const struct dsa_port *dp, u16 vid)
 	struct net_device *brport_dev = dsa_port_to_bridge_port(dp);
 	struct switchdev_notifier_fdb_info info = {
 		.vid = vid,
+		.is_locked = false,
 	};
 
 	/* When the port becomes standalone it has already left the bridge.
@@ -950,12 +951,13 @@ int dsa_port_mtu_change(struct dsa_port *dp, int new_mtu)
 }
 
 int dsa_port_fdb_add(struct dsa_port *dp, const unsigned char *addr,
-		     u16 vid)
+		     u16 vid, bool is_locked)
 {
 	struct dsa_notifier_fdb_info info = {
 		.dp = dp,
 		.addr = addr,
 		.vid = vid,
+		.is_locked = is_locked,
 		.db = {
 			.type = DSA_DB_BRIDGE,
 			.bridge = *dp->bridge,
@@ -979,6 +981,7 @@ int dsa_port_fdb_del(struct dsa_port *dp, const unsigned char *addr,
 		.dp = dp,
 		.addr = addr,
 		.vid = vid,
+		.is_locked = false,
 		.db = {
 			.type = DSA_DB_BRIDGE,
 			.bridge = *dp->bridge,
@@ -999,6 +1002,7 @@ static int dsa_port_host_fdb_add(struct dsa_port *dp,
 		.dp = dp,
 		.addr = addr,
 		.vid = vid,
+		.is_locked = false,
 		.db = db,
 	};
 
@@ -1050,6 +1054,7 @@ static int dsa_port_host_fdb_del(struct dsa_port *dp,
 		.dp = dp,
 		.addr = addr,
 		.vid = vid,
+		.is_locked = false,
 		.db = db,
 	};
 
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 801a5d445833..905b15e4eab9 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -2784,6 +2784,7 @@ static void dsa_slave_switchdev_event_work(struct work_struct *work)
 		container_of(work, struct dsa_switchdev_event_work, work);
 	const unsigned char *addr = switchdev_work->addr;
 	struct net_device *dev = switchdev_work->dev;
+	bool is_locked = switchdev_work->is_locked;
 	u16 vid = switchdev_work->vid;
 	struct dsa_switch *ds;
 	struct dsa_port *dp;
@@ -2799,7 +2800,7 @@ static void dsa_slave_switchdev_event_work(struct work_struct *work)
 		else if (dp->lag)
 			err = dsa_port_lag_fdb_add(dp, addr, vid);
 		else
-			err = dsa_port_fdb_add(dp, addr, vid);
+			err = dsa_port_fdb_add(dp, addr, vid, is_locked);
 		if (err) {
 			dev_err(ds->dev,
 				"port %d failed to add %pM vid %d to fdb: %d\n",
@@ -2907,6 +2908,7 @@ static int dsa_slave_fdb_event(struct net_device *dev,
 	ether_addr_copy(switchdev_work->addr, fdb_info->addr);
 	switchdev_work->vid = fdb_info->vid;
 	switchdev_work->host_addr = host_addr;
+	switchdev_work->is_locked = fdb_info->is_locked;
 
 	dsa_schedule_work(&switchdev_work->work);
 
diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index 2b56218fc57c..32b1e7ac6373 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -234,7 +234,7 @@ static int dsa_port_do_mdb_del(struct dsa_port *dp,
 }
 
 static int dsa_port_do_fdb_add(struct dsa_port *dp, const unsigned char *addr,
-			       u16 vid, struct dsa_db db)
+			       u16 vid, bool is_locked, struct dsa_db db)
 {
 	struct dsa_switch *ds = dp->ds;
 	struct dsa_mac_addr *a;
@@ -398,7 +398,7 @@ static int dsa_switch_host_fdb_add(struct dsa_switch *ds,
 	dsa_switch_for_each_port(dp, ds) {
 		if (dsa_port_host_address_match(dp, info->dp)) {
 			err = dsa_port_do_fdb_add(dp, info->addr, info->vid,
-						  info->db);
+						  false, info->db);
 			if (err)
 				break;
 		}
@@ -437,7 +437,7 @@ static int dsa_switch_fdb_add(struct dsa_switch *ds,
 	if (!ds->ops->port_fdb_add)
 		return -EOPNOTSUPP;
 
-	return dsa_port_do_fdb_add(dp, info->addr, info->vid, info->db);
+	return dsa_port_do_fdb_add(dp, info->addr, info->vid, info->is_locked, info->db);
 }
 
 static int dsa_switch_fdb_del(struct dsa_switch *ds,
-- 
2.30.2

[PATCH v4 net-next 3/6] drivers: net: dsa: add locked fdb entry flag to drivers

[Hans Schultz]

Ignore locked fdb entries coming in on all drivers.

Signed-off-by: Hans Schultz <netdev@kapio-technology.com>
---
 drivers/net/dsa/b53/b53_common.c       | 5 +++++
 drivers/net/dsa/b53/b53_priv.h         | 1 +
 drivers/net/dsa/hirschmann/hellcreek.c | 5 +++++
 drivers/net/dsa/lan9303-core.c         | 5 +++++
 drivers/net/dsa/lantiq_gswip.c         | 5 +++++
 drivers/net/dsa/microchip/ksz9477.c    | 5 +++++
 drivers/net/dsa/mt7530.c               | 5 +++++
 drivers/net/dsa/mv88e6xxx/chip.c       | 5 +++++
 drivers/net/dsa/ocelot/felix.c         | 5 +++++
 drivers/net/dsa/qca8k.c                | 5 +++++
 drivers/net/dsa/sja1105/sja1105_main.c | 5 +++++
 include/net/dsa.h                      | 1 +
 net/dsa/switch.c                       | 4 ++--
 13 files changed, 54 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index fbb32aa49b24..3567d4fcbaa7 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1688,11 +1688,16 @@ static int b53_arl_op(struct b53_device *dev, int op, int port,
 
 int b53_fdb_add(struct dsa_switch *ds, int port,
 		const unsigned char *addr, u16 vid,
+		bool is_locked,
 		struct dsa_db db)
 {
 	struct b53_device *priv = ds->priv;
 	int ret;
 
+	/* Ignore locked entries */
+	if (is_locked)
+		return 0;
+
 	/* 5325 and 5365 require some more massaging, but could
 	 * be supported eventually
 	 */
diff --git a/drivers/net/dsa/b53/b53_priv.h b/drivers/net/dsa/b53/b53_priv.h
index 795cbffd5c2b..19501b76b9df 100644
--- a/drivers/net/dsa/b53/b53_priv.h
+++ b/drivers/net/dsa/b53/b53_priv.h
@@ -362,6 +362,7 @@ int b53_vlan_del(struct dsa_switch *ds, int port,
 		 const struct switchdev_obj_port_vlan *vlan);
 int b53_fdb_add(struct dsa_switch *ds, int port,
 		const unsigned char *addr, u16 vid,
+		bool is_locked,
 		struct dsa_db db);
 int b53_fdb_del(struct dsa_switch *ds, int port,
 		const unsigned char *addr, u16 vid,
diff --git a/drivers/net/dsa/hirschmann/hellcreek.c b/drivers/net/dsa/hirschmann/hellcreek.c
index ac1f3b3a7040..0125d901c988 100644
--- a/drivers/net/dsa/hirschmann/hellcreek.c
+++ b/drivers/net/dsa/hirschmann/hellcreek.c
@@ -829,12 +829,17 @@ static int hellcreek_fdb_get(struct hellcreek *hellcreek,
 
 static int hellcreek_fdb_add(struct dsa_switch *ds, int port,
 			     const unsigned char *addr, u16 vid,
+			     bool is_locked,
 			     struct dsa_db db)
 {
 	struct hellcreek_fdb_entry entry = { 0 };
 	struct hellcreek *hellcreek = ds->priv;
 	int ret;
 
+	/* Ignore locked entries */
+	if (is_locked)
+		return 0;
+
 	dev_dbg(hellcreek->dev, "Add FDB entry for MAC=%pM\n", addr);
 
 	mutex_lock(&hellcreek->reg_lock);
diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index e03ff1f267bb..426b9ea668da 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -1190,10 +1190,15 @@ static void lan9303_port_fast_age(struct dsa_switch *ds, int port)
 
 static int lan9303_port_fdb_add(struct dsa_switch *ds, int port,
 				const unsigned char *addr, u16 vid,
+				bool is_locked,
 				struct dsa_db db)
 {
 	struct lan9303 *chip = ds->priv;
 
+	/* Ignore locked entries */
+	if (is_locked)
+		return 0;
+
 	dev_dbg(chip->dev, "%s(%d, %pM, %d)\n", __func__, port, addr, vid);
 	if (vid)
 		return -EOPNOTSUPP;
diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
index 8af4def38a98..4da7186935f4 100644
--- a/drivers/net/dsa/lantiq_gswip.c
+++ b/drivers/net/dsa/lantiq_gswip.c
@@ -1399,8 +1399,13 @@ static int gswip_port_fdb(struct dsa_switch *ds, int port,
 
 static int gswip_port_fdb_add(struct dsa_switch *ds, int port,
 			      const unsigned char *addr, u16 vid,
+			      bool is_locked,
 			      struct dsa_db db)
 {
+	/* Ignore locked entries */
+	if (is_locked)
+		return 0;
+
 	return gswip_port_fdb(ds, port, addr, vid, true);
 }
 
diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index ab40b700cf1a..c177eb97a072 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -481,6 +481,7 @@ static int ksz9477_port_vlan_del(struct dsa_switch *ds, int port,
 
 static int ksz9477_port_fdb_add(struct dsa_switch *ds, int port,
 				const unsigned char *addr, u16 vid,
+				bool is_locked,
 				struct dsa_db db)
 {
 	struct ksz_device *dev = ds->priv;
@@ -488,6 +489,10 @@ static int ksz9477_port_fdb_add(struct dsa_switch *ds, int port,
 	u32 data;
 	int ret = 0;
 
+	/* Ignore locked entries */
+	if (is_locked)
+		return ret;
+
 	mutex_lock(&dev->alu_mutex);
 
 	/* find any entry with mac & vid */
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 2b02d823d497..cbf922cf425e 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -1356,12 +1356,17 @@ mt7530_port_bridge_leave(struct dsa_switch *ds, int port,
 static int
 mt7530_port_fdb_add(struct dsa_switch *ds, int port,
 		    const unsigned char *addr, u16 vid,
+		    bool is_locked,
 		    struct dsa_db db)
 {
 	struct mt7530_priv *priv = ds->priv;
 	int ret;
 	u8 port_mask = BIT(port);
 
+	/* Ignore locked entries */
+	if (is_locked)
+		return 0;
+
 	mutex_lock(&priv->reg_mutex);
 	mt7530_fdb_write(priv, vid, port_mask, addr, -1, STATIC_ENT);
 	ret = mt7530_fdb_cmd(priv, MT7530_FDB_WRITE, NULL);
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 5d2c57a7c708..d134153ef023 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2717,11 +2717,16 @@ static int mv88e6xxx_vlan_msti_set(struct dsa_switch *ds,
 
 static int mv88e6xxx_port_fdb_add(struct dsa_switch *ds, int port,
 				  const unsigned char *addr, u16 vid,
+				  bool is_locked,
 				  struct dsa_db db)
 {
 	struct mv88e6xxx_chip *chip = ds->priv;
 	int err;
 
+	/* Ignore locked entries */
+	if (is_locked)
+		return 0;
+
 	mv88e6xxx_reg_lock(chip);
 	err = mv88e6xxx_port_db_load_purge(chip, port, addr, vid,
 					   MV88E6XXX_G1_ATU_DATA_STATE_UC_STATIC);
diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index 3e07dc39007a..00ba495a7ab5 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -695,12 +695,17 @@ static int felix_fdb_dump(struct dsa_switch *ds, int port,
 
 static int felix_fdb_add(struct dsa_switch *ds, int port,
 			 const unsigned char *addr, u16 vid,
+			 bool is_locked,
 			 struct dsa_db db)
 {
 	struct net_device *bridge_dev = felix_classify_db(db);
 	struct dsa_port *dp = dsa_to_port(ds, port);
 	struct ocelot *ocelot = ds->priv;
 
+	/* Ignore locked entries */
+	if (is_locked)
+		return 0;
+
 	if (IS_ERR(bridge_dev))
 		return PTR_ERR(bridge_dev);
 
diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 2727d3169c25..6267e94af485 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -2369,11 +2369,16 @@ qca8k_port_fdb_insert(struct qca8k_priv *priv, const u8 *addr,
 static int
 qca8k_port_fdb_add(struct dsa_switch *ds, int port,
 		   const unsigned char *addr, u16 vid,
+		   bool is_locked,
 		   struct dsa_db db)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
 	u16 port_mask = BIT(port);
 
+	/* Ignore locked entries */
+	if (is_locked)
+		return 0;
+
 	return qca8k_port_fdb_insert(priv, addr, port_mask, vid);
 }
 
diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index 72b6fc1932b5..9b5c2b56d06d 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -1802,10 +1802,15 @@ int sja1105pqrs_fdb_del(struct dsa_switch *ds, int port,
 
 static int sja1105_fdb_add(struct dsa_switch *ds, int port,
 			   const unsigned char *addr, u16 vid,
+			   bool is_locked,
 			   struct dsa_db db)
 {
 	struct sja1105_private *priv = ds->priv;
 
+	/* Ignore locked entries */
+	if (is_locked)
+		return 0;
+
 	if (!vid) {
 		switch (db.type) {
 		case DSA_DB_PORT:
diff --git a/include/net/dsa.h b/include/net/dsa.h
index a5a843b2d67d..ebae403ce734 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -1010,6 +1010,7 @@ struct dsa_switch_ops {
 	 */
 	int	(*port_fdb_add)(struct dsa_switch *ds, int port,
 				const unsigned char *addr, u16 vid,
+				bool is_locked,
 				struct dsa_db db);
 	int	(*port_fdb_del)(struct dsa_switch *ds, int port,
 				const unsigned char *addr, u16 vid,
diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index 32b1e7ac6373..ea3b9c1acb8c 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -243,7 +243,7 @@ static int dsa_port_do_fdb_add(struct dsa_port *dp, const unsigned char *addr,
 
 	/* No need to bother with refcounting for user ports */
 	if (!(dsa_port_is_cpu(dp) || dsa_port_is_dsa(dp)))
-		return ds->ops->port_fdb_add(ds, port, addr, vid, db);
+		return ds->ops->port_fdb_add(ds, port, addr, vid, is_locked, db);
 
 	mutex_lock(&dp->addr_lists_lock);
 
@@ -259,7 +259,7 @@ static int dsa_port_do_fdb_add(struct dsa_port *dp, const unsigned char *addr,
 		goto out;
 	}
 
-	err = ds->ops->port_fdb_add(ds, port, addr, vid, db);
+	err = ds->ops->port_fdb_add(ds, port, addr, vid, is_locked, db);
 	if (err) {
 		kfree(a);
 		goto out;
-- 
2.30.2

[PATCH v4 net-next 4/6] net: dsa: mv88e6xxx: allow reading FID when handling ATU violations

[Hans Schultz]

For convenience the function mv88e6xxx_g1_atu_op() has been used to read
ATU violations, but the function has other purposes and does not enable
the possibility to read the FID when reading ATU violations.

The FID is needed to get hold of which VID was involved in the violation,
thus the need to be able to read the FID.

Signed-off-by: Hans Schultz <netdev@kapio-technology.com>
---
 drivers/net/dsa/mv88e6xxx/global1_atu.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/global1_atu.c b/drivers/net/dsa/mv88e6xxx/global1_atu.c
index 40bd67a5c8e9..5d120d53823c 100644
--- a/drivers/net/dsa/mv88e6xxx/global1_atu.c
+++ b/drivers/net/dsa/mv88e6xxx/global1_atu.c
@@ -114,6 +114,19 @@ static int mv88e6xxx_g1_atu_op_wait(struct mv88e6xxx_chip *chip)
 	return mv88e6xxx_g1_wait_bit(chip, MV88E6XXX_G1_ATU_OP, bit, 0);
 }
 
+static int mv88e6xxx_g1_read_atu_violation(struct mv88e6xxx_chip *chip)
+{
+	int err;
+
+	err = mv88e6xxx_g1_write(chip, MV88E6XXX_G1_ATU_OP,
+				 MV88E6XXX_G1_ATU_OP_BUSY |
+				 MV88E6XXX_G1_ATU_OP_GET_CLR_VIOLATION);
+	if (err)
+		return err;
+
+	return mv88e6xxx_g1_atu_op_wait(chip);
+}
+
 static int mv88e6xxx_g1_atu_op(struct mv88e6xxx_chip *chip, u16 fid, u16 op)
 {
 	u16 val;
@@ -359,8 +372,7 @@ static irqreturn_t mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
 
 	mv88e6xxx_reg_lock(chip);
 
-	err = mv88e6xxx_g1_atu_op(chip, 0,
-				  MV88E6XXX_G1_ATU_OP_GET_CLR_VIOLATION);
+	err = mv88e6xxx_g1_read_atu_violation(chip);
 	if (err)
 		goto out;
 
-- 
2.30.2

[PATCH v4 net-next 5/6] net: dsa: mv88e6xxx: mac-auth/MAB implementation

[Hans Schultz]

This implementation for the Marvell mv88e6xxx chip series,
is based on handling ATU miss violations occurring when packets
ingress on a port that is locked. The mac address triggering
the ATU miss violation will be added to the ATU with a zero-DPV,
and is then communicated through switchdev to the bridge module,
which adds a fdb entry with the fdb locked flag set. The entry
is kept according to the bridges ageing time, thus simulating a
dynamic entry.

As this is essentially a form of CPU based learning, the amount
of locked entries will be limited by a hardcoded value for now,
so as to prevent DOS attacks.

Signed-off-by: Hans Schultz <netdev@kapio-technology.com>
---
 drivers/net/dsa/mv88e6xxx/Makefile      |   1 +
 drivers/net/dsa/mv88e6xxx/chip.c        |  49 ++++-
 drivers/net/dsa/mv88e6xxx/chip.h        |  15 ++
 drivers/net/dsa/mv88e6xxx/global1.h     |   1 +
 drivers/net/dsa/mv88e6xxx/global1_atu.c |  16 +-
 drivers/net/dsa/mv88e6xxx/port.c        |  30 ++-
 drivers/net/dsa/mv88e6xxx/port.h        |   2 +
 drivers/net/dsa/mv88e6xxx/switchdev.c   | 280 ++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/switchdev.h   |  37 ++++
 9 files changed, 414 insertions(+), 17 deletions(-)
 create mode 100644 drivers/net/dsa/mv88e6xxx/switchdev.c
 create mode 100644 drivers/net/dsa/mv88e6xxx/switchdev.h

diff --git a/drivers/net/dsa/mv88e6xxx/Makefile b/drivers/net/dsa/mv88e6xxx/Makefile
index c8eca2b6f959..be903a983780 100644
--- a/drivers/net/dsa/mv88e6xxx/Makefile
+++ b/drivers/net/dsa/mv88e6xxx/Makefile
@@ -15,3 +15,4 @@ mv88e6xxx-objs += port_hidden.o
 mv88e6xxx-$(CONFIG_NET_DSA_MV88E6XXX_PTP) += ptp.o
 mv88e6xxx-objs += serdes.o
 mv88e6xxx-objs += smi.o
+mv88e6xxx-objs += switchdev.o
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index d134153ef023..c1fb7b5ba3ac 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -42,6 +42,7 @@
 #include "ptp.h"
 #include "serdes.h"
 #include "smi.h"
+#include "switchdev.h"
 
 static void assert_reg_lock(struct mv88e6xxx_chip *chip)
 {
@@ -919,6 +920,13 @@ static void mv88e6xxx_mac_link_down(struct dsa_switch *ds, int port,
 	if (err)
 		dev_err(chip->dev,
 			"p%d: failed to force MAC link down\n", port);
+	else
+		if (mv88e6xxx_port_is_locked(chip, port)) {
+			err = mv88e6xxx_atu_locked_entry_flush(ds, port);
+			if (err)
+				dev_err(chip->dev,
+					"p%d: failed to clear locked entries\n", port);
+		}
 }
 
 static void mv88e6xxx_mac_link_up(struct dsa_switch *ds, int port,
@@ -1685,6 +1693,12 @@ static void mv88e6xxx_port_fast_age(struct dsa_switch *ds, int port)
 	struct mv88e6xxx_chip *chip = ds->priv;
 	int err;
 
+	if (mv88e6xxx_port_is_locked(chip, port))
+		err = mv88e6xxx_atu_locked_entry_flush(ds, port);
+	if (err)
+		dev_err(chip->ds->dev, "p%d: failed to clear locked entries: %d\n",
+			port, err);
+
 	mv88e6xxx_reg_lock(chip);
 	err = mv88e6xxx_port_fast_age_fid(chip, port, 0);
 	mv88e6xxx_reg_unlock(chip);
@@ -1721,11 +1735,11 @@ static int mv88e6xxx_vtu_get(struct mv88e6xxx_chip *chip, u16 vid,
 	return err;
 }
 
-static int mv88e6xxx_vtu_walk(struct mv88e6xxx_chip *chip,
-			      int (*cb)(struct mv88e6xxx_chip *chip,
-					const struct mv88e6xxx_vtu_entry *entry,
-					void *priv),
-			      void *priv)
+int mv88e6xxx_vtu_walk(struct mv88e6xxx_chip *chip,
+		       int (*cb)(struct mv88e6xxx_chip *chip,
+				 const struct mv88e6xxx_vtu_entry *entry,
+				 void *priv),
+		       void *priv)
 {
 	struct mv88e6xxx_vtu_entry entry = {
 		.vid = mv88e6xxx_max_vid(chip),
@@ -2727,6 +2741,9 @@ static int mv88e6xxx_port_fdb_add(struct dsa_switch *ds, int port,
 	if (is_locked)
 		return 0;
 
+	if (mv88e6xxx_port_is_locked(chip, port))
+		mv88e6xxx_atu_locked_entry_find_purge(ds, port, addr, vid);
+
 	mv88e6xxx_reg_lock(chip);
 	err = mv88e6xxx_port_db_load_purge(chip, port, addr, vid,
 					   MV88E6XXX_G1_ATU_DATA_STATE_UC_STATIC);
@@ -2740,12 +2757,17 @@ static int mv88e6xxx_port_fdb_del(struct dsa_switch *ds, int port,
 				  struct dsa_db db)
 {
 	struct mv88e6xxx_chip *chip = ds->priv;
-	int err;
+	bool locked_found = false;
+	int err = 0;
 
-	mv88e6xxx_reg_lock(chip);
-	err = mv88e6xxx_port_db_load_purge(chip, port, addr, vid, 0);
-	mv88e6xxx_reg_unlock(chip);
+	if (mv88e6xxx_port_is_locked(chip, port))
+		locked_found = mv88e6xxx_atu_locked_entry_find_purge(ds, port, addr, vid);
 
+	if (!locked_found) {
+		mv88e6xxx_reg_lock(chip);
+		err = mv88e6xxx_port_db_load_purge(chip, port, addr, vid, 0);
+		mv88e6xxx_reg_unlock(chip);
+	}
 	return err;
 }
 
@@ -3814,11 +3836,18 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
 
 static int mv88e6xxx_port_setup(struct dsa_switch *ds, int port)
 {
-	return mv88e6xxx_setup_devlink_regions_port(ds, port);
+	int err;
+
+	err = mv88e6xxx_setup_devlink_regions_port(ds, port);
+	if (!err)
+		return mv88e6xxx_init_violation_handler(ds, port);
+
+	return err;
 }
 
 static void mv88e6xxx_port_teardown(struct dsa_switch *ds, int port)
 {
+	mv88e6xxx_teardown_violation_handler(ds, port);
 	mv88e6xxx_teardown_devlink_regions_port(ds, port);
 }
 
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index 5e03cfe50156..3cc2db722ad9 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -280,6 +280,12 @@ struct mv88e6xxx_port {
 	unsigned int serdes_irq;
 	char serdes_irq_name[64];
 	struct devlink_region *region;
+
+	/* List and maintenance of ATU locked entries */
+	struct mutex ale_list_lock;
+	struct list_head ale_list;
+	struct delayed_work ale_work;
+	int ale_cnt;
 };
 
 enum mv88e6xxx_region_id {
@@ -399,6 +405,9 @@ struct mv88e6xxx_chip {
 	int egress_dest_port;
 	int ingress_dest_port;
 
+	/* Keep the register written age time for easy access */
+	u8 age_time;
+
 	/* Per-port timestamping resources. */
 	struct mv88e6xxx_port_hwtstamp port_hwtstamp[DSA_MAX_PORTS];
 
@@ -803,6 +812,12 @@ static inline void mv88e6xxx_reg_unlock(struct mv88e6xxx_chip *chip)
 	mutex_unlock(&chip->reg_lock);
 }
 
+int mv88e6xxx_vtu_walk(struct mv88e6xxx_chip *chip,
+		       int (*cb)(struct mv88e6xxx_chip *chip,
+				 const struct mv88e6xxx_vtu_entry *entry,
+				 void *priv),
+		       void *priv);
+
 int mv88e6xxx_fid_map(struct mv88e6xxx_chip *chip, unsigned long *bitmap);
 
 #endif /* _MV88E6XXX_CHIP_H */
diff --git a/drivers/net/dsa/mv88e6xxx/global1.h b/drivers/net/dsa/mv88e6xxx/global1.h
index 65958b2a0d3a..503fbf216670 100644
--- a/drivers/net/dsa/mv88e6xxx/global1.h
+++ b/drivers/net/dsa/mv88e6xxx/global1.h
@@ -136,6 +136,7 @@
 #define MV88E6XXX_G1_ATU_DATA_TRUNK				0x8000
 #define MV88E6XXX_G1_ATU_DATA_TRUNK_ID_MASK			0x00f0
 #define MV88E6XXX_G1_ATU_DATA_PORT_VECTOR_MASK			0x3ff0
+#define MV88E6XXX_G1_ATU_DATA_PORT_VECTOR_NO_EGRESS		0x0000
 #define MV88E6XXX_G1_ATU_DATA_STATE_MASK			0x000f
 #define MV88E6XXX_G1_ATU_DATA_STATE_UC_UNUSED			0x0000
 #define MV88E6XXX_G1_ATU_DATA_STATE_UC_AGE_1_OLDEST		0x0001
diff --git a/drivers/net/dsa/mv88e6xxx/global1_atu.c b/drivers/net/dsa/mv88e6xxx/global1_atu.c
index 5d120d53823c..d4ca976dac91 100644
--- a/drivers/net/dsa/mv88e6xxx/global1_atu.c
+++ b/drivers/net/dsa/mv88e6xxx/global1_atu.c
@@ -12,6 +12,8 @@
 
 #include "chip.h"
 #include "global1.h"
+#include "port.h"
+#include "switchdev.h"
 
 /* Offset 0x01: ATU FID Register */
 
@@ -54,6 +56,7 @@ int mv88e6xxx_g1_atu_set_age_time(struct mv88e6xxx_chip *chip,
 
 	/* Round to nearest multiple of coeff */
 	age_time = (msecs + coeff / 2) / coeff;
+	chip->age_time = age_time;
 
 	err = mv88e6xxx_g1_read(chip, MV88E6XXX_G1_ATU_CTL, &val);
 	if (err)
@@ -369,6 +372,7 @@ static irqreturn_t mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
 	int spid;
 	int err;
 	u16 val;
+	u16 fid;
 
 	mv88e6xxx_reg_lock(chip);
 
@@ -380,6 +384,10 @@ static irqreturn_t mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
 	if (err)
 		goto out;
 
+	err = mv88e6xxx_g1_read(chip, MV88E6352_G1_ATU_FID, &fid);
+	if (err)
+		goto out;
+
 	err = mv88e6xxx_g1_atu_data_read(chip, &entry);
 	if (err)
 		goto out;
@@ -388,6 +396,8 @@ static irqreturn_t mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
 	if (err)
 		goto out;
 
+	mv88e6xxx_reg_unlock(chip);
+
 	spid = entry.state;
 
 	if (val & MV88E6XXX_G1_ATU_OP_AGE_OUT_VIOLATION) {
@@ -408,6 +418,11 @@ static irqreturn_t mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
 				    "ATU miss violation for %pM portvec %x spid %d\n",
 				    entry.mac, entry.portvec, spid);
 		chip->ports[spid].atu_miss_violation++;
+		if (fid && mv88e6xxx_port_is_locked(chip, spid))
+			err = mv88e6xxx_handle_violation(chip, spid, &entry, fid,
+							 MV88E6XXX_G1_ATU_OP_MISS_VIOLATION);
+		if (err)
+			goto out;
 	}
 
 	if (val & MV88E6XXX_G1_ATU_OP_FULL_VIOLATION) {
@@ -416,7 +431,6 @@ static irqreturn_t mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
 				    entry.mac, entry.portvec, spid);
 		chip->ports[spid].atu_full_violation++;
 	}
-	mv88e6xxx_reg_unlock(chip);
 
 	return IRQ_HANDLED;
 
diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
index 795b3128768f..57eb25e9eb63 100644
--- a/drivers/net/dsa/mv88e6xxx/port.c
+++ b/drivers/net/dsa/mv88e6xxx/port.c
@@ -14,9 +14,11 @@
 #include <linux/phylink.h>
 
 #include "chip.h"
+#include "global1.h"
 #include "global2.h"
 #include "port.h"
 #include "serdes.h"
+#include "switchdev.h"
 
 int mv88e6xxx_port_read(struct mv88e6xxx_chip *chip, int port, int reg,
 			u16 *val)
@@ -1239,6 +1241,17 @@ int mv88e6xxx_port_set_mirror(struct mv88e6xxx_chip *chip, int port,
 	return err;
 }
 
+bool mv88e6xxx_port_is_locked(struct mv88e6xxx_chip *chip, int port)
+{
+	u16 reg;
+
+	mv88e6xxx_reg_lock(chip);
+	mv88e6xxx_port_read(chip, port, MV88E6XXX_PORT_CTL0, &reg);
+	mv88e6xxx_reg_unlock(chip);
+
+	return reg & MV88E6XXX_PORT_CTL0_SA_FILT_DROP_ON_LOCK;
+}
+
 int mv88e6xxx_port_set_lock(struct mv88e6xxx_chip *chip, int port,
 			    bool locked)
 {
@@ -1257,13 +1270,18 @@ int mv88e6xxx_port_set_lock(struct mv88e6xxx_chip *chip, int port,
 	if (err)
 		return err;
 
-	err = mv88e6xxx_port_read(chip, port, MV88E6XXX_PORT_ASSOC_VECTOR, &reg);
-	if (err)
-		return err;
+	if (!locked) {
+		err = mv88e6xxx_atu_locked_entry_flush(chip->ds, port);
+		if (err)
+			return err;
+	}
 
-	reg &= ~MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT;
-	if (locked)
-		reg |= MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT;
+	reg = 0;
+	if (locked) {
+		reg = (1 << port);
+		reg |= MV88E6XXX_PORT_ASSOC_VECTOR_IGNORE_WRONG |
+			MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT;
+	}
 
 	return mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_ASSOC_VECTOR, reg);
 }
diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h
index e0a705d82019..5c1d485e7442 100644
--- a/drivers/net/dsa/mv88e6xxx/port.h
+++ b/drivers/net/dsa/mv88e6xxx/port.h
@@ -231,6 +231,7 @@
 #define MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT		0x2000
 #define MV88E6XXX_PORT_ASSOC_VECTOR_IGNORE_WRONG	0x1000
 #define MV88E6XXX_PORT_ASSOC_VECTOR_REFRESH_LOCKED	0x0800
+#define MV88E6XXX_PORT_ASSOC_VECTOR_PAV_MASK		0x07ff
 
 /* Offset 0x0C: Port ATU Control */
 #define MV88E6XXX_PORT_ATU_CTL		0x0c
@@ -374,6 +375,7 @@ int mv88e6xxx_port_set_fid(struct mv88e6xxx_chip *chip, int port, u16 fid);
 int mv88e6xxx_port_get_pvid(struct mv88e6xxx_chip *chip, int port, u16 *pvid);
 int mv88e6xxx_port_set_pvid(struct mv88e6xxx_chip *chip, int port, u16 pvid);
 
+bool mv88e6xxx_port_is_locked(struct mv88e6xxx_chip *chip, int port);
 int mv88e6xxx_port_set_lock(struct mv88e6xxx_chip *chip, int port,
 			    bool locked);
 
diff --git a/drivers/net/dsa/mv88e6xxx/switchdev.c b/drivers/net/dsa/mv88e6xxx/switchdev.c
new file mode 100644
index 000000000000..e6c67fb741af
--- /dev/null
+++ b/drivers/net/dsa/mv88e6xxx/switchdev.c
@@ -0,0 +1,280 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * switchdev.c
+ *
+ *	Authors:
+ *	Hans J. Schultz		<hans.schultz@westermo.com>
+ *
+ */
+
+#include <net/switchdev.h>
+#include <linux/list.h>
+#include "chip.h"
+#include "global1.h"
+#include "switchdev.h"
+
+static void mv88e6xxx_atu_locked_entry_purge(struct mv88e6xxx_atu_locked_entry *ale, bool notify)
+{
+	struct switchdev_notifier_fdb_info info = {
+		.addr = ale->mac,
+		.vid = ale->vid,
+		.is_locked = true,
+		.offloaded = true,
+	};
+	struct mv88e6xxx_atu_entry entry;
+	struct net_device *brport;
+	struct dsa_port *dp;
+
+	entry.portvec = MV88E6XXX_G1_ATU_DATA_PORT_VECTOR_NO_EGRESS;
+	entry.state = MV88E6XXX_G1_ATU_DATA_STATE_UC_UNUSED;
+	entry.trunk = false;
+	ether_addr_copy(entry.mac, ale->mac);
+
+	mv88e6xxx_reg_lock(ale->chip);
+	mv88e6xxx_g1_atu_loadpurge(ale->chip, ale->fid, &entry);
+	mv88e6xxx_reg_unlock(ale->chip);
+
+	dp = dsa_to_port(ale->chip->ds, ale->port);
+
+	if (notify) {
+		rtnl_lock();
+		brport = dsa_port_to_bridge_port(dp);
+
+		if (brport) {
+			call_switchdev_notifiers(SWITCHDEV_FDB_DEL_TO_BRIDGE,
+						 brport, &info.info, NULL);
+		} else {
+			dev_err(ale->chip->dev, "No bridge port for dsa port belonging to port %d\n",
+				ale->port);
+		}
+		rtnl_unlock();
+	}
+
+	list_del(&ale->list);
+	kfree(ale);
+}
+
+static void mv88e6xxx_atu_locked_entry_cleanup(struct work_struct *work)
+{
+	struct mv88e6xxx_port *p = container_of(work, struct mv88e6xxx_port, ale_work.work);
+	struct mv88e6xxx_atu_locked_entry *ale, *tmp;
+
+	mutex_lock(&p->ale_list_lock);
+	list_for_each_entry_safe(ale, tmp, &p->ale_list, list) {
+		if (time_after(jiffies, ale->expires)) {
+			mv88e6xxx_atu_locked_entry_purge(ale, true);
+			p->ale_cnt--;
+		}
+	}
+	mutex_unlock(&p->ale_list_lock);
+
+	mod_delayed_work(system_long_wq, &p->ale_work, msecs_to_jiffies(100));
+}
+
+static int mv88e6xxx_new_atu_locked_entry(struct mv88e6xxx_chip *chip, const unsigned char *addr,
+					  int port, u16 fid, u16 vid,
+					  struct mv88e6xxx_atu_locked_entry **alep)
+{
+	struct mv88e6xxx_atu_locked_entry *ale;
+	unsigned long now, age_time;
+
+	ale = kmalloc(sizeof(*ale), GFP_ATOMIC);
+	if (!ale)
+		return -ENOMEM;
+
+	ether_addr_copy(ale->mac, addr);
+	ale->chip = chip;
+	ale->port = port;
+	ale->fid = fid;
+	ale->vid = vid;
+	now = jiffies;
+	age_time = chip->age_time * chip->info->age_time_coeff;
+	ale->expires = now + age_time;
+
+	*alep = ale;
+	return 0;
+}
+
+struct mv88e6xxx_fid_search_ctx {
+	u16 fid_search;
+	u16 vid_found;
+};
+
+static int mv88e6xxx_find_vid_on_matching_fid(struct mv88e6xxx_chip *chip,
+					      const struct mv88e6xxx_vtu_entry *entry,
+					      void *priv)
+{
+	struct mv88e6xxx_fid_search_ctx *ctx = priv;
+
+	if (ctx->fid_search == entry->fid) {
+		ctx->vid_found = entry->vid;
+		return 1;
+	}
+
+	return 0;
+}
+
+int mv88e6xxx_handle_violation(struct mv88e6xxx_chip *chip, int port,
+			       struct mv88e6xxx_atu_entry *entry,
+			       u16 fid, u16 type)
+{
+	struct switchdev_notifier_fdb_info info = {
+		.addr = entry->mac,
+		.vid = 0,
+		.is_locked = true,
+		.offloaded = true,
+	};
+	struct mv88e6xxx_atu_locked_entry *ale;
+	struct mv88e6xxx_fid_search_ctx ctx;
+	struct net_device *brport;
+	struct mv88e6xxx_port *p;
+	struct dsa_port *dp;
+	int err;
+
+	if (!mv88e6xxx_is_invalid_port(chip, port))
+		p = &chip->ports[port];
+	else
+		return -ENODEV;
+
+	ctx.fid_search = fid;
+	mv88e6xxx_reg_lock(chip);
+	err = mv88e6xxx_vtu_walk(chip, mv88e6xxx_find_vid_on_matching_fid, &ctx);
+	mv88e6xxx_reg_unlock(chip);
+	if (err < 0)
+		return err;
+	if (err == 1)
+		info.vid = ctx.vid_found;
+	else
+		return -ENODATA;
+
+	switch (type) {
+	case MV88E6XXX_G1_ATU_OP_MISS_VIOLATION:
+		mutex_lock(&p->ale_list_lock);
+		if (p->ale_cnt >= ATU_LOCKED_ENTRIES_MAX)
+			goto exit;
+		mutex_unlock(&p->ale_list_lock);
+		entry->portvec = MV88E6XXX_G1_ATU_DATA_PORT_VECTOR_NO_EGRESS;
+		entry->state = MV88E6XXX_G1_ATU_DATA_STATE_UC_STATIC;
+		entry->trunk = false;
+
+		mv88e6xxx_reg_lock(chip);
+		err = mv88e6xxx_g1_atu_loadpurge(chip, fid, entry);
+		if (err)
+			goto fail;
+		mv88e6xxx_reg_unlock(chip);
+
+		dp = dsa_to_port(chip->ds, port);
+		err = mv88e6xxx_new_atu_locked_entry(chip, entry->mac, port, fid,
+						     info.vid, &ale);
+		if (err)
+			return err;
+
+		mutex_lock(&p->ale_list_lock);
+		list_add(&ale->list, &p->ale_list);
+		p->ale_cnt++;
+		mutex_unlock(&p->ale_list_lock);
+
+		rtnl_lock();
+		brport = dsa_port_to_bridge_port(dp);
+		if (!brport) {
+			rtnl_unlock();
+			return -ENODEV;
+		}
+		err = call_switchdev_notifiers(SWITCHDEV_FDB_ADD_TO_BRIDGE,
+					       brport, &info.info, NULL);
+		rtnl_unlock();
+		break;
+	}
+
+	return err;
+
+fail:
+	mv88e6xxx_reg_unlock(chip);
+	return err;
+
+exit:
+	mutex_unlock(&p->ale_list_lock);
+	return 0;
+}
+
+bool mv88e6xxx_atu_locked_entry_find_purge(struct dsa_switch *ds, int port,
+					   const unsigned char *addr, u16 vid)
+{
+	struct mv88e6xxx_atu_locked_entry *ale, *tmp;
+	struct mv88e6xxx_chip *chip = ds->priv;
+	struct mv88e6xxx_port *p;
+	bool found = false;
+
+	p = &chip->ports[port];
+	mutex_lock(&p->ale_list_lock);
+	list_for_each_entry_safe(ale, tmp, &p->ale_list, list) {
+		if (ether_addr_equal(ale->mac, addr) == 0) {
+			if (ale->vid == vid) {
+				mv88e6xxx_atu_locked_entry_purge(ale, false);
+				p->ale_cnt--;
+				found = true;
+				break;
+			}
+		}
+	}
+	mutex_unlock(&p->ale_list_lock);
+	return found;
+}
+
+int mv88e6xxx_atu_locked_entry_flush(struct dsa_switch *ds, int port)
+{
+	struct mv88e6xxx_atu_locked_entry *ale, *tmp;
+	struct mv88e6xxx_chip *chip = ds->priv;
+	struct mv88e6xxx_port *p;
+
+	if (!mv88e6xxx_is_invalid_port(chip, port))
+		p = &chip->ports[port];
+	else
+		return -ENODEV;
+
+	mutex_lock(&p->ale_list_lock);
+	list_for_each_entry_safe(ale, tmp, &p->ale_list, list) {
+		mv88e6xxx_atu_locked_entry_purge(ale, false);
+		p->ale_cnt--;
+	}
+	mutex_unlock(&p->ale_list_lock);
+
+	return 0;
+}
+
+int mv88e6xxx_init_violation_handler(struct dsa_switch *ds, int port)
+{
+	struct mv88e6xxx_chip *chip = ds->priv;
+	struct mv88e6xxx_port *p;
+
+	if (!mv88e6xxx_is_invalid_port(chip, port))
+		p = &chip->ports[port];
+	else
+		return -ENODEV;
+
+	INIT_LIST_HEAD(&p->ale_list);
+	mutex_init(&p->ale_list_lock);
+	p->ale_cnt = 0;
+	INIT_DELAYED_WORK(&p->ale_work, mv88e6xxx_atu_locked_entry_cleanup);
+	mod_delayed_work(system_long_wq, &p->ale_work, msecs_to_jiffies(500));
+
+	ds->ageing_time_min = 100;
+	return 0;
+}
+
+int mv88e6xxx_teardown_violation_handler(struct dsa_switch *ds, int port)
+{
+	struct mv88e6xxx_chip *chip = ds->priv;
+	struct mv88e6xxx_port *p;
+
+	if (!mv88e6xxx_is_invalid_port(chip, port))
+		p = &chip->ports[port];
+	else
+		return -ENODEV;
+
+	cancel_delayed_work(&p->ale_work);
+	mv88e6xxx_atu_locked_entry_flush(ds, port);
+	mutex_destroy(&p->ale_list_lock);
+
+	return 0;
+}
diff --git a/drivers/net/dsa/mv88e6xxx/switchdev.h b/drivers/net/dsa/mv88e6xxx/switchdev.h
new file mode 100644
index 000000000000..df2005c36f47
--- /dev/null
+++ b/drivers/net/dsa/mv88e6xxx/switchdev.h
@@ -0,0 +1,37 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * switchdev.h
+ *
+ *	Authors:
+ *	Hans J. Schultz		<hans.schultz@westermo.com>
+ *
+ */
+
+#ifndef DRIVERS_NET_DSA_MV88E6XXX_SWITCHDEV_H_
+#define DRIVERS_NET_DSA_MV88E6XXX_SWITCHDEV_H_
+
+#include <net/switchdev.h>
+#include "chip.h"
+
+#define ATU_LOCKED_ENTRIES_MAX	64
+
+struct mv88e6xxx_atu_locked_entry {
+	struct		list_head list;
+	struct		mv88e6xxx_chip *chip;
+	int		port;
+	u8		mac[ETH_ALEN];
+	u16		fid;
+	u16		vid;
+	unsigned long	expires;
+};
+
+int mv88e6xxx_handle_violation(struct mv88e6xxx_chip *chip, int port,
+			       struct mv88e6xxx_atu_entry *entry,
+			       u16 fid, u16 type);
+bool mv88e6xxx_atu_locked_entry_find_purge(struct dsa_switch *ds, int port,
+					   const unsigned char *addr, u16 vid);
+int mv88e6xxx_atu_locked_entry_flush(struct dsa_switch *ds, int port);
+int mv88e6xxx_init_violation_handler(struct dsa_switch *ds, int port);
+int mv88e6xxx_teardown_violation_handler(struct dsa_switch *ds, int port);
+
+#endif /* DRIVERS_NET_DSA_MV88E6XXX_SWITCHDEV_H_ */
-- 
2.30.2

[PATCH v4 net-next 6/6] selftests: forwarding: add test of MAC-Auth Bypass to locked port tests

[Hans Schultz]

Verify that the MAC-Auth mechanism works by adding a FDB entry with the
locked flag set, denying access until the FDB entry is replaced with a
FDB entry without the locked flag set.

Signed-off-by: Hans Schultz <netdev@kapio-technology.com>
---
 .../net/forwarding/bridge_locked_port.sh      | 30 ++++++++++++++++++-
 1 file changed, 29 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..1ee12d7b5c8b 100755
--- a/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
+++ b/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
@@ -1,7 +1,7 @@
 #!/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"
 NUM_NETIFS=4
 CHECK_TC="no"
 source lib.sh
@@ -166,6 +166,34 @@ locked_port_ipv6()
 	log_test "Locked port ipv6"
 }
 
+locked_port_mab()
+{
+	RET=0
+	check_locked_port_support || return 0
+
+	ping_do $h1 192.0.2.2
+	check_err $? "MAB: Ping did not work before locking port"
+
+	bridge link set dev $swp1 locked on
+	bridge link set dev $swp1 learning on
+
+	ping_do $h1 192.0.2.2
+	check_fail $? "MAB: Ping worked on locked port without FDB entry"
+
+	bridge fdb show | grep `mac_get $h1` | grep -q "locked"
+	check_err $? "MAB: No locked fdb entry after ping on locked port"
+
+	bridge fdb replace `mac_get $h1` dev $swp1 master static
+
+	ping_do $h1 192.0.2.2
+	check_err $? "MAB: Ping did not work with fdb entry without locked flag"
+
+	bridge fdb del `mac_get $h1` dev $swp1 master
+	bridge link set dev $swp1 learning off
+	bridge link set dev $swp1 locked off
+
+	log_test "Locked port MAB"
+}
 trap cleanup EXIT
 
 setup_prepare
-- 
2.30.2

Re: [PATCH v4 net-next 0/6] Extend locked port feature with FDB locked flag (MAC-Auth/MAB)

[Jakub Kicinski]

On Thu,  7 Jul 2022 17:29:24 +0200 Hans Schultz wrote:
>  [PATCH v4 net-next 0/6] Extend locked port feature with FDB locked flag (MAC-Auth/MAB)

Let's give it a day or two for feedback but the series does not apply
cleanly to net-next so a rebase & repost will be needed even if it's
otherwise perfect.

Re: [PATCH v4 net-next 3/6] drivers: net: dsa: add locked fdb entry flag to drivers

[kernel test robot]

Hi Hans,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net/master]
[also build test ERROR on shuah-kselftest/next linus/master v5.19-rc5]
[cannot apply to net-next/master next-20220707]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Hans-Schultz/Extend-locked-port-feature-with-FDB-locked-flag-MAC-Auth-MAB/20220707-233246
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 07266d066301b97ad56a693f81b29b7ced429b27
config: i386-allyesconfig (https://download.01.org/0day-ci/archive/20220708/202207081529.riNiUWOf-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/ebd598d7ea6c015001489c4293da887763491086
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Hans-Schultz/Extend-locked-port-feature-with-FDB-locked-flag-MAC-Auth-MAB/20220707-233246
        git checkout ebd598d7ea6c015001489c4293da887763491086
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/net/dsa/sja1105/sja1105_main.c: In function 'sja1105_mdb_add':
>> drivers/net/dsa/sja1105/sja1105_main.c:1952:63: error: incompatible type for argument 5 of 'sja1105_fdb_add'
    1952 |         return sja1105_fdb_add(ds, port, mdb->addr, mdb->vid, db);
         |                                                               ^~
         |                                                               |
         |                                                               struct dsa_db
   drivers/net/dsa/sja1105/sja1105_main.c:1805:33: note: expected 'bool' {aka '_Bool'} but argument is of type 'struct dsa_db'
    1805 |                            bool is_locked,
         |                            ~~~~~^~~~~~~~~
>> drivers/net/dsa/sja1105/sja1105_main.c:1952:16: error: too few arguments to function 'sja1105_fdb_add'
    1952 |         return sja1105_fdb_add(ds, port, mdb->addr, mdb->vid, db);
         |                ^~~~~~~~~~~~~~~
   drivers/net/dsa/sja1105/sja1105_main.c:1803:12: note: declared here
    1803 | static int sja1105_fdb_add(struct dsa_switch *ds, int port,
         |            ^~~~~~~~~~~~~~~
   drivers/net/dsa/sja1105/sja1105_main.c:1953:1: error: control reaches end of non-void function [-Werror=return-type]
    1953 | }
         | ^
   cc1: some warnings being treated as errors


vim +/sja1105_fdb_add +1952 drivers/net/dsa/sja1105/sja1105_main.c

5126ec72a094bd3 Vladimir Oltean 2021-08-08  1947  
a52b2da778fc93e Vladimir Oltean 2021-01-09  1948  static int sja1105_mdb_add(struct dsa_switch *ds, int port,
c26933639b5402c Vladimir Oltean 2022-02-25  1949  			   const struct switchdev_obj_port_mdb *mdb,
c26933639b5402c Vladimir Oltean 2022-02-25  1950  			   struct dsa_db db)
291d1e72b756424 Vladimir Oltean 2019-05-02  1951  {
c26933639b5402c Vladimir Oltean 2022-02-25 @1952  	return sja1105_fdb_add(ds, port, mdb->addr, mdb->vid, db);
291d1e72b756424 Vladimir Oltean 2019-05-02  1953  }
291d1e72b756424 Vladimir Oltean 2019-05-02  1954  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v4 net-next 3/6] drivers: net: dsa: add locked fdb entry flag to drivers

[Vladimir Oltean]

Hi Hans,

On Thu, Jul 07, 2022 at 05:29:27PM +0200, Hans Schultz wrote:
> Ignore locked fdb entries coming in on all drivers.
> 
> Signed-off-by: Hans Schultz <netdev@kapio-technology.com>
> ---

A good patch should have a reason for the change in the commit message.
This has no reason because there is no reason.

Think about it, you've said it yourself in patch 1:

| Only the kernel can set this FDB entry flag, while userspace can read
| the flag and remove it by replacing or deleting the FDB entry.

So if user space will never add locked FDB entries to the bridge,
then FDB entries with is_locked=true are never transported using
SWITCHDEV_FDB_ADD_TO_DEVICE to drivers, and so, there is no reason at
all to pass is_locked to drivers, just for them to ignore something that
won't appear.

You just need this for SWITCHDEV_FDB_ADD_TO_BRIDGE, so please keep it
only in those code paths, and remove it from net/dsa/slave.c as well.

>  drivers/net/dsa/b53/b53_common.c       | 5 +++++
>  drivers/net/dsa/b53/b53_priv.h         | 1 +
>  drivers/net/dsa/hirschmann/hellcreek.c | 5 +++++
>  drivers/net/dsa/lan9303-core.c         | 5 +++++
>  drivers/net/dsa/lantiq_gswip.c         | 5 +++++
>  drivers/net/dsa/microchip/ksz9477.c    | 5 +++++
>  drivers/net/dsa/mt7530.c               | 5 +++++
>  drivers/net/dsa/mv88e6xxx/chip.c       | 5 +++++
>  drivers/net/dsa/ocelot/felix.c         | 5 +++++
>  drivers/net/dsa/qca8k.c                | 5 +++++
>  drivers/net/dsa/sja1105/sja1105_main.c | 5 +++++
>  include/net/dsa.h                      | 1 +
>  net/dsa/switch.c                       | 4 ++--
>  13 files changed, 54 insertions(+), 2 deletions(-)

Re: [PATCH v4 net-next 2/6] net: switchdev: add support for offloading of fdb locked flag

[Vladimir Oltean]

On Thu, Jul 07, 2022 at 05:29:26PM +0200, Hans Schultz wrote:
> Used for Mac-auth/MAB feature in the offloaded case.
> Send flag through switchdev to driver.
> 
> Signed-off-by: Hans Schultz <netdev@kapio-technology.com>
> ---
>  include/net/dsa.h         | 6 ++++++
>  include/net/switchdev.h   | 1 +
>  net/bridge/br.c           | 3 ++-
>  net/bridge/br_fdb.c       | 7 +++++--
>  net/bridge/br_private.h   | 2 +-
>  net/bridge/br_switchdev.c | 1 +
>  net/dsa/dsa_priv.h        | 4 +++-
>  net/dsa/port.c            | 7 ++++++-
>  net/dsa/slave.c           | 4 +++-
>  net/dsa/switch.c          | 6 +++---
>  10 files changed, 31 insertions(+), 10 deletions(-)
> 
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 14f07275852b..a5a843b2d67d 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -330,6 +330,12 @@ struct dsa_port {
>  	/* List of VLANs that CPU and DSA ports are members of. */
>  	struct mutex		vlans_lock;
>  	struct list_head	vlans;
> +
> +	/* List and maintenance of locked ATU entries */
> +	struct mutex		locked_entries_list_lock;
> +	struct list_head	atu_locked_entries_list;
> +	atomic_t		atu_locked_entry_cnt;
> +	struct delayed_work	atu_work;

Leftovers from an old change, please drop from the patch.

>  };
>  
>  /* TODO: ideally DSA ports would have a single dp->link_dp member,
> diff --git a/include/net/switchdev.h b/include/net/switchdev.h
> index aa0171d5786d..9f83c835ee62 100644
> --- a/include/net/switchdev.h
> +++ b/include/net/switchdev.h
> @@ -245,6 +245,7 @@ struct switchdev_notifier_fdb_info {
>  	u16 vid;
>  	u8 added_by_user:1,
>  	   is_local:1,
> +	   is_locked:1,
>  	   offloaded:1;
>  };
>  
> diff --git a/net/bridge/br.c b/net/bridge/br.c
> index 96e91d69a9a8..fe0a4741fcda 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, false,
> +						fdb_info->is_locked);
>  		if (err) {
>  			err = notifier_from_errno(err);
>  			break;
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index ee9064a536ae..32ebb18050b9 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -1136,7 +1136,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, true, false);
>  	} else {
>  		spin_lock_bh(&br->hash_lock);
>  		err = fdb_add_entry(br, p, addr, ndm, nlh_flags, vid, nfea_tb);
> @@ -1366,7 +1366,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,
> -			      bool swdev_notify)
> +			      bool swdev_notify, bool locked)
>  {
>  	struct net_bridge_fdb_entry *fdb;
>  	bool modified = false;
> @@ -1386,6 +1386,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_ENTRY_LOCKED);
> +
>  		fdb = fdb_create(br, p, addr, vid, flags);
>  		if (!fdb) {
>  			err = -ENOMEM;
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 47a3598d25c8..9082451b4d40 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 swdev_notify, bool locked);
>  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..85e566b856e1 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->is_locked = test_bit(BR_FDB_ENTRY_LOCKED, &fdb->flags);
>  	item->info.dev = (!p || item->is_local) ? br->dev : p->dev;
>  	item->info.ctx = ctx;
>  }
> diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
> index d9722e49864b..42f47a94b0f0 100644
> --- a/net/dsa/dsa_priv.h
> +++ b/net/dsa/dsa_priv.h
> @@ -65,6 +65,7 @@ struct dsa_notifier_fdb_info {
>  	const struct dsa_port *dp;
>  	const unsigned char *addr;
>  	u16 vid;
> +	bool is_locked;

drop

>  	struct dsa_db db;
>  };
>  
> @@ -131,6 +132,7 @@ struct dsa_switchdev_event_work {
>  	unsigned char addr[ETH_ALEN];
>  	u16 vid;
>  	bool host_addr;
> +	bool is_locked;

drop

>  };
>  
>  enum dsa_standalone_event {
> @@ -232,7 +234,7 @@ int dsa_port_vlan_msti(struct dsa_port *dp,
>  		       const struct switchdev_vlan_msti *msti);
>  int dsa_port_mtu_change(struct dsa_port *dp, int new_mtu);
>  int dsa_port_fdb_add(struct dsa_port *dp, const unsigned char *addr,
> -		     u16 vid);
> +		     u16 vid, bool is_locked);

drop

>  int dsa_port_fdb_del(struct dsa_port *dp, const unsigned char *addr,
>  		     u16 vid);
>  int dsa_port_standalone_host_fdb_add(struct dsa_port *dp,
> diff --git a/net/dsa/port.c b/net/dsa/port.c
> index 3738f2d40a0b..8bdac9aabe5d 100644
> --- a/net/dsa/port.c
> +++ b/net/dsa/port.c
> @@ -35,6 +35,7 @@ static void dsa_port_notify_bridge_fdb_flush(const struct dsa_port *dp, u16 vid)
>  	struct net_device *brport_dev = dsa_port_to_bridge_port(dp);
>  	struct switchdev_notifier_fdb_info info = {
>  		.vid = vid,
> +		.is_locked = false,

drop

>  	};
>  
>  	/* When the port becomes standalone it has already left the bridge.
> @@ -950,12 +951,13 @@ int dsa_port_mtu_change(struct dsa_port *dp, int new_mtu)
>  }
>  
>  int dsa_port_fdb_add(struct dsa_port *dp, const unsigned char *addr,
> -		     u16 vid)
> +		     u16 vid, bool is_locked)

drop

>  {
>  	struct dsa_notifier_fdb_info info = {
>  		.dp = dp,
>  		.addr = addr,
>  		.vid = vid,
> +		.is_locked = is_locked,

drop

>  		.db = {
>  			.type = DSA_DB_BRIDGE,
>  			.bridge = *dp->bridge,
> @@ -979,6 +981,7 @@ int dsa_port_fdb_del(struct dsa_port *dp, const unsigned char *addr,
>  		.dp = dp,
>  		.addr = addr,
>  		.vid = vid,
> +		.is_locked = false,

drop

>  		.db = {
>  			.type = DSA_DB_BRIDGE,
>  			.bridge = *dp->bridge,
> @@ -999,6 +1002,7 @@ static int dsa_port_host_fdb_add(struct dsa_port *dp,
>  		.dp = dp,
>  		.addr = addr,
>  		.vid = vid,
> +		.is_locked = false,

drop

>  		.db = db,
>  	};
>  
> @@ -1050,6 +1054,7 @@ static int dsa_port_host_fdb_del(struct dsa_port *dp,
>  		.dp = dp,
>  		.addr = addr,
>  		.vid = vid,
> +		.is_locked = false,

drop

>  		.db = db,
>  	};
>  
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 801a5d445833..905b15e4eab9 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -2784,6 +2784,7 @@ static void dsa_slave_switchdev_event_work(struct work_struct *work)
>  		container_of(work, struct dsa_switchdev_event_work, work);
>  	const unsigned char *addr = switchdev_work->addr;
>  	struct net_device *dev = switchdev_work->dev;
> +	bool is_locked = switchdev_work->is_locked;

drop

>  	u16 vid = switchdev_work->vid;
>  	struct dsa_switch *ds;
>  	struct dsa_port *dp;
> @@ -2799,7 +2800,7 @@ static void dsa_slave_switchdev_event_work(struct work_struct *work)
>  		else if (dp->lag)
>  			err = dsa_port_lag_fdb_add(dp, addr, vid);
>  		else
> -			err = dsa_port_fdb_add(dp, addr, vid);
> +			err = dsa_port_fdb_add(dp, addr, vid, is_locked);

drop

>  		if (err) {
>  			dev_err(ds->dev,
>  				"port %d failed to add %pM vid %d to fdb: %d\n",
> @@ -2907,6 +2908,7 @@ static int dsa_slave_fdb_event(struct net_device *dev,
>  	ether_addr_copy(switchdev_work->addr, fdb_info->addr);
>  	switchdev_work->vid = fdb_info->vid;
>  	switchdev_work->host_addr = host_addr;
> +	switchdev_work->is_locked = fdb_info->is_locked;

drop

>  
>  	dsa_schedule_work(&switchdev_work->work);
>  
> diff --git a/net/dsa/switch.c b/net/dsa/switch.c
> index 2b56218fc57c..32b1e7ac6373 100644
> --- a/net/dsa/switch.c
> +++ b/net/dsa/switch.c
> @@ -234,7 +234,7 @@ static int dsa_port_do_mdb_del(struct dsa_port *dp,
>  }
>  
>  static int dsa_port_do_fdb_add(struct dsa_port *dp, const unsigned char *addr,
> -			       u16 vid, struct dsa_db db)
> +			       u16 vid, bool is_locked, struct dsa_db db)

drop

>  {
>  	struct dsa_switch *ds = dp->ds;
>  	struct dsa_mac_addr *a;
> @@ -398,7 +398,7 @@ static int dsa_switch_host_fdb_add(struct dsa_switch *ds,
>  	dsa_switch_for_each_port(dp, ds) {
>  		if (dsa_port_host_address_match(dp, info->dp)) {
>  			err = dsa_port_do_fdb_add(dp, info->addr, info->vid,
> -						  info->db);
> +						  false, info->db);

drop

>  			if (err)
>  				break;
>  		}
> @@ -437,7 +437,7 @@ static int dsa_switch_fdb_add(struct dsa_switch *ds,
>  	if (!ds->ops->port_fdb_add)
>  		return -EOPNOTSUPP;
>  
> -	return dsa_port_do_fdb_add(dp, info->addr, info->vid, info->db);
> +	return dsa_port_do_fdb_add(dp, info->addr, info->vid, info->is_locked, info->db);

drop

>  }
>  
>  static int dsa_switch_fdb_del(struct dsa_switch *ds,
> -- 
> 2.30.2
> 

Re: [PATCH v4 net-next 3/6] drivers: net: dsa: add locked fdb entry flag to drivers

[netdev]

On 2022-07-08 10:49, Vladimir Oltean wrote:
> Hi Hans,
> 
> On Thu, Jul 07, 2022 at 05:29:27PM +0200, Hans Schultz wrote:
>> Ignore locked fdb entries coming in on all drivers.
>> 
>> Signed-off-by: Hans Schultz <netdev@kapio-technology.com>
>> ---
> 
> A good patch should have a reason for the change in the commit message.
> This has no reason because there is no reason.
> 
> Think about it, you've said it yourself in patch 1:
> 
> | Only the kernel can set this FDB entry flag, while userspace can read
> | the flag and remove it by replacing or deleting the FDB entry.
> 
> So if user space will never add locked FDB entries to the bridge,
> then FDB entries with is_locked=true are never transported using
> SWITCHDEV_FDB_ADD_TO_DEVICE to drivers, and so, there is no reason at
> all to pass is_locked to drivers, just for them to ignore something 
> that
> won't appear.

Correct me if I am wrong, but since the bridge can add locked entries, 
and
the ensuring fdb update will create a SWITCHDEV_FDB_ADD_TO_DEVICE, those 
entries
should reach the driver. The policy to ignore those in the driver can be
seen as either the right thing to do, or not yet implemented.

I remember Ido wrote at a point that the scheme they use is to trap 
various
packets to the CPU and let the bridge add the locked entry, which I then
understand is sent to the driver with a SWITCHDEV_FDB_ADD_TO_DEVICE 
event.

> 
> You just need this for SWITCHDEV_FDB_ADD_TO_BRIDGE, so please keep it
> only in those code paths, and remove it from net/dsa/slave.c as well.
> 
>>  drivers/net/dsa/b53/b53_common.c       | 5 +++++
>>  drivers/net/dsa/b53/b53_priv.h         | 1 +
>>  drivers/net/dsa/hirschmann/hellcreek.c | 5 +++++
>>  drivers/net/dsa/lan9303-core.c         | 5 +++++
>>  drivers/net/dsa/lantiq_gswip.c         | 5 +++++
>>  drivers/net/dsa/microchip/ksz9477.c    | 5 +++++
>>  drivers/net/dsa/mt7530.c               | 5 +++++
>>  drivers/net/dsa/mv88e6xxx/chip.c       | 5 +++++
>>  drivers/net/dsa/ocelot/felix.c         | 5 +++++
>>  drivers/net/dsa/qca8k.c                | 5 +++++
>>  drivers/net/dsa/sja1105/sja1105_main.c | 5 +++++
>>  include/net/dsa.h                      | 1 +
>>  net/dsa/switch.c                       | 4 ++--
>>  13 files changed, 54 insertions(+), 2 deletions(-)

Re: [PATCH v4 net-next 3/6] drivers: net: dsa: add locked fdb entry flag to drivers

[Vladimir Oltean]

On Fri, Jul 08, 2022 at 11:06:24AM +0200, netdev@kapio-technology.com wrote:
> On 2022-07-08 10:49, Vladimir Oltean wrote:
> > Hi Hans,
> > 
> > On Thu, Jul 07, 2022 at 05:29:27PM +0200, Hans Schultz wrote:
> > > Ignore locked fdb entries coming in on all drivers.
> > > 
> > > Signed-off-by: Hans Schultz <netdev@kapio-technology.com>
> > > ---
> > 
> > A good patch should have a reason for the change in the commit message.
> > This has no reason because there is no reason.
> > 
> > Think about it, you've said it yourself in patch 1:
> > 
> > | Only the kernel can set this FDB entry flag, while userspace can read
> > | the flag and remove it by replacing or deleting the FDB entry.
> > 
> > So if user space will never add locked FDB entries to the bridge,
> > then FDB entries with is_locked=true are never transported using
> > SWITCHDEV_FDB_ADD_TO_DEVICE to drivers, and so, there is no reason at
> > all to pass is_locked to drivers, just for them to ignore something that
> > won't appear.
> 
> Correct me if I am wrong, but since the bridge can add locked entries, and
> the ensuring fdb update will create a SWITCHDEV_FDB_ADD_TO_DEVICE, those
> entries
> should reach the driver. The policy to ignore those in the driver can be
> seen as either the right thing to do, or not yet implemented.
> 
> I remember Ido wrote at a point that the scheme they use is to trap various
> packets to the CPU and let the bridge add the locked entry, which I then
> understand is sent to the driver with a SWITCHDEV_FDB_ADD_TO_DEVICE event.

Well, yes, but if I'm correct, the bridge right now can't create locked
FDB entries, so is_locked will always be false in the ADD_TO_DEVICE
direction.

When the possibility for it to be true will exist, _all_ switchdev
drivers will need to be updated to ignore that (mlxsw, cpss, ocelot,
rocker, prestera, etc etc), not just DSA. And you don't need to
propagate the is_locked flag to all individual DSA sub-drivers when none
care about is_locked in the ADD_TO_DEVICE direction, you can just ignore
within DSA until needed otherwise.

> > 
> > You just need this for SWITCHDEV_FDB_ADD_TO_BRIDGE, so please keep it
> > only in those code paths, and remove it from net/dsa/slave.c as well.
> > 
> > >  drivers/net/dsa/b53/b53_common.c       | 5 +++++
> > >  drivers/net/dsa/b53/b53_priv.h         | 1 +
> > >  drivers/net/dsa/hirschmann/hellcreek.c | 5 +++++
> > >  drivers/net/dsa/lan9303-core.c         | 5 +++++
> > >  drivers/net/dsa/lantiq_gswip.c         | 5 +++++
> > >  drivers/net/dsa/microchip/ksz9477.c    | 5 +++++
> > >  drivers/net/dsa/mt7530.c               | 5 +++++
> > >  drivers/net/dsa/mv88e6xxx/chip.c       | 5 +++++
> > >  drivers/net/dsa/ocelot/felix.c         | 5 +++++
> > >  drivers/net/dsa/qca8k.c                | 5 +++++
> > >  drivers/net/dsa/sja1105/sja1105_main.c | 5 +++++
> > >  include/net/dsa.h                      | 1 +
> > >  net/dsa/switch.c                       | 4 ++--
> > >  13 files changed, 54 insertions(+), 2 deletions(-)

Re: [PATCH v4 net-next 3/6] drivers: net: dsa: add locked fdb entry flag to drivers

[netdev]

On 2022-07-08 11:15, Vladimir Oltean wrote:

> 
> Well, yes, but if I'm correct, the bridge right now can't create locked
> FDB entries, so is_locked will always be false in the ADD_TO_DEVICE
> direction.

The bridge can create locked FDB entries with this patch set. That is 
part of the idea that a SW bridge will have the same features as the HW 
version, so first I made it in the bridge.

> 
> When the possibility for it to be true will exist, _all_ switchdev
> drivers will need to be updated to ignore that (mlxsw, cpss, ocelot,
> rocker, prestera, etc etc), not just DSA. And you don't need to
> propagate the is_locked flag to all individual DSA sub-drivers when 
> none
> care about is_locked in the ADD_TO_DEVICE direction, you can just 
> ignore
> within DSA until needed otherwise.
> 

Re: [PATCH v4 net-next 5/6] net: dsa: mv88e6xxx: mac-auth/MAB implementation

[kernel test robot]

Hi Hans,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net/master]
[also build test WARNING on shuah-kselftest/next linus/master v5.19-rc5]
[cannot apply to net-next/master next-20220707]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Hans-Schultz/Extend-locked-port-feature-with-FDB-locked-flag-MAC-Auth-MAB/20220707-233246
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 07266d066301b97ad56a693f81b29b7ced429b27
config: x86_64-randconfig-a005 (https://download.01.org/0day-ci/archive/20220708/202207081751.PPV8GRpf-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 562c3467a6738aa89203f72fc1d1343e5baadf3c)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/74f76ae0b0d4b12864a0cf5ff3c0685bc420aea3
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Hans-Schultz/Extend-locked-port-feature-with-FDB-locked-flag-MAC-Auth-MAB/20220707-233246
        git checkout 74f76ae0b0d4b12864a0cf5ff3c0685bc420aea3
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/net/dsa/mv88e6xxx/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/net/dsa/mv88e6xxx/chip.c:1696:6: warning: variable 'err' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
           if (mv88e6xxx_port_is_locked(chip, port))
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/dsa/mv88e6xxx/chip.c:1698:6: note: uninitialized use occurs here
           if (err)
               ^~~
   drivers/net/dsa/mv88e6xxx/chip.c:1696:2: note: remove the 'if' if its condition is always true
           if (mv88e6xxx_port_is_locked(chip, port))
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/dsa/mv88e6xxx/chip.c:1694:9: note: initialize the variable 'err' to silence this warning
           int err;
                  ^
                   = 0
   1 warning generated.


vim +1696 drivers/net/dsa/mv88e6xxx/chip.c

  1690	
  1691	static void mv88e6xxx_port_fast_age(struct dsa_switch *ds, int port)
  1692	{
  1693		struct mv88e6xxx_chip *chip = ds->priv;
  1694		int err;
  1695	
> 1696		if (mv88e6xxx_port_is_locked(chip, port))
  1697			err = mv88e6xxx_atu_locked_entry_flush(ds, port);
  1698		if (err)
  1699			dev_err(chip->ds->dev, "p%d: failed to clear locked entries: %d\n",
  1700				port, err);
  1701	
  1702		mv88e6xxx_reg_lock(chip);
  1703		err = mv88e6xxx_port_fast_age_fid(chip, port, 0);
  1704		mv88e6xxx_reg_unlock(chip);
  1705	
  1706		if (err)
  1707			dev_err(chip->ds->dev, "p%d: failed to flush ATU: %d\n",
  1708				port, err);
  1709	}
  1710	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v4 net-next 3/6] drivers: net: dsa: add locked fdb entry flag to drivers

[netdev]

On 2022-07-08 11:15, Vladimir Oltean wrote:
> When the possibility for it to be true will exist, _all_ switchdev
> drivers will need to be updated to ignore that (mlxsw, cpss, ocelot,
> rocker, prestera, etc etc), not just DSA. And you don't need to
> propagate the is_locked flag to all individual DSA sub-drivers when 
> none
> care about is_locked in the ADD_TO_DEVICE direction, you can just 
> ignore
> within DSA until needed otherwise.
> 

Maybe I have it wrong, but I think that Ido requested me to send it to 
all the drivers, and have them ignore entries with is_locked=true ...

Re: [PATCH v4 net-next 3/6] drivers: net: dsa: add locked fdb entry flag to drivers

[Vladimir Oltean]

On Fri, Jul 08, 2022 at 11:50:33AM +0200, netdev@kapio-technology.com wrote:
> On 2022-07-08 11:15, Vladimir Oltean wrote:
> > When the possibility for it to be true will exist, _all_ switchdev
> > drivers will need to be updated to ignore that (mlxsw, cpss, ocelot,
> > rocker, prestera, etc etc), not just DSA. And you don't need to
> > propagate the is_locked flag to all individual DSA sub-drivers when none
> > care about is_locked in the ADD_TO_DEVICE direction, you can just ignore
> > within DSA until needed otherwise.
> > 
> 
> Maybe I have it wrong, but I think that Ido requested me to send it to all
> the drivers, and have them ignore entries with is_locked=true ...

I don't think Ido requested you to ignore is_locked from all DSA
drivers, but instead from all switchdev drivers maybe. Quite different.

In any case I'm going to take a look at this patch set more closely and
run the selftest on my Marvell switch, but I can't do this today
unfortunately. I'll return with more comments.

Re: [PATCH v4 net-next 3/6] drivers: net: dsa: add locked fdb entry flag to drivers

[netdev]

On 2022-07-08 13:56, Vladimir Oltean wrote:
> On Fri, Jul 08, 2022 at 11:50:33AM +0200, netdev@kapio-technology.com 
> wrote:
>> On 2022-07-08 11:15, Vladimir Oltean wrote:
>> > When the possibility for it to be true will exist, _all_ switchdev
>> > drivers will need to be updated to ignore that (mlxsw, cpss, ocelot,
>> > rocker, prestera, etc etc), not just DSA. And you don't need to
>> > propagate the is_locked flag to all individual DSA sub-drivers when none
>> > care about is_locked in the ADD_TO_DEVICE direction, you can just ignore
>> > within DSA until needed otherwise.
>> >
>> 
>> Maybe I have it wrong, but I think that Ido requested me to send it to 
>> all
>> the drivers, and have them ignore entries with is_locked=true ...
> 
> I don't think Ido requested you to ignore is_locked from all DSA
> drivers, but instead from all switchdev drivers maybe. Quite different.

So without changing the signature on port_fdb_add(). If that is to avoid 
changing that signature, which needs to be changed anyhow for any 
switchcore driver to act on it, then my next patch set will change the 
signarure also as it is needed for creating dynamic ATU entries from 
userspace, which is needed to make the whole thing complete.

As it is already done (with the is_locked to the drivers) and needed for 
future application, I would like Ido to comment on it before I take 
action.

> 
> In any case I'm going to take a look at this patch set more closely and
> run the selftest on my Marvell switch, but I can't do this today
> unfortunately. I'll return with more comments.

Yes :-)

Re: [PATCH v4 net-next 3/6] drivers: net: dsa: add locked fdb entry flag to drivers

[kernel test robot]

Hi Hans,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net/master]
[also build test ERROR on shuah-kselftest/next linus/master v5.19-rc5]
[cannot apply to net-next/master next-20220708]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Hans-Schultz/Extend-locked-port-feature-with-FDB-locked-flag-MAC-Auth-MAB/20220707-233246
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 07266d066301b97ad56a693f81b29b7ced429b27
config: i386-randconfig-a013 (https://download.01.org/0day-ci/archive/20220709/202207090438.PlGIeA4G-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 562c3467a6738aa89203f72fc1d1343e5baadf3c)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/ebd598d7ea6c015001489c4293da887763491086
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Hans-Schultz/Extend-locked-port-feature-with-FDB-locked-flag-MAC-Auth-MAB/20220707-233246
        git checkout ebd598d7ea6c015001489c4293da887763491086
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/net/dsa/sja1105/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/net/dsa/sja1105/sja1105_main.c:1952:58: error: too few arguments to function call, expected 6, have 5
           return sja1105_fdb_add(ds, port, mdb->addr, mdb->vid, db);
                  ~~~~~~~~~~~~~~~                                  ^
   drivers/net/dsa/sja1105/sja1105_main.c:1803:12: note: 'sja1105_fdb_add' declared here
   static int sja1105_fdb_add(struct dsa_switch *ds, int port,
              ^
   1 error generated.


vim +1952 drivers/net/dsa/sja1105/sja1105_main.c

5126ec72a094bd3 Vladimir Oltean 2021-08-08  1947  
a52b2da778fc93e Vladimir Oltean 2021-01-09  1948  static int sja1105_mdb_add(struct dsa_switch *ds, int port,
c26933639b5402c Vladimir Oltean 2022-02-25  1949  			   const struct switchdev_obj_port_mdb *mdb,
c26933639b5402c Vladimir Oltean 2022-02-25  1950  			   struct dsa_db db)
291d1e72b756424 Vladimir Oltean 2019-05-02  1951  {
c26933639b5402c Vladimir Oltean 2022-02-25 @1952  	return sja1105_fdb_add(ds, port, mdb->addr, mdb->vid, db);
291d1e72b756424 Vladimir Oltean 2019-05-02  1953  }
291d1e72b756424 Vladimir Oltean 2019-05-02  1954  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v4 net-next 6/6] selftests: forwarding: add test of MAC-Auth Bypass to locked port tests

[Ido Schimmel]

On Thu, Jul 07, 2022 at 05:29:30PM +0200, Hans Schultz wrote:
> +locked_port_mab()
> +{
> +	RET=0
> +	check_locked_port_support || return 0
> +
> +	ping_do $h1 192.0.2.2
> +	check_err $? "MAB: Ping did not work before locking port"
> +
> +	bridge link set dev $swp1 locked on
> +	bridge link set dev $swp1 learning on

I was under the impression that we agreed that learning does not need to
be enabled in the bridge driver

> +
> +	ping_do $h1 192.0.2.2
> +	check_fail $? "MAB: Ping worked on locked port without FDB entry"
> +
> +	bridge fdb show | grep `mac_get $h1` | grep -q "locked"
> +	check_err $? "MAB: No locked fdb entry after ping on locked port"
> +
> +	bridge fdb replace `mac_get $h1` dev $swp1 master static
> +
> +	ping_do $h1 192.0.2.2
> +	check_err $? "MAB: Ping did not work with fdb entry without locked flag"
> +
> +	bridge fdb del `mac_get $h1` dev $swp1 master
> +	bridge link set dev $swp1 learning off
> +	bridge link set dev $swp1 locked off
> +
> +	log_test "Locked port MAB"
> +}
>  trap cleanup EXIT
>  
>  setup_prepare
> -- 
> 2.30.2
> 

Re: [PATCH v4 net-next 1/6] net: bridge: add locked entry fdb flag to extend locked port feature

[Ido Schimmel]

On Thu, Jul 07, 2022 at 05:29:25PM +0200, Hans Schultz wrote:
> Add an intermediate state for clients behind a locked port to allow for
> possible opening of the port for said clients. The clients mac address
> will be added with the locked flag set, denying access through the port
> for the mac address, but also creating a new FDB add event giving
> userspace daemons the ability to unlock the mac address. This feature
> corresponds to the Mac-Auth and MAC Authentication Bypass (MAB) named
> features. The latter defined by Cisco.
> 
> Only the kernel can set this FDB entry flag, while userspace can read
> the flag and remove it by replacing or deleting the FDB entry.
> 
> Signed-off-by: Hans Schultz <netdev@kapio-technology.com>
> ---
>  include/uapi/linux/neighbour.h |  1 +
>  net/bridge/br_fdb.c            | 12 ++++++++++++
>  net/bridge/br_input.c          | 10 +++++++++-
>  net/bridge/br_private.h        |  3 ++-
>  4 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/include/uapi/linux/neighbour.h b/include/uapi/linux/neighbour.h
> index 39c565e460c7..76d65b481086 100644
> --- a/include/uapi/linux/neighbour.h
> +++ b/include/uapi/linux/neighbour.h
> @@ -53,6 +53,7 @@ enum {
>  #define NTF_ROUTER	(1 << 7)
>  /* Extended flags under NDA_FLAGS_EXT: */
>  #define NTF_EXT_MANAGED	(1 << 0)
> +#define NTF_EXT_LOCKED	(1 << 1)
>  
>  /*
>   *	Neighbor Cache Entry States.
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index e7f4fccb6adb..ee9064a536ae 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_ENTRY_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 */

Need to add validation that 'NTF_EXT_LOCKED' is not set in
'NDA_FLAGS_EXT' in entries installed by user space.

>  		+ nla_total_size(sizeof(u16)) /* NDA_VLAN */
>  		+ nla_total_size(sizeof(struct nda_cacheinfo))
>  		+ nla_total_size(0) /* NDA_FDB_EXT_ATTRS */
> @@ -1082,6 +1089,11 @@ static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source,
>  		modified = true;
>  	}
>  
> +	if (test_bit(BR_FDB_ENTRY_LOCKED, &fdb->flags)) {
> +		clear_bit(BR_FDB_ENTRY_LOCKED, &fdb->flags);
> +		modified = true;
> +	}
> +
>  	if (fdb_handle_notify(fdb, notify))
>  		modified = true;
>  
> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> index 68b3e850bcb9..3d15548cfda6 100644
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -110,8 +110,16 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>  			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))
> +		    test_bit(BR_FDB_LOCAL, &fdb_src->flags) ||
> +		    test_bit(BR_FDB_ENTRY_LOCKED, &fdb_src->flags)) {
> +			if (!fdb_src) {
> +				unsigned long flags = 0;
> +
> +				__set_bit(BR_FDB_ENTRY_LOCKED, &flags);
> +				br_fdb_update(br, p, eth_hdr(skb)->h_source, vid, flags);
> +			}

We need a better definition of what 'BR_FDB_ENTRY_LOCKED' means. An
entry marked with this flag is not good enough to let traffic with this
SA ingress from a locked port, but if traffic is received from a
different port with this DA, the bridge will forward it as known
unicast.

If we are going to tell switchdev drivers to ignore locked entries,
packets with this DA will be flooded as unknown unicast in hardware. For
mv88e6xxx you write "The mac address triggering the ATU miss violation
will be added to the ATU with a zero-DPV". What does it mean? Traffic
with this DA will be blackholed?

It would be good to agree on one consistent behavior for all hardware
implementations and the bridge driver. Do you know what happens in other
implementations (e.g., Cisco)?

FWIW, to me it makes sense to have the bridge ignore locked entries and
let traffic be flooded. If user space does not want traffic to egress a
locked port, then it can turn off flooding on the port while it is
locked.

>  			goto drop;
> +		}
>  	}
>  
>  	nbp_switchdev_frame_mark(p, skb);
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 06e5f6faa431..47a3598d25c8 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_ENTRY_LOCKED,
>  };
>  
>  struct net_bridge_fdb_key {
> -- 
> 2.30.2
> 

Re: [PATCH v4 net-next 3/6] drivers: net: dsa: add locked fdb entry flag to drivers

[Ido Schimmel]

On Fri, Jul 08, 2022 at 02:34:25PM +0200, netdev@kapio-technology.com wrote:
> On 2022-07-08 13:56, Vladimir Oltean wrote:
> > On Fri, Jul 08, 2022 at 11:50:33AM +0200, netdev@kapio-technology.com
> > wrote:
> > > On 2022-07-08 11:15, Vladimir Oltean wrote:
> > > > When the possibility for it to be true will exist, _all_ switchdev
> > > > drivers will need to be updated to ignore that (mlxsw, cpss, ocelot,
> > > > rocker, prestera, etc etc), not just DSA. And you don't need to
> > > > propagate the is_locked flag to all individual DSA sub-drivers when none
> > > > care about is_locked in the ADD_TO_DEVICE direction, you can just ignore
> > > > within DSA until needed otherwise.
> > > >
> > > 
> > > Maybe I have it wrong, but I think that Ido requested me to send it
> > > to all
> > > the drivers, and have them ignore entries with is_locked=true ...
> > 
> > I don't think Ido requested you to ignore is_locked from all DSA
> > drivers, but instead from all switchdev drivers maybe. Quite different.
> 
> So without changing the signature on port_fdb_add(). If that is to avoid
> changing that signature, which needs to be changed anyhow for any switchcore
> driver to act on it, then my next patch set will change the signarure also
> as it is needed for creating dynamic ATU entries from userspace, which is
> needed to make the whole thing complete.
> 
> As it is already done (with the is_locked to the drivers) and needed for
> future application, I would like Ido to comment on it before I take action.

It's related to my reply here [1]. AFAICT, we have two classes of device
drivers:

1. Drivers like mv88e6xxx that report locked entries to the bridge
driver via 'SWITCHDEV_FDB_ADD_TO_BRIDGE'.

2. Drivers like mlxsw that trap packets that incurred an FDB miss to the
bridge driver. These packets will cause the bridge driver to emit
'SWITCHDEV_FDB_ADD_TO_DEVICE' notifications with the locked flag.

If we can agree that locked entries are only meant to signal to user
space that a certain MAC tried to gain authorization and that the bridge
should ignore them while forwarding, then there is no point in
generating the 'SWITCHDEV_FDB_ADD_TO_DEVICE' notifications. We should
teach the bridge driver to suppress these so that there is no need to
patch all the device drivers.

[1] https://lore.kernel.org/netdev/YsqLyxTRtUjzDj6D@shredder/

> 
> > 
> > In any case I'm going to take a look at this patch set more closely and
> > run the selftest on my Marvell switch, but I can't do this today
> > unfortunately. I'll return with more comments.
> 
> Yes :-)

Re: [PATCH v4 net-next 6/6] selftests: forwarding: add test of MAC-Auth Bypass to locked port tests

[netdev]

On 2022-07-10 09:29, Ido Schimmel wrote:
> On Thu, Jul 07, 2022 at 05:29:30PM +0200, Hans Schultz wrote:
>> +locked_port_mab()
>> +{
>> +	RET=0
>> +	check_locked_port_support || return 0
>> +
>> +	ping_do $h1 192.0.2.2
>> +	check_err $? "MAB: Ping did not work before locking port"
>> +
>> +	bridge link set dev $swp1 locked on
>> +	bridge link set dev $swp1 learning on
> 
> I was under the impression that we agreed that learning does not need 
> to
> be enabled in the bridge driver
> 

Sorry, you are right. I forgot to change it here.

>> +
>> +	ping_do $h1 192.0.2.2
>> +	check_fail $? "MAB: Ping worked on locked port without FDB entry"
>> +
>> +	bridge fdb show | grep `mac_get $h1` | grep -q "locked"
>> +	check_err $? "MAB: No locked fdb entry after ping on locked port"
>> +
>> +	bridge fdb replace `mac_get $h1` dev $swp1 master static
>> +
>> +	ping_do $h1 192.0.2.2
>> +	check_err $? "MAB: Ping did not work with fdb entry without locked 
>> flag"
>> +
>> +	bridge fdb del `mac_get $h1` dev $swp1 master
>> +	bridge link set dev $swp1 learning off
>> +	bridge link set dev $swp1 locked off
>> +
>> +	log_test "Locked port MAB"
>> +}
>>  trap cleanup EXIT
>> 
>>  setup_prepare
>> --
>> 2.30.2
>> 

Re: [PATCH v4 net-next 3/6] drivers: net: dsa: add locked fdb entry flag to drivers

[netdev]

On 2022-07-10 10:35, Ido Schimmel wrote:
> On Fri, Jul 08, 2022 at 02:34:25PM +0200, netdev@kapio-technology.com 
> wrote:
>> On 2022-07-08 13:56, Vladimir Oltean wrote:
>> > On Fri, Jul 08, 2022 at 11:50:33AM +0200, netdev@kapio-technology.com
>> > wrote:
>> > > On 2022-07-08 11:15, Vladimir Oltean wrote:
>> > > > When the possibility for it to be true will exist, _all_ switchdev
>> > > > drivers will need to be updated to ignore that (mlxsw, cpss, ocelot,
>> > > > rocker, prestera, etc etc), not just DSA. And you don't need to
>> > > > propagate the is_locked flag to all individual DSA sub-drivers when none
>> > > > care about is_locked in the ADD_TO_DEVICE direction, you can just ignore
>> > > > within DSA until needed otherwise.
>> > > >
>> > >
>> > > Maybe I have it wrong, but I think that Ido requested me to send it
>> > > to all
>> > > the drivers, and have them ignore entries with is_locked=true ...
>> >
>> > I don't think Ido requested you to ignore is_locked from all DSA
>> > drivers, but instead from all switchdev drivers maybe. Quite different.
>> 
>> So without changing the signature on port_fdb_add(). If that is to 
>> avoid
>> changing that signature, which needs to be changed anyhow for any 
>> switchcore
>> driver to act on it, then my next patch set will change the signarure 
>> also
>> as it is needed for creating dynamic ATU entries from userspace, which 
>> is
>> needed to make the whole thing complete.
>> 
>> As it is already done (with the is_locked to the drivers) and needed 
>> for
>> future application, I would like Ido to comment on it before I take 
>> action.
> 
> It's related to my reply here [1]. AFAICT, we have two classes of 
> device
> drivers:
> 
> 1. Drivers like mv88e6xxx that report locked entries to the bridge
> driver via 'SWITCHDEV_FDB_ADD_TO_BRIDGE'.
> 
> 2. Drivers like mlxsw that trap packets that incurred an FDB miss to 
> the
> bridge driver. These packets will cause the bridge driver to emit
> 'SWITCHDEV_FDB_ADD_TO_DEVICE' notifications with the locked flag.
> 
> If we can agree that locked entries are only meant to signal to user
> space that a certain MAC tried to gain authorization and that the 
> bridge
> should ignore them while forwarding, then there is no point in
> generating the 'SWITCHDEV_FDB_ADD_TO_DEVICE' notifications. We should
> teach the bridge driver to suppress these so that there is no need to
> patch all the device drivers.

I do not know of all about what other switchcores there are and how they 
work, but those that I have knowledge of, it has been prudent in 
connection with the locked port feature to install Storm Prevention or 
zero-DPV (Destination Port Vector) FDB entries. I would think that that 
should be the case for other switchcores too.
Those entries cannot normally be installed from userspace (of course 
special tools can do anything).

But if the decision is to drop locked entries at the DSA layer, I can do 
that. I just want to ensure that all considerations have been taken.

> 
> [1] https://lore.kernel.org/netdev/YsqLyxTRtUjzDj6D@shredder/
> 
>> 
>> >
>> > In any case I'm going to take a look at this patch set more closely and
>> > run the selftest on my Marvell switch, but I can't do this today
>> > unfortunately. I'll return with more comments.
>> 
>> Yes :-)

Re: [PATCH v4 net-next 3/6] drivers: net: dsa: add locked fdb entry flag to drivers

[Ido Schimmel]

On Wed, Jul 13, 2022 at 09:09:58AM +0200, netdev@kapio-technology.com wrote:
> On 2022-07-10 10:35, Ido Schimmel wrote:
> > On Fri, Jul 08, 2022 at 02:34:25PM +0200, netdev@kapio-technology.com
> > wrote:
> > > On 2022-07-08 13:56, Vladimir Oltean wrote:
> > > > On Fri, Jul 08, 2022 at 11:50:33AM +0200, netdev@kapio-technology.com
> > > > wrote:
> > > > > On 2022-07-08 11:15, Vladimir Oltean wrote:
> > > > > > When the possibility for it to be true will exist, _all_ switchdev
> > > > > > drivers will need to be updated to ignore that (mlxsw, cpss, ocelot,
> > > > > > rocker, prestera, etc etc), not just DSA. And you don't need to
> > > > > > propagate the is_locked flag to all individual DSA sub-drivers when none
> > > > > > care about is_locked in the ADD_TO_DEVICE direction, you can just ignore
> > > > > > within DSA until needed otherwise.
> > > > > >
> > > > >
> > > > > Maybe I have it wrong, but I think that Ido requested me to send it
> > > > > to all
> > > > > the drivers, and have them ignore entries with is_locked=true ...
> > > >
> > > > I don't think Ido requested you to ignore is_locked from all DSA
> > > > drivers, but instead from all switchdev drivers maybe. Quite different.
> > > 
> > > So without changing the signature on port_fdb_add(). If that is to
> > > avoid
> > > changing that signature, which needs to be changed anyhow for any
> > > switchcore
> > > driver to act on it, then my next patch set will change the
> > > signarure also
> > > as it is needed for creating dynamic ATU entries from userspace,
> > > which is
> > > needed to make the whole thing complete.
> > > 
> > > As it is already done (with the is_locked to the drivers) and needed
> > > for
> > > future application, I would like Ido to comment on it before I take
> > > action.
> > 
> > It's related to my reply here [1]. AFAICT, we have two classes of device
> > drivers:
> > 
> > 1. Drivers like mv88e6xxx that report locked entries to the bridge
> > driver via 'SWITCHDEV_FDB_ADD_TO_BRIDGE'.
> > 
> > 2. Drivers like mlxsw that trap packets that incurred an FDB miss to the
> > bridge driver. These packets will cause the bridge driver to emit
> > 'SWITCHDEV_FDB_ADD_TO_DEVICE' notifications with the locked flag.
> > 
> > If we can agree that locked entries are only meant to signal to user
> > space that a certain MAC tried to gain authorization and that the bridge
> > should ignore them while forwarding, then there is no point in
> > generating the 'SWITCHDEV_FDB_ADD_TO_DEVICE' notifications. We should
> > teach the bridge driver to suppress these so that there is no need to
> > patch all the device drivers.
> 
> I do not know of all about what other switchcores there are and how they
> work, but those that I have knowledge of, it has been prudent in connection
> with the locked port feature to install Storm Prevention or zero-DPV
> (Destination Port Vector) FDB entries.

What are "Storm Prevention" and "zero-DPV" FDB entries?

> I would think that that should be the case for other switchcores too.
> Those entries cannot normally be installed from userspace (of course special
> tools can do anything).
> 
> But if the decision is to drop locked entries at the DSA layer, I can do
> that. I just want to ensure that all considerations have been taken.

There is no decision that I'm aware of. I'm simply trying to understand
how FDB entries that have 'BR_FDB_ENTRY_LOCKED' set are handled in
mv88e6xxx and other devices in this class. We have at least three
different implementations to consolidate:

1. The bridge driver, pure software forwarding. The locked entry is
dynamically created by the bridge. Packets received via the locked port
with a SA corresponding to the locked entry will be dropped, but will
refresh the entry. On the other hand, packets with a DA corresponding to
the locked entry will be forwarded as known unicast through the locked
port.

2. Hardware implementations like Spectrum that can be programmed to trap
packets that incurred an FDB miss. Like in the first case, the locked
entry is dynamically created by the bridge driver and also aged by it.
Unlike in the first case, since this entry is not present in hardware,
packets with a DA corresponding to the locked entry will be flooded as
unknown unicast.

3. Hardware implementations like mv88e6xxx that fire an interrupt upon
FDB miss. Need your help to understand how the above works there and
why. Specifically, how locked entries are represented in hardware (if at
all) and what is the significance of not installing corresponding
entries in hardware.

> 
> > 
> > [1] https://lore.kernel.org/netdev/YsqLyxTRtUjzDj6D@shredder/
> > 
> > > 
> > > >
> > > > In any case I'm going to take a look at this patch set more closely and
> > > > run the selftest on my Marvell switch, but I can't do this today
> > > > unfortunately. I'll return with more comments.
> > > 
> > > Yes :-)

Re: [PATCH v4 net-next 5/6] net: dsa: mv88e6xxx: mac-auth/MAB implementation

[Vladimir Oltean]

On Thu, Jul 07, 2022 at 05:29:29PM +0200, Hans Schultz wrote:
> This implementation for the Marvell mv88e6xxx chip series,
> is based on handling ATU miss violations occurring when packets
> ingress on a port that is locked. The mac address triggering
> the ATU miss violation will be added to the ATU with a zero-DPV,
> and is then communicated through switchdev to the bridge module,
> which adds a fdb entry with the fdb locked flag set. The entry
> is kept according to the bridges ageing time, thus simulating a
> dynamic entry.
> 
> As this is essentially a form of CPU based learning, the amount
> of locked entries will be limited by a hardcoded value for now,
> so as to prevent DOS attacks.
> 
> Signed-off-by: Hans Schultz <netdev@kapio-technology.com>
> ---
>  static void assert_reg_lock(struct mv88e6xxx_chip *chip)
>  {
> @@ -919,6 +920,13 @@ static void mv88e6xxx_mac_link_down(struct dsa_switch *ds, int port,
>  	if (err)
>  		dev_err(chip->dev,
>  			"p%d: failed to force MAC link down\n", port);
> +	else
> +		if (mv88e6xxx_port_is_locked(chip, port)) {
> +			err = mv88e6xxx_atu_locked_entry_flush(ds, port);
> +			if (err)
> +				dev_err(chip->dev,
> +					"p%d: failed to clear locked entries\n", port);
> +		}
>  }
>  
>  static void mv88e6xxx_mac_link_up(struct dsa_switch *ds, int port,
> @@ -1685,6 +1693,12 @@ static void mv88e6xxx_port_fast_age(struct dsa_switch *ds, int port)
>  	struct mv88e6xxx_chip *chip = ds->priv;
>  	int err;
>  
> +	if (mv88e6xxx_port_is_locked(chip, port))
> +		err = mv88e6xxx_atu_locked_entry_flush(ds, port);
> +	if (err)

Kernel build robot pointed this out too, but it's worth repeating to
make sure it doesn't get missed. If the port isn't locked, this function
returns a junk integer from the stack which isn't zero, so it's treated
as an error code.

> +		dev_err(chip->ds->dev, "p%d: failed to clear locked entries: %d\n",
> +			port, err);
> +
>  	mv88e6xxx_reg_lock(chip);
>  	err = mv88e6xxx_port_fast_age_fid(chip, port, 0);
>  	mv88e6xxx_reg_unlock(chip);
> @@ -1721,11 +1735,11 @@ static int mv88e6xxx_vtu_get(struct mv88e6xxx_chip *chip, u16 vid,
>  	return err;
>  }
>  
>  int mv88e6xxx_port_set_lock(struct mv88e6xxx_chip *chip, int port,
>  			    bool locked)
>  {
> @@ -1257,13 +1270,18 @@ int mv88e6xxx_port_set_lock(struct mv88e6xxx_chip *chip, int port,
>  	if (err)
>  		return err;
>  
> -	err = mv88e6xxx_port_read(chip, port, MV88E6XXX_PORT_ASSOC_VECTOR, &reg);
> -	if (err)
> -		return err;
> +	if (!locked) {
> +		err = mv88e6xxx_atu_locked_entry_flush(chip->ds, port);

Did you re-run the selftest with v4? Because this deadlocks due to the
double reg_lock illustrated below:

+ vrf_name=vlan1
+ ip vrf exec vlan1 ping 192.0.2.2 -c 10 -i 0.1 -w 5
+ check_err 1 'Ping did not work after locking port and adding FDB entry'
+ local err=1
+ local 'msg=Ping did not work after locking port and adding FDB entry'
+ [[ 0 -eq 0 ]]
+ [[ 1 -ne 0 ]]
+ RET=1
+ retmsg='Ping did not work after locking port and adding FDB entry'
+ bridge link set dev lan2 locked off
[  733.273994]
[  733.275515] ============================================
[  733.280823] WARNING: possible recursive locking detected
[  733.286133] 5.19.0-rc6-07010-ga9b9500ffaac-dirty #3293 Not tainted
[  733.292311] --------------------------------------------
[  733.297613] kworker/0:0/601 is trying to acquire lock:
[  733.302751] ffff00000213c110 (&chip->reg_lock){+.+.}-{4:4}, at: mv88e6xxx_atu_locked_entry_purge+0x70/0x1a4
[  733.312524]
[  733.312524] but task is already holding lock:
[  733.318351] ffff00000213c110 (&chip->reg_lock){+.+.}-{4:4}, at: mv88e6xxx_port_bridge_flags+0x48/0x234
[  733.327674]
[  733.327674] other info that might help us debug this:
[  733.334198]  Possible unsafe locking scenario:
[  733.334198]
[  733.340109]        CPU0
[  733.342549]        ----
[  733.344990]   lock(&chip->reg_lock);
[  733.348567]   lock(&chip->reg_lock);
[  733.352149]
[  733.352149]  *** DEADLOCK ***
[  733.352149]
[  733.358063]  May be due to missing lock nesting notation
[  733.358063]
[  733.364846] 6 locks held by kworker/0:0/601:
[  733.369115]  #0: ffff00000000b748 ((wq_completion)events){+.+.}-{0:0}, at: process_one_work+0x1f4/0x6c4
[  733.378541]  #1: ffff80000c43bdc8 (deferred_process_work){+.+.}-{0:0}, at: process_one_work+0x1f4/0x6c4
[  733.387966]  #2: ffff80000b020cb0 (rtnl_mutex){+.+.}-{4:4}, at: rtnl_lock+0x1c/0x30
[  733.395660]  #3: ffff80000b073370 ((switchdev_blocking_notif_chain).rwsem){++++}-{4:4}, at: blocking_notifier_call_chain+0x34/0xa0
[  733.407432]  #4: ffff00000213c110 (&chip->reg_lock){+.+.}-{4:4}, at: mv88e6xxx_port_bridge_flags+0x48/0x234
[  733.417202]  #5: ffff00000213e0f0 (&p->ale_list_lock){+.+.}-{4:4}, at: mv88e6xxx_atu_locked_entry_flush+0x4c/0xc0
[  733.427495]
[  733.427495] stack backtrace:
[  733.431858] CPU: 0 PID: 601 Comm: kworker/0:0 Not tainted 5.19.0-rc6-07010-ga9b9500ffaac-dirty #3293
[  733.440992] Hardware name: CZ.NIC Turris Mox Board (DT)
[  733.446219] Workqueue: events switchdev_deferred_process_work
[  733.451982] Call trace:
[  733.454424]  dump_backtrace.part.0+0xcc/0xe0
[  733.458702]  show_stack+0x18/0x6c
[  733.462028]  dump_stack_lvl+0x8c/0xb8
[  733.465703]  dump_stack+0x18/0x34
[  733.469029]  __lock_acquire+0x1074/0x1fd0
[  733.473052]  lock_acquire.part.0+0xe4/0x220
[  733.477244]  lock_acquire+0x68/0x8c
[  733.480744]  __mutex_lock+0x9c/0x460
[  733.484332]  mutex_lock_nested+0x40/0x50
[  733.488268]  mv88e6xxx_atu_locked_entry_purge+0x70/0x1a4
[  733.493592]  mv88e6xxx_atu_locked_entry_flush+0x7c/0xc0
[  733.498827]  mv88e6xxx_port_set_lock+0xfc/0x10c
[  733.503374]  mv88e6xxx_port_bridge_flags+0x200/0x234
[  733.508351]  dsa_port_bridge_flags+0x44/0xe0
[  733.512635]  dsa_slave_port_attr_set+0x1ec/0x230
[  733.517268]  __switchdev_handle_port_attr_set+0x58/0x100
[  733.522594]  switchdev_handle_port_attr_set+0x10/0x24
[  733.527659]  dsa_slave_switchdev_blocking_event+0x8c/0xd4
[  733.533074]  blocking_notifier_call_chain+0x6c/0xa0
[  733.537968]  switchdev_port_attr_notify.constprop.0+0x4c/0xb0
[  733.543729]  switchdev_port_attr_set_deferred+0x24/0x80
[  733.548967]  switchdev_deferred_process+0x90/0x164
[  733.553774]  switchdev_deferred_process_work+0x14/0x2c
[  733.558926]  process_one_work+0x28c/0x6c4
[  733.562949]  worker_thread+0x74/0x450
[  733.566623]  kthread+0x118/0x11c
[  733.569860]  ret_from_fork+0x10/0x20
++ mac_get lan1
++ local if_name=lan1
++ jq -r '.[]["address"]'
++ ip -j link show dev lan1

I've tentatively fixed this in my tree by taking the register lock more
localized to the sub-functions of mv88e6xxx_port_bridge_flags(), and not
taking it at caller side for mv88e6xxx_port_set_lock(), but rather
letting the callee take it:

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 7b57ac121589..ec5954f32774 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -6557,41 +6557,47 @@ static int mv88e6xxx_port_bridge_flags(struct dsa_switch *ds, int port,
 	struct mv88e6xxx_chip *chip = ds->priv;
 	int err = -EOPNOTSUPP;
 
-	mv88e6xxx_reg_lock(chip);
-
 	if (flags.mask & BR_LEARNING) {
 		bool learning = !!(flags.val & BR_LEARNING);
 		u16 pav = learning ? (1 << port) : 0;
 
+		mv88e6xxx_reg_lock(chip);
 		err = mv88e6xxx_port_set_assoc_vector(chip, port, pav);
+		mv88e6xxx_reg_unlock(chip);
 		if (err)
-			goto out;
+			return err;
 	}
 
 	if (flags.mask & BR_FLOOD) {
 		bool unicast = !!(flags.val & BR_FLOOD);
 
+		mv88e6xxx_reg_lock(chip);
 		err = chip->info->ops->port_set_ucast_flood(chip, port,
 							    unicast);
+		mv88e6xxx_reg_unlock(chip);
 		if (err)
-			goto out;
+			return err;
 	}
 
 	if (flags.mask & BR_MCAST_FLOOD) {
 		bool multicast = !!(flags.val & BR_MCAST_FLOOD);
 
+		mv88e6xxx_reg_lock(chip);
 		err = chip->info->ops->port_set_mcast_flood(chip, port,
 							    multicast);
+		mv88e6xxx_reg_unlock(chip);
 		if (err)
-			goto out;
+			return err;
 	}
 
 	if (flags.mask & BR_BCAST_FLOOD) {
 		bool broadcast = !!(flags.val & BR_BCAST_FLOOD);
 
+		mv88e6xxx_reg_lock(chip);
 		err = mv88e6xxx_port_broadcast_sync(chip, port, broadcast);
+		mv88e6xxx_reg_unlock(chip);
 		if (err)
-			goto out;
+			return err;
 	}
 
 	if (flags.mask & BR_PORT_LOCKED) {
@@ -6599,10 +6605,8 @@ static int mv88e6xxx_port_bridge_flags(struct dsa_switch *ds, int port,
 
 		err = mv88e6xxx_port_set_lock(chip, port, locked);
 		if (err)
-			goto out;
+			return err;
 	}
-out:
-	mv88e6xxx_reg_unlock(chip);
 
 	return err;
 }
diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
index 5e4d5e9265a4..2a60aded6fbe 100644
--- a/drivers/net/dsa/mv88e6xxx/port.c
+++ b/drivers/net/dsa/mv88e6xxx/port.c
@@ -1222,15 +1222,19 @@ int mv88e6xxx_port_set_lock(struct mv88e6xxx_chip *chip, int port,
 	u16 reg;
 	int err;
 
+	mv88e6xxx_reg_lock(chip);
 	err = mv88e6xxx_port_read(chip, port, MV88E6XXX_PORT_CTL0, &reg);
-	if (err)
+	if (err) {
+		mv88e6xxx_reg_unlock(chip);
 		return err;
+	}
 
 	reg &= ~MV88E6XXX_PORT_CTL0_SA_FILT_MASK;
 	if (locked)
 		reg |= MV88E6XXX_PORT_CTL0_SA_FILT_DROP_ON_LOCK;
 
 	err = mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_CTL0, reg);
+	mv88e6xxx_reg_unlock(chip);
 	if (err)
 		return err;
 
@@ -1247,7 +1251,11 @@ int mv88e6xxx_port_set_lock(struct mv88e6xxx_chip *chip, int port,
 			MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT;
 	}
 
-	return mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_ASSOC_VECTOR, reg);
+	mv88e6xxx_reg_lock(chip);
+	err = mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_ASSOC_VECTOR, reg);
+	mv88e6xxx_reg_unlock(chip);
+
+	return err;
 }
 
 int mv88e6xxx_port_set_8021q_mode(struct mv88e6xxx_chip *chip, int port,

> +		if (err)
> +			return err;
> +	}
>  
> -	reg &= ~MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT;
> -	if (locked)
> -		reg |= MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT;
> +	reg = 0;
> +	if (locked) {
> +		reg = (1 << port);
> +		reg |= MV88E6XXX_PORT_ASSOC_VECTOR_IGNORE_WRONG |
> +			MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT;
> +	}
>  
>  	return mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_ASSOC_VECTOR, reg);
>  }
> diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h
> index e0a705d82019..5c1d485e7442 100644
> --- a/drivers/net/dsa/mv88e6xxx/port.h
> +++ b/drivers/net/dsa/mv88e6xxx/port.h
> @@ -231,6 +231,7 @@
>  #define MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT		0x2000
>  #define MV88E6XXX_PORT_ASSOC_VECTOR_IGNORE_WRONG	0x1000
>  #define MV88E6XXX_PORT_ASSOC_VECTOR_REFRESH_LOCKED	0x0800
> +#define MV88E6XXX_PORT_ASSOC_VECTOR_PAV_MASK		0x07ff
>  
>  /* Offset 0x0C: Port ATU Control */
>  #define MV88E6XXX_PORT_ATU_CTL		0x0c
> @@ -374,6 +375,7 @@ int mv88e6xxx_port_set_fid(struct mv88e6xxx_chip *chip, int port, u16 fid);
>  int mv88e6xxx_port_get_pvid(struct mv88e6xxx_chip *chip, int port, u16 *pvid);
>  int mv88e6xxx_port_set_pvid(struct mv88e6xxx_chip *chip, int port, u16 pvid);
>  
> +bool mv88e6xxx_port_is_locked(struct mv88e6xxx_chip *chip, int port);
>  int mv88e6xxx_port_set_lock(struct mv88e6xxx_chip *chip, int port,
>  			    bool locked);
>  

Other than that, after going through some hoops and patching iproute2,
the switch appears to do something, I do see locked FDB entries:

[root@mox:~/.../drivers/net/dsa] # bridge fdb show dev lan2
00:01:02:03:04:01 vlan 1 locked extern_learn offload master br0 
00:01:02:03:04:02 vlan 1 master br0 permanent
00:01:02:03:04:02 master br0 permanent

however all selftests except for MAB appear to fail:

[root@mox:~/.../drivers/net/dsa] # ./bridge_locked_port.sh lan1 lan2 lan3 lan4
[ 2394.084584] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
[ 2398.984189] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
RTNETLINK answers: File exists
TEST: Locked port ipv4                                              [FAIL]
        Ping did not work after locking port and adding FDB entry
[ 2410.452638] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
[ 2415.366824] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
RTNETLINK answers: File exists
TEST: Locked port ipv6                                              [FAIL]
        Ping6 did not work after locking port and adding FDB entry
[ 2425.653013] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 2425.663784] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 2425.752771] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 2425.853188] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 2425.954193] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 2426.054593] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 2426.155646] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 2426.256733] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 2426.357159] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 2426.457638] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 2427.354018] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
[ 2430.679471] mv88e6xxx_g1_vtu_prob_irq_thread_fn: 33 callbacks suppressed
[ 2430.679502] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 2430.723742] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 2430.783752] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 2430.887742] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 2430.991738] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 2431.095714] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 2431.199505] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 2431.303700] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 2431.407707] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 2431.511674] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
RTNETLINK answers: File exists
[ 2435.683201] mv88e6xxx_g1_vtu_prob_irq_thread_fn: 28 callbacks suppressed
[ 2435.683232] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 2435.787060] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 2435.891223] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 2435.995249] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 2436.099218] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 2436.203215] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 2436.307207] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 2436.411175] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 2436.515179] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 2436.619220] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 2440.712164] mv88e6xxx_g1_vtu_prob_irq_thread_fn: 26 callbacks suppressed
[ 2440.712194] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 2440.812777] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 2440.913630] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 2441.014523] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 2441.114196] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 2441.214961] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
TEST: Locked port vlan                                              [FAIL]
        Ping through vlan did not work after locking port and adding FDB entry
[ 2443.945008] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
[ 2448.131708] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
TEST: Locked port MAB                                               [ OK ]
[ 2455.139841] mv88e6085 d0032004.mdio-mii:10 lan3: Link is Down
[ 2455.158623] br0: port 2(lan3) entered disabled state
[ 2455.288477] mv88e6085 d0032004.mdio-mii:10 lan2: Link is Down
[ 2455.309996] br0: port 1(lan2) entered disabled state
[ 2455.446341] device lan3 left promiscuous mode
[ 2455.452208] br0: port 2(lan3) entered disabled state
[ 2455.574232] device lan2 left promiscuous mode
[ 2455.580378] br0: port 1(lan2) entered disabled state
[ 2455.906058] mv88e6085 d0032004.mdio-mii:10 lan4: Link is Down
[ 2456.069399] mv88e6085 d0032004.mdio-mii:10 lan1: Link is Down

Compare this to the same selftest, run just before applying the
mv88e6xxx MAB patch:

TEST: Locked port ipv4                                              [ OK ]
TEST: Locked port ipv6                                              [ OK ]
[  119.080114] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[  119.091009] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[  119.180557] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[  119.280916] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[  119.381256] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[  119.481618] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[  119.581993] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[  119.682416] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[  119.782799] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[  119.883177] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[  124.100262] mv88e6xxx_g1_vtu_prob_irq_thread_fn: 33 callbacks suppressed
[  124.100293] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[  124.136182] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[  124.204408] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[  124.312440] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[  124.416384] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[  124.520361] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[  124.624347] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[  124.728344] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[  124.832336] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[  124.936341] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[  130.014610] mv88e6xxx_g1_vtu_prob_irq_thread_fn: 20 callbacks suppressed
[  130.014637] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[  130.118292] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[  130.219203] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[  130.324289] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[  130.335695] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[  130.424680] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[  130.525600] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[  130.626466] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[  130.728275] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[  130.829422] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
TEST: Locked port vlan                                              [ OK ]
[  133.926477] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
[  134.028380] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
[  134.132571] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
[  134.244056] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
[  134.344703] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
[  134.448416] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
[  134.552692] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
[  134.656642] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
[  134.760786] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
[  134.864605] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
[  139.247209] mv88e6xxx_g1_atu_prob_irq_thread_fn: 41 callbacks suppressed
[  139.247241] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
[  144.358773] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
TEST: Locked port MAB                                               [FAIL]
        MAB: No locked fdb entry after ping on locked port

In other words, this patch set makes MAB work and breaks everything else.
I'm willing to investigate exactly what is it that breaks the other
selftest, but not today. It may be related to the "RTNETLINK answers: File exists"
messages, which themselves come from the commands
| bridge fdb add 00:01:02:03:04:01 dev lan2 master static

If I were to randomly guess at almost 4AM in the morning, it has to do with
"bridge fdb add" rather than the "bridge fdb replace" that's used for
the MAB selftest. The fact I pointed out a few revisions ago, that MAB
needs to be opt-in, is now coming back to bite us. Since it's not
opt-in, the mv88e6xxx driver always creates locked FDB entries, and when
we try to "bridge fdb add", the kernel says "hey, the FDB entry is
already there!". Is that it?

As for how to opt into MAB. Hmm. MAB seems to be essentially CPU
assisted learning, which creates locked FDB entries. I wonder whether we
should reconsider the position that address learning makes no sense on
locked ports, and say that "+locked -learning" means no MAB, and
"+locked +learning" means MAB? This would make a bunch of things more
natural to handle in the kernel, and would also give us the opt-in we need.



Side note, the VTU and ATU member violation printks annoy me so badly.
They aren't stating something super useful and they're a DoS attack
vector in itself, even if they're rate limited. I wonder whether we
could just turn the prints into a set of ethtool counters and call it a day?

Re: [PATCH v4 net-next 3/6] drivers: net: dsa: add locked fdb entry flag to drivers

[netdev]

On 2022-07-13 14:39, Ido Schimmel wrote:
> On Wed, Jul 13, 2022 at 09:09:58AM +0200, netdev@kapio-technology.com 
> wrote:

> 
> What are "Storm Prevention" and "zero-DPV" FDB entries?

They are both FDB entries that at the HW level drops all packets having 
a specific SA, thus using minimum resources.
(thus the name "Storm Prevention" aka, protection against DOS attacks. 
We must remember that we operate with CPU based learning.)

> 
> There is no decision that I'm aware of. I'm simply trying to understand
> how FDB entries that have 'BR_FDB_ENTRY_LOCKED' set are handled in
> mv88e6xxx and other devices in this class. We have at least three
> different implementations to consolidate:
> 
> 1. The bridge driver, pure software forwarding. The locked entry is
> dynamically created by the bridge. Packets received via the locked port
> with a SA corresponding to the locked entry will be dropped, but will
> refresh the entry. On the other hand, packets with a DA corresponding 
> to
> the locked entry will be forwarded as known unicast through the locked
> port.
> 
> 2. Hardware implementations like Spectrum that can be programmed to 
> trap
> packets that incurred an FDB miss. Like in the first case, the locked
> entry is dynamically created by the bridge driver and also aged by it.
> Unlike in the first case, since this entry is not present in hardware,
> packets with a DA corresponding to the locked entry will be flooded as
> unknown unicast.
> 
> 3. Hardware implementations like mv88e6xxx that fire an interrupt upon
> FDB miss. Need your help to understand how the above works there and
> why. Specifically, how locked entries are represented in hardware (if 
> at
> all) and what is the significance of not installing corresponding
> entries in hardware.
> 

With the mv88e6xxx, a miss violation with the SA occurs when there is no 
entry. If you then add a normal entry with the SA, the port is open for 
that SA of course. The zero-DPV entry is an entry that ensures that 
there is no more miss violation interrupts from that SA, while dropping 
all entries with the SA.

Re: [PATCH v4 net-next 5/6] net: dsa: mv88e6xxx: mac-auth/MAB implementation

[netdev]

> 
> In other words, this patch set makes MAB work and breaks everything 
> else.
> I'm willing to investigate exactly what is it that breaks the other
> selftest, but not today. It may be related to the "RTNETLINK answers:
> File exists"
> messages, which themselves come from the commands
> | bridge fdb add 00:01:02:03:04:01 dev lan2 master static
> 
> If I were to randomly guess at almost 4AM in the morning, it has to do 
> with
> "bridge fdb add" rather than the "bridge fdb replace" that's used for
> the MAB selftest. The fact I pointed out a few revisions ago, that MAB
> needs to be opt-in, is now coming back to bite us. Since it's not
> opt-in, the mv88e6xxx driver always creates locked FDB entries, and 
> when
> we try to "bridge fdb add", the kernel says "hey, the FDB entry is
> already there!". Is that it?

Yes, that sounds like a reasonable explanation, as it adds 'ext learned, 
offloaded' entries. If you try and replace the 'add' with 'replace' in 
those tests, does it work?

> 
> As for how to opt into MAB. Hmm. MAB seems to be essentially CPU
> assisted learning, which creates locked FDB entries. I wonder whether 
> we
> should reconsider the position that address learning makes no sense on
> locked ports, and say that "+locked -learning" means no MAB, and
> "+locked +learning" means MAB? This would make a bunch of things more
> natural to handle in the kernel, and would also give us the opt-in we 
> need.

I have done the one and then the other. We need to have some final 
decision on this point. And remember that this gave rise to an extra 
patch to fix link-local learning if learning is turned on on a locked 
port, which resulted in the decision to allways have learning off on 
locked ports.

> 
> 
> 
> Side note, the VTU and ATU member violation printks annoy me so badly.
> They aren't stating something super useful and they're a DoS attack
> vector in itself, even if they're rate limited. I wonder whether we
> could just turn the prints into a set of ethtool counters and call it a 
> day?

Sounds like a good idea to me. :-)

Re: [PATCH v4 net-next 3/6] drivers: net: dsa: add locked fdb entry flag to drivers

[Vladimir Oltean]

On Sun, Jul 17, 2022 at 02:21:47PM +0200, netdev@kapio-technology.com wrote:
> On 2022-07-13 14:39, Ido Schimmel wrote:
> > On Wed, Jul 13, 2022 at 09:09:58AM +0200, netdev@kapio-technology.com
> > wrote:
> 
> > 
> > What are "Storm Prevention" and "zero-DPV" FDB entries?
> 
> They are both FDB entries that at the HW level drops all packets having a
> specific SA, thus using minimum resources.
> (thus the name "Storm Prevention" aka, protection against DOS attacks. We
> must remember that we operate with CPU based learning.)

DPV means Destination Port Vector, and an ATU entry with a DPV of 0
essentially means a FDB entry pointing nowhere, so it will drop the
packet. That's a slight problem with Hans' implementation, the bridge
thinks that the locked FDB entry belongs to port X, but in reality it
matches on all bridged ports (since it matches by FID). FID allocation
in mv88e6xxx is slightly strange, all VLAN-unaware bridge ports,
belonging to any bridge, share the same FID, so the FDB databases are
not exactly isolated from each other.

Re: [PATCH v4 net-next 3/6] drivers: net: dsa: add locked fdb entry flag to drivers

[netdev]

On 2022-07-17 14:57, Vladimir Oltean wrote:
> On Sun, Jul 17, 2022 at 02:21:47PM +0200, netdev@kapio-technology.com 
> wrote:
>> On 2022-07-13 14:39, Ido Schimmel wrote:
>> > On Wed, Jul 13, 2022 at 09:09:58AM +0200, netdev@kapio-technology.com
>> > wrote:
>> 
>> >
>> > What are "Storm Prevention" and "zero-DPV" FDB entries?
>> 
>> They are both FDB entries that at the HW level drops all packets 
>> having a
>> specific SA, thus using minimum resources.
>> (thus the name "Storm Prevention" aka, protection against DOS attacks. 
>> We
>> must remember that we operate with CPU based learning.)
> 
> DPV means Destination Port Vector, and an ATU entry with a DPV of 0
> essentially means a FDB entry pointing nowhere, so it will drop the
> packet. That's a slight problem with Hans' implementation, the bridge
> thinks that the locked FDB entry belongs to port X, but in reality it
> matches on all bridged ports (since it matches by FID). FID allocation
> in mv88e6xxx is slightly strange, all VLAN-unaware bridge ports,
> belonging to any bridge, share the same FID, so the FDB databases are
> not exactly isolated from each other.

But if the locked port is vlan aware and has a pvid, it should not block 
other ports. Besides the fid will be zero with vlan unaware afaik, and 
all with zero fid do not create locked entries.

Re: [PATCH v4 net-next 3/6] drivers: net: dsa: add locked fdb entry flag to drivers

[Vladimir Oltean]

On Sun, Jul 17, 2022 at 03:09:10PM +0200, netdev@kapio-technology.com wrote:
> On 2022-07-17 14:57, Vladimir Oltean wrote:
> > On Sun, Jul 17, 2022 at 02:21:47PM +0200, netdev@kapio-technology.com
> > wrote:
> > > On 2022-07-13 14:39, Ido Schimmel wrote:
> > > > On Wed, Jul 13, 2022 at 09:09:58AM +0200, netdev@kapio-technology.com
> > > > wrote:
> > > 
> > > >
> > > > What are "Storm Prevention" and "zero-DPV" FDB entries?
> > > 
> > > They are both FDB entries that at the HW level drops all packets
> > > having a
> > > specific SA, thus using minimum resources.
> > > (thus the name "Storm Prevention" aka, protection against DOS
> > > attacks. We
> > > must remember that we operate with CPU based learning.)
> > 
> > DPV means Destination Port Vector, and an ATU entry with a DPV of 0
> > essentially means a FDB entry pointing nowhere, so it will drop the
> > packet. That's a slight problem with Hans' implementation, the bridge
> > thinks that the locked FDB entry belongs to port X, but in reality it
> > matches on all bridged ports (since it matches by FID). FID allocation
> > in mv88e6xxx is slightly strange, all VLAN-unaware bridge ports,
> > belonging to any bridge, share the same FID, so the FDB databases are
> > not exactly isolated from each other.
> 
> But if the locked port is vlan aware and has a pvid, it should not block
> other ports.

I don't understand what you want to say by that. It will block all other
packets with the same MAC SA that are classified to the same FID.
In case of VLAN-aware bridges, the mv88e6xxx driver allocates a new FID
for each VID (see mv88e6xxx_atu_new). In other words, if a locked port
is VLAN-aware and has a pvid, then whatever the PVID may be, all ports
in that same VLAN are still blocked in the same way.

> Besides the fid will be zero with vlan unaware afaik, and all with
> zero fid do not create locked entries.

If by 0 you mean 1 (MV88E6XXX_FID_BRIDGED), then you are correct: ports
with FID 0 (MV88E6XXX_FID_STANDALONE) should not create locked FDB
entries, because they are, well, standalone and not bridged.
Again I don't exactly see the relevance though.

Re: [PATCH v4 net-next 3/6] drivers: net: dsa: add locked fdb entry flag to drivers

[netdev]

On 2022-07-17 15:59, Vladimir Oltean wrote:
> On Sun, Jul 17, 2022 at 03:09:10PM +0200, netdev@kapio-technology.com 
> wrote:
>> On 2022-07-17 14:57, Vladimir Oltean wrote:
>> > On Sun, Jul 17, 2022 at 02:21:47PM +0200, netdev@kapio-technology.com
>> > wrote:
>> > > On 2022-07-13 14:39, Ido Schimmel wrote:
>> > > > On Wed, Jul 13, 2022 at 09:09:58AM +0200, netdev@kapio-technology.com
>> > > > wrote:
>> > >
>> > > >
>> > > > What are "Storm Prevention" and "zero-DPV" FDB entries?
>> > >
>> > > They are both FDB entries that at the HW level drops all packets
>> > > having a
>> > > specific SA, thus using minimum resources.
>> > > (thus the name "Storm Prevention" aka, protection against DOS
>> > > attacks. We
>> > > must remember that we operate with CPU based learning.)
>> >
>> > DPV means Destination Port Vector, and an ATU entry with a DPV of 0
>> > essentially means a FDB entry pointing nowhere, so it will drop the
>> > packet. That's a slight problem with Hans' implementation, the bridge
>> > thinks that the locked FDB entry belongs to port X, but in reality it
>> > matches on all bridged ports (since it matches by FID). FID allocation
>> > in mv88e6xxx is slightly strange, all VLAN-unaware bridge ports,
>> > belonging to any bridge, share the same FID, so the FDB databases are
>> > not exactly isolated from each other.
>> 
>> But if the locked port is vlan aware and has a pvid, it should not 
>> block
>> other ports.
> 
> I don't understand what you want to say by that. It will block all 
> other
> packets with the same MAC SA that are classified to the same FID.
> In case of VLAN-aware bridges, the mv88e6xxx driver allocates a new FID
> for each VID (see mv88e6xxx_atu_new). In other words, if a locked port
> is VLAN-aware and has a pvid, then whatever the PVID may be, all ports
> in that same VLAN are still blocked in the same way.

Maybe I am just trying to understand the problem you are posing, so 
afaics MAC addresses should be unique and having the same MAC address 
behind a locked port and a not-locked port seems like a 
mis-configuration regardless of vlan setup? As the zero-DPV entry only 
blocks the specific SA MAC on a specific vlan, which is behind a locked 
port, there shouldn't be any problem...?

If the host behind a locked port starts sending on another vlan than 
where it got the first locked entry, another locked entry will occur, as 
the locked entries are MAC + vlan.

> 
>> Besides the fid will be zero with vlan unaware afaik, and all with
>> zero fid do not create locked entries.
> 
> If by 0 you mean 1 (MV88E6XXX_FID_BRIDGED), then you are correct: ports
> with FID 0 (MV88E6XXX_FID_STANDALONE) should not create locked FDB
> entries, because they are, well, standalone and not bridged.
> Again I don't exactly see the relevance though.

Re: [PATCH v4 net-next 3/6] drivers: net: dsa: add locked fdb entry flag to drivers

[Vladimir Oltean]

On Sun, Jul 17, 2022 at 04:57:50PM +0200, netdev@kapio-technology.com wrote:
> 
> Maybe I am just trying to understand the problem you are posing, so afaics
> MAC addresses should be unique and having the same MAC address behind a
> locked port and a not-locked port seems like a mis-configuration regardless
> of vlan setup? As the zero-DPV entry only blocks the specific SA MAC on a
> specific vlan, which is behind a locked port, there shouldn't be any
> problem...?
> 
> If the host behind a locked port starts sending on another vlan than where
> it got the first locked entry, another locked entry will occur, as the
> locked entries are MAC + vlan.

I don't think it's an invalid configuration, I have a 17-port Marvell
switch which I use as infrastructure to connect my PC with my board farm
and to the Internet. I've cropped 4 out of those 17 ports for use in
selftests, effectively now having 2 bridges (br0 used by the selftests
and br-lan for systemd-networkd).

Currently all the traffic sent and received by the selftests is done
through lan1-lan4, but if I wanted to run some bridge locked port tests
with traffic from my PC, what I'd do is I'd connect a (locked) port from br0
to a port from br-lan, and my PC would thus gain indirect connectivity to the
locked port.

Then I'd send a packet and the switch would create a locked FDB entry
for my PC's MAC address, but that FDB entry would span across the entire
MV88E6XXX_FID_BRIDGED, so practically speaking, it would block my PC's
MAC address from doing anything, including accessing the Internet, i.e.
traffic that has nothing at all to do with the locked port in br0.
That isn't quite ok.

Re: [PATCH v4 net-next 3/6] drivers: net: dsa: add locked fdb entry flag to drivers

[Ido Schimmel]

On Sun, Jul 17, 2022 at 02:21:47PM +0200, netdev@kapio-technology.com wrote:
> On 2022-07-13 14:39, Ido Schimmel wrote:
> > On Wed, Jul 13, 2022 at 09:09:58AM +0200, netdev@kapio-technology.com
> > wrote:
> 
> > 
> > What are "Storm Prevention" and "zero-DPV" FDB entries?
> 
> They are both FDB entries that at the HW level drops all packets having a
> specific SA, thus using minimum resources.
> (thus the name "Storm Prevention" aka, protection against DOS attacks. We
> must remember that we operate with CPU based learning.)
> 
> > 
> > There is no decision that I'm aware of. I'm simply trying to understand
> > how FDB entries that have 'BR_FDB_ENTRY_LOCKED' set are handled in
> > mv88e6xxx and other devices in this class. We have at least three
> > different implementations to consolidate:
> > 
> > 1. The bridge driver, pure software forwarding. The locked entry is
> > dynamically created by the bridge. Packets received via the locked port
> > with a SA corresponding to the locked entry will be dropped, but will
> > refresh the entry. On the other hand, packets with a DA corresponding to
> > the locked entry will be forwarded as known unicast through the locked
> > port.
> > 
> > 2. Hardware implementations like Spectrum that can be programmed to trap
> > packets that incurred an FDB miss. Like in the first case, the locked
> > entry is dynamically created by the bridge driver and also aged by it.
> > Unlike in the first case, since this entry is not present in hardware,
> > packets with a DA corresponding to the locked entry will be flooded as
> > unknown unicast.
> > 
> > 3. Hardware implementations like mv88e6xxx that fire an interrupt upon
> > FDB miss. Need your help to understand how the above works there and
> > why. Specifically, how locked entries are represented in hardware (if at
> > all) and what is the significance of not installing corresponding
> > entries in hardware.
> > 
> 
> With the mv88e6xxx, a miss violation with the SA occurs when there is no
> entry. If you then add a normal entry with the SA, the port is open for that
> SA of course.

Good

> The zero-DPV entry is an entry that ensures that there is no more miss
> violation interrupts from that SA, while dropping all entries with the
> SA.

Few questions:

1. Is it correct to think of this entry as an entry pointing to a
special /dev/null port?

2. After installing this entry, you no longer get miss violation
interrupts because packets with this SA incur a mismatch violation
(src_port != /dev/null) and therefore discarded in hardware?

3. What happens to packets with a DA matching the zero-DPV entry, are
they also discarded in hardware? If so, here we differ from the bridge
driver implementation where such packets will be forwarded according to
the locked entry and egress the locked port

4. The reason for installing this entry is to suppress further miss
violation interrupts?

5. If not replaced, will this entry always age out after the ageing
time? Not sure what can refresh it given that traffic does not ingress
from the /dev/null port.

Thanks

Re: [PATCH v4 net-next 3/6] drivers: net: dsa: add locked fdb entry flag to drivers

[netdev]

On 2022-07-17 17:20, Ido Schimmel wrote:
> On Sun, Jul 17, 2022 at 02:21:47PM +0200, netdev@kapio-technology.com 
> wrote:
>> On 2022-07-13 14:39, Ido Schimmel wrote:
>> > On Wed, Jul 13, 2022 at 09:09:58AM +0200, netdev@kapio-technology.com
>> > wrote:
>> 
>> >
>> > What are "Storm Prevention" and "zero-DPV" FDB entries?
>> 
>> They are both FDB entries that at the HW level drops all packets 
>> having a
>> specific SA, thus using minimum resources.
>> (thus the name "Storm Prevention" aka, protection against DOS attacks. 
>> We
>> must remember that we operate with CPU based learning.)
>> 
>> >
>> > There is no decision that I'm aware of. I'm simply trying to understand
>> > how FDB entries that have 'BR_FDB_ENTRY_LOCKED' set are handled in
>> > mv88e6xxx and other devices in this class. We have at least three
>> > different implementations to consolidate:
>> >
>> > 1. The bridge driver, pure software forwarding. The locked entry is
>> > dynamically created by the bridge. Packets received via the locked port
>> > with a SA corresponding to the locked entry will be dropped, but will
>> > refresh the entry. On the other hand, packets with a DA corresponding to
>> > the locked entry will be forwarded as known unicast through the locked
>> > port.
>> >
>> > 2. Hardware implementations like Spectrum that can be programmed to trap
>> > packets that incurred an FDB miss. Like in the first case, the locked
>> > entry is dynamically created by the bridge driver and also aged by it.
>> > Unlike in the first case, since this entry is not present in hardware,
>> > packets with a DA corresponding to the locked entry will be flooded as
>> > unknown unicast.
>> >
>> > 3. Hardware implementations like mv88e6xxx that fire an interrupt upon
>> > FDB miss. Need your help to understand how the above works there and
>> > why. Specifically, how locked entries are represented in hardware (if at
>> > all) and what is the significance of not installing corresponding
>> > entries in hardware.
>> >
>> 
>> With the mv88e6xxx, a miss violation with the SA occurs when there is 
>> no
>> entry. If you then add a normal entry with the SA, the port is open 
>> for that
>> SA of course.
> 
> Good
> 
>> The zero-DPV entry is an entry that ensures that there is no more miss
>> violation interrupts from that SA, while dropping all entries with the
>> SA.
> 
> Few questions:
> 
> 1. Is it correct to think of this entry as an entry pointing to a
> special /dev/null port?

I guess you can think of it like that. It's internal to the chipset how 
it does it.

> 
> 2. After installing this entry, you no longer get miss violation
> interrupts because packets with this SA incur a mismatch violation
> (src_port != /dev/null) and therefore discarded in hardware?

Yes, and mismatch violations are suppressed in this implementation when 
locking the port.

> 
> 3. What happens to packets with a DA matching the zero-DPV entry, are
> they also discarded in hardware? If so, here we differ from the bridge
> driver implementation where such packets will be forwarded according to
> the locked entry and egress the locked port

I understand that egress will follow what is setup with regard to UC, MC 
and BC, though I haven't tested that. But no replies will get through of 
course as long as the port hasn't been opened for the iface behind the 
locked port.

> 
> 4. The reason for installing this entry is to suppress further miss
> violation interrupts?

Yes, while still HW dropping all ingress packets with the same (SA-mac, 
vlan) on  the port.

> 
> 5. If not replaced, will this entry always age out after the ageing
> time? Not sure what can refresh it given that traffic does not ingress
> from the /dev/null port.

That is where my implementation keeps the entries in a list and removes 
them after the bridge timeout using a kernel worker and jiffies.
So by default they age out after approx. 5 min.

> 
> Thanks

Re: [PATCH v4 net-next 3/6] drivers: net: dsa: add locked fdb entry flag to drivers

[netdev]

On 2022-07-17 17:08, Vladimir Oltean wrote:
> On Sun, Jul 17, 2022 at 04:57:50PM +0200, netdev@kapio-technology.com 
> wrote:
>> 
>> Maybe I am just trying to understand the problem you are posing, so 
>> afaics
>> MAC addresses should be unique and having the same MAC address behind 
>> a
>> locked port and a not-locked port seems like a mis-configuration 
>> regardless
>> of vlan setup? As the zero-DPV entry only blocks the specific SA MAC 
>> on a
>> specific vlan, which is behind a locked port, there shouldn't be any
>> problem...?
>> 
>> If the host behind a locked port starts sending on another vlan than 
>> where
>> it got the first locked entry, another locked entry will occur, as the
>> locked entries are MAC + vlan.
> 
> I don't think it's an invalid configuration, I have a 17-port Marvell
> switch which I use as infrastructure to connect my PC with my board 
> farm
> and to the Internet. I've cropped 4 out of those 17 ports for use in
> selftests, effectively now having 2 bridges (br0 used by the selftests
> and br-lan for systemd-networkd).
> 
> Currently all the traffic sent and received by the selftests is done
> through lan1-lan4, but if I wanted to run some bridge locked port tests
> with traffic from my PC, what I'd do is I'd connect a (locked) port 
> from br0
> to a port from br-lan, and my PC would thus gain indirect connectivity 
> to the
> locked port.
> 
> Then I'd send a packet and the switch would create a locked FDB entry
> for my PC's MAC address, but that FDB entry would span across the 
> entire
> MV88E6XXX_FID_BRIDGED, so practically speaking, it would block my PC's
> MAC address from doing anything, including accessing the Internet, i.e.
> traffic that has nothing at all to do with the locked port in br0.
> That isn't quite ok.

Okay, I see the problem you refer to. I think that we have to accept 
some limitations unless you think that just zeroing the specific port 
bit in the DPV would be a better solution, and there wouldn't be caveats 
with that besides having to do a FDB search etc to get the correct DPV 
if I am not too mistaken.

Also trunk ports is a limitation as that is not supported in this 
implementation.

Re: [PATCH v4 net-next 3/6] drivers: net: dsa: add locked fdb entry flag to drivers

[Vladimir Oltean]

On Fri, Jul 08, 2022 at 11:50:33AM +0200, netdev@kapio-technology.com wrote:
> On 2022-07-08 11:15, Vladimir Oltean wrote:
> > When the possibility for it to be true will exist, _all_ switchdev
> > drivers will need to be updated to ignore that (mlxsw, cpss, ocelot,
> > rocker, prestera, etc etc), not just DSA. And you don't need to
> > propagate the is_locked flag to all individual DSA sub-drivers when none
> > care about is_locked in the ADD_TO_DEVICE direction, you can just ignore
> > within DSA until needed otherwise.
> > 
> 
> Maybe I have it wrong, but I think that Ido requested me to send it to all
> the drivers, and have them ignore entries with is_locked=true ...

Yes, but re-read my message about what "all the drivers" means.

Re: [PATCH v4 net-next 3/6] drivers: net: dsa: add locked fdb entry flag to drivers

[Vladimir Oltean]

On Sun, Jul 17, 2022 at 06:10:22PM +0200, netdev@kapio-technology.com wrote:
> Okay, I see the problem you refer to. I think that we have to accept some
> limitations unless you think that just zeroing the specific port bit in the
> DPV would be a better solution, and there wouldn't be caveats with that
> besides having to do a FDB search etc to get the correct DPV if I am not too
> mistaken.

No, honestly I believe that what we should do to improve the limitation
is to have proper ATU database separation between one VLAN-unaware
bridge and another (i.e. what is now MV88E6XXX_FID_BRIDGED to be one FID
per bridge).

Re: [PATCH v4 net-next 3/6] drivers: net: dsa: add locked fdb entry flag to drivers

[Vladimir Oltean]

On Sun, Jul 17, 2022 at 05:53:22PM +0200, netdev@kapio-technology.com wrote:
> > 3. What happens to packets with a DA matching the zero-DPV entry, are
> > they also discarded in hardware? If so, here we differ from the bridge
> > driver implementation where such packets will be forwarded according to
> > the locked entry and egress the locked port
> 
> I understand that egress will follow what is setup with regard to UC, MC and
> BC, though I haven't tested that. But no replies will get through of course
> as long as the port hasn't been opened for the iface behind the locked port.

Here, should we be rather fixing the software bridge, if the current
behavior is to forward packets towards locked FDB entries?

Re: [PATCH v4 net-next 5/6] net: dsa: mv88e6xxx: mac-auth/MAB implementation

[Vladimir Oltean]

On Sun, Jul 17, 2022 at 02:34:22PM +0200, netdev@kapio-technology.com wrote:
> > If I were to randomly guess at almost 4AM in the morning, it has to do with
> > "bridge fdb add" rather than the "bridge fdb replace" that's used for
> > the MAB selftest. The fact I pointed out a few revisions ago, that MAB
> > needs to be opt-in, is now coming back to bite us. Since it's not
> > opt-in, the mv88e6xxx driver always creates locked FDB entries, and when
> > we try to "bridge fdb add", the kernel says "hey, the FDB entry is
> > already there!". Is that it?
> 
> Yes, that sounds like a reasonable explanation, as it adds 'ext learned,
> offloaded' entries. If you try and replace the 'add' with 'replace' in those
> tests, does it work?

Well, you have access to the selftests too... But yes, that is the
reason, and it works when I change 'add' to 'replace', although of
course this isn't the correct solution.

> > As for how to opt into MAB. Hmm. MAB seems to be essentially CPU
> > assisted learning, which creates locked FDB entries. I wonder whether we
> > should reconsider the position that address learning makes no sense on
> > locked ports, and say that "+locked -learning" means no MAB, and
> > "+locked +learning" means MAB? This would make a bunch of things more
> > natural to handle in the kernel, and would also give us the opt-in we
> > need.
> 
> I have done the one and then the other. We need to have some final decision
> on this point. And remember that this gave rise to an extra patch to fix
> link-local learning if learning is turned on on a locked port, which
> resulted in the decision to allways have learning off on locked ports.

I think part of the reason for the back-and-forth was not making a very
clear distinction between basic 802.1X using hostapd, and MAB. While I
agree hostapd doesn't have what to do with learning, for MAB I'm still
wondering. It's the same situation for mv88e6xxx's Port Association
Vector in fact.

> > Side note, the VTU and ATU member violation printks annoy me so badly.
> > They aren't stating something super useful and they're a DoS attack
> > vector in itself, even if they're rate limited. I wonder whether we
> > could just turn the prints into a set of ethtool counters and call it a
> > day?
> 
> Sounds like a good idea to me. :-)

Thinking this through, what we really want is trace points here,
otherwise we'd lose information about which MAC address/VID/FID was it
that caused the violation.

Re: [PATCH v4 net-next 3/6] drivers: net: dsa: add locked fdb entry flag to drivers

[Ido Schimmel]

On Thu, Jul 21, 2022 at 02:59:35PM +0300, Vladimir Oltean wrote:
> On Sun, Jul 17, 2022 at 05:53:22PM +0200, netdev@kapio-technology.com wrote:
> > > 3. What happens to packets with a DA matching the zero-DPV entry, are
> > > they also discarded in hardware? If so, here we differ from the bridge
> > > driver implementation where such packets will be forwarded according to
> > > the locked entry and egress the locked port
> > 
> > I understand that egress will follow what is setup with regard to UC, MC and
> > BC, though I haven't tested that. But no replies will get through of course
> > as long as the port hasn't been opened for the iface behind the locked port.
> 
> Here, should we be rather fixing the software bridge, if the current
> behavior is to forward packets towards locked FDB entries?

I think the bridge needs to be fixed, but not to discard packets. If I
decided to lock a port, it means I do not blindly trust whoever who
is behind the port, but instead want to authorize them first. Since an
unauthorized user is able to create locked FDB entries we need to
carefully define what they mean. I tried looking information about MAB
online, but couldn't find detailed material that answers my questions,
so my answers are based on what I believe is logical, which might be
wrong.

Currently, the bridge will forward packets to a locked entry which
effectively means that an unauthorized host can cause the bridge to
direct packets to it and sniff them. Yes, the host can't send any
packets through the port (while locked) and can't overtake an existing
(unlocked) FDB entry, but it still seems like an odd decision. IMO, the
situation in mv88e6xxx is even worse because there an unauthorized host
can cause packets to a certain DMAC to be blackholed via its zero-DPV
entry.

Another (minor?) issue is that locked entries cannot roam between locked
ports. Lets say that my user space MAB policy is to authorize MAC X if
it appears behind one of the locked ports swp1-swp4. An unauthorized
host behind locked port swp5 can generate packets with SMAC X,
preventing the true owner of this MAC behind swp1 from ever being
authorized.

It seems like the main purpose of these locked entries is to signal to
user space the presence of a certain MAC behind a locked port, but they
should not be able to affect packet forwarding in the bridge, unlike
regular entries.

Regarding a separate knob for MAB, I tend to agree we need it. Otherwise
we cannot control which locked ports are able to populate the FDB with
locked entries. I don't particularly like the fact that we overload an
existing flag ("learning") for that. Any reason not to add an explicit
flag ("mab")? At least with the current implementation, locked entries
cannot roam between locked ports and cannot be refreshed, which differs
from regular learning.

Re: [PATCH v4 net-next 3/6] drivers: net: dsa: add locked fdb entry flag to drivers

[Vladimir Oltean]

On Thu, Jul 21, 2022 at 04:27:52PM +0300, Ido Schimmel wrote:
> I tried looking information about MAB online, but couldn't find
> detailed material that answers my questions, so my answers are based
> on what I believe is logical, which might be wrong.

I'm kind of in the same situation here.

> Currently, the bridge will forward packets to a locked entry which
> effectively means that an unauthorized host can cause the bridge to
> direct packets to it and sniff them. Yes, the host can't send any
> packets through the port (while locked) and can't overtake an existing
> (unlocked) FDB entry, but it still seems like an odd decision. IMO, the
> situation in mv88e6xxx is even worse because there an unauthorized host
> can cause packets to a certain DMAC to be blackholed via its zero-DPV
> entry.
> 
> Another (minor?) issue is that locked entries cannot roam between locked
> ports. Lets say that my user space MAB policy is to authorize MAC X if
> it appears behind one of the locked ports swp1-swp4. An unauthorized
> host behind locked port swp5 can generate packets with SMAC X,
> preventing the true owner of this MAC behind swp1 from ever being
> authorized.

In the mv88e6xxx offload implementation, the locked entries eventually
age out from time to time, practically giving the true owner of the MAC
address another chance every 5 minutes or so. In the pure software
implementation of locked FDB entries I'm not quite sure. It wouldn't
make much sense for the behavior to differ significantly though.

> It seems like the main purpose of these locked entries is to signal to
> user space the presence of a certain MAC behind a locked port, but they
> should not be able to affect packet forwarding in the bridge, unlike
> regular entries.

So essentially what you want is for br_handle_frame_finish() to treat
"dst = br_fdb_find_rcu(br, eth_hdr(skb)->h_dest, vid);" as NULL if
test_bit(BR_FDB_LOCKED, &dst->flags) is true?

> Regarding a separate knob for MAB, I tend to agree we need it. Otherwise
> we cannot control which locked ports are able to populate the FDB with
> locked entries. I don't particularly like the fact that we overload an
> existing flag ("learning") for that. Any reason not to add an explicit
> flag ("mab")? At least with the current implementation, locked entries
> cannot roam between locked ports and cannot be refreshed, which differs
> from regular learning.

Well, assuming we model the software bridge closer to mv88e6xxx (where
locked FDB entries can roam after a certain time), does this change things?
In the software implementation I think it would make sense for them to
be able to roam right away (the age-out interval in mv88e6xxx is just a
compromise between responsiveness to roaming and resistance to DoS).

Re: [PATCH v4 net-next 3/6] drivers: net: dsa: add locked fdb entry flag to drivers

[Ido Schimmel]

On Thu, Jul 21, 2022 at 05:20:01PM +0300, Vladimir Oltean wrote:
> On Thu, Jul 21, 2022 at 04:27:52PM +0300, Ido Schimmel wrote:
> > I tried looking information about MAB online, but couldn't find
> > detailed material that answers my questions, so my answers are based
> > on what I believe is logical, which might be wrong.
> 
> I'm kind of in the same situation here.

:(

> 
> > Currently, the bridge will forward packets to a locked entry which
> > effectively means that an unauthorized host can cause the bridge to
> > direct packets to it and sniff them. Yes, the host can't send any
> > packets through the port (while locked) and can't overtake an existing
> > (unlocked) FDB entry, but it still seems like an odd decision. IMO, the
> > situation in mv88e6xxx is even worse because there an unauthorized host
> > can cause packets to a certain DMAC to be blackholed via its zero-DPV
> > entry.
> > 
> > Another (minor?) issue is that locked entries cannot roam between locked
> > ports. Lets say that my user space MAB policy is to authorize MAC X if
> > it appears behind one of the locked ports swp1-swp4. An unauthorized
> > host behind locked port swp5 can generate packets with SMAC X,
> > preventing the true owner of this MAC behind swp1 from ever being
> > authorized.
> 
> In the mv88e6xxx offload implementation, the locked entries eventually
> age out from time to time, practically giving the true owner of the MAC
> address another chance every 5 minutes or so. In the pure software
> implementation of locked FDB entries I'm not quite sure. It wouldn't
> make much sense for the behavior to differ significantly though.

From what I can tell, the same happens in software, but this behavior
does not really make sense to me. It differs from how other learned
entries age/roam and can lead to problems such as the one described
above. It is also not documented anywhere, so I can't tell if it's
intentional or an oversight. We need to have a good reason for such a
behavior other than the fact that it appears to conform to the quirks of
one hardware implementation.

> 
> > It seems like the main purpose of these locked entries is to signal to
> > user space the presence of a certain MAC behind a locked port, but they
> > should not be able to affect packet forwarding in the bridge, unlike
> > regular entries.
> 
> So essentially what you want is for br_handle_frame_finish() to treat
> "dst = br_fdb_find_rcu(br, eth_hdr(skb)->h_dest, vid);" as NULL if
> test_bit(BR_FDB_LOCKED, &dst->flags) is true?

Yes. It's not clear to me why unauthorized hosts should be given the
ability to affect packet forwarding in the bridge through these locked
entries when their primary purpose seems to be notifying user space
about the presence of the MAC. At the very least this should be
explained in the commit message, to indicate that some thought went into
this decision.

> 
> > Regarding a separate knob for MAB, I tend to agree we need it. Otherwise
> > we cannot control which locked ports are able to populate the FDB with
> > locked entries. I don't particularly like the fact that we overload an
> > existing flag ("learning") for that. Any reason not to add an explicit
> > flag ("mab")? At least with the current implementation, locked entries
> > cannot roam between locked ports and cannot be refreshed, which differs
> > from regular learning.
> 
> Well, assuming we model the software bridge closer to mv88e6xxx (where
> locked FDB entries can roam after a certain time), does this change things?
> In the software implementation I think it would make sense for them to
> be able to roam right away (the age-out interval in mv88e6xxx is just a
> compromise between responsiveness to roaming and resistance to DoS).

Exactly. If this is the best that we can do with mv88e6xxx, then so be
it, but other implementations (software/hardware) do not have the same
limitations and I don't see a reason to bend them.

Regarding "learning" vs. "mab" (or something else), the former is a
well-defined flag available since forever. In 5.18 and 5.19 it can also
be enabled together with "locked" and packets from an unauthorized host
(modulo link-local ones) will not populate the FDB. I prefer not to
change an existing behavior.

From usability point of view, I think a new flag would be easier to
explain than explaining that "learning on" behaves like A or B, based on
whether "locked on" is set. The bridge can also be taught to forbid the
new flag from being set when "locked" is not set.

A user space daemon that wants to try 802.1x and fallback to MAB can
enable both flags or enable "mab" after some timer expires.

Re: [PATCH v4 net-next 3/6] drivers: net: dsa: add locked fdb entry flag to drivers

[netdev]

On 2022-07-24 13:10, Ido Schimmel wrote:
> On Thu, Jul 21, 2022 at 05:20:01PM +0300, Vladimir Oltean wrote:
>> On Thu, Jul 21, 2022 at 04:27:52PM +0300, Ido Schimmel wrote:
>> > I tried looking information about MAB online, but couldn't find
>> > detailed material that answers my questions, so my answers are based
>> > on what I believe is logical, which might be wrong.
>> 
>> I'm kind of in the same situation here.
> 
> :(
> 

Sorry for being off here a while... here is my best link regarding MAB. 
Maybe it can answer some of your questions...

https://www.cisco.com/c/en/us/td/docs/solutions/Enterprise/Security/TrustSec_1-99/MAB/MAB_Dep_Guide.html#wp392522

Re: [PATCH v4 net-next 3/6] drivers: net: dsa: add locked fdb entry flag to drivers

[netdev]

On 2022-07-24 13:10, Ido Schimmel wrote:
> 
>> In the mv88e6xxx offload implementation, the locked entries eventually
>> age out from time to time, practically giving the true owner of the 
>> MAC
>> address another chance every 5 minutes or so. In the pure software
>> implementation of locked FDB entries I'm not quite sure. It wouldn't
>> make much sense for the behavior to differ significantly though.
> 
> From what I can tell, the same happens in software, but this behavior
> does not really make sense to me. It differs from how other learned
> entries age/roam and can lead to problems such as the one described
> above. It is also not documented anywhere, so I can't tell if it's
> intentional or an oversight. We need to have a good reason for such a
> behavior other than the fact that it appears to conform to the quirks 
> of
> one hardware implementation.
> 
>> 
>> > It seems like the main purpose of these locked entries is to signal to
>> > user space the presence of a certain MAC behind a locked port, but they
>> > should not be able to affect packet forwarding in the bridge, unlike
>> > regular entries.
>> 
>> So essentially what you want is for br_handle_frame_finish() to treat
>> "dst = br_fdb_find_rcu(br, eth_hdr(skb)->h_dest, vid);" as NULL if
>> test_bit(BR_FDB_LOCKED, &dst->flags) is true?
> 
> Yes. It's not clear to me why unauthorized hosts should be given the
> ability to affect packet forwarding in the bridge through these locked
> entries when their primary purpose seems to be notifying user space
> about the presence of the MAC. At the very least this should be
> explained in the commit message, to indicate that some thought went 
> into
> this decision.
> 

I guess you are right that the SW setup locked entries can be used to 
gain uni-directional traffic through a switch, which should really not 
be the case.
In this case I expect the zero-DPV entries to not give this ability, 
which is the correct behaviour with MAB IMHO.

>> 
>> > Regarding a separate knob for MAB, I tend to agree we need it. Otherwise
>> > we cannot control which locked ports are able to populate the FDB with
>> > locked entries. I don't particularly like the fact that we overload an
>> > existing flag ("learning") for that. Any reason not to add an explicit
>> > flag ("mab")? At least with the current implementation, locked entries
>> > cannot roam between locked ports and cannot be refreshed, which differs
>> > from regular learning.
>> 
>> Well, assuming we model the software bridge closer to mv88e6xxx (where
>> locked FDB entries can roam after a certain time), does this change 
>> things?
>> In the software implementation I think it would make sense for them to
>> be able to roam right away (the age-out interval in mv88e6xxx is just 
>> a
>> compromise between responsiveness to roaming and resistance to DoS).
> 
> Exactly. If this is the best that we can do with mv88e6xxx, then so be
> it, but other implementations (software/hardware) do not have the same
> limitations and I don't see a reason to bend them.
> 
> Regarding "learning" vs. "mab" (or something else), the former is a
> well-defined flag available since forever. In 5.18 and 5.19 it can also
> be enabled together with "locked" and packets from an unauthorized host
> (modulo link-local ones) will not populate the FDB. I prefer not to
> change an existing behavior.
> 
> From usability point of view, I think a new flag would be easier to
> explain than explaining that "learning on" behaves like A or B, based 
> on
> whether "locked on" is set. The bridge can also be taught to forbid the
> new flag from being set when "locked" is not set.
> 
> A user space daemon that wants to try 802.1x and fallback to MAB can
> enable both flags or enable "mab" after some timer expires.

With this driver it is not really an option to use +learning for a 
opt-in for MAB. I think locked port should always have -learning before 
locking the port. In fact there is a problem in this implementation with 
MAB if -learning is applied after locking the port, as that will disable 
MAB, but also refresh and all other violation interrupts.
So I guess I need to disable the learning flag to the driver when the 
port is locked, or even from the bridge?

Re: [PATCH v4 net-next 3/6] drivers: net: dsa: add locked fdb entry flag to drivers

[netdev]

On 2022-07-13 14:39, Ido Schimmel wrote:

> 
> What are "Storm Prevention" and "zero-DPV" FDB entries?
> 

For the zero-DPV entries, I can summarize:

Since a CPU can become saturated from constant SA Miss Violations from a 
denied source, source MAC address are masked by loading a zero-DPV 
(Destination Port Vector) entry in the ATU. As the address now appears 
in the database it will not cause more Miss Violations. ANY port trying 
to send a frame to this unauthorized address is discarded. Any locked 
port trying to use this unauthorized address has its frames discarded 
too (as the ports SA bit is not set in the ATU entry).

Re: [PATCH v4 net-next 2/6] net: switchdev: add support for offloading of fdb locked flag

[netdev]

On 2022-07-08 10:54, Vladimir Oltean wrote:
>>  };
>> 
>>  /* TODO: ideally DSA ports would have a single dp->link_dp member,
>> diff --git a/include/net/switchdev.h b/include/net/switchdev.h
>> index aa0171d5786d..9f83c835ee62 100644
>> --- a/include/net/switchdev.h
>> +++ b/include/net/switchdev.h
>> @@ -245,6 +245,7 @@ struct switchdev_notifier_fdb_info {
>>  	u16 vid;
>>  	u8 added_by_user:1,
>>  	   is_local:1,
>> +	   is_locked:1,
>>  	   offloaded:1;
>>  };
>> 
>> diff --git a/net/bridge/br.c b/net/bridge/br.c
>> index 96e91d69a9a8..fe0a4741fcda 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, false,
>> +						fdb_info->is_locked);
>>  		if (err) {
>>  			err = notifier_from_errno(err);
>>  			break;
>> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
>> index ee9064a536ae..32ebb18050b9 100644
>> --- a/net/bridge/br_fdb.c
>> +++ b/net/bridge/br_fdb.c
>> @@ -1136,7 +1136,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, true, false);
>>  	} else {
>>  		spin_lock_bh(&br->hash_lock);
>>  		err = fdb_add_entry(br, p, addr, ndm, nlh_flags, vid, nfea_tb);
>> @@ -1366,7 +1366,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,
>> -			      bool swdev_notify)
>> +			      bool swdev_notify, bool locked)
>>  {
>>  	struct net_bridge_fdb_entry *fdb;
>>  	bool modified = false;
>> @@ -1386,6 +1386,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_ENTRY_LOCKED);
>> +
>>  		fdb = fdb_create(br, p, addr, vid, flags);
>>  		if (!fdb) {
>>  			err = -ENOMEM;
>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>> index 47a3598d25c8..9082451b4d40 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 swdev_notify, bool locked);
>>  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..85e566b856e1 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->is_locked = test_bit(BR_FDB_ENTRY_LOCKED, &fdb->flags);
>>  	item->info.dev = (!p || item->is_local) ? br->dev : p->dev;
>>  	item->info.ctx = ctx;
>>  }
>> diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
>> index d9722e49864b..42f47a94b0f0 100644
>> --- a/net/dsa/dsa_priv.h
>> +++ b/net/dsa/dsa_priv.h
>> @@ -65,6 +65,7 @@ struct dsa_notifier_fdb_info {
>>  	const struct dsa_port *dp;
>>  	const unsigned char *addr;
>>  	u16 vid;
>> +	bool is_locked;
> 
> drop
> 

If this is dropped, the whole scheme will not work as userspace will not 
get notifications of new locked entries from the driver.

Re: [PATCH v4 net-next 2/6] net: switchdev: add support for offloading of fdb locked flag

[netdev]

On 2022-07-08 10:54, Vladimir Oltean wrote:

>>  	struct dsa_db db;
>>  };
>> 
>> @@ -131,6 +132,7 @@ struct dsa_switchdev_event_work {
>>  	unsigned char addr[ETH_ALEN];
>>  	u16 vid;
>>  	bool host_addr;
>> +	bool is_locked;
> 
> drop
> 
>>  };
>> 
>>  enum dsa_standalone_event {
>> @@ -232,7 +234,7 @@ int dsa_port_vlan_msti(struct dsa_port *dp,
>>  		       const struct switchdev_vlan_msti *msti);
>>  int dsa_port_mtu_change(struct dsa_port *dp, int new_mtu);
>>  int dsa_port_fdb_add(struct dsa_port *dp, const unsigned char *addr,
>> -		     u16 vid);
>> +		     u16 vid, bool is_locked);
> 
> drop
> 
>>  int dsa_port_fdb_del(struct dsa_port *dp, const unsigned char *addr,
>>  		     u16 vid);
>>  int dsa_port_standalone_host_fdb_add(struct dsa_port *dp,
>> diff --git a/net/dsa/port.c b/net/dsa/port.c
>> index 3738f2d40a0b..8bdac9aabe5d 100644
>> --- a/net/dsa/port.c
>> +++ b/net/dsa/port.c
>> @@ -35,6 +35,7 @@ static void dsa_port_notify_bridge_fdb_flush(const 
>> struct dsa_port *dp, u16 vid)
>>  	struct net_device *brport_dev = dsa_port_to_bridge_port(dp);
>>  	struct switchdev_notifier_fdb_info info = {
>>  		.vid = vid,
>> +		.is_locked = false,
> 
> drop
> 
>>  	};
>> 
>>  	/* When the port becomes standalone it has already left the bridge.
>> @@ -950,12 +951,13 @@ int dsa_port_mtu_change(struct dsa_port *dp, int 
>> new_mtu)
>>  }
>> 
>>  int dsa_port_fdb_add(struct dsa_port *dp, const unsigned char *addr,
>> -		     u16 vid)
>> +		     u16 vid, bool is_locked)
> 
> drop
> 
>>  {
>>  	struct dsa_notifier_fdb_info info = {
>>  		.dp = dp,
>>  		.addr = addr,
>>  		.vid = vid,
>> +		.is_locked = is_locked,
> 
> drop
> 
>>  		.db = {
>>  			.type = DSA_DB_BRIDGE,
>>  			.bridge = *dp->bridge,
>> @@ -979,6 +981,7 @@ int dsa_port_fdb_del(struct dsa_port *dp, const 
>> unsigned char *addr,
>>  		.dp = dp,
>>  		.addr = addr,
>>  		.vid = vid,
>> +		.is_locked = false,
> 
> drop
> 
>>  		.db = {
>>  			.type = DSA_DB_BRIDGE,
>>  			.bridge = *dp->bridge,
>> @@ -999,6 +1002,7 @@ static int dsa_port_host_fdb_add(struct dsa_port 
>> *dp,
>>  		.dp = dp,
>>  		.addr = addr,
>>  		.vid = vid,
>> +		.is_locked = false,
> 
> drop
> 
>>  		.db = db,
>>  	};
>> 
>> @@ -1050,6 +1054,7 @@ static int dsa_port_host_fdb_del(struct dsa_port 
>> *dp,
>>  		.dp = dp,
>>  		.addr = addr,
>>  		.vid = vid,
>> +		.is_locked = false,
> 
> drop
> 
>>  		.db = db,
>>  	};
>> 
>> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
>> index 801a5d445833..905b15e4eab9 100644
>> --- a/net/dsa/slave.c
>> +++ b/net/dsa/slave.c
>> @@ -2784,6 +2784,7 @@ static void 
>> dsa_slave_switchdev_event_work(struct work_struct *work)
>>  		container_of(work, struct dsa_switchdev_event_work, work);
>>  	const unsigned char *addr = switchdev_work->addr;
>>  	struct net_device *dev = switchdev_work->dev;
>> +	bool is_locked = switchdev_work->is_locked;
> 
> drop
> 
>>  	u16 vid = switchdev_work->vid;
>>  	struct dsa_switch *ds;
>>  	struct dsa_port *dp;
>> @@ -2799,7 +2800,7 @@ static void 
>> dsa_slave_switchdev_event_work(struct work_struct *work)
>>  		else if (dp->lag)
>>  			err = dsa_port_lag_fdb_add(dp, addr, vid);
>>  		else
>> -			err = dsa_port_fdb_add(dp, addr, vid);
>> +			err = dsa_port_fdb_add(dp, addr, vid, is_locked);
> 
> drop
> 
>>  		if (err) {
>>  			dev_err(ds->dev,
>>  				"port %d failed to add %pM vid %d to fdb: %d\n",
>> @@ -2907,6 +2908,7 @@ static int dsa_slave_fdb_event(struct net_device 
>> *dev,
>>  	ether_addr_copy(switchdev_work->addr, fdb_info->addr);
>>  	switchdev_work->vid = fdb_info->vid;
>>  	switchdev_work->host_addr = host_addr;
>> +	switchdev_work->is_locked = fdb_info->is_locked;
> 
> drop
> 
>> 
>>  	dsa_schedule_work(&switchdev_work->work);
>> 
>> diff --git a/net/dsa/switch.c b/net/dsa/switch.c
>> index 2b56218fc57c..32b1e7ac6373 100644
>> --- a/net/dsa/switch.c
>> +++ b/net/dsa/switch.c
>> @@ -234,7 +234,7 @@ static int dsa_port_do_mdb_del(struct dsa_port 
>> *dp,
>>  }
>> 
>>  static int dsa_port_do_fdb_add(struct dsa_port *dp, const unsigned 
>> char *addr,
>> -			       u16 vid, struct dsa_db db)
>> +			       u16 vid, bool is_locked, struct dsa_db db)
> 
> drop
> 
>>  {
>>  	struct dsa_switch *ds = dp->ds;
>>  	struct dsa_mac_addr *a;
>> @@ -398,7 +398,7 @@ static int dsa_switch_host_fdb_add(struct 
>> dsa_switch *ds,
>>  	dsa_switch_for_each_port(dp, ds) {
>>  		if (dsa_port_host_address_match(dp, info->dp)) {
>>  			err = dsa_port_do_fdb_add(dp, info->addr, info->vid,
>> -						  info->db);
>> +						  false, info->db);
> 
> drop
> 
>>  			if (err)
>>  				break;
>>  		}
>> @@ -437,7 +437,7 @@ static int dsa_switch_fdb_add(struct dsa_switch 
>> *ds,
>>  	if (!ds->ops->port_fdb_add)
>>  		return -EOPNOTSUPP;
>> 
>> -	return dsa_port_do_fdb_add(dp, info->addr, info->vid, info->db);
>> +	return dsa_port_do_fdb_add(dp, info->addr, info->vid, 
>> info->is_locked, info->db);
> 
> drop
> 
>>  }
>> 
>>  static int dsa_switch_fdb_del(struct dsa_switch *ds,
>> --
>> 2.30.2
>> 

Hi Vladimir and Ido,

I can either ignore locked entries early or late in the dsa/switchdev 
layers.

If I ignore early, I think it should be in br_switchdev_fdb_notify() in 
net/bridge/br_switchdev.c.
If I ignore late, I would think that it should be jut before sending it 
to the driver(s), e.g. in dsa_port_do_fdb_add() in net/dsa/switch.c.

There is of course pros and cons of both options, but if the flag is 
never to be sent to the driver, then it should be ignored early.

If ignored late most of this patch should not be dropped.

Re: [PATCH v4 net-next 3/6] drivers: net: dsa: add locked fdb entry flag to drivers

[netdev]

On 2022-07-21 13:59, Vladimir Oltean wrote:
> On Sun, Jul 17, 2022 at 05:53:22PM +0200, netdev@kapio-technology.com 
> wrote:
>> > 3. What happens to packets with a DA matching the zero-DPV entry, are
>> > they also discarded in hardware? If so, here we differ from the bridge
>> > driver implementation where such packets will be forwarded according to
>> > the locked entry and egress the locked port
>> 
>> I understand that egress will follow what is setup with regard to UC, 
>> MC and
>> BC, though I haven't tested that. But no replies will get through of 
>> course
>> as long as the port hasn't been opened for the iface behind the locked 
>> port.
> 
> Here, should we be rather fixing the software bridge, if the current
> behavior is to forward packets towards locked FDB entries?

Yes, I think that locked entries should block egress to the respective 
hosts behind the locked port, which should be fixed in the bridge.

Re: [PATCH v4 net-next 3/6] drivers: net: dsa: add locked fdb entry flag to drivers

[Ido Schimmel]

On Mon, Aug 01, 2022 at 05:33:49PM +0200, netdev@kapio-technology.com wrote:
> On 2022-07-13 14:39, Ido Schimmel wrote:
> 
> > 
> > What are "Storm Prevention" and "zero-DPV" FDB entries?
> > 
> 
> For the zero-DPV entries, I can summarize:
> 
> Since a CPU can become saturated from constant SA Miss Violations from a
> denied source, source MAC address are masked by loading a zero-DPV
> (Destination Port Vector) entry in the ATU. As the address now appears in
> the database it will not cause more Miss Violations. ANY port trying to send
> a frame to this unauthorized address is discarded. Any locked port trying to
> use this unauthorized address has its frames discarded too (as the ports SA
> bit is not set in the ATU entry).

What happens to unlocked ports that have learning enabled and are trying
to use this address as SMAC? AFAICT, at least in the bridge driver, the
locked entry will roam, but will keep the "locked" flag, which is
probably not what we want. Let's see if we can agree on these semantics
for a "locked" entry:

1. It discards packets with matching DMAC, regardless of ingress port. I
read the document [1] you linked to in a different reply and could not
find anything against this approach, so this might be fine or at least
not very significant.

Note that this means that "locked" entries need to be notified to device
drivers so that they will install a matching entry in the HW FDB.

2. It is not refreshed and has ageing enabled. That is, after initial
installation it will be removed by the bridge driver after configured
ageing time unless converted to a regular (unlocked) entry.

I assume this allows you to remove the timer implementation from your
driver and let the bridge driver notify you about the removal of this
entry.

3. With regards to roaming, the entry cannot roam between locked ports
(they need to have learning disabled anyway), but can roam to an
unlocked port, in which case it becomes a regular entry that can roam
and age.

If we agree on these semantics, then I can try to verify that at least
Spectrum can support them (it seems mv88e6xxx can).

P.S. Sorry for the delay, I'm busy with other tasks at the moment.

[1] https://www.cisco.com/c/en/us/td/docs/solutions/Enterprise/Security/TrustSec_1-99/MAB/MAB_Dep_Guide.html#wp392522

Re: [PATCH v4 net-next 3/6] drivers: net: dsa: add locked fdb entry flag to drivers

[netdev]

On 2022-08-09 11:20, Ido Schimmel wrote:
> On Mon, Aug 01, 2022 at 05:33:49PM +0200, netdev@kapio-technology.com 
> wrote:
>> On 2022-07-13 14:39, Ido Schimmel wrote:
>> 
>> >
>> > What are "Storm Prevention" and "zero-DPV" FDB entries?
>> >
>> 
>> For the zero-DPV entries, I can summarize:
>> 
>> Since a CPU can become saturated from constant SA Miss Violations from 
>> a
>> denied source, source MAC address are masked by loading a zero-DPV
>> (Destination Port Vector) entry in the ATU. As the address now appears 
>> in
>> the database it will not cause more Miss Violations. ANY port trying 
>> to send
>> a frame to this unauthorized address is discarded. Any locked port 
>> trying to
>> use this unauthorized address has its frames discarded too (as the 
>> ports SA
>> bit is not set in the ATU entry).
> 
> What happens to unlocked ports that have learning enabled and are 
> trying
> to use this address as SMAC? AFAICT, at least in the bridge driver, the
> locked entry will roam, but will keep the "locked" flag, which is
> probably not what we want. Let's see if we can agree on these semantics
> for a "locked" entry:

The next version of this will block forwarding to locked entries in the 
bridge, so they will behave like the zero-DPV entries.

> 
> 1. It discards packets with matching DMAC, regardless of ingress port. 
> I
> read the document [1] you linked to in a different reply and could not
> find anything against this approach, so this might be fine or at least
> not very significant.
> 
> Note that this means that "locked" entries need to be notified to 
> device
> drivers so that they will install a matching entry in the HW FDB.

Okay, so as V4 does (just without the error noted).

> 
> 2. It is not refreshed and has ageing enabled. That is, after initial
> installation it will be removed by the bridge driver after configured
> ageing time unless converted to a regular (unlocked) entry.
> 
> I assume this allows you to remove the timer implementation from your
> driver and let the bridge driver notify you about the removal of this
> entry.

Okay, but only if the scheme is not so that the driver creates the 
locked entries itself, unless you indicate that the driver notifies the 
bridge, which then notifies back to the driver and installs the zero-DPV 
entry? If not I think the current implementation for the mv88e6xxx is 
fine.

> 
> 3. With regards to roaming, the entry cannot roam between locked ports
> (they need to have learning disabled anyway), but can roam to an
> unlocked port, in which case it becomes a regular entry that can roam
> and age.
> 
> If we agree on these semantics, then I can try to verify that at least
> Spectrum can support them (it seems mv88e6xxx can).

The consensus here is that at least for the mv88e6xxx, learning should 
be on and link local learning should be blocked by the userspace setting 
you pointed to earlier.

> 
> P.S. Sorry for the delay, I'm busy with other tasks at the moment.

I understand :-)

> 
> [1] 
> https://www.cisco.com/c/en/us/td/docs/solutions/Enterprise/Security/TrustSec_1-99/MAB/MAB_Dep_Guide.html#wp392522

Re: [PATCH v4 net-next 3/6] drivers: net: dsa: add locked fdb entry flag to drivers

[Ido Schimmel]

On Tue, Aug 09, 2022 at 10:00:49PM +0200, netdev@kapio-technology.com wrote:
> On 2022-08-09 11:20, Ido Schimmel wrote:
> > On Mon, Aug 01, 2022 at 05:33:49PM +0200, netdev@kapio-technology.com
> > wrote:
> > > On 2022-07-13 14:39, Ido Schimmel wrote:
> > > 
> > > >
> > > > What are "Storm Prevention" and "zero-DPV" FDB entries?
> > > >
> > > 
> > > For the zero-DPV entries, I can summarize:
> > > 
> > > Since a CPU can become saturated from constant SA Miss Violations
> > > from a
> > > denied source, source MAC address are masked by loading a zero-DPV
> > > (Destination Port Vector) entry in the ATU. As the address now
> > > appears in
> > > the database it will not cause more Miss Violations. ANY port trying
> > > to send
> > > a frame to this unauthorized address is discarded. Any locked port
> > > trying to
> > > use this unauthorized address has its frames discarded too (as the
> > > ports SA
> > > bit is not set in the ATU entry).
> > 
> > What happens to unlocked ports that have learning enabled and are trying
> > to use this address as SMAC? AFAICT, at least in the bridge driver, the
> > locked entry will roam, but will keep the "locked" flag, which is
> > probably not what we want. Let's see if we can agree on these semantics
> > for a "locked" entry:
> 
> The next version of this will block forwarding to locked entries in the
> bridge, so they will behave like the zero-DPV entries.

I'm talking about roaming, not forwarding. Let's say you have a locked
entry with MAC X pointing to port Y. Now you get a packet with SMAC X
from port Z which is unlocked. Will the FDB entry roam to port Z? I
think it should, but at least in current implementation it seems that
the "locked" flag will not be reset and having locked entries pointing
to an unlocked port looks like a bug.

> 
> > 
> > 1. It discards packets with matching DMAC, regardless of ingress port. I
> > read the document [1] you linked to in a different reply and could not
> > find anything against this approach, so this might be fine or at least
> > not very significant.
> > 
> > Note that this means that "locked" entries need to be notified to device
> > drivers so that they will install a matching entry in the HW FDB.
> 
> Okay, so as V4 does (just without the error noted).
> 
> > 
> > 2. It is not refreshed and has ageing enabled. That is, after initial
> > installation it will be removed by the bridge driver after configured
> > ageing time unless converted to a regular (unlocked) entry.
> > 
> > I assume this allows you to remove the timer implementation from your
> > driver and let the bridge driver notify you about the removal of this
> > entry.
> 
> Okay, but only if the scheme is not so that the driver creates the locked
> entries itself, unless you indicate that the driver notifies the bridge,
> which then notifies back to the driver and installs the zero-DPV entry? If
> not I think the current implementation for the mv88e6xxx is fine.

I don't see a problem in having the driver notifying the bridge about
the installation of this entry and the bridge notifying the driver that
the entry needs to be removed. It removes complexity from device drivers
like mv88e6xxx and doesn't add extra complexity to the bridge driver.

Actually, there is one complication, 'SWITCHDEV_FDB_ADD_TO_BRIDGE' will
add the locked entry as externally learned, which means the bridge will
not age it. Might need something like this:

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index e7f4fccb6adb..5f73d0b44ed9 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -530,7 +530,8 @@ void br_fdb_cleanup(struct work_struct *work)
 		unsigned long this_timer = f->updated + delay;
 
 		if (test_bit(BR_FDB_STATIC, &f->flags) ||
-		    test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &f->flags)) {
+		    (test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &f->flags) &&
+		     !test_bit(BR_FDB_ENTRY_LOCKED, &f->flags))) {
 			if (test_bit(BR_FDB_NOTIFY, &f->flags)) {
 				if (time_after(this_timer, now))
 					work_delay = min(work_delay,

> 
> > 
> > 3. With regards to roaming, the entry cannot roam between locked ports
> > (they need to have learning disabled anyway), but can roam to an
> > unlocked port, in which case it becomes a regular entry that can roam
> > and age.
> > 
> > If we agree on these semantics, then I can try to verify that at least
> > Spectrum can support them (it seems mv88e6xxx can).
> 
> The consensus here is that at least for the mv88e6xxx, learning should be on
> and link local learning should be blocked by the userspace setting you
> pointed to earlier.

Why learning needs to be on in the bridge (not mv88e6xxx) driver?

Re: [PATCH v4 net-next 3/6] drivers: net: dsa: add locked fdb entry flag to drivers

[netdev]

On 2022-08-10 09:21, Ido Schimmel wrote:
> On Tue, Aug 09, 2022 at 10:00:49PM +0200, netdev@kapio-technology.com 
> wrote:
>> On 2022-08-09 11:20, Ido Schimmel wrote:
>> > On Mon, Aug 01, 2022 at 05:33:49PM +0200, netdev@kapio-technology.com
>> > wrote:
>> > > On 2022-07-13 14:39, Ido Schimmel wrote:
>> > >
>> > > >
>> > > > What are "Storm Prevention" and "zero-DPV" FDB entries?
>> > > >
>> > >
>> > > For the zero-DPV entries, I can summarize:
>> > >
>> > > Since a CPU can become saturated from constant SA Miss Violations
>> > > from a
>> > > denied source, source MAC address are masked by loading a zero-DPV
>> > > (Destination Port Vector) entry in the ATU. As the address now
>> > > appears in
>> > > the database it will not cause more Miss Violations. ANY port trying
>> > > to send
>> > > a frame to this unauthorized address is discarded. Any locked port
>> > > trying to
>> > > use this unauthorized address has its frames discarded too (as the
>> > > ports SA
>> > > bit is not set in the ATU entry).
>> >
>> > What happens to unlocked ports that have learning enabled and are trying
>> > to use this address as SMAC? AFAICT, at least in the bridge driver, the
>> > locked entry will roam, but will keep the "locked" flag, which is
>> > probably not what we want. Let's see if we can agree on these semantics
>> > for a "locked" entry:
>> 
>> The next version of this will block forwarding to locked entries in 
>> the
>> bridge, so they will behave like the zero-DPV entries.
> 
> I'm talking about roaming, not forwarding. Let's say you have a locked
> entry with MAC X pointing to port Y. Now you get a packet with SMAC X
> from port Z which is unlocked. Will the FDB entry roam to port Z? I
> think it should, but at least in current implementation it seems that
> the "locked" flag will not be reset and having locked entries pointing
> to an unlocked port looks like a bug.
> 

Remember that zero-DPV entries blackhole (mask) the MAC, so whenever a 
packet appears with the same MAC on another port it is just dropped in 
the HW, so there is no possibility of doing any CPU processing in this 
case. Only after the timeout (5 min) can the MAC get a normal ATU on an 
open port.
For the bridge to do what you suggest, a FDB search would be needed 
afaics, and this might be in a process sensitive part of the code, thus 
leading to too heavy a cost.

>> 
>> >
>> > 1. It discards packets with matching DMAC, regardless of ingress port. I
>> > read the document [1] you linked to in a different reply and could not
>> > find anything against this approach, so this might be fine or at least
>> > not very significant.
>> >
>> > Note that this means that "locked" entries need to be notified to device
>> > drivers so that they will install a matching entry in the HW FDB.
>> 
>> Okay, so as V4 does (just without the error noted).
>> 
>> >
>> > 2. It is not refreshed and has ageing enabled. That is, after initial
>> > installation it will be removed by the bridge driver after configured
>> > ageing time unless converted to a regular (unlocked) entry.
>> >
>> > I assume this allows you to remove the timer implementation from your
>> > driver and let the bridge driver notify you about the removal of this
>> > entry.
>> 
>> Okay, but only if the scheme is not so that the driver creates the 
>> locked
>> entries itself, unless you indicate that the driver notifies the 
>> bridge,
>> which then notifies back to the driver and installs the zero-DPV 
>> entry? If
>> not I think the current implementation for the mv88e6xxx is fine.
> 
> I don't see a problem in having the driver notifying the bridge about
> the installation of this entry and the bridge notifying the driver that
> the entry needs to be removed. It removes complexity from device 
> drivers
> like mv88e6xxx and doesn't add extra complexity to the bridge driver.
> 
> Actually, there is one complication, 'SWITCHDEV_FDB_ADD_TO_BRIDGE' will
> add the locked entry as externally learned, which means the bridge will
> not age it. Might need something like this:
> 
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index e7f4fccb6adb..5f73d0b44ed9 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -530,7 +530,8 @@ void br_fdb_cleanup(struct work_struct *work)
>  		unsigned long this_timer = f->updated + delay;
> 
>  		if (test_bit(BR_FDB_STATIC, &f->flags) ||
> -		    test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &f->flags)) {
> +		    (test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &f->flags) &&
> +		     !test_bit(BR_FDB_ENTRY_LOCKED, &f->flags))) {
>  			if (test_bit(BR_FDB_NOTIFY, &f->flags)) {
>  				if (time_after(this_timer, now))
>  					work_delay = min(work_delay,
> 

There is a case of ownership of the FDB/ATU entry, which if I remember 
correctly, will point to the current implementation being the right way 
to do it, thus having the driver keeping ownership of the entry and 
thereby also ageing it, but I think Vladimir should have his say here.

>> 
>> >
>> > 3. With regards to roaming, the entry cannot roam between locked ports
>> > (they need to have learning disabled anyway), but can roam to an
>> > unlocked port, in which case it becomes a regular entry that can roam
>> > and age.
>> >
>> > If we agree on these semantics, then I can try to verify that at least
>> > Spectrum can support them (it seems mv88e6xxx can).
>> 
>> The consensus here is that at least for the mv88e6xxx, learning should 
>> be on
>> and link local learning should be blocked by the userspace setting you
>> pointed to earlier.
> 
> Why learning needs to be on in the bridge (not mv88e6xxx) driver?

I think it is seen as 'cheating' to enable learning only in the driver 
behind the scenes, so kind of hackish. E.g. 'bridge -d link show' will 
then report 'learning off', while learning is on in the driver.
And learning needs to be on for the driver as discussed earlier, which 
only gives rise to the link local learning problem, which is then solved 
by the user space setting.

Re: [PATCH v4 net-next 0/6] Extend locked port feature with FDB locked flag (MAC-Auth/MAB)

[Benjamin Poirier]

On 2022-07-07 17:29 +0200, Hans Schultz wrote:
> This patch set extends the locked port feature for devices
> that are behind a locked port, but do not have the ability to
> authorize themselves as a supplicant using IEEE 802.1X.
> Such devices can be printers, meters or anything related to
> fixed installations. Instead of 802.1X authorization, devices
> can get access based on their MAC addresses being whitelisted.
                                                    ^

Please consider using the alternate vocabulary discussed in
Documentation/process/coding-style.rst §4.

Re: [PATCH v4 net-next 3/6] drivers: net: dsa: add locked fdb entry flag to drivers

[Ido Schimmel]

On Wed, Aug 10, 2022 at 10:40:45AM +0200, netdev@kapio-technology.com wrote:
> On 2022-08-10 09:21, Ido Schimmel wrote:
> > On Tue, Aug 09, 2022 at 10:00:49PM +0200, netdev@kapio-technology.com
> > wrote:
> > > On 2022-08-09 11:20, Ido Schimmel wrote:
> > > > On Mon, Aug 01, 2022 at 05:33:49PM +0200, netdev@kapio-technology.com
> > > > wrote:
> > > > > On 2022-07-13 14:39, Ido Schimmel wrote:
> > > > >
> > > > > >
> > > > > > What are "Storm Prevention" and "zero-DPV" FDB entries?
> > > > > >
> > > > >
> > > > > For the zero-DPV entries, I can summarize:
> > > > >
> > > > > Since a CPU can become saturated from constant SA Miss Violations
> > > > > from a
> > > > > denied source, source MAC address are masked by loading a zero-DPV
> > > > > (Destination Port Vector) entry in the ATU. As the address now
> > > > > appears in
> > > > > the database it will not cause more Miss Violations. ANY port trying
> > > > > to send
> > > > > a frame to this unauthorized address is discarded. Any locked port
> > > > > trying to
> > > > > use this unauthorized address has its frames discarded too (as the
> > > > > ports SA
> > > > > bit is not set in the ATU entry).
> > > >
> > > > What happens to unlocked ports that have learning enabled and are trying
> > > > to use this address as SMAC? AFAICT, at least in the bridge driver, the
> > > > locked entry will roam, but will keep the "locked" flag, which is
> > > > probably not what we want. Let's see if we can agree on these semantics
> > > > for a "locked" entry:
> > > 
> > > The next version of this will block forwarding to locked entries in
> > > the
> > > bridge, so they will behave like the zero-DPV entries.
> > 
> > I'm talking about roaming, not forwarding. Let's say you have a locked
> > entry with MAC X pointing to port Y. Now you get a packet with SMAC X
> > from port Z which is unlocked. Will the FDB entry roam to port Z? I
> > think it should, but at least in current implementation it seems that
> > the "locked" flag will not be reset and having locked entries pointing
> > to an unlocked port looks like a bug.
> > 
> 
> Remember that zero-DPV entries blackhole (mask) the MAC, so whenever a
> packet appears with the same MAC on another port it is just dropped in the

What do you mean by "same MAC"? As SMAC or DMAC? I'm talking about SMAC
and when the packet is received via an unlocked port. Why would such a
packet be dropped?

> HW, so there is no possibility of doing any CPU processing in this case.
> Only after the timeout (5 min) can the MAC get a normal ATU on an open port.
> For the bridge to do what you suggest, a FDB search would be needed afaics,
> and this might be in a process sensitive part of the code, thus leading to
> too heavy a cost.

When learning is enabled the bridge already performs a lookup on the
SMAC.

TBH, I don't think this is progressing well because there is too much
discrepancy between how this feature works in the bridge driver and in
the hardware you work with.

I suggest to first define the model in the bridge driver and then take
care of the offload. My suggestion is to send another RFC with only the
bridge changes with emphasize on the following aspects:

* Forwarding rules for "locked" entries. Do they drop matching packets?
Forward them? Or not considered at all for forwarding?

* Roaming rules for "locked" entries. Can they roam between locked
ports? Can they roam from a locked port to an unlocked port and
vice versa? Or are they completely sticky?

* Ageing rule for "locked" entries. Are these entries subject to the
ageing time or are they static? If they are not static, are they
refreshed by incoming traffic from a locked port or not?

* MAB enablement. New option? Overload an existing one? No option?

The commit messages should explain these design choices and new tests
cases should verify the desired behavior.

Once we have an agreement we can work out the switchdev/mv88e6xxx parts
and eventually the entire thing can be merged together. Fair?

Re: [PATCH v4 net-next 3/6] drivers: net: dsa: add locked fdb entry flag to drivers

[netdev]

On 2022-08-11 13:28, Ido Schimmel wrote:
>> >
>> > I'm talking about roaming, not forwarding. Let's say you have a locked
>> > entry with MAC X pointing to port Y. Now you get a packet with SMAC X
>> > from port Z which is unlocked. Will the FDB entry roam to port Z? I
>> > think it should, but at least in current implementation it seems that
>> > the "locked" flag will not be reset and having locked entries pointing
>> > to an unlocked port looks like a bug.
>> >

Yes, now I have tried to test with a case like this using the bridge and 
have verified the locked entry pointing to an unlocked port, which I 
agree seems to be a bug, which I will get fixed.

Re: [PATCH v4 net-next 3/6] drivers: net: dsa: add locked fdb entry flag to drivers

[netdev]

On 2022-08-10 09:21, Ido Schimmel wrote:
>> >
>> > 1. It discards packets with matching DMAC, regardless of ingress port. I
>> > read the document [1] you linked to in a different reply and could not
>> > find anything against this approach, so this might be fine or at least
>> > not very significant.
>> >
>> > Note that this means that "locked" entries need to be notified to device
>> > drivers so that they will install a matching entry in the HW FDB.
>> 

I just want to be completely sure as what should be done in version 5 
with locked entries from the bridge, as - if I should implement it so 
that they are sent to all the drivers, and the drivers then ignore them 
if they don't need to take action? (for the mv88e6xxx driver, it does 
not need them and can ignore but other drivers might need.)

Re: [PATCH v4 net-next 3/6] drivers: net: dsa: add locked fdb entry flag to drivers

[Ido Schimmel]

On Tue, Aug 16, 2022 at 09:51:32AM +0200, netdev@kapio-technology.com wrote:
> On 2022-08-10 09:21, Ido Schimmel wrote:
> > > >
> > > > 1. It discards packets with matching DMAC, regardless of ingress port. I
> > > > read the document [1] you linked to in a different reply and could not
> > > > find anything against this approach, so this might be fine or at least
> > > > not very significant.
> > > >
> > > > Note that this means that "locked" entries need to be notified to device
> > > > drivers so that they will install a matching entry in the HW FDB.
> > > 
> 
> I just want to be completely sure as what should be done in version 5 with
> locked entries from the bridge, as - if I should implement it so that they
> are sent to all the drivers, and the drivers then ignore them if they don't
> need to take action? (for the mv88e6xxx driver, it does not need them and
> can ignore but other drivers might need.)

Yes, I think that would be best. At least when mlxsw starts supporting
MAB it will need to program the locked entry as an FDB with discard
action.

To be clear, I'm aware that all drivers other than mv88e6xxx currently
forbid a port from being locked, making it unlikely that they will
receive such notifications, but if you do it now then we will not need
more changes in the bridge when other drivers gain support for
802.1X/MAB.

Re: [PATCH v4 net-next 5/6] net: dsa: mv88e6xxx: mac-auth/MAB implementation

[netdev]

On 2022-07-17 02:47, Vladimir Oltean wrote:
>> @@ -1721,11 +1735,11 @@ static int mv88e6xxx_vtu_get(struct 
>> mv88e6xxx_chip *chip, u16 vid,
>>  	return err;
>>  }
>> 
>>  int mv88e6xxx_port_set_lock(struct mv88e6xxx_chip *chip, int port,
>>  			    bool locked)
>>  {
>> @@ -1257,13 +1270,18 @@ int mv88e6xxx_port_set_lock(struct 
>> mv88e6xxx_chip *chip, int port,
>>  	if (err)
>>  		return err;
>> 
>> -	err = mv88e6xxx_port_read(chip, port, MV88E6XXX_PORT_ASSOC_VECTOR, 
>> &reg);
>> -	if (err)
>> -		return err;
>> +	if (!locked) {
>> +		err = mv88e6xxx_atu_locked_entry_flush(chip->ds, port);
> 
> Did you re-run the selftest with v4? Because this deadlocks due to the
> double reg_lock illustrated below:

I did some selftest, but I didn't experience the problem of the double 
chip lock as it only happens on unlock which is only called where the 
MAB test ends.

> 
> + vrf_name=vlan1
> + ip vrf exec vlan1 ping 192.0.2.2 -c 10 -i 0.1 -w 5
> + check_err 1 'Ping did not work after locking port and adding FDB 
> entry'
> + local err=1
> + local 'msg=Ping did not work after locking port and adding FDB entry'
> + [[ 0 -eq 0 ]]
> + [[ 1 -ne 0 ]]
> + RET=1
> + retmsg='Ping did not work after locking port and adding FDB entry'
> + bridge link set dev lan2 locked off
> [  733.273994]
> [  733.275515] ============================================
> [  733.280823] WARNING: possible recursive locking detected
> [  733.286133] 5.19.0-rc6-07010-ga9b9500ffaac-dirty #3293 Not tainted
> [  733.292311] --------------------------------------------
> [  733.297613] kworker/0:0/601 is trying to acquire lock:
> [  733.302751] ffff00000213c110 (&chip->reg_lock){+.+.}-{4:4}, at:
> mv88e6xxx_atu_locked_entry_purge+0x70/0x1a4
> [  733.312524]
> [  733.312524] but task is already holding lock:
> [  733.318351] ffff00000213c110 (&chip->reg_lock){+.+.}-{4:4}, at:
> mv88e6xxx_port_bridge_flags+0x48/0x234
> [  733.327674]
> [  733.327674] other info that might help us debug this:
> [  733.334198]  Possible unsafe locking scenario:
> [  733.334198]
> [  733.340109]        CPU0
> [  733.342549]        ----
> [  733.344990]   lock(&chip->reg_lock);
> [  733.348567]   lock(&chip->reg_lock);
> [  733.352149]
> [  733.352149]  *** DEADLOCK ***
> [  733.352149]
> [  733.358063]  May be due to missing lock nesting notation
> [  733.358063]
> [  733.364846] 6 locks held by kworker/0:0/601:
> [  733.369115]  #0: ffff00000000b748
> ((wq_completion)events){+.+.}-{0:0}, at: process_one_work+0x1f4/0x6c4
> [  733.378541]  #1: ffff80000c43bdc8
> (deferred_process_work){+.+.}-{0:0}, at: process_one_work+0x1f4/0x6c4
> [  733.387966]  #2: ffff80000b020cb0 (rtnl_mutex){+.+.}-{4:4}, at:
> rtnl_lock+0x1c/0x30
> [  733.395660]  #3: ffff80000b073370
> ((switchdev_blocking_notif_chain).rwsem){++++}-{4:4}, at:
> blocking_notifier_call_chain+0x34/0xa0
> [  733.407432]  #4: ffff00000213c110 (&chip->reg_lock){+.+.}-{4:4},
> at: mv88e6xxx_port_bridge_flags+0x48/0x234
> [  733.417202]  #5: ffff00000213e0f0 (&p->ale_list_lock){+.+.}-{4:4},
> at: mv88e6xxx_atu_locked_entry_flush+0x4c/0xc0
> [  733.427495]
> [  733.427495] stack backtrace:
> [  733.431858] CPU: 0 PID: 601 Comm: kworker/0:0 Not tainted
> 5.19.0-rc6-07010-ga9b9500ffaac-dirty #3293
> [  733.440992] Hardware name: CZ.NIC Turris Mox Board (DT)
> [  733.446219] Workqueue: events switchdev_deferred_process_work
> [  733.451982] Call trace:
> [  733.454424]  dump_backtrace.part.0+0xcc/0xe0
> [  733.458702]  show_stack+0x18/0x6c
> [  733.462028]  dump_stack_lvl+0x8c/0xb8
> [  733.465703]  dump_stack+0x18/0x34
> [  733.469029]  __lock_acquire+0x1074/0x1fd0
> [  733.473052]  lock_acquire.part.0+0xe4/0x220
> [  733.477244]  lock_acquire+0x68/0x8c
> [  733.480744]  __mutex_lock+0x9c/0x460
> [  733.484332]  mutex_lock_nested+0x40/0x50
> [  733.488268]  mv88e6xxx_atu_locked_entry_purge+0x70/0x1a4
> [  733.493592]  mv88e6xxx_atu_locked_entry_flush+0x7c/0xc0
> [  733.498827]  mv88e6xxx_port_set_lock+0xfc/0x10c
> [  733.503374]  mv88e6xxx_port_bridge_flags+0x200/0x234
> [  733.508351]  dsa_port_bridge_flags+0x44/0xe0
> [  733.512635]  dsa_slave_port_attr_set+0x1ec/0x230
> [  733.517268]  __switchdev_handle_port_attr_set+0x58/0x100
> [  733.522594]  switchdev_handle_port_attr_set+0x10/0x24
> [  733.527659]  dsa_slave_switchdev_blocking_event+0x8c/0xd4
> [  733.533074]  blocking_notifier_call_chain+0x6c/0xa0
> [  733.537968]  switchdev_port_attr_notify.constprop.0+0x4c/0xb0
> [  733.543729]  switchdev_port_attr_set_deferred+0x24/0x80
> [  733.548967]  switchdev_deferred_process+0x90/0x164
> [  733.553774]  switchdev_deferred_process_work+0x14/0x2c
> [  733.558926]  process_one_work+0x28c/0x6c4
> [  733.562949]  worker_thread+0x74/0x450
> [  733.566623]  kthread+0x118/0x11c
> [  733.569860]  ret_from_fork+0x10/0x20
> ++ mac_get lan1
> ++ local if_name=lan1
> ++ jq -r '.[]["address"]'
> ++ ip -j link show dev lan1
> 
> I've tentatively fixed this in my tree by taking the register lock more
> localized to the sub-functions of mv88e6xxx_port_bridge_flags(), and 
> not
> taking it at caller side for mv88e6xxx_port_set_lock(), but rather
> letting the callee take it:

Yes, the double chip lock came from calling 
mv88e6xxx_atu_locked_entry_flush() in mv88e6xxx_port_set_lock() that is 
called from mv88e6xxx_port_bridge_flags() whic has the lock taken. I 
have fixed this now, but when I unlock a port with locked entries, I 
cannot have notify the bridge to clear the same locked entries by having 
mv88e6xxx_atu_locked_entry_flush() calling with the notify on, thus 
taking the rtnl lock, as this also results in a double lock.
To remove the locked entries in the bridge FDB it is then necessary as I 
see it, to take the link down or have a userspace daemon clear them. I 
hope that is okay if documented?

> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c 
> b/drivers/net/dsa/mv88e6xxx/chip.c
> index 7b57ac121589..ec5954f32774 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -6557,41 +6557,47 @@ static int mv88e6xxx_port_bridge_flags(struct
> dsa_switch *ds, int port,
>  	struct mv88e6xxx_chip *chip = ds->priv;
>  	int err = -EOPNOTSUPP;
> 
> -	mv88e6xxx_reg_lock(chip);
> -
>  	if (flags.mask & BR_LEARNING) {
>  		bool learning = !!(flags.val & BR_LEARNING);
>  		u16 pav = learning ? (1 << port) : 0;
> 
> +		mv88e6xxx_reg_lock(chip);
>  		err = mv88e6xxx_port_set_assoc_vector(chip, port, pav);
> +		mv88e6xxx_reg_unlock(chip);
>  		if (err)
> -			goto out;
> +			return err;
>  	}
> 
>  	if (flags.mask & BR_FLOOD) {
>  		bool unicast = !!(flags.val & BR_FLOOD);
> 
> +		mv88e6xxx_reg_lock(chip);
>  		err = chip->info->ops->port_set_ucast_flood(chip, port,
>  							    unicast);
> +		mv88e6xxx_reg_unlock(chip);
>  		if (err)
> -			goto out;
> +			return err;
>  	}
> 
>  	if (flags.mask & BR_MCAST_FLOOD) {
>  		bool multicast = !!(flags.val & BR_MCAST_FLOOD);
> 
> +		mv88e6xxx_reg_lock(chip);
>  		err = chip->info->ops->port_set_mcast_flood(chip, port,
>  							    multicast);
> +		mv88e6xxx_reg_unlock(chip);
>  		if (err)
> -			goto out;
> +			return err;
>  	}
> 
>  	if (flags.mask & BR_BCAST_FLOOD) {
>  		bool broadcast = !!(flags.val & BR_BCAST_FLOOD);
> 
> +		mv88e6xxx_reg_lock(chip);
>  		err = mv88e6xxx_port_broadcast_sync(chip, port, broadcast);
> +		mv88e6xxx_reg_unlock(chip);
>  		if (err)
> -			goto out;
> +			return err;
>  	}
> 
>  	if (flags.mask & BR_PORT_LOCKED) {
> @@ -6599,10 +6605,8 @@ static int mv88e6xxx_port_bridge_flags(struct
> dsa_switch *ds, int port,
> 
>  		err = mv88e6xxx_port_set_lock(chip, port, locked);
>  		if (err)
> -			goto out;
> +			return err;
>  	}
> -out:
> -	mv88e6xxx_reg_unlock(chip);
> 
>  	return err;
>  }
> diff --git a/drivers/net/dsa/mv88e6xxx/port.c 
> b/drivers/net/dsa/mv88e6xxx/port.c
> index 5e4d5e9265a4..2a60aded6fbe 100644
> --- a/drivers/net/dsa/mv88e6xxx/port.c
> +++ b/drivers/net/dsa/mv88e6xxx/port.c
> @@ -1222,15 +1222,19 @@ int mv88e6xxx_port_set_lock(struct
> mv88e6xxx_chip *chip, int port,
>  	u16 reg;
>  	int err;
> 
> +	mv88e6xxx_reg_lock(chip);
>  	err = mv88e6xxx_port_read(chip, port, MV88E6XXX_PORT_CTL0, &reg);
> -	if (err)
> +	if (err) {
> +		mv88e6xxx_reg_unlock(chip);
>  		return err;
> +	}
> 
>  	reg &= ~MV88E6XXX_PORT_CTL0_SA_FILT_MASK;
>  	if (locked)
>  		reg |= MV88E6XXX_PORT_CTL0_SA_FILT_DROP_ON_LOCK;
> 
>  	err = mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_CTL0, reg);
> +	mv88e6xxx_reg_unlock(chip);
>  	if (err)
>  		return err;
> 
> @@ -1247,7 +1251,11 @@ int mv88e6xxx_port_set_lock(struct
> mv88e6xxx_chip *chip, int port,
>  			MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT;
>  	}
> 
> -	return mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_ASSOC_VECTOR, 
> reg);
> +	mv88e6xxx_reg_lock(chip);
> +	err = mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_ASSOC_VECTOR, 
> reg);
> +	mv88e6xxx_reg_unlock(chip);
> +
> +	return err;
>  }
> 
>  int mv88e6xxx_port_set_8021q_mode(struct mv88e6xxx_chip *chip, int 
> port,
> 
>> +		if (err)
>> +			return err;
>> +	}
>> 
>> -	reg &= ~MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT;
>> -	if (locked)
>> -		reg |= MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT;
>> +	reg = 0;
>> +	if (locked) {
>> +		reg = (1 << port);
>> +		reg |= MV88E6XXX_PORT_ASSOC_VECTOR_IGNORE_WRONG |
>> +			MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT;
>> +	}
>> 
>>  	return mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_ASSOC_VECTOR, 
>> reg);
>>  }
>> diff --git a/drivers/net/dsa/mv88e6xxx/port.h 
>> b/drivers/net/dsa/mv88e6xxx/port.h
>> index e0a705d82019..5c1d485e7442 100644
>> --- a/drivers/net/dsa/mv88e6xxx/port.h
>> +++ b/drivers/net/dsa/mv88e6xxx/port.h
>> @@ -231,6 +231,7 @@
>>  #define MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT		0x2000
>>  #define MV88E6XXX_PORT_ASSOC_VECTOR_IGNORE_WRONG	0x1000
>>  #define MV88E6XXX_PORT_ASSOC_VECTOR_REFRESH_LOCKED	0x0800
>> +#define MV88E6XXX_PORT_ASSOC_VECTOR_PAV_MASK		0x07ff
>> 
>>  /* Offset 0x0C: Port ATU Control */
>>  #define MV88E6XXX_PORT_ATU_CTL		0x0c
>> @@ -374,6 +375,7 @@ int mv88e6xxx_port_set_fid(struct mv88e6xxx_chip 
>> *chip, int port, u16 fid);
>>  int mv88e6xxx_port_get_pvid(struct mv88e6xxx_chip *chip, int port, 
>> u16 *pvid);
>>  int mv88e6xxx_port_set_pvid(struct mv88e6xxx_chip *chip, int port, 
>> u16 pvid);
>> 
>> +bool mv88e6xxx_port_is_locked(struct mv88e6xxx_chip *chip, int port);
>>  int mv88e6xxx_port_set_lock(struct mv88e6xxx_chip *chip, int port,
>>  			    bool locked);
>>