Re: [PATCH 05/20] net: xilinx: tsn: bring up the endpoint MCDMA channels

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Requesting DMA channels in ndo_open() breaks probe deferral and exposes internal error codes.
- [High] Race condition in TX descriptor allocation corrupts the DMA ring.
- [High] RX DMA rings are permanently disabled due to missing initialization.
- [High] Using skb_dst_set_noref() before __netif_rx() causes Use-After-Free of metadata dst.
- [Medium] Manual scatterlist construction risks NULL pointer dereferences in DMA backend.
--

--- Patch [5]: [PATCH 05/20] net: xilinx: tsn: bring up the endpoint MCDMA channels ---
commit 083d653fd2c94a113e47e527d00ee7dae164f904
Author: Srinivas Neeli <[email protected]>

net: xilinx: tsn: bring up the endpoint MCDMA channels

Before the endpoint can move frames, each MCDMA channel needs a ring of
buffers and a live dmaengine channel.

Request every TX and RX channel named in the device tree when the
interface opens, and give each one a ring of preallocated SKB
descriptor slots. Reset the controller through tx_chan0 on open, and
release the channels on stop. Add the XILINX_DMA dependency for the
dmaengine and Xilinx DMA APIs.

> diff --git a/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c b/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c
> index 062cb94c21533..fa21d59ade165 100644
> --- a/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c
> +++ b/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c
[ ... ]
> @@ -59,8 +122,27 @@ static netdev_tx_t ep_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>  	return NETDEV_TX_OK;
>  }
>  
> +static int ep_reset_dma_controller(struct xlnx_tsn_ep *ep);
> +static int ep_init_dmaengine(struct xlnx_tsn_ep *ep);
> +static void ep_exit_dmaengine(struct xlnx_tsn_ep *ep);
> +
> +static int ep_open(struct net_device *ndev)
> +{
> +	struct xlnx_tsn_ep *ep = netdev_priv(ndev);
> +	int ret;
> +
> +	WRITE_ONCE(ep->closing, false);
> +
> +	ret = ep_reset_dma_controller(ep);
> +	if (ret)
> +		return ret;
> +
> +	ret = ep_init_dmaengine(ep);
> +	if (ret) {
> +		netdev_err(ndev, "failed to initialize DMA engine\n");
> +		return ret;
> +	}
> +
[Severity: High]
Does this sequence need to prime the RX rings by calling ep_rx_submit_desc()
before starting the queues?  It appears the rings are permanently disabled
because the dmaengine has no initial memory targets for receive interrupts.

> +	netif_tx_start_all_queues(ndev);
> +
> +	return 0;
> +}
[ ... ]
> +static int ep_reset_dma_controller(struct xlnx_tsn_ep *ep)
> +{
> +	struct xilinx_vdma_config cfg = { .reset = 1 };
> +	struct dma_chan *tx_chan0;
> +	int ret;
> +
> +	tx_chan0 = dma_request_chan(ep->dev, "tx_chan0");
> +	if (IS_ERR(tx_chan0))
> +		return dev_err_probe(ep->dev, PTR_ERR(tx_chan0),
> +				     "failed to request tx_chan0 for reset\n");
[Severity: High]
Since this is the ndo_open() path rather than the driver probe() path, will
using dma_request_chan() and dev_err_probe() here break probe deferral?

If the DMA controller is not ready, this will return -EPROBE_DEFER to
userspace when the interface is brought up instead of deferring the driver
probe.

> +
> +	ret = xilinx_vdma_channel_set_config(tx_chan0, &cfg);
> +	dma_release_channel(tx_chan0);
> +	if (ret < 0)
> +		return dev_err_probe(ep->dev, ret,
> +				     "failed to reset DMA controller\n");
> +
> +	return 0;
> +}

Additionally, I noticed a few other issues in this file:

In ep_start_xmit():

	spin_lock_bh(&ep->tx_lock);
	if (CIRC_SPACE(xchan->ring_head, READ_ONCE(xchan->ring_tail),
		       TX_BD_NUM_DEFAULT) <= 1) {
		...
	skbuf_dma = ep_get_desc(xchan, xchan->ring_head & (TX_BD_NUM_DEFAULT - 1));
	if (!skbuf_dma) {
		spin_unlock_bh(&ep->tx_lock);
		goto err_drop_skb;
	}
	spin_unlock_bh(&ep->tx_lock);

	sg_init_table(skbuf_dma->sgl, sg_len);
	...
	spin_lock_bh(&ep->tx_lock);
	xchan->ring_head++;

[Severity: High]
Does dropping ep->tx_lock while preparing the descriptor expose the slot to
concurrent modification? If multiple threads transmit on logical queues
mapping to the same physical channel, could they grab the same descriptor slot
and overwrite it before the producer index is incremented?

In ep_dma_rx_cb():

	skb_put(skb, rx_len);
	skb_dst_set_noref(skb, &ep->port_md[port_id]->dst);
	skb->dev = ndev;
	skb->protocol = eth_type_trans(skb, ndev);
	skb->ip_summed = CHECKSUM_NONE;
	__netif_rx(skb);

[Severity: High]
Can using skb_dst_set_noref() here cause a use-after-free? __netif_rx() defers
execution to the CPU backlog, which crosses softirq and RCU boundaries. If
the driver is unloaded, the metadata destination could be freed while deferred
SKBs still reference it.

In ep_rx_submit_desc():

	sg_init_table(skbuf_dma->sgl, 1);
	addr = dma_map_single(xchan->dma_dev, skb->data, ep->max_frm_size,
			      DMA_FROM_DEVICE);
	...
	sg_dma_address(skbuf_dma->sgl) = addr;
	sg_dma_len(skbuf_dma->sgl) = ep->max_frm_size;
	dma_rx_desc = dmaengine_prep_slave_sg(xchan->chan, skbuf_dma->sgl,
					      1, DMA_DEV_TO_MEM,
					      DMA_PREP_INTERRUPT);

[Severity: Medium]
Is it safe to construct the scatterlist manually like this? This bypasses
sg_init_one() and dma_map_sg(), which risks NULL pointer dereferences in the
DMA backend since fields like page_link might remain uninitialized.

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