Merge pull request #25980 from yuwata/udev-fail-to-rename-netif

udev,pid1: gracefully handle failure in renaming network interface

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 */

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;

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);

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;

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 = "devnode",  .fmt = 'N', .type = FORMAT_SUBST_DEVNODE       },
-           { .name = "tempnode", .fmt = 'N', .type = FORMAT_SUBST_DEVNODE }, /* deprecated */
+           { .name = "tempnode", .fmt = 'N', .type = FORMAT_SUBST_DEVNODE       }, /* deprecated */
-           { .name = "attr",     .fmt = 's', .type = FORMAT_SUBST_ATTR },
+           { .name = "attr",     .fmt = 's', .type = FORMAT_SUBST_ATTR          },
-           { .name = "sysfs",    .fmt = 's', .type = FORMAT_SUBST_ATTR }, /* deprecated */
+           { .name = "sysfs",    .fmt = 's', .type = FORMAT_SUBST_ATTR          }, /* deprecated */
-           { .name = "env",      .fmt = 'E', .type = FORMAT_SUBST_ENV },
+           { .name = "env",      .fmt = 'E', .type = FORMAT_SUBST_ENV           },
-           { .name = "kernel",   .fmt = 'k', .type = FORMAT_SUBST_KERNEL },
+           { .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 = "driver",   .fmt = 'd', .type = FORMAT_SUBST_DRIVER        },
-           { .name = "devpath",  .fmt = 'p', .type = FORMAT_SUBST_DEVPATH },
+           { .name = "devpath",  .fmt = 'p', .type = FORMAT_SUBST_DEVPATH       },
-           { .name = "id",       .fmt = 'b', .type = FORMAT_SUBST_ID },
+           { .name = "id",       .fmt = 'b', .type = FORMAT_SUBST_ID            },
-           { .name = "major",    .fmt = 'M', .type = FORMAT_SUBST_MAJOR },
+           { .name = "major",    .fmt = 'M', .type = FORMAT_SUBST_MAJOR         },
-           { .name = "minor",    .fmt = 'm', .type = FORMAT_SUBST_MINOR },
+           { .name = "minor",    .fmt = 'm', .type = FORMAT_SUBST_MINOR         },
-           { .name = "result",   .fmt = 'c', .type = FORMAT_SUBST_RESULT },
+           { .name = "result",   .fmt = 'c', .type = FORMAT_SUBST_RESULT        },
-           { .name = "parent",   .fmt = 'P', .type = FORMAT_SUBST_PARENT },
+           { .name = "parent",   .fmt = 'P', .type = FORMAT_SUBST_PARENT        },
-           { .name = "name",     .fmt = 'D', .type = FORMAT_SUBST_NAME },
+           { .name = "name",     .fmt = 'D', .type = FORMAT_SUBST_NAME          },
-           { .name = "links",    .fmt = 'L', .type = FORMAT_SUBST_LINKS },
+           { .name = "links",    .fmt = 'L', .type = FORMAT_SUBST_LINKS         },
-           { .name = "root",     .fmt = 'r', .type = FORMAT_SUBST_ROOT },
+           { .name = "root",     .fmt = 'r', .type = FORMAT_SUBST_ROOT          },
-           { .name = "sys",      .fmt = 'S', .type = FORMAT_SUBST_SYS },
+           { .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 = sd_device_get_sysname(dev, &s);
-        r = device_add_property(dev, "ID_RENAMING", "1");
         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)
+        if (r < 0) {
-                return log_device_warning_errno(dev, r, "Failed to update properties with new name '%s': %m", event->name);
+                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)
+        if (r < 0) {
-                return log_device_warning_errno(event->dev_db_clone, r, "Failed to add 'ID_RENAMING' property: %m");
+                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)
+        if (r < 0) {
-                return log_device_debug_errno(event->dev_db_clone, r, "Failed to update database under /run/udev/data/: %m");
+                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) {
+        if (r < 0) {
-                log_device_info(dev, "Network interface '%s' is already up, cannot rename to '%s'.",
+                if (r == -EBUSY) {
-                                oldname, event->name);
+                        log_device_info(dev, "Network interface '%s' is already up, cannot rename to '%s'.",
-                return 0;
+                                        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) {

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 <<EOF
+ACTION!="add", GOTO="hoge_end"
+SUBSYSTEM!="net", GOTO="hoge_end"
+
+OPTIONS="log_level=debug"
+
+KERNEL=="foobar", NAME="hoge"
+
+LABEL="hoge_end"
+EOF
+
+    udevadm control --log-priority=debug --reload --timeout=30
+
+    ip link add hoge type dummy
+    udevadm wait --timeout=30 --settle /sys/devices/virtual/net/hoge
+
+    TMPDIR=$(mktemp -d -p /tmp udev-tests.XXXXXX)
+    udevadm monitor --udev --property --subsystem-match=net >"$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