From bc5e8ebbfbf2f92622b5efeeb3ca98cbff8bcba3 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 19 Sep 2022 07:03:21 +0900 Subject: [PATCH 1/9] sd-device: use path_hash_ops to store sysattrs As the stored values are actually path. Just for safety. This also drops unnecessary duplication of path. --- src/libsystemd/sd-device/sd-device.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/libsystemd/sd-device/sd-device.c b/src/libsystemd/sd-device/sd-device.c index 13328ffd83..f2556ca4bc 100644 --- a/src/libsystemd/sd-device/sd-device.c +++ b/src/libsystemd/sd-device/sd-device.c @@ -1936,7 +1936,10 @@ static int device_sysattrs_read_all_internal(sd_device *device, const char *subd if ((statbuf.st_mode & (S_IRUSR | S_IWUSR)) == 0) continue; - r = set_put_strdup(&device->sysattrs, p ?: de->d_name); + if (p) + r = set_ensure_consume(&device->sysattrs, &path_hash_ops_free, TAKE_PTR(p)); + else + r = set_put_strdup_full(&device->sysattrs, &path_hash_ops_free, de->d_name); if (r < 0) return r; } From 65c0f14bc16b925f936de39aafa2b2698ee080b5 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 19 Sep 2022 07:08:13 +0900 Subject: [PATCH 2/9] sd-device: use fstatat() No functional changes, just refactoring. --- src/libsystemd/sd-device/sd-device.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/libsystemd/sd-device/sd-device.c b/src/libsystemd/sd-device/sd-device.c index f2556ca4bc..f5f3669402 100644 --- a/src/libsystemd/sd-device/sd-device.c +++ b/src/libsystemd/sd-device/sd-device.c @@ -1901,7 +1901,7 @@ static int device_sysattrs_read_all_internal(sd_device *device, const char *subd return -errno; FOREACH_DIRENT_ALL(de, dir, return -errno) { - _cleanup_free_ char *path = NULL, *p = NULL; + _cleanup_free_ char *p = NULL; struct stat statbuf; if (dot_or_dot_dot(de->d_name)) @@ -1926,11 +1926,7 @@ static int device_sysattrs_read_all_internal(sd_device *device, const char *subd continue; } - path = path_join(syspath, p ?: de->d_name); - if (!path) - return -ENOMEM; - - if (lstat(path, &statbuf) != 0) + if (fstatat(dirfd(dir), de->d_name, &statbuf, AT_SYMLINK_NOFOLLOW) < 0) continue; if ((statbuf.st_mode & (S_IRUSR | S_IWUSR)) == 0) From 5b304c70529ad979548ff761473da63a480cd59f Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 19 Sep 2022 07:23:30 +0900 Subject: [PATCH 3/9] sd-device: use faccessat() No functional changes, just refactoring. --- src/libsystemd/sd-device/sd-device.c | 31 ++++++++++++++-------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/src/libsystemd/sd-device/sd-device.c b/src/libsystemd/sd-device/sd-device.c index f5f3669402..96a8ebf6b8 100644 --- a/src/libsystemd/sd-device/sd-device.c +++ b/src/libsystemd/sd-device/sd-device.c @@ -1877,28 +1877,29 @@ static int device_sysattrs_read_all_internal(sd_device *device, const char *subd return r; if (subdir) { - _cleanup_free_ char *p = NULL; - - p = path_join(syspath, subdir, "uevent"); - if (!p) - return -ENOMEM; - - if (access(p, F_OK) >= 0) - /* this is a child device, skipping */ - return 0; - if (errno != ENOENT) { - log_device_debug_errno(device, errno, "sd-device: Failed to stat %s, ignoring subdir: %m", p); - return 0; - } - path_dir = path_join(syspath, subdir); if (!path_dir) return -ENOMEM; } dir = opendir(path_dir ?: syspath); - if (!dir) + if (!dir) { + if (errno == ENOENT && subdir) + return 0; /* Maybe, this is a child device, and is already removed. */ + return -errno; + } + + if (subdir) { + if (faccessat(dirfd(dir), "uevent", F_OK, 0) >= 0) + return 0; /* this is a child device, skipping */ + if (errno != ENOENT) { + log_device_debug_errno(device, errno, + "sd-device: Failed to access %s/uevent, ignoring sub-directory %s: %m", + subdir, subdir); + return 0; + } + } FOREACH_DIRENT_ALL(de, dir, return -errno) { _cleanup_free_ char *p = NULL; From 4bc9d8165b57b23a2d70f66141cff6877256a613 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 19 Sep 2022 07:04:37 +0900 Subject: [PATCH 4/9] sd-device: re-implement device_sysattrs_read_all() without recursion --- src/libsystemd/sd-device/sd-device.c | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/src/libsystemd/sd-device/sd-device.c b/src/libsystemd/sd-device/sd-device.c index 96a8ebf6b8..fd2bc19323 100644 --- a/src/libsystemd/sd-device/sd-device.c +++ b/src/libsystemd/sd-device/sd-device.c @@ -1866,12 +1866,15 @@ _public_ const char *sd_device_get_property_next(sd_device *device, const char * return key; } -static int device_sysattrs_read_all_internal(sd_device *device, const char *subdir) { +static int device_sysattrs_read_all_internal(sd_device *device, const char *subdir, Set **stack) { _cleanup_free_ char *path_dir = NULL; _cleanup_closedir_ DIR *dir = NULL; const char *syspath; int r; + assert(device); + assert(stack); + r = sd_device_get_syspath(device, &syspath); if (r < 0) return r; @@ -1919,8 +1922,11 @@ static int device_sysattrs_read_all_internal(sd_device *device, const char *subd } if (de->d_type == DT_DIR) { - /* read subdirectory */ - r = device_sysattrs_read_all_internal(device, p ?: de->d_name); + /* push the sub-directory into the stack, and read it later. */ + if (p) + r = set_ensure_consume(stack, &path_hash_ops_free, TAKE_PTR(p)); + else + r = set_put_strdup_full(stack, &path_hash_ops_free, de->d_name); if (r < 0) return r; @@ -1945,6 +1951,7 @@ static int device_sysattrs_read_all_internal(sd_device *device, const char *subd } static int device_sysattrs_read_all(sd_device *device) { + _cleanup_set_free_ Set *stack = NULL; int r; assert(device); @@ -1952,10 +1959,22 @@ static int device_sysattrs_read_all(sd_device *device) { if (device->sysattrs_read) return 0; - r = device_sysattrs_read_all_internal(device, NULL); + r = device_sysattrs_read_all_internal(device, NULL, &stack); if (r < 0) return r; + for (;;) { + _cleanup_free_ char *subdir = NULL; + + subdir = set_steal_first(stack); + if (!subdir) + break; + + r = device_sysattrs_read_all_internal(device, subdir, &stack); + if (r < 0) + return r; + } + device->sysattrs_read = true; return 0; From c42033e7bea6d1eda2391f91b7ff78fcbd9ce948 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 19 Sep 2022 07:32:44 +0900 Subject: [PATCH 5/9] udev: use faccessat() No functional changes, just refactoring. --- src/udev/udev-builtin-net_id.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/udev/udev-builtin-net_id.c b/src/udev/udev-builtin-net_id.c index c1ad190ca8..377ea60722 100644 --- a/src/udev/udev-builtin-net_id.c +++ b/src/udev/udev-builtin-net_id.c @@ -285,9 +285,9 @@ static bool is_pci_bridge(sd_device *dev) { return b; } -static int parse_hotplug_slot_from_function_id(sd_device *dev, const char *slots, uint32_t *ret) { +static int parse_hotplug_slot_from_function_id(sd_device *dev, int slots_dirfd, uint32_t *ret) { uint64_t function_id; - char path[PATH_MAX]; + char filename[NAME_MAX+1]; const char *attr; int r; @@ -300,7 +300,7 @@ static int parse_hotplug_slot_from_function_id(sd_device *dev, const char *slots * between PCI function and its hotplug slot. */ assert(dev); - assert(slots); + assert(slots_dirfd >= 0); assert(ret); if (!naming_scheme_has(NAMING_SLOT_FUNCTION_ID)) @@ -318,12 +318,12 @@ static int parse_hotplug_slot_from_function_id(sd_device *dev, const char *slots "Invalid function id (0x%"PRIx64"), ignoring.", function_id); - if (!snprintf_ok(path, sizeof path, "%s/%08"PRIx64, slots, function_id)) + if (!snprintf_ok(filename, sizeof(filename), "%08"PRIx64, function_id)) return log_device_debug_errno(dev, SYNTHETIC_ERRNO(ENAMETOOLONG), "PCI slot path is too long, ignoring."); - if (access(path, F_OK) < 0) - return log_device_debug_errno(dev, errno, "Cannot access %s, ignoring: %m", path); + if (faccessat(slots_dirfd, filename, F_OK, 0) < 0) + return log_device_debug_errno(dev, errno, "Cannot access %s under pci slots, ignoring: %m", filename); *ret = (uint32_t) function_id; return 1; @@ -423,7 +423,7 @@ static int dev_pci_slot(sd_device *dev, const LinkInfo *info, NetNames *names) { hotplug_slot_dev = names->pcidev; while (hotplug_slot_dev) { - r = parse_hotplug_slot_from_function_id(hotplug_slot_dev, slots, &hotplug_slot); + r = parse_hotplug_slot_from_function_id(hotplug_slot_dev, dirfd(dir), &hotplug_slot); if (r < 0) return 0; if (r > 0) { From a80ce209b99cc23860943147ea990ded4c56a345 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 19 Sep 2022 07:34:49 +0900 Subject: [PATCH 6/9] udev: use sd_device_get_sysattr_value() No functional changes, just refactoring. --- src/udev/udev-builtin-net_id.c | 47 +++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 20 deletions(-) diff --git a/src/udev/udev-builtin-net_id.c b/src/udev/udev-builtin-net_id.c index 377ea60722..76ea34ffa6 100644 --- a/src/udev/udev-builtin-net_id.c +++ b/src/udev/udev-builtin-net_id.c @@ -436,8 +436,8 @@ static int dev_pci_slot(sd_device *dev, const LinkInfo *info, NetNames *names) { return log_device_debug_errno(hotplug_slot_dev, r, "Failed to get sysname: %m"); FOREACH_DIRENT_ALL(de, dir, break) { - _cleanup_free_ char *address = NULL; - char str[PATH_MAX]; + _cleanup_free_ char *path = NULL; + const char *address; uint32_t i; if (dot_or_dot_dot(de->d_name)) @@ -447,30 +447,37 @@ static int dev_pci_slot(sd_device *dev, const LinkInfo *info, NetNames *names) { if (r < 0 || i <= 0) continue; + path = path_join("slots", de->d_name, "address"); + if (!path) + return -ENOMEM; + + if (sd_device_get_sysattr_value(pci, path, &address) < 0) + continue; + /* match slot address with device by stripping the function */ - if (snprintf_ok(str, sizeof str, "%s/%s/address", slots, de->d_name) && - read_one_line_file(str, &address) >= 0 && - startswith(sysname, address)) { - hotplug_slot = i; + if (!startswith(sysname, address)) + continue; - /* We found the match between PCI device and slot. However, we won't use the - * slot index if the device is a PCI bridge, because it can have other child - * devices that will try to claim the same index and that would create name - * collision. */ - if (naming_scheme_has(NAMING_BRIDGE_NO_SLOT) && is_pci_bridge(hotplug_slot_dev)) { - if (naming_scheme_has(NAMING_BRIDGE_MULTIFUNCTION_SLOT) && is_pci_multifunction(names->pcidev) <= 0) { - log_device_debug(dev, "Not using slot information because the PCI device associated with the hotplug slot is a bridge and the PCI device has single function."); - return 0; - } + hotplug_slot = i; - if (!naming_scheme_has(NAMING_BRIDGE_MULTIFUNCTION_SLOT)) { - log_device_debug(dev, "Not using slot information because the PCI device is a bridge."); - return 0; - } + /* We found the match between PCI device and slot. However, we won't use the slot + * index if the device is a PCI bridge, because it can have other child devices that + * will try to claim the same index and that would create name collision. */ + if (naming_scheme_has(NAMING_BRIDGE_NO_SLOT) && is_pci_bridge(hotplug_slot_dev)) { + if (naming_scheme_has(NAMING_BRIDGE_MULTIFUNCTION_SLOT) && is_pci_multifunction(names->pcidev) <= 0) { + log_device_debug(dev, + "Not using slot information because the PCI device associated with " + "the hotplug slot is a bridge and the PCI device has a single function."); + return 0; } - break; + if (!naming_scheme_has(NAMING_BRIDGE_MULTIFUNCTION_SLOT)) { + log_device_debug(dev, "Not using slot information because the PCI device is a bridge."); + return 0; + } } + + break; } if (hotplug_slot > 0) break; From db3049b6f0af4643b2678ad58ffdb93ab736ceaf Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 30 Aug 2022 05:23:05 +0900 Subject: [PATCH 7/9] sd-device: introduce device_opendir() --- src/libsystemd/sd-device/device-private.h | 3 +++ src/libsystemd/sd-device/sd-device.c | 30 +++++++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/src/libsystemd/sd-device/device-private.h b/src/libsystemd/sd-device/device-private.h index 724061a28c..d53479e8c9 100644 --- a/src/libsystemd/sd-device/device-private.h +++ b/src/libsystemd/sd-device/device-private.h @@ -1,6 +1,7 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ #pragma once +#include #include #include #include @@ -14,6 +15,8 @@ int device_new_from_mode_and_devnum(sd_device **ret, mode_t mode, dev_t devnum); int device_new_from_nulstr(sd_device **ret, char *nulstr, size_t len); int device_new_from_strv(sd_device **ret, char **strv); +int device_opendir(sd_device *device, const char *subdir, DIR **ret); + int device_get_property_bool(sd_device *device, const char *key); int device_get_sysattr_unsigned(sd_device *device, const char *sysattr, unsigned *ret_value); int device_get_sysattr_bool(sd_device *device, const char *sysattr); diff --git a/src/libsystemd/sd-device/sd-device.c b/src/libsystemd/sd-device/sd-device.c index fd2bc19323..7a5403be6f 100644 --- a/src/libsystemd/sd-device/sd-device.c +++ b/src/libsystemd/sd-device/sd-device.c @@ -2472,3 +2472,33 @@ _public_ int sd_device_open(sd_device *device, int flags) { return TAKE_FD(fd2); } + +int device_opendir(sd_device *device, const char *subdir, DIR **ret) { + _cleanup_closedir_ DIR *d = NULL; + _cleanup_free_ char *path = NULL; + const char *syspath; + int r; + + assert(device); + assert(ret); + + r = sd_device_get_syspath(device, &syspath); + if (r < 0) + return r; + + if (subdir) { + if (!path_is_safe(subdir)) + return -EINVAL; + + path = path_join(syspath, subdir); + if (!path) + return -ENOMEM; + } + + d = opendir(path ?: syspath); + if (!d) + return -errno; + + *ret = TAKE_PTR(d); + return 0; +} From 62ccd11d38ccd2959c7ab2674890fb44bde5943b Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 19 Sep 2022 07:08:59 +0900 Subject: [PATCH 8/9] sd-device: use device_opendir() --- src/libsystemd/sd-device/sd-device.c | 20 +++----------------- 1 file changed, 3 insertions(+), 17 deletions(-) diff --git a/src/libsystemd/sd-device/sd-device.c b/src/libsystemd/sd-device/sd-device.c index 7a5403be6f..7e3d5eeae1 100644 --- a/src/libsystemd/sd-device/sd-device.c +++ b/src/libsystemd/sd-device/sd-device.c @@ -1867,32 +1867,18 @@ _public_ const char *sd_device_get_property_next(sd_device *device, const char * } static int device_sysattrs_read_all_internal(sd_device *device, const char *subdir, Set **stack) { - _cleanup_free_ char *path_dir = NULL; _cleanup_closedir_ DIR *dir = NULL; - const char *syspath; int r; assert(device); assert(stack); - r = sd_device_get_syspath(device, &syspath); + r = device_opendir(device, subdir, &dir); + if (r == -ENOENT && subdir) + return 0; /* Maybe, this is a child device, and is already removed. */ if (r < 0) return r; - if (subdir) { - path_dir = path_join(syspath, subdir); - if (!path_dir) - return -ENOMEM; - } - - dir = opendir(path_dir ?: syspath); - if (!dir) { - if (errno == ENOENT && subdir) - return 0; /* Maybe, this is a child device, and is already removed. */ - - return -errno; - } - if (subdir) { if (faccessat(dirfd(dir), "uevent", F_OK, 0) >= 0) return 0; /* this is a child device, skipping */ From 97268bdf6fbbe1cec3823861285e99d80cd54aa9 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 19 Sep 2022 07:36:09 +0900 Subject: [PATCH 9/9] udev: use device_opendir() --- src/udev/udev-builtin-net_id.c | 27 ++++++++------------------- 1 file changed, 8 insertions(+), 19 deletions(-) diff --git a/src/udev/udev-builtin-net_id.c b/src/udev/udev-builtin-net_id.c index 76ea34ffa6..4acbc87b96 100644 --- a/src/udev/udev-builtin-net_id.c +++ b/src/udev/udev-builtin-net_id.c @@ -24,6 +24,7 @@ #include "alloc-util.h" #include "chase-symlinks.h" +#include "device-private.h" #include "device-util.h" #include "dirent-util.h" #include "fd-util.h" @@ -98,7 +99,7 @@ static sd_device *skip_virtio(sd_device *dev) { static int get_virtfn_info(sd_device *pcidev, sd_device **ret_physfn_pcidev, char **ret_suffix) { _cleanup_(sd_device_unrefp) sd_device *physfn_pcidev = NULL; - const char *physfn_syspath, *syspath; + const char *syspath; _cleanup_closedir_ DIR *dir = NULL; int r; @@ -115,15 +116,11 @@ static int get_virtfn_info(sd_device *pcidev, sd_device **ret_physfn_pcidev, cha if (r < 0) return r; - r = sd_device_get_syspath(physfn_pcidev, &physfn_syspath); + /* Find the virtual function number by finding the right virtfn link. */ + r = device_opendir(physfn_pcidev, NULL, &dir); if (r < 0) return r; - /* Find the virtual function number by finding the right virtfn link. */ - dir = opendir(physfn_syspath); - if (!dir) - return -errno; - FOREACH_DIRENT_ALL(de, dir, break) { _cleanup_(sd_device_unrefp) sd_device *virtfn_pcidev = NULL; const char *n, *s; @@ -330,15 +327,15 @@ static int parse_hotplug_slot_from_function_id(sd_device *dev, int slots_dirfd, } static int dev_pci_slot(sd_device *dev, const LinkInfo *info, NetNames *names) { - const char *sysname, *attr, *syspath; + const char *sysname, *attr; _cleanup_(sd_device_unrefp) sd_device *pci = NULL; _cleanup_closedir_ DIR *dir = NULL; unsigned domain, bus, slot, func; sd_device *hotplug_slot_dev; unsigned long dev_port = 0; uint32_t hotplug_slot = 0; - char slots[PATH_MAX], *s; size_t l; + char *s; int r; assert(dev); @@ -409,17 +406,9 @@ static int dev_pci_slot(sd_device *dev, const LinkInfo *info, NetNames *names) { if (r < 0) return log_debug_errno(r, "sd_device_new_from_subsystem_sysname() failed: %m"); - r = sd_device_get_syspath(pci, &syspath); + r = device_opendir(pci, "slots", &dir); if (r < 0) - return log_device_debug_errno(pci, r, "sd_device_get_syspath() failed: %m"); - - if (!snprintf_ok(slots, sizeof slots, "%s/slots", syspath)) - return log_device_debug_errno(dev, SYNTHETIC_ERRNO(ENAMETOOLONG), - "Cannot access %s/slots: %m", syspath); - - dir = opendir(slots); - if (!dir) - return log_device_debug_errno(dev, errno, "Cannot access %s: %m", slots); + return log_device_debug_errno(dev, r, "Cannot access 'slots' subdirectory: %m"); hotplug_slot_dev = names->pcidev; while (hotplug_slot_dev) {