diff --git a/src/udev/udev-manager.c b/src/udev/udev-manager.c index 08ed176faf..32bc0db345 100644 --- a/src/udev/udev-manager.c +++ b/src/udev/udev-manager.c @@ -583,9 +583,6 @@ static int event_queue_start(Manager *manager) { if (!manager->events || manager->exit || manager->stop_exec_queue) return 0; - /* To make the stack directory /run/udev/links cleaned up later. */ - manager->udev_node_needs_cleanup = true; - r = event_source_disable(manager->kill_workers_event); if (r < 0) log_warning_errno(r, "Failed to disable event source for cleaning up idle workers, ignoring: %m"); @@ -1181,11 +1178,6 @@ static int on_post(sd_event_source *s, void *userdata) { /* There are no idle workers. */ - if (manager->udev_node_needs_cleanup) { - (void) udev_node_cleanup(); - manager->udev_node_needs_cleanup = false; - } - if (manager->exit) return sd_event_exit(manager->event, 0); diff --git a/src/udev/udev-manager.h b/src/udev/udev-manager.h index 41aa73698d..5921d333d1 100644 --- a/src/udev/udev-manager.h +++ b/src/udev/udev-manager.h @@ -44,7 +44,6 @@ typedef struct Manager { int timeout_signal; bool blockdev_read_only; - bool udev_node_needs_cleanup; bool stop_exec_queue; bool exit; } Manager; diff --git a/src/udev/udev-node.c b/src/udev/udev-node.c index a0bf0a9619..673f723ff2 100644 --- a/src/udev/udev-node.c +++ b/src/udev/udev-node.c @@ -28,48 +28,6 @@ #define UDEV_NODE_HASH_KEY SD_ID128_MAKE(b9,6a,f1,ce,40,31,44,1a,9e,19,ec,8b,ae,f3,e3,2f) -int udev_node_cleanup(void) { - _cleanup_closedir_ DIR *dir = NULL; - - /* This must not be called when any workers exist. It would cause a race between mkdir() called - * by stack_directory_lock() and unlinkat() called by this. */ - - dir = opendir("/run/udev/links"); - if (!dir) { - if (errno == ENOENT) - return 0; - - return log_debug_errno(errno, "Failed to open directory '/run/udev/links', ignoring: %m"); - } - - FOREACH_DIRENT_ALL(de, dir, break) { - _cleanup_free_ char *lockfile = NULL; - - if (de->d_name[0] == '.') - continue; - - if (de->d_type != DT_DIR) - continue; - - /* As commented in the above, this is called when no worker exists, hence the file is not - * locked. On a later uevent, the lock file will be created if necessary. So, we can safely - * remove the file now. */ - lockfile = path_join(de->d_name, ".lock"); - if (!lockfile) - return log_oom_debug(); - - if (unlinkat(dirfd(dir), lockfile, 0) < 0 && errno != ENOENT) { - log_debug_errno(errno, "Failed to remove '/run/udev/links/%s', ignoring: %m", lockfile); - continue; - } - - if (unlinkat(dirfd(dir), de->d_name, AT_REMOVEDIR) < 0 && errno != ENOTEMPTY) - log_debug_errno(errno, "Failed to remove '/run/udev/links/%s', ignoring: %m", de->d_name); - } - - return 0; -} - static int node_remove_symlink(sd_device *dev, const char *slink) { assert(dev); assert(slink); @@ -339,7 +297,7 @@ toolong: } static int stack_directory_get_name(const char *slink, char **ret) { - _cleanup_free_ char *s = NULL, *dirname = NULL; + _cleanup_free_ char *s = NULL; char name_enc[NAME_MAX+1]; const char *name; int r; @@ -360,45 +318,59 @@ static int stack_directory_get_name(const char *slink, char **ret) { udev_node_escape_path(name, name_enc, sizeof(name_enc)); - dirname = path_join("/run/udev/links", name_enc); - if (!dirname) - return -ENOMEM; - - *ret = TAKE_PTR(dirname); - return 0; + return strdup_to(ret, name_enc); } -static int stack_directory_open(sd_device *dev, const char *slink, int *ret_dirfd, int *ret_lockfd) { - _cleanup_close_ int dirfd = -EBADF, lockfd = -EBADF; - _cleanup_free_ char *dirname = NULL; +static int stack_directory_open_and_lock( + sd_device *dev, + const char *slink, + char **ret_dirpath, + int *ret_dirfd, + LockFile *ret_lockfile) { + + _cleanup_(release_lock_file) LockFile lockfile = LOCK_FILE_INIT; + _cleanup_close_ int dirfd = -EBADF; + _cleanup_free_ char *name = NULL, *dirpath = NULL, *lockname = NULL; int r; assert(dev); assert(slink); + assert(ret_dirpath); assert(ret_dirfd); - assert(ret_lockfd); + assert(ret_lockfile); - r = stack_directory_get_name(slink, &dirname); + r = stack_directory_get_name(slink, &name); if (r < 0) return log_device_debug_errno(dev, r, "Failed to build stack directory name for '%s': %m", slink); - r = mkdir_parents(dirname, 0755); + FOREACH_STRING(s, "/run/udev/links/", "/run/udev/links.lock/") { + r = mkdir_p(s, 0755); + if (r < 0) + return log_device_debug_errno(dev, r, "Failed to create '%s': %m", s); + } + + /* 1. Take a lock for the stack directory. */ + lockname = path_join("/run/udev/links.lock/", name); + if (!lockname) + return -ENOMEM; + + r = make_lock_file(lockname, LOCK_EX, &lockfile); if (r < 0) - return log_device_debug_errno(dev, r, "Failed to create stack directory '%s': %m", dirname); + return log_device_debug_errno(dev, r, "Failed to create and lock '%s': %m", lockname); - dirfd = open_mkdir(dirname, O_CLOEXEC | O_DIRECTORY | O_NOFOLLOW | O_RDONLY, 0755); + /* 2. Create and open the stack directory. Do not create the stack directory before taking a lock, + * otherwise the directory may be removed by another worker. */ + dirpath = path_join("/run/udev/links/", name); + if (!dirpath) + return -ENOMEM; + + dirfd = open_mkdir(dirpath, O_CLOEXEC | O_DIRECTORY | O_NOFOLLOW | O_RDONLY, 0755); if (dirfd < 0) - return log_device_debug_errno(dev, dirfd, "Failed to open stack directory '%s': %m", dirname); - - lockfd = openat(dirfd, ".lock", O_CLOEXEC | O_NOFOLLOW | O_RDONLY | O_CREAT, 0600); - if (lockfd < 0) - return log_device_debug_errno(dev, errno, "Failed to create lock file for stack directory '%s': %m", dirname); - - if (flock(lockfd, LOCK_EX) < 0) - return log_device_debug_errno(dev, errno, "Failed to place a lock on lock file for %s: %m", dirname); + return log_device_debug_errno(dev, dirfd, "Failed to open stack directory '%s': %m", dirpath); + *ret_dirpath = TAKE_PTR(dirpath); *ret_dirfd = TAKE_FD(dirfd); - *ret_lockfd = TAKE_FD(lockfd); + *ret_lockfile = TAKE_GENERIC(lockfile, LockFile, LOCK_FILE_INIT); return 0; } @@ -434,98 +406,22 @@ static int node_get_current(const char *slink, int dirfd, char **ret_id, int *re return 0; } -static int link_update_diskseq(sd_device *dev, const char *slink, bool add) { - _cleanup_free_ char *buf = NULL; - const char *fname, *diskseq; - int r; - - assert(dev); - assert(slink); - - if (!device_in_subsystem(dev, "block")) - return 0; - - fname = path_startswith(slink, "/dev/disk/by-diskseq"); - if (isempty(fname)) - return 0; - - if (device_is_devtype(dev, "partition")) { - _cleanup_free_ char *suffix = NULL; - const char *partn, *p; - - /* Check if the symlink has an expected suffix "-part%n". See 60-persistent-storage.rules. */ - - r = sd_device_get_sysnum(dev, &partn); - if (r < 0) { - /* Cannot verify the symlink is owned by this device. Let's create the stack directory for the symlink. */ - log_device_debug_errno(dev, r, "Failed to get sysnum, but symlink '%s' is requested, ignoring: %m", slink); - return 0; - } - - suffix = strjoin("-part", partn); - if (!suffix) - return -ENOMEM; - - p = endswith(fname, suffix); - if (!p) { - log_device_debug(dev, "Unexpected by-diskseq symlink '%s' is requested, proceeding anyway.", slink); - return 0; - } - - buf = strndup(fname, p - fname); - if (!buf) - return -ENOMEM; - - fname = buf; - } - - /* Check if the diskseq part of the symlink is in digits. */ - if (!in_charset(fname, DIGITS)) { - log_device_debug(dev, "Unexpected by-diskseq symlink '%s' is requested, proceeding anyway.", slink); - return 0; /* unexpected by-diskseq symlink */ - } - - /* On removal, we cannot verify the diskseq. Skipping further check below. */ - if (!add) { - r = node_remove_symlink(dev, slink); - if (r < 0) - return r; - - return 1; /* done */ - } - - /* Check if the diskseq matches with the DISKSEQ property. */ - r = sd_device_get_property_value(dev, "DISKSEQ", &diskseq); - if (r < 0) { - log_device_debug_errno(dev, r, "Failed to get DISKSEQ property, but symlink '%s' is requested, ignoring: %m", slink); - return 0; - } - - if (!streq(fname, diskseq)) { - log_device_debug(dev, "Unexpected by-diskseq symlink '%s' is requested (DISKSEQ=%s), proceeding anyway.", slink, diskseq); - return 0; - } - - r = node_create_symlink(dev, /* devnode = */ NULL, slink); - if (r < 0) - return r; - - return 1; /* done */ -} - static int link_update(sd_device *dev, const char *slink, bool add) { + /* On cleaning up, + * 1. close the stack directory, + * 2. remove the stack directory if it is empty, + * 3. then finally release the lock. + * Hence, the variables must be declared in the reverse order. */ + _cleanup_(release_lock_file) LockFile lockfile = LOCK_FILE_INIT; /* #3 */ + _cleanup_(rmdir_and_freep) char *dirpath = NULL; /* #2 */ + _cleanup_close_ int dirfd = -EBADF; /* #1 */ _cleanup_free_ char *current_id = NULL, *devnode = NULL; - _cleanup_close_ int dirfd = -EBADF, lockfd = -EBADF; int r, current_prio; assert(dev); assert(slink); - r = link_update_diskseq(dev, slink, add); - if (r != 0) - return r; - - r = stack_directory_open(dev, slink, &dirfd, &lockfd); + r = stack_directory_open_and_lock(dev, slink, &dirpath, &dirfd, &lockfile); if (r < 0) return r; diff --git a/src/udev/udev-node.h b/src/udev/udev-node.h index 0c545e4a0f..86a829545a 100644 --- a/src/udev/udev-node.h +++ b/src/udev/udev-node.h @@ -24,6 +24,5 @@ int static_node_apply_permissions( int udev_node_remove(sd_device *dev); int udev_node_update(sd_device *dev, sd_device *dev_old); -int udev_node_cleanup(void); size_t udev_node_escape_path(const char *src, char *dest, size_t size); diff --git a/test/units/TEST-17-UDEV.diskseq.sh b/test/units/TEST-17-UDEV.diskseq.sh index 0cb717fbd0..53ee666984 100755 --- a/test/units/TEST-17-UDEV.diskseq.sh +++ b/test/units/TEST-17-UDEV.diskseq.sh @@ -1,6 +1,7 @@ #!/usr/bin/env bash # SPDX-License-Identifier: LGPL-2.1-or-later # shellcheck disable=SC2010 +# shellcheck disable=SC2012 # shellcheck disable=SC2317 set -ex set -o pipefail @@ -43,15 +44,28 @@ systemctl daemon-reload udevadm settle -# If an initrd from the host is used, stack directories for by-diskseq symlinks -# may already exist. Save the number of the directories here. -NUM_DISKSEQ_EXPECTED=$(ls /run/udev/links | grep -c by-diskseq || :) +# Check if no lock file exists, if the lock directory exists. +if [[ -d /run/udev/links.lock/ ]]; then + [[ "$(ls /run/udev/links.lock/ | wc -l)" == 0 ]] +fi + +# Save the current number of the directories. +NUM_DISKSEQ=$(ls /run/udev/links/ | grep -c by-diskseq || :) systemctl start --no-block test-diskseq.service for _ in {0..100}; do sleep .1 - assert_eq "$(ls /run/udev/links | grep -c by-diskseq || :)" "$NUM_DISKSEQ_EXPECTED" + n=$(ls /run/udev/links/ | grep -c by-diskseq || :) + (( n <= NUM_DISKSEQ + 1 )) done +systemctl stop test-diskseq.service || : + +udevadm settle + +# Check if the lock directory exists, but no lock file exists in it. +[[ -d /run/udev/links.lock/ ]] +[[ "$(ls /run/udev/links.lock/ | wc -l)" == 0 ]] + exit 0