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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 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] The new `writel(0, geth->base + GLOBAL_SWFQ_RWPTR_REG)` in
  geth_setup_freeq() comes with the comment "Discard pointer state…
- [Low] The patch fixes a real out-of-bounds access but has no Fixes:
  tag.

--

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

This commit masks the free queue page index in geth_fill_freeq() to the
active queue order. Before the initial fill, it also writes 0 to
GLOBAL_SWFQ_RWPTR_REG in geth_setup_freeq(). Together these are meant to
stop out-of-bounds access to the replacement page table when the free
queue is rebuilt at a smaller order.

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index c03541deed495..c3d1abdbebf61 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -887,6 +887,7 @@ static unsigned int geth_fill_freeq(struct gemini_ethernet *geth, bool refill)
>  
>  	rw.bits32 = readl(geth->base + GLOBAL_SWFQ_RWPTR_REG);
>  	pn = (refill ? rw.bits.wptr : rw.bits.rptr) >> fpp_order;
> +	pn &= m_pn;

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

The unmasked pn computation in geth_fill_freeq() comes from commit
4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet").
If rptr is left over from a larger ring, the old code indexed
geth->freeq_pages[pn] past the end of the newly allocated table. It then
called page_ref_count()/page_ref_add() on whatever pointer was there.

A user with CAP_NET_ADMIN can reach this. Running ethtool -G on both ports
while they are down shrinks the free queue through:

gmac_set_ringparam()->geth_resize_freeq()->geth_cleanup_freeq()->
    geth_setup_freeq()->geth_fill_freeq(false)

The other free queue lifecycle fixes in this series all carry
Fixes: 4d5ae32f5e1e:

  "Keep shared free queue parent-owned"
  "Drain free queue IRQ before resize"
  "Correct free queue DMA mappings"
  "Preserve in-flight free queue pages"

The mask does not depend on any other patch in the series, so it should
backport on its own.

>  	epn = (rw.bits.rptr >> fpp_order) - 1;
>  	epn &= m_pn;
>  
> @@ -958,6 +959,9 @@ static int geth_setup_freeq(struct gemini_ethernet *geth)
>  		if (!geth_freeq_alloc_map_page(geth, pn))
>  			goto err_freeq_alloc;
>  
> +	/* Discard pointer state belonging to the previous queue geometry. */
> +	writel(0, geth->base + GLOBAL_SWFQ_RWPTR_REG);

[Severity: Medium]
Does this writel() actually reset the read pointer? gemini.h defines the
register like this:

union dma_rwptr {
	unsigned int bits32;
	struct bit_000c {
		unsigned int rptr	: 16;	/* Read Ptr, RO */
		unsigned int wptr	: 16;	/* Write Ptr, RW */
	} bits;
};

geth_cleanup_freeq() follows that layout and writes only the wptr half:

	writew(readw(geth->base + GLOBAL_SWFQ_RWPTR_REG),
	       geth->base + GLOBAL_SWFQ_RWPTR_REG + 2);

If rptr is read-only, the 32-bit write of 0 clears only wptr. The hardware
rptr then keeps its position from the old, larger ring.

In that case the new comment, and this part of the commit message, would
not match what the hardware does:

  Reset the pointer register before initially filling a rebuilt queue

geth_fill_freeq(false) masks the stale rptr only for the CPU-side page
index. It then publishes a masked wptr:

	writew(pn << fpp_order, geth->base + GLOBAL_SWFQ_RWPTR_REG + 2);

For example, take a rebuild from order 10 with rptr at 768 to order 8
(256 entries). Software posts entries 0..253 and sets wptr to 254, but the
hardware rptr is still 768.

Suppose the free queue engine computes descriptor addresses from the raw
rptr (freeq_dma_base + rptr * 16). Could it then read descriptors past the
end of the new dma_alloc_coherent() ring and DMA received frames to
whatever buf_adr values it finds there?

Nothing in the driver shows the hardware reducing rptr modulo the new
queue size. The commit message also hedges with "if hardware retains its
read pointer while the queue is disabled".

Later in the series, geth_setup_freeq() still has the same writel(0) and
comment. geth_fill_freeq() then starts at wptr and stops at
((rptr >> fpp_order) - 1) & m_pn. If rptr is retained, a rebuild after
traffic would fill fewer entries than expected. Wouldn't
geth_setup_freeq() then fail with -ENOMEM?

> +
>  	filled = geth_fill_freeq(geth, false);
>  	if (!filled)
>  		goto err_freeq_alloc;

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