Re: [PATCH net v5 5/5] net: ethernet: mtk_wed: bail out when failed to set up rings in wed_start

[netdev-bot+sashiko@kernel.org]

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

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

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 new bail-outs in mtk_wed_start()
  (drivers/net/ethernet/mediatek/mtk_wed.c) log a dev_err() and return,…
- [Medium] net: ethernet: mtk_wed: memory leak on ring setup error paths
- [Low] The commit fixes error handling for two calls in mtk_wed_start()
  but has no Fixes: tag.

Pre-existing issues:
- [Critical] This problem wasn't introduced by this patch, but it is a
  double free in the same reset -> start lifecycle.

--

net: ethernet: mtk_wed: bail out when failed to set up rings in wed_start

This commit checks the return values of mtk_wed_wdma_rx_ring_setup() and
mtk_wed_wdma_tx_ring_setup() in mtk_wed_start(). When either one fails, it
prints an error and returns early.

Link: https://lore.kernel.org/all/179000700034.2160803.10594281691384015089@kernel.org/

> Add error message and return when fail to set up tx/rx wdma rings.
>
> This is suggested by Sashiko.

[Severity: Low]
Should this patch have a Fixes: tag?

The previous patch in this series, "net: ethernet: mtk_wed: setup
WDMA_RING_TX(0) for non-DBDC MT7986", added the
mtk_wed_wdma_tx_ring_setup(dev, 0, ...) call checked here, and that patch
carries Fixes: 4c5de09eb0d0. Without a tag on this patch, a stable
backport of the earlier one could land without its error check.

The unchecked mtk_wed_wdma_rx_ring_setup() call comes from commit
23dca7a90017 ("net: ethernet: mtk_wed: add reset to tx_ring_setup
callback").

Every other patch in this series has a Fixes: tag. The line "This is
suggested by Sashiko." is free text and does not say which bug is being
fixed.

> diff --git a/drivers/net/ethernet/mediatek/mtk_wed.c b/drivers/net/ethernet/mediatek/mtk_wed.c
> index 3a7c2bb33691f..86845893ae20f 100644
> --- a/drivers/net/ethernet/mediatek/mtk_wed.c
> +++ b/drivers/net/ethernet/mediatek/mtk_wed.c
> @@ -2353,14 +2353,20 @@ mtk_wed_start(struct mtk_wed_device *dev, u32 irq_mask)
>  
>  	for (i = 0; i < ARRAY_SIZE(dev->rx_wdma); i++)
>  		if (!dev->rx_wdma[i].desc)
> -			mtk_wed_wdma_rx_ring_setup(dev, i, 16, false);
> +			if (mtk_wed_wdma_rx_ring_setup(dev, i, 16, false)) {
> +				dev_err(dev->hw->dev, "wdma rx ring %d setup failed\n", i);
> +				return;
> +			}

[Severity: Medium]
Does the caller ever find out that start failed here? The same question
applies to the tx_wdma[0] return below.

mtk_wed_start() is static void and is installed as the .start op. In
include/linux/soc/mediatek/mtk_wed.h the op is declared as:

	void (*start)(struct mtk_wed_device *dev, u32 irq_mask);

The early return therefore skips mtk_wed_hw_init(),
mtk_wed_configure_irq(), mtk_wed_set_ext_int(), the v1 PCIe mirror map
write, mtk_wed_rro_cfg(), mtk_wed_amsdu_init(), mtk_wed_dma_enable() and
dev->running = true, and nothing is reported back.

mtk_wed_device_active() only checks the ops pointer:

#define mtk_wed_device_active(_dev) !!(_dev)->ops

so mt76 still treats WED as active. mt7915_dma_start() has already put
WFDMA into WED mode and written MT_INT_WED_MASK_CSR before it gets here:

		mtk_wed_device_start(&dev->mt76.mmio.wed, wed_irq_mask);
	}

	irq_mask = reset ? MT_INT_MCU_CMD : irq_mask;

	mt7915_irq_enable(dev, irq_mask);

It then returns 0, even though it returns int and could pass an error up.
mt7996_dma_start() does the same after writing MT_INT_MASK_CSR.

On WED v3 with hw_rro, mt76 later calls mtk_wed_start_hw_rro(). That
function has no init or running check. It writes MTK_WED_WPDMA_INT_MASK
and MTK_WED_INT_MASK and sets MTK_WED_RRO_MSDU_PG_DRV_EN. Because of the
early return, mtk_wed_hw_init() never programmed MTK_WED_RX_BM_BASE or
MTK_WED_RRO_PG_BM_BASE.

Can this leave a half-initialized WED that the wlan driver keeps using as
if it had started, with only the dev_err() to show anything went wrong?

