* Re: [PATCH v2] net: meth: Fix skb allocation failure handling in RX init
2026-06-24 3:19 [PATCH v2] net: meth: Fix skb allocation failure handling in RX init Haoxiang Li
@ 2026-06-25 8:47 ` Simon Horman
0 siblings, 0 replies; 2+ messages in thread
From: Simon Horman @ 2026-06-25 8:47 UTC (permalink / raw)
To: haoxiang_li2024
Cc: Simon Horman, andrew+netdev, davem, edumazet, kuba, pabeni,
pavan.chebbi, netdev, linux-kernel
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
Simon says:
Hi Haoxiang,
I think that the pre-existing issues flagged below do not need
to be considered in the context of this patch. But I am wondering
if you could take a look over the others.
---
net: meth: Fix skb allocation failure handling in RX init
meth_init_rx_ring() does not check the return value of alloc_skb().
If the allocation fails, the NULL skb is passed to skb_reserve() and
then dereferenced through skb->head.
> --- a/drivers/net/ethernet/sgi/meth.c
> +++ b/drivers/net/ethernet/sgi/meth.c
> @@ -228,6 +228,9 @@ static int meth_init_rx_ring(struct meth_private *priv)
>
> for (i = 0; i < RX_RING_ENTRIES; i++) {
> priv->rx_skbs[i] = alloc_skb(METH_RX_BUFF_SIZE, 0);
> + if (!priv->rx_skbs[i])
> + goto err_free_skbs;
> +
> /* 8byte status vector + 3quad padding + 2byte padding,
> * to put data on 64bit aligned boundary */
> skb_reserve(priv->rx_skbs[i],METH_RX_HEAD);
[ ... ]
> @@ -240,6 +243,17 @@ static int meth_init_rx_ring(struct meth_private *priv)
> }
> priv->rx_write = 0;
> return 0;
> +
> +err_free_skbs:
> + while (i--) {
> + dma_unmap_single(&priv->pdev->dev, priv->rx_ring_dmas[i],
> + METH_RX_BUFF_SIZE, DMA_FROM_DEVICE);
[Severity: Critical]
During the allocation loop, each successfully mapped DMA address is pushed
immediately to the hardware via the mace->eth.rx_fifo register. Unmapping
them here frees the memory, but the hardware FIFO still retains the physical
addresses.
When meth_init_rx_ring() returns -ENOMEM to meth_tx_timeout(), the error is
ignored and the RX DMA engine is enabled anyway:
meth_tx_timeout()
...
meth_init_rx_ring(priv);
priv->dma_ctrl |= METH_DMA_TX_EN | METH_DMA_RX_EN | METH_DMA_RX_INT_EN;
Will this cause the hardware to DMA incoming packet data into the unmapped
and potentially reallocated memory?
> + priv->rx_ring[i] = 0;
[Severity: High]
If the hardware remains active as described above, will the RX interrupt
handler dereference this NULL pointer?
When the hardware receives a packet, it raises an RX interrupt and meth_rx()
attempts to process it:
meth_rx()
...
status = priv->rx_ring[priv->rx_write]->status.raw;
Since the pointer at this index was cleared in the error path, this looks
like it will cause a kernel panic.
> + priv->rx_ring_dmas[i] = 0;
> + kfree_skb(priv->rx_skbs[i]);
> + priv->rx_skbs[i] = NULL;
[Severity: Critical]
Does this partial cleanup lead to a double free of socket buffers?
When meth_init_rx_ring() fails at index N, this loop zeroes out elements 0
through N-1. The elements from N through 63 retain their dangling pointers
from the previous execution of meth_free_rx_ring(), which fails to clear SKB
pointers after freeing them.
Since meth_tx_timeout() ignores the error and leaves the device running, the
next time meth_free_rx_ring() is called, it iterates over all 64 elements
again. Will this call kfree_skb() on the dangling pointers in indices N
through 63?
> + }
> + return -ENOMEM;
> }
[Severity: High]
This isn't a bug introduced by this patch, but dma_map_single() is called in
meth_init_rx_ring() without checking for mapping errors:
meth_init_rx_ring()
...
priv->rx_ring_dmas[i] = dma_map_single(&priv->pdev->dev,
priv->rx_ring[i],
METH_RX_BUFF_SIZE,
DMA_FROM_DEVICE);
mace->eth.rx_fifo = priv->rx_ring_dmas[i];
If the mapping fails, an error cookie is passed directly to the hardware.
Could this lead to IOMMU faults or memory corruption when DMA bounce buffers
are exhausted?
[Severity: High]
This is a pre-existing issue, but there appears to be a race condition due
to lockless processing in meth_rx().
The interrupt handler drops the spinlock before processing the ring:
meth_rx()
...
spin_unlock_irqrestore(&priv->meth_lock, flags);
...
while (priv->rx_write != fifo_rptr) {
Concurrently, meth_tx_timeout() can acquire the lock and destroy the RX rings,
falsely assuming it is protected:
meth_tx_timeout()
...
spin_lock_irqsave(&priv->meth_lock,flags);
meth_reset(dev);
...
meth_free_rx_ring(priv);
If meth_free_rx_ring() frees the SKBs while meth_rx() is still executing the
lockless while loop, could this result in a use-after-free or NULL pointer
dereference?
^ permalink raw reply [flat|nested] 2+ messages in thread