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

[PATCH net v2] 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 before the RFD is recycled.  Factor the RFD lookup and
DMA unmap into a helper shared with the good path, using le32_to_cpu() when
decoding the descriptor index.  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>
---
v1 -> v2, after Paolo Abeni's review:
 - decode rrs->word0 with le32_to_cpu() before extracting the RFD index;
   this fixes the sparse warning on the new code
 - factor the RFD lookup and DMA unmap into atl1c_unmap_rx_buffer(), shared
   by the error and good paths instead of duplicating them

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

Spotted while reviewing a fix for a different problem in the same function
(skb_over_panic on oversized RX frames), by the Sashiko review bot.  It is
independent of that patch and applies on its own.

The guard on buffer_info->skb is there because the slot can be empty after a
reset; rfd_num != 1 stays on the old path, since multi-RFD is not supported
by this driver anywhere else either.
---
 .../net/ethernet/atheros/atl1c/atl1c_main.c   | 33 +++++++++++++++----
 1 file changed, 27 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
index eddd840..689941e 100644
--- a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
+++ b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
@@ -1864,6 +1864,23 @@ 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,
+		      struct atl1c_recv_ret_status *rrs)
+{
+	struct atl1c_buffer *buffer_info;
+	u16 rfd_index;
+
+	rfd_index = (le32_to_cpu(rrs->word0) >> RRS_RX_RFD_INDEX_SHIFT) &
+		    RRS_RX_RFD_INDEX_MASK;
+	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 +1891,7 @@ 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;
 	u16 length;
 	struct pci_dev *pdev = adapter->pdev;
 	struct net_device *netdev  = adapter->netdev;
@@ -1908,6 +1925,14 @@ 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() drops the only pointer to the skb. */
+			if (likely(rfd_num == 1)) {
+				buffer_info = atl1c_unmap_rx_buffer(pdev, rfd_ring,
+								    rrs);
+				if (buffer_info->skb)
+					kfree_skb_reason(buffer_info->skb,
+							 SKB_DROP_REASON_NOT_SPECIFIED);
+			}
 			atl1c_clean_rfd(rfd_ring, rrs, rfd_num);
 			if (netif_msg_rx_err(adapter))
 				dev_warn(&pdev->dev,
@@ -1920,11 +1945,7 @@ 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, rrs);
 			skb = buffer_info->skb;
 		} else {
 			/* TODO */
-- 
2.54.0 (Apple Git-157)

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[netdev-bot+sashiko]

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

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

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).

- [High] The new error-path free can release a buffer the device still
  owns when a stale RRD is processed after a reset.
- [Medium] The patch converts only one of the two decodes of the same RFD
  index.
- [Medium] The leak fix covers only single-RFD frames.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch
  edits this exact branch.
- [High] This problem wasn't introduced by this patch, but the new error-
  path unmap/free adds another site exposed to it.
- [High] This problem wasn't introduced by this patch, but the patch
  notes rely on the same state.
- [Medium] This problem wasn't introduced by this patch.

--

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

Before this commit, atl1c_clean_rx() only recycled the RFD for a return
descriptor flagged RRS_RX_ERR_SUM or RRS_802_3_LEN_ERR. Now it also
unmaps and frees the skb on that path. The RFD index lookup and DMA unmap
move into a new helper, atl1c_unmap_rx_buffer(), which the good receive
path also uses, and the index is now decoded with le32_to_cpu().

> diff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> index eddd8406867f7..689941e46ff43 100644
> --- a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> +++ b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> @@ -1864,6 +1864,23 @@ 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,
> +		      struct atl1c_recv_ret_status *rrs)
> +{
> +	struct atl1c_buffer *buffer_info;
> +	u16 rfd_index;
> +
> +	rfd_index = (le32_to_cpu(rrs->word0) >> RRS_RX_RFD_INDEX_SHIFT) &
> +		    RRS_RX_RFD_INDEX_MASK;

[Severity: Medium]
Only one of the two decoders of this index is converted.
atl1c_clean_rx() calls atl1c_clean_rfd() on the same rrs right after this
helper, on both the error and good paths. In
drivers/net/ethernet/atheros/atl1c/atl1c_main.c that function still decodes
the field raw:

atl1c_clean_rfd() {
	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],
					ATL1C_BUFFER_FREE);
	...
}

rfd_num in atl1c_clean_rx() is also extracted from the raw rrs->word0.

On a big-endian host, could these two decodes pick different slots? The
helper would unmap slot X and free its skb (or hand it to the stack).
atl1c_clean_rfd() would then clear a different slot Y and mark it
ATL1C_BUFFER_FREE.

