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

[netdev-bot+sashiko@kernel.org]

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