Re: [PATCH net-next v2 01/11] net: ethernet: cortina: Keep shared free queue parent-owned

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message is incomplete for a stable-relevant memory-
  safety fix.
- [Low] At this commit, the new guard `if (!geth->freeq_ring) return;` in
  geth_cleanup_freeq() treats NULL as meaning 'no queue'.
- [Low] The patch moves all shared global-register teardown in the remove
  path so it runs after devm_of_platform_depopulate().
- [Low] The concern is that GLOBAL_INTERRUPT_SELECT_4_REG (0xFF000003,
  line 2429) routes the free-queue-empty interrupt to int1 (port1's…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  geth->port0/geth->port1 are written only in…
- [High] This problem wasn't introduced by this patch, but
  gemini_port_remove() calls phy_disconnect(port->netdev->phydev) before…
- [High] At this commit, geth_cleanup_freeq() forces each free-queue
  page's refcount to zero with `while (page_ref_count(gpage->page) > 0)…

--

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

This commit stops each Gemini port child device from tearing down the
software free queue that both ports share. Only the parent's
gemini_ethernet_remove() now does the cleanup, after it depopulates the
children and disables the free-queue interrupt. It also adds a NULL check
on freeq_ring in geth_cleanup_freeq().

Resolves: https://lore.kernel.org/netdev/20260917192835.58126-1-mhun512@gmail.com/ (local)

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

The software free queue is shared by both Ethernet ports, but each
child removal tears it down. The parent removal then tears it down once
more. This can destroy the queue while the sibling port is active and can
free it repeatedly.

[ ... ]

Reported-by: Myeonghun Pak <redacted>
Resolves: https://lore.kernel.org/netdev/20260917192835.58126-1-mhun512@gmail.com/ (local)

[Severity: Low]
Is the order described here accurate for the common case?

Before this patch, gemini_ethernet_remove() called geth_cleanup_freeq()
first, while both netdevs were still registered. Devres then depopulated
the children, and each gemini_port_remove() freed the queue again. So a
plain rmmod or parent unbind could free the queue up to three times, not
only after a single-port unbind.

This fixes a repeated free of the shared queue. Should it carry a Fixes:
tag? The per-child geth_cleanup_freeq(port->geth) call removed here was
added by:

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

Also, Resolves: isn't a standard trailer. Should it be Closes:, placed
directly after Reported-by:? That is what checkpatch expects.

[Severity: Low]
This guard treats a NULL freeq_ring as "no queue allocated". However,
geth_cleanup_freeq() itself does not keep that rule:

	kfree(geth->freeq_pages);

	dma_free_coherent(geth->dev,
			  sizeof(*geth->freeq_ring) << geth->freeq_order,
			  geth->freeq_ring, geth->freeq_dma_base);

Both freeq_pages and freeq_ring are left dangling afterwards. Only the
error path in geth_setup_freeq() sets freeq_ring to NULL. Can the guard
detect a queue that has already been cleaned up?

No second cleanup call seems reachable at this commit. The later patch in
this series, "net: ethernet: cortina: Preserve in-flight free queue
pages", clears freeq_ring, freeq_pages, freeq_page_bitmap and
num_freeq_pages at the end of geth_cleanup_freeq() and on every
geth_setup_freeq() error path.

 	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: High]
This isn't a bug introduced by this patch. Further down in
geth_cleanup_freeq(), the refcount of every free queue page is forced to
zero:

		gpage = &geth->freeq_pages[pn];
		while (page_ref_count(gpage->page) > 0)
			put_page(gpage->page);

Does this also drop the references held by skb fragments? gmac_rx() hands
these pages to the stack through skb_fill_page_desc() and
napi_gro_frags(). Those skbs can outlive the queue, so this looks like a
use-after-free of in-flight RX pages.

This is fixed later in the series by "net: ethernet: cortina: Preserve
in-flight free queue pages". There, geth_freeq_release_pages() drops only
the outstanding per-fragment hardware references and the queue's base
reference. The related DMA map/unmap size mismatch in this function is
fixed by "net: ethernet: cortina: Correct free queue DMA mappings".

[ ... ]

[Severity: High]
This is a pre-existing issue. The new devm_of_platform_depopulate() call
in gemini_ethernet_remove() now leads straight here. What happens if the
interface is still up when it is removed?

gemini_port_remove() does:

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

phy_disconnect()->phy_detach()->phy_detach_internal() runs:

	phydev->attached_dev->phydev = NULL;

For an IFF_UP interface, unregister_netdev() then calls ndo_stop:

gemini_ethernet_remove()
  devm_of_platform_depopulate()
    gemini_ethernet_port_remove()
      gemini_port_remove()
        phy_disconnect()              /* netdev->phydev = NULL */
        unregister_netdev()
          gmac_stop()
            phy_stop(netdev->phydev)  /* NULL */

Can phy_stop() oops when it dereferences phydev->attached_dev?

Unbinding a single port reaches the same path. The order dates back to
3e813d61401a ("net: gemini: Clean up phy registration") and is unchanged
at the end of the series.

[Severity: High]
This isn't a bug introduced by this patch. Now that a sibling port is
meant to keep running after the other port is removed, what happens to
geth->port0 and geth->port1?

They are only set in gemini_ethernet_port_probe():

	if (!id)
		geth->port0 = port;
	else
		geth->port1 = port;

Nothing clears them: not gemini_port_remove(), and not the probe error
paths after the assignment (devm_request_threaded_irq(),
gmac_setup_phy() or register_netdev() failure).

The port struct is the netdev_priv() of a netdev allocated with
devm_alloc_etherdev_mqs() on the port device. It is freed when that port
unbinds or fails probe.

geth_resize_freeq() then dereferences the sibling unconditionally:

	if (netdev->dev_id == 0)
		other_netdev = geth->port1->netdev;
	else
		other_netdev = geth->port0->netdev;

It follows up with netif_running(other_netdev) and
other_port->rxq_order. Could this be a use-after-free after, for
example, a sysfs unbind of one port followed by ip link set up or
ethtool -G on the surviving port?

The same seems possible in two other cases:

- port0 fails probe (for example, no PHY is found) and eth1 is then
  brought up.
- During depopulate, one child has been freed while the other is still
  registered.

These dereferences are still there at the end of the series, and more are
added: synchronize_irq(geth->port1->irq) in geth_resize_freeq(), and
geth->port0->rxq_order / geth->port1->rxq_order in geth_setup_freeq().

[Severity: Low]
gemini_ethernet_init() routes the FreeQ interrupt to int1, which is
port1's line:

	writel(0xFF000003, geth->base + GLOBAL_INTERRUPT_SELECT_4_REG);

With the per-port cleanup gone, unbinding only port1 leaves
SWFQ_EMPTY_INT_BIT enabled while port0 keeps using the shared queue. No
handler is left on int1. Is that a problem in practice?

From the code it looks harmless:

- port0's gemini_port_irq() is registered with IRQF_SHARED and checks the
  global SWFQ_EMPTY bit in GLOBAL_INTERRUPT_STATUS_4_REG, whatever the
  routing.
- gmac_napi_poll() also refills the queue through geth_fill_freeq().
- free_irq() masks port1's line at the interrupt controller.

Before this patch, the same scenario was a use-after-free of the freed
queue.

[ ... ]

[Severity: Low]
When these global register writes run, both children have already gone
through gemini_port_remove() and called clk_disable_unprepare(port->pclk).

The gmac0-gate and gmac1-gate clocks in drivers/clk/clk-gemini.c are
registered with flags 0, so they really do switch off. The parent node in
gemini.dtsi has no clocks property.

Does the global register block at 0x60000000 depend on either GMAC gate?
If so, some accesses could be lost or could stall the bus:

- the writel() to GLOBAL_INTERRUPT_ENABLE_4_REG here
- the GLOBAL_SWFQ_RWPTR_REG and GLOBAL_SW_FREEQ_BASE_SIZE_REG accesses in
  geth_cleanup_freeq()

Before this patch, the parent's cleanup ran while those clocks were still
enabled.

This may be fine. gemini_ethernet_probe() already reads
GLOBAL_TOE_VERSION_REG before any child enables PCLK, which suggests the
global block has its own clock.

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