Slot X would stay marked busy with stale skb and dma values. A later
atl1c_clean_rx_ring()->atl1c_clean_buffer() would unmap it a second time
and call napi_consume_skb() on an skb that is already freed or owned by the
stack. The mapping for slot Y would leak.

Before this patch both sites used the same raw index, so they at least
agreed. Big-endian RX in this driver already decodes word3 raw, so these
setups were already broken in other ways.

The commit message says "using le32_to_cpu() when decoding the descriptor
index", but this is the only decoder that does. Could word0 be decoded once
into a CPU-endian local, and that value used for rfd_num, for this helper
and for atl1c_clean_rfd()?

[Severity: Medium]
This is a pre-existing issue, but this helper adds another reader of
rrs->word0. atl1c_clean_rx() checks the valid bit the device writes, then
reads word0 with no barrier in between:

	if (likely(RRS_RXD_IS_VALID(rrs->word3))) {
		rfd_num = (rrs->word0 >> RRS_RX_RFD_CNT_SHIFT) &
			RRS_RX_RFD_CNT_MASK;

atl1c_main.c has no dma_rmb() or rmb() anywhere. On weakly ordered CPUs
such as arm64 or ppc, can the word0 load (or the vlan_tag and rss_hash
loads) complete before the word3 load and return a value from the previous
lap?

In steady state RRD slot p maps to RFD slot p, so a stale word0 usually
gives the same rfd_index, which limits the damage. Would a dma_rmb() after
the RRS_RXD_IS_VALID() check be appropriate?

[ ... ]

> @@ -1908,6 +1925,14 @@ 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() drops the only pointer to the skb. */
> +			if (likely(rfd_num == 1)) {

[Severity: Medium]
Are the skbs and DMA mappings still leaked when an errored descriptor
reports more than one RFD? The unmap and free only run when rfd_num == 1.
atl1c_clean_rfd() runs either way and walks all rfd_num slots:

	for (i = 0; i < num; i++) {
		buffer_info[rfd_index].skb = NULL;
		ATL1C_SET_BUFFER_STATE(&buffer_info[rfd_index],
					ATL1C_BUFFER_FREE);

rfd_num comes from a 4-bit field written by the hardware. The earlier
rfd_num != 1 check in atl1c_clean_rx() only prints "Multi rfd not support
yet!" before jumping to rrs_checked, so such a descriptor still reaches
this branch.

The driver doesn't show whether the hardware ever spreads an errored frame
over several RFDs. Since the frame is dropped anyway, could this path walk
all rfd_num slots, with ring wrap, and unmap and free each one?

> +				buffer_info = atl1c_unmap_rx_buffer(pdev, rfd_ring,
> +								    rrs);
> +				if (buffer_info->skb)
> +					kfree_skb_reason(buffer_info->skb,
> +							 SKB_DROP_REASON_NOT_SPECIFIED);
> +			}

[Severity: High]
Can this free a buffer the device still owns, when a stale RRD is
processed after a reset?

The RRD ring is only zeroed when it is allocated at open. Some resets keep
the existing rings: the reset worker in atl1c_common_task(),
atl1c_reinit_locked(), and the link-down path in atl1c_check_link_status().
On those, atl1c_clean_rx_ring() clears only the RFD descriptors:

	memset(rfd_ring->desc, 0, rfd_ring->size);
	rfd_ring->next_to_clean = 0;
	rfd_ring->next_to_use = 0;
	rrd_ring->next_to_use = 0;
	rrd_ring->next_to_clean = 0;

atl1c_down() calls napi_disable() before atl1c_reset_mac(). Any RRD the
hardware writes in that window keeps RRS_RXD_UPDATED set.

The refill then posts fresh mapped skbs to RFD slots 0..count-2. Software
can reach a stale index before the device rewrites it. RRD and RFD indices
advance in lockstep, so the stale rfd_index names a slot that was just
reposted.

If the stale word3 has RRS_RX_ERR_SUM or RRS_802_3_LEN_ERR set, this new
code unmaps and frees that skb, and the device can later DMA into the
freed buffer.

Before this patch, the same stale error descriptor only leaked the skb. The
if (buffer_info->skb) guard only covers the one empty slot, count-1. The
good path already had this hazard before this patch.

Would clearing the RRD ring on reset, or checking rfd_index against
rfd_ring->next_to_clean, close this?

[Severity: High]
This isn't a bug introduced by this patch, but the new unmap and free here
is one more site exposed to it. The link-change worker tears down and
refills the RX rings without stopping RX NAPI:

atl1c_common_task()
    atl1c_irq_disable()
    atl1c_check_link_status()
        atl1c_reset_dma_ring()
            atl1c_clean_rx_ring()
                atl1c_clean_buffer()
        atl1c_configure()

atl1c_irq_disable() masks the interrupt and calls synchronize_irq().
Nothing on this path calls napi_disable() or napi_synchronize().
atl1c_clean_rx() checks netif_carrier_ok() only at poll entry.

A poll may already be inside its loop on another CPU, for example threaded
NAPI preempted mid-poll. Can atl1c_clean_buffer() then find this slot still
busy while atl1c_clean_rx() is between atl1c_unmap_rx_buffer() and
atl1c_clean_rfd()?

atl1c_clean_buffer() {
	if (buffer_info->flags & ATL1C_BUFFER_FREE)
		return;
	...
	if (buffer_info->skb)
		napi_consume_skb(buffer_info->skb, budget);
	...
}

That would mean a double dma_unmap_single() and a double free of the skb.
The two sides would also race on next_to_use and next_to_clean.

Should the link-down path call napi_disable() on the RX rings before
atl1c_reset_dma_ring()?

>  			atl1c_clean_rfd(rfd_ring, rrs, rfd_num);
>  			if (netif_msg_rx_err(adapter))
>  				dev_warn(&pdev->dev,

[Severity: High]
This isn't a bug introduced by this patch, but this branch is being changed
anyway. It recycles the RFD and then hits continue without counting the
work:

			atl1c_clean_rfd(rfd_ring, rrs, rfd_num);
			...
			continue;
		}

The only RX-time refill is gated on work_done. It is also the only place
the RFD producer index is written during RX:

	if (work_done)
		atl1c_alloc_rx_buffer(adapter, rrd_ring->num, true);

Suppose about a ring's worth of errored frames (CRC, runt, 802.3 length
errors) arrives with no good frame in between. Can the device then run out
of RFDs?

Once that happens it stops writing RRDs, so there is no RX interrupt, no
poll and no refill. atl1c_intr() only logs ISR_OVER. RX would stay stopped
until an administrative reset.

The oversized-frame drop in the same function already does work_done++ for
this reason. Should the error path do the same? That would also let the
budget check limit polls that see only errored frames.

[ ... ]

> @@ -1920,11 +1945,7 @@ static int atl1c_clean_rx(struct napi_struct *napi, int budget)
>  				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, rrs);
>  			skb = buffer_info->skb;

