Re: [PATCH net-next 15/15] e1000e: Avoid DMA re-mapping on RX copybreak

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
e1000e: Avoid DMA re-mapping on RX copybreak

The patch stops unmapping the RX buffer before the copybreak and error
checks, so recycled skbs keep their DMA mapping and only the consumed path
unmaps. The refill loop is adjusted to skip mapping when buffer_info->dma is
already set, and the copybreak path now uses dma_sync_single_for_cpu() /
dma_sync_single_for_device() around the copy.

No Link: tags are present in the commit header.

> e1000e: Avoid DMA re-mapping on RX copybreak
>
>     The buffer allocation loop is adjusted to not assume that DMA is
>     unmapped, handling mapping errors gracefully.

[Severity: Low]
Is "handling mapping errors gracefully" the whole story for the new
buffer_info->dma = 0 assignment in e1000_alloc_rx_buffers()?

Before this patch, a failed dma_map_single() there left DMA_MAPPING_ERROR
(~(dma_addr_t)0) in buffer_info->dma and broke out of the loop.
e1000_clean_rx_ring() gates its unmap on a non-zero value:

	if (buffer_info->dma) {
		if (adapter->clean_rx == e1000_clean_rx_irq)
			dma_unmap_single(&pdev->dev, buffer_info->dma,
					 adapter->rx_buffer_len,
					 DMA_FROM_DEVICE);

so a down or reset before the next refill unmapped an address that was never
mapped. The new assignment closes that, which makes this hunk a bug fix on
top of the performance work.

Would it make sense to split that one line out with a Fixes: bc7f75fa9788
tag (and possibly Cc: stable), so stable triage can see it separately from
the rest of the change?

> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> index 844f31ab37ad4..b08bcbdb19d97 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -675,6 +675,8 @@ static void e1000_alloc_rx_buffers(struct e1000_ring *rx_ring,
>  		skb = buffer_info->skb;
>  		if (skb) {
>  			skb_trim(skb, 0);
> +			if (likely(buffer_info->dma))
> +				goto write_desc;
>  			goto map_skb;
>  		}
>  
> @@ -692,10 +694,12 @@ static void e1000_alloc_rx_buffers(struct e1000_ring *rx_ring,
>  						  DMA_FROM_DEVICE);
>  		if (dma_mapping_error(&pdev->dev, buffer_info->dma)) {
>  			dev_err(&pdev->dev, "Rx DMA map failed\n");
> +			buffer_info->dma = 0;
>  			adapter->rx_dma_failed++;
>  			break;
>  		}
>  
> +write_desc:
>  		rx_desc = E1000_RX_DESC_EXT(*rx_ring, i);
>  		rx_desc->read.buffer_addr = cpu_to_le64(buffer_info->dma);
>  

[Severity: High]
This isn't a bug introduced by this patch, but should the two sibling
refill routines in the same file be brought in line with the
"buffer_info->dma == 0 means not mapped" contract this hunk makes explicit?

e1000_alloc_rx_buffers_ps() still leaves the error cookie behind:

		if (dma_mapping_error(&pdev->dev, buffer_info->dma)) {
			dev_err(&pdev->dev, "Rx DMA map failed\n");
			adapter->rx_dma_failed++;
			/* cleanup skb */
			dev_kfree_skb_any(skb);
			buffer_info->skb = NULL;
			break;
		}

buffer_info->skb is cleared but buffer_info->dma keeps DMA_MAPPING_ERROR,
which is non-zero, so e1000_clean_rx_ring() on a subsequent down or reset
calls dma_unmap_single() on an address that was never mapped.

e1000_alloc_jumbo_rx_buffers() already uses the same map-skip idiom this
patch adopts, and also skips the zeroing:

		if (!buffer_info->dma) {
			buffer_info->dma = dma_map_page(&pdev->dev,
							buffer_info->page, 0,
							PAGE_SIZE,
							DMA_FROM_DEVICE);
			if (dma_mapping_error(&pdev->dev, buffer_info->dma)) {
				adapter->alloc_rx_buff_failed++;
				break;
			}
		}

After that break, rx_ring->next_to_use points at the failed entry, so the
next refill finds buffer_info->skb and buffer_info->page set, evaluates
!buffer_info->dma as false, skips the mapping and programs the descriptor:

		rx_desc = E1000_RX_DESC_EXT(*rx_ring, i);
		rx_desc->read.buffer_addr = cpu_to_le64(buffer_info->dma);

Can that arm a hardware RX descriptor with the DMA_MAPPING_ERROR cookie as
the bus address after the tail write, and is the same cookie later passed to
dma_unmap_page() from e1000_clean_rx_ring()?

> @@ -967,9 +970,6 @@ static bool e1000_clean_rx_irq(struct e1000_ring *rx_ring, int *work_done,
>  
>  		cleaned = true;
>  		cleaned_count++;
> -		dma_unmap_single(&pdev->dev, buffer_info->dma,
> -				 adapter->rx_buffer_len, DMA_FROM_DEVICE);
> -		buffer_info->dma = 0;
>  
>  		length = le16_to_cpu(rx_desc->wb.upper.length);
>  

[ ... ]

> @@ -994,8 +992,6 @@ static bool e1000_clean_rx_irq(struct e1000_ring *rx_ring, int *work_done,
>  
>  		if (unlikely((staterr & E1000_RXDEXT_ERR_FRAME_ERR_MASK) &&
>  			     !(netdev->features & NETIF_F_RXALL))) {
> -			/* recycle */
> -			buffer_info->skb = skb;
>  			goto next_desc;
>  		}
>  
> @@ -1022,19 +1018,33 @@ static bool e1000_clean_rx_irq(struct e1000_ring *rx_ring, int *work_done,
>  			struct sk_buff *new_skb =
>  				napi_alloc_skb(&adapter->napi, length);
>  			if (new_skb) {
> +				dma_sync_single_for_cpu(&pdev->dev,
> +							buffer_info->dma,
> +							adapter->rx_buffer_len,
> +							DMA_FROM_DEVICE);
>  				skb_copy_to_linear_data_offset(new_skb,
>  							       -NET_IP_ALIGN,
>  							       (skb->data -
>  								NET_IP_ALIGN),
>  							       (length +
>  								NET_IP_ALIGN));

[Severity: High]
This is a pre-existing issue, but since this hunk rewrites the block, is
this a good place to bound length against adapter->rx_buffer_len?

length comes straight from the descriptor written by the NIC:

		length = le16_to_cpu(rx_desc->wb.upper.length);

The new sync window is adapter->rx_buffer_len, while the copy source is the
ring skb and the copy size is length + NET_IP_ALIGN. Can this read past the
end of the RX buffer allocation and hand those bytes to the stack via
e1000_receive_skb() when length exceeds adapter->rx_buffer_len?

The precondition seems to be documented in the driver itself.
e1000_change_mtu() shrinks the allocation to 1522 bytes:

	/* adjust allocation if LPE protects us, and we aren't using SBP */
	if (max_frame <= (VLAN_ETH_FRAME_LEN + ETH_FCS_LEN))
		adapter->rx_buffer_len = VLAN_ETH_FRAME_LEN + ETH_FCS_LEN;

but e1000_setup_rctl() programs the hardware size from the default case:

	switch (adapter->rx_buffer_len) {
	case 2048:
	default:
		rctl |= E1000_RCTL_SZ_2048;
		rctl &= ~E1000_RCTL_BSEX;
		break;

and sets E1000_RCTL_SBP when NETIF_F_RXALL is enabled, which is user
settable through hw_features. With rx-all on, the frame error drop above is
also bypassed, so an oversize single-descriptor frame reaches this copy.

Note the copybreak=2048 setting recommended in the commit message puts a
1523..2048 byte length inside the length < copybreak window.

The sibling e1000 driver took the corresponding fix in commit 9c72a5182ed9
("e1000: fix OOB in e1000_tbi_should_accept()"), which added:

	if (unlikely(length > adapter->rx_buffer_len))
		return false;

> +				dma_sync_single_for_device(&pdev->dev,
> +							   buffer_info->dma,
> +							   adapter->rx_buffer_len,
> +							   DMA_FROM_DEVICE);
> -				/* save the skb in buffer_info as good */
> -				buffer_info->skb = skb;
>  				skb = new_skb;
> +				goto copybreak_done;
>  			}
>  			/* else just continue with the old one */
>  		}
> -		/* end copybreak code */
> +
> +		buffer_info->skb = NULL;
> +		dma_unmap_single(&pdev->dev, buffer_info->dma,
> +				 adapter->rx_buffer_len,
> +				 DMA_FROM_DEVICE);
> +		buffer_info->dma = 0;
> +
> +copybreak_done:
>  		skb_put(skb, length);

[Severity: High]
Same unvalidated length on the fall-through path: the ring skb was allocated
with bufsz = adapter->rx_buffer_len, so can skb_put(skb, length) push
skb->tail past skb->end and reach skb_over_panic() for the oversize case
described above?

[ ... ]