linux-kernel - Re: [EXTERNAL] Re: [PATCH net v2 1/4] octeon_ep: fix race conditions in ndo_get

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: <CANn89iJXNYRNn7N9AHKr0jECxn0Lh6_CtKG7kk9xjqhbVjjkjQ@mail.gmail.com>
Date: Wed, 18 Dec 2024 15:21:12 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Larysa Zaremba <larysa.zaremba@...el.com>
Cc: Shinas Rasheed <srasheed@...vell.com>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Haseeb Gani <hgani@...vell.com>, 
	Sathesh B Edara <sedara@...vell.com>, Vimlesh Kumar <vimleshk@...vell.com>, 
	"thaller@...hat.com" <thaller@...hat.com>, "wizhao@...hat.com" <wizhao@...hat.com>, 
	"kheib@...hat.com" <kheib@...hat.com>, "konguyen@...hat.com" <konguyen@...hat.com>, 
	"horms@...nel.org" <horms@...nel.org>, "einstein.xue@...axg.com" <einstein.xue@...axg.com>, 
	Veerasenareddy Burru <vburru@...vell.com>, Andrew Lunn <andrew+netdev@...n.ch>, 
	"David S. Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Abhijit Ayarekar <aayarekar@...vell.com>, Satananda Burla <sburla@...vell.com>
Subject: Re: [EXTERNAL] Re: [PATCH net v2 1/4] octeon_ep: fix race conditions
 in ndo_get_stats64

On Wed, Dec 18, 2024 at 2:25 PM Larysa Zaremba <larysa.zaremba@...el.com> wrote:

>
> It is hard to know without testing (but testing should not be hard). I think the
> phrase "Statistics must persist across routine operations like bringing the
> interface down and up." [0] implies that bringing the interface down may not
> necessarily prevent stats calls.

Please don't  add workarounds to individual drivers.

I think the core networking stack should handle the possible races.

Most dev_get_stats() callers are correctly testing dev_isalive() or
are protected by RTNL.

There are few nested cases that are not properly handled, the
following patch should take care of them.


diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 2593019ad5b1614f3b8c037afb4ba4fa740c7d51..768afc2a18d343d051e7a1b631124910af9922d2
100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -5342,6 +5342,12 @@ static inline const char
*netdev_reg_state(const struct net_device *dev)
        return " (unknown)";
 }

+/* Caller holds RTNL or RCU */
+static inline int dev_isalive(const struct net_device *dev)
+{
+       return READ_ONCE(dev->reg_state) <= NETREG_REGISTERED;
+}
+
 #define MODULE_ALIAS_NETDEV(device) \
        MODULE_ALIAS("netdev-" device)

diff --git a/net/core/dev.c b/net/core/dev.c
index c7f3dea3e0eb9eb05865e49dd7a8535afb974149..f11f305f3136f208fcb285c7b314914aef20dfad
100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -11044,8 +11044,13 @@ struct rtnl_link_stats64
*dev_get_stats(struct net_device *dev,
        const struct net_device_ops *ops = dev->netdev_ops;
        const struct net_device_core_stats __percpu *p;

+       memset(storage, 0, sizeof(*storage));
+       rcu_read_lock();
+
+       if (unlikely(!dev_isalive(dev)))
+               goto unlock;
+
        if (ops->ndo_get_stats64) {
-               memset(storage, 0, sizeof(*storage));
                ops->ndo_get_stats64(dev, storage);
        } else if (ops->ndo_get_stats) {
                netdev_stats_to_stats64(storage, ops->ndo_get_stats(dev));
@@ -11071,6 +11076,8 @@ struct rtnl_link_stats64 *dev_get_stats(struct
net_device *dev,
                        storage->rx_otherhost_dropped +=
READ_ONCE(core_stats->rx_otherhost_dropped);
                }
        }
+unlock:
+       rcu_read_unlock();
        return storage;
 }
 EXPORT_SYMBOL(dev_get_stats);
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 2d9afc6e2161efa51ffa62813ec10c8f43944bce..3f4851d67015c959dd531c571c46fc2ac18beb65
100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -36,12 +36,6 @@ static const char fmt_uint[] = "%u\n";
 static const char fmt_ulong[] = "%lu\n";
 static const char fmt_u64[] = "%llu\n";

-/* Caller holds RTNL or RCU */
-static inline int dev_isalive(const struct net_device *dev)
-{
-       return READ_ONCE(dev->reg_state) <= NETREG_REGISTERED;
-}
-
 /* use same locking rules as GIF* ioctl's */
 static ssize_t netdev_show(const struct device *dev,
                           struct device_attribute *attr, char *buf,