Re: [PATCH net-next v6 RESEND 4/4] net: stmmac: hold RTNL when reading descriptor rings in descriptors_status

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit fixes a NULL pointer dereference that a user can
  trigger, but its message has no Fixes: tag (and no Cc: stable).
- [Low] stmmac_rings_status_show() reads its loop bounds before taking
  RTNL.

--

net: stmmac: hold RTNL when reading descriptor rings in descriptors_status

This commit takes RTNL around the descriptors_status debugfs reader so it
is serialized against the control-plane paths that free the descriptor
rings. It also skips queues whose rings are NULL, which happens when the
interface stays IFF_UP after a failed internal reopen.

> Skip such queues with a hint instead of dereferencing a NULL pointer
> in stmmac_rings_status_show().
>
> Signed-off-by: Ding Hui <dinghui@lixiang.com>

[Severity: Low]
This fixes a NULL pointer dereference that can be triggered from debugfs,
but the commit has no Fixes: tag and no Cc: stable. Should they be added so
the affected stable trees pick this up?

The state described above comes from stmmac_change_mtu() when the reopen
fails:

stmmac_change_mtu()
    __stmmac_release(dev);
    ret = __stmmac_open(dev, dma_conf);
        init_error:
            free_dma_desc_resources(priv, &priv->dma_conf);

free_dma_desc_resources() frees the rings and sets dma_rx/dma_erx and
dma_tx/dma_etx to NULL. IFF_UP stays set.

Before this patch, stmmac_rings_status_show() checked only IFF_UP. It then
passed the NULL ring to sysfs_display_ring(), which dereferences p->des0. A
reader that raced with ndo_stop, an MTU change, an XDP change or an XSK
change could also read DMA memory while it was being freed.

There are two candidate targets. One is the commit that added the
unsynchronized descriptors_status reader. The other is the commit that
added the MTU-change reopen path:

Fixes: 347007968744 ("net: ethernet: stmicro: stmmac: permit MTU change with interface up")

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index cd4631efe47ab..10d33a43db947 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -6748,8 +6748,10 @@ static int stmmac_rings_status_show(struct seq_file *seq, void *v)
>  	u8 tx_count = priv->plat->tx_queues_to_use;
>  	u8 queue;
>  
> +	rtnl_lock();
> +

[Severity: Low]
Are the loop bounds still read outside the lock? rx_count and tx_count are
set in the declarations, before rtnl_lock() is taken:

    u8 rx_count = priv->plat->rx_queues_to_use;
    u8 tx_count = priv->plat->tx_queues_to_use;

stmmac_reinit_queues() (ethtool -L) runs under RTNL. It releases the rings
and then updates both fields:

stmmac_reinit_queues()
    stmmac_release(dev);
    ...
    priv->plat->rx_queues_to_use = rx_cnt;
    priv->plat->tx_queues_to_use = tx_cnt;
    ...
    ret = stmmac_open(dev);

Suppose the reader loads the counts and then blocks in rtnl_lock() while
set_channels runs. The loops then use stale bounds.

If the count shrank, the reader prints "Descriptor ring not allocated" for
queues that no longer exist. If it grew, live queues are skipped without
any message. This is also a data race that KCSAN could report, since the
fields are written under RTNL.

The rx_queue[] and tx_queue[] arrays are sized by MTL_MAX_*_QUEUES, so the
accesses stay in bounds. Only the debugfs output is affected.

The commit message says RTNL now synchronizes this reader with the control
plane. Would it make sense to read rx_queues_to_use and tx_queues_to_use
after rtnl_lock() as well?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106090507.1020751-1-dinghui1111%40163.com