[RFC net-next (resend) 0/4] Send notifications for roaming hosts

[RFC net-next (resend) 0/4] Send notifications for roaming hosts

[Elliot Ayrey]

Apologies, this is a resend as the first version didn't have the correct CCs.


For 802.1x operation it is useful to know when a host authorised to on port
has roamed to another port, and take action on it.

The fdb sticky flag is used to configure a host for a single port and
prohibit it from moving to another, so this flag is used as the means of
determining when to send the notification.

In this set of patches the fdb notify mechanism is extended to including a
roaming bit, so user applications can configure an fdb entry for a host as
sticky (likely also static) and turn on notifications. With this
configuration, if traffic matching this entry appears on another port the
entry will not be updated but a notification will be sent to userspace.

This is achieved by temporarily updating the fdb entry with the new port,
setting the roaming bit, firing off a notification, and restoring the
original port. The port remains unchanged but userspace is now notified of
the new port the host was seen on.

For this specific implementation the mv88e6xxx chip's member-violation
interrupt is used to inform the kernel bridge via switchdev that a host is
roaming to a new port. For this to work the br_fdb_external_learn_add()
function has been changed to respect the stick flag where it previously
wasn't.

Elliot Ayrey (4):
  net: bridge: respect sticky flag on external learn
  net: bridge: send notification for roaming hosts
  net: dsa: mv88e6xxx: handle member-violations
  net: dsa: mv88e6xxx: cache fid-to-vid association

 drivers/net/dsa/mv88e6xxx/chip.h        |  2 +
 drivers/net/dsa/mv88e6xxx/global1_atu.c | 37 +++++++++++++++
 drivers/net/dsa/mv88e6xxx/global1_vtu.c |  6 ++-
 drivers/net/dsa/mv88e6xxx/switchdev.c   | 60 ++++++++++++------------
 drivers/net/dsa/mv88e6xxx/switchdev.h   |  2 +
 include/uapi/linux/neighbour.h          |  4 +-
 net/bridge/br_fdb.c                     | 61 +++++++++++++++++--------
 net/bridge/br_input.c                   |  9 +++-
 net/bridge/br_private.h                 |  3 ++
 9 files changed, 132 insertions(+), 52 deletions(-)

[RFC net-next (resend) 1/4] net: bridge: respect sticky flag on external learn

[Elliot Ayrey]

The fdb sticky flag is used to stop a host from roaming to another
port. However upon receiving a switchdev notification to update an fdb
entry the sticky flag is not respected and as long as the new entry is
not locked the host will be allowed to roam to the new port.

Fix this by considering the sticky flag before allowing an externally
learned host to roam.

Signed-off-by: Elliot Ayrey <elliot.ayrey@alliedtelesis.co.nz>
---
 net/bridge/br_fdb.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 1cd7bade9b3b..72663ca824d3 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -1457,7 +1457,8 @@ int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
 
 		fdb->updated = jiffies;
 
