Re: [PATCH V6 3/4] net: xilinx: axienet: Derive RX frame length from residue in dmaengine path

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

Pre-existing issues:
- [High] The DMA engine TX submission path fails to decrement `tx_ring_head` on error, corrupting the ring state and causing use-after-free and double-free on subsequent TX completions.
- [High] The `axienet_stop()` function leaks pending `sk_buff` structures and their DMA mappings when using the DMA engine.
--

--- Patch [3]: [PATCH V6 3/4] net: xilinx: axienet: Derive RX frame length from residue in dmaengine path ---
commit 86c32a60f9e05880f70cba98d261769dec79ad80
Author: Srinivas Neeli <[email protected]>

net: xilinx: axienet: Derive RX frame length from residue in dmaengine path

The dmaengine RX path derived the received frame length from the descriptor
APP metadata. That only works when the optional AXI4-Stream status/control
interface is present, because the hardware populates the APP fields solely
when that interface is enabled. On designs without it the length read back
is invalid.

> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index fcf517069d161..67d1b8e91d683 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the DMA ring
management, I noticed an issue in axienet_start_xmit_dmaengine() where the
ring state can become corrupted on error.

If an error occurs during mapping, tx_ring_head remains incremented while the 
SKB is freed:

drivers/net/ethernet/xilinx/xilinx_axienet_main.c:axienet_start_xmit_dmaengine() {
	lp->tx_ring_head++;
	sg_init_table(skbuf_dma->sgl, sg_len);
	ret = skb_to_sgvec(skb, skbuf_dma->sgl, 0, skb->len);
	if (ret < 0)
		goto xmit_error_drop_skb;
...
xmit_error_drop_skb:
	dev_kfree_skb_any(skb);
	return NETDEV_TX_OK;
}

When the next valid packet is successfully submitted and completes, will
axienet_dma_tx_cb() fetch the descriptor using lp->tx_ring_tail++ and read
the freed skb?

drivers/net/ethernet/xilinx/xilinx_axienet_main.c:axienet_dma_tx_cb() {
...
	skbuf_dma = axienet_get_tx_desc(lp, lp->tx_ring_tail++);
	len = skbuf_dma->skb->len;
	txq = skb_get_tx_queue(lp->ndev, skbuf_dma->skb);
	...
	dev_consume_skb_any(skbuf_dma->skb);
}

Does this out-of-sync ring state lead to a use-after-free and double-free
of the SKB?

[Severity: High]
This is a pre-existing issue, but I also noticed a potential memory and DMA
mapping leak in axienet_stop() when the interface is brought down.

The teardown loops free the wrapper structures, but do they fail to unmap
and free the associated SKBs?

drivers/net/ethernet/xilinx/xilinx_axienet_main.c:axienet_stop() {
...
		for (i = 0; i < TX_BD_NUM_MAX; i++)
			kfree(lp->tx_skb_ring[i]);
		kfree(lp->tx_skb_ring);
		for (i = 0; i < RX_BUF_NUM_DEFAULT; i++)
			kfree(lp->rx_skb_ring[i]);
		kfree(lp->rx_skb_ring);
...
}

The SKBs and DMA mappings were allocated in axienet_rx_submit_desc():

drivers/net/ethernet/xilinx/xilinx_axienet_main.c:axienet_rx_submit_desc() {
...
	addr = dma_map_single(lp->dev, skb->data, lp->max_frm_size, DMA_FROM_DEVICE);
	...
	skbuf_dma->skb = skb;
	skbuf_dma->dma_address = sg_dma_address(skbuf_dma->sgl);
...
}

Can this cause a permanent leak of SKBs and mappings each time the
interface is stopped?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3