Re: [PATCH net-next v2 1/2] net: dsa: qca8k: serialize register access

[netdev-bot+sashiko@kernel.org]

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 fixes a regmap work_buf race that already exists in
  released kernels, but it has no Fixes: tag and no stable Cc.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is a high-
  severity bug the series builds on.

--

net: dsa: qca8k: serialize register access

This patch drops .disable_locking from the qca8k regmap config, so regmap
serializes use of its shared work_buf. It also swaps the order in which
qca8k_conduit_change() takes the MIB and management mutexes, so the order
matches the autocast path.

> diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
> index 07227ae8bc04..9a8b9e34a27c 100644
> --- a/drivers/net/dsa/qca/qca8k-8xxx.c
> +++ b/drivers/net/dsa/qca/qca8k-8xxx.c
> @@ -579,7 +579,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 */

[Severity: Medium]
The work_buf race described in the commit message is already in released
kernels. Should this patch carry a Fixes: tag and go to net, not ride
along as groundwork for the MIB stats change?

The race seems to come from commit c766e077d927 ("net: dsa: qca8k: convert
to regmap read/write API"). Before that commit, qca8k_regmap_config used
.reg_read/.reg_write. In that case __regmap_init() skips format
initialization and work_buf is never used, so .disable_locking was safe
then.

c766e077d927 switched to .read = qca8k_bulk_read and .write =
qca8k_bulk_write but kept .disable_locking. In __regmap_init(), this
branch:

    if (config->read && config->write) {
        map->reg_read  = _regmap_bus_read;

routes reads and writes through the formatted path. That path writes the
register (and value) into the shared map->work_buf and then passes it to
the driver callbacks.

Callers outside rtnl can already hit this without the MIB work this
series adds. One path is per-port phylink resolve work:

    phylink_resolve()
      qca8k_phylink_mac_link_up()
        qca8k_write(priv, QCA8K_REG_PORT_STATUS(port), reg)
          regmap_write()

Another is the LED class callbacks, such as regmap_read() in
qca8k_cled_hw_control_get(). Both can run concurrently and end up writing
the wrong switch register or returning the wrong value.

Would something like this be appropriate?

Fixes: c766e077d927 ("net: dsa: qca8k: convert to regmap read/write API")

That would let stable kernels since v6.3 pick up the regmap part.

>  	.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
> @@ -1782,13 +1781,16 @@ qca8k_conduit_change(struct dsa_switch *ds, const struct net_device *conduit,
>  	if (dp->index != 0)
>  		return;
>  
> -	mutex_lock(&priv->mgmt_eth_data.mutex);
> +	/* Same order as an autocast, which holds the MIB mutex while its
> +	 * register write takes the management one.
> +	 */
>  	mutex_lock(&priv->mib_eth_data.mutex);
> +	mutex_lock(&priv->mgmt_eth_data.mutex);

[Severity: High]
This is a pre-existing issue, not one introduced by this patch, but the
series builds on the autocast path, so it seems worth raising here.

qca8k_get_ethtool_stats_eth() publishes the caller's ethtool buffer and
then waits with a short timeout:

    mib_eth_data->req_port = dp->index;
    mib_eth_data->data = data;
    refcount_set(&mib_eth_data->port_parsed, QCA8K_NUM_PORTS);
    ...
    ret = wait_for_completion_timeout(&mib_eth_data->rw_done, QCA8K_ETHERNET_TIMEOUT);

exit:
    mutex_unlock(&mib_eth_data->mutex);

QCA8K_ETHERNET_TIMEOUT is msecs_to_jiffies(5), which can be a single
jiffy. On timeout, mib_eth_data->data and ->req_port are left pointing at
the buffer. ethtool_get_stats() then vfree()s that buffer once
qca8k_get_ethtool_stats() returns.

qca8k_mib_autocast_handler() runs in conduit rx softirq context with no
lock and no check that a request is still pending:

    port = FIELD_GET(QCA_HDR_RECV_SOURCE_PORT, ntohs(mib_ethhdr->hdr));
    if (port != mib_eth_data->req_port)
        goto exit;
    ...
        mib_eth_data->data[i] = get_unaligned_le32(mib_ethhdr->data + i);

If the MIB frames arrive late, for example because conduit rx is delayed,
can the frame for req_port write into the vfree'd buffer from softirq?
ETHTOOL_GSTATS does not need CAP_NET_ADMIN, so the request side can be
triggered by an unprivileged user.

The exit path also runs for every frame:

exit:
    if (refcount_dec_and_test(&mib_eth_data->port_parsed))
        complete(&mib_eth_data->rw_done);

Can stale frames underflow port_parsed after it has already reached 0?
That would trigger a refcount WARN.

If a stale frame lands after the next request's refcount_set(), can it
also complete that request early with partly stale counters?

Separately, qca_tag_rcv() only does pskb_may_pull(skb, QCA_HDR_LEN) before
it dispatches to this handler. The handler then reads mib_count counters
from skb->data without checking skb->len. Can a short MIB-type frame cause
an out-of-bounds read here?

The later patch in this series, "net: dsa: qca8k: report port statistics
from the MIB", keeps the lockless handler. It adds a periodic autocast and
a QCA8K_MIB_ALL_PORTS mode that indexes mib_eth_data->data +
port * QCA8K_QCA833X_MIB_COUNT. Doesn't that make timed-out requests and
late frames more frequent?

>  
>  	priv->mgmt_conduit = operational ? (struct net_device *)conduit : NULL;
>  
> -	mutex_unlock(&priv->mib_eth_data.mutex);
>  	mutex_unlock(&priv->mgmt_eth_data.mutex);
> +	mutex_unlock(&priv->mib_eth_data.mutex);
>  }

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