-		if (READ_ONCE(fdb->dst) != p) {
+		if (READ_ONCE(fdb->dst) != p &&
+		    !test_bit(BR_FDB_STICKY, &fdb->flags)) {
 			WRITE_ONCE(fdb->dst, p);
 			modified = true;
 		}

[RFC net-next (resend) 2/4] net: bridge: send notification for roaming hosts

[Elliot Ayrey]

When an fdb entry is configured as static and sticky it should never
roam. However there are times where it would be useful to know when
this happens so a user application can act on it. For this reason,
extend the fdb notification mechanism to send a notification when the
bridge detects a host that is attempting to roam when it has been
configured not to.

This is achieved by temporarily updating the fdb entry with the new
port, setting a new notify roaming bit, firing off a notification, and
restoring the original port immediately afterwards. The port remains
unchanged, respecting the sticky flag, but userspace is now notified
of the new port the host was seen on.

The roaming bit is cleared if the entry becomes inactive or if it is
replaced by a user entry.

Signed-off-by: Elliot Ayrey <elliot.ayrey@alliedtelesis.co.nz>
---
 include/uapi/linux/neighbour.h |  4 ++-
 net/bridge/br_fdb.c            | 64 +++++++++++++++++++++++-----------
 net/bridge/br_input.c          | 10 ++++--
 net/bridge/br_private.h        |  3 ++
 4 files changed, 58 insertions(+), 23 deletions(-)

diff --git a/include/uapi/linux/neighbour.h b/include/uapi/linux/neighbour.h
index 5e67a7eaf4a7..e1c686268808 100644
--- a/include/uapi/linux/neighbour.h
+++ b/include/uapi/linux/neighbour.h
@@ -201,10 +201,12 @@ enum {
  /* FDB activity notification bits used in NFEA_ACTIVITY_NOTIFY:
   * - FDB_NOTIFY_BIT - notify on activity/expire for any entry
   * - FDB_NOTIFY_INACTIVE_BIT - mark as inactive to avoid multiple notifications
+  * - FDB_NOTIFY_ROAMING_BIT - mark as attempting to roam
   */
 enum {
 	FDB_NOTIFY_BIT		= (1 << 0),
-	FDB_NOTIFY_INACTIVE_BIT	= (1 << 1)
+	FDB_NOTIFY_INACTIVE_BIT	= (1 << 1),
+	FDB_NOTIFY_ROAMING_BIT	= (1 << 2)
 };
 
 /* embedded into NDA_FDB_EXT_ATTRS:
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 72663ca824d3..a8b841e74e15 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -145,6 +145,8 @@ static int fdb_fill_info(struct sk_buff *skb, const struct net_bridge *br,
 			goto nla_put_failure;
 		if (test_bit(BR_FDB_NOTIFY_INACTIVE, &fdb->flags))
 			notify_bits |= FDB_NOTIFY_INACTIVE_BIT;
+		if (test_bit(BR_FDB_NOTIFY_ROAMING, &fdb->flags))
+			notify_bits |= FDB_NOTIFY_ROAMING_BIT;
 
 		if (nla_put_u8(skb, NFEA_ACTIVITY_NOTIFY, notify_bits)) {
 			nla_nest_cancel(skb, nest);
@@ -554,8 +556,10 @@ void br_fdb_cleanup(struct work_struct *work)
 					work_delay = min(work_delay,
 							 this_timer - now);
 				else if (!test_and_set_bit(BR_FDB_NOTIFY_INACTIVE,
-							   &f->flags))
+							   &f->flags)) {
+					clear_bit(BR_FDB_NOTIFY_ROAMING, &f->flags);
 					fdb_notify(br, f, RTM_NEWNEIGH, false);
+				}
 			}
 			continue;
 		}
@@ -880,6 +884,19 @@ static bool __fdb_mark_active(struct net_bridge_fdb_entry *fdb)
 		  test_and_clear_bit(BR_FDB_NOTIFY_INACTIVE, &fdb->flags));
 }
 
+void br_fdb_notify_roaming(struct net_bridge *br, struct net_bridge_port *p,
+			   struct net_bridge_fdb_entry *fdb)
+{
+	struct net_bridge_port *old_p = READ_ONCE(fdb->dst);
+
+	if (test_bit(BR_FDB_NOTIFY, &fdb->flags) &&
+	    !test_and_set_bit(BR_FDB_NOTIFY_ROAMING, &fdb->flags)) {
+		WRITE_ONCE(fdb->dst, p);
+		fdb_notify(br, fdb, RTM_NEWNEIGH, false);
+		WRITE_ONCE(fdb->dst, old_p);
+	}
+}
+
 void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
 		   const unsigned char *addr, u16 vid, unsigned long flags)
 {
@@ -906,21 +923,24 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
 			}
 
 			/* fastpath: update of existing entry */
-			if (unlikely(source != READ_ONCE(fdb->dst) &&
-				     !test_bit(BR_FDB_STICKY, &fdb->flags))) {
-				br_switchdev_fdb_notify(br, fdb, RTM_DELNEIGH);
-				WRITE_ONCE(fdb->dst, source);
-				fdb_modified = true;
-				/* Take over HW learned entry */
-				if (unlikely(test_bit(BR_FDB_ADDED_BY_EXT_LEARN,
-						      &fdb->flags)))
-					clear_bit(BR_FDB_ADDED_BY_EXT_LEARN,
-						  &fdb->flags);
-				/* Clear locked flag when roaming to an
-				 * unlocked port.
-				 */
-				if (unlikely(test_bit(BR_FDB_LOCKED, &fdb->flags)))
-					clear_bit(BR_FDB_LOCKED, &fdb->flags);
+			if (unlikely(source != READ_ONCE(fdb->dst))) {
+				if (unlikely(test_bit(BR_FDB_STICKY, &fdb->flags))) {
+					br_fdb_notify_roaming(br, source, fdb);
+				} else {
+					br_switchdev_fdb_notify(br, fdb, RTM_DELNEIGH);
+					WRITE_ONCE(fdb->dst, source);
+					fdb_modified = true;
+					/* Take over HW learned entry */
+					if (unlikely(test_bit(BR_FDB_ADDED_BY_EXT_LEARN,
+							      &fdb->flags)))
+						clear_bit(BR_FDB_ADDED_BY_EXT_LEARN,
+							  &fdb->flags);
+					/* Clear locked flag when roaming to an
+					 * unlocked port.
+					 */
+					if (unlikely(test_bit(BR_FDB_LOCKED, &fdb->flags)))
+						clear_bit(BR_FDB_LOCKED, &fdb->flags);
+				}
 			}
 
 			if (unlikely(test_bit(BR_FDB_ADDED_BY_USER, &flags))) {
@@ -1045,6 +1065,7 @@ static bool fdb_handle_notify(struct net_bridge_fdb_entry *fdb, u8 notify)
 		   test_and_clear_bit(BR_FDB_NOTIFY, &fdb->flags)) {
 		/* disabled activity tracking, clear notify state */
 		clear_bit(BR_FDB_NOTIFY_INACTIVE, &fdb->flags);
+		clear_bit(BR_FDB_NOTIFY_ROAMING, &fdb->flags);
 		modified = true;
 	}
 
@@ -1457,10 +1478,13 @@ int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
 
 		fdb->updated = jiffies;
 
-		if (READ_ONCE(fdb->dst) != p &&
-		    !test_bit(BR_FDB_STICKY, &fdb->flags)) {
-			WRITE_ONCE(fdb->dst, p);
-			modified = true;
+		if (READ_ONCE(fdb->dst) != p) {
+			if (test_bit(BR_FDB_STICKY, &fdb->flags)) {
+				br_fdb_notify_roaming(br, p, fdb);
+			} else {
+				WRITE_ONCE(fdb->dst, p);
+				modified = true;
+			}
 		}
 
 		if (test_and_set_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags)) {
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index ceaa5a89b947..512ffab16f5d 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -120,8 +120,14 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 				br_fdb_update(br, p, eth_hdr(skb)->h_source,
 					      vid, BIT(BR_FDB_LOCKED));
 			goto drop;
-		} else if (READ_ONCE(fdb_src->dst) != p ||
-			   test_bit(BR_FDB_LOCAL, &fdb_src->flags)) {
+		} else if (READ_ONCE(fdb_src->dst) != p) {
+			/* FDB is trying to roam. Notify userspace and drop
+			 * the packet
+			 */
+			if (test_bit(BR_FDB_STICKY, &fdb_src->flags))
+				br_fdb_notify_roaming(br, p, fdb_src);
+			goto drop;
+		} else if (test_bit(BR_FDB_LOCAL, &fdb_src->flags)) {
 			/* FDB mismatch. Drop the packet without roaming. */
 			goto drop;
 		} else if (test_bit(BR_FDB_LOCKED, &fdb_src->flags)) {
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 041f6e571a20..18d3cb5fec0e 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -277,6 +277,7 @@ enum {
 	BR_FDB_NOTIFY_INACTIVE,
 	BR_FDB_LOCKED,
 	BR_FDB_DYNAMIC_LEARNED,
+	BR_FDB_NOTIFY_ROAMING,
 };
 
 struct net_bridge_fdb_key {
@@ -874,6 +875,8 @@ int br_fdb_external_learn_del(struct net_bridge *br, struct net_bridge_port *p,
 			      bool swdev_notify);
 void br_fdb_offloaded_set(struct net_bridge *br, struct net_bridge_port *p,
 			  const unsigned char *addr, u16 vid, bool offloaded);
+void br_fdb_notify_roaming(struct net_bridge *br, struct net_bridge_port *p,
+			   struct net_bridge_fdb_entry *fdb);
 
 /* br_forward.c */
 enum br_pkt_type {

[RFC net-next (resend) 3/4] net: dsa: mv88e6xxx: handle member-violations

[Elliot Ayrey]

Add a handler for servicing member-violations to the mv88e6xxx switch
driver.

When we receive a member-violation from the hardware first check the
ATU for the corresponding entry and only service the interrupt if the
ATU entry has a non-zero DPV and the new port that raised the
interrupt is not in the DPV.

Servicing this interrupt will send a switchdev notification for the
new port.

Signed-off-by: Elliot Ayrey <elliot.ayrey@alliedtelesis.co.nz>
---
 drivers/net/dsa/mv88e6xxx/global1_atu.c | 38 +++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/switchdev.c   | 33 ++++++++++++++++++++-
 drivers/net/dsa/mv88e6xxx/switchdev.h   |  2 ++
 3 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mv88e6xxx/global1_atu.c b/drivers/net/dsa/mv88e6xxx/global1_atu.c
index c47f068f56b3..5c5c53cb2ad0 100644
--- a/drivers/net/dsa/mv88e6xxx/global1_atu.c
+++ b/drivers/net/dsa/mv88e6xxx/global1_atu.c
@@ -399,12 +399,36 @@ int mv88e6xxx_g1_atu_remove(struct mv88e6xxx_chip *chip, u16 fid, int port,
 	return mv88e6xxx_g1_atu_move(chip, fid, from_port, to_port, all);
 }
 
+static int mv88e6xxx_g1_atu_entry_check(struct mv88e6xxx_chip *chip, u16 fid, u8 mac[ETH_ALEN],
+					bool *in_atu, u16 *dpv)
+{
+	struct mv88e6xxx_atu_entry entry;
+	int err;
+
+	entry.state = 0;
+	ether_addr_copy(entry.mac, mac);
+	eth_addr_dec(entry.mac);
+
+	mv88e6xxx_reg_lock(chip);
+	err = mv88e6xxx_g1_atu_getnext(chip, fid, &entry);
+	mv88e6xxx_reg_unlock(chip);
+	if (err)
+		return err;
+
+	*in_atu = ether_addr_equal(entry.mac, mac);
+	if (dpv)
+		*dpv = entry.portvec;
+
+	return err;
+}
+
 static irqreturn_t mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
 {
 	struct mv88e6xxx_chip *chip = dev_id;
 	struct mv88e6xxx_atu_entry entry;
 	int err, spid;
 	u16 val, fid;
+	bool in_atu = false;
 
 	mv88e6xxx_reg_lock(chip);
 
@@ -437,6 +461,20 @@ static irqreturn_t mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
 						     entry.portvec, entry.mac,
 						     fid);
 		chip->ports[spid].atu_member_violation++;
+
+		if (fid != MV88E6XXX_FID_STANDALONE && chip->ports[spid].mab) {
+			u16 dpv = 0;
+
+			err = mv88e6xxx_g1_atu_entry_check(chip, fid, entry.mac, &in_atu, &dpv);
+			if (err)
+				goto out;
+
+			if (in_atu && dpv != 0 && !(dpv & BIT(spid))) {
+				err = mv88e6xxx_handle_member_violation(chip, spid, &entry, fid);
+				if (err)
+					goto out;
+			}
+		}
 	}
 
 	if (val & MV88E6XXX_G1_ATU_OP_MISS_VIOLATION) {
diff --git a/drivers/net/dsa/mv88e6xxx/switchdev.c b/drivers/net/dsa/mv88e6xxx/switchdev.c
index 4c346a884fb2..88761677ff10 100644
--- a/drivers/net/dsa/mv88e6xxx/switchdev.c
+++ b/drivers/net/dsa/mv88e6xxx/switchdev.c
@@ -79,5 +79,36 @@ int mv88e6xxx_handle_miss_violation(struct mv88e6xxx_chip *chip, int port,
 				       brport, &info.info, NULL);
 	rtnl_unlock();
 
-	return err;
+	return notifier_to_errno(err);
+}
+
+int mv88e6xxx_handle_member_violation(struct mv88e6xxx_chip *chip, int port,
+				      struct mv88e6xxx_atu_entry *entry, u16 fid)
+{
+	struct switchdev_notifier_fdb_info info = {
+		.addr = entry->mac,
+	};
+	struct net_device *brport;
+	struct dsa_port *dp;
+	u16 vid;
+	int err;
+
+	err = mv88e6xxx_find_vid(chip, fid, &vid);
+	if (err)
+		return err;
+
+	info.vid = vid;
+	dp = dsa_to_port(chip->ds, port);
+
+	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();
+
+	return notifier_to_errno(err);
 }
diff --git a/drivers/net/dsa/mv88e6xxx/switchdev.h b/drivers/net/dsa/mv88e6xxx/switchdev.h
index 62214f9d62b0..f718dbfaf45d 100644
--- a/drivers/net/dsa/mv88e6xxx/switchdev.h
+++ b/drivers/net/dsa/mv88e6xxx/switchdev.h
@@ -15,5 +15,7 @@
 int mv88e6xxx_handle_miss_violation(struct mv88e6xxx_chip *chip, int port,
 				    struct mv88e6xxx_atu_entry *entry,
 				    u16 fid);
+int mv88e6xxx_handle_member_violation(struct mv88e6xxx_chip *chip, int port,
+				      struct mv88e6xxx_atu_entry *entry, u16 fid);
 
 #endif /* _MV88E6XXX_SWITCHDEV_H_ */

[RFC net-next (resend) 4/4] net: dsa: mv88e6xxx: cache fid-to-vid association

[Elliot Ayrey]

When servicing ATU violations we need to walk the VTU to find the vlan
id for the ATU's FID which is inefficient.

Add a cache for this association and replace the VTU walk so we don't
have to do this costly operation all the time.

Signed-off-by: Elliot Ayrey <elliot.ayrey@alliedtelesis.co.nz>
---
 drivers/net/dsa/mv88e6xxx/chip.h        |  2 ++
 drivers/net/dsa/mv88e6xxx/global1_vtu.c |  6 ++++-
 drivers/net/dsa/mv88e6xxx/switchdev.c   | 35 ++-----------------------
 3 files changed, 9 insertions(+), 34 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index 48399ab5355a..91c3e4b304cf 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -445,6 +445,8 @@ struct mv88e6xxx_chip {
 
 	/* FID map */
 	DECLARE_BITMAP(fid_bitmap, MV88E6XXX_N_FID);
+
+	u16 vid_cache[MV88E6XXX_N_FID];
 };
 
 struct mv88e6xxx_bus_ops {
diff --git a/drivers/net/dsa/mv88e6xxx/global1_vtu.c b/drivers/net/dsa/mv88e6xxx/global1_vtu.c
index b524f27a2f0d..af1c40480303 100644
--- a/drivers/net/dsa/mv88e6xxx/global1_vtu.c
+++ b/drivers/net/dsa/mv88e6xxx/global1_vtu.c
@@ -464,7 +464,11 @@ int mv88e6390_g1_vtu_loadpurge(struct mv88e6xxx_chip *chip,
 	}
 
 	/* Load/Purge VTU entry */
-	return mv88e6xxx_g1_vtu_op(chip, MV88E6XXX_G1_VTU_OP_VTU_LOAD_PURGE);
+	err = mv88e6xxx_g1_vtu_op(chip, MV88E6XXX_G1_VTU_OP_VTU_LOAD_PURGE);
+	if (err == 0)
+		chip->vid_cache[entry->fid] = entry->valid ? entry->vid : 0;
+
+	return err;
 }
 
 int mv88e6xxx_g1_vtu_flush(struct mv88e6xxx_chip *chip)
diff --git a/drivers/net/dsa/mv88e6xxx/switchdev.c b/drivers/net/dsa/mv88e6xxx/switchdev.c
index 88761677ff10..e96daa2dcaf4 100644
--- a/drivers/net/dsa/mv88e6xxx/switchdev.c
+++ b/drivers/net/dsa/mv88e6xxx/switchdev.c
@@ -12,42 +12,11 @@
 #include "global1.h"
 #include "switchdev.h"
 
-struct mv88e6xxx_fid_search_ctx {
-	u16 fid_search;
-	u16 vid_found;
-};
-
-static int __mv88e6xxx_find_vid(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;
-}
-
 static int mv88e6xxx_find_vid(struct mv88e6xxx_chip *chip, u16 fid, u16 *vid)
 {
-	struct mv88e6xxx_fid_search_ctx ctx;
-	int err;
-
-	ctx.fid_search = fid;
-	mv88e6xxx_reg_lock(chip);
-	err = mv88e6xxx_vtu_walk(chip, __mv88e6xxx_find_vid, &ctx);
-	mv88e6xxx_reg_unlock(chip);
-	if (err < 0)
-		return err;
-	if (err == 1)
-		*vid = ctx.vid_found;
-	else
-		return -ENOENT;
+	*vid = chip->vid_cache[fid];
 
-	return 0;
+	return *vid ? 0 : -ENOENT;
 }
 
 int mv88e6xxx_handle_miss_violation(struct mv88e6xxx_chip *chip, int port,

Re: [RFC net-next (resend) 2/4] net: bridge: send notification for roaming hosts

[Andrew Lunn]

> This is achieved by temporarily updating the fdb entry with the new
> port, setting a new notify roaming bit, firing off a notification, and
> restoring the original port immediately afterwards. The port remains
> unchanged, respecting the sticky flag, but userspace is now notified
> of the new port the host was seen on.

This sounds a bit hacky. Could you add a new optional attribute to the
netlink message indicating the roam destination, so there is no need
to play games with the actual port?

I'm not too deep into how these all works, but i also wounder about
backwards compatibility. Old code which does not look for
FDB_NOTIFY_ROAMING_BIT is going to think it really has moved, with
your code. By using a new attribute, and not changing the port, old
code just sees a notification it is on the port it always was on,
which is less likely to cause issues?

And do we want to differentiate between it wants to roam, but the
sticky bit has stopped that, and it really has roamed?

	Andrew

Re: [RFC net-next (resend) 3/4] net: dsa: mv88e6xxx: handle member-violations

[Andrew Lunn]

> --- a/drivers/net/dsa/mv88e6xxx/switchdev.c
> +++ b/drivers/net/dsa/mv88e6xxx/switchdev.c
> @@ -79,5 +79,36 @@ int mv88e6xxx_handle_miss_violation(struct mv88e6xxx_chip *chip, int port,
>  				       brport, &info.info, NULL);
>  	rtnl_unlock();
>  
> -	return err;
> +	return notifier_to_errno(err);
> +}

This change does not look obviously correct to me. What has a miss
violation got to do with member violation? Is the existing code wrong?
What about the case when mv88e6xxx_find_vid() returns an error?

	Andrew

Re: [RFC net-next (resend) 3/4] net: dsa: mv88e6xxx: handle member-violations

[Elliot Ayrey]

On Fri, 2024-11-08 at 14:49 +0100, Andrew Lunn wrote:
> > --- a/drivers/net/dsa/mv88e6xxx/switchdev.c
> > +++ b/drivers/net/dsa/mv88e6xxx/switchdev.c
> > @@ -79,5 +79,36 @@ int mv88e6xxx_handle_miss_violation(struct
> > mv88e6xxx_chip *chip, int port,
> >  				       brport,
> > &http://scanmail.trustwave.com/?c=20988&d=jZeu528qsdfVmICHdkZAoueog
> > WLEwsN_Wa_RILla0Q&u=http%3a%2f%2finfo%2einfo NULL);
> >  	rtnl_unlock();
> >  
> > -	return err;
> > +	return notifier_to_errno(err);
> > +}
> 
> This change does not look obviously correct to me. What has a miss
> violation got to do with member violation? Is the existing code
> wrong?
> What about the case when mv88e6xxx_find_vid() returns an error?
> 
> 	Andrew

Hi Andrew, I forgot to remove this when preparing the patches, this was
intended to be a separate bug fix.

If mv88e6xxx_find_vid() returns an error it will return early, so the
notifier_to_errno() conversion will only happen after
call_switchdev_notifiers().

Re: [RFC net-next (resend) 1/4] net: bridge: respect sticky flag on external learn

[Elliot Ayrey]

On Sat, 2024-11-09 at 15:32 +0200, Nikolay Aleksandrov wrote:
> So you have a sticky fdb entry added, but it is still allowed to roam
> in HW?

No the hardware has informed us a host has _tried_ to roam.

As I think about this more, using the sticky bit alone probably isn't
the best idea and it might be better if this mechanism were related to
a port being locked? After all the port being locked in hardware is
what generates this event.

Re: [RFC net-next (resend) 2/4] net: bridge: send notification for roaming hosts

[Elliot Ayrey]

On Sat, 2024-11-09 at 15:40 +0200, Nikolay Aleksandrov wrote:
> No way, this is ridiculous. Changing the port like that for a notification is not
> ok at all. It is also not the bridge's job to notify user-space for sticky fdbs
> that are trying to roam, you already have some user-space app and you can catch
> such fdbs by other means (sniffing, ebpf hooks, netfilter matching etc). Such
> change can also lead to DDoS attacks with many notifications.

Unfortunately in this case the only indication we get from the hardware of this
event happening is a switchdev notification to the bridge. All traffic is dropped
in hardware when the port is in this mode so the methods you suggest will not work.

I have changed my implementation to use Andrew's suggestion of using a new attribute
rather than messing with the port. But would this also be more appropriate if the
notification was only triggered when receiving the event from hardware? If not
then do you have any suggestions for getting these kinds of events from hardware
to userspace without going through the bridge?

Re: [RFC net-next (resend) 2/4] net: bridge: send notification for roaming hosts

[Elliot Ayrey]

On Fri, 2024-11-08 at 14:42 +0100, Andrew Lunn wrote:
> This sounds a bit hacky. Could you add a new optional attribute to the
> netlink message indicating the roam destination, so there is no need
> to play games with the actual port?

Yes that's another option.

> I'm not too deep into how these all works, but i also wounder about
> backwards compatibility. Old code which does not look for
> FDB_NOTIFY_ROAMING_BIT is going to think it really has moved, with
> your code. By using a new attribute, and not changing the port, old
> code just sees a notification it is on the port it always was on,
> which is less likely to cause issues?

Thanks that's a good point. I'll have a look at making it an attribute
instead.

> And do we want to differentiate between it wants to roam, but the
> sticky bit has stopped that, and it really has roamed?

That is partly what this patch is trying to do, since actually roaming
hosts will already trigger a notification with the new port.

I will try your suggestion as it seems better than what I have here. I
also think moving away from relying on the sticky bit might be good.
This behaviour relies on the port being locked in hardware so it might
be more appropriate for this to be part of locked behaviour in the
kernel also?

Re: [RFC net-next (resend) 2/4] net: bridge: send notification for roaming hosts

[Andrew Lunn]

> I have changed my implementation to use Andrew's suggestion of using a new attribute
> rather than messing with the port. But would this also be more appropriate if the
> notification was only triggered when receiving the event from hardware?

Hardware only accelerates what the Linux network stack already does in
software. You need something which makes sense for a pure software
setup.

	Andrew

Re: [RFC net-next (resend) 2/4] net: bridge: send notification for roaming hosts

[Nikolay Aleksandrov]

On 24/11/2024 23:23, Elliot Ayrey wrote:
> On Sat, 2024-11-09 at 15:40 +0200, Nikolay Aleksandrov wrote:
>> No way, this is ridiculous. Changing the port like that for a notification is not
>> ok at all. It is also not the bridge's job to notify user-space for sticky fdbs
>> that are trying to roam, you already have some user-space app and you can catch
>> such fdbs by other means (sniffing, ebpf hooks, netfilter matching etc). Such
>> change can also lead to DDoS attacks with many notifications.
> 
> Unfortunately in this case the only indication we get from the hardware of this
> event happening is a switchdev notification to the bridge. All traffic is dropped
> in hardware when the port is in this mode so the methods you suggest will not work.
> 

I see

> I have changed my implementation to use Andrew's suggestion of using a new attribute
> rather than messing with the port. But would this also be more appropriate if the
> notification was only triggered when receiving the event from hardware? If not
> then do you have any suggestions for getting these kinds of events from hardware
> to userspace without going through the bridge?
> 
> 

We want to have the same behaviour (or as close as possible) between sw and hw.
Since this can cause many notifications to be sent up for current setups, maybe
make it optional so we'll get notifications for roam attempts only when we
explicitly enable them, with default off. You can look into bridge's bool options
for this (e.g. link-local fdb learning option).


Cheers,
 Nik

Re: [RFC net-next (resend) 1/4] net: bridge: respect sticky flag on external learn

[Nikolay Aleksandrov]

On 24/11/2024 23:01, Elliot Ayrey wrote:
> On Sat, 2024-11-09 at 15:32 +0200, Nikolay Aleksandrov wrote:
>> So you have a sticky fdb entry added, but it is still allowed to roam
>> in HW?
> 
> No the hardware has informed us a host has _tried_ to roam.
> 
> As I think about this more, using the sticky bit alone probably isn't
> the best idea and it might be better if this mechanism were related to
> a port being locked? After all the port being locked in hardware is
> what generates this event.

That does sound better, but we should have the same functionality
in software as well. Perhaps optional to avoid potential flood of
current users with unexpected notifications, see my response to
the other mail for more info.

Cheers,
 Nik