[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

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 (1) · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch tells a late autocast frame from a live one only by
  checking whether mib_eth_data->data is NULL.

Pre-existing issues:
- [High] At this commit, qca8k_mib_autocast_handler() sets data2 =
  (__le32 *)skb->data and reads counters 3..mib_count-1 from it.

--

net: dsa: qca8k: drop MIB autocast frames once the request is over

This patch adds a spinlock to qca8k_mib_eth_data, taken by both
qca8k_mib_autocast_handler() and qca8k_get_ethtool_stats_eth(). The
request's data pointer is cleared when the request finishes or times out,
and the handler drops any frame that arrives while data is NULL. This is
meant to stop late frames from writing into a freed ethtool buffer and
from underflowing port_parsed.

> diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
> index 49be53b7fb61f..56796df14bf5a 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;

[Severity: Medium]

Can this check tell a late frame from a frame of the current request?
struct mib_ethhdr has only the three counters and the QCA tag, so nothing
ties a frame to a particular MIB_CAST.

Also, qca8k_get_ethtool_stats_eth() does not poll QCA8K_MIB_BUSY before
sending a new cast. qca8k_mib_init() does poll it for the flush.

Consider this sequence:

request N, qca8k_get_ethtool_stats_eth()
    wait_for_completion_timeout() times out, frames still in flight
    mib_eth_data->data = NULL;

request N+1, qca8k_get_ethtool_stats_eth()
    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);

qca8k_mib_autocast_handler(), leftover frames of request N
    data is not NULL, so the frames pass this check
    each one decrements request N+1's port_parsed
    one whose source port matches req_port writes N's counters into
    the new buffer

When port_parsed reaches zero on a mix of old and new frames, the handler
runs:

	if (refcount_dec_and_test(&mib_eth_data->port_parsed)) {
		mib_eth_data->data = NULL;
		complete(&mib_eth_data->rw_done);
	}

Request N+1's own remaining frames, possibly including its req_port frame,
are then dropped by this NULL check.

wait_for_completion_timeout() returns a positive value, so
qca8k_get_ethtool_stats() takes the early return:

	if (priv->mgmt_conduit && priv->info->ops->autocast_mib &&
	    priv->info->ops->autocast_mib(ds, port, data) > 0)
		return;

That skips the register-read fallback. Could userspace then get zeros from
the vzalloc'd buffer, or request N's old counters, reported as a
successful read?

QCA8K_ETHERNET_TIMEOUT is msecs_to_jiffies(5), which is only one or two
jiffies at low HZ. A timeout with frames still queued in the conduit RX
path seems plausible.

The commit message says:

    so that frames arriving after that are dropped without
    touching data or port_parsed.

The new comments also say "A late frame of a request that has already
returned", "retire the request so that the handler drops them" and "NULL
with no request pending". These claims seem to hold only until the next
request publishes data again. The later patch "net: dsa: qca8k: check the
length of MIB autocast frames" does not change this logic either.

Would it be possible to resync with the hardware after a timeout, for
example by polling for QCA8K_MIB_BUSY to clear before the next cast? Or
could the code check that the req_port frame was actually parsed before
reporting success? Failing that, could the commit message and comments be
corrected?

> +
>  	/* The switch autocast every port. Ignore other packet and
>  	 * parse only the requested one.
>  	 */

[Severity: High]

This is a pre-existing issue, not one introduced by this patch. A little
further down in qca8k_mib_autocast_handler(), the remaining counters are
read from skb->data with no length check:

	data2 = (__le32 *)skb->data;
	...
		if (mib->size == 2)
			mib_eth_data->data[i] = get_unaligned_le64((__le64 *)data2);
		else
			mib_eth_data->data[i] = get_unaligned_le32(data2);

That is 156 bytes on QCA832x and 164 bytes on QCA833x. Before calling the
handler, qca_tag_rcv() only guarantees the 2-byte tag:

	if (unlikely(!pskb_may_pull(skb, QCA_HDR_LEN))) {
	...
	if (pk_type == QCA_HDR_RECV_TYPE_MIB) {
		if (likely(tagger_data->mib_autocast_handler))
			tagger_data->mib_autocast_handler(ds, skb);

Can a short or non-linear MIB frame make this loop read past
skb_headlen()? The bytes would land in the buffer that ethtool_get_stats()
copies to an unprivileged caller.

The next patch in the series, "net: dsa: qca8k: check the length of MIB
autocast frames", fixes this. It adds pskb_may_pull(skb, len) before data2
is used and reloads mib_ethhdr afterwards.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022192933.384898-1-kuncy7%40gmail.com

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

[Stanislaw Pal]

Thanks for the review. The sequence is right: a frame carries nothing
that ties it to a cast, so after a timeout the frames of the previous
cast are taken for the next request's. v2 will poll QCA8K_MIB_BUSY
before sending a cast, as qca8k_mib_init() does for the flush, so the
switch never has two casts in flight, and the handler will record the
ports it has seen in a bitmap instead of counting frames down, so an
extra frame can neither underflow the count nor complete the request
early. What remains is that a frame of the previous cast which was
still queued on the host side can serve the next request; its counters
are then one cast old, which the commit message will say instead of
claiming that every late frame is dropped.

v2 will also carry two fixes for the issues you found in the same
handlers: the MDIO fallback of qca8k_bulk_read() storing each register
one byte after the previous one, and qca8k_rw_reg_ack_handler() reading
past a short frame and past its 16-byte buffer.

pw-bot: cr

[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