diff --git a/src/core/device.c b/src/core/device.c index 6e07f2745b4..ec018a4c442 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -1124,19 +1124,38 @@ static int device_dispatch_io(sd_device_monitor *monitor, sd_device *dev, void * r = sd_device_get_syspath(dev, &sysfs); if (r < 0) { - log_device_error_errno(dev, r, "Failed to get device syspath, ignoring: %m"); + log_device_warning_errno(dev, r, "Failed to get device syspath, ignoring: %m"); return 0; } r = sd_device_get_action(dev, &action); if (r < 0) { - log_device_error_errno(dev, r, "Failed to get udev action, ignoring: %m"); + log_device_warning_errno(dev, r, "Failed to get udev action, ignoring: %m"); return 0; } if (action == SD_DEVICE_MOVE) device_remove_old_on_move(m, dev); + /* When udevd failed to process the device, SYSTEMD_ALIAS or any other properties may contain invalid + * values. Let's refuse to handle the uevent. */ + if (sd_device_get_property_value(dev, "UDEV_WORKER_FAILED", NULL) >= 0) { + int v; + + if (device_get_property_int(dev, "UDEV_WORKER_ERRNO", &v) >= 0) + log_device_warning_errno(dev, v, "systemd-udevd failed to process the device, ignoring: %m"); + else if (device_get_property_int(dev, "UDEV_WORKER_EXIT_STATUS", &v) >= 0) + log_device_warning(dev, "systemd-udevd failed to process the device with exit status %i, ignoring.", v); + else if (device_get_property_int(dev, "UDEV_WORKER_SIGNAL", &v) >= 0) { + const char *s; + (void) sd_device_get_property_value(dev, "UDEV_WORKER_SIGNAL_NAME", &s); + log_device_warning(dev, "systemd-udevd failed to process the device with signal %i(%s), ignoring.", v, strna(s)); + } else + log_device_warning(dev, "systemd-udevd failed to process the device with unknown result, ignoring."); + + return 0; + } + /* A change event can signal that a device is becoming ready, in particular if the device is using * the SYSTEMD_READY logic in udev so we need to reach the else block of the following if, even for * change events */ diff --git a/src/libsystemd/sd-device/device-private.c b/src/libsystemd/sd-device/device-private.c index 7cda48c4caf..182c4d66ff7 100644 --- a/src/libsystemd/sd-device/device-private.c +++ b/src/libsystemd/sd-device/device-private.c @@ -618,51 +618,6 @@ int device_get_devlink_priority(sd_device *device, int *ret) { return 0; } -int device_rename(sd_device *device, const char *name) { - _cleanup_free_ char *new_syspath = NULL; - const char *interface; - int r; - - assert(device); - assert(name); - - if (!filename_is_valid(name)) - return -EINVAL; - - r = path_extract_directory(device->syspath, &new_syspath); - if (r < 0) - return r; - - if (!path_extend(&new_syspath, name)) - return -ENOMEM; - - if (!path_is_safe(new_syspath)) - return -EINVAL; - - /* At the time this is called, the renamed device may not exist yet. Hence, we cannot validate - * the new syspath. */ - r = device_set_syspath(device, new_syspath, false); - if (r < 0) - return r; - - /* Here, only clear the sysname and sysnum. They will be set when requested. */ - device->sysnum = NULL; - device->sysname = mfree(device->sysname); - - r = sd_device_get_property_value(device, "INTERFACE", &interface); - if (r == -ENOENT) - return 0; - if (r < 0) - return r; - - /* like DEVPATH_OLD, INTERFACE_OLD is not saved to the db, but only stays around for the current event */ - r = device_add_property_internal(device, "INTERFACE_OLD", interface); - if (r < 0) - return r; - - return device_add_property_internal(device, "INTERFACE", name); -} - static int device_shallow_clone(sd_device *device, sd_device **ret) { _cleanup_(sd_device_unrefp) sd_device *dest = NULL; const char *val = NULL; diff --git a/src/libsystemd/sd-device/device-private.h b/src/libsystemd/sd-device/device-private.h index a59f130affc..d9a519a4d98 100644 --- a/src/libsystemd/sd-device/device-private.h +++ b/src/libsystemd/sd-device/device-private.h @@ -18,6 +18,7 @@ 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_property_int(sd_device *device, const char *key, int *ret); int device_get_sysattr_int(sd_device *device, const char *sysattr, int *ret_value); 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); @@ -53,7 +54,6 @@ int device_properties_prepare(sd_device *device); int device_get_properties_nulstr(sd_device *device, const char **ret_nulstr, size_t *ret_len); int device_get_properties_strv(sd_device *device, char ***ret); -int device_rename(sd_device *device, const char *name); int device_clone_with_db(sd_device *device, sd_device **ret); int device_tag_index(sd_device *dev, sd_device *dev_old, bool add); diff --git a/src/libsystemd/sd-device/sd-device.c b/src/libsystemd/sd-device/sd-device.c index 2fcdce7bac9..c3160b04bb7 100644 --- a/src/libsystemd/sd-device/sd-device.c +++ b/src/libsystemd/sd-device/sd-device.c @@ -249,6 +249,10 @@ int device_set_syspath(sd_device *device, const char *_syspath, bool verify) { free_and_replace(device->syspath, syspath); device->devpath = devpath; + + /* Unset sysname and sysnum, they will be assigned when requested. */ + device->sysnum = NULL; + device->sysname = mfree(device->sysname); return 0; } @@ -2181,6 +2185,26 @@ int device_get_property_bool(sd_device *device, const char *key) { return parse_boolean(value); } +int device_get_property_int(sd_device *device, const char *key, int *ret) { + const char *value; + int r, v; + + assert(device); + assert(key); + + r = sd_device_get_property_value(device, key, &value); + if (r < 0) + return r; + + r = safe_atoi(value, &v); + if (r < 0) + return r; + + if (ret) + *ret = v; + return 0; +} + _public_ int sd_device_get_trigger_uuid(sd_device *device, sd_id128_t *ret) { const char *s; sd_id128_t id; diff --git a/src/udev/udev-event.c b/src/udev/udev-event.c index 7a59e7c759c..6d47a2a49de 100644 --- a/src/udev/udev-event.c +++ b/src/udev/udev-event.c @@ -12,6 +12,7 @@ #include "sd-event.h" #include "alloc-util.h" +#include "device-internal.h" #include "device-private.h" #include "device-util.h" #include "fd-util.h" @@ -119,24 +120,24 @@ struct subst_map_entry { }; static const struct subst_map_entry map[] = { - { .name = "devnode", .fmt = 'N', .type = FORMAT_SUBST_DEVNODE }, - { .name = "tempnode", .fmt = 'N', .type = FORMAT_SUBST_DEVNODE }, /* deprecated */ - { .name = "attr", .fmt = 's', .type = FORMAT_SUBST_ATTR }, - { .name = "sysfs", .fmt = 's', .type = FORMAT_SUBST_ATTR }, /* deprecated */ - { .name = "env", .fmt = 'E', .type = FORMAT_SUBST_ENV }, - { .name = "kernel", .fmt = 'k', .type = FORMAT_SUBST_KERNEL }, + { .name = "devnode", .fmt = 'N', .type = FORMAT_SUBST_DEVNODE }, + { .name = "tempnode", .fmt = 'N', .type = FORMAT_SUBST_DEVNODE }, /* deprecated */ + { .name = "attr", .fmt = 's', .type = FORMAT_SUBST_ATTR }, + { .name = "sysfs", .fmt = 's', .type = FORMAT_SUBST_ATTR }, /* deprecated */ + { .name = "env", .fmt = 'E', .type = FORMAT_SUBST_ENV }, + { .name = "kernel", .fmt = 'k', .type = FORMAT_SUBST_KERNEL }, { .name = "number", .fmt = 'n', .type = FORMAT_SUBST_KERNEL_NUMBER }, - { .name = "driver", .fmt = 'd', .type = FORMAT_SUBST_DRIVER }, - { .name = "devpath", .fmt = 'p', .type = FORMAT_SUBST_DEVPATH }, - { .name = "id", .fmt = 'b', .type = FORMAT_SUBST_ID }, - { .name = "major", .fmt = 'M', .type = FORMAT_SUBST_MAJOR }, - { .name = "minor", .fmt = 'm', .type = FORMAT_SUBST_MINOR }, - { .name = "result", .fmt = 'c', .type = FORMAT_SUBST_RESULT }, - { .name = "parent", .fmt = 'P', .type = FORMAT_SUBST_PARENT }, - { .name = "name", .fmt = 'D', .type = FORMAT_SUBST_NAME }, - { .name = "links", .fmt = 'L', .type = FORMAT_SUBST_LINKS }, - { .name = "root", .fmt = 'r', .type = FORMAT_SUBST_ROOT }, - { .name = "sys", .fmt = 'S', .type = FORMAT_SUBST_SYS }, + { .name = "driver", .fmt = 'd', .type = FORMAT_SUBST_DRIVER }, + { .name = "devpath", .fmt = 'p', .type = FORMAT_SUBST_DEVPATH }, + { .name = "id", .fmt = 'b', .type = FORMAT_SUBST_ID }, + { .name = "major", .fmt = 'M', .type = FORMAT_SUBST_MAJOR }, + { .name = "minor", .fmt = 'm', .type = FORMAT_SUBST_MINOR }, + { .name = "result", .fmt = 'c', .type = FORMAT_SUBST_RESULT }, + { .name = "parent", .fmt = 'P', .type = FORMAT_SUBST_PARENT }, + { .name = "name", .fmt = 'D', .type = FORMAT_SUBST_NAME }, + { .name = "links", .fmt = 'L', .type = FORMAT_SUBST_LINKS }, + { .name = "root", .fmt = 'r', .type = FORMAT_SUBST_ROOT }, + { .name = "sys", .fmt = 'S', .type = FORMAT_SUBST_SYS }, }; static const char *format_type_to_string(FormatSubstitutionType t) { @@ -859,8 +860,54 @@ int udev_event_spawn( return r; /* 0 for success, and positive if the program failed */ } +static int device_rename(sd_device *device, const char *name) { + _cleanup_free_ char *new_syspath = NULL; + const char *s; + int r; + + assert(device); + assert(name); + + if (!filename_is_valid(name)) + return -EINVAL; + + r = sd_device_get_syspath(device, &s); + if (r < 0) + return r; + + r = path_extract_directory(s, &new_syspath); + if (r < 0) + return r; + + if (!path_extend(&new_syspath, name)) + return -ENOMEM; + + if (!path_is_safe(new_syspath)) + return -EINVAL; + + /* At the time this is called, the renamed device may not exist yet. Hence, we cannot validate + * the new syspath. */ + r = device_set_syspath(device, new_syspath, /* verify = */ false); + if (r < 0) + return r; + + r = sd_device_get_property_value(device, "INTERFACE", &s); + if (r == -ENOENT) + return 0; + if (r < 0) + return r; + + /* like DEVPATH_OLD, INTERFACE_OLD is not saved to the db, but only stays around for the current event */ + r = device_add_property_internal(device, "INTERFACE_OLD", s); + if (r < 0) + return r; + + return device_add_property_internal(device, "INTERFACE", name); +} + static int rename_netif(UdevEvent *event) { - const char *oldname; + _cleanup_free_ char *old_syspath = NULL, *old_sysname = NULL; + const char *s; sd_device *dev; int ifindex, r; @@ -871,15 +918,6 @@ static int rename_netif(UdevEvent *event) { dev = ASSERT_PTR(event->dev); - /* Read sysname from cloned sd-device object, otherwise use-after-free is triggered, as the - * main object will be renamed and dev->sysname will be freed in device_rename(). */ - r = sd_device_get_sysname(event->dev_db_clone, &oldname); - if (r < 0) - return log_device_error_errno(dev, r, "Failed to get sysname: %m"); - - if (streq(event->name, oldname)) - return 0; /* The interface name is already requested name. */ - if (!device_for_action(dev, SD_DEVICE_ADD)) return 0; /* Rename the interface only when it is added. */ @@ -887,7 +925,7 @@ static int rename_netif(UdevEvent *event) { if (r == -ENOENT) return 0; /* Device is not a network interface. */ if (r < 0) - return log_device_error_errno(dev, r, "Failed to get ifindex: %m"); + return log_device_warning_errno(dev, r, "Failed to get ifindex: %m"); if (naming_scheme_has(NAMING_REPLACE_STRICTLY) && !ifname_valid(event->name)) { @@ -895,39 +933,82 @@ static int rename_netif(UdevEvent *event) { return 0; } - /* Set ID_RENAMING boolean property here. It will be dropped when the corresponding move uevent is processed. */ - r = device_add_property(dev, "ID_RENAMING", "1"); + r = sd_device_get_sysname(dev, &s); if (r < 0) - return log_device_warning_errno(dev, r, "Failed to add 'ID_RENAMING' property: %m"); + return log_device_warning_errno(dev, r, "Failed to get sysname: %m"); + + if (streq(event->name, s)) + return 0; /* The interface name is already requested name. */ + + old_sysname = strdup(s); + if (!old_sysname) + return -ENOMEM; + + r = sd_device_get_syspath(dev, &s); + if (r < 0) + return log_device_warning_errno(dev, r, "Failed to get syspath: %m"); + + old_syspath = strdup(s); + if (!old_syspath) + return -ENOMEM; r = device_rename(dev, event->name); - if (r < 0) - return log_device_warning_errno(dev, r, "Failed to update properties with new name '%s': %m", event->name); + if (r < 0) { + log_device_warning_errno(dev, r, "Failed to update properties with new name '%s': %m", event->name); + goto revert; + } + + /* Set ID_RENAMING boolean property here. It will be dropped when the corresponding move uevent is processed. */ + r = device_add_property(dev, "ID_RENAMING", "1"); + if (r < 0) { + log_device_warning_errno(dev, r, "Failed to add 'ID_RENAMING' property: %m"); + goto revert; + } /* Also set ID_RENAMING boolean property to cloned sd_device object and save it to database * before calling rtnl_set_link_name(). Otherwise, clients (e.g., systemd-networkd) may receive * RTM_NEWLINK netlink message before the database is updated. */ r = device_add_property(event->dev_db_clone, "ID_RENAMING", "1"); - if (r < 0) - return log_device_warning_errno(event->dev_db_clone, r, "Failed to add 'ID_RENAMING' property: %m"); + if (r < 0) { + log_device_warning_errno(event->dev_db_clone, r, "Failed to add 'ID_RENAMING' property: %m"); + goto revert; + } r = device_update_db(event->dev_db_clone); - if (r < 0) - return log_device_debug_errno(event->dev_db_clone, r, "Failed to update database under /run/udev/data/: %m"); + if (r < 0) { + log_device_debug_errno(event->dev_db_clone, r, "Failed to update database under /run/udev/data/: %m"); + goto revert; + } r = rtnl_set_link_name(&event->rtnl, ifindex, event->name); - if (r == -EBUSY) { - log_device_info(dev, "Network interface '%s' is already up, cannot rename to '%s'.", - oldname, event->name); - return 0; + if (r < 0) { + if (r == -EBUSY) { + log_device_info(dev, "Network interface '%s' is already up, cannot rename to '%s'.", + old_sysname, event->name); + r = 0; + } else + log_device_error_errno(dev, r, "Failed to rename network interface %i from '%s' to '%s': %m", + ifindex, old_sysname, event->name); + goto revert; } - if (r < 0) - return log_device_error_errno(dev, r, "Failed to rename network interface %i from '%s' to '%s': %m", - ifindex, oldname, event->name); - - log_device_debug(dev, "Network interface %i is renamed from '%s' to '%s'", ifindex, oldname, event->name); + log_device_debug(dev, "Network interface %i is renamed from '%s' to '%s'", ifindex, old_sysname, event->name); return 1; + +revert: + /* Restore 'dev_db_clone' */ + (void) device_add_property(event->dev_db_clone, "ID_RENAMING", NULL); + (void) device_update_db(event->dev_db_clone); + + /* Restore 'dev' */ + (void) device_set_syspath(dev, old_syspath, /* verify = */ false); + if (sd_device_get_property_value(dev, "INTERFACE_OLD", &s) >= 0) { + (void) device_add_property_internal(dev, "INTERFACE", s); + (void) device_add_property_internal(dev, "INTERFACE_OLD", NULL); + } + (void) device_add_property(dev, "ID_RENAMING", NULL); + + return r; } static int update_devnode(UdevEvent *event) { diff --git a/test/units/testsuite-17.02.sh b/test/units/testsuite-17.02.sh index 7abbce77476..ed3b39d074a 100755 --- a/test/units/testsuite-17.02.sh +++ b/test/units/testsuite-17.02.sh @@ -102,4 +102,82 @@ timeout 30 bash -c 'while [[ "$(systemctl show --property=ActiveState --value /s # cleanup ip link del hoge +teardown_netif_renaming_conflict() { + set +ex + + if [[ -n "$KILL_PID" ]]; then + kill "$KILL_PID" + fi + + rm -rf "$TMPDIR" + + rm -f /run/udev/rules.d/50-testsuite.rules + udevadm control --reload --timeout=30 + + ip link del hoge + ip link del foobar +} + +test_netif_renaming_conflict() { + local since found= + + trap teardown_netif_renaming_conflict RETURN + + cat >/run/udev/rules.d/50-testsuite.rules <"$TMPDIR"/monitor.txt & + KILL_PID="$!" + + # make sure that 'udevadm monitor' actually monitor uevents + sleep 1 + + since="$(date '+%H:%M:%S')" + + # add another interface which will conflict with an existing interface + ip link add foobar type dummy + + for _ in {1..40}; do + if ( + grep -q 'ACTION=add' "$TMPDIR"/monitor.txt + grep -q 'DEVPATH=/devices/virtual/net/foobar' "$TMPDIR"/monitor.txt + grep -q 'SUBSYSTEM=net' "$TMPDIR"/monitor.txt + grep -q 'INTERFACE=foobar' "$TMPDIR"/monitor.txt + grep -q 'ID_NET_DRIVER=dummy' "$TMPDIR"/monitor.txt + grep -q 'ID_NET_NAME=foobar' "$TMPDIR"/monitor.txt + # Even when network interface renaming is failed, SYSTEMD_ALIAS with the conflicting name will be broadcast. + grep -q 'SYSTEMD_ALIAS=/sys/subsystem/net/devices/hoge' "$TMPDIR"/monitor.txt + grep -q 'UDEV_WORKER_FAILED=1' "$TMPDIR"/monitor.txt + grep -q 'UDEV_WORKER_ERRNO=17' "$TMPDIR"/monitor.txt + grep -q 'UDEV_WORKER_ERRNO_NAME=EEXIST' "$TMPDIR"/monitor.txt + ); then + cat "$TMPDIR"/monitor.txt + found=1 + break + fi + sleep .5 + done + test -n "$found" + + timeout 30 bash -c "while ! journalctl _PID=1 _COMM=systemd --since $since | grep -q 'foobar: systemd-udevd failed to process the device, ignoring: File exists'; do sleep 1; done" + # check if the invalid SYSTEMD_ALIAS property for the interface foobar is ignored by PID1 + assert_eq "$(systemctl show --property=SysFSPath --value /sys/subsystem/net/devices/hoge)" "/sys/devices/virtual/net/hoge" +} + +test_netif_renaming_conflict + exit 0