[PATCH net] net: Fix inconsistent rtnl_lock usage on dev_get_stats().

Some callers take rtnl_lock() before calling dev_get_stats() and some
don't.  Most network drivers expect the ndo_get_stats64() to be called
under rtnl_lock() to avoid race conditions with device close or ethtool
reconfigurations.  Fix it so that all callers take rtnl_lock().

Rename the original dev_get_stats() as __dev_get_stats() and add a new
dev_get_stats() that takes rtnl_lock() before calling __dev_get_stats().
Modify all callers that already take rtnl_lock() to call __dev_get_stats().

Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/bonding/bond_main.c                      |  4 ++--
 drivers/net/ethernet/hisilicon/hns/hns_ethtool.c     |  2 +-
 drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c     |  2 +-
 drivers/net/ethernet/intel/ixgbevf/ethtool.c         |  2 +-
 drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c |  2 +-
 include/linux/netdevice.h                            |  2 ++
 net/core/dev.c                                       | 19 ++++++++++++++++---
 net/core/rtnetlink.c                                 |  4 ++--
 8 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 8029dd4..9a2fbea 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c

@@ -1509,7 +1509,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 
 	slave_dev->priv_flags |= IFF_BONDING;
 	/* initialize slave stats */
-	dev_get_stats(new_slave->dev, &new_slave->slave_stats);
+	__dev_get_stats(new_slave->dev, &new_slave->slave_stats);
 
 	if (bond_is_lb(bond)) {
 		/* bond_alb_init_slave() must be called before all other stages since

@@ -3351,7 +3351,7 @@ static struct rtnl_link_stats64 *bond_get_stats(struct net_device *bond_dev,
 	rcu_read_lock();
 	bond_for_each_slave_rcu(bond, slave, iter) {
 		const struct rtnl_link_stats64 *new =
-			dev_get_stats(slave->dev, &temp);
+			__dev_get_stats(slave->dev, &temp);
 
 		bond_fold_stats(stats, new, &slave->slave_stats);
 

diff --git a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
index 3ac2183..8396336 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c

@@ -865,7 +865,7 @@ void hns_get_ethtool_stats(struct net_device *netdev,
 
 	h->dev->ops->update_stats(h, &netdev->stats);
 
-	net_stats = dev_get_stats(netdev, &temp);
+	net_stats = __dev_get_stats(netdev, &temp);
 
 	/* get netdev statistics */
 	p[0] = net_stats->rx_packets;

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
index fd192bf..f8097c4 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c

@@ -1145,7 +1145,7 @@ static void ixgbe_get_ethtool_stats(struct net_device *netdev,
 	char *p = NULL;
 
 	ixgbe_update_stats(adapter);
-	net_stats = dev_get_stats(netdev, &temp);
+	net_stats = __dev_get_stats(netdev, &temp);
 	for (i = 0; i < IXGBE_GLOBAL_STATS_LEN; i++) {
 		switch (ixgbe_gstrings_stats[i].type) {
 		case NETDEV_STATS:

diff --git a/drivers/net/ethernet/intel/ixgbevf/ethtool.c b/drivers/net/ethernet/intel/ixgbevf/ethtool.c
index 508e72c..622ccad 100644
--- a/drivers/net/ethernet/intel/ixgbevf/ethtool.c
+++ b/drivers/net/ethernet/intel/ixgbevf/ethtool.c

@@ -406,7 +406,7 @@ static void ixgbevf_get_ethtool_stats(struct net_device *netdev,
 	char *p;
 
 	ixgbevf_update_stats(adapter);
-	net_stats = dev_get_stats(netdev, &temp);
+	net_stats = __dev_get_stats(netdev, &temp);
 	for (i = 0; i < IXGBEVF_GLOBAL_STATS_LEN; i++) {
 		switch (ixgbevf_gstrings_stats[i].type) {
 		case NETDEV_STATS:

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
index 1b26e96..ea77de0 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c

@@ -270,7 +270,7 @@ static void nfp_net_get_stats(struct net_device *netdev,
 	int i, j, k;
 	u8 *p;
 
-	netdev_stats = dev_get_stats(netdev, &temp);
+	netdev_stats = __dev_get_stats(netdev, &temp);
 
 	for (i = 0; i < NN_ET_GLOBAL_STATS_LEN; i++) {
 		switch (nfp_net_et_stats[i].type) {

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 994f742..76bc92f 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h

@@ -3787,6 +3787,8 @@ static inline void __dev_mc_unsync(struct net_device *dev,
 void netdev_features_change(struct net_device *dev);
 /* Load a device via the kmod */
 void dev_load(struct net *net, const char *name);
+struct rtnl_link_stats64 *__dev_get_stats(struct net_device *dev,
+					  struct rtnl_link_stats64 *storage);
 struct rtnl_link_stats64 *dev_get_stats(struct net_device *dev,
 					struct rtnl_link_stats64 *storage);
 void netdev_stats_to_stats64(struct rtnl_link_stats64 *stats64,

diff --git a/net/core/dev.c b/net/core/dev.c
index 8db5a0b..ecd1eee 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -7578,7 +7578,7 @@ void netdev_stats_to_stats64(struct rtnl_link_stats64 *stats64,
 EXPORT_SYMBOL(netdev_stats_to_stats64);
 
 /**
- *	dev_get_stats	- get network device statistics
+ *	__dev_get_stats	- get network device statistics
  *	@dev: device to get statistics from
  *	@storage: place to store stats
  *

@@ -7587,11 +7587,12 @@ void netdev_stats_to_stats64(struct rtnl_link_stats64 *stats64,
  *	dev->netdev_ops->get_stats64 or dev->netdev_ops->get_stats;
  *	otherwise the internal statistics structure is used.
  */
-struct rtnl_link_stats64 *dev_get_stats(struct net_device *dev,
-					struct rtnl_link_stats64 *storage)
+struct rtnl_link_stats64 *__dev_get_stats(struct net_device *dev,
+					  struct rtnl_link_stats64 *storage)
 {
 	const struct net_device_ops *ops = dev->netdev_ops;
 
+	ASSERT_RTNL();
 	if (ops->ndo_get_stats64) {
 		memset(storage, 0, sizeof(*storage));
 		ops->ndo_get_stats64(dev, storage);

@@ -7605,6 +7606,18 @@ struct rtnl_link_stats64 *dev_get_stats(struct net_device *dev,
 	storage->rx_nohandler += atomic_long_read(&dev->rx_nohandler);
 	return storage;
 }
+EXPORT_SYMBOL(__dev_get_stats);
+
+struct rtnl_link_stats64 *dev_get_stats(struct net_device *dev,
+					struct rtnl_link_stats64 *storage)
+{
+	struct rtnl_link_stats64 *stats;
+
+	rtnl_lock();
+	stats = __dev_get_stats(dev, storage);
+	rtnl_unlock();
+	return stats;
+}
 EXPORT_SYMBOL(dev_get_stats);
 
 struct netdev_queue *dev_ingress_queue_create(struct net_device *dev)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 75e3ea7..37c9ccd 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c

@@ -1100,7 +1100,7 @@ static noinline_for_stack int rtnl_fill_stats(struct sk_buff *skb,
 		return -EMSGSIZE;
 
 	sp = nla_data(attr);
-	dev_get_stats(dev, sp);
+	__dev_get_stats(dev, sp);
 
 	attr = nla_reserve(skb, IFLA_STATS,
 			   sizeof(struct rtnl_link_stats));

@@ -3767,7 +3767,7 @@ static int rtnl_fill_statsinfo(struct sk_buff *skb, struct net_device *dev,
 			goto nla_put_failure;
 
 		sp = nla_data(attr);
-		dev_get_stats(dev, sp);
+		__dev_get_stats(dev, sp);
 	}
 
 	if (stats_attr_valid(filter_mask, IFLA_STATS_LINK_XSTATS, *idxattr)) {

-- 
1.8.3.1

FYI, we noticed the following commit:

commit: bf7d95337899f2e3ab5c14d6117fa7cd95a3d0d4 ("net: Fix inconsistent rtnl_lock usage on dev_get_stats().")
url: https://github.com/0day-ci/linux/commits/Michael-Chan/net-Fix-inconsistent-rtnl_lock-usage-on-dev_get_stats/20170106-144936


in testcase: boot

on test machine: qemu-system-x86_64 -enable-kvm -m 320M

caused below changes:


+-----------------------------------------------------------------------------+------------+------------+
|                                                                             | 71eae1ca77 | bf7d953378 |
+-----------------------------------------------------------------------------+------------+------------+
| boot_successes                                                              | 8          | 2          |
| boot_failures                                                               | 0          | 7          |
| BUG:sleeping_function_called_from_invalid_context_at_kernel/locking/mutex.c | 0          | 7          |
| BUG:scheduling_while_atomic                                                 | 0          | 7          |
| WARNING:at_arch/x86/lib/usercopy.c:#_copy_to_user                           | 0          | 7          |
+-----------------------------------------------------------------------------+------------+------------+



[    8.959014] random: uci: uninitialized urandom read (6 bytes read)
[    8.991559] random: uci: uninitialized urandom read (6 bytes read)
[    9.004266] random: uci: uninitialized urandom read (6 bytes read)
[    9.379490] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:232
[    9.383499] in_atomic(): 1, irqs_disabled(): 0, pid: 1816, name: dropbearkey
[    9.386025] CPU: 0 PID: 1816 Comm: dropbearkey Not tainted 4.10.0-rc1-00075-gbf7d953 #5
[    9.389553] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.9.3-20161025_171302-gandalf 04/01/2014
[    9.393545] Call Trace:
[    9.395029]  dump_stack+0x86/0xc0
[    9.396698]  ___might_sleep+0x11a/0x129
[    9.398482]  __might_sleep+0x77/0x7e


To reproduce:

        git clone git://git.kernel.org/pub/scm/linux/kernel/git/wfg/lkp-tests.git
        cd lkp-tests
        bin/lkp qemu -k <bzImage> job-script  # job-script is attached in this email



Thanks,
Kernel Test Robot

On Thu, 2017-01-05 at 23:21 -0500, Michael Chan wrote:

Some callers take rtnl_lock() before calling dev_get_stats() and some
don't.  Most network drivers expect the ndo_get_stats64() to be called
under rtnl_lock() to avoid race conditions with device close or ethtool
reconfigurations.  Fix it so that all callers take rtnl_lock().

Rename the original dev_get_stats() as __dev_get_stats() and add a new
dev_get_stats() that takes rtnl_lock() before calling __dev_get_stats().
Modify all callers that already take rtnl_lock() to call __dev_get_stats().

This makes no sense to me.

RTNL is absolutely not needed to get device stats.

We try to not add RTNL, especially when not required.

Sure, RTNETLINK dumps currently hold RTNL, but we had various attempts
in the past to get rid of this behavior.

If a device driver expects RTNL being locked, it is clearly a bug that
needs a fix anyway.

From: Eric Dumazet <redacted>
Date: Fri, 06 Jan 2017 09:32:56 -0800

This makes no sense to me.

RTNL is absolutely not needed to get device stats.

We try to not add RTNL, especially when not required.

Sure, RTNETLINK dumps currently hold RTNL, but we had various attempts
in the past to get rid of this behavior.

If a device driver expects RTNL being locked, it is clearly a bug that
needs a fix anyway.

This is extremely problematic when the driver has to synchronize some
piece of state between the get stats method and open/close.  It is
exactly the case we are trying to solve in tg3, and lots of drivers
end up hitting the same exact issue.

If open/close can happen asynchronously to get stats, it is very hard
to make dynamically allocated data structures or DMA buffers usable
from the stats call.

Drivers in this situation will just add a mutex specifically for this
situation if we don't consistently apply RTNL locking here.

On Fri, Jan 6, 2017 at 10:01 AM, David Miller [off-list ref] wrote:

From: Eric Dumazet <redacted>
Date: Fri, 06 Jan 2017 09:32:56 -0800

quoted

This makes no sense to me.

RTNL is absolutely not needed to get device stats.

We try to not add RTNL, especially when not required.

Sure, RTNETLINK dumps currently hold RTNL, but we had various attempts
in the past to get rid of this behavior.

If a device driver expects RTNL being locked, it is clearly a bug that
needs a fix anyway.

This is extremely problematic when the driver has to synchronize some
piece of state between the get stats method and open/close.  It is
exactly the case we are trying to solve in tg3, and lots of drivers
end up hitting the same exact issue.

If open/close can happen asynchronously to get stats, it is very hard
to make dynamically allocated data structures or DMA buffers usable
from the stats call.

Drivers in this situation will just add a mutex specifically for this
situation if we don't consistently apply RTNL locking here.

The patch doesn't work anyway in the net-procfs code path upon closer
examination.  Because we are using seq_ops and taking the RCU lock at
the beginning of the sequence, we cannot take RTNL.  That means
drivers cannot use mutex as well.

For tg3, I think I will just use tp->lock spinlock to protect
get_stats64 and the freeing of the stats memory.

On Fri, 2017-01-06 at 13:01 -0500, David Miller wrote:

From: Eric Dumazet <redacted>
Date: Fri, 06 Jan 2017 09:32:56 -0800

quoted

This makes no sense to me.

RTNL is absolutely not needed to get device stats.

We try to not add RTNL, especially when not required.

Sure, RTNETLINK dumps currently hold RTNL, but we had various attempts
in the past to get rid of this behavior.

If a device driver expects RTNL being locked, it is clearly a bug that
needs a fix anyway.

This is extremely problematic when the driver has to synchronize some
piece of state between the get stats method and open/close.  It is
exactly the case we are trying to solve in tg3, and lots of drivers
end up hitting the same exact issue.

If open/close can happen asynchronously to get stats, it is very hard
to make dynamically allocated data structures or DMA buffers usable
from the stats call.

Yes, I had some issues lately with mlx4. netdevices are protected by
RCU, adding proper RCU logic for the stats is doable.

Drivers in this situation will just add a mutex specifically for this
situation if we don't consistently apply RTNL locking here.

Well, there are cases where RTNL is quite contended, but supervisions
like to get /proc/net/devices or various sysfs attributes
(netstat_show() can be called very very often
for /sys/class/net/*/statistics/*) in a reasonable amount of time.


I fear that such a change will add drifts, when devices are constantly
added/removed.

From: Eric Dumazet <redacted>
Date: Fri, 06 Jan 2017 12:13:23 -0800

Well, there are cases where RTNL is quite contended, but supervisions
like to get /proc/net/devices or various sysfs attributes
(netstat_show() can be called very very often
for /sys/class/net/*/statistics/*) in a reasonable amount of time.

I fear that such a change will add drifts, when devices are constantly
added/removed.

It stands to reason that RTNETLINK could use pure RCU locking for
device and stat dumps, and frankly that would make their usage and
overhead superior to poking files over and over.