Re: [PATCH net-next v3 01/12] net: ethernet: cortina: Keep PHY-less port bound for shared IRQ

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 2 · 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).

- [High] The new guard in geth_resize_freeq() only checks geth->port1,
  but the new gemini_port_clear() in the probe error path can now leave…
- [Medium] Port 0 still loses the shared free-queue handler when the IRQ-
  only port 1 is unbound or fails probe late.
- [Medium] The new `if (!geth->port1) return -ENODEV;` check in
  geth_resize_freeq() is a lockless check-then-use that does not…

--

net: ethernet: cortina: Keep PHY-less port bound for shared IRQ

The commit keeps Gemini ethernet port children that have no PHY
description bound as IRQ-only ports. This keeps the shared free queue
threaded IRQ, which is routed through port 1, registered. It also clears
the parent's port pointers on probe failure and removal, and makes
geth_resize_freeq() return -ENODEV when port 1 is absent.

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 2fe7fd0202d26..2be3e90519811 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -1048,10 +1048,15 @@ static int geth_resize_freeq(struct gemini_ethernet_port *port)
>  	u32 en;
>  	int ret;
>  
> +	/* The software free queue interrupt is routed through port 1. */
> +	if (!geth->port1)
> +		return -ENODEV;
> +

[Severity: High]
Is checking only geth->port1 enough here? With the new
gemini_port_clear() call in the unprepare path of
gemini_ethernet_port_probe(), geth->port0 can now be NULL by the time
port 1 probes. In that case the shared block is never initialized.

Take a board with PHYs on both ports:

port 0 probe
  geth->port0 = port;
  gemini_ethernet_init()          /* port1 NULL, returns early */
  gmac_setup_phy() fails          /* e.g. -ENODEV, PHY not on MDIO */
  unprepare: gemini_port_clear()  /* geth->port0 = NULL */

port 1 probe
  geth->port1 = port;
  gemini_ethernet_init()          /* port0 NULL, returns early again */
  register_netdev()               /* eth1 is registered */

gemini_ethernet_init() is the only place that programs the
GLOBAL_INTERRUPT_SELECT_* and SWFQ/HWFQ registers and sets
freeq_frag_order and freeq_order:

	if (geth->port0 && geth->port1)
		geth->initialized = true;
	else
		return;
	...
	geth->freeq_frag_order = DEFAULT_RX_BUF_ORDER;

When eth1 is opened, gmac_open()->geth_resize_freeq() passes this check
and calls geth_setup_freeq() while freeq_frag_order is still 0 from
devm_kzalloc(). That makes fpp_order equal to PAGE_SHIFT and
pages = 1024 >> 12 = 0. kzalloc_objs(..., 0) then returns ZERO_SIZE_PTR,
which passes the NULL check.

geth_fill_freeq() then does:

	m_pn = (1 << (geth->freeq_order - fpp_order)) - 1;
	...
		gpage = &geth->freeq_pages[pn];
		page = gpage->page;

The shift is out of range, and the page lookup goes through
ZERO_SIZE_PTR. Can this oops?

Later in the series, "Scale Gemini RX queues to system memory" sends the
same uninitialized state to geth_freeq_post_page(). That function writes
1 << 12 descriptors starting at geth->freeq_ring + (pn << 12) into a
ring sized for 1 << 10 entries. Would that write past the DMA coherent
allocation? As far as I can tell, nothing later in the series closes
this path.

Before this patch, geth->port0 stayed non-NULL (dangling), so
gemini_ethernet_init() still ran.

Should this check geth->initialized, or both ports, instead of only
geth->port1?

>  	if (netdev->dev_id == 0)
> -		other_netdev = geth->port1->netdev;
> +		other_port = geth->port1;
>  	else
> -		other_netdev = geth->port0->netdev;
> +		other_port = geth->port0;
> +	other_netdev = other_port ? other_port->netdev : NULL;

[Severity: Medium]
Is this check-then-use of geth->port1 safe if port 1 is probed or
removed at the same time? geth_resize_freeq() runs under RTNL from
gmac_open() and gmac_set_ringparam(). geth->port1 is written without
RTNL or any other common lock.

On the remove side, gemini_port_remove() skips this block for the
IRQ-only port:

	if (port->netdev) {
		phy_disconnect(port->netdev->phydev);
		unregister_netdev(port->netdev);
	}

so it never takes RTNL. gemini_port_clear() runs next, and devres frees
the port (it is part of the devm netdev allocation) right afterwards.

A concurrent open of eth0 can pass the guard and load
other_port = geth->port1. It then reads other_port->netdev and
other_port->rxq_order. Could those reads hit freed memory?

Later in the series, geth_resize_freeq() also reads geth->port1->irq
again for disable_irq() and enable_irq(). Could that be a NULL
dereference if port 1 is cleared between the reads?

On the probe side, gemini_ethernet_port_probe() publishes geth->port1
before gemini_ethernet_init() and before devm_request_threaded_irq().

Can an eth0 open in that window resize the queue and enable SWFQ_EMPTY
before the handler exists? gemini_ethernet_init() may still be writing
GLOBAL_INTERRUPT_ENABLE_4_REG, the SW free queue registers,
freeq_frag_order and freeq_order without irq_lock at that point.

>  
>  	if (other_netdev && netif_running(other_netdev))
>  		return -EBUSY;

[ ... ]

> @@ -2392,6 +2406,7 @@ static void gemini_port_remove(struct gemini_ethernet_port *port)
>  	}
>  	clk_disable_unprepare(port->pclk);
>  	geth_cleanup_freeq(port->geth);
> +	gemini_port_clear(port);
>  }

[Severity: Medium]
Can port 0 still lose the shared free queue handler here? Port 1 now
stays bound only to provide the SWFQ threaded handler. Nothing stops it
from being unbound while eth0 is running: the port driver does not set
suppress_bind_attrs, and the IRQ-only remove path does not check the
sibling port.

In this commit, unbinding the IRQ-only port 1 while eth0 is up also
calls geth_cleanup_freeq() unconditionally. That frees and unmaps the
shared ring and pages while port 0's RX DMA, and possibly
gemini_port_irq_thread(), still use them. Is that a use-after-free? The
later patch "net: ethernet: cortina: Keep shared free queue
parent-owned" removes this call.

Even with that later patch, this unbind still runs gemini_port_clear():

  echo 6000c000.ethernet-port > /sys/bus/platform/drivers/gemini-ethernet-port/unbind

devres then frees port 1's threaded IRQ. SWFQ_EMPTY_INT_BIT stays set in
GLOBAL_INTERRUPT_ENABLE_4_REG, but no handler is left on int1, and
gemini_port_irq_thread() is the only refill path.

Would port 0 RX stall once the posted fragments are used up, while eth0
still reports itself as running?

The same thing can happen if eth0 is opened after port 1 publishes
geth->port1, and port 1 then fails late in gmac_setup_phy() or
register_netdev(). The unprepare path clears geth->port1, and devres
frees the IRQ.

This looks like the "releases the threaded handler needed by the shared
queue" case the commit message describes, reached through the remove
path and the late probe failure path.

[ ... ]

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