From 7727b694db7b8e83b0f9991f197b78f35bd065e0 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 10 Oct 2024 10:33:22 +0900 Subject: [PATCH 1/4] udev-node: remove lockfile and stack directory when not necessary if possible Replaces 09373c1a50297079e6b0447ea97af4e9a60f77fa. Let's remove stack directories and their lock files by workers if possible. Now, lock files must be created before creating stack directories, hence lock files are moved to /run/udev/links.lock/ , e.g., Before: /run/udev/links/disk\x2fby-diskseq\x2f1/.lock After: /run/udev/links.lock/disk\x2fby-diskseq\x2f1 Fixes ##34637. --- src/udev/udev-node.c | 73 ++++++++++++++++++++++++++++---------------- 1 file changed, 47 insertions(+), 26 deletions(-) diff --git a/src/udev/udev-node.c b/src/udev/udev-node.c index a0bf0a9619..589a56e502 100644 --- a/src/udev/udev-node.c +++ b/src/udev/udev-node.c @@ -339,7 +339,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 +360,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; } @@ -514,8 +528,15 @@ static int link_update_diskseq(sd_device *dev, const char *slink, bool add) { } 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); @@ -525,7 +546,7 @@ static int link_update(sd_device *dev, const char *slink, bool 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; From b364af59f0afb01132a497abeb57dd20a6f357b1 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 10 Oct 2024 10:36:46 +0900 Subject: [PATCH 2/4] udev-node: drop workaround for by-diskseq symlinks This effectively reverts 09373c1a50297079e6b0447ea97af4e9a60f77fa, as stack directories and lock files are removed by udev workers on unlocking. --- src/udev/udev-node.c | 83 ------------------------------ test/units/TEST-17-UDEV.diskseq.sh | 8 +-- 2 files changed, 4 insertions(+), 87 deletions(-) diff --git a/src/udev/udev-node.c b/src/udev/udev-node.c index 589a56e502..6d917c4dd2 100644 --- a/src/udev/udev-node.c +++ b/src/udev/udev-node.c @@ -448,85 +448,6 @@ 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, @@ -542,10 +463,6 @@ static int link_update(sd_device *dev, const char *slink, bool add) { assert(dev); assert(slink); - r = link_update_diskseq(dev, slink, add); - if (r != 0) - return r; - r = stack_directory_open_and_lock(dev, slink, &dirpath, &dirfd, &lockfile); if (r < 0) return r; diff --git a/test/units/TEST-17-UDEV.diskseq.sh b/test/units/TEST-17-UDEV.diskseq.sh index 0cb717fbd0..1aafbe9834 100755 --- a/test/units/TEST-17-UDEV.diskseq.sh +++ b/test/units/TEST-17-UDEV.diskseq.sh @@ -43,15 +43,15 @@ 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 || :) +# 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 exit 0 From ea4af553d8c0fd9669d329d89e329d7270102032 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 10 Oct 2024 10:42:24 +0900 Subject: [PATCH 3/4] udev-node: drop unnecessary manager side cleaning up logic for stack directory Unnecessary stack directories and their lock files are removed by workers. Hence, the logic is not necessary anymore. --- src/udev/udev-manager.c | 8 -------- src/udev/udev-manager.h | 1 - src/udev/udev-node.c | 42 ----------------------------------------- src/udev/udev-node.h | 1 - 4 files changed, 52 deletions(-) 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 6d917c4dd2..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); 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); From 5a22b27b5c5ba5bef9afe06060bd16164b7082eb Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 11 Oct 2024 03:37:01 +0900 Subject: [PATCH 4/4] TEST-17-UDEV: also check if /run/udev/links.lock/ is empty on settle --- test/units/TEST-17-UDEV.diskseq.sh | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/test/units/TEST-17-UDEV.diskseq.sh b/test/units/TEST-17-UDEV.diskseq.sh index 1aafbe9834..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,6 +44,11 @@ systemctl daemon-reload udevadm settle +# 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 || :) @@ -54,4 +60,12 @@ for _ in {0..100}; do (( 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