Re: [PATCH net-next v7 05/18] net: ethernet: cortina: Correct free queue DMA mappings

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 (5) · 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).

- [Medium] geth_freeq_lookup() now holds the global geth->freeq_lock with
  spin_lock_irqsave() while it scans all num_freeq_pages entries…

Pre-existing issues:
- [Medium] After this patch, geth_freeq_alloc_map_page() still overwrites
  geth->freeq_pages[pn].page and .mapping (gpage->mapping =…
- [Medium] When a free-queue page is replaced,
  geth_freeq_alloc_map_page() overwrites the old page's PAGE_SIZE…
- [Medium] In this patch, gmac_rx() takes frag_len from the 16-bit
  hardware fields word0.bits.buffer_size and word1.bits.byte_count.
- [Medium] geth_cleanup_freeq() drains references with 'while
  (page_ref_count(gpage->page) > 0) put_page(gpage->page);'.
- [Medium] Free-queue pages use long-lived DMA_FROM_DEVICE streaming
  mappings.

--

net: ethernet: cortina: Correct free queue DMA mappings

This patch keeps the free queue page DMA base separate from the fragment
iterator, drops the replacement path's unmap of the wrong buffer, and uses
the saved base and PAGE_SIZE for setup unwind and teardown. It replaces
gmac_get_queue_page() with geth_freeq_lookup(), which derives the fragment
offset from the saved base and snapshots page, base and offset under
freeq_lock. It also rejects mappings that cannot fit in the 32-bit
hardware descriptors.

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index fa51f2b54073f..71d7e61895312 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -726,32 +726,43 @@ static int gmac_setup_rxq(struct net_device *netdev)

[ ... ]

> +	spin_lock_irqsave(&geth->freeq_lock, flags);
>  	if (!geth->freeq_pages) {
>  		dev_err_ratelimited(geth->dev,
>  				    "try to get page with no page list\n");
> -		return NULL;
> +		goto unlock;
>  	}
>  
>  	/* Look up a ring buffer page from virtual mapping */
>  	for (i = 0; i < geth->num_freeq_pages; i++) {
>  		gpage = &geth->freeq_pages[i];
> -		if (gpage->mapping == mapping)
> -			return gpage;
> +		if (!gpage->page || mapping < gpage->mapping)
> +			continue;
> +
> +		page_mapping = gpage->mapping;
> +		if (mapping - page_mapping > PAGE_SIZE - frag_len ||
> +		    ((mapping - page_mapping) & (frag_len - 1)))
> +			continue;
> +
> +		page = gpage->page;
> +		*page_offs = mapping - page_mapping;
> +		break;
>  	}
>  
> -	return NULL;
> +unlock:
> +	spin_unlock_irqrestore(&geth->freeq_lock, flags);
> +	return page;
>  }

[Severity: Medium]
geth_freeq_lookup() now takes the shared geth->freeq_lock with
spin_lock_irqsave() and holds it while it scans all num_freeq_pages
entries one by one. gmac_rx() calls it once per RX descriptor. With
default sizing that is about 1024 iterations per descriptor with IRQs
off, and it contends with refill and with the other port.

Is this extra IRQ-off latency on the RX hot path acceptable for this
intermediate state?

The next patch in the series, "net: ethernet: cortina: Index free queue
fragments with XArray", replaces the scan with a single xa_load() under
the lock. The cost only exists between these two patches.

[ ... ]

