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