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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 (3) · Medium: 1 (1) · Low: 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] This patch fixes a real RX-path bug that dates back to the
  original driver, but the commit message has no Fixes: tag.
- [Low] This is a netdev hardware-driver bug fix, but the commit message
  does not say how the issue was found, how it was validated, or how the…

Pre-existing issues:
- [High] geth_cleanup_freeq() still runs 'while
  (page_ref_count(gpage->page) > 0) put_page(gpage->page);'.
- [High] gmac_rx() never checks the descriptor-supplied fragment length
  against the posted buffer.
- [High] Free-queue pages keep one DMA_FROM_DEVICE mapping for the
  queue's whole lifetime.
- [Medium] When geth_fill_freeq() replaces a slot whose
  page_ref_count(page) > 1, geth_freeq_alloc_map_page() calls put_page()…

--

net: ethernet: cortina: Correct free queue DMA mappings

This commit keeps the free queue page DMA base separate from the
per-fragment iterator. It removes the replacement-path unmap, which was
unmapping the new buffer, and unmaps the full PAGE_SIZE mapping on setup
unwind and teardown. It also replaces gmac_get_queue_page() with
geth_freeq_lookup(), which works out fragment offsets from the saved DMA
base under freeq_lock, and rejects mappings that do not fit in 32-bit
descriptors.

> Assisted-by: LLM
> Signed-off-by: Linus Walleij <linusw@kernel.org>

[Severity: Medium]
This looks like a fix for behaviour that has been there since the driver
was added, but there is no Fixes: tag. maintainer-netdev.rst says "for
fixes the Fixes: tag is required, regardless of the tree".

Should this carry something like:

Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet")

Other patches in the series do have Fixes: tags. That includes "net:
ethernet: cortina: Preserve in-flight free queue pages", which builds on
this one. Without a tag here, could stable tooling pick up the dependent
patches but not this one?

Could the commit message also describe the symptom users saw before the
fix?

Before this patch, gpage->mapping held base + PAGE_SIZE for pages mapped
at setup. For pages replaced from geth_fill_freeq() it held the new
page's base. The lookup always used:

    gpage = gmac_get_queue_page(geth, port, mapping + PAGE_SIZE);

This only matches the setup case. For a replaced page at base X the key
becomes X + PAGE_SIZE, so one of two things happens in gmac_rx():

- it logs "could not find mapping" and drops the frame, or
- it matches a different page whose stored value happens to be
  X + PAGE_SIZE, attaches that page to the skb and drops that page's
  reference.

[Severity: Low]
The "Bug fixes" section of Documentation/process/maintainer-netdev.rst
says "All bug fixes must state how the issue was discovered and
validated, and how the fix was tested." The only trailers here are
Assisted-by: and Signed-off-by:, so they don't supply that information.

Later patches in the series include a statement such as "The series was
tested on a D-Link DIR-685...". Could a similar note on discovery and
testing be added here? This patch changes DMA map/unmap sizes,
descriptor programming and the RX lookup path.

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 7ebd62f8a3a46..006362a033952 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c

[ ... ]

> @@ -820,9 +832,17 @@ static struct page *geth_freeq_alloc_map_page(struct gemini_ethernet *geth,
>  	if (!page)
>  		return NULL;
>  
> -	mapping = dma_map_single(geth->dev, page_address(page),
> -				 PAGE_SIZE, DMA_FROM_DEVICE);
> -	if (dma_mapping_error(geth->dev, mapping)) {
> +	page_mapping = dma_map_single(geth->dev, page_address(page),
> +				      PAGE_SIZE, DMA_FROM_DEVICE);

[Severity: High]
This is a pre-existing issue, but free queue pages keep this one
DMA_FROM_DEVICE mapping for the whole life of the queue. When the stack
releases a page, geth_fill_freeq() gives it back to the hardware with
only:

    /* Add one reference per fragment in the page */
    page_ref_add(page, 1 << fpp_order);

plus the SWFQ write pointer update. There is no
dma_sync_single_for_device() call.

gmac_rx() also passes fragments to the stack without calling
dma_sync_single_range_for_cpu().

The FA526 is not cache-coherent. Can cache lines filled while the stack
read an earlier frame survive the reuse? If so, the CPU would see stale
data for the next frame DMA'd into the same page.

This seems to be handled later in the series:

- "net: ethernet: cortina: Preserve in-flight free queue pages" adds
  dma_sync_single_range_for_cpu() in geth_freeq_claim().
- "net: ethernet: cortina: Recycle claimed free queue pages" adds
  dma_sync_single_for_device() in geth_freeq_post_page().

> +	if (dma_mapping_error(geth->dev, page_mapping)) {

[ ... ]

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

[Severity: Medium]
This isn't a bug introduced by this patch. When geth_fill_freeq()
replaces a slot because page_ref_count(page) > 1, the old page's
PAGE_SIZE DMA_FROM_DEVICE mapping is overwritten here and not recorded
anywhere else.

Is that mapping ever unmapped? geth_cleanup_freeq() only unmaps the
current gpage->mapping.

RX descriptors that still point into the old page also can't be found by
geth_freeq_lookup(), because it only searches the current freeq_pages[]
entries. In gmac_rx() that ends up in:

    page = geth_freeq_lookup(geth, mapping, &page_offs);
    if (!page) {
        ...
        goto err_drop;
    }

err_drop runs with page == NULL here. Does that drop the frame and leak
the fragment's reference on the old page?

The commit message defers this ("The replaced mapping cannot be released
until its in-flight fragments have been claimed, which is handled with
their lifetime later in the series"). Two later commits appear to fix
it:

- "net: ethernet: cortina: Preserve in-flight free queue pages" adds
  geth_freeq_claim() with per-fragment xarray tracking.
- "net: ethernet: cortina: Recycle claimed free queue pages" adds
  dma_unmap_single_attrs() in geth_freeq_recycle_page().

[ ... ]

> @@ -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: High]
This is a pre-existing issue, but can this loop drop references that
belong to skbs already passed to the stack?

gmac_rx() attaches fragments with skb_fill_page_desc() and hands them
off with napi_gro_frags(). Those skbs may still be in socket queues when
the free queue is torn down.

Draining page_ref_count() to zero would free the page under them. After
the page is freed and possibly reallocated, the next page_ref_count()
read could also lead to dropping a reference that belongs to the new
owner.

Later in the series, "net: ethernet: cortina: Preserve in-flight free
queue pages" gives each posted fragment its own reference. With that
change, geth_freeq_release_pages() only puts gpage->fragments + 1
references.

>  	}

[ ... ]

> @@ -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;
>  
>  		if (word3.bits32 & SOF_BIT) {
>  			skb = gmac_skb_if_good_frame(port, word0, frame_len);

[Severity: High]
This is a pre-existing issue. geth_freeq_lookup() only checks the
fragment's starting offset, and gmac_rx() never checks the length from
the descriptor against the posted buffer:

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

If the hardware reports an inconsistent buffer_size or byte_count, can
frag_len underflow or exceed the 2 KiB fragment? The skb fragment would
then describe memory past the buffer or past the end of the page.

The later commit "net: ethernet: cortina: Validate RX fragment lengths"
adds this validation:

- it rejects frag_len < NET_IP_ALIGN and frame_len < skb->len
- it limits frag_len to what fits in the fragment and the page

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