[Severity: High]
This isn't a bug introduced by this patch. The patch notes say the
if (buffer_info->skb) guard is there because "the slot can be empty after a
reset". Can the good path hit that same empty slot? skb is used with no
NULL check:

		skb_put(skb, length - ETH_FCS_LEN);
		skb->protocol = eth_type_trans(skb, netdev);

After a reset that keeps the existing rings, atl1c_alloc_rx_buffer()
refills slots 0..count-2 and leaves slot count-1 with skb == NULL.
atl1c_clean_buffer() doesn't reset buffer_info->length, so that slot keeps
its old rx_buffer_len and the length > buffer_info->length check still
passes.

Only rfd_ring->desc is cleared on reset. A valid RRD at index count-1 that
was not processed before the reset survives it. When software reaches that
index in the first lap after the reset, would it take this path with a NULL
skb and oops in skb_put() or eth_type_trans() in NAPI context?

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

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

[Federico Musto]

Thank you for the detailed review. The findings on endian decoding,
multi-RFD error handling, stale RRDs after reset and missing error-path
work accounting were valid and have been addressed in v3.

The v3 now:
- decodes rrs->word0 once in CPU byte order and passes the decoded RFD
  index to both the unmap and cleanup paths;
- unmaps, frees and recycles every RFD of an errored multi-RFD frame,
  with ring wrap;
- counts errored descriptors as work so the RX refill runs;
- clears the RRD ring on reset, preventing stale error descriptors from
  freeing newly reposted buffers.

The memory-ordering and link-reset/NAPI issues are pre-existing and are
left to separate patches rather than expanding this fix.

Validation on netdev/net:
- scripts/checkpatch.pl: 0 errors, 0 warnings, 0 checks;
- baseline and patched W=1 + Sparse builds both succeeded;
- no new warnings; three endian-related Sparse warnings removed;
- git apply --check and git diff --check passed.

The issues were found by code inspection; no hardware test has been
performed.

pw-bot: cr