netdev - Re: [PATCH net] net: cadence: macb: Synchronize stats calculations

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: <20250219094851.0419759f@2a02-8440-d103-5c0b-874f-3af8-c06f-cd89.rev.sfr.net>
Date: Wed, 19 Feb 2025 09:48:51 +0100
From: Maxime Chevallier <maxime.chevallier@...tlin.com>
To: Sean Anderson <sean.anderson@...ux.dev>
Cc: Nicolas Ferre <nicolas.ferre@...rochip.com>, Claudiu Beznea
 <claudiu.beznea@...on.dev>, netdev@...r.kernel.org, Eric Dumazet
 <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
 linux-kernel@...r.kernel.org, "David S . Miller" <davem@...emloft.net>,
 Andrew Lunn <andrew+netdev@...n.ch>, Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH net] net: cadence: macb: Synchronize stats calculations

Hello Sean,

On Tue, 18 Feb 2025 14:50:36 -0500
Sean Anderson <sean.anderson@...ux.dev> wrote:

> Stats calculations involve a RMW to add the stat update to the existing
> value. This is currently not protected by any synchronization mechanism,
> so data races are possible. Add a spinlock to protect the update. The
> reader side could be protected using u64_stats, but we would still need
> a spinlock for the update side anyway. And we always do an update
> immediately before reading the stats anyway.
> 
> Fixes: 89e5785fc8a6 ("[PATCH] Atmel MACB ethernet driver")
> Signed-off-by: Sean Anderson <sean.anderson@...ux.dev>
> ---
> 
>  drivers/net/ethernet/cadence/macb.h      |  2 ++
>  drivers/net/ethernet/cadence/macb_main.c | 12 ++++++++++--
>  2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 5740c98d8c9f..2847278d9cd4 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -1279,6 +1279,8 @@ struct macb {
>  	struct clk		*rx_clk;
>  	struct clk		*tsu_clk;
>  	struct net_device	*dev;
> +	/* Protects hw_stats and ethtool_stats */
> +	spinlock_t		stats_lock;
>  	union {
>  		struct macb_stats	macb;
>  		struct gem_stats	gem;
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 48496209fb16..990a3863c6e1 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -1978,10 +1978,12 @@ static irqreturn_t macb_interrupt(int irq, void *dev_id)
>  
>  		if (status & MACB_BIT(ISR_ROVR)) {
>  			/* We missed at least one packet */
> +			spin_lock(&bp->stats_lock);
>  			if (macb_is_gem(bp))
>  				bp->hw_stats.gem.rx_overruns++;
>  			else
>  				bp->hw_stats.macb.rx_overruns++;
> +			spin_unlock(&bp->stats_lock);
>  
>  			if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
>  				queue_writel(queue, ISR, MACB_BIT(ISR_ROVR));
> @@ -3102,6 +3104,7 @@ static struct net_device_stats *gem_get_stats(struct macb *bp)
>  	if (!netif_running(bp->dev))
>  		return nstat;
>  
> +	spin_lock(&bp->stats_lock);
>  	gem_update_stats(bp);
>  
>  	nstat->rx_errors = (hwstat->rx_frame_check_sequence_errors +
> @@ -3131,6 +3134,7 @@ static struct net_device_stats *gem_get_stats(struct macb *bp)
>  	nstat->tx_aborted_errors = hwstat->tx_excessive_collisions;
>  	nstat->tx_carrier_errors = hwstat->tx_carrier_sense_errors;
>  	nstat->tx_fifo_errors = hwstat->tx_underrun;
> +	spin_unlock(&bp->stats_lock);
>  
>  	return nstat;
>  }
> @@ -3138,12 +3142,13 @@ static struct net_device_stats *gem_get_stats(struct macb *bp)
>  static void gem_get_ethtool_stats(struct net_device *dev,
>  				  struct ethtool_stats *stats, u64 *data)
>  {
> -	struct macb *bp;
> +	struct macb *bp = netdev_priv(dev);
>  
> -	bp = netdev_priv(dev);
> +	spin_lock(&bp->stats_lock);

Sorry if I missed something, but as you're using that lock within the
macb_interrupt(), shouldn't it be a spin_lock_irqsave() for all the
callsites that aren't in irq context ?

You would risk a deadlock otherwise.

Thanks,

Maxime