[PATCH net] net: gianfar: fix DMA unmap of time stamped frames at teardown

[PATCH net] net: gianfar: fix DMA unmap of time stamped frames at teardown

[Rosen Penev]

free_skb_tx_queue() walks the TxBDs assuming each frame occupies
one descriptor plus one more per fragment.  A frame sent with
hardware time stamping instead consumes one additional TxBD for
the time stamp buffer, which sits between the FCB and the frame
data and belongs to the head DMA mapping.  The current walk then
lands on the wrong descriptors: it treats the time stamp BD as a
fragment (unmapping the still-outstanding time stamp buffer) while
the real fragment descriptors are skipped, so their DMA mappings
leak and remain attached to a skb that is about to be freed.

Fix the walk the same way gfar_clean_tx_ring() does on the
transmit path: identify time stamped frames, derive the head
buffer length from the time stamp BD length plus GMAC_FCB_LEN and
GMAC_TXPAL_LEN, skip the time stamp BD without unmapping it, and
start the fragment recycling on the correct descriptor.

Fixes: f0ee7acfcdd4 ("gianfar: Add hardware TX timestamping support")
Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/net/ethernet/freescale/gianfar.c | 30 ++++++++++++++++++++----
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index cf636fc5aafa..c5a716d91fd1 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -1064,21 +1064,41 @@ static void free_skb_tx_queue(struct gfar_priv_tx_q *tx_queue)
 	txbdp = tx_queue->tx_bd_base;
 
 	for (i = 0; i < tx_queue->tx_ring_size; i++) {
-		if (!tx_queue->tx_skbuff[i])
+		struct sk_buff *skb = tx_queue->tx_skbuff[i];
+		bool do_tstamp;
+		int buflen;
+
+		if (!skb)
 			continue;
 
+		do_tstamp = (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
+			    priv->hwts_tx_en;
+
+		/* Sending a time stamped frame requires two additional
+		 * buffers, the time stamp buffer itself being between the
+		 * FCB and the actual frame data, all mapped together.
+		 */
+		if (unlikely(do_tstamp))
+			buflen = be16_to_cpu(txbdp[1].length) +
+				 GMAC_FCB_LEN + GMAC_TXPAL_LEN;
+		else
+			buflen = be16_to_cpu(txbdp->length);
+
 		dma_unmap_single(priv->dev, be32_to_cpu(txbdp->bufPtr),
-				 be16_to_cpu(txbdp->length), DMA_TO_DEVICE);
+				 buflen, DMA_TO_DEVICE);
 		txbdp->lstatus = 0;
-		for (j = 0; j < skb_shinfo(tx_queue->tx_skbuff[i])->nr_frags;
-		     j++) {
+
+		if (unlikely(do_tstamp))
+			txbdp++;
+
+		for (j = 0; j < skb_shinfo(skb)->nr_frags; j++) {
 			txbdp++;
 			dma_unmap_page(priv->dev, be32_to_cpu(txbdp->bufPtr),
 				       be16_to_cpu(txbdp->length),
 				       DMA_TO_DEVICE);
 		}
 		txbdp++;
-		dev_kfree_skb_any(tx_queue->tx_skbuff[i]);
+		dev_kfree_skb_any(skb);
 		tx_queue->tx_skbuff[i] = NULL;
 	}
 	kfree(tx_queue->tx_skbuff);
-- 
2.55.0

Re: [PATCH net] net: gianfar: fix DMA unmap of time stamped frames at teardown

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: gianfar: fix DMA unmap of time stamped frames at teardown

This teaches free_skb_tx_queue() that a time stamped frame occupies an
extra TxBD for the time stamp buffer: the head unmap length is derived
from the time stamp BD length plus GMAC_FCB_LEN and GMAC_TXPAL_LEN, and
the time stamp BD is skipped before the fragment loop runs.  The intent
is to mirror what gfar_clean_tx_ring() does on the transmit path.

A couple of questions about the new walk below.

> diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
> index cf636fc5aafa..c5a716d91fd1 100644
> --- a/drivers/net/ethernet/freescale/gianfar.c
> +++ b/drivers/net/ethernet/freescale/gianfar.c
> @@ -1064,21 +1064,41 @@ static void free_skb_tx_queue(struct gfar_priv_tx_q *tx_queue)
>  	txbdp = tx_queue->tx_bd_base;
>  
>  	for (i = 0; i < tx_queue->tx_ring_size; i++) {
> -		if (!tx_queue->tx_skbuff[i])
> +		struct sk_buff *skb = tx_queue->tx_skbuff[i];
> +		bool do_tstamp;
> +		int buflen;
> +
> +		if (!skb)
>  			continue;

[Severity: High]
The commit message says the walk is fixed "the same way gfar_clean_tx_ring()
does on the transmit path", but the cursor here is still anchored at
tx_queue->tx_bd_base and it is not advanced when a tx_skbuff[] slot is NULL.
Should it start from tx_queue->dirty_tx / tx_queue->skb_dirtytx like the
reference implementation does?

gfar_clean_tx_ring() anchors both cursors and advances them together:

	bdp = tx_queue->dirty_tx;
	skb_dirtytx = tx_queue->skb_dirtytx;

	while ((skb = tx_queue->tx_skbuff[skb_dirtytx])) {
		...
		skb_dirtytx = (skb_dirtytx + 1) &
			      TX_RING_MOD_MASK(tx_ring_size);
	}

gfar_init_bds() only resets dirty_tx and skb_dirtytx to the ring base at
startup, so at teardown they sit wherever transmit completion left them.
Outstanding frames live in [dirty_tx, cur_tx) with skb indices
[skb_dirtytx, skb_curtx).

Given that, when skb_dirtytx != 0 does the first processed skb get unmapped
using descriptor 0, i.e. a descriptor belonging to an already completed
frame?  gfar_clear_txbd_status() in gianfar.h keeps only the wrap bit and
never clears bufPtr:

	lstatus &= BD_LFLAG(TXBD_WRAP);
	bdp->lstatus = cpu_to_be32(lstatus);

so the dma_unmap_single() below would use a stale bufPtr with length 0, the
fragment loop would then dma_unmap_page() further stale bufPtr values, and
the head plus fragment mappings of the skb actually being freed are never
released.  Is that not the same leak the commit message is trying to remove?

stop_gfar() does netif_tx_stop_all_queues(), set_bit(GFAR_DOWN),
disable_napi(), gfar_halt() and then free_skb_resources(), without draining
the TX ring, so non-NULL tx_skbuff[] entries with live mappings look like the
normal case here - in particular on the
gfar_timeout() -> gfar_reset_task() -> reset_gfar() -> stop_gfar() path.

> +		do_tstamp = (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
> +			    priv->hwts_tx_en;

[Severity: Medium]
Can priv->hwts_tx_en differ here from the value that was in effect when the
frame was enqueued?  gfar_start_xmit() computes the same expression and that
is where the descriptor topology is fixed (frags + 2 BDs, head BD length set
to GMAC_FCB_LEN, time stamp BD length set to skb_headlen(skb) - fcb_len).

gfar_hwtstamp_set() flips the flag with no lock, no WRITE_ONCE, no TX drain
and no reset_gfar() on the tx_type transitions:

	case HWTSTAMP_TX_OFF:
		priv->hwts_tx_en = 0;
		break;
	case HWTSTAMP_TX_ON:
		if (!(priv->device_flags & FSL_GIANFAR_DEV_HAS_TIMER))
			return -ERANGE;
		priv->hwts_tx_en = 1;
		break;

For the on-to-off direction, if a genuinely time stamped frame is still in
tx_skbuff[] when the flag is cleared, does this take the non-tstamp branch
and unmap only be16_to_cpu(txbdp->length) == GMAC_FCB_LEN bytes of a
skb_headlen()-sized mapping, then dma_unmap_page() the time stamp BD whose
bufPtr is an interior offset of that same single mapping, leaving the real
fragment BDs unvisited?

For the off-to-on direction, an ordinary frame carrying SKBTX_HW_TSTAMP set
by the socket takes the tstamp branch, sizes the head unmap from
txbdp[1].length belonging to another frame, skips one real fragment BD, and
consumes one BD more than the frame occupied.

Would keying this on SKBTX_IN_PROGRESS, which gfar_start_xmit() sets only
when the time stamp BD was actually emitted, or on a recorded per-frame BD
count, keep the teardown view consistent with what was mapped?

reset_gfar() reaches free_skb_resources() from gfar_reset_task, a workqueue
without rtnl, holding only the GFAR_RESETTING bit that the tx_type path of
gfar_hwtstamp_set() does not test, so the flag can also change while this
walk is running.

> +
> +		/* Sending a time stamped frame requires two additional
> +		 * buffers, the time stamp buffer itself being between the
> +		 * FCB and the actual frame data, all mapped together.
> +		 */
> +		if (unlikely(do_tstamp))
> +			buflen = be16_to_cpu(txbdp[1].length) +
> +				 GMAC_FCB_LEN + GMAC_TXPAL_LEN;
> +		else
> +			buflen = be16_to_cpu(txbdp->length);
> +
>  		dma_unmap_single(priv->dev, be32_to_cpu(txbdp->bufPtr),
> -				 be16_to_cpu(txbdp->length), DMA_TO_DEVICE);
> +				 buflen, DMA_TO_DEVICE);
>  		txbdp->lstatus = 0;
> -		for (j = 0; j < skb_shinfo(tx_queue->tx_skbuff[i])->nr_frags;
> -		     j++) {
> +
> +		if (unlikely(do_tstamp))
> +			txbdp++;
> +
> +		for (j = 0; j < skb_shinfo(skb)->nr_frags; j++) {
>  			txbdp++;
>  			dma_unmap_page(priv->dev, be32_to_cpu(txbdp->bufPtr),
>  				       be16_to_cpu(txbdp->length),
>  				       DMA_TO_DEVICE);
>  		}

[Severity: High]
The new txbdp[1] read and the new txbdp++ use plain pointer arithmetic
rather than the ring helpers.  Should these use next_txbd() / skip_txbd()?

	static inline struct txbd8 *next_txbd(struct txbd8 *bdp, struct txbd8 *base,
					      int ring_size)
	{
		return skip_txbd(bdp, 1, base, ring_size);
	}

gfar_start_xmit() and gfar_clean_tx_ring() step the ring exclusively with
those, and gfar_clean_tx_ring() reads the time stamp BD length through
next_txbd() for exactly this case:

		if (unlikely(do_tstamp)) {
			next = next_txbd(bdp, base, tx_ring_size);
			buflen = be16_to_cpu(next->length) +
				 GMAC_FCB_LEN + GMAC_TXPAL_LEN;
		} else
			buflen = be16_to_cpu(bdp->length);

Without wrapping, when the cursor lands on the last entry of the ring does
txbdp[1] read outside this queue's BD array?  gfar_alloc_skb_resources()
places all tx rings and then all rx rings in one dma_alloc_coherent() block:

	vaddr = dma_alloc_coherent(dev,
				   (priv->total_tx_ring_size *
				    sizeof(struct txbd8)) +
				   (priv->total_rx_ring_size *
				    sizeof(struct rxbd8)),

so the length would come from the adjacent tx ring or from an RxBD whose
length field is written by hardware, and that value is then handed to
dma_unmap_single() / dma_unmap_page().

Both of these look reachable from ordinary teardown - ip link set dev ethX
down, an MTU change, an ethtool ring resize, or the tx-timeout reset path.

Thanks for looking at this code; the time stamp BD handling was clearly
missing here.
-- 
pw-bot: cr