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]
Date:	Mon, 05 Jul 2010 20:16:20 +0200
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Ben Hutchings <bhutchings@...arflare.com>
Cc:	David Miller <davem@...emloft.net>,
	Stephen Hemminger <shemminger@...tta.com>,
	Arnd Bergmann <arnd@...db.de>, netdev@...r.kernel.org,
	linux-net-drivers@...arflare.com
Subject: Re: [PATCHv3 2/2] sfc: Implement 64-bit net device statistics on
 all architectures

Le mardi 08 juin 2010 à 18:21 +0100, Ben Hutchings a écrit :
> Signed-off-by: Ben Hutchings <bhutchings@...arflare.com>
> ---
> This is unchanged from v1.
> 
> Ben.
> 
>  drivers/net/sfc/efx.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/sfc/efx.c b/drivers/net/sfc/efx.c
> index 26b0cc2..8ad476a 100644
> --- a/drivers/net/sfc/efx.c
> +++ b/drivers/net/sfc/efx.c
> @@ -1492,11 +1492,11 @@ static int efx_net_stop(struct net_device *net_dev)
>  }
>  
>  /* Context: process, dev_base_lock or RTNL held, non-blocking. */
> -static struct net_device_stats *efx_net_stats(struct net_device *net_dev)
> +static struct rtnl_link_stats64 *efx_net_stats(struct net_device *net_dev)
>  {
>  	struct efx_nic *efx = netdev_priv(net_dev);
>  	struct efx_mac_stats *mac_stats = &efx->mac_stats;
> -	struct net_device_stats *stats = &net_dev->stats;
> +	struct rtnl_link_stats64 *stats = &net_dev->stats64;
>  
>  	spin_lock_bh(&efx->stats_lock);
>  	efx->type->update_stats(efx);
> @@ -1630,7 +1630,7 @@ static void efx_set_multicast_list(struct net_device *net_dev)
>  static const struct net_device_ops efx_netdev_ops = {
>  	.ndo_open		= efx_net_open,
>  	.ndo_stop		= efx_net_stop,
> -	.ndo_get_stats		= efx_net_stats,
> +	.ndo_get_stats64	= efx_net_stats,
>  	.ndo_tx_timeout		= efx_watchdog,
>  	.ndo_start_xmit		= efx_hard_start_xmit,
>  	.ndo_validate_addr	= eth_validate_addr,
> -- 
> 1.6.2.5
> 

Ben, David

I believe following patch is needed after our recent commits.
Not sure a seqlock is really needed, maybe a spinlock would be enough.

Thanks

[PATCH net-next-2.6] net: fix 64 bit counters on 32 bit arches

There is a small possibility that a reader gets incorrect values on 32
bit arches. SNMP applications could catch incorrect counters when a
32bit high part is changed.

I believe we need to add some synchronisation to avoid this.

A driver that provides 64bit stats on a 32bit arches should make sure it
gets the seqlock before updating them. Other drivers can stay unchanged,
since upper 32bits are not updated.

This new seqlock should be acquired in slow path only in process context
(no BH protection for instance).

Drivers should not use it in their rx/tx path. (They better stay with
32bit counters on 32bit arches)

Signed-off-by: Eric Dumazet <eric.dumazet@...il.com>
---
 drivers/net/loopback.c    |    4 ++
 drivers/net/macvlan.c     |    2 +
 drivers/net/sfc/efx.c     |    2 +
 include/linux/netdevice.h |   43 +++++++++++++++++++++++
 net/8021q/vlan_dev.c      |    2 +
 net/core/dev.c            |   38 ++++++++++++--------
 net/core/rtnetlink.c      |   66 +++++++++++++++++++-----------------
 7 files changed, 112 insertions(+), 45 deletions(-)

diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
index 4dd0510..5e82e4a 100644
--- a/drivers/net/loopback.c
+++ b/drivers/net/loopback.c
@@ -123,12 +123,16 @@ static struct rtnl_link_stats64 *loopback_get_stats64(struct net_device *dev)
 		bytes   += tbytes;
 		packets += tpackets;
 	}
+
+	netdev_stats_update_begin(dev);
 	stats->rx_packets = packets;
 	stats->tx_packets = packets;
 	stats->rx_dropped = drops;
 	stats->rx_errors  = drops;
 	stats->rx_bytes   = bytes;
 	stats->tx_bytes   = bytes;
+	netdev_stats_update_end(dev);
+
 	return stats;
 }
 
diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index e6d626e..c39a90a 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -458,11 +458,13 @@ static struct rtnl_link_stats64 *macvlan_dev_get_stats64(struct net_device *dev)
 			/* rx_errors is an ulong, updated without syncp protection */
 			accum.rx_errors		+= p->rx_errors;
 		}
+		netdev_stats_update_begin(dev);
 		stats->rx_packets = accum.rx_packets;
 		stats->rx_bytes   = accum.rx_bytes;
 		stats->rx_errors  = accum.rx_errors;
 		stats->rx_dropped = accum.rx_errors;
 		stats->multicast  = accum.rx_multicast;
