On Fri, 2017-06-09 at 15:16 -0700, Stephen Hemminger wrote:
> On Fri, 9 Jun 2017 16:29:47 +0200
> Dominik Heidler <dheid...@suse.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 <tpar...@katalix.com>
> > Fixes: 7b7c0719cd7a ("l2tp: avoid deadlock in l2tp stats update")
> > Signed-off-by: Dominik Heidler <dheid...@suse.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);
