[PATCH net v3] net: atl1c: free the RX buffer when the descriptor reports an error

[PATCH net v3] net: atl1c: free the RX buffer when the descriptor reports an error

[Federico Musto]

atl1c_clean_rx() drops a frame whose return descriptor carries
RRS_RX_ERR_SUM or RRS_802_3_LEN_ERR by calling atl1c_clean_rfd(), which
only marks the RFD free and clears the skb pointer. Nothing unmaps the DMA
mapping and nothing frees the skb, so every errored frame - CRC, alignment,
truncation, runt - leaks one skb and one DMA mapping. The driver uses a
32-bit DMA mask, so on a host without an IOMMU each of those mappings also
pins a swiotlb slot.

Unmap and free every RFD slot referenced by an errored RRD before the slots
are recycled. Decode word0 once in CPU byte order and use it for both the
RFD count and index, so the unmap and the subsequent RFD cleanup always
select the same slots. Share the unmap helper with the good receive path.

Count errored descriptors as completed work so the RX refill also runs when
a poll contains no good frames. Clear the RRD ring on reset, preventing a
stale error descriptor from causing a freshly reposted RX buffer to be
unmapped and freed while the device still owns it.

The hardware already counts these frames in its own error counters, which
atl1c_get_stats() reports, so no software counter is added here.

Fixes: 43250ddd75a3 ("atl1c: Atheros L1C Gigabit Ethernet driver")
Cc: stable@vger.kernel.org
Signed-off-by: Federico Musto <federico@rimpianto.com>
---
v2 -> v3, after the Sashiko AI review:
 - decode rrs->word0 once in CPU byte order and pass the same value to the
   unmap and RFD cleanup paths;
 - unmap and free all RFDs of an errored multi-RFD frame, with ring wrap;
 - count errored descriptors as work so the RX refill cannot starve;
 - clear RRD descriptors on reset so stale error reports cannot free newly
   reposted buffers.

v2: https://lore.kernel.org/netdev/20261102215253.68185-1-federico@rimpianto.com/
v1: https://lore.kernel.org/netdev/20261027115156.17128-1-federico@rimpianto.com/

The issues were found by code inspection; no hardware test has been
performed. The pre-existing memory-ordering and link-reset/NAPI races are
left to separate patches rather than expanding this fix.
---
 .../net/ethernet/atheros/atl1c/atl1c_main.c   | 52 ++++++++++++++-----
 1 file changed, 38 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
index eddd840..0d7d842 100644
--- a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
+++ b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
@@ -912,8 +912,9 @@ static void atl1c_clean_rx_ring(struct atl1c_adapter *adapter, u32 queue)
 		buffer_info = &rfd_ring->buffer_info[j];
 		atl1c_clean_buffer(pdev, buffer_info, 0);
 	}
-	/* zero out the descriptor ring */
+	/* zero out the descriptor rings */
 	memset(rfd_ring->desc, 0, rfd_ring->size);
+	memset(rrd_ring->desc, 0, rrd_ring->size);
 	rfd_ring->next_to_clean = 0;
 	rfd_ring->next_to_use = 0;
 	rrd_ring->next_to_use = 0;