+		netdev_stats_update_end(dev);
 	}
 	return stats;
 }
diff --git a/drivers/net/sfc/efx.c b/drivers/net/sfc/efx.c
index 35b3f29..06afff6 100644
--- a/drivers/net/sfc/efx.c
+++ b/drivers/net/sfc/efx.c
@@ -1543,6 +1543,7 @@ static struct rtnl_link_stats64 *efx_net_stats(struct net_device *net_dev)
 	efx->type->update_stats(efx);
 	spin_unlock_bh(&efx->stats_lock);
 
+	netdev_stats_update_begin(net_dev);
 	stats->rx_packets = mac_stats->rx_packets;
 	stats->tx_packets = mac_stats->tx_packets;
 	stats->rx_bytes = mac_stats->rx_bytes;
@@ -1565,6 +1566,7 @@ static struct rtnl_link_stats64 *efx_net_stats(struct net_device *net_dev)
 	stats->tx_errors = (stats->tx_window_errors +
 			    mac_stats->tx_bad);
 
+	netdev_stats_update_end(net_dev);
 	return stats;
 }
 
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 4d27368..d6726cf 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -886,6 +886,9 @@ struct net_device {
 	int			ifindex;
 	int			iflink;
 
+#if BITS_PER_LONG==32
+	seqlock_t		stats_lock;
+#endif
 	union {
 		struct rtnl_link_stats64 stats64;
 		struct net_device_stats stats;
@@ -1080,6 +1083,46 @@ struct net_device {
 };
 #define to_net_dev(d) container_of(d, struct net_device, dev)
 
+
+static inline void netdev_stats_initlock(struct net_device *dev)
+{
+#if BITS_PER_LONG==32
+	seqlock_init(&dev->stats_lock);
+#endif
+}
+
+static inline void netdev_stats_update_begin(struct net_device *dev)
+{
+#if BITS_PER_LONG==32
+	write_seqlock(&dev->stats_lock);
+#endif
+}
+
+static inline void netdev_stats_update_end(struct net_device *dev)
+{
+#if BITS_PER_LONG==32
+	write_sequnlock(&dev->stats_lock);
+#endif
+}
+
+static inline unsigned int netdev_stats_fetch_begin(const struct net_device *dev)
+{
+#if BITS_PER_LONG==32
+	return read_seqbegin(&dev->stats_lock);
+#else
+	return 0;
+#endif
+}
+
+static bool inline netdev_stats_fetch_retry(const struct net_device *dev, unsigned int start)
+{
+#if BITS_PER_LONG==32
+	return read_seqretry(&dev->stats_lock, start);
+#else
+	return false;
+#endif
+}
+
 #define	NETDEV_ALIGN		32
 
 static inline
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index c6456cb..3d4e2f0 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -830,10 +830,12 @@ static struct rtnl_link_stats64 *vlan_dev_get_stats64(struct net_device *dev)
 			/* rx_errors is an ulong, not protected by syncp */
 			accum.rx_errors  += p->rx_errors;
 		}
+		netdev_stats_update_begin(dev);
 		stats->rx_packets = accum.rx_packets;
 		stats->rx_bytes   = accum.rx_bytes;
 		stats->rx_errors  = accum.rx_errors;
 		stats->multicast  = accum.rx_multicast;
+		netdev_stats_update_end(dev);
 	}
 	return stats;
 }
