[PATCH net 0/3] net: dsa: qca8k: fix concurrent register and MIB autocast access

[PATCH net 0/3] net: dsa: qca8k: fix concurrent register and MIB autocast access

[Stanislaw Pal]

The netdev AI review of my net-next MIB statistics series [1] found
three bugs that are already in released kernels. They do not depend on
that series, so here they are for net:

1. Since the conversion to the regmap read/write API, the regmap uses
   a shared work_buf but is created with .disable_locking. phylink's
   resolve work and the LED callbacks access registers outside rtnl,
   so concurrent users can swap or corrupt register and value.

2. An ethtool MIB request over Ethernet (autocast) that times out
   leaves the caller's buffer published. Late frames then write into
   the buffer after ethtool has freed it, and extra frames underflow
   the frame counter.

3. The autocast handler reads up to 164 bytes of counters without
   checking the frame length.

The MIB statistics series will be rebased on these once net-next
reopens.

Testing: on a GL-B3000 (IPQ5018 + QCA8337, CPU on port 0) with the qca
tagger, backported to OpenWrt's 6.18 kernel with PROVE_LOCKING. That
tree also carries the MIB statistics series, so a work polls the MIB
every 3 s next to ethtool -S. In four minutes under about 50 Mbit/s
of routed traffic, 938 MIB frames arrived after their request had
already returned, and all were dropped. Frames made artificially short
were left uncounted, and the requests fell back to register reads. The
ethtool and netdev counters stayed monotonic, and there were no lockdep,
refcount or other warnings.

[1] https://lore.kernel.org/netdev/20261019191137.307514-1-kuncy7@gmail.com/

Stanislaw Pal (3):
  net: dsa: qca8k: let regmap serialize register access
  net: dsa: qca8k: drop MIB autocast frames once the request is over
  net: dsa: qca8k: check the length of MIB autocast frames

 drivers/net/dsa/qca/qca8k-8xxx.c | 34 ++++++++++++++++++++++++++++++--
 drivers/net/dsa/qca/qca8k.h      |  3 ++-
 2 files changed, 34 insertions(+), 3 deletions(-)

-- 
2.43.0

[PATCH net 1/3] net: dsa: qca8k: let regmap serialize register access

[Stanislaw Pal]

The regmap is created with .disable_locking, on the ground that the
qca8k read and write callbacks do their own locking. They only lock the
bus transaction, though. Since the driver moved to the regmap
read/write API, regmap formats the register and parses the value in the
map's shared work_buf, outside of the callbacks, so two concurrent
users can swap or corrupt each other's register and value.

Most register access runs under rtnl, but not all of it: phylink's
resolve work (mac_link_up/down) and the LED class callbacks do not, so
a link change can race with an ethtool or LED access and write the
wrong switch register or read back the wrong value.

Let regmap take its own lock. Every user of the map can already sleep,
on the MDIO bus or on the management frame round trip.

