Re: [PATCH net-next v3 7/8] net: xilinx: tsn: add the endpoint TX data path

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] net: Use-after-free of skb in ep_start_xmit() due to late timestamping
--

commit 6fa02caf949cb04194e431c851b503c66e08312c
Author: Srinivas Neeli <srinivas.neeli@xxxxxxx>

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

This commit adds the transmit path so the endpoint can send frames. It
selects the TX channel by indexing tx_chans[] with the frame's logical
queue, maps the frame, and submits it to the DMA engine.

> diff --git a/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c b/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c
> index 0a13e04d40575..bd8cd504cefb4 100644
> --- a/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c
> +++ b/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c

[ ... ]

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

[ ... ]

> +	spin_lock_bh(&xchan->tx_lock);
> +	cookie = dmaengine_submit(dma_tx_desc);
> +	if (dma_submit_error(cookie)) {
> +		spin_unlock_bh(&xchan->tx_lock);
> +		skbuf_dma->skb = NULL;
> +		goto err_unmap_sg;
> +	}
> +	WRITE_ONCE(xchan->ring_head, xchan->ring_head + 1);
> +	netdev_tx_sent_queue(txq, skb->len);
> +	netif_txq_maybe_stop(txq,
> +			     CIRC_SPACE(xchan->ring_head,
> +					READ_ONCE(xchan->ring_tail),
> +					xchan->ring_size),
> +			     2, 2);
> +	spin_unlock_bh(&xchan->tx_lock);
> +
> +	skb_tx_timestamp(skb);

[Severity: High]
Is there a use-after-free risk for the skb here?

If dmaengine_submit() successfully transfers ownership of the descriptor to
the DMA engine, and the lock is subsequently released via spin_unlock_bh(),
could the following sequence occur:

CPU1 calls dmaengine_submit() and unlocks xchan->tx_lock.

The hardware begins processing the descriptor and triggers the completion
callback on another CPU.

CPU2 executes ep_dma_tx_cb(), unmaps the buffer, and frees the skb using
dev_consume_skb_any(skb).

CPU1 resumes execution and calls skb_tx_timestamp(skb), which attempts to
access the shinfo area of the already-freed skb.

Should skb_tx_timestamp() be called before the transmit lock is dropped,
or prior to handing the skb over to dmaengine_submit()?

> +	dma_async_issue_pending(xchan->chan);
> +
> +	return NETDEV_TX_OK;
> +
> +err_unmap_sg:
> +	dma_unmap_sg(xchan->dma_dev, skbuf_dma->sgl, sg_len, DMA_TO_DEVICE);
> +err_drop_skb:
>  	dev_kfree_skb_any(skb);
>  	DEV_STATS_INC(ndev, tx_dropped);
>  	return NETDEV_TX_OK;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016-patches_v2_external-v3-0-e33d3960ca1e@xxxxxxx?part=7