> @@ -835,31 +855,25 @@ static struct page *geth_freeq_alloc_map_page(struct gemini_ethernet *geth,
>  	 */
>  	frag_len = 1 << geth->freeq_frag_order; /* Usually 2048 */
>  	fpp_order = PAGE_SHIFT - geth->freeq_frag_order;
> +	gpage = &geth->freeq_pages[pn];
> +	if (gpage->page)
> +		put_page(gpage->page);

[Severity: Medium]
This isn't a bug introduced by this patch, but when a slot is replaced
here, the old page's PAGE_SIZE streaming mapping in gpage->mapping is
overwritten without being unmapped. geth_cleanup_freeq() and the
geth_setup_freeq() unwind path only unmap the slot's current
gpage->mapping.

Does the replaced DMA mapping leak? Can the page also go back to the
allocator while it is still mapped?

The old code never unmapped the old mapping either. It unmapped the first
fragment of the new buffer instead. The commit message also defers this
to later in the series.

At the end of the series this is handled: geth_freeq_recycle_page() calls
dma_unmap_single_attrs() before it drops a page that is still referenced,
geth_freeq_release_pages() unmaps every live slot, and slots are no longer
overwritten while their fragments are in flight. These changes come from
"net: ethernet: cortina: Preserve in-flight free queue pages" and
"Recycle claimed free queue pages".

> +
> +	/* Then put our new mapping into the page table */
> +	gpage->mapping = page_mapping;
> +	gpage->page = page;

[Severity: Medium]
This is a pre-existing issue, but the slot metadata is still overwritten
here when geth_fill_freeq() replaces a page whose page_ref_count() > 1.
RX descriptors can still hold fragment addresses from the replaced page,
and geth_freeq_lookup() only matches each slot's current base:

geth_fill_freeq()
  geth_freeq_alloc_map_page()
    gpage->mapping = page_mapping;  <- old base is forgotten
gmac_rx()
  geth_freeq_lookup()               <- NULL for an old-page fragment
  goto err_drop;                    <- page == NULL, no put_page()

Could this drop the frame and leak that fragment's page reference?

gmac_cleanup_rxq() would hit the same NULL return and skip put_page().

The baseline behaved the same way, and the commit message defers the
lifetime of in-flight fragments to later in the series.

At the end of the series, "net: ethernet: cortina: Preserve in-flight
free queue pages" fixes this. Metadata slots are allocated independently
of the free queue position through geth_freeq_alloc_slot(), and each
fragment address stays in geth->freeq_mappings until geth_freeq_claim()
claims it.

[ ... ]

> @@ -1023,12 +1034,10 @@ static void geth_cleanup_freeq(struct gemini_ethernet *geth)
>  
>  	for (pn = 0; pn < pages; pn++) {
>  		struct gmac_queue_page *gpage;
> -		dma_addr_t mapping;
> -
> -		mapping = geth->freeq_ring[pn << fpp_order].word2.buf_adr;
> -		dma_unmap_single(geth->dev, mapping, frag_len, DMA_FROM_DEVICE);
>  
>  		gpage = &geth->freeq_pages[pn];
> +		dma_unmap_single(geth->dev, gpage->mapping, PAGE_SIZE,
> +				 DMA_FROM_DEVICE);
>  		while (page_ref_count(gpage->page) > 0)
>  			put_page(gpage->page);

[Severity: Medium]
This isn't a bug introduced by this patch, but this while loop drops
references the driver doesn't own, including references held by skbs
that were already handed to the stack. It also re-reads the refcount
after the page may already have been freed.

Can this over-put a page, or read a freed one, at queue teardown (ndo_stop
or a ring resize)?

At the end of the series, geth_cleanup_freeq() calls
geth_freeq_release_pages() instead. That function puts exactly
gpage->fragments outstanding hardware references plus the single base
reference ("net: ethernet: cortina: Preserve in-flight free queue pages").

[ ... ]

> @@ -1523,8 +1531,6 @@ static unsigned int gmac_rx(struct net_device *netdev, unsigned int budget,
>  
>  		frag_len = word0.bits.buffer_size;
>  		frame_len = word1.bits.byte_count;
> -		page_offs = mapping & ~PAGE_MASK;
> -

[Severity: Medium]
This isn't a bug introduced by this patch, but frag_len and frame_len come
straight from the 16-bit hardware fields word0.bits.buffer_size and
word1.bits.byte_count. Later, frag_len gets unchecked unsigned
adjustments (frag_len -= NET_IP_ALIGN; frag_len = frame_len - skb->len)
and is then passed to skb_fill_page_desc(). geth_freeq_lookup() only
validates the offset.

Could a malformed descriptor attach a page fragment that runs past the
free queue fragment or past the end of the page?

This is fixed later in the series by "net: ethernet: cortina: Validate RX
fragment lengths". That patch rejects frag_len < NET_IP_ALIGN, frame_len
< ETH_ZLEN or frame_len < skb->len, zero-length descriptors, and frag_len
values larger than the space left in the fragment or the page.

>  		if (word3.bits32 & SOF_BIT) {
>  			if (skb) {
>  				napi_free_frags(&port->napi);
> @@ -1543,14 +1549,12 @@ static unsigned int gmac_rx(struct net_device *netdev, unsigned int budget,
>  			goto err_drop;
>  		}
>  
> -		/* Freeq pointers are one page off */
> -		gpage = gmac_get_queue_page(geth, port, mapping + PAGE_SIZE);
> -		if (!gpage) {
> +		page = geth_freeq_lookup(geth, mapping, &page_offs);
> +		if (!page) {
>  			dev_err_ratelimited(geth->dev,
>  					    "could not find mapping\n");
>  			goto err_drop;
>  		}
> -		page = gpage->page;

[Severity: Medium]
This is a pre-existing issue, but the free queue pages use long-lived
DMA_FROM_DEVICE streaming mappings. gmac_rx() hands fragments written by
the device to the stack without any dma_sync_single_*_for_cpu() call.
geth_fill_freeq() also re-posts pages to the device without
dma_sync_single_for_device().

Doesn't this break the DMA API ownership rules for these buffers?

The CPU sync is a no-op on the Gemini FA526, so there should be no
visible effect on the target hardware. Other implementations could see
stale data.

At the end of the series this is handled:

  - geth_freeq_claim() calls dma_sync_single_range_for_cpu() for each
    claimed fragment.
  - geth_freeq_post_page() calls dma_sync_single_for_device() when it
    recycles a page.
  - geth_freeq_release_pages() syncs fragments still owned by the
    hardware before unmapping with DMA_ATTR_SKIP_CPU_SYNC.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110-gemini-ethernet-fixes-3-v7-0-9c32a1a46aec%40kernel.org