Re: [PATCH net-next v3 02/12] net: ethernet: cortina: Keep shared free queue parent-owned

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] This patch makes the parent device own the shared software
  free queue (SWFQ), but the IRQ handler that refills it is still owned…
- [Medium] gemini_port_clear() clears geth->port0/port1 without any
  synchronization against sibling readers in geth_resize_freeq().

Pre-existing issues:
- [Critical] At this commit, geth_cleanup_freeq() frees each queue page
  with `while (page_ref_count(gpage->page) > 0) put_page(gpage->page);`…
- [High] This bug was not introduced by this patch, but the new
  devm_of_platform_depopulate() call now reaches it directly.

--

net: ethernet: cortina: Keep shared free queue parent-owned

This commit stops each port's remove path from tearing down the shared
software free queue. That cleanup now runs only from the parent remove
callback. The parent first depopulates both port devices and masks
GLOBAL_INTERRUPT_ENABLE_4_REG, then calls geth_cleanup_freeq(). That
function now returns early if no queue was allocated.

Closes: https://lore.kernel.org/netdev/20260917192835.58126-1-mhun512@gmail.com/

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 2be3e90519811..15ad9f721dd52 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -1004,6 +1004,9 @@ static void geth_cleanup_freeq(struct gemini_ethernet *geth)
>  	unsigned int pages = len >> fpp_order;
>  	unsigned int pn;
>  
> +	if (!geth->freeq_ring)
> +		return;
> +
>  	writew(readw(geth->base + GLOBAL_SWFQ_RWPTR_REG),
>  	       geth->base + GLOBAL_SWFQ_RWPTR_REG + 2);
>  	writel(0, geth->base + GLOBAL_SW_FREEQ_BASE_SIZE_REG);

[Severity: Critical]
This is a pre-existing issue, but gemini_ethernet_remove() is now the only
caller of geth_cleanup_freeq(). Could the page release loop further down
in this function over-release pages?

drivers/net/ethernet/cortina/gemini.c:geth_cleanup_freeq() {
    ...
		gpage = &geth->freeq_pages[pn];
		while (page_ref_count(gpage->page) > 0)
			put_page(gpage->page);
    ...
}

This loop also drops the references held by skbs that gmac_rx() passed up
the stack. Pages still referenced by sockets would then go back to the
allocator. The loop also reads the refcount of a page that may already
have been freed and reused.

If the parent is unbound, or the module is removed, while delivered skbs
are still queued, could this cause a use-after-free or a double free?

This appears to be fixed later in the series by "net: ethernet: cortina:
Preserve in-flight free queue pages". In that patch,
geth_freeq_release_pages() drops only the references the driver owns.

> @@ -2405,7 +2408,6 @@ static void gemini_port_remove(struct gemini_ethernet_port *port)
>  		unregister_netdev(port->netdev);
>  	}
>  	clk_disable_unprepare(port->pclk);
> -	geth_cleanup_freeq(port->geth);
>  	gemini_port_clear(port);
>  }

[Severity: Medium]
With the parent now owning the queue, what happens if only port1 is
unbound through sysfs while port0 is up?

The port driver does not set suppress_bind_attrs, so this can be done
from sysfs. After this change, gemini_port_remove() on port1 leaves
GLOBAL_SW_FREEQ_BASE_SIZE_REG programmed. It also leaves
SWFQ_EMPTY_INT_BIT set in GLOBAL_INTERRUPT_ENABLE_4_REG.

Devres then frees the port1 handler that gemini_ethernet_port_probe()
requested:

	ret = devm_request_threaded_irq(port->dev,
					port->irq,
					gemini_port_irq,
					gemini_port_irq_thread,
	...

However, gemini_ethernet_init() routes the free queue interrupt to the
port1 line:

	 *	FreeQ intr -----------> int1 ----> eth1

The queue-empty event would then arrive on an IRQ with no handler, while
port0 keeps using up fragments. A refill would only happen if an
unrelated int0 interrupt ran gemini_port_irq() for port0 and it noticed
the STATUS_4 bit. Once RX is starved, that may never happen.

Can port0 RX stall in this case?

Also, geth->port1 is now NULL, so reopening port0 would fail in
geth_resize_freeq() while the hardware queue is still live and armed:

	/* The software free queue interrupt is routed through port 1. */
	if (!geth->port1)
		return -ENODEV;

The new writel() to GLOBAL_INTERRUPT_ENABLE_4_REG runs only from
gemini_ethernet_remove(), not when port1 is unbound by itself. This looks
the same in the final state of the series.

[Severity: Medium]
What keeps gemini_port_clear() from running while the sibling port is
inside geth_resize_freeq()?

When gmac_open() runs for port0, it holds only RTNL, and
geth_resize_freeq() does:

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

On the port1 unbind side, gemini_port_clear() runs after
unregister_netdev() has released RTNL. If port1 has no netdev, the unbind
path takes RTNL at no point. Devres then frees the netdev that contains
the port struct.

The platform bus takes no parent lock, so these two paths do not share a
lock.

Could geth_resize_freeq() read other_port->netdev or
other_port->rxq_order after the port struct has been freed?

The window gets wider later in the series. There, geth_resize_freeq()
reads geth->port1 again for enable_irq(geth->port1->irq) after
geth_setup_freeq(). geth_setup_freeq() can sleep in GFP_KERNEL and
dma_alloc_coherent() allocations.

If port1 is cleared during that window, would this be a NULL dereference?
Or a use-after-free plus an unbalanced enable_irq() on a freed IRQ?

In the final state of the series, geth_freeq_page_slots() also
dereferences geth->port0 and geth->port1 with only a NULL check.

> @@ -2706,6 +2708,8 @@ static void gemini_ethernet_remove(struct platform_device *pdev)
>  {
>  	struct gemini_ethernet *geth = platform_get_drvdata(pdev);
>  
> +	devm_of_platform_depopulate(&pdev->dev);

[Severity: High]
This isn't a bug introduced by this patch, but the new
devm_of_platform_depopulate() call now reaches it straight from the parent
remove. If a port's netdev is up, does gemini_port_remove() end up calling
phy_stop() with a NULL phydev?

gemini_port_remove()
  phy_disconnect(port->netdev->phydev)
    phy_detach()
      phydev->attached_dev->phydev = NULL;
  unregister_netdev(port->netdev)
    ... __dev_close_many()
      gmac_stop()
        phy_stop(netdev->phydev)   <-- now NULL
          struct net_device *dev = phydev->attached_dev;

phy_stop() does not check for NULL. This looks like a deterministic oops
when the parent or a port is unbound, or the module is removed, while the
interface is up.

The same ordering is present before this patch and in the final state of
the series.

> +	writel(0, geth->base + GLOBAL_INTERRUPT_ENABLE_4_REG);
>  	geth_cleanup_freeq(geth);
>  	geth->initialized = false;
>  }

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