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

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: ethernet: cortina: Normalize free queue pointers on rebuild

This commit masks the software free queue page indices to the active
queue order. When the free queue is rebuilt, it raises the order so the
new ring covers the read pointer the hardware kept. It also seeds the
software write pointer from that read pointer before filling the ring.

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 003108270d060..7ebd62f8a3a46 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;
>  	epn = (rw.bits.rptr >> fpp_order) - 1;
>  	epn &= m_pn;

[Severity: Low]
This fixes an out-of-bounds access. Should it have a Fixes: tag?

Before this change, geth_fill_freeq() used the unmasked pn taken from the
retained hardware rptr to compute &geth->freeq_pages[pn]. After a rebuild
at a smaller order, the first loop iteration could index past the newly
allocated pages array. page_ref_count() and page_ref_add() would then run
on a garbage struct page pointer:

gmac_set_ringparam() / gmac_open()
  geth_resize_freeq()
    geth_setup_freeq()
      geth_fill_freeq(geth, false)

The unmasked code and geth_resize_freeq() both come from:

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

The other fixes in this series have that tag. Later patches such as
"Correct free queue DMA mappings" and "Preserve in-flight free queue
pages" rely on this patch keeping rptr inside the ring.

Without the tag, could stable pick up those patches without this one?

[ ... ]

> @@ -958,6 +960,10 @@ static int geth_setup_freeq(struct gemini_ethernet *geth)
>  		if (!geth_freeq_alloc_map_page(geth, pn))
>  			goto err_freeq_alloc;
>  
> +	/* Match the software pointer to the retained hardware pointer. */
> +	rptr = GET_RPTR(geth->base + GLOBAL_SWFQ_RWPTR_REG);
> +	SET_WPTR(geth->base + GLOBAL_SWFQ_RWPTR_REG, rptr & (len - 1));
> +

[Severity: High]
This is a pre-existing issue and was not introduced by this patch. Do the
TX queues have the same retained rptr problem this patch fixes for the
free queue?

gmac_set_ringparam() lets txq_order shrink while the interface is down.
On the next open, gmac_setup_txqs() allocates skb_tab and desc_ring with
1 << txq_order entries. It then seeds wptr and cptr from the hardware
rptr without masking it:

drivers/net/ethernet/cortina/gemini.c:gmac_setup_txqs() {
    ...
		r = readw(rwptr_reg);
		rwptr_reg += 2;
		writew(r, rwptr_reg);
		rwptr_reg += 2;
		txq->cptr = r;
    ...
}

The TX rptr uses the same union dma_rwptr layout, where rptr is
read-only, and gmac_cleanup_txqs() only sets wptr = rptr. Take the
default order 8 shrunk with "ethtool -G ethX tx 16": any retained
rptr >= 16 is then outside the new ring.

gmac_start_xmit() reads the unmasked wptr, and the space check masks only
the difference:

	w = rw.bits.wptr;

	d = txq->cptr - w - 1;
	d &= m;

gmac_map_tx_bufs() then uses w before masking it:

			txq->skb[w] = skb;
    ...
		txd = txq->ring + w;

Can this write past skb_tab and past the coherent TX descriptor ring, and
also give the hardware a pointer outside its ring?

gmac_clean_txq() also starts from the unmasked txq->cptr and dereferences
txq->ring + c and txq->skb[c].

This is still present at the end of the series.

[ ... ]

> @@ -1099,6 +1106,10 @@ static int geth_resize_freeq(struct gemini_ethernet_port *port)
>  	}
>  
>  	new_order = min(15, ilog2(new_size - 1) + 1);
> +	rptr = GET_RPTR(geth->base + GLOBAL_SWFQ_RWPTR_REG);
> +	if (rptr >= BIT(15))
> +		return -EIO;
> +	new_order = max_t(unsigned int, new_order, fls(rptr));
>  	dev_dbg(geth->dev, "set shared queue to size %d order %d\n",
>  		new_size, new_order);
>  	if (geth->freeq_ring && geth->freeq_order == new_order)

[Severity: Medium]
Does this let leftover hardware state, not the configured RX ring sizes,
decide the size of the shared free queue?

gemini.h marks rptr as read-only:

	unsigned int rptr	: 16;	/* Read Ptr, RO */

so the writel(0, GLOBAL_SWFQ_RWPTR_REG) in geth_global_init() only
clears wptr in practice. After a ring of order N has been used, rptr can
be anywhere in [0, 2^N). Whenever rptr >= 2^(N-1), fls(rptr) == N.

In that case, lowering rx_pending through gmac_set_ringparam() hits:

	if (geth->freeq_ring && geth->freeq_order == new_order)
		return 0;

ethtool -G then reports success, but the free queue keeps its old
descriptor ring. It also keeps its (1 << order) >> fpp_order pages,
which were allocated with GFP_ATOMIC.

Both callers resize only while RX DMA is idle. gmac_set_ringparam()
requires !netif_running(), and gmac_open() resizes before
gmac_start_dma(). So rptr cannot move between attempts.

Suppose a rebuild at the forced order fails in dma_alloc_coherent() or in
the GFP_ATOMIC page allocations in geth_setup_freeq(). Won't every later
gmac_open() compute the same order and fail the same way? gmac_open()
treats that as fatal:

	err = geth_resize_freeq(port);
	if (err && (err != -EBUSY)) {
		netdev_err(netdev, "could not resize freeq\n");
		goto err_stop_phy;

Asking for a smaller ring no longer helps in that state.

A stale rptr left by a bootloader, or by a kexec'd kernel with larger
rings, would set the same floor at first open. That seems to work against
the later "Scale Gemini RX queues to system memory" patch, which shrinks
the free queue on 32/64 MiB systems.

The commit message only says the replacement ring is kept "large enough
for the retained hardware pointer". Could it mention that requested
shrinks may be skipped? Is there a fallback when allocation at the
forced order fails?

This behaviour is unchanged at the end of the series.