net-sysfs: remove rtnl_trylock from device attributes
authorAntoine Tenart <atenart@kernel.org>
Tue, 4 Feb 2025 17:03:10 +0000 (18:03 +0100)
committerJakub Kicinski <kuba@kernel.org>
Thu, 6 Feb 2025 01:49:07 +0000 (17:49 -0800)
There is an ABBA deadlock between net device unregistration and sysfs
files being accessed[1][2]. To prevent this from happening all paths
taking the rtnl lock after the sysfs one (actually kn->active refcount)
use rtnl_trylock and return early (using restart_syscall)[3], which can
make syscalls to spin for a long time when there is contention on the
rtnl lock[4].

There are not many possibilities to improve the above:
- Rework the entire net/ locking logic.
- Invert two locks in one of the paths — not possible.

But here it's actually possible to drop one of the locks safely: the
kernfs_node refcount. More details in the code itself, which comes with
lots of comments.

Note that we check the device is alive in the added sysfs_rtnl_lock
helper to disallow sysfs operations to run after device dismantle has
started. This also help keeping the same behavior as before. Because of
this calls to dev_isalive in sysfs ops were removed.

[1] https://lore.kernel.org/netdev/49A4D5D5.5090602@trash.net/
[2] https://lore.kernel.org/netdev/m14oyhis31.fsf@fess.ebiederm.org/
[3] https://lore.kernel.org/netdev/20090226084924.16cb3e08@nehalam/
[4] https://lore.kernel.org/all/20210928125500.167943-1-atenart@kernel.org/T/

Signed-off-by: Antoine Tenart <atenart@kernel.org>
Link: https://patch.msgid.link/20250204170314.146022-2-atenart@kernel.org
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
include/linux/rtnetlink.h
net/core/net-sysfs.c
net/core/rtnetlink.c

index 4bc2ee0b10b053bf255c85bb0adc6f5a1502a040..ccaaf4c7d5f6a4005352c4f617162d2aab221a37 100644 (file)
@@ -43,6 +43,7 @@ extern void rtnl_lock(void);
 extern void rtnl_unlock(void);
 extern int rtnl_trylock(void);
 extern int rtnl_is_locked(void);
+extern int rtnl_lock_interruptible(void);
 extern int rtnl_lock_killable(void);
 extern bool refcount_dec_and_rtnl_lock(refcount_t *r);
 
index 07cb99b114bdd770d93a08088d6b3738776a86a2..e012234c739a3822bbd1df66801c42928f7218e6 100644 (file)
@@ -42,6 +42,87 @@ static inline int dev_isalive(const struct net_device *dev)
        return READ_ONCE(dev->reg_state) <= NETREG_REGISTERED;
 }
 
