Re: [PATCH net-next v5 04/14] net: ethernet: cortina: Normalize free queue pointers on rebuild

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit fixes an out-of-bounds access but has no Fixes: tag.

Pre-existing issues:
- [Medium] The hardware SWFQ read pointer advances one fragment
  descriptor at a time.
- [Medium] In geth_freeq_alloc_map_page(), when a slot already holds a
  page, the loop `freeq_entry->word2.buf_adr = mapping` first overwrites…

--

net: ethernet: cortina: Normalize free queue pointers on rebuild

This patch masks the software free queue page index to the active queue
order in geth_fill_freeq(). It sizes the rebuilt free queue so that the
retained hardware read pointer fits inside it. Before the ring is filled,
it sets the write pointer from that read pointer.

[Severity: Low]
Should this patch carry a Fixes: tag?

Without this mask, rebuilding the queue at a smaller order through ethtool
-G could use the retained hardware rptr to index geth->freeq_pages[pn] past
the end of the kzalloc'd array:

gmac_set_ringparam()
  geth_resize_freeq()
    geth_setup_freeq()
      geth_fill_freeq(geth, false)
        pn = rw.bits.rptr >> fpp_order;
        gpage = &geth->freeq_pages[pn];

The unmasked computation appears to come from 4d5ae32f5e1e ("net:
ethernet: Add a driver for Gemini gigabit ethernet").

The other fixes in this series ("Keep shared free queue parent-owned",
"Drain free queue IRQ before resize" and "Preserve in-flight free queue
pages") all carry Fixes: 4d5ae32f5e1e. Without the tag here, stable trees
may not pick up this out-of-bounds fix.

[ ... ]

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch. Does
geth_freeq_alloc_map_page() unmap the right address when it replaces a
page?

The function writes the new page's mapping into the ring and then reads
the "old" mapping back from the same slot:

    for (i = (1 << fpp_order); i > 0; i--) {
        freeq_entry->word2.buf_adr = mapping;
        freeq_entry++;
        mapping += frag_len;
    }

    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);
        ...
        put_page(gpage->page);
    }

The refill path in geth_fill_freeq() calls this to replace an in-use page
when page_ref_count(page) > 1 and fl <= 64 >> fpp_order. In that case it
looks like the new buffer, which the hardware is about to DMA into, gets
unmapped, and the old page's mapping is never unmapped.

The unmap also uses frag_len, not the PAGE_SIZE passed to
dma_map_single(). The error path in geth_setup_freeq() and
geth_cleanup_freeq() also unmap with frag_len.

When no page was present, gpage->mapping = mapping stores the address
after the fragment loop has moved past it, not the page base.

The next commit in this series, "Correct free queue DMA mappings", appears
to fix this. It removes the invalid replacement unmap, keeps the page DMA
base separate from the fragment iterator, and unmaps with PAGE_SIZE.

[Severity: Medium]
This isn't a bug introduced by this patch, but what happens when the
retained rptr is odd?

The hardware read pointer moves one fragment descriptor at a time. With
the default fpp_order of 1, each page has two descriptors. However,
geth_fill_freeq() works in whole pages:

    pn = (refill ? rw.bits.wptr : rw.bits.rptr) >> fpp_order;
    pn &= m_pn;
    epn = (rw.bits.rptr >> fpp_order) - 1;
    ...
        page_ref_add(page, 1 << fpp_order);
    ...
    writew(pn << fpp_order, geth->base + GLOBAL_SWFQ_RWPTR_REG + 2);

Take len = 1024 and rptr = 701. pn starts at 350 and the final wptr is
698. Both fragments of page 350 get a reference, but entry 700 sits
behind rptr and outside [rptr, wptr). The hardware is never offered it on
this lap.

On the next refill, page 350 still has page_ref_count() > 1, so the loop
breaks until the queue drains to 64 descriptors or fewer. That can let
the free queue fall to the swfq_empty threshold.

geth_freeq_alloc_map_page() then replaces the page but drops only one
reference. Does that leak the old page and its DMA mapping?

At the end of the series, the orphaned fragment seems to hold its
metadata slot, page and mapping until the next queue teardown.

The old setup fill also used rptr >> fpp_order, so this truncation
predates this patch.

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