udev: make algorithm that selects highest priority devlink less susceptible to race conditions

Previously it was very likely, when multiple contenders for the symlink
appear in parallel, that algorithm would select wrong symlink (i.e. one
with lower-priority).

Now the algorithm is much more defensive and when we detect change in
set of contenders for the symlink we reevaluate the selection. Same
happens when new symlink replaces already existing symlink that points
to different device node.

2 files changed, 67 insertions, 15 deletions

diff --git a/src/udev/udev-event.c b/src/udev/udev-event.c
index ede8e3aef7..87ae30cfa3 100644
--- a/src/udev/udev-event.c
+++ b/src/udev/udev-event.c
@@ -1041,6 +1041,13 @@ 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;
+
         device_set_is_initialized(dev);
 
         return 0;
diff --git a/src/udev/udev-node.c b/src/udev/udev-node.c
index 2cc26d29fa..de05556686 100644
--- a/src/udev/udev-node.c
+++ b/src/udev/udev-node.c
@@ -20,12 +20,15 @@
 #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;
@@ -99,7 +102,9 @@ 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;
 }
@@ -192,7 +197,7 @@ static int link_update(sd_device *dev, const char *slink, bool add) {
         _cleanup_free_ char *target = NULL, *filename = NULL, *dirname = NULL;
         char name_enc[PATH_MAX];
         const char *id_filename;
-        int r;
+        int i, r, retries;
 
         assert(dev);
         assert(slink);
@@ -212,14 +217,6 @@ static int link_update(sd_device *dev, const char *slink, bool add) {
         if (!add && unlink(filename) == 0)
                 (void) rmdir(dirname);
 
-        r = link_find_prioritized(dev, add, dirname, &target);
-        if (r < 0) {
-                log_device_debug(dev, "No reference left, removing '%s'", slink);
-                if (unlink(slink) == 0)
-                        (void) rmdir_parents(slink, "/");
-        } else
-                (void) node_symlink(dev, target, slink);
-
         if (add)
                 do {
                         _cleanup_close_ int fd = -1;
@@ -232,7 +229,49 @@ static int link_update(sd_device *dev, const char *slink, bool add) {
                                 r = -errno;
                 } while (r == -ENOENT);
 
-        return r;
+        /* 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;
+
+        for (i = 0; i < retries; i++) {
+                struct stat st1 = {}, st2 = {};
+
+                r = stat(dirname, &st1);
+                if (r < 0 && errno != ENOENT)
+                        return -errno;
+
+                r = link_find_prioritized(dev, add, dirname, &target);
+                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 i < LINK_UPDATE_MAX_RETRIES ? 0 : -ELOOP;
 }
 
 int udev_node_update_old_links(sd_device *dev, sd_device *dev_old) {
@@ -451,8 +490,11 @@ int udev_node_add(sd_device *dev, bool apply,
         (void) node_symlink(dev, devnode, filename);
 
         /* create/update symlinks, add symlinks to name index */
-        FOREACH_DEVICE_DEVLINK(dev, devlink)
-                (void) link_update(dev, devlink, true);
+        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");
+        }
 
         return 0;
 }
@@ -465,8 +507,11 @@ int udev_node_remove(sd_device *dev) {
         assert(dev);
 
         /* remove/update symlinks, remove symlinks from name index */
-        FOREACH_DEVICE_DEVLINK(dev, devlink)
-                (void) link_update(dev, devlink, false);
+        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");
+        }
 
         r = xsprintf_dev_num_path_from_sd_device(dev, &filename);
         if (r < 0)