From b08c3fbe0e3f310b520d17be92110b4cb96a5f2c Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 7 Apr 2021 19:09:50 +0900 Subject: [PATCH 1/7] udev: add missing initialization to fix freeing invalid address --- src/udev/udev-builtin-net_id.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/udev/udev-builtin-net_id.c b/src/udev/udev-builtin-net_id.c index 66c52e624c..53653c492a 100644 --- a/src/udev/udev-builtin-net_id.c +++ b/src/udev/udev-builtin-net_id.c @@ -358,8 +358,8 @@ static int dev_pci_slot(sd_device *dev, struct netnames *names) { */ if (naming_scheme_has(NAMING_SLOT_FUNCTION_ID) && sd_device_get_sysattr_value(hotplug_slot_dev, "function_id", &attr) >= 0) { + _cleanup_free_ char *str = NULL; int function_id; - _cleanup_free_ char *str; if (safe_atoi(attr, &function_id) >= 0 && asprintf(&str, "%s/%08x/", slots, function_id) >= 0 && From 70c35e4bfd64f24c7cb3536bdf63af537e0f2971 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 7 Apr 2021 19:19:45 +0900 Subject: [PATCH 2/7] udev: it is not necessary that the path is readable --- src/udev/udev-builtin-net_id.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/udev/udev-builtin-net_id.c b/src/udev/udev-builtin-net_id.c index 53653c492a..c6f2e6ddf5 100644 --- a/src/udev/udev-builtin-net_id.c +++ b/src/udev/udev-builtin-net_id.c @@ -363,7 +363,7 @@ static int dev_pci_slot(sd_device *dev, struct netnames *names) { if (safe_atoi(attr, &function_id) >= 0 && asprintf(&str, "%s/%08x/", slots, function_id) >= 0 && - access(str, R_OK) == 0) { + access(str, F_OK) == 0) { hotplug_slot = function_id; domain = 0; } else From fa28023c0fd3b474eb3fbb747f0202aaed73db15 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 7 Apr 2021 22:11:00 +0900 Subject: [PATCH 3/7] udev: split out logic of parsing s390 PCI slots This also adds several debugging logs. --- src/udev/udev-builtin-net_id.c | 72 ++++++++++++++++++++++------------ 1 file changed, 46 insertions(+), 26 deletions(-) diff --git a/src/udev/udev-builtin-net_id.c b/src/udev/udev-builtin-net_id.c index c6f2e6ddf5..6be57db7c7 100644 --- a/src/udev/udev-builtin-net_id.c +++ b/src/udev/udev-builtin-net_id.c @@ -23,6 +23,7 @@ #include #include "alloc-util.h" +#include "device-util.h" #include "dirent-util.h" #include "fd-util.h" #include "fileio.h" @@ -262,6 +263,43 @@ static bool is_pci_bridge(sd_device *dev) { return strneq(p + 2, "04", 2); } +static int parse_hotplug_slot_from_function_id(sd_device *dev, const char *slots, int *ret) { + char path[PATH_MAX]; + const char *attr; + int function_id, r; + + /* The /function_id attribute is unique to the s390 PCI driver. If present, we know + * that the slot's directory name for this device is /sys/bus/pci/XXXXXXXX/ where XXXXXXXX is + * the fixed length 8 hexadecimal character string representation of function_id. Therefore we + * can short cut here and just check for the existence of the slot directory. As this directory + * has to exist, we're emitting a debug message for the unlikely case it's not found. Note that + * the domain part doesn't belong to the slot name here because there's a 1-to-1 relationship + * between PCI function and its hotplug slot. */ + + assert(dev); + assert(slots); + assert(ret); + + if (!naming_scheme_has(NAMING_SLOT_FUNCTION_ID)) + return 0; + + if (sd_device_get_sysattr_value(dev, "function_id", &attr) < 0) + return 0; + + r = safe_atoi(attr, &function_id); + if (r < 0) + return log_device_debug_errno(dev, r, "Failed to parse function_id, ignoring: %s", attr); + + if (!snprintf_ok(path, sizeof path, "%s/%08x/", slots, 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); + + *ret = function_id; + return 1; +} + static int dev_pci_slot(sd_device *dev, struct netnames *names) { unsigned long dev_port = 0; unsigned domain, bus, slot, func; @@ -342,35 +380,17 @@ static int dev_pci_slot(sd_device *dev, struct netnames *names) { hotplug_slot_dev = names->pcidev; while (hotplug_slot_dev) { - if (sd_device_get_sysname(hotplug_slot_dev, &sysname) < 0) - continue; - - /* The /function_id attribute is unique to the s390 PCI driver. - If present, we know that the slot's directory name for this device is - /sys/bus/pci/XXXXXXXX/ where XXXXXXXX is the fixed length 8 hexadecimal - character string representation of function_id. - Therefore we can short cut here and just check for the existence of - the slot directory. As this directory has to exist, we're emitting a - debug message for the unlikely case it's not found. - Note that the domain part of doesn't belong to the slot name here - because there's a 1-to-1 relationship between PCI function and its hotplug - slot. - */ - if (naming_scheme_has(NAMING_SLOT_FUNCTION_ID) && - sd_device_get_sysattr_value(hotplug_slot_dev, "function_id", &attr) >= 0) { - _cleanup_free_ char *str = NULL; - int function_id; - - if (safe_atoi(attr, &function_id) >= 0 && - asprintf(&str, "%s/%08x/", slots, function_id) >= 0 && - access(str, F_OK) == 0) { - hotplug_slot = function_id; - domain = 0; - } else - log_debug("No matching slot for function_id (%s).", attr); + r = parse_hotplug_slot_from_function_id(hotplug_slot_dev, slots, &hotplug_slot); + if (r < 0) + return 0; + if (r > 0) { + domain = 0; /* See comments in parse_hotplug_slot_from_function_id(). */ break; } + if (sd_device_get_sysname(hotplug_slot_dev, &sysname) < 0) + continue; + FOREACH_DIRENT_ALL(dent, dir, break) { int i; char str[PATH_MAX]; From c304cb0146f2dea72f8bc3244bf9dd4ca52fa44f Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 7 Apr 2021 19:17:23 +0900 Subject: [PATCH 4/7] udev: use uint32_t for hotplug_slot This also makes function id is parsed as uint64_t. Kernel internally uses uint32_t for function id (see the definition of 'struct zpci_dev), but it maybe extended in the future. --- src/udev/udev-builtin-net_id.c | 45 ++++++++++++++++++++-------------- 1 file changed, 26 insertions(+), 19 deletions(-) diff --git a/src/udev/udev-builtin-net_id.c b/src/udev/udev-builtin-net_id.c index 6be57db7c7..fa7f162a57 100644 --- a/src/udev/udev-builtin-net_id.c +++ b/src/udev/udev-builtin-net_id.c @@ -263,10 +263,11 @@ static bool is_pci_bridge(sd_device *dev) { return strneq(p + 2, "04", 2); } -static int parse_hotplug_slot_from_function_id(sd_device *dev, const char *slots, int *ret) { +static int parse_hotplug_slot_from_function_id(sd_device *dev, const char *slots, uint32_t *ret) { + uint64_t function_id; char path[PATH_MAX]; const char *attr; - int function_id, r; + int r; /* The /function_id attribute is unique to the s390 PCI driver. If present, we know * that the slot's directory name for this device is /sys/bus/pci/XXXXXXXX/ where XXXXXXXX is @@ -286,32 +287,36 @@ static int parse_hotplug_slot_from_function_id(sd_device *dev, const char *slots if (sd_device_get_sysattr_value(dev, "function_id", &attr) < 0) return 0; - r = safe_atoi(attr, &function_id); + r = safe_atou64(attr, &function_id); if (r < 0) return log_device_debug_errno(dev, r, "Failed to parse function_id, ignoring: %s", attr); - if (!snprintf_ok(path, sizeof path, "%s/%08x/", slots, function_id)) - return log_device_debug_errno(dev, SYNTHETIC_ERRNO(ENAMETOOLONG), "PCI slot path is too long, ignoring."); + if (function_id <= 0 || function_id > UINT32_MAX) + return log_device_debug_errno(dev, SYNTHETIC_ERRNO(EINVAL), + "Invalid function id (0x%"PRIx64"), ignoring.", + function_id); + + if (!snprintf_ok(path, sizeof path, "%s/%08"PRIx64, slots, 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); - *ret = function_id; + *ret = (uint32_t) function_id; return 1; } static int dev_pci_slot(sd_device *dev, struct netnames *names) { - unsigned long dev_port = 0; - unsigned domain, bus, slot, func; - int hotplug_slot = -1; - size_t l; - char *s; const char *sysname, *attr, *port_name = NULL, *syspath; _cleanup_(sd_device_unrefp) sd_device *pci = NULL; - sd_device *hotplug_slot_dev; - char slots[PATH_MAX]; _cleanup_closedir_ DIR *dir = NULL; - struct dirent *dent; + 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; int r; r = sd_device_get_sysname(names->pcidev, &sysname); @@ -380,6 +385,8 @@ static int dev_pci_slot(sd_device *dev, struct netnames *names) { hotplug_slot_dev = names->pcidev; while (hotplug_slot_dev) { + struct dirent *dent; + r = parse_hotplug_slot_from_function_id(hotplug_slot_dev, slots, &hotplug_slot); if (r < 0) return 0; @@ -392,14 +399,14 @@ static int dev_pci_slot(sd_device *dev, struct netnames *names) { continue; FOREACH_DIRENT_ALL(dent, dir, break) { - int i; - char str[PATH_MAX]; _cleanup_free_ char *address = NULL; + char str[PATH_MAX]; + uint32_t i; if (dot_or_dot_dot(dent->d_name)) continue; - r = safe_atoi(dent->d_name, &i); + r = safe_atou32(dent->d_name, &i); if (r < 0 || i <= 0) continue; @@ -419,7 +426,7 @@ static int dev_pci_slot(sd_device *dev, struct netnames *names) { break; } } - if (hotplug_slot >= 0) + if (hotplug_slot > 0) break; if (sd_device_get_parent_with_subsystem_devtype(hotplug_slot_dev, "pci", NULL, &hotplug_slot_dev) < 0) break; @@ -431,7 +438,7 @@ static int dev_pci_slot(sd_device *dev, struct netnames *names) { l = sizeof(names->pci_slot); if (domain > 0) l = strpcpyf(&s, l, "P%d", domain); - l = strpcpyf(&s, l, "s%d", hotplug_slot); + l = strpcpyf(&s, l, "s%"PRIu32, hotplug_slot); if (func > 0 || is_pci_multifunction(names->pcidev)) l = strpcpyf(&s, l, "f%d", func); if (port_name) From 19ed9a114cfc936f6301b4f784ff15aea0d2b872 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 7 Apr 2021 22:26:41 +0900 Subject: [PATCH 5/7] udev: make dev_pci_slot() return earlier when PCI bridge is found --- src/udev/udev-builtin-net_id.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/udev/udev-builtin-net_id.c b/src/udev/udev-builtin-net_id.c index fa7f162a57..f4b200ea42 100644 --- a/src/udev/udev-builtin-net_id.c +++ b/src/udev/udev-builtin-net_id.c @@ -421,7 +421,7 @@ static int dev_pci_slot(sd_device *dev, struct netnames *names) { * 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)) - hotplug_slot = 0; + return 0; break; } From 5a6e56ec1703e84f7860152e483da816ca4b1cbb Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 7 Apr 2021 22:36:12 +0900 Subject: [PATCH 6/7] udev: fix potential infinite loop --- src/udev/udev-builtin-net_id.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/udev/udev-builtin-net_id.c b/src/udev/udev-builtin-net_id.c index f4b200ea42..f84c2669a8 100644 --- a/src/udev/udev-builtin-net_id.c +++ b/src/udev/udev-builtin-net_id.c @@ -395,8 +395,9 @@ static int dev_pci_slot(sd_device *dev, struct netnames *names) { break; } - if (sd_device_get_sysname(hotplug_slot_dev, &sysname) < 0) - continue; + r = sd_device_get_sysname(hotplug_slot_dev, &sysname); + if (r < 0) + return log_device_debug_errno(hotplug_slot_dev, r, "Failed to get sysname: %m"); FOREACH_DIRENT_ALL(dent, dir, break) { _cleanup_free_ char *address = NULL; From cb8c948738e65a8c33697ee83efdacd1aee88311 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 7 Apr 2021 22:22:22 +0900 Subject: [PATCH 7/7] udev: use snprintf_ok() --- src/udev/udev-builtin-net_id.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/udev/udev-builtin-net_id.c b/src/udev/udev-builtin-net_id.c index f84c2669a8..088bfe38d9 100644 --- a/src/udev/udev-builtin-net_id.c +++ b/src/udev/udev-builtin-net_id.c @@ -999,7 +999,7 @@ static int builtin_net_id(sd_device *dev, int argc, char *argv[], bool test) { udev_builtin_add_property(dev, test, "ID_NET_NAME_PATH", str); if (names.pci_slot[0] && - snprintf(str, sizeof str, "%s%s%s", prefix, names.pci_slot, names.bcma_core)) + snprintf_ok(str, sizeof str, "%s%s%s", prefix, names.pci_slot, names.bcma_core)) udev_builtin_add_property(dev, test, "ID_NET_NAME_SLOT", str); return 0; }