netdev - Re: [net-next,v25,6/6] eea: introduce callback for ndo_get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20260204040841.1703575-1-kuba@kernel.org>
Date: Tue,  3 Feb 2026 20:08:41 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: xuanzhuo@...ux.alibaba.com
Cc: Jakub Kicinski <kuba@...nel.org>,
	lorenzo@...nel.org,
	andrew+netdev@...n.ch,
	pabeni@...hat.com,
	vadim.fedorenko@...ux.dev,
	davem@...emloft.net,
	guwen@...ux.alibaba.com,
	lulie@...ux.alibaba.com,
	hkallweit1@...il.com,
	edumazet@...gle.com,
	lukas.bulwahn@...hat.com,
	andrew@...n.ch,
	dong100@...se.com,
	dust.li@...ux.alibaba.com,
	netdev@...r.kernel.org
Subject: Re: [net-next,v25,6/6] eea: introduce callback for ndo_get_stats64

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
eea: introduce callback for ndo_get_stats64

This commit adds ndo_get_stats64 support to the eea driver, implementing
statistics collection for rx/tx packets and bytes. A spinlock is introduced
to protect access to the rx and tx queue pointers during stats collection.

> diff --git a/drivers/net/ethernet/alibaba/eea/eea_net.c b/drivers/net/ethernet/alibaba/eea/eea_net.c
> --- a/drivers/net/ethernet/alibaba/eea/eea_net.c
> +++ b/drivers/net/ethernet/alibaba/eea/eea_net.c
> @@ -27,8 +27,10 @@ static int enet_bind_new_q_and_cfg(struct eea_net *enet,
>
>  	enet->cfg = ctx->cfg;
>
> +	spin_lock(&enet->stats_lock);
>  	enet->rx = ctx->rx;
>  	enet->tx = ctx->tx;
> +	spin_unlock(&enet->stats_lock);

[ ... ]

> @@ -57,11 +59,13 @@ static void eea_free_rxtx_q_mem(struct eea_net *enet)
>  	struct eea_net_tx *tx, *tx_array;
>  	int i;
>
> +	spin_lock(&enet->stats_lock);
>  	rx_array = enet->rx;
>  	tx_array = enet->tx;
>
>  	enet->rx = NULL;
>  	enet->tx = NULL;
> +	spin_unlock(&enet->stats_lock);

[ ... ]

> @@ -260,6 +264,50 @@ static int eea_netdev_open(struct net_device *netdev)
>  	return err;
>  }
>
> +static void eea_stats(struct net_device *netdev, struct rtnl_link_stats64 *tot)
> +{
> +	struct eea_net *enet = netdev_priv(netdev);
> +	u64 packets, bytes;
> +	u32 start;
> +	int i;
> +
> +	spin_lock_bh(&enet->stats_lock);

Can this cause a deadlock? The enet_bind_new_q_and_cfg() and
eea_free_rxtx_q_mem() functions use spin_lock(&enet->stats_lock), but
eea_stats() uses spin_lock_bh(&enet->stats_lock).

If a thread holds stats_lock via spin_lock() (softirqs still enabled) and a
softirq fires that calls eea_stats() through ndo_get_stats64, the
spin_lock_bh() will attempt to acquire the already-held lock.

Should all acquisition sites use spin_lock_bh()/spin_unlock_bh() for
consistency?

> +
> +	if (enet->rx) {
> +		for (i = 0; i < enet->cfg.rx_ring_num; i++) {
> +			struct eea_net_rx *rx = enet->rx[i];

[ ... ]