netdev - Re: [PATCH net-next] net: dlink: add synchronization for stats update

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: <a942be9e-2799-478d-b8c2-7a85f3a58f6c@intel.com>
Date: Mon, 21 Apr 2025 16:23:19 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Moon Yeounsu <yyyynoom@...il.com>, Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>
CC: <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next] net: dlink: add synchronization for stats update



On 4/21/2025 12:16 PM, Moon Yeounsu wrote:
> There are two paths that call `get_stats()`:
>     1. From user space via the `ip` command
>     2. From interrupt context via `rio_interrupt()`
> 
> Case 1 is synchronized by `rtnl_lock()`, so it is safe.
> However, the two cases above are not synchronized with each other.
> Therefore, `spin_lock` is needed to protect `get_stats()` as it can be
> preempted by an interrupt. In this context, `spin_lock_irq()` is required
> (using `spin_lock_bh()` may result in a deadlock).
> 
> While `spin_lock` protects `get_stats()`, it does not protect updates to
> `dev->stats.tx_errors` and `dev->stats.collisions`, which may be
> concurrently modified by the interrupt handler and user space.
> By using temporary variables in `np->tx_errors` and `np->collisions`,
> we can safely update `dev->stats` without additional locking.
> 
> Tested-on: D-Link DGE-550T Rev-A3
> Signed-off-by: Moon Yeounsu <yyyynoom@...il.com>
> ---
> Question:
> 	This might be a bit off-topic, but I don’t fully understand why a single global
> 	`rtnl_lock` is used for synchronization. While I may not be fully aware of the 
> 	design rationale, it seems somewhat suboptimal. I believe it could be improved.

Its been a long standing effort to reduce the use of RTNL lock, with a
lot of effort going into switching calls to use the per-netdev lock instead.

I think you could switch the driver over to the ops locked method by
setting the relevant flag in netdev to avoid global lock contention,
since 605ef7aec060 ("net: add option to request netdev instance lock")
which is coming in v6.15

Thanks,
Jake

> ---
>  drivers/net/ethernet/dlink/dl2k.c | 11 +++++++++--
>  drivers/net/ethernet/dlink/dl2k.h |  5 +++++
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/dlink/dl2k.c b/drivers/net/ethernet/dlink/dl2k.c
> index d0ea92607870..2d929a83e101 100644
> --- a/drivers/net/ethernet/dlink/dl2k.c
> +++ b/drivers/net/ethernet/dlink/dl2k.c
> @@ -865,7 +865,7 @@ tx_error (struct net_device *dev, int tx_status)
>  	frame_id = (tx_status & 0xffff0000);
>  	printk (KERN_ERR "%s: Transmit error, TxStatus %4.4x, FrameId %d.\n",
>  		dev->name, tx_status, frame_id);
> -	dev->stats.tx_errors++;
> +	np->tx_errors++;
>  	/* Ttransmit Underrun */
>  	if (tx_status & 0x10) {
>  		dev->stats.tx_fifo_errors++;
> @@ -904,7 +904,7 @@ tx_error (struct net_device *dev, int tx_status)
>  	}
>  	/* Maximum Collisions */
>  	if (tx_status & 0x08)
> -		dev->stats.collisions++;
> +		np->collisions++;
>  	/* Restart the Tx */
>  	dw32(MACCtrl, dr16(MACCtrl) | TxEnable);
>  }
> @@ -1074,6 +1074,7 @@ get_stats (struct net_device *dev)
>  #endif
>  	unsigned int stat_reg;
>  
> +	spin_lock_irq(&np->stats_lock);
>  	/* All statistics registers need to be acknowledged,
>  	   else statistic overflow could cause problems */
>  
> @@ -1085,6 +1086,7 @@ get_stats (struct net_device *dev)
>  	dev->stats.multicast = dr32(McstFramesRcvdOk);
>  	dev->stats.collisions += dr32(SingleColFrames)
>  			     +  dr32(MultiColFrames);
> +	dev->stats.collisions += np->collisions;
>  
>  	/* detailed tx errors */
>  	stat_reg = dr16(FramesAbortXSColls);
> @@ -1095,6 +1097,8 @@ get_stats (struct net_device *dev)
>  	dev->stats.tx_carrier_errors += stat_reg;
>  	dev->stats.tx_errors += stat_reg;
>  
> +	dev->stats.tx_errors += np->tx_errors;
> +
>  	/* Clear all other statistic register. */
>  	dr32(McstOctetXmtOk);
>  	dr16(BcstFramesXmtdOk);
> @@ -1123,6 +1127,9 @@ get_stats (struct net_device *dev)
>  	dr16(TCPCheckSumErrors);
>  	dr16(UDPCheckSumErrors);
>  	dr16(IPCheckSumErrors);
> +
> +	spin_unlock_irq(&np->stats_lock);
> +
>  	return &dev->stats;
>  }
>  
> diff --git a/drivers/net/ethernet/dlink/dl2k.h b/drivers/net/ethernet/dlink/dl2k.h
> index 195dc6cfd895..dc8755a69b73 100644
> --- a/drivers/net/ethernet/dlink/dl2k.h
> +++ b/drivers/net/ethernet/dlink/dl2k.h
> @@ -372,6 +372,8 @@ struct netdev_private {
>  	struct pci_dev *pdev;
>  	void __iomem *ioaddr;
>  	void __iomem *eeprom_addr;
> +	// To ensure synchronization when stats are updated.
> +	spinlock_t stats_lock;
>  	spinlock_t tx_lock;
>  	spinlock_t rx_lock;
>  	unsigned int rx_buf_sz;		/* Based on MTU+slack. */
> @@ -401,6 +403,9 @@ struct netdev_private {
>  	u16 negotiate;		/* Negotiated media */
>  	int phy_addr;		/* PHY addresses. */
>  	u16 led_mode;		/* LED mode read from EEPROM (IP1000A only) */
> +
> +	u64 collisions;
> +	u64 tx_errors;
>  };

Code changes seem fine to me.

Reviewed-by: Jacob Keller <jacob.e.keller@...el.com>

>  
>  /* The station address location in the EEPROM. */