diff --git a/net/core/dev.c b/net/core/dev.c
index 93b8929..b41c7ac 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3704,24 +3704,31 @@ void dev_seq_stop(struct seq_file *seq, void *v)
 static void dev_seq_printf_stats(struct seq_file *seq, struct net_device *dev)
 {
 	const struct rtnl_link_stats64 *stats = dev_get_stats(dev);
+	struct rtnl_link_stats64 temp;
+	unsigned int start;
+	
+	do {
+		start = netdev_stats_fetch_begin(dev);
+		memcpy(&temp, stats, sizeof(temp));
+	} while (netdev_stats_fetch_retry(dev, start));
 
 	seq_printf(seq, "%6s: %7llu %7llu %4llu %4llu %4llu %5llu %10llu %9llu "
 		   "%8llu %7llu %4llu %4llu %4llu %5llu %7llu %10llu\n",
-		   dev->name, stats->rx_bytes, stats->rx_packets,
-		   stats->rx_errors,
-		   stats->rx_dropped + stats->rx_missed_errors,
-		   stats->rx_fifo_errors,
-		   stats->rx_length_errors + stats->rx_over_errors +
-		    stats->rx_crc_errors + stats->rx_frame_errors,
-		   stats->rx_compressed, stats->multicast,
-		   stats->tx_bytes, stats->tx_packets,
-		   stats->tx_errors, stats->tx_dropped,
-		   stats->tx_fifo_errors, stats->collisions,
-		   stats->tx_carrier_errors +
-		    stats->tx_aborted_errors +
-		    stats->tx_window_errors +
-		    stats->tx_heartbeat_errors,
-		   stats->tx_compressed);
+		   dev->name, temp.rx_bytes, temp.rx_packets,
+		   temp.rx_errors,
+		   temp.rx_dropped + temp.rx_missed_errors,
+		   temp.rx_fifo_errors,
+		   temp.rx_length_errors + temp.rx_over_errors +
+		    temp.rx_crc_errors + temp.rx_frame_errors,
+		   temp.rx_compressed, temp.multicast,
+		   temp.tx_bytes, temp.tx_packets,
+		   temp.tx_errors, temp.tx_dropped,
+		   temp.tx_fifo_errors, temp.collisions,
+		   temp.tx_carrier_errors +
+		    temp.tx_aborted_errors +
+		    temp.tx_window_errors +
+		    temp.tx_heartbeat_errors,
+		   temp.tx_compressed);
 }
 
 /*
@@ -5386,6 +5393,7 @@ struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name,
 	if (dev_addr_init(dev))
 		goto free_rx;
 
+	netdev_stats_initlock(dev);
 	dev_mc_init(dev);
 	dev_uc_init(dev);
 
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index e645778..b3a3812 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -610,37 +610,43 @@ static void copy_rtnl_link_stats(struct rtnl_link_stats *a,
 	a->tx_compressed = b->tx_compressed;
 }
 
-static void copy_rtnl_link_stats64(void *v, const struct rtnl_link_stats64 *b)
+static void copy_rtnl_link_stats64(void *v, const struct rtnl_link_stats64 *b,
+				   struct net_device *dev)
 {
 	struct rtnl_link_stats64 a;
-
-	a.rx_packets = b->rx_packets;
-	a.tx_packets = b->tx_packets;
-	a.rx_bytes = b->rx_bytes;
-	a.tx_bytes = b->tx_bytes;
-	a.rx_errors = b->rx_errors;
-	a.tx_errors = b->tx_errors;
-	a.rx_dropped = b->rx_dropped;
-	a.tx_dropped = b->tx_dropped;
-
-	a.multicast = b->multicast;
-	a.collisions = b->collisions;
-
-	a.rx_length_errors = b->rx_length_errors;
-	a.rx_over_errors = b->rx_over_errors;
-	a.rx_crc_errors = b->rx_crc_errors;
-	a.rx_frame_errors = b->rx_frame_errors;
-	a.rx_fifo_errors = b->rx_fifo_errors;
-	a.rx_missed_errors = b->rx_missed_errors;
-
-	a.tx_aborted_errors = b->tx_aborted_errors;
-	a.tx_carrier_errors = b->tx_carrier_errors;
-	a.tx_fifo_errors = b->tx_fifo_errors;
-	a.tx_heartbeat_errors = b->tx_heartbeat_errors;
-	a.tx_window_errors = b->tx_window_errors;
-
-	a.rx_compressed = b->rx_compressed;
-	a.tx_compressed = b->tx_compressed;
+	unsigned int start;
+
+	do {
+		start = netdev_stats_fetch_begin(dev);
+
+		a.rx_packets = b->rx_packets;
+		a.tx_packets = b->tx_packets;
+		a.rx_bytes = b->rx_bytes;
+		a.tx_bytes = b->tx_bytes;
+		a.rx_errors = b->rx_errors;
+		a.tx_errors = b->tx_errors;
+		a.rx_dropped = b->rx_dropped;
+		a.tx_dropped = b->tx_dropped;
+	
+		a.multicast = b->multicast;
+		a.collisions = b->collisions;
+
+		a.rx_length_errors = b->rx_length_errors;
+		a.rx_over_errors = b->rx_over_errors;
+		a.rx_crc_errors = b->rx_crc_errors;
+		a.rx_frame_errors = b->rx_frame_errors;
+		a.rx_fifo_errors = b->rx_fifo_errors;
+		a.rx_missed_errors = b->rx_missed_errors;
+
+		a.tx_aborted_errors = b->tx_aborted_errors;
+		a.tx_carrier_errors = b->tx_carrier_errors;
+		a.tx_fifo_errors = b->tx_fifo_errors;
+		a.tx_heartbeat_errors = b->tx_heartbeat_errors;
+		a.tx_window_errors = b->tx_window_errors;
+
+		a.rx_compressed = b->rx_compressed;
+		a.tx_compressed = b->tx_compressed;
+	} while (netdev_stats_fetch_retry(dev, start));
 	memcpy(v, &a, sizeof(a));
 }
 
@@ -854,7 +860,7 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 			sizeof(struct rtnl_link_stats64));
 	if (attr == NULL)
 		goto nla_put_failure;
-	copy_rtnl_link_stats64(nla_data(attr), stats);
+	copy_rtnl_link_stats64(nla_data(attr), stats, dev);
 
 	if (dev->dev.parent)
 		NLA_PUT_U32(skb, IFLA_NUM_VF, dev_num_vf(dev->dev.parent));


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