Re: [PATCH net-next v3 04/12] net: ethernet: cortina: Correct free queue DMA mappings

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 (2) · Medium: 3 (2) · 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] In geth_freeq_alloc_map_page()
  (drivers/net/ethernet/cortina/gemini.c:856-862), replacing a page…
- [Medium] geth_freeq_alloc_map_page() maps PAGE_SIZE (line 833).
- [Medium] geth_freeq_lookup() takes freeq_lock with spin_lock_irqsave()
  (line 738) and then linearly scans all num_freeq_pages entries (line…

Pre-existing issues:
- [High] geth_cleanup_freeq() (lines 1034-1036) runs 'while
  (page_ref_count(gpage->page) > 0) put_page(gpage->page);'.
- [High] gmac_rx() takes frag_len from the 16-bit hardware buffer_size
  (line 1523).
- [Medium] This problem wasn't introduced by this patch, but
  gmac_napi_poll() (line 1629-1635) wraps the whole gmac_rx() call in…
- [Medium] Freeq pages are mapped DMA_FROM_DEVICE once.

--

net: ethernet: cortina: Correct free queue DMA mappings

This commit keeps the free queue page DMA base separate from the fragment
iterator, drops the incorrect unmap on page replacement, and unwinds setup
mappings with PAGE_SIZE. It also replaces gmac_get_queue_page() with
geth_freeq_lookup(), which works out the fragment offset from the saved DMA
base under freeq_lock and rejects mappings that don't fit in 32 bits.

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index a2daf22e76989..809274aff8e56 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -724,32 +724,43 @@ static int gmac_setup_rxq(struct net_device *netdev)

[ ... ]

> +static struct page *geth_freeq_lookup(struct gemini_ethernet *geth,
> +				      dma_addr_t mapping,
> +				      unsigned int *page_offs)
>  {
> +	unsigned int frag_len = 1 << geth->freeq_frag_order;
>  	struct gmac_queue_page *gpage;
> -	dma_addr_t mapping;
> +	unsigned long flags;
> +	dma_addr_t page_mapping;
> +	struct page *page = NULL;
>  	int i;
>  
> -	/* Only look for even pages */
> -	mapping = addr & PAGE_MASK;
> -
> +	spin_lock_irqsave(&geth->freeq_lock, flags);

[Severity: Medium]
This isn't a bug introduced by this patch, but on 32-bit PREEMPT_RT
kernels gmac_napi_poll() wraps the whole gmac_rx() call in the
rx_stats_syncp writer section:

gmac_napi_poll()
    u64_stats_update_begin(&port->rx_stats_syncp);
    received = gmac_rx(napi->dev, budget, &freeq_consumed, &reschedule);
    ...
    u64_stats_update_end(&port->rx_stats_syncp);

On 32-bit, u64_stats_update_begin() disables preemption, and on
PREEMPT_RT a spinlock_t can sleep.

Can the freeq_lock taken here for each descriptor trigger a "sleeping
function called from invalid context" splat with
CONFIG_DEBUG_ATOMIC_SLEEP?

The baseline gmac_rx() already took irq_lock and called napi_gro_frags()
inside the same section. The pattern is still there at the end of the
series, where geth_freeq_claim() runs inside it.

>  	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;
>  	}

[Severity: Medium]
gmac_rx() calls geth_freeq_lookup() once per RX descriptor in NAPI
context. Doesn't this put a linear scan over all num_freeq_pages entries,
with interrupts disabled, on the RX hot path?

The next patch in the series, "Index free queue fragments with XArray",
replaces this scan with an xa_load() in geth_freeq_claim().

[ ... ]

> @@ -833,31 +853,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);
> +
> +	/* Then put our new mapping into the page table */
> +	gpage->mapping = page_mapping;
> +	gpage->page = page;

[Severity: Medium]
With the replacement unmap gone, what releases the old page's PAGE_SIZE
streaming mapping here?

