[PATCH net] net: dsa: mv88e6xxx: Verify after ATU Load ops

[PATCH net] net: dsa: mv88e6xxx: Verify after ATU Load ops

[Joseph Huang]

ATU Load operations could fail silently if there's not enough space
on the device to hold the new entry.

Do a Read-After-Write verification after each fdb/mdb add operation
to make sure that the operation was really successful, and return
-ENOSPC otherwise.

Fixes: defb05b9b9b4 ("net: dsa: mv88e6xxx: Add support for fdb_add, fdb_del, and fdb_getnext")
Signed-off-by: Joseph Huang <Joseph.Huang@garmin.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 52 +++++++++++++++++++++++---------
 1 file changed, 38 insertions(+), 14 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 68d1e891752b..cb3c4ebc5534 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2208,9 +2208,20 @@ mv88e6xxx_port_vlan_prepare(struct dsa_switch *ds, int port,
 	return err;
 }
 
+static int mv88e6xxx_port_db_get(struct mv88e6xxx_chip *chip,
+				 struct mv88e6xxx_atu_entry *entry,
+				 u16 fid, const unsigned char *addr)
+{
+	entry->state = 0;
+	ether_addr_copy(entry->mac, addr);
+	eth_addr_dec(entry->mac);
+
+	return mv88e6xxx_g1_atu_getnext(chip, fid, entry);
+}
+
 static int mv88e6xxx_port_db_load_purge(struct mv88e6xxx_chip *chip, int port,
 					const unsigned char *addr, u16 vid,
-					u8 state)
+					u8 state, bool verify)
 {
 	struct mv88e6xxx_atu_entry entry;
 	struct mv88e6xxx_vtu_entry vlan;
@@ -2238,11 +2249,7 @@ static int mv88e6xxx_port_db_load_purge(struct mv88e6xxx_chip *chip, int port,
 		fid = vlan.fid;
 	}
 
-	entry.state = 0;
-	ether_addr_copy(entry.mac, addr);
-	eth_addr_dec(entry.mac);
-
-	err = mv88e6xxx_g1_atu_getnext(chip, fid, &entry);
+	err = mv88e6xxx_port_db_get(chip, &entry, fid, addr);
 	if (err)
 		return err;
 
@@ -2266,7 +2273,20 @@ static int mv88e6xxx_port_db_load_purge(struct mv88e6xxx_chip *chip, int port,
 		entry.state = state;
 	}
 
-	return mv88e6xxx_g1_atu_loadpurge(chip, fid, &entry);
+	err = mv88e6xxx_g1_atu_loadpurge(chip, fid, &entry);
+	if (err)
+		return err;
+
+	if (!!state && verify) {
+		err = mv88e6xxx_port_db_get(chip, &entry, fid, addr);
+		if (err)
+			return err;
+
+		if (!entry.state || !ether_addr_equal(entry.mac, addr))
+			return -ENOSPC;
+	}
+
+	return 0;
 }
 
 static int mv88e6xxx_policy_apply(struct mv88e6xxx_chip *chip, int port,
@@ -2298,7 +2318,7 @@ static int mv88e6xxx_policy_apply(struct mv88e6xxx_chip *chip, int port,
 			return -EOPNOTSUPP;
 
 		err = mv88e6xxx_port_db_load_purge(chip, port, addr, vid,
-						   state);
+						   state, false);
 		if (err)
 			return err;
 		break;
@@ -2487,7 +2507,8 @@ static int mv88e6xxx_port_add_broadcast(struct mv88e6xxx_chip *chip, int port,
 
 	eth_broadcast_addr(broadcast);
 
-	return mv88e6xxx_port_db_load_purge(chip, port, broadcast, vid, state);
+	return mv88e6xxx_port_db_load_purge(chip, port, broadcast, vid, state,
+					    false);
 }
 
 static int mv88e6xxx_broadcast_setup(struct mv88e6xxx_chip *chip, u16 vid)
@@ -2539,7 +2560,7 @@ mv88e6xxx_port_broadcast_sync_vlan(struct mv88e6xxx_chip *chip,
 	eth_broadcast_addr(broadcast);
 
 	return mv88e6xxx_port_db_load_purge(chip, ctx->port, broadcast,
-					    vlan->vid, state);
+					    vlan->vid, state, false);
 }
 
 static int mv88e6xxx_port_broadcast_sync(struct mv88e6xxx_chip *chip, int port,
@@ -2845,7 +2866,8 @@ static int mv88e6xxx_port_fdb_add(struct dsa_switch *ds, int port,
 
 	mv88e6xxx_reg_lock(chip);
 	err = mv88e6xxx_port_db_load_purge(chip, port, addr, vid,
-					   MV88E6XXX_G1_ATU_DATA_STATE_UC_STATIC);
+					   MV88E6XXX_G1_ATU_DATA_STATE_UC_STATIC,
+					   true);
 	mv88e6xxx_reg_unlock(chip);
 
 	return err;
@@ -2859,7 +2881,7 @@ static int mv88e6xxx_port_fdb_del(struct dsa_switch *ds, int port,
 	int err;
 
 	mv88e6xxx_reg_lock(chip);
-	err = mv88e6xxx_port_db_load_purge(chip, port, addr, vid, 0);
+	err = mv88e6xxx_port_db_load_purge(chip, port, addr, vid, 0, false);
 	mv88e6xxx_reg_unlock(chip);
 
 	return err;
@@ -6613,7 +6635,8 @@ static int mv88e6xxx_port_mdb_add(struct dsa_switch *ds, int port,
 
 	mv88e6xxx_reg_lock(chip);
 	err = mv88e6xxx_port_db_load_purge(chip, port, mdb->addr, mdb->vid,
-					   MV88E6XXX_G1_ATU_DATA_STATE_MC_STATIC);
+					   MV88E6XXX_G1_ATU_DATA_STATE_MC_STATIC,
+					   true);
 	mv88e6xxx_reg_unlock(chip);
 
 	return err;
@@ -6627,7 +6650,8 @@ static int mv88e6xxx_port_mdb_del(struct dsa_switch *ds, int port,
 	int err;
 
 	mv88e6xxx_reg_lock(chip);
-	err = mv88e6xxx_port_db_load_purge(chip, port, mdb->addr, mdb->vid, 0);
+	err = mv88e6xxx_port_db_load_purge(chip, port, mdb->addr, mdb->vid, 0,
+					   false);
 	mv88e6xxx_reg_unlock(chip);
 
 	return err;
-- 
2.48.1

Re: [PATCH net] net: dsa: mv88e6xxx: Verify after ATU Load ops

[Andrew Lunn]

On Tue, Mar 04, 2025 at 06:53:51PM -0500, Joseph Huang wrote:
> ATU Load operations could fail silently if there's not enough space
> on the device to hold the new entry.
> 
> Do a Read-After-Write verification after each fdb/mdb add operation
> to make sure that the operation was really successful, and return
> -ENOSPC otherwise.

Please could you add a description of what the user sees when the ATU
is full. What makes this a bug which needs fixing? I would of thought
at least for unicast addresses, the switch has no entry for the
destination, so sends the packet to the CPU. The CPU will then
software bridge it out the correct port. Reporting ENOSPC will not
change that.

> @@ -2845,7 +2866,8 @@ static int mv88e6xxx_port_fdb_add(struct dsa_switch *ds, int port,
>  
>  	mv88e6xxx_reg_lock(chip);
>  	err = mv88e6xxx_port_db_load_purge(chip, port, addr, vid,
> -					   MV88E6XXX_G1_ATU_DATA_STATE_UC_STATIC);
> +					   MV88E6XXX_G1_ATU_DATA_STATE_UC_STATIC,
> +					   true);
>  	mv88e6xxx_reg_unlock(chip);
>  
>  	return err;

> @@ -6613,7 +6635,8 @@ static int mv88e6xxx_port_mdb_add(struct dsa_switch *ds, int port,
>  
>  	mv88e6xxx_reg_lock(chip);
>  	err = mv88e6xxx_port_db_load_purge(chip, port, mdb->addr, mdb->vid,
> -					   MV88E6XXX_G1_ATU_DATA_STATE_MC_STATIC);
> +					   MV88E6XXX_G1_ATU_DATA_STATE_MC_STATIC,
> +					   true);
>  	mv88e6xxx_reg_unlock(chip);

This change seems bigger than what it needs to be. Rather than modify
mv88e6xxx_port_db_load_purge(), why not perform the lookup just in
these two functions via a helper?

    Andrew

---
pw-bot: cr

Re: [PATCH net] net: dsa: mv88e6xxx: Verify after ATU Load ops

[Joseph Huang]

On 3/5/2025 10:14 AM, Andrew Lunn wrote:
> On Tue, Mar 04, 2025 at 06:53:51PM -0500, Joseph Huang wrote:
>> ATU Load operations could fail silently if there's not enough space
>> on the device to hold the new entry.
>>
>> Do a Read-After-Write verification after each fdb/mdb add operation
>> to make sure that the operation was really successful, and return
>> -ENOSPC otherwise.
> 
> Please could you add a description of what the user sees when the ATU
> is full. What makes this a bug which needs fixing? I would of thought
> at least for unicast addresses, the switch has no entry for the
> destination, so sends the packet to the CPU. The CPU will then
> software bridge it out the correct port. Reporting ENOSPC will not
> change that.

Hi Andrew,

What the user will see when the ATU table is full depends on the unknown 
flood setting. If a user has unknown multicast flood disabled, what the 
user will see is that multicast packets are dropped when the ATU table 
is full. In other words, IGMP snooping is broken when the ATU Load 
operation fails silently.

Even if the packet is kicked up to the CPU and the CPU intends to 
forward the packet out via the software bridge, the forwarding attempt 
is going to be blocked due to the 'offload_fwd_mark' flag in 
nbp_switchdev_allowed_egress(). Even if that somehow worked, we will not 
be fully utilizing the hardware's switching capability and will be 
relying on the CPU to do the forwarding, which will likely result in 
lower throughput.

Reporting -ENOSPC will not change the fact that the ATU table is full, 
however it does give switchdev a chance to notify the user and then the 
user can take some further action accordingly. If nothing else, at least 
'bridge monitor' will now report that the entries are not offloaded.

Some other DSA drivers are reporting -ENOSPC as well when the table is 
full (at least b53 and ocelot).

> 
>> @@ -2845,7 +2866,8 @@ static int mv88e6xxx_port_fdb_add(struct dsa_switch *ds, int port,
>>   
>>   	mv88e6xxx_reg_lock(chip);
>>   	err = mv88e6xxx_port_db_load_purge(chip, port, addr, vid,
>> -					   MV88E6XXX_G1_ATU_DATA_STATE_UC_STATIC);
>> +					   MV88E6XXX_G1_ATU_DATA_STATE_UC_STATIC,
>> +					   true);
>>   	mv88e6xxx_reg_unlock(chip);
>>   
>>   	return err;
> 
>> @@ -6613,7 +6635,8 @@ static int mv88e6xxx_port_mdb_add(struct dsa_switch *ds, int port,
>>   
>>   	mv88e6xxx_reg_lock(chip);
>>   	err = mv88e6xxx_port_db_load_purge(chip, port, mdb->addr, mdb->vid,
>> -					   MV88E6XXX_G1_ATU_DATA_STATE_MC_STATIC);
>> +					   MV88E6XXX_G1_ATU_DATA_STATE_MC_STATIC,
>> +					   true);
>>   	mv88e6xxx_reg_unlock(chip);
> 
> This change seems bigger than what it needs to be. Rather than modify
> mv88e6xxx_port_db_load_purge(), why not perform the lookup just in
> these two functions via a helper?
> 
>      Andrew

I will make that change. Thanks for the review.

Thanks,
Joseph
> 
> ---
> pw-bot: cr

Re: [PATCH net] net: dsa: mv88e6xxx: Verify after ATU Load ops

[Andrew Lunn]

On Wed, Mar 05, 2025 at 12:44:54PM -0500, Joseph Huang wrote:
> On 3/5/2025 10:14 AM, Andrew Lunn wrote:
> > On Tue, Mar 04, 2025 at 06:53:51PM -0500, Joseph Huang wrote:
> > > ATU Load operations could fail silently if there's not enough space
> > > on the device to hold the new entry.
> > > 
> > > Do a Read-After-Write verification after each fdb/mdb add operation
> > > to make sure that the operation was really successful, and return
> > > -ENOSPC otherwise.
> > 
> > Please could you add a description of what the user sees when the ATU
> > is full. What makes this a bug which needs fixing? I would of thought
> > at least for unicast addresses, the switch has no entry for the
> > destination, so sends the packet to the CPU. The CPU will then
> > software bridge it out the correct port. Reporting ENOSPC will not
> > change that.
> 
> Hi Andrew,
> 
> What the user will see when the ATU table is full depends on the unknown
> flood setting. If a user has unknown multicast flood disabled, what the user
> will see is that multicast packets are dropped when the ATU table is full.
> In other words, IGMP snooping is broken when the ATU Load operation fails
> silently.

Please add this to the commit message. This describes the real problem
being fixed, which is what somebody reading the commit message wants
to know.

   Andrew

[PATCH v2 net 1/1] net: dsa: mv88e6xxx: Verify after ATU Load ops

[Joseph Huang]

ATU Load operations could fail silently if there's not enough space
on the device to hold the new entry. When this happens, the symptom
depends on the unknown flood settings. If unknown multicast flood is
disabled, the multicast packets are dropped when the ATU table is
full. If unknown multicast flood is enabled, the multicast packets
will be flooded to all ports. Either way, IGMP snooping is broken
when the ATU Load operation fails silently.

Do a Read-After-Write verification after each fdb/mdb add operation
to make sure that the operation was really successful, and return
-ENOSPC otherwise.

Fixes: defb05b9b9b4 ("net: dsa: mv88e6xxx: Add support for fdb_add, fdb_del, and fdb_getnext")
Signed-off-by: Joseph Huang <Joseph.Huang@garmin.com>
---
V1: https://lore.kernel.org/lkml/20250304235352.3259613-1-Joseph.Huang@garmin.com/
V2: Add helper function to check the existence of an entry and only
    call it in mv88e6xxx_port_fdb/mdb_add().
---
 drivers/net/dsa/mv88e6xxx/chip.c | 59 ++++++++++++++++++++++++++------
 1 file changed, 48 insertions(+), 11 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 68d1e891752b..1a2b51598275 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2208,13 +2208,11 @@ mv88e6xxx_port_vlan_prepare(struct dsa_switch *ds, int port,
 	return err;
 }
 
-static int mv88e6xxx_port_db_load_purge(struct mv88e6xxx_chip *chip, int port,
-					const unsigned char *addr, u16 vid,
-					u8 state)
+static int mv88e6xxx_port_db_get(struct mv88e6xxx_chip *chip,
+				 const unsigned char *addr, u16 vid,
+				 u16 *fid, struct mv88e6xxx_atu_entry *entry)
 {
-	struct mv88e6xxx_atu_entry entry;
 	struct mv88e6xxx_vtu_entry vlan;
-	u16 fid;
 	int err;
 
 	/* Ports have two private address databases: one for when the port is
@@ -2225,7 +2223,7 @@ static int mv88e6xxx_port_db_load_purge(struct mv88e6xxx_chip *chip, int port,
 	 * VLAN ID into the port's database used for VLAN-unaware bridging.
 	 */
 	if (vid == 0) {
-		fid = MV88E6XXX_FID_BRIDGED;
+		*fid = MV88E6XXX_FID_BRIDGED;
 	} else {
 		err = mv88e6xxx_vtu_get(chip, vid, &vlan);
 		if (err)
@@ -2235,14 +2233,39 @@ static int mv88e6xxx_port_db_load_purge(struct mv88e6xxx_chip *chip, int port,
 		if (!vlan.valid)
 			return -EOPNOTSUPP;
 
-		fid = vlan.fid;
+		*fid = vlan.fid;
 	}
 
-	entry.state = 0;
-	ether_addr_copy(entry.mac, addr);
-	eth_addr_dec(entry.mac);
+	entry->state = 0;
+	ether_addr_copy(entry->mac, addr);
+	eth_addr_dec(entry->mac);
+
+	return mv88e6xxx_g1_atu_getnext(chip, *fid, entry);
+}
+
+static bool mv88e6xxx_port_db_find(struct mv88e6xxx_chip *chip,
+				   const unsigned char *addr, u16 vid)
+{
+	struct mv88e6xxx_atu_entry entry;
+	u16 fid;
+	int err;
 
-	err = mv88e6xxx_g1_atu_getnext(chip, fid, &entry);
+	err = mv88e6xxx_port_db_get(chip, addr, vid, &fid, &entry);
+	if (err)
+		return false;
+
+	return entry.state && ether_addr_equal(entry.mac, addr);
+}
+
+static int mv88e6xxx_port_db_load_purge(struct mv88e6xxx_chip *chip, int port,
+					const unsigned char *addr, u16 vid,
+					u8 state)
+{
+	struct mv88e6xxx_atu_entry entry;
+	u16 fid;
+	int err;
+
+	err = mv88e6xxx_port_db_get(chip, addr, vid, &fid, &entry);
 	if (err)
 		return err;
 
@@ -2847,6 +2870,13 @@ static int mv88e6xxx_port_fdb_add(struct dsa_switch *ds, int port,
 	err = mv88e6xxx_port_db_load_purge(chip, port, addr, vid,
 					   MV88E6XXX_G1_ATU_DATA_STATE_UC_STATIC);
 	mv88e6xxx_reg_unlock(chip);
+	if (err)
+		return err;
+
+	mv88e6xxx_reg_lock(chip);
+	if (!mv88e6xxx_port_db_find(chip, addr, vid))
+		err = -ENOSPC;
+	mv88e6xxx_reg_unlock(chip);
 
 	return err;
 }
@@ -6615,6 +6645,13 @@ static int mv88e6xxx_port_mdb_add(struct dsa_switch *ds, int port,
 	err = mv88e6xxx_port_db_load_purge(chip, port, mdb->addr, mdb->vid,
 					   MV88E6XXX_G1_ATU_DATA_STATE_MC_STATIC);
 	mv88e6xxx_reg_unlock(chip);
+	if (err)
+		return err;
+
+	mv88e6xxx_reg_lock(chip);
+	if (!mv88e6xxx_port_db_find(chip, mdb->addr, mdb->vid))
+		err = -ENOSPC;
+	mv88e6xxx_reg_unlock(chip);
 
 	return err;
 }
-- 
2.48.1

Re: [PATCH v2 net 1/1] net: dsa: mv88e6xxx: Verify after ATU Load ops

[Andrew Lunn]

On Wed, Mar 05, 2025 at 03:28:27PM -0500, Joseph Huang wrote:
> ATU Load operations could fail silently if there's not enough space
> on the device to hold the new entry. When this happens, the symptom
> depends on the unknown flood settings. If unknown multicast flood is
> disabled, the multicast packets are dropped when the ATU table is
> full. If unknown multicast flood is enabled, the multicast packets
> will be flooded to all ports. Either way, IGMP snooping is broken
> when the ATU Load operation fails silently.
> 
> Do a Read-After-Write verification after each fdb/mdb add operation
> to make sure that the operation was really successful, and return
> -ENOSPC otherwise.
> 
> Fixes: defb05b9b9b4 ("net: dsa: mv88e6xxx: Add support for fdb_add, fdb_del, and fdb_getnext")
> Signed-off-by: Joseph Huang <Joseph.Huang@garmin.com>
> ---
> V1: https://lore.kernel.org/lkml/20250304235352.3259613-1-Joseph.Huang@garmin.com/
> V2: Add helper function to check the existence of an entry and only
>     call it in mv88e6xxx_port_fdb/mdb_add().

You need to start a new thread for each version of the
patch. Otherwise the tooling does not always recognise it.

> @@ -2847,6 +2870,13 @@ static int mv88e6xxx_port_fdb_add(struct dsa_switch *ds, int port,
>  	err = mv88e6xxx_port_db_load_purge(chip, port, addr, vid,
>  					   MV88E6XXX_G1_ATU_DATA_STATE_UC_STATIC);
>  	mv88e6xxx_reg_unlock(chip);
> +	if (err)
> +		return err;
> +
> +	mv88e6xxx_reg_lock(chip);
> +	if (!mv88e6xxx_port_db_find(chip, addr, vid))
> +		err = -ENOSPC;
> +	mv88e6xxx_reg_unlock(chip);

unlocking and lock the registers seems to introduce a race
condition. Could another thread delete the just added entry before you
test to see if it was correctly added?

Please hold the lock across the entire operation.

    Andrew

---
pw-bot: cr