Message-ID: <20201130193230.f5aopdmcc5x3ldey@skbuf>
Date:   Mon, 30 Nov 2020 21:32:30 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Eric Dumazet <edumazet@...gle.com>
Cc:     Eric Dumazet <eric.dumazet@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Stephen Hemminger <stephen@...workplumber.org>,
        netdev <netdev@...r.kernel.org>,
        Paul Gortmaker <paul.gortmaker@...driver.com>,
        Jiri Benc <jbenc@...hat.com>,
        Or Gerlitz <ogerlitz@...lanox.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>
Subject: Re: Correct usage of dev_base_lock in 2020

On Mon, Nov 30, 2020 at 08:22:01PM +0100, Eric Dumazet wrote:
> On Mon, Nov 30, 2020 at 8:03 PM Vladimir Oltean <olteanv@...il.com> wrote:
> > On Mon, Nov 30, 2020 at 08:00:40PM +0100, Eric Dumazet wrote:
> > > On 11/30/20 7:48 PM, Vladimir Oltean wrote:
> > > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > > > index e0880a3840d7..1d44534e95d2 100644
> > > > --- a/drivers/net/bonding/bond_main.c
> > > > +++ b/drivers/net/bonding/bond_main.c
> > > > @@ -3738,21 +3738,17 @@ static void bond_get_stats(struct net_device *bond_dev,
> > > >                        struct rtnl_link_stats64 *stats)
> > > >  {
> > > >     struct bonding *bond = netdev_priv(bond_dev);
> > > > +   bool rtnl_locked = rtnl_is_locked();
> > > >     struct rtnl_link_stats64 temp;
> > > >     struct list_head *iter;
> > > >     struct slave *slave;
> > > > -   int nest_level = 0;
> > > >
> > > > +   if (!rtnl_locked)
> > > > +           rtnl_lock();
> > >
> > > Gosh, do not do that.
> > >
> > > Convert the bonding ->stats_lock to a mutex instead.
> > >
> > > Adding more reliance to RTNL is not helping cases where
> > > access to stats should not be blocked by other users of RTNL (which can be abused)
> >
> > I can't, Eric. The bond_for_each_slave() macro needs protection against
> > net devices being registered and unregistered.
>
> And ?
>
> A bonding device can absolutely maintain a private list, ready for
> bonding ndo_get_stats() use, regardless
> of register/unregister logic.
>
> bond_for_each_slave() is simply a macro, you can replace it by something else.

Yeah, ok, let's assume I can do that for the particular case of bonding.
What about this one though.

-----------------------------[cut here]-----------------------------
>From 0d114e38ee20d93b41f8e29082e9e5e0fa7f6b0e Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@....com>
Date: Mon, 30 Nov 2020 02:27:28 +0200
Subject: [PATCH] s390/appldata_net_sum: retrieve device statistics under RTNL,
 not RCU

In the effort of making .ndo_get_stats64 be able to sleep, we need to
ensure the callers of dev_get_stats do not use atomic context.

In the case of the appldata driver, an RCU read-side critical section is
used to ensure the integrity of the list of network interfaces, because
the driver iterates through all net devices in the netns to aggregate
statistics. We still need some protection against an interface
registering or deregistering, and the writer-side lock, the RTNL mutex,
is fine for that, because it offers sleepable context.

The ops->callback function is called from under appldata_ops_mutex
protection, so this is proof that the context is sleepable and holding
RTNL is therefore fine.

Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
---
 arch/s390/appldata/appldata_net_sum.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/s390/appldata/appldata_net_sum.c b/arch/s390/appldata/appldata_net_sum.c
index 59c282ca002f..22da28ab10d8 100644
--- a/arch/s390/appldata/appldata_net_sum.c
+++ b/arch/s390/appldata/appldata_net_sum.c
@@ -78,8 +78,9 @@ static void appldata_get_net_sum_data(void *data)
 	tx_dropped = 0;
 	collisions = 0;

-	rcu_read_lock();
-	for_each_netdev_rcu(&init_net, dev) {
+	rtnl_lock();
+
+	for_each_netdev(&init_net, dev) {
 		const struct rtnl_link_stats64 *stats;
 		struct rtnl_link_stats64 temp;

@@ -95,7 +96,8 @@ static void appldata_get_net_sum_data(void *data)
 		collisions += stats->collisions;
 		i++;
 	}
-	rcu_read_unlock();
+
+	rtnl_unlock();

 	net_data->nr_interfaces = i;
 	net_data->rx_packets = rx_packets;
-----------------------------[cut here]-----------------------------

Or this one.

-----------------------------[cut here]-----------------------------
>From 93ffc25f30849aaf89e50e58d32b0b047831f94d Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@....com>
Date: Mon, 30 Nov 2020 02:49:25 +0200
Subject: [PATCH] parisc/led: retrieve device statistics under RTNL, not RCU

In the effort of making .ndo_get_stats64 be able to sleep, we need to
ensure the callers of dev_get_stats do not use atomic context.

The LED driver for HP-PARISC workstations uses a workqueue to
periodically check for updates in network interface statistics, and
flicker when those have changed (i.e. there has been activity on the
line). Honestly that is a strange idea even when protected by RCU, but
now, the dev_get_stats call can sleep, and iterating through the list of
network interfaces still needs to ensure the integrity of list of
network interfaces. So that leaves us only one locking option given the
current design of the network stack, and that is the RTNL mutex. In the
future we might be able to make this a little bit less expensive by
creating a separate mutex for the list of network interfaces.

Cc: "James E.J. Bottomley" <James.Bottomley@...senPartnership.com>
Cc: Helge Deller <deller@....de>
Cc: linux-parisc@...r.kernel.org
Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
---
 drivers/parisc/led.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/parisc/led.c b/drivers/parisc/led.c
index 36c6613f7a36..09dcffaed85f 100644
--- a/drivers/parisc/led.c
+++ b/drivers/parisc/led.c
@@ -38,7 +38,6 @@
 #include <linux/ctype.h>
 #include <linux/blkdev.h>
 #include <linux/workqueue.h>
-#include <linux/rcupdate.h>
 #include <asm/io.h>
 #include <asm/processor.h>
 #include <asm/hardware.h>
@@ -356,12 +355,13 @@ static __inline__ int led_get_net_activity(void)
 
 	rx_total = tx_total = 0;
 	
-	/* we are running as a workqueue task, so we can use an RCU lookup */
-	rcu_read_lock();
-	for_each_netdev_rcu(&init_net, dev) {
+	/* we are running as a workqueue task, so we can take the RTNL mutex */
+	rtnl_lock();
+
+	for_each_netdev(&init_net, dev) {
 	    const struct rtnl_link_stats64 *stats;
 	    struct rtnl_link_stats64 temp;
-	    struct in_device *in_dev = __in_dev_get_rcu(dev);
+	    struct in_device *in_dev = __in_dev_get_rtnl(dev);
 	    if (!in_dev || !in_dev->ifa_list)
 		continue;
 	    if (ipv4_is_loopback(in_dev->ifa_list->ifa_local))
@@ -370,7 +370,8 @@ static __inline__ int led_get_net_activity(void)
 	    rx_total += stats->rx_packets;
 	    tx_total += stats->tx_packets;
 	}
-	rcu_read_unlock();
+
+	rtnl_unlock();
 
 	retval = 0;
 
-----------------------------[cut here]-----------------------------

Where I'm getting at is that we're going to need a new mutex for
write-side protection of the network interface lists, one that is !=
RTNL mutex.

Powered by blists - more mailing lists