daandemeyer / rpms / systemd

Forked from rpms/systemd 2 years ago
Clone
21255d
From 1d5f966c1758eb620755fcae54abd07a1ac36d3d Mon Sep 17 00:00:00 2001
21255d
From: Michal Sekletar <msekleta@redhat.com>
21255d
Date: Wed, 6 Jan 2021 11:43:50 +0100
21255d
Subject: [PATCH] udev: make algorithm that selects highest priority devlink
21255d
 less susceptible to race conditions
21255d
21255d
Previously it was very likely, when multiple contenders for the symlink
21255d
appear in parallel, that algorithm would select wrong symlink (i.e. one
21255d
with lower-priority).
21255d
21255d
Now the algorithm is much more defensive and when we detect change in
21255d
set of contenders for the symlink we reevaluate the selection. Same
21255d
happens when new symlink replaces already existing symlink that points
21255d
to different device node.
21255d
21255d
Resolves: #1642728
21255d
---
21255d
 src/udev/udev-event.c |  71 +++++++-----
21255d
 src/udev/udev-node.c  | 244 ++++++++++++++++++++++++++++++------------
21255d
 2 files changed, 216 insertions(+), 99 deletions(-)
21255d
21255d
diff --git a/src/udev/udev-event.c b/src/udev/udev-event.c
21255d
index fd8406d959..9004634f65 100644
21255d
--- a/src/udev/udev-event.c
21255d
+++ b/src/udev/udev-event.c
21255d
@@ -833,6 +833,41 @@ static int rename_netif(struct udev_event *event) {
21255d
         return 0;
21255d
 }
21255d
 
21255d
+static void update_devnode(struct udev_event *event) {
21255d
+        struct udev_device *dev = event->dev;
21255d
+
21255d
+        if (major(udev_device_get_devnum(dev)) > 0) {
21255d
+                bool apply;
21255d
+
21255d
+                /* remove/update possible left-over symlinks from old database entry */
21255d
+                if (event->dev_db != NULL)
21255d
+                        udev_node_update_old_links(dev, event->dev_db);
21255d
+
21255d
+                if (!event->owner_set)
21255d
+                        event->uid = udev_device_get_devnode_uid(dev);
21255d
+
21255d
+                if (!event->group_set)
21255d
+                        event->gid = udev_device_get_devnode_gid(dev);
21255d
+
21255d
+                if (!event->mode_set) {
21255d
+                        if (udev_device_get_devnode_mode(dev) > 0) {
21255d
+                                /* kernel supplied value */
21255d
+                                event->mode = udev_device_get_devnode_mode(dev);
21255d
+                        } else if (event->gid > 0) {
21255d
+                                /* default 0660 if a group is assigned */
21255d
+                                event->mode = 0660;
21255d
+                        }
21255d
+                        else {
21255d
+                                /* default 0600 */
21255d
+                                event->mode = 0600;
21255d
+                        }
21255d
+                }
21255d
+
21255d
+                apply = streq(udev_device_get_action(dev), "add") || event->owner_set || event->group_set || event->mode_set;
21255d
+                udev_node_add(dev, apply, event->mode, event->uid, event->gid, &event->seclabel_list);
21255d
+        }
21255d
+}
21255d
+
21255d
 void udev_event_execute_rules(struct udev_event *event,
21255d
                               usec_t timeout_usec, usec_t timeout_warn_usec,
21255d
                               struct udev_list *properties_list,
21255d
@@ -891,35 +926,7 @@ void udev_event_execute_rules(struct udev_event *event,
21255d
                         }
21255d
                 }
21255d
 
21255d
-                if (major(udev_device_get_devnum(dev)) > 0) {
21255d
-                        bool apply;
21255d
-
21255d
-                        /* remove/update possible left-over symlinks from old database entry */
21255d
-                        if (event->dev_db != NULL)
21255d
-                                udev_node_update_old_links(dev, event->dev_db);
21255d
-
21255d
-                        if (!event->owner_set)
21255d
-                                event->uid = udev_device_get_devnode_uid(dev);
21255d
-
21255d
-                        if (!event->group_set)
21255d
-                                event->gid = udev_device_get_devnode_gid(dev);
21255d
-
21255d
-                        if (!event->mode_set) {
21255d
-                                if (udev_device_get_devnode_mode(dev) > 0) {
21255d
-                                        /* kernel supplied value */
21255d
-                                        event->mode = udev_device_get_devnode_mode(dev);
21255d
-                                } else if (event->gid > 0) {
21255d
-                                        /* default 0660 if a group is assigned */
21255d
-                                        event->mode = 0660;
21255d
-                                } else {
21255d
-                                        /* default 0600 */
21255d
-                                        event->mode = 0600;
21255d
-                                }
21255d
-                        }
21255d
-
21255d
-                        apply = streq(udev_device_get_action(dev), "add") || event->owner_set || event->group_set || event->mode_set;
21255d
-                        udev_node_add(dev, apply, event->mode, event->uid, event->gid, &event->seclabel_list);
21255d
-                }
21255d
+                update_devnode(event);
21255d
 