@@ -1846,14 +1847,11 @@ static void atl1c_clean_rrd(struct atl1c_rrd_ring *rrd_ring,
 }
 
 static void atl1c_clean_rfd(struct atl1c_rfd_ring *rfd_ring,
-	struct atl1c_recv_ret_status *rrs, u16 num)
+	u16 rfd_index, u16 num)
 {
 	u16 i;
-	u16 rfd_index;
 	struct atl1c_buffer *buffer_info = rfd_ring->buffer_info;
 
-	rfd_index = (rrs->word0 >> RRS_RX_RFD_INDEX_SHIFT) &
-			RRS_RX_RFD_INDEX_MASK;
 	for (i = 0; i < num; i++) {
 		buffer_info[rfd_index].skb = NULL;
 		ATL1C_SET_BUFFER_STATE(&buffer_info[rfd_index],
@@ -1864,6 +1862,20 @@ static void atl1c_clean_rfd(struct atl1c_rfd_ring *rfd_ring,
 	rfd_ring->next_to_clean = rfd_index;
 }
 
+static struct atl1c_buffer *
+atl1c_unmap_rx_buffer(struct pci_dev *pdev, struct atl1c_rfd_ring *rfd_ring,
+		      u16 rfd_index)
+{
+	struct atl1c_buffer *buffer_info;
+
+	buffer_info = &rfd_ring->buffer_info[rfd_index];
+	if (buffer_info->skb)
+		dma_unmap_single(&pdev->dev, buffer_info->dma,
+				 buffer_info->length, DMA_FROM_DEVICE);
+
+	return buffer_info;
+}
+
 /**
  * atl1c_clean_rx - NAPI Rx polling callback
  * @napi: napi info
@@ -1874,7 +1886,8 @@ static int atl1c_clean_rx(struct napi_struct *napi, int budget)
 	struct atl1c_rrd_ring *rrd_ring =
 		container_of(napi, struct atl1c_rrd_ring, napi);
 	struct atl1c_adapter *adapter = rrd_ring->adapter;
-	u16 rfd_num, rfd_index;
+	u16 rfd_num, rfd_index, i;
+	u32 rrs_word0;
 	u16 length;
 	struct pci_dev *pdev = adapter->pdev;
 	struct net_device *netdev  = adapter->netdev;
@@ -1894,8 +1907,11 @@ static int atl1c_clean_rx(struct napi_struct *napi, int budget)
 			break;
 		rrs = ATL1C_RRD_DESC(rrd_ring, rrd_ring->next_to_clean);
 		if (likely(RRS_RXD_IS_VALID(rrs->word3))) {
-			rfd_num = (rrs->word0 >> RRS_RX_RFD_CNT_SHIFT) &
+			rrs_word0 = le32_to_cpu(rrs->word0);
+			rfd_num = (rrs_word0 >> RRS_RX_RFD_CNT_SHIFT) &
 				RRS_RX_RFD_CNT_MASK;
+			rfd_index = (rrs_word0 >> RRS_RX_RFD_INDEX_SHIFT) &
+				RRS_RX_RFD_INDEX_MASK;
 			if (unlikely(rfd_num != 1))
 				/* TODO support mul rfd*/
 				if (netif_msg_rx_err(adapter))
@@ -1908,11 +1924,22 @@ static int atl1c_clean_rx(struct napi_struct *napi, int budget)
 rrs_checked:
 		atl1c_clean_rrd(rrd_ring, rrs, rfd_num);
 		if (rrs->word3 & (RRS_RX_ERR_SUM | RRS_802_3_LEN_ERR)) {
-			atl1c_clean_rfd(rfd_ring, rrs, rfd_num);
+			/* atl1c_clean_rfd() drops the only pointer to each skb. */
+			for (i = 0; i < rfd_num; i++) {
+				buffer_info = atl1c_unmap_rx_buffer(pdev, rfd_ring,
+								    rfd_index);
+				if (buffer_info->skb)
+					kfree_skb_reason(buffer_info->skb,
+							 SKB_DROP_REASON_NOT_SPECIFIED);
+				atl1c_clean_rfd(rfd_ring, rfd_index, 1);
+				if (++rfd_index == rfd_ring->count)
+					rfd_index = 0;
+			}
 			if (netif_msg_rx_err(adapter))
 				dev_warn(&pdev->dev,
 					 "wrong packet! rrs word3 is %x\n",
 					 rrs->word3);
+			work_done++;
 			continue;
 		}
 
@@ -1920,11 +1947,8 @@ rrs_checked:
 				RRS_PKT_SIZE_MASK);
 		/* Good Receive */
 		if (likely(rfd_num == 1)) {
-			rfd_index = (rrs->word0 >> RRS_RX_RFD_INDEX_SHIFT) &
-					RRS_RX_RFD_INDEX_MASK;
-			buffer_info = &rfd_ring->buffer_info[rfd_index];
-			dma_unmap_single(&pdev->dev, buffer_info->dma,
-					 buffer_info->length, DMA_FROM_DEVICE);
+			buffer_info = atl1c_unmap_rx_buffer(pdev, rfd_ring,
+							    rfd_index);
 			skb = buffer_info->skb;
 		} else {
 			/* TODO */
@@ -1933,7 +1957,7 @@ rrs_checked:
 					"Multi rfd not support yet!\n");
 			break;
 		}
-		atl1c_clean_rfd(rfd_ring, rrs, rfd_num);
+		atl1c_clean_rfd(rfd_ring, rfd_index, rfd_num);
 
 		/* A link partner whose l2mtu is larger than ours can hand us a
 		 * frame that does not fit the buffer the hardware was given:
-- 
2.54.0 (Apple Git-157)

Re: [PATCH net v3] net: atl1c: free the RX buffer when the descriptor reports an error

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 (1) · Medium: 1 (2) · Low: 0 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new errored-frame branch in atl1c_clean_rx()
  (drivers/net/ethernet/atheros/atl1c/atl1c_main.c:1926-1943) unmaps and…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch
  widens it: the link-change worker resets and reposts the RX rings…
- [Medium] In atl1c_clean_rx(), a valid RRD with an error bit set and
  RRS_RX_RFD_CNT == 0 is never consumed.
- [Medium] atl1c_clean_rx() checks RRS_RXD_IS_VALID(rrs->word3) (line
  1909) and then reads rrs->word0 (line 1910) with no dma_rmb() between…
- [Low] struct atl1c_recv_ret_status (atl1c.h) declares word0 and word3
  both as __le32.

--

net: atl1c: free the RX buffer when the descriptor reports an error

This patch changes how atl1c_clean_rx() handles an RRD that reports
RRS_RX_ERR_SUM or RRS_802_3_LEN_ERR. It now unmaps and frees the skb of
every RFD slot the RRD references, where before it only marked those slots
free. It also decodes word0 once with le32_to_cpu(), counts errored
descriptors as work so the RX refill runs, and clears the RRD ring in
atl1c_clean_rx_ring().

> diff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> index eddd8406867f7..0d7d8426ce99b 100644
> --- a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> +++ b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> @@ -912,8 +912,9 @@ static void atl1c_clean_rx_ring(struct atl1c_adapter *adapter, u32 queue)
>  		buffer_info = &rfd_ring->buffer_info[j];
>  		atl1c_clean_buffer(pdev, buffer_info, 0);
>  	}
> -	/* zero out the descriptor ring */
> +	/* zero out the descriptor rings */
>  	memset(rfd_ring->desc, 0, rfd_ring->size);
> +	memset(rrd_ring->desc, 0, rrd_ring->size);

[Severity: High]

This is a pre-existing issue, but does the RRD memset fully prevent what
the commit message describes?

  "Clear the RRD ring on reset, preventing a stale error descriptor from
  causing a freshly reposted RX buffer to be unmapped and freed while the
  device still owns it."

The link-change worker gets here without stopping NAPI:

atl1c_common_task()
  atl1c_irq_disable()
  atl1c_check_link_status()      /* link down branch */
    netif_carrier_off()
    atl1c_reset_mac()
    atl1c_reset_dma_ring()
      atl1c_clean_rx_ring()
        atl1c_clean_buffer()     /* unmap + napi_consume_skb() each RFD */
        memset(rrd_ring->desc, 0, rrd_ring->size);
    atl1c_configure()
      atl1c_alloc_rx_buffer()    /* repost fresh buffers from index 0 */

atl1c_irq_disable() masks the interrupt and calls synchronize_irq(). There
is no napi_disable(), and atl1c_clean_rx() checks netif_carrier_ok() only
on entry.

Suppose a poll on another CPU is already inside its loop and has decoded
rfd_index. Can the new error branch then see buffer_info->skb as non-NULL
and call dma_unmap_single() and kfree_skb_reason() on it? That would
either double unmap and free a buffer that atl1c_clean_buffer() also
frees, or unmap and free a buffer the worker just reposted to the device.

The good receive path has the same race. The notes below the --- line say
the link-reset/NAPI races are left to separate patches. Still, this patch
adds another free site inside that window.

[ ... ]

> @@ -1894,8 +1907,11 @@ static int atl1c_clean_rx(struct napi_struct *napi, int budget)
>  			break;
>  		rrs = ATL1C_RRD_DESC(rrd_ring, rrd_ring->next_to_clean);
>  		if (likely(RRS_RXD_IS_VALID(rrs->word3))) {
> -			rfd_num = (rrs->word0 >> RRS_RX_RFD_CNT_SHIFT) &
> +			rrs_word0 = le32_to_cpu(rrs->word0);
> +			rfd_num = (rrs_word0 >> RRS_RX_RFD_CNT_SHIFT) &
>  				RRS_RX_RFD_CNT_MASK;
> +			rfd_index = (rrs_word0 >> RRS_RX_RFD_INDEX_SHIFT) &
> +				RRS_RX_RFD_INDEX_MASK;

[Severity: Medium]

This isn't a bug introduced by this patch, but should there be a dma_rmb()
between the RRS_RXD_IS_VALID(rrs->word3) check and the load of rrs->word0?
Without one, a weakly ordered CPU can pair a stale word0 with a fresh
word3.

The practical effect looks limited. The RRD and RFD rings advance
one-for-one, as the comment in atl1c_clean_rrd() says ("the relationship
between rrd and rfd is one map one"), so a stale rfd_index at RRD slot i
normally still names RFD slot i. After the new memset, a stale word0 also
gives rfd_num == 0.

The good path has the same missing barrier, and the notes defer the
memory-ordering issues to a separate patch.

> @@ -1908,11 +1924,22 @@ static int atl1c_clean_rx(struct napi_struct *napi, int budget)
>  rrs_checked:
>  		atl1c_clean_rrd(rrd_ring, rrs, rfd_num);
>  		if (rrs->word3 & (RRS_RX_ERR_SUM | RRS_802_3_LEN_ERR)) {

[Severity: Low]

This is a pre-existing issue, but struct atl1c_recv_ret_status declares
word3 as __le32 too. Now that word0 goes through le32_to_cpu(), should
word3 be converted as well?

These accesses all apply CPU-order masks to the raw little-endian value,
so they test the wrong bits on big-endian hosts:

  RRS_RXD_IS_VALID(rrs->word3)
  this error test
  the RRS_PKT_SIZE extraction
  rrs->word3 &= ~RRS_RXD_UPDATED in atl1c_clean_rrd()

The driver was already broken on big-endian before this patch.

> -			atl1c_clean_rfd(rfd_ring, rrs, rfd_num);
> +			/* atl1c_clean_rfd() drops the only pointer to each skb. */
> +			for (i = 0; i < rfd_num; i++) {
> +				buffer_info = atl1c_unmap_rx_buffer(pdev, rfd_ring,
> +								    rfd_index);
> +				if (buffer_info->skb)
> +					kfree_skb_reason(buffer_info->skb,
> +							 SKB_DROP_REASON_NOT_SPECIFIED);
> +				atl1c_clean_rfd(rfd_ring, rfd_index, 1);
> +				if (++rfd_index == rfd_ring->count)
> +					rfd_index = 0;
> +			}

[Severity: Medium]

Can this loop unmap and free a buffer the NIC still owns? rfd_num and
rfd_index come straight from the device-written RRD:

#define RRS_RX_RFD_CNT_MASK	0x000F
#define RRS_RX_RFD_INDEX_MASK	0x0FFF

So rfd_index can be as large as 4095 against a 512-entry RFD ring, and
rfd_num can be as large as 15.

Before any slot is freed, nothing checks that:

  - rfd_index < rfd_ring->count
  - rfd_index is the slot software expects (rfd_ring->next_to_clean is
    written but never read)
  - the slot is ATL1C_BUFFER_BUSY

atl1c_unmap_rx_buffer() only checks buffer_info->skb.

Before this patch, a wrong index or count here leaked the buffer but left
the mapping valid. Now the buffer is unmapped and freed, and the device
can then DMA into freed memory or a released swiotlb slot.

The wrap test is ++rfd_index == rfd_ring->count, so a starting index at or
above count never wraps. Could the loop then walk into another queue's
buffer_info entries, or past the end of the single allocation that
atl1c_setup_ring_resources() shares between the TPD and RFD buffer_info
arrays, and pass whatever it finds to kfree_skb_reason()?

For comparison, alx_clean_rx_irq() uses the same RRD layout. It unmaps by
its software index and schedules a reset on a mismatch:

	if (ALX_GET_FIELD(le32_to_cpu(rrd->word0),
			  RRD_SI) != rxq->read_idx ||
	    ALX_GET_FIELD(le32_to_cpu(rrd->word0),
			  RRD_NOR) != 1) {
		alx_schedule_reset(alx);
		return work;
	}

Would a similar sanity check, or using the software index, make sense
here?

Multi-RFD freeing is also added only on the error path. The good path
still treats rfd_num != 1 as unsupported.

The patch notes say it hasn't been tested on hardware, so it isn't known
how atl1c fills these fields for errored frames.

>  			if (netif_msg_rx_err(adapter))
>  				dev_warn(&pdev->dev,
>  					 "wrong packet! rrs word3 is %x\n",
>  					 rrs->word3);
> +			work_done++;
>  			continue;

[Severity: Medium]

This isn't a bug introduced by this patch, but what happens with a valid
errored RRD whose RFD count is 0?

atl1c_clean_rrd(rrd_ring, rrs, 0) neither clears RRS_RXD_UPDATED nor
advances next_to_clean. The free loop runs zero times, and continue
re-reads the same descriptor.

Before this patch, that looped forever inside a single poll. With the new
work_done++, the poll returns budget and NAPI repolls the same RRD. That
is an improvement, but RX on that queue still makes no progress.

Should rfd_num == 0 be rejected, or the RRD consumed, explicitly? It's not
known whether the hardware ever produces such a descriptor.

>  		}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261103225218.19206-1-federico%40rimpianto.com