File 1001-udev-use-lock-when-selecting-the-highest-p... of Package systemd-mini.25518

Overview Repositories Revisions Requests Users Attributes Meta

File 1001-udev-use-lock-when-selecting-the-highest-priority-de.patch of Package systemd-mini.25518

From 46f34a8c5ac73c8687f06b9245fd1ceeff7c57b4 Mon Sep 17 00:00:00 2001
From: Franck Bui <fbui@suse.com>
Date: Wed, 3 Mar 2021 17:34:39 +0100
Subject: [PATCH 1/1] udev: use lock when selecting the highest priority
 devlink

Commit 30f6dce62cb3a738b20253f2192270607c31b55b introduced a lock-less
algorithm, which is a kind of spinlock in userspace spinning on a heavy loop
(!?!), that is supposed to fix the race that might happen when multiple
workers/devices claim for the same symlink.

But the algorithm is boggus as every worker will prefer itself and will try to
"steal" the link from another worker with the same priority.

Rather than trying to fix the algorithm and close all possible races, let's use
a much simplest approach which consists in using a lock. This was originally
implemented by Martin Wilck: https://github.com/systemd/systemd/pull/8667 but
somehow upstream preferred the more complex lock-less algo without any proof of
benefits (quite the opposite since there're only drawback AFAICS).

In fact the figures we collected so far tends to show that the lock approach is
even faster than the racy version without any fix.

This patch basically drop the lock-less algo introduced by commit
30f6dce62cb3a738b20253f2192270607c31b55b and reuses Martin's lock approach with
some minor improvements.

Compare to the old implementation (before commit 30f6dce62cb3a738b20253f21), it
changes the moment the symlink are created: it's now down *after* udev DB has
been updated, hence removing the risk for another worker to overwrite a just
created symlink with a higher priority which has not yet been registed in the
DB. Latest versions of upstream calls update_devnode() twice, see
https://github.com/systemd/systemd/pull/19560#issuecomment-836693914 for the
"rationale".

[fbui: fixes bsc#1181192]
---
 src/udev/udev-event.c |  7 ----
 src/udev/udev-node.c  | 90 ++++++++++++++++---------------------------
 2 files changed, 33 insertions(+), 64 deletions(-)

diff --git a/src/udev/udev-event.c b/src/udev/udev-event.c
index d3b5995fc5..328e23f192 100644
--- a/src/udev/udev-event.c
+++ b/src/udev/udev-event.c
@@ -1008,10 +1008,6 @@ int udev_event_execute_rules(UdevEvent *event,
         if (r < 0)
                 return r;
 
-        r = update_devnode(event);
-        if (r < 0)
-                return r;
-
         /* preserve old, or get new initialization timestamp */
         r = device_ensure_usec_initialized(dev, event->dev_db_clone);
         if (r < 0)
@@ -1026,9 +1022,6 @@ int udev_event_execute_rules(UdevEvent *event,
         if (r < 0)
                 return log_device_debug_errno(dev, r, "Failed to update database under /run/udev/data/: %m");
 
-        /* Yes, we run update_devnode() twice, because in the first invocation, that is before update of udev database,
-         * it could happen that two contenders are replacing each other's symlink. Hence we run it again to make sure
-         * symlinks point to devices that claim them with the highest priority. */
         r = update_devnode(event);
         if (r < 0)
                 return r;
diff --git a/src/udev/udev-node.c b/src/udev/udev-node.c
index 228d20d5e4..eaa79f3438 100644
--- a/src/udev/udev-node.c
+++ b/src/udev/udev-node.c
@@ -16,19 +16,17 @@
 #include "format-util.h"
 #include "fs-util.h"
 #include "libudev-util.h"
+#include "lockfile-util.h"
 #include "mkdir.h"
 #include "path-util.h"
 #include "selinux-util.h"
 #include "smack-util.h"
-#include "stat-util.h"
 #include "stdio-util.h"
 #include "string-util.h"
 #include "strxcpyx.h"
 #include "udev-node.h"
 #include "user-util.h"
 
-#define LINK_UPDATE_MAX_RETRIES 128
-
 static int node_symlink(sd_device *dev, const char *node, const char *slink) {
         _cleanup_free_ char *slink_dirname = NULL, *target = NULL;
         const char *id_filename, *slink_tmp;
@@ -102,9 +100,7 @@ static int node_symlink(sd_device *dev, const char *node, const char *slink) {
         if (rename(slink_tmp, slink) < 0) {
                 r = log_device_error_errno(dev, errno, "Failed to rename '%s' to '%s': %m", slink_tmp, slink);
                 (void) unlink(slink_tmp);
-        } else
-                /* Tell caller that we replaced already existing symlink. */
-                r = 1;
+        }
 
         return r;
 }
@@ -194,10 +190,11 @@ static int link_find_prioritized(sd_device *dev, bool add, const char *stackdir,
 
 /* manage "stack of names" with possibly specified device priorities */
 static int link_update(sd_device *dev, const char *slink, bool add) {
-        _cleanup_free_ char *filename = NULL, *dirname = NULL;
+        _cleanup_(release_lock_file) LockFile lf = LOCK_FILE_INIT;
+        _cleanup_free_ char *target = NULL, *filename = NULL, *dirname = NULL;
         char name_enc[PATH_MAX];
         const char *id_filename;
-        int i, r, retries;
+        int r;
 
         assert(dev);
         assert(slink);
@@ -214,68 +211,47 @@ static int link_update(sd_device *dev, const char *slink, bool add) {
         if (!filename)
                 return log_oom();
 
+        mkdir_parents(dirname, 0755);
+        r = make_lock_file_for(dirname, LOCK_EX, &lf);
+        if (r < 0)
+                return log_error_errno(r, "failed to lock %s", dirname);
+
         if (!add) {
                 if (unlink(filename) == 0)
                         (void) rmdir(dirname);
-        } else
-                for (;;) {
-                        _cleanup_close_ int fd = -1;
-
-                        r = mkdir_parents(filename, 0755);
-                        if (!IN_SET(r, 0, -ENOENT))
-                                return r;
-
-                        fd = open(filename, O_WRONLY|O_CREAT|O_CLOEXEC|O_TRUNC|O_NOFOLLOW, 0444);
-                        if (fd >= 0)
-                                break;
-                        if (errno != ENOENT)
-                                return -errno;
-                }
-
-        /* If the database entry is not written yet we will just do one iteration and possibly wrong symlink
-         * will be fixed in the second invocation. */
-        retries = sd_device_get_is_initialized(dev) > 0 ? LINK_UPDATE_MAX_RETRIES : 1;
+        } else {
+                _cleanup_close_ int fd = -1;
 
-        for (i = 0; i < retries; i++) {
-                _cleanup_free_ char *target = NULL;
-                struct stat st1 = {}, st2 = {};
+                r = mkdir_parents(filename, 0755);
+                if (!IN_SET(r, 0, -ENOENT))
+                        return r;
 
-                r = stat(dirname, &st1);
-                if (r < 0 && errno != ENOENT)
+                fd = open(filename, O_WRONLY|O_CREAT|O_CLOEXEC|O_TRUNC|O_NOFOLLOW, 0444);
+                if (fd < 0)
                         return -errno;
+        }
 
-                r = link_find_prioritized(dev, add, dirname, &target);
+        /* link_find_prioritized() relies on 'dev' but that's not necessary as
+         * 'filename' has been created previously.  */
+        r = link_find_prioritized(dev, add, dirname, &target);
+        if (r < 0) {
                 if (r == -ENOENT) {
                         log_device_debug(dev, "No reference left, removing '%s'", slink);
+
                         if (unlink(slink) == 0)
                                 (void) rmdir_parents(slink, "/");
 
-                        break;
-                } else if (r < 0)
-                        return log_device_error_errno(dev, r, "Failed to determine highest priority symlink: %m");
-
-                r = node_symlink(dev, target, slink);
-                if (r < 0) {
-                        (void) unlink(filename);
-                        break;
-                } else if (r == 1)
-                        /* We have replaced already existing symlink, possibly there is some other device trying
-                         * to claim the same symlink. Let's do one more iteration to give us a chance to fix
-                         * the error if other device actually claims the symlink with higher priority. */
-                        continue;
-
-                /* Skip the second stat() if the first failed, stat_inode_unmodified() would return false regardless. */
-                if ((st1.st_mode & S_IFMT) != 0) {
-                        r = stat(dirname, &st2);
-                        if (r < 0 && errno != ENOENT)
-                                return -errno;
-
-                        if (stat_inode_unmodified(&st1, &st2))
-                                break;
+                        return 0;
                 }
+
+                return log_device_error_errno(dev, r, "Failed to find highest priority symlink: %m");
         }
 
-        return i < LINK_UPDATE_MAX_RETRIES ? 0 : -ELOOP;
+        r = node_symlink(dev, target, slink);
+        if (r < 0)
+                (void) unlink(filename);
+
+        return r;
 }
 
 int udev_node_update_old_links(sd_device *dev, sd_device *dev_old) {
@@ -486,7 +462,7 @@ int udev_node_add(sd_device *dev, bool apply,
         FOREACH_DEVICE_DEVLINK(dev, devlink) {
                 r = link_update(dev, devlink, true);
                 if (r < 0)
-                        log_device_info_errno(dev, r, "Failed to update device symlinks: %m");
+                        log_device_warning_errno(dev, r, "Failed to update device symlinks: %m");
         }
 
         return 0;
@@ -503,7 +479,7 @@ int udev_node_remove(sd_device *dev) {
         FOREACH_DEVICE_DEVLINK(dev, devlink) {
                 r = link_update(dev, devlink, false);
                 if (r < 0)
-                        log_device_info_errno(dev, r, "Failed to update device symlinks: %m");
+                        log_device_warning_errno(dev, r, "Failed to update device symlinks: %m");
         }
 
         r = xsprintf_dev_num_path_from_sd_device(dev, &filename);
-- 
2.26.2

Places

File 1001-udev-use-lock-when-selecting-the-highest-priority-de.patch of Package systemd-mini.25518

Places