Fixes: c766e077d927 ("net: dsa: qca8k: convert to regmap read/write API")
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Stanislaw Pal <kuncy7@gmail.com>
---
 drivers/net/dsa/qca/qca8k-8xxx.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index 4c928983b..49be53b7f 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -574,7 +574,6 @@ static const struct regmap_config qca8k_regmap_config = {
 	.write = qca8k_bulk_write,
 	.reg_update_bits = qca8k_regmap_update_bits,
 	.rd_table = &qca8k_readable_table,
-	.disable_locking = true, /* Locking is handled by qca8k read/write */
 	.cache_type = REGCACHE_NONE, /* Explicitly disable CACHE */
 	.max_raw_read = 32, /* mgmt eth can read up to 8 registers at time */
 	/* ATU regs suffer from a bug where some data are not correctly
-- 
2.43.0

[PATCH net 2/3] net: dsa: qca8k: drop MIB autocast frames once the request is over

[Stanislaw Pal]

qca8k_get_ethtool_stats_eth() points mib_eth_data at the caller's
ethtool buffer, asks the switch for an autocast and waits 5 ms for the
frames. On a timeout it returns with the request still published, and
the caller falls back to register reads; ethtool_get_stats() then frees
the buffer. The autocast handler runs from the conduit's receive path
without the request's mutex, so a frame that arrives late still writes
the requested port's counters through mib_eth_data->data into freed
memory, and frames beyond the seven expected ones underflow port_parsed.
ETHTOOL_GSTATS does not need CAP_NET_ADMIN.

Protect the request with a spinlock that the handler takes, retire it
on the way out, and let the handler retire it when the last frame
completes it, so that frames arriving after that are dropped without
touching data or port_parsed.

Fixes: 5c957c7ca78c ("net: dsa: qca8k: add support for mib autocast in Ethernet packet")
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Stanislaw Pal <kuncy7@gmail.com>
---
 drivers/net/dsa/qca/qca8k-8xxx.c | 22 +++++++++++++++++++++-
 drivers/net/dsa/qca/qca8k.h      |  3 ++-
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index 49be53b7f..56796df14 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -1663,6 +1663,12 @@ static void qca8k_mib_autocast_handler(struct dsa_switch *ds, struct sk_buff *sk
 	mib_ethhdr = (struct mib_ethhdr *)skb_mac_header(skb);
 	mib_eth_data = &priv->mib_eth_data;
 
+	spin_lock(&mib_eth_data->lock);
+
+	/* A late frame of a request that has already returned */
+	if (!mib_eth_data->data)
+		goto unlock;
+
 	/* The switch autocast every port. Ignore other packet and
 	 * parse only the requested one.
 	 */
@@ -1692,8 +1698,12 @@ static void qca8k_mib_autocast_handler(struct dsa_switch *ds, struct sk_buff *sk
 
 exit:
 	/* Complete on receiving all the mib packet */
-	if (refcount_dec_and_test(&mib_eth_data->port_parsed))
+	if (refcount_dec_and_test(&mib_eth_data->port_parsed)) {
+		mib_eth_data->data = NULL;
 		complete(&mib_eth_data->rw_done);
+	}
+unlock:
+	spin_unlock(&mib_eth_data->lock);
 }
 
 static int
@@ -1710,9 +1720,11 @@ qca8k_get_ethtool_stats_eth(struct dsa_switch *ds, int port, u64 *data)
 
 	reinit_completion(&mib_eth_data->rw_done);
 
+	spin_lock_bh(&mib_eth_data->lock);
 	mib_eth_data->req_port = dp->index;
 	mib_eth_data->data = data;
 	refcount_set(&mib_eth_data->port_parsed, QCA8K_NUM_PORTS);
+	spin_unlock_bh(&mib_eth_data->lock);
 
 	mutex_lock(&priv->reg_mutex);
 
@@ -1730,6 +1742,13 @@ qca8k_get_ethtool_stats_eth(struct dsa_switch *ds, int port, u64 *data)
 	ret = wait_for_completion_timeout(&mib_eth_data->rw_done, QCA8K_ETHERNET_TIMEOUT);
 
 exit:
+	/* On a timeout the frames can still arrive after the caller has
+	 * freed data: retire the request so that the handler drops them.
+	 */
+	spin_lock_bh(&mib_eth_data->lock);
+	mib_eth_data->data = NULL;
+	spin_unlock_bh(&mib_eth_data->lock);
+
 	mutex_unlock(&mib_eth_data->mutex);
 
 	return ret;
@@ -2107,6 +2126,7 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
 	init_completion(&priv->mgmt_eth_data.rw_done);
 
 	mutex_init(&priv->mib_eth_data.mutex);
+	spin_lock_init(&priv->mib_eth_data.lock);
 	init_completion(&priv->mib_eth_data.rw_done);
 
 	priv->ds->dev = &mdiodev->dev;
diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
index 956338893..a550f331c 100644
--- a/drivers/net/dsa/qca/qca8k.h
+++ b/drivers/net/dsa/qca/qca8k.h
@@ -402,9 +402,10 @@ struct qca8k_mgmt_eth_data {
 struct qca8k_mib_eth_data {
 	struct completion rw_done;
 	struct mutex mutex; /* Process one command at time */
+	spinlock_t lock; /* Protects the request against the rx handler */
 	refcount_t port_parsed; /* Counter to track parsed port */
 	u8 req_port;
-	u64 *data; /* pointer to ethtool data */
+	u64 *data; /* pointer to ethtool data, NULL with no request pending */
 };
 
 struct qca8k_ports_config {
-- 
2.43.0

[PATCH net 3/3] net: dsa: qca8k: check the length of MIB autocast frames

[Stanislaw Pal]

qca_tag_rcv() makes only the 2 byte tag linear before it hands a MIB
frame to qca8k_mib_autocast_handler(). The handler then reads the
counters that follow the Ethernet header, up to 164 bytes on QCA833x,
straight from skb->data, without checking the frame length. A short or
non-linear frame makes it read past the packet data.

Pull the counters into the linear area before reading them. A frame
that is too short is left uncounted, so the request times out and
the caller reads the counters from the registers instead.

Fixes: 5c957c7ca78c ("net: dsa: qca8k: add support for mib autocast in Ethernet packet")
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Stanislaw Pal <kuncy7@gmail.com>
---
 drivers/net/dsa/qca/qca8k-8xxx.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index 56796df14..e5d858230 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -1656,6 +1656,7 @@ static void qca8k_mib_autocast_handler(struct dsa_switch *ds, struct sk_buff *sk
 	struct qca8k_priv *priv = ds->priv;
 	const struct qca8k_mib_desc *mib;
 	struct mib_ethhdr *mib_ethhdr;
+	unsigned int len = 0;
 	__le32 *data2;
 	u8 port;
 	int i;
@@ -1676,6 +1677,16 @@ static void qca8k_mib_autocast_handler(struct dsa_switch *ds, struct sk_buff *sk
 	if (port != mib_eth_data->req_port)
 		goto exit;
 
+	/* Past the first 3, the counters follow the header. Leave a short
+	 * frame uncounted, so that the request times out and the caller
+	 * reads the registers instead.
+	 */
+	for (i = 3; i < priv->info->mib_count; i++)
+		len += ar8327_mib[i].size * sizeof(u32);
+	if (!pskb_may_pull(skb, len))
+		goto unlock;
+
+	mib_ethhdr = (struct mib_ethhdr *)skb_mac_header(skb);
 	data2 = (__le32 *)skb->data;
 
 	for (i = 0; i < priv->info->mib_count; i++) {
-- 
2.43.0