Re: [PATCH net-next v7 15/18] net: ethernet: cortina: Scope RX statistics updates

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · 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] net: cortina: softirq-unsafe tx_stats_syncp update in
  gmac_clean_txq()

--

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -2870,6 +2879,9 @@ static int gemini_ethernet_port_probe(struct platform_device *pdev)
>  	}
>  
>  	port = netdev_priv(netdev);
> +	u64_stats_init(&port->tx_stats_syncp);
> +	u64_stats_init(&port->rx_stats_syncp);
> +	u64_stats_init(&port->ir_stats_syncp);
>  	SET_NETDEV_DEV(netdev, dev);
>  	port->netdev = netdev;
>  	port->id = id;
>

[Severity: Medium]
Initializing tx_stats_syncp here is correct. On 32-bit lockdep
kernels, though, it also exposes an existing tx_stats_syncp writer
that runs from the wrong context.

gmac_clean_txq() bumps port->tx_frag_stats[nfrags] under a plain
u64_stats_update_begin(&port->tx_stats_syncp). When called from
gmac_start_xmit(), this runs with BH disabled, so lockdep records the
seqcount as used in softirq.

gmac_stop() also reaches it, via gmac_cleanup_txqs() and
gmac_clean_txq(), but from process context with BH and preemption
enabled. The last frames queued before the interface goes down are
normally still in the ring at that point. If one of them was a
multi-fragment frame (nfrags > 0), the tx_stats_syncp update runs
preemptible and with softirqs enabled.

On 32-bit non-RT, that hits the lockdep_assert_preemption_disabled()
check in __u64_stats_update_begin(). With the lockdep map now
initialized, it should also produce an inconsistent
IN-SOFTIRQ-W -> SOFTIRQ-ON-W report for tx_stats_syncp.

There is a real hazard behind the splat too. While the sequence count
is odd, nothing stops a softirq on the same CPU from reading TX stats
through gmac_get_stats64(). That reader would spin forever in
u64_stats_fetch_begin().

The other counters in these paths are already protected:

- The rx_stats_syncp update in gmac_stop() uses
  u64_stats_update_begin_irqsave().
- The ir_stats_syncp update in gmac_clean_txq() is done under
  geth->irq_lock.

Should the tx_stats_syncp update get the same treatment? Or would you
rather disable BH around gmac_cleanup_txqs() in gmac_stop()? Either
way, I think the fix belongs in this patch or just before it, since
the u64_stats_init() added here is what lets lockdep start tracking
this seqcount.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110-gemini-ethernet-fixes-3-v7-0-9c32a1a46aec%40kernel.org