netdev - Re: [PATCH] l2tp: cast l2tp traffic counter to unsigned

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: <1497054551.736.99.camel@edumazet-glaptop3.roam.corp.google.com>
Date:   Fri, 09 Jun 2017 17:29:11 -0700
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Stephen Hemminger <stephen@...workplumber.org>
Cc:     Dominik Heidler <dheidler@...e.de>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, Tom Parkin <tparkin@...alix.com>
Subject: Re: [PATCH] l2tp: cast l2tp traffic counter to unsigned

On Fri, 2017-06-09 at 15:16 -0700, Stephen Hemminger wrote:
> On Fri,  9 Jun 2017 16:29:47 +0200
> Dominik Heidler <dheidler@...e.de> wrote:
> 
> > This fixes a counter problem on 32bit systems:
> > When the rx_bytes counter reached 2 GiB, it jumpd to (2^64 Bytes - 2GiB) Bytes.
> > 
> > rtnl_link_stats64 has __u64 type and atomic_long_read returns
> > atomic_long_t which is signed. Due to the conversation
> > we get an incorrect value on 32bit systems if the MSB of
> > the atomic_long_t value is set.
> > 
> > CC: Tom Parkin <tparkin@...alix.com>
> > Fixes: 7b7c0719cd7a ("l2tp: avoid deadlock in l2tp stats update")
> > Signed-off-by: Dominik Heidler <dheidler@...e.de>
> > ---
> >  net/l2tp/l2tp_eth.c | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/net/l2tp/l2tp_eth.c b/net/l2tp/l2tp_eth.c
> > index 8b21af7321b9..668a75e002e9 100644
> > --- a/net/l2tp/l2tp_eth.c
> > +++ b/net/l2tp/l2tp_eth.c
> > @@ -114,12 +114,13 @@ static void l2tp_eth_get_stats64(struct net_device *dev,
> >  {
> >  	struct l2tp_eth *priv = netdev_priv(dev);
> >  
> > -	stats->tx_bytes   = atomic_long_read(&priv->tx_bytes);
> > -	stats->tx_packets = atomic_long_read(&priv->tx_packets);
> > -	stats->tx_dropped = atomic_long_read(&priv->tx_dropped);
> > -	stats->rx_bytes   = atomic_long_read(&priv->rx_bytes);
> > -	stats->rx_packets = atomic_long_read(&priv->rx_packets);
> > -	stats->rx_errors  = atomic_long_read(&priv->rx_errors);
> > +	stats->tx_bytes   = (unsigned long) atomic_long_read(&priv->tx_bytes);
> > +	stats->tx_packets = (unsigned long) atomic_long_read(&priv->tx_packets);
> > +	stats->tx_dropped = (unsigned long) atomic_long_read(&priv->tx_dropped);
> > +	stats->rx_bytes   = (unsigned long) atomic_long_read(&priv->rx_bytes);
> > +	stats->rx_packets = (unsigned long) atomic_long_read(&priv->rx_packets);
> > +	stats->rx_errors  = (unsigned long) atomic_long_read(&priv->rx_errors);
> > +
> >  }
> >  
> >  static const struct net_device_ops l2tp_eth_netdev_ops = {
> 
> This is not the right way to fix this.
> 
> 1. shouldn't be using atomic's for network counters, look at other network devices.
> 
> 2. should be using u64_stats_fetch  api to handle 64 bit counters.

But they do not want 64bit counters, and not per cpu counters for a
driver handling few packets per second.

Just use native size of "unsigned long".

We use the same atomic_long_t for (struct netdev)->rx_dropped,
tx_dropped & rx_nohandler

So I guess same fix is needed in dev_get_stats()

diff --git a/net/core/dev.c b/net/core/dev.c
index 54bb8d99d26afcc1a9c5a56f1e8c2d1f6e06db98..1a66a8761f9a579c9bf6b6ab5b1415770adcf76b 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -7785,9 +7785,9 @@ struct rtnl_link_stats64 *dev_get_stats(struct net_device *dev,
 	} else {
 		netdev_stats_to_stats64(storage, &dev->stats);
 	}
-	storage->rx_dropped += atomic_long_read(&dev->rx_dropped);
-	storage->tx_dropped += atomic_long_read(&dev->tx_dropped);
-	storage->rx_nohandler += atomic_long_read(&dev->rx_nohandler);
+	storage->rx_dropped += (unsigned long)atomic_long_read(&dev->rx_dropped);
+	storage->tx_dropped += (unsigned long)atomic_long_read(&dev->tx_dropped);
+	storage->rx_nohandler += (unsigned long)atomic_long_read(&dev->rx_nohandler);
 	return storage;
 }
 EXPORT_SYMBOL(dev_get_stats);