+/* There is a possible ABBA deadlock between rtnl_lock and kernfs_node->active,
+ * when unregistering a net device and accessing associated sysfs files. The
+ * potential deadlock is as follow:
+ *
+ *         CPU 0                                         CPU 1
+ *
+ *    rtnl_lock                                   vfs_read
+ *    unregister_netdevice_many                   kernfs_seq_start
+ *    device_del / kobject_put                      kernfs_get_active (kn->active++)
+ *    kernfs_drain                                sysfs_kf_seq_show
+ *    wait_event(                                 rtnl_lock
+ *       kn->active == KN_DEACTIVATED_BIAS)       -> waits on CPU 0 to release
+ *    -> waits on CPU 1 to decrease kn->active       the rtnl lock.
+ *
+ * The historical fix was to use rtnl_trylock with restart_syscall to bail out
+ * of sysfs operations when the lock couldn't be taken. This fixed the above
+ * issue as it allowed CPU 1 to bail out of the ABBA situation.
+ *
+ * But it came with performances issues, as syscalls are being restarted in
+ * loops when there was contention on the rtnl lock, with huge slow downs in
+ * specific scenarios (e.g. lots of virtual interfaces created and userspace
+ * daemons querying their attributes).
+ *
+ * The idea below is to bail out of the active kernfs_node protection
+ * (kn->active) while trying to take the rtnl lock.
+ *
+ * This replaces rtnl_lock() and still has to be used with rtnl_unlock(). The
+ * net device is guaranteed to be alive if this returns successfully.
+ */
+static int sysfs_rtnl_lock(struct kobject *kobj, struct attribute *attr,
+                          struct net_device *ndev)
+{
+       struct kernfs_node *kn;
+       int ret = 0;
+
+       /* First, we hold a reference to the net device as the unregistration
+        * path might run in parallel. This will ensure the net device and the
+        * associated sysfs objects won't be freed while we try to take the rtnl
+        * lock.
+        */
+       dev_hold(ndev);
+       /* sysfs_break_active_protection was introduced to allow self-removal of
+        * devices and their associated sysfs files by bailing out of the
+        * sysfs/kernfs protection. We do this here to allow the unregistration
+        * path to complete in parallel. The following takes a reference on the
+        * kobject and the kernfs_node being accessed.
+        *
+        * This works because we hold a reference onto the net device and the
+        * unregistration path will wait for us eventually in netdev_run_todo
+        * (outside an rtnl lock section).
+        */
+       kn = sysfs_break_active_protection(kobj, attr);
+       /* We can now try to take the rtnl lock. This can't deadlock us as the
+        * unregistration path is able to drain sysfs files (kernfs_node) thanks
+        * to the above dance.
+        */
+       if (rtnl_lock_interruptible()) {
+               ret = -ERESTARTSYS;
+               goto unbreak;
+       }
+       /* Check dismantle on the device hasn't started, otherwise deny the
+        * operation.
+        */
+       if (!dev_isalive(ndev)) {
+               rtnl_unlock();
+               ret = -ENODEV;
+               goto unbreak;
+       }
+       /* We are now sure the device dismantle hasn't started nor that it can
+        * start before we exit the locking section as we hold the rtnl lock.
+        * There's no need to keep unbreaking the sysfs protection nor to hold
+        * a net device reference from that point; that was only needed to take
+        * the rtnl lock.
+        */
+unbreak:
+       sysfs_unbreak_active_protection(kn);
+       dev_put(ndev);
+
+       return ret;
+}
+
 /* use same locking rules as GIF* ioctl's */
 static ssize_t netdev_show(const struct device *dev,
                           struct device_attribute *attr, char *buf,
@@ -95,14 +176,14 @@ static ssize_t netdev_store(struct device *dev, struct device_attribute *attr,
        if (ret)
                goto err;
 
-       if (!rtnl_trylock())
-               return restart_syscall();
+       ret = sysfs_rtnl_lock(&dev->kobj, &attr->attr, netdev);
+       if (ret)
+               goto err;
+
+       ret = (*set)(netdev, new);
+       if (ret == 0)
+               ret = len;
 
-       if (dev_isalive(netdev)) {
-               ret = (*set)(netdev, new);
-               if (ret == 0)
-                       ret = len;
-       }
        rtnl_unlock();
  err:
        return ret;
@@ -220,7 +301,7 @@ static ssize_t carrier_store(struct device *dev, struct device_attribute *attr,
        struct net_device *netdev = to_net_dev(dev);
 
        /* The check is also done in change_carrier; this helps returning early
-        * without hitting the trylock/restart in netdev_store.
+        * without hitting the locking section in netdev_store.
         */
        if (!netdev->netdev_ops->ndo_change_carrier)
                return -EOPNOTSUPP;
@@ -234,8 +315,9 @@ static ssize_t carrier_show(struct device *dev,
        struct net_device *netdev = to_net_dev(dev);
        int ret = -EINVAL;
 
-       if (!rtnl_trylock())
-               return restart_syscall();
+       ret = sysfs_rtnl_lock(&dev->kobj, &attr->attr, netdev);
+       if (ret)
+               return ret;
 
        if (netif_running(netdev)) {
                /* Synchronize carrier state with link watch,
@@ -245,8 +327,8 @@ static ssize_t carrier_show(struct device *dev,
 
                ret = sysfs_emit(buf, fmt_dec, !!netif_carrier_ok(netdev));
        }
-       rtnl_unlock();
 
+       rtnl_unlock();
        return ret;
 }
 static DEVICE_ATTR_RW(carrier);
@@ -258,13 +340,14 @@ static ssize_t speed_show(struct device *dev,
        int ret = -EINVAL;
 
        /* The check is also done in __ethtool_get_link_ksettings; this helps
-        * returning early without hitting the trylock/restart below.
+        * returning early without hitting the locking section below.
         */
        if (!netdev->ethtool_ops->get_link_ksettings)
                return ret;
 
-       if (!rtnl_trylock())
-               return restart_syscall();
+       ret = sysfs_rtnl_lock(&dev->kobj, &attr->attr, netdev);
+       if (ret)
+               return ret;
 
        if (netif_running(netdev)) {
                struct ethtool_link_ksettings cmd;
@@ -284,13 +367,14 @@ static ssize_t duplex_show(struct device *dev,
        int ret = -EINVAL;
 
        /* The check is also done in __ethtool_get_link_ksettings; this helps
-        * returning early without hitting the trylock/restart below.
+        * returning early without hitting the locking section below.
         */
        if (!netdev->ethtool_ops->get_link_ksettings)
                return ret;
 
-       if (!rtnl_trylock())
-               return restart_syscall();
+       ret = sysfs_rtnl_lock(&dev->kobj, &attr->attr, netdev);
+       if (ret)
+               return ret;
 
        if (netif_running(netdev)) {
                struct ethtool_link_ksettings cmd;
@@ -490,16 +574,15 @@ static ssize_t ifalias_store(struct device *dev, struct device_attribute *attr,
        if (len >  0 && buf[len - 1] == '\n')
                --count;
 
-       if (!rtnl_trylock())
-               return restart_syscall();
+       ret = sysfs_rtnl_lock(&dev->kobj, &attr->attr, netdev);
+       if (ret)
+               return ret;
 
-       if (dev_isalive(netdev)) {
-               ret = dev_set_alias(netdev, buf, count);
-               if (ret < 0)
-                       goto err;
-               ret = len;
-               netdev_state_change(netdev);
-       }
+       ret = dev_set_alias(netdev, buf, count);
+       if (ret < 0)
+               goto err;
+       ret = len;
+       netdev_state_change(netdev);
 err:
        rtnl_unlock();
 
@@ -551,24 +634,23 @@ static ssize_t phys_port_id_show(struct device *dev,
                                 struct device_attribute *attr, char *buf)
 {
        struct net_device *netdev = to_net_dev(dev);
+       struct netdev_phys_item_id ppid;
        ssize_t ret = -EINVAL;
 
        /* The check is also done in dev_get_phys_port_id; this helps returning
-        * early without hitting the trylock/restart below.
+        * early without hitting the locking section below.
         */
        if (!netdev->netdev_ops->ndo_get_phys_port_id)
                return -EOPNOTSUPP;
 
-       if (!rtnl_trylock())
-               return restart_syscall();
+       ret = sysfs_rtnl_lock(&dev->kobj, &attr->attr, netdev);
+       if (ret)
+               return ret;
 
-       if (dev_isalive(netdev)) {
-               struct netdev_phys_item_id ppid;
+       ret = dev_get_phys_port_id(netdev, &ppid);
+       if (!ret)
+               ret = sysfs_emit(buf, "%*phN\n", ppid.id_len, ppid.id);
 
-               ret = dev_get_phys_port_id(netdev, &ppid);
-               if (!ret)
-                       ret = sysfs_emit(buf, "%*phN\n", ppid.id_len, ppid.id);
-       }
        rtnl_unlock();
 
        return ret;
@@ -580,24 +662,23 @@ static ssize_t phys_port_name_show(struct device *dev,
 {
        struct net_device *netdev = to_net_dev(dev);
        ssize_t ret = -EINVAL;
+       char name[IFNAMSIZ];
 
        /* The checks are also done in dev_get_phys_port_name; this helps
-        * returning early without hitting the trylock/restart below.
+        * returning early without hitting the locking section below.
         */
        if (!netdev->netdev_ops->ndo_get_phys_port_name &&
            !netdev->devlink_port)
                return -EOPNOTSUPP;
 
-       if (!rtnl_trylock())
-               return restart_syscall();
+       ret = sysfs_rtnl_lock(&dev->kobj, &attr->attr, netdev);
+       if (ret)
+               return ret;
 
-       if (dev_isalive(netdev)) {
-               char name[IFNAMSIZ];
+       ret = dev_get_phys_port_name(netdev, name, sizeof(name));
+       if (!ret)
+               ret = sysfs_emit(buf, "%s\n", name);
 
-               ret = dev_get_phys_port_name(netdev, name, sizeof(name));
-               if (!ret)
-                       ret = sysfs_emit(buf, "%s\n", name);
-       }
        rtnl_unlock();
 
        return ret;
@@ -608,26 +689,25 @@ static ssize_t phys_switch_id_show(struct device *dev,
                                   struct device_attribute *attr, char *buf)
 {
        struct net_device *netdev = to_net_dev(dev);
+       struct netdev_phys_item_id ppid = { };
        ssize_t ret = -EINVAL;
 
        /* The checks are also done in dev_get_phys_port_name; this helps
-        * returning early without hitting the trylock/restart below. This works
+        * returning early without hitting the locking section below. This works
         * because recurse is false when calling dev_get_port_parent_id.
         */
        if (!netdev->netdev_ops->ndo_get_port_parent_id &&
            !netdev->devlink_port)
                return -EOPNOTSUPP;
 
-       if (!rtnl_trylock())
-               return restart_syscall();
+       ret = sysfs_rtnl_lock(&dev->kobj, &attr->attr, netdev);
+       if (ret)
+               return ret;
 
-       if (dev_isalive(netdev)) {
-               struct netdev_phys_item_id ppid = { };
+       ret = dev_get_port_parent_id(netdev, &ppid, false);
+       if (!ret)
+               ret = sysfs_emit(buf, "%*phN\n", ppid.id_len, ppid.id);
 
-               ret = dev_get_port_parent_id(netdev, &ppid, false);
-               if (!ret)
-                       ret = sysfs_emit(buf, "%*phN\n", ppid.id_len, ppid.id);
-       }
        rtnl_unlock();
 
        return ret;
index 1f4d4b5570ab896593a6b3216678b90c7b0696de..cb7fad8d1f95ff287810229c341de6a6d20a9c07 100644 (file)
@@ -80,6 +80,11 @@ void rtnl_lock(void)
 }
 EXPORT_SYMBOL(rtnl_lock);
 
+int rtnl_lock_interruptible(void)
+{
+       return mutex_lock_interruptible(&rtnl_mutex);
+}
+
 int rtnl_lock_killable(void)
 {
        return mutex_lock_killable(&rtnl_mutex);