Would it be better to have ->start return an int, so that mtk_wed_start()
and the mt76 callers can propagate the error and unwind?

>  
>  	/*
>  	 * non-DBDC MT7986 allocates only rx_ring[1] and tx_wdma[1] during setup
>  	 * but tx_wdma[0] is also needed for WED to function.
>  	 */
>  	if (mtk_wed_is_v2(dev->hw) && !dev->rx_ring[0].desc)
> -		mtk_wed_wdma_tx_ring_setup(dev, 0, MTK_WED_WDMA_RING_SIZE, !!dev->tx_wdma[0].desc);
> +		if (mtk_wed_wdma_tx_ring_setup(dev, 0, MTK_WED_WDMA_RING_SIZE, !!dev->tx_wdma[0].desc)) {
> +			dev_err(dev->hw->dev, "wdma tx ring 0 setup failed\n");
> +			return;
> +		}

[Severity: Medium]
This is outside the lines changed here. It is the same ring setup
error handling this patch is tightening, though, so it seems worth
raising.

In mtk_wed_rx_ring_setup(), mtk_wed_ring_alloc() first allocates the
descriptor memory for dev->rx_ring[idx]. If the following
mtk_wed_wdma_tx_ring_setup() call fails, the function returns -ENOMEM
without setting MTK_WED_RING_CONFIGURED on that ring.

mtk_wed_tx_ring_setup() has the same shape. dev->tx_ring[idx] is
allocated, and a failure from mtk_wed_wdma_rx_ring_setup() returns
without setting the flag.

The teardown helpers only free rings that have
MTK_WED_RING_CONFIGURED set. In both cases the coherent DMA buffer
for the wlan-facing ring is therefore never released by
mtk_wed_free_tx_rings() or mtk_wed_free_rx_rings(). If setup is
later retried with reset == false, mtk_wed_ring_alloc() overwrites
ring->desc and the old buffer is lost for good.

mtk_wed_free_rx_rings() also looks like it tests the wrong array. It
frees dev->rx_ring[i] based on dev->tx_ring[i].flags. Whether an rx
ring is freed or skipped then depends on the tx ring with the same
index, not on the rx ring itself. That makes the rx side leak more
easily still.

Should the error paths in mtk_wed_rx_ring_setup() and
mtk_wed_tx_ring_setup() free the ring they just allocated? Or should
the free helpers key off ring->desc instead of the flag? And should
the check in mtk_wed_free_rx_rings() test dev->rx_ring[i].flags?

[ ... ]

[Severity: Critical]
This isn't a bug introduced by this patch, but it sits in the same reset
and start lifecycle. mtk_wed_rx_reset() does:

	mtk_wed_free_rx_buffer(dev);
	mtk_wed_hwrro_free_buffer(dev);

When dev->rx_buf_ring.desc is non-NULL, mtk_wed_free_rx_buffer() already
ends with its own call:

	dev->wlan.release_rx_buf(dev);
	dma_free_coherent(dev->hw->dev, dev->rx_buf_ring.size * sizeof(*desc),
			  desc, dev->rx_buf_ring.desc_phys);

	mtk_wed_hwrro_free_buffer(dev);

mtk_wed_hwrro_free_buffer() frees everything but never clears
dev->hw_rro.pages or dev->hw_rro.desc:

	dma_free_coherent(dev->hw->dev, dev->hw_rro.size * sizeof(*desc),
			  desc, dev->hw_rro.desc_phys);

free_pagelist:
	kfree(page_list);

Does the second call in mtk_wed_rx_reset() then read page_list[] from
freed memory? It would also __free_page() the hw_rro pages,
dma_free_coherent() hw_rro.desc and kfree() the page list a second time.

This looks reachable on every SER/DMA reset on WED v3 with wlan.hw_rro
set, which mt7996 does for the primary WED:

mt7996_dma_reset()
  mtk_wed_device_dma_reset()
    mtk_wed_reset_dma()
      mtk_wed_rx_reset()
        mtk_wed_free_rx_buffer()
          mtk_wed_hwrro_free_buffer()
        mtk_wed_hwrro_free_buffer()

There is a second, separate issue: mtk_wed_free_rx_buffer() leaves
dev->rx_buf_ring.desc pointing at freed memory. Suppose the next
mtk_wed_start()->mtk_wed_rx_buffer_alloc() fails at dma_alloc_coherent()
before that pointer is reassigned. Could a later
__mtk_wed_detach()->mtk_wed_free_rx_rings()->mtk_wed_free_rx_buffer() then
free it again?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261016-wedfix-v5-0-8b02e78fd34f%40gmail.com