Re: [PATCH net-next v4 05/15] 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 (2) · 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).

- [Medium] In geth_cleanup_freeq()
  (drivers/net/ethernet/cortina/gemini.c:1038-1039), teardown still…
- [Medium] When geth_fill_freeq() replaces a slot whose page still has
  outstanding references (page_ref_count(page) > 1, lines 919-927),…

Pre-existing issues:
- [High] geth_fill_freeq() (lines 919-927) replaces freeq_pages[pn]
  whenever page_ref_count(page) > 1.
- [High] geth_cleanup_freeq() (lines 1041-1042) runs 'while
  (page_ref_count(gpage->page) > 0) put_page(gpage->page);'.
- [Medium] geth_freeq_lookup() checks the fragment start offset, but
  gmac_rx() never bounds the length it passes to skb_fill_page_desc()…

--

net: ethernet: cortina: Correct free queue DMA mappings

This patch keeps the free queue page DMA base separate from the fragment
iterator and removes the replacement unmap, which was unmapping the new
buffer. It also uses PAGE_SIZE in the setup unwind path and rejects
mappings above 32 bits. It replaces gmac_get_queue_page() with
geth_freeq_lookup(), which derives fragment offsets from the saved DMA
base under freeq_lock.

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index c3d1abdbebf61..3b915a4c89009 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c

[ ... ]

> @@ -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;
> +
>  	freeq_entry = geth->freeq_ring + (pn << fpp_order);

[ ... ]

> -
> -	/* If the freeq entry already has a page mapped, then unmap it. */
> -	gpage = &geth->freeq_pages[pn];
> -	if (gpage->page) {
> -		mapping = geth->freeq_ring[pn << fpp_order].word2.buf_adr;
> -		dma_unmap_single(geth->dev, mapping, frag_len, DMA_FROM_DEVICE);
> -		/* This should be the last reference to the page so it gets
> -		 * released
> -		 */
> -		put_page(gpage->page);
> -	}

[Severity: Medium]
With the replacement unmap removed, what releases the old page's DMA
mapping here?

When geth_fill_freeq() sees page_ref_count(page) > 1 and calls
geth_freeq_alloc_map_page(), the old page is put and gpage->mapping is
overwritten with the new page_mapping. The old PAGE_SIZE DMA_FROM_DEVICE
mapping is never unmapped. Its handle is not recorded anywhere else, so
geth_cleanup_freeq() can't release it either.

The old code unmapped the wrong buffer, but every mapping was at least
partly released. After this change, each refill replacement under RX
load leaves one more streaming mapping live.

The commit message says "Releasing mappings with in-flight fragments is
handled together with their lifetime later in the series". The later
commits "net: ethernet: cortina: Preserve in-flight free queue pages" and
"net: ethernet: cortina: Recycle claimed free queue pages" do unmap
gpage->mapping with PAGE_SIZE.

This patch has a Fixes: tag. Would a standalone stable backport pick up
this leaking intermediate state?

[ ... ]

> @@ -1025,7 +1037,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);
> -
>  		gpage = &geth->freeq_pages[pn];

[Severity: Medium]
Should this unmap also use gpage->mapping and PAGE_SIZE?

The page is mapped in geth_freeq_alloc_map_page() with:

    page_mapping = dma_map_single(geth->dev, page_address(page),
                                  PAGE_SIZE, DMA_FROM_DEVICE);

This patch changes the geth_setup_freeq() error unwind to:

    dma_unmap_single(geth->dev, gpage->mapping, PAGE_SIZE,
                     DMA_FROM_DEVICE);

However, geth_cleanup_freeq() still unmaps only frag_len bytes, at the
address read back from the descriptor. This path runs from
geth_resize_freeq() on open and on ethtool set_ringparam, and from the
remove path. CONFIG_DMA_API_DEBUG would flag the size mismatch.

The later commit "net: ethernet: cortina: Preserve in-flight free queue
pages" replaces this loop with geth_freeq_release_pages(), which unmaps
gpage->mapping with PAGE_SIZE. The end state of the series looks
correct.

>  		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 free pages
that skbs still reference?

gmac_rx() passes fragments of these pages to the stack through
skb_fill_page_desc() and napi_gro_frags(). Suppose one of those skbs is
still queued (on an unread socket, for example) when this runs:

    geth_resize_freeq()
      geth_cleanup_freeq()
        while (page_ref_count(gpage->page) > 0)
            put_page(gpage->page);

The page goes back to the allocator while the skb still points to it.
The later kfree_skb() would then put a page that is already free.

This can be reached by bringing the ports down and up with a different
ring order, by ethtool set_ringparam, or by removing the device.

The later commit "net: ethernet: cortina: Preserve in-flight free queue
pages" replaces this loop with geth_freeq_release_pages(). That function
drops only the gpage->fragments references plus the base reference.

[ ... ]

> @@ -1516,8 +1526,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;
> -
>  		if (word3.bits32 & SOF_BIT) {

[Severity: Medium]
This isn't a bug introduced by this patch, but is frag_len checked
anywhere before it reaches skb_fill_page_desc()?

geth_freeq_lookup() now checks the fragment start offset. The length,
though, still comes straight from the descriptor in gmac_rx():

    frag_len = word0.bits.buffer_size;
    ...
        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);

The NET_IP_ALIGN subtraction has no lower bound, and frame_len - skb->len
can wrap. If the MAC reports inconsistent lengths, could the page frag
cover memory past the 2 KiB fragment or past the end of the page?

The later commit "net: ethernet: cortina: Validate RX fragment lengths"
appears to add these checks.

[ ... ]

> @@ -1536,14 +1544,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;
>  		}

[Severity: High]
This is a pre-existing issue, but what happens here if a refill has
already replaced the fragment's page?

geth_fill_freeq() decides to replace freeq_pages[pn] from the refcount
alone:

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

That count also includes fragments the hardware has already written into
RX descriptors that gmac_rx() hasn't processed yet. Once gpage->page and
gpage->mapping are overwritten, geth_freeq_lookup() finds no match for
the old buf_adr and returns NULL. The frame is then dropped with "could
not find mapping", and err_drop skips the put:

    if (page)
        put_page(page);

Doesn't that leak the reference page_ref_add() took for this fragment,
and so the old page itself? gmac_cleanup_rxq() hits the same lookup
failure and also skips put_page().

The new locking in geth_freeq_lookup() only covers the window between
lookup and use. The later commit "net: ethernet: cortina: Preserve
in-flight free queue pages" tracks each fragment address in an XArray
until geth_freeq_claim(). That looks like it resolves this.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261011-gemini-ethernet-fixes-3-v4-0-4b1631b08f5d%40kernel.org