geth_fill_freeq() takes this path whenever page_ref_count(page) > 1:

		if (page_ref_count(page) > 1) {
			...
			page = geth_freeq_alloc_map_page(geth, pn);

The old gpage->mapping is overwritten right after the put_page(), and
geth_cleanup_freeq() only unmaps the address currently posted in the ring.

At this commit, does every replacement leak one DMA mapping when an IOMMU,
swiotlb or dma-debug is in use?

The commit message says these mappings are released later in the series.
After "Preserve in-flight free queue pages" and "Recycle claimed free queue
pages", geth_freeq_release_pages() unmaps every live slot with PAGE_SIZE.
The old code also left the old mapping live.

>  
>  	freeq_entry = geth->freeq_ring + (pn << fpp_order);

[ ... ]

> @@ -1019,7 +1031,6 @@ static void geth_cleanup_freeq(struct gemini_ethernet *geth)
>  
>  		mapping = geth->freeq_ring[pn << fpp_order].word2.buf_adr;
>  		dma_unmap_single(geth->dev, mapping, frag_len, DMA_FROM_DEVICE);

[Severity: Medium]
geth_freeq_alloc_map_page() maps PAGE_SIZE, and this patch changes the
geth_setup_freeq() error path to unmap with gpage->mapping and PAGE_SIZE.

Should geth_cleanup_freeq() do the same, rather than using frag_len and
the address read back from the descriptor? As it stands, the two release
sites for the same mapping disagree, and dma-debug would warn about a
size mismatch on ifdown or remove.

At the end of the series, geth_cleanup_freeq() goes through
geth_freeq_release_pages(), which calls
dma_unmap_single_attrs(geth->dev, gpage->mapping, PAGE_SIZE,
DMA_FROM_DEVICE, DMA_ATTR_SKIP_CPU_SYNC).

> -
>  		gpage = &geth->freeq_pages[pn];
>  		while (page_ref_count(gpage->page) > 0)
>  			put_page(gpage->page);

[Severity: High]
This isn't a bug introduced by this patch, but can this loop drop
references held by skbs that gmac_rx() already passed to the stack through
napi_gro_frags()?

It keeps calling put_page() until the refcount reaches zero. That can free
pages the stack is still using, and page_ref_count() is read again after
the final put_page().

Later in the series, geth_freeq_release_pages() drops only the
gpage->fragments references plus one base reference, and leaves
skb-owned references alone.

[ ... ]

> @@ -1532,14 +1540,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: High]
This isn't a bug introduced by this patch, but the length passed to
skb_fill_page_desc() further down in gmac_rx() comes straight from the
hardware descriptor:

		if (word3.bits32 & SOF_BIT) {
			...
			page_offs += NET_IP_ALIGN;
			frag_len -= NET_IP_ALIGN;
			...
		}

		if (word3.bits32 & EOF_BIT)
			frag_len = frame_len - skb->len;
		...
		skb_fill_page_desc(skb, frag_nr, page, page_offs, frag_len);

Can frag_len underflow here? That would happen if buffer_size is smaller
than NET_IP_ALIGN, or if frame_len is smaller than skb->len.

Nothing checks the result against the 2 KiB fragment size or against
PAGE_SIZE - page_offs. Could a malformed descriptor expose adjacent memory
to the stack?

The "Validate RX fragment lengths" patch later in the series adds these
checks.

[Severity: Medium]
This is a pre-existing issue, but the freeq pages are mapped
DMA_FROM_DEVICE once, and gemini.c has no dma_sync_* calls at this commit.

geth_fill_freeq() reposts reused pages with only page_ref_add(). gmac_rx()
then hands fragments written by the hardware to skb_fill_page_desc()
without syncing them for the CPU.

Could this give the stack stale cache contents on non-coherent DMA setups?

Later in the series, geth_freeq_claim() calls
dma_sync_single_range_for_cpu() for each claimed fragment.
geth_freeq_post_page() syncs for the device when recycling, and
geth_freeq_release_pages() syncs fragments still owned by the hardware
before unmapping. A later commit also notes that the CPU sync is a no-op
on Gemini's FA526.

>  
>  		if (word3.bits32 & SOF_BIT) {
>  			skb = gmac_skb_if_good_frame(port, word0, frame_len);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002-gemini-ethernet-fixes-3-v3-0-3e1f25890ae8%40kernel.org