21255d
                 /* preserve old, or get new initialization timestamp */
21255d
                 udev_device_ensure_usec_initialized(event->dev, event->dev_db);
21255d
@@ -927,6 +934,12 @@ void udev_event_execute_rules(struct udev_event *event,
21255d
                 /* (re)write database file */
21255d
                 udev_device_tag_index(dev, event->dev_db, true);
21255d
                 udev_device_update_db(dev);
21255d
+
21255d
+                /* Yes, we run update_devnode() twice, because in the first invocation, that is before update of udev database,
21255d
+                 * it could happen that two contenders are replacing each other's symlink. Hence we run it again to make sure
21255d
+                 * symlinks point to devices that claim them with the highest priority. */
21255d
+                update_devnode(event);
21255d
+
21255d
                 udev_device_set_is_initialized(dev);
21255d
 
21255d
                 event->dev_db = udev_device_unref(event->dev_db);
21255d
diff --git a/src/udev/udev-node.c b/src/udev/udev-node.c
21255d
index 333dcae6b9..2eeeccdd3a 100644
21255d
--- a/src/udev/udev-node.c
21255d
+++ b/src/udev/udev-node.c
21255d
@@ -13,19 +13,27 @@
21255d
 #include <unistd.h>
21255d
 
21255d
 #include "device-nodes.h"
21255d
+#include "device-private.h"
21255d
 #include "dirent-util.h"
21255d
+#include "fd-util.h"
21255d
 #include "format-util.h"
21255d
 #include "fs-util.h"
21255d
+#include "sd-device.h"
21255d
 #include "selinux-util.h"
21255d
 #include "smack-util.h"
21255d
+#include "stat-util.h"
21255d
 #include "stdio-util.h"
21255d
 #include "string-util.h"
21255d
 #include "udev.h"
21255d
+#include "libudev-device-internal.h"
21255d
 
21255d
-static int node_symlink(struct udev_device *dev, const char *node, const char *slink) {
21255d
+#define LINK_UPDATE_MAX_RETRIES 128
21255d
+
21255d
+static int node_symlink(sd_device *dev, const char *node, const char *slink) {
21255d
         struct stat stats;
21255d
         char target[UTIL_PATH_SIZE];
21255d
         char *s;
21255d
+        const char *id_filename;
21255d
         size_t l;
21255d
         char slink_tmp[UTIL_PATH_SIZE + 32];
21255d
         int i = 0;
21255d
@@ -89,7 +97,10 @@ static int node_symlink(struct udev_device *dev, const char *node, const char *s
21255d
         }
21255d
 
21255d
         log_debug("atomically replace '%s'", slink);
21255d
-        strscpyl(slink_tmp, sizeof(slink_tmp), slink, ".tmp-", udev_device_get_id_filename(dev), NULL);
21255d
+        err = device_get_id_filename(dev, &id_filename);
21255d
+        if (err < 0)
21255d
+                return log_error_errno(err, "Failed to get id_filename: %m");
21255d
+        strscpyl(slink_tmp, sizeof(slink_tmp), slink, ".tmp-", id_filename, NULL);
21255d
         unlink(slink_tmp);
21255d
         do {
21255d
                 err = mkdir_parents_label(slink_tmp, 0755);
21255d
@@ -109,104 +120,187 @@ static int node_symlink(struct udev_device *dev, const char *node, const char *s
21255d
         if (err != 0) {
21255d
                 log_error_errno(errno, "rename '%s' '%s' failed: %m", slink_tmp, slink);
21255d
                 unlink(slink_tmp);
21255d
-        }
21255d
+        } else
21255d
+                /* Tell caller that we replaced already existing symlink. */
21255d
+                return 1;
21255d
 exit:
21255d
         return err;
21255d
 }
21255d
 
21255d
 /* find device node of device with highest priority */
21255d
-static const char *link_find_prioritized(struct udev_device *dev, bool add, const char *stackdir, char *buf, size_t bufsize) {
21255d
-        struct udev *udev = udev_device_get_udev(dev);
21255d
-        DIR *dir;
21255d
+static int link_find_prioritized(sd_device *dev, bool add, const char *stackdir, char **ret) {
21255d
+        _cleanup_closedir_ DIR *dir = NULL;
21255d
+        _cleanup_free_ char *target = NULL;
21255d
         struct dirent *dent;
21255d
-        int priority = 0;
21255d
-        const char *target = NULL;
21255d
+        int r, priority = 0;
21255d
+
21255d
+        assert(!add || dev);
21255d
+        assert(stackdir);
21255d
+        assert(ret);
21255d
 
21255d
         if (add) {
21255d
-                priority = udev_device_get_devlink_priority(dev);
21255d
-                strscpy(buf, bufsize, udev_device_get_devnode(dev));
21255d
-                target = buf;
21255d
+                const char *devnode;
21255d
+
21255d
+                r = device_get_devlink_priority(dev, &priority);
21255d
+                if (r < 0)
21255d
+                        return r;
21255d
+
21255d
+                r = sd_device_get_devname(dev, &devnode);
21255d
+                if (r < 0)
21255d
+                        return r;
21255d
+
21255d
+                target = strdup(devnode);
21255d
+                if (!target)
21255d
+                        return -ENOMEM;
21255d
         }
21255d
 
21255d
         dir = opendir(stackdir);
21255d
-        if (dir == NULL)
21255d
-                return target;
21255d
+        if (!dir) {
21255d
+                if (target) {
21255d
+                        *ret = TAKE_PTR(target);
21255d
+                        return 0;
21255d
+                }
21255d
+
21255d
+                return -errno;
21255d
+        }
21255d
+
21255d
         FOREACH_DIRENT_ALL(dent, dir, break) {
21255d
-                struct udev_device *dev_db;
21255d
+                _cleanup_(sd_device_unrefp) sd_device *dev_db = NULL;
21255d
+                const char *devnode, *id_filename;
21255d
+                int db_prio = 0;
21255d
 
21255d
                 if (dent->d_name[0] == '\0')
21255d
                         break;
21255d
                 if (dent->d_name[0] == '.')
21255d
                         continue;
21255d
 
21255d
-                log_debug("found '%s' claiming '%s'", dent->d_name, stackdir);
21255d
+                log_debug("Found '%s' claiming '%s'", dent->d_name, stackdir);
21255d
+
21255d
+                if (device_get_id_filename(dev, &id_filename) < 0)
21255d
+                        continue;
21255d
 
21255d
                 /* did we find ourself? */
21255d
-                if (streq(dent->d_name, udev_device_get_id_filename(dev)))
21255d
+                if (streq(dent->d_name, id_filename))
21255d
                         continue;
21255d
 
21255d
-                dev_db = udev_device_new_from_device_id(udev, dent->d_name);
21255d
-                if (dev_db != NULL) {
21255d
-                        const char *devnode;
21255d
-
21255d
-                        devnode = udev_device_get_devnode(dev_db);
21255d
-                        if (devnode != NULL) {
21255d
-                                if (target == NULL || udev_device_get_devlink_priority(dev_db) > priority) {
21255d
-                                        log_debug("'%s' claims priority %i for '%s'",
21255d
-                                                  udev_device_get_syspath(dev_db), udev_device_get_devlink_priority(dev_db), stackdir);
21255d
-                                        priority = udev_device_get_devlink_priority(dev_db);
21255d
-                                        strscpy(buf, bufsize, devnode);
21255d
-                                        target = buf;
21255d
-                                }
21255d
-                        }
21255d
-                        udev_device_unref(dev_db);
21255d
+                if (sd_device_new_from_device_id(&dev_db, dent->d_name) < 0)
21255d
+                        continue;
21255d
+
21255d
+                if (sd_device_get_devname(dev_db, &devnode) < 0)
21255d
+                        continue;
21255d
+
21255d
+                if (device_get_devlink_priority(dev_db, &db_prio) < 0)
21255d
+                        continue;
21255d
+
21255d
+                if (target && db_prio <= priority)
21255d
+                        continue;
21255d
+
21255d
+                if (DEBUG_LOGGING) {
21255d
+                        const char *syspath = NULL;
21255d
+
21255d
+                        (void) sd_device_get_syspath(dev_db, &syspath);
21255d
+                        log_debug("Device '%s' claims priority %i for '%s'", strnull(syspath), db_prio, stackdir);
21255d
                 }
21255d
+
21255d
+                r = free_and_strdup(&target, devnode);
21255d
+                if (r < 0)
21255d
+                        return r;
21255d
+                priority = db_prio;
21255d
         }
21255d
-        closedir(dir);
21255d
-        return target;
21255d
+
21255d
+        if (!target)
21255d
+                return -ENOENT;
21255d
+
21255d
+        *ret = TAKE_PTR(target);
21255d
+        return 0;
21255d
 }
21255d
 
21255d
+
21255d
 /* manage "stack of names" with possibly specified device priorities */
21255d
-static void link_update(struct udev_device *dev, const char *slink, bool add) {
21255d
-        char name_enc[UTIL_PATH_SIZE];
21255d
-        char filename[UTIL_PATH_SIZE * 2];
21255d
-        char dirname[UTIL_PATH_SIZE];
21255d
-        const char *target;
21255d
-        char buf[UTIL_PATH_SIZE];
21255d
+static int link_update(sd_device *dev, const char *slink, bool add) {
21255d
+        _cleanup_free_ char *filename = NULL, *dirname = NULL;
21255d
+        char name_enc[PATH_MAX];
21255d
+        const char *id_filename;
21255d
+        int i, r, retries;
21255d
+
21255d
+        assert(dev);
21255d
+        assert(slink);
21255d
+
21255d
+        r = device_get_id_filename(dev, &id_filename);
21255d
+        if (r < 0)
21255d
+                return log_debug_errno(r, "Failed to get id_filename: %m");
21255d
 
21255d
         util_path_encode(slink + STRLEN("/dev"), name_enc, sizeof(name_enc));
21255d
-        strscpyl(dirname, sizeof(dirname), "/run/udev/links/", name_enc, NULL);
21255d
-        strscpyl(filename, sizeof(filename), dirname, "/", udev_device_get_id_filename(dev), NULL);
21255d
+        dirname = path_join(NULL, "/run/udev/links/", name_enc);
21255d
+        if (!dirname)
21255d
+                return log_oom();
21255d
+        filename = path_join(NULL, dirname, id_filename);
21255d
+        if (!filename)
21255d
+                return log_oom();
21255d
+
21255d
+        if (!add) {
21255d
+                if (unlink(filename) == 0)
21255d
+                        (void) rmdir(dirname);
21255d
+        } else
21255d
+                for (;;) {
21255d
+                        _cleanup_close_ int fd = -1;
21255d
+
21255d
+                        r = mkdir_parents(filename, 0755);
21255d
+                        if (!IN_SET(r, 0, -ENOENT))
21255d
+                                return r;
21255d
 
21255d
-        if (!add && unlink(filename) == 0)
21255d
-                rmdir(dirname);
21255d
+                        fd = open(filename, O_WRONLY|O_CREAT|O_CLOEXEC|O_TRUNC|O_NOFOLLOW, 0444);
21255d
+                        if (fd >= 0)
21255d
+                                break;
21255d
+                        if (errno != ENOENT)
21255d
+                                return -errno;
21255d
+                }
21255d
 
21255d
-        target = link_find_prioritized(dev, add, dirname, buf, sizeof(buf));
21255d
-        if (target == NULL) {
21255d
-                log_debug("no reference left, remove '%s'", slink);
21255d
-                if (unlink(slink) == 0)
21255d
-                        rmdir_parents(slink, "/");
21255d
-        } else {
21255d
-                log_debug("creating link '%s' to '%s'", slink, target);
21255d
-                node_symlink(dev, target, slink);
21255d
-        }
21255d
+        /* If the database entry is not written yet we will just do one iteration and possibly wrong symlink
21255d
+         * will be fixed in the second invocation. */
21255d
+        (void) sd_device_get_is_initialized(dev, &r);
21255d
+        retries = r > 0 ? LINK_UPDATE_MAX_RETRIES : 1;
21255d
 
21255d
-        if (add) {
21255d
-                int err;
21255d
+        for (i = 0; i < retries; i++) {
21255d
+                _cleanup_free_ char *target = NULL;
21255d
+                struct stat st1 = {}, st2 = {};
21255d
 
21255d
-                do {
21255d
-                        int fd;
21255d
+                r = stat(dirname, &st1;;
21255d
+                if (r < 0 && errno != ENOENT)
21255d
+                        return -errno;
21255d
 
21255d
-                        err = mkdir_parents(filename, 0755);
21255d
-                        if (!IN_SET(err, 0, -ENOENT))
21255d
+                r = link_find_prioritized(dev, add, dirname, &target);
21255d
+                if (r == -ENOENT) {
21255d
+                        log_debug("No reference left, removing '%s'", slink);
21255d
+                        if (unlink(slink) == 0)
21255d
+                                (void) rmdir_parents(slink, "/");
21255d
+
21255d
+                        break;
21255d
+                } else if (r < 0)
21255d
+                        return log_error_errno(r, "Failed to determine highest priority symlink: %m");
21255d
+
21255d
+                r = node_symlink(dev, target, slink);
21255d
+                if (r < 0) {
21255d
+                        (void) unlink(filename);
21255d
+                        break;
21255d
+                } else if (r == 1)
21255d
+                        /* We have replaced already existing symlink, possibly there is some other device trying
21255d
+                         * to claim the same symlink. Let's do one more iteration to give us a chance to fix
21255d
+                         * the error if other device actually claims the symlink with higher priority. */
21255d
+                        continue;
21255d
+
21255d
+               /* Skip the second stat() if the first failed, stat_inode_unmodified() would return false regardless. */
21255d
+                if ((st1.st_mode & S_IFMT) != 0) {
21255d
+                        r = stat(dirname, &st2;;
21255d
+                        if (r < 0 && errno != ENOENT)
21255d
+                                return -errno;
21255d
+
21255d
+                        if (stat_inode_unmodified(&st1, &st2))
21255d
                                 break;
21255d
-                        fd = open(filename, O_WRONLY|O_CREAT|O_CLOEXEC|O_TRUNC|O_NOFOLLOW, 0444);
21255d
-                        if (fd >= 0)
21255d
-                                close(fd);
21255d
-                        else
21255d
-                                err = -errno;
21255d
-                } while (err == -ENOENT);
21255d
+                }
21255d
         }
21255d
+
21255d
+        return i < LINK_UPDATE_MAX_RETRIES ? 0 : -ELOOP;
21255d
 }
21255d
 
21255d
 void udev_node_update_old_links(struct udev_device *dev, struct udev_device *dev_old) {
21255d
@@ -233,7 +327,7 @@ void udev_node_update_old_links(struct udev_device *dev, struct udev_device *dev
21255d
 
21255d
                 log_debug("update old name, '%s' no longer belonging to '%s'",
21255d
                      name, udev_device_get_devpath(dev));
21255d
-                link_update(dev, name, false);
21255d
+                link_update(dev->device, name, false);
21255d
         }
21255d
 }
21255d
 
21255d
@@ -338,11 +432,16 @@ void udev_node_add(struct udev_device *dev, bool apply,
21255d
         xsprintf_dev_num_path(filename,
21255d
                               streq(udev_device_get_subsystem(dev), "block") ? "block" : "char",
21255d
                               udev_device_get_devnum(dev));
21255d
-        node_symlink(dev, udev_device_get_devnode(dev), filename);
21255d
+        node_symlink(dev->device, udev_device_get_devnode(dev), filename);
21255d
 
21255d
         /* create/update symlinks, add symlinks to name index */
21255d
-        udev_list_entry_foreach(list_entry, udev_device_get_devlinks_list_entry(dev))
21255d
-                        link_update(dev, udev_list_entry_get_name(list_entry), true);
21255d
+        udev_list_entry_foreach(list_entry, udev_device_get_devlinks_list_entry(dev)) {
21255d
+                int r;
21255d
+
21255d
+                r = link_update(dev->device, udev_list_entry_get_name(list_entry), true);
21255d
+                if (r < 0)
21255d
+                        log_info_errno(r, "Failed to update device symlinks: %m");
21255d
+        }
21255d
 }
21255d
 
21255d
 void udev_node_remove(struct udev_device *dev) {
21255d
@@ -350,8 +449,13 @@ void udev_node_remove(struct udev_device *dev) {
21255d
         char filename[DEV_NUM_PATH_MAX];
21255d
 
21255d
         /* remove/update symlinks, remove symlinks from name index */
21255d
-        udev_list_entry_foreach(list_entry, udev_device_get_devlinks_list_entry(dev))
21255d
-                link_update(dev, udev_list_entry_get_name(list_entry), false);
21255d
+        udev_list_entry_foreach(list_entry, udev_device_get_devlinks_list_entry(dev)) {
21255d
+                int r;
21255d
+
21255d
+                r = link_update(dev->device, udev_list_entry_get_name(list_entry), false);
21255d
+                if (r < 0)
21255d
+                        log_info_errno(r, "Failed to update device symlinks: %m");
21255d
+        }
21255d
 
21255d
         /* remove /dev/{block,char}/$major:$minor */
21255d
         xsprintf_dev_num_path(filename,