Re: [PATCH net-next v2 02/11] net: ethernet: cortina: Drain free queue IRQ before resize

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 (1) · Medium: 2 (1) · Low: 1 (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 sequence in geth_resize_freeq() (lines 1096-1098) is
  mask, synchronize_irq(port1->irq), mask.
- [Medium] At this commit, geth_resize_freeq() calls
  geth_set_freeq_irq(geth, true) (line 1112) without checking the return…
- [Low] The commit fixes a use-after-free race, but it has no Fixes:
  trailer.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  geth_resize_freeq() assumes the peer port object is valid.
- [Medium] This problem wasn't introduced by this patch, but at this
  commit geth_resize_freeq() sets `geth->freeq_order = new_order` (line…
- [Low] This problem wasn't introduced by this patch, but the kernel-doc
  of geth_resize_freeq() is wrong.

--

net: ethernet: cortina: Drain free queue IRQ before resize

This commit changes how geth_resize_freeq() disables the SWFQ empty
interrupt. It used to only mask it. It now masks it, calls synchronize_irq()
on port 1's IRQ, and masks it again, so that a threaded handler which is
already running gets drained. The read-modify-write of
GLOBAL_INTERRUPT_ENABLE_4_REG moves into a new helper, geth_set_freeq_irq().

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

[Severity: Low]
The commit message describes a fix for a race, but there is no Fixes: tag.
In the race, gemini_port_irq_thread() refills the queue while
geth_resize_freeq() tears down freeq_ring and freeq_pages.

The mask-only sequence being replaced in geth_resize_freeq() appears to
date back to the original driver submission. Should this carry a tag so
stable and backport tooling can pick it up?

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

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 31bcd41c17fad..e5531e41ae9a3 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -1030,6 +1030,21 @@ static void geth_cleanup_freeq(struct gemini_ethernet *geth)

[ ... ]

>  /**
>   * geth_resize_freeq() - resize the software queue depth
>   * @port: the port requesting the change

[Severity: Low]
This isn't a bug introduced by this patch, but the kernel-doc for
geth_resize_freeq() says:

 * This gets called at least once during probe() so the device queue gets
 * "resized" from the hardware defaults.

The only callers appear to be gmac_open() and gmac_set_ringparam(). Neither
gemini_ethernet_port_probe() nor gemini_ethernet_init() calls it.

This patch reworks the IRQ handling in this function. Could the comment be
updated at the same time? It is also unchanged at the end of the series.

> @@ -1047,8 +1062,6 @@ static int geth_resize_freeq(struct gemini_ethernet_port *port)
>  	struct net_device *other_netdev;
>  	unsigned int new_size = 0;
>  	unsigned int new_order;
> -	unsigned long flags;
> -	u32 en;
>  	int ret;
>
>  	if (netdev->dev_id == 0)

[Severity: High]
This is a pre-existing issue, but geth_resize_freeq() dereferences the peer
port here without checking it. The patch adds another unchecked dereference
further down with synchronize_irq(geth->port1->irq).

Can geth->port1 or geth->port0 be NULL, or point to freed memory, at this
point?

If the peer ethernet-port node is missing, the pointer stays NULL. The
binding doesn't require either child node.

Several in-tree boards declare port@1 with no phy-mode or phy-handle:
gemini-dlink-dns-313, nas4210b, nas4220b, rut1xx, wbd111 and
dlink-dir-685. The ns2502 and verbatim boards inherit port@1 from
gemini.dtsi in the same state. gemini_ethernet_port_probe() publishes the
pointer before the steps that can fail:

	if (!id)
		geth->port0 = port;
	else
		geth->port1 = port;
	...
	ret = gmac_setup_phy(netdev);
	if (ret) {
		netdev_err(netdev,
			   "PHY init failed\n");
		goto unprepare;
	}

On those boards gmac_setup_phy() returns -ENODEV. Devres then frees the
netdev from devm_alloc_etherdev_mqs() that contains port, and geth->port1 is
never cleared.

Wouldn't a later ip link set eth0 up or ethtool -G eth0 then go through
gmac_open()->geth_resize_freeq() and read freed memory? That would happen
via other_netdev, netif_running(other_netdev), other_port->rxq_order, and
now geth->port1->irq.

gmac_open() also treats a spurious -EBUSY from the netif_running() check as
success. The interface would then come up without a configured free queue.

This still appears to be present at the end of the series.

[ ... ]

> @@ -1079,13 +1092,10 @@ static int geth_resize_freeq(struct gemini_ethernet_port *port)
>  	if (geth->freeq_order == new_order)
>  		return 0;

[Severity: Medium]
This isn't a bug introduced by this patch, but geth_resize_freeq() stores
the new order before geth_setup_freeq() succeeds. It doesn't restore the
old order on failure:

	geth->freeq_order = new_order;
	ret = geth_setup_freeq(geth);

geth_setup_freeq() can fail, for example in dma_alloc_coherent() or in
alloc_page(GFP_ATOMIC) inside geth_freeq_alloc_map_page(). After such a
failure freeq_ring is NULL, and GLOBAL_SW_FREEQ_BASE_SIZE_REG is still 0
from geth_cleanup_freeq().

The next gmac_open() or ethtool -G would compute the same order. Could it
then take this early return with 0, so that gmac_open() goes on to
gmac_setup_rxq() and gmac_start_dma() without a valid shared free queue?

Later in this series, "net: ethernet: cortina: Preserve in-flight free
queue pages" changes the check to the following and NULLs freeq_ring on the
setup error paths, which resolves this:

	if (geth->freeq_ring && geth->freeq_order == new_order)

>
> -	spin_lock_irqsave(&geth->irq_lock, flags);
> -
> -	/* Disable the software queue IRQs */
> -	en = readl(geth->base + GLOBAL_INTERRUPT_ENABLE_4_REG);
> -	en &= ~SWFQ_EMPTY_INT_BIT;
> -	writel(en, geth->base + GLOBAL_INTERRUPT_ENABLE_4_REG);
> -	spin_unlock_irqrestore(&geth->irq_lock, flags);
> +	/* The threaded handler can already be running and re-enable the IRQ. */
> +	geth_set_freeq_irq(geth, false);
> +	synchronize_irq(geth->port1->irq);
> +	geth_set_freeq_irq(geth, false);

[Severity: Medium]
Does this sequence fully close the window described in the commit message?

synchronize_irq() only waits for threads that were already woken. After its
refill, the drained gemini_port_irq_thread() re-enables the interrupt
unconditionally:

	/* ACK queue interrupt */
	writel(irqmask, geth->base + GLOBAL_INTERRUPT_STATUS_4_REG);
	/* Enable queue interrupt again */
	irqmask |= readl(geth->base + GLOBAL_INTERRUPT_ENABLE_4_REG);
	writel(irqmask, geth->base + GLOBAL_INTERRUPT_ENABLE_4_REG);

Suppose SWFQ_EMPTY is asserted, and the hardirq is handled after that
thread finishes but before the second geth_set_freeq_irq(geth, false). Then
gemini_port_irq() sees:

	if (val & en & SWFQ_EMPTY_INT_BIT) {

It masks the bit and returns IRQ_WAKE_THREAD. The second mask then clears a
bit that is already clear, and nothing waits for the newly woken thread.

Couldn't that thread run geth_fill_freeq() on freeq_pages and freeq_ring
while geth_cleanup_freeq() frees them? It could also race with
geth_setup_freeq() rebuilding them with the new freeq_order, and then
re-enable the interrupt after teardown.

The sequence and the unconditional re-enable in the thread appear to be
unchanged at the end of the series.

Would something other than a second mask be more robust? One option is
disable_irq(geth->port1->irq) and enable_irq() around the resize. Another is
state protected by irq_lock that stops the thread from re-enabling the
interrupt during a resize.

>
>  	/* Drop the old queue */
>  	if (geth->freeq_ring)
> @@ -1099,10 +1109,7 @@ static int geth_resize_freeq(struct gemini_ethernet_port *port)
>  	 * after probe(), this is where the interrupts get turned on
>  	 * in the first place.
>  	 */
> -	spin_lock_irqsave(&geth->irq_lock, flags);
> -	en |= SWFQ_EMPTY_INT_BIT;
> -	writel(en, geth->base + GLOBAL_INTERRUPT_ENABLE_4_REG);
> -	spin_unlock_irqrestore(&geth->irq_lock, flags);
> +	geth_set_freeq_irq(geth, true);

[Severity: Medium]
Should this re-enable be skipped when geth_setup_freeq() returns an error?

By this point geth_cleanup_freeq() has already done the following:

- set wptr = rptr
- written 0 to GLOBAL_SW_FREEQ_BASE_SIZE_REG
- kfree()d freeq_pages and freed the ring, without NULLing either pointer

The geth_setup_freeq() error paths kfree() the new freeq_pages without
NULLing it, and they set freeq_ring to NULL. A dma_alloc_coherent() failure
leaves the stale freeq_pages from cleanup in place.

Setup failure looks easy to hit. For example, ethtool -G ethX rx 16384 asks
for an order 15 queue backed by many alloc_page(GFP_ATOMIC) pages.

The queue has just been emptied, and resize never ACKs
GLOBAL_INTERRUPT_STATUS_4_REG. If SWFQ_EMPTY status is latched, this
re-enable wakes gemini_port_irq_thread()->geth_fill_freeq(geth, true).

Can that walk freeq_pages[pn] in freed memory using the new, larger
freeq_order? Could it call page_ref_count() and page_ref_add() on stale page
pointers, and write through the NULL or freed freeq_ring in
geth_freeq_alloc_map_page()?

Later in this series, "net: ethernet: cortina: Preserve in-flight free
queue pages" resolves this. It returns early on error before the re-enable,
and it NULLs freeq_pages and freeq_ring in the cleanup and error paths.

>
>  	return ret;
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928-gemini-ethernet-fixes-3-v2-0-758a795d7a78%40kernel.org