Re: [PATCH net-next v4 13/15] net: ethernet: cortina: Release partial RX frames on stop

[netdev-bot+sashiko@kernel.org]

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 fixes a real leak in gmac_stop() but has no Fixes:
  trailer (and no Cc: stable).

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the
  rx_coalesce_timer lifecycle in gmac_open()/gmac_stop() is out of order…
- [Low] This problem wasn't introduced by this patch, but
  port->rx_stats_syncp (and tx_stats_syncp/ir_stats_syncp) is never…

--

net: ethernet: cortina: Release partial RX frames on stop

This patch frees any partially assembled RX frag skb in gmac_stop() after
napi_disable() and before the saved RX state is cleared. The discarded
frame is counted in rx_dropped, using the irqsave u64_stats helpers.

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 43fd8fd773e3c..32291d4c33904 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -2209,12 +2209,19 @@ static int gmac_open(struct net_device *netdev)
>  static int gmac_stop(struct net_device *netdev)
>  {
>  	struct gemini_ethernet_port *port = netdev_priv(netdev);
> +	unsigned long flags;
>  
>  	hrtimer_cancel(&port->rx_coalesce_timer);

[Severity: High]
This isn't a bug introduced by this patch, but is rx_coalesce_timer
cancelled too early here?

hrtimer_cancel() runs before gmac_enable_irq(netdev, 0) masks the RX
interrupt, and before free_irq(). While NAPI is idle the RX queue interrupt
is enabled, because gmac_napi_poll() calls gmac_enable_rx_irq(napi->dev, 1)
when it completes. If rx_coalesce_nsecs is non-zero, an RX interrupt in
that window re-arms the timer from gmac_irq():

drivers/net/ethernet/cortina/gemini.c:gmac_irq() {
    ...
	if (val & (DEFAULT_Q0_INT_BIT << netdev->dev_id)) {
		gmac_enable_rx_irq(netdev, 0);

		if (!port->rx_coalesce_nsecs) {
			napi_schedule(&port->napi);
		} else {
			...
			hrtimer_start(&port->rx_coalesce_timer, ktime,
				      HRTIMER_MODE_REL);
		}
	}
    ...
}

Can the timer then still be pending after gmac_stop() returns? On device
removal, the netdev private area that embeds the hrtimer could be freed
while the timer is enqueued. gmac_coalesce_delay_expired() would then run
on freed memory.

On a quick reopen, gmac_open() calls hrtimer_setup(). Its
__hrtimer_setup() does a memset() on a timer that may still be linked into
the hrtimer base rbtree.

gmac_open() has a similar ordering problem, because interrupts are enabled
before the timer is set up:

drivers/net/ethernet/cortina/gemini.c:gmac_open() {
    ...
	gmac_start_dma(port);
	gmac_enable_irq(netdev, 1);
	gmac_enable_tx_rx(netdev);
	netif_tx_start_all_queues(netdev);

	hrtimer_setup(&port->rx_coalesce_timer, &gmac_coalesce_delay_expired, CLOCK_MONOTONIC,
		      HRTIMER_MODE_REL);
    ...
}

gmac_set_coalesce() accepts a non-zero rx-usecs while the interface is
down. On the first open after that, an RX interrupt in this window would
call hrtimer_start() on a zeroed hrtimer. lock_hrtimer_base() would then
dereference a NULL timer->base in hardirq context.

On later opens, an interrupt in the same window can arm the old timer just
before hrtimer_setup() clears it.

Would it make sense to set up the timer once at probe time, and to cancel
it in gmac_stop() only after the RX interrupt is masked?

>  	netif_tx_stop_all_queues(netdev);
>  	gmac_disable_tx_rx(netdev);
>  	gmac_stop_dma(port);
>  	napi_disable(&port->napi);
> +	if (port->rx_skb) {
> +		napi_free_frags(&port->napi);
> +		flags = u64_stats_update_begin_irqsave(&port->rx_stats_syncp);

[Severity: Low]
This is a pre-existing issue, but is rx_stats_syncp ever initialized with
u64_stats_init()?

The port is allocated with devm_alloc_etherdev_mqs() in
gemini_ethernet_port_probe(). The driver has no u64_stats_init() call for
rx_stats_syncp, tx_stats_syncp or ir_stats_syncp.

On 32-bit builds with CONFIG_DEBUG_LOCK_ALLOC, write_seqcount_begin()
calls lock_acquire() on a dep_map that has no key. Lockdep then prints
"trying to register non-static key" and turns itself off.

The existing writer in gmac_napi_poll() and the readers already run into
this. The new call in gmac_stop() is one more user.

> +		port->stats.rx_dropped++;
> +		u64_stats_update_end_irqrestore(&port->rx_stats_syncp, flags);
> +	}
>  	port->rx_skb = NULL;

[Severity: Low]
Should this patch have a Fixes: tag?

The leak seems to come from commit 06937db21ee3 ("net: ethernet: cortina:
Make RX SKB per-port"). That commit added the "port->rx_skb = NULL;" line
in gmac_stop() without freeing napi->skb.

Before 06937db21ee3, the static skb in gmac_rx() kept its value across
stop and open. The start-of-frame handling in gmac_rx() would then free the
stale partial frame:

	if (skb) {
		napi_free_frags(&port->napi);
		port->stats.rx_dropped++;
		...
	}

napi_disable() and napi_enable() do not release napi->skb, and
napi_get_frags() returns the existing napi->skb. After a reopen, the stale
skb and its old frags are reused, and the page references they hold are
leaked.

06937db21ee3 carries its own Fixes: tag and is headed for stable. Would it
help to add the following, so this fix reaches the same trees?

Fixes: 06937db21ee3 ("net: ethernet: cortina: Make RX SKB per-port")

>  	port->rx_frag_nr = 0;
>  	port->rx_dropping = false;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261011-gemini-ethernet-fixes-3-v4-0-4b1631b08f5d%40kernel.org