Re: [PATCH 07/20] net: xilinx: tsn: add the endpoint TX data path

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Time-of-check to time-of-use (TOCTOU) race condition during TX ring slot allocation allows concurrent transmissions to overwrite the same descriptor slot.
- [Critical] Un-serialized submission to the DMA engine allows thread preemption to invert submission order, leading to premature DMA unmapping (Use-After-Free).
- [High] The TX completion callback wakes only the specific logical queue of the completed packet, permanently starving other logical queues mapped to the same physical channel.
- [High] Pending TX and posted RX SKBs are not freed when the interface is stopped, causing a memory leak.
--

--- Patch [7]: [PATCH 07/20] net: xilinx: tsn: add the endpoint TX data path ---
commit 74572301861a89d8c7ecbf1624d4c45457a33266
Author: Srinivas Neeli <[email protected]>

net: xilinx: tsn: add the endpoint TX data path

Add the transmit path so the endpoint can send frames. Look up the physical
DMA channel for the frame's queue through the map built at probe, map the
frame and submit it to that channel. Stop the queue when the ring runs out
of space, and wake it from the completion callback once a slot frees up.

> diff --git a/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c b/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c
> index 207ca3242a123..7615d27f3b360 100644
> --- a/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c
> +++ b/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c

[ ... ]

> @@ -268,8 +273,145 @@ static void ep_dma_rx_cb(void *data, const struct dmaengine_result *result)
>  	dma_async_issue_pending(xchan->chan);
>  }
>  
> +static void ep_dma_tx_cb(void *data, const struct dmaengine_result *result)
> +{

[ ... ]

> +	ndev = skb->dev;
> +	txq = netdev_get_tx_queue(ndev, skb_get_queue_mapping(skb));
> +	len = skb->len;
> +
> +	if (unlikely(result->result != DMA_TRANS_NOERROR)) {
> +		DEV_STATS_INC(ndev, tx_errors);
> +	} else {
> +		DEV_STATS_INC(ndev, tx_packets);
> +		DEV_STATS_ADD(ndev, tx_bytes, len);
> +	}
> +
> +	dev_consume_skb_any(skb);
> +	netif_txq_completed_wake(txq, 1, len,
> +				 CIRC_SPACE(READ_ONCE(xchan->ring_head),
> +					    READ_ONCE(xchan->ring_tail),
> +					    TX_BD_NUM_DEFAULT), 2);
> +}

[Severity: High]
Since multiple logical queues can share the same physical DMA channel,
will waking only the logical queue associated with this specific SKB
cause other stopped queues mapped to the same channel to starve?

If space frees up in the shared channel, it seems like all stopped
logical queues mapped to this channel would need to be awakened to
prevent transmission stalls.

> +static netdev_tx_t ep_start_xmit(struct sk_buff *skb, struct net_device *ndev)
> +{

[ ... ]

> +	spin_lock_bh(&ep->tx_lock);
> +	if (CIRC_SPACE(xchan->ring_head, READ_ONCE(xchan->ring_tail),
> +		       TX_BD_NUM_DEFAULT) <= 1) {
> +		netif_txq_try_stop(txq,
> +				   CIRC_SPACE(xchan->ring_head,
> +					      READ_ONCE(xchan->ring_tail),
> +					      TX_BD_NUM_DEFAULT),
> +				   2);
> +		spin_unlock_bh(&ep->tx_lock);
> +		if (net_ratelimit())
> +			netdev_warn(ndev, "TSN TX ring full\n");
> +
> +		return NETDEV_TX_BUSY;
> +	}
> +
> +	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);

[Severity: Critical]
Does dropping ep->tx_lock before incrementing xchan->ring_head allow
concurrent calls to ep_start_xmit() on different queues to allocate the
same ring slot?

Since the descriptor slot is reserved by ring_head, another thread could
enter and overwrite the same skbuf_dma slot before ring_head is
incremented, potentially causing data corruption and leaks.

[ ... ]

> +	spin_lock_bh(&ep->tx_lock);
> +	xchan->ring_head++;
> +	netdev_tx_sent_queue(txq, skb->len);
> +	netif_txq_maybe_stop(txq,
> +			     CIRC_SPACE(xchan->ring_head,
> +					READ_ONCE(xchan->ring_tail),
> +					TX_BD_NUM_DEFAULT),
> +			     2, 2);
> +	spin_unlock_bh(&ep->tx_lock);
> +
> +	dmaengine_submit(dma_tx_desc);
> +	dma_async_issue_pending(xchan->chan);
> +
> +	return NETDEV_TX_OK;

[Severity: Critical]
Because dmaengine_submit() is called after dropping ep->tx_lock, could
thread preemption cause the DMA engine to receive submissions out of
order compared to the logical ring_head order?

Since ep_dma_tx_cb() unmaps and frees descriptors strictly based on
xchan->ring_tail order, an out-of-order execution by the hardware could
cause ep_dma_tx_cb() to unmap a slot that is still actively being
transmitted, leading to a use-after-free.

[ ... ]

> @@ -304,10 +446,13 @@ static int ep_open(struct net_device *ndev)
>  static int ep_stop(struct net_device *ndev)
>  {
>  	struct xlnx_tsn_ep *ep = netdev_priv(ndev);
> +	unsigned int i;
>  
>  	netif_tx_disable(ndev);
>  	WRITE_ONCE(ep->closing, true);
>  	ep_exit_dmaengine(ep);
> +	for (i = 0; i < ndev->num_tx_queues; i++)
> +		netdev_tx_reset_subqueue(ndev, i);
>  
>  	return 0;
>  }

[Severity: High]
Does stopping the DMA engine here leak any SKBs that are currently pending
in the TX or RX rings?

ep_exit_dmaengine() cancels pending transactions without executing their
completion callbacks, but there doesn't appear to be a cleanup loop here
to iterate over the rings and unmap/free the left-over skbuf_dma->skb
pointers.

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