From 29d02458327234333cba109d9355181c78030eb4 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 15 Apr 2022 12:00:56 +0900 Subject: [PATCH 1/3] udev: use device ID to find blockers If two devices have the same devnum and subsystem (more specifically, if the device is block or not), or have the same ifindex, then IDs of these devices are equivalent. Hence, the previous conditions are covered by comparing device IDs. Of course, events with a same ID should be already blocked by the devpath check. So, this should not change anything. However, udevd saves many kinds of data under /run/udev named with the device ID. If multiple workers processes events for the same device ID, then the database may become corrupted. Let's explicitly check the device IDs for safety and simplicity. --- src/udev/udevd.c | 89 ++++++++++++++++-------------------------------- 1 file changed, 30 insertions(+), 59 deletions(-) diff --git a/src/udev/udevd.c b/src/udev/udevd.c index d140dcf6f64..859b169f1b6 100644 --- a/src/udev/udevd.c +++ b/src/udev/udevd.c @@ -131,6 +131,9 @@ typedef struct Event { sd_device_action_t action; uint64_t seqnum; uint64_t blocker_seqnum; + const char *id; + const char *devpath; + const char *devpath_old; usec_t retry_again_next_usec; usec_t retry_again_timeout_usec; @@ -852,12 +855,9 @@ static int event_run(Event *event) { } static int event_is_blocked(Event *event) { - const char *subsystem, *devpath, *devpath_old = NULL; - dev_t devnum = makedev(0, 0); Event *loop_event = NULL; size_t devpath_len; - int r, ifindex = 0; - bool is_block; + int r; /* lookup event for identical, parent, child device */ @@ -903,34 +903,11 @@ static int event_is_blocked(Event *event) { assert(loop_event->seqnum > event->blocker_seqnum && loop_event->seqnum < event->seqnum); - r = sd_device_get_subsystem(event->dev, &subsystem); - if (r < 0) - return r; - - is_block = streq(subsystem, "block"); - - r = sd_device_get_devpath(event->dev, &devpath); - if (r < 0) - return r; - - devpath_len = strlen(devpath); - - r = sd_device_get_property_value(event->dev, "DEVPATH_OLD", &devpath_old); - if (r < 0 && r != -ENOENT) - return r; - - r = sd_device_get_devnum(event->dev, &devnum); - if (r < 0 && r != -ENOENT) - return r; - - r = sd_device_get_ifindex(event->dev, &ifindex); - if (r < 0 && r != -ENOENT) - return r; + devpath_len = strlen(event->devpath); /* check if queue contains events we depend on */ LIST_FOREACH(event, e, loop_event) { size_t loop_devpath_len, common; - const char *loop_devpath; loop_event = e; @@ -938,42 +915,20 @@ static int event_is_blocked(Event *event) { if (loop_event->seqnum >= event->seqnum) goto no_blocker; - /* check major/minor */ - if (major(devnum) != 0) { - const char *s; - dev_t d; - - if (sd_device_get_subsystem(loop_event->dev, &s) < 0) - continue; - - if (sd_device_get_devnum(loop_event->dev, &d) >= 0 && - devnum == d && is_block == streq(s, "block")) - break; - } - - /* check network device ifindex */ - if (ifindex > 0) { - int i; - - if (sd_device_get_ifindex(loop_event->dev, &i) >= 0 && - ifindex == i) - break; - } - - if (sd_device_get_devpath(loop_event->dev, &loop_devpath) < 0) - continue; - - /* check our old name */ - if (devpath_old && streq(devpath_old, loop_devpath)) + if (streq_ptr(loop_event->id, event->id)) break; - loop_devpath_len = strlen(loop_devpath); + /* check our old name */ + if (event->devpath_old && streq(event->devpath_old, loop_event->devpath)) + break; + + loop_devpath_len = strlen(loop_event->devpath); /* compare devpath */ common = MIN(devpath_len, loop_devpath_len); /* one devpath is contained in the other? */ - if (!strneq(devpath, loop_devpath, common)) + if (!strneq(event->devpath, loop_event->devpath, common)) continue; /* identical device event found */ @@ -981,11 +936,11 @@ static int event_is_blocked(Event *event) { break; /* parent device event found */ - if (devpath[common] == '/') + if (event->devpath[common] == '/') break; /* child device event found */ - if (loop_devpath[common] == '/') + if (loop_event->devpath[common] == '/') break; } @@ -1134,6 +1089,7 @@ static int event_queue_assume_block_device_unlocked(Manager *manager, sd_device } static int event_queue_insert(Manager *manager, sd_device *dev) { + const char *devpath, *devpath_old = NULL, *id = NULL; sd_device_action_t action; uint64_t seqnum; Event *event; @@ -1154,6 +1110,18 @@ static int event_queue_insert(Manager *manager, sd_device *dev) { if (r < 0) return r; + r = sd_device_get_devpath(dev, &devpath); + if (r < 0) + return r; + + r = sd_device_get_property_value(dev, "DEVPATH_OLD", &devpath_old); + if (r < 0 && r != -ENOENT) + return r; + + r = device_get_device_id(dev, &id); + if (r < 0 && r != -ENOENT) + return r; + event = new(Event, 1); if (!event) return -ENOMEM; @@ -1163,6 +1131,9 @@ static int event_queue_insert(Manager *manager, sd_device *dev) { .dev = sd_device_ref(dev), .seqnum = seqnum, .action = action, + .id = id, + .devpath = devpath, + .devpath_old = devpath_old, .state = EVENT_QUEUED, }; From a1af9668ec78fc11a2e7ac942397eac0c8fceced Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 15 Apr 2022 12:23:48 +0900 Subject: [PATCH 2/3] udev: make newer event also blocked by DEVPATH_OLD Previously, a device has DEVPATH_OLD is blocked by a previous event whose devpath is equivalent to the DEVPATH_OLD. This extends the condtion. 1. an event has DEVPATH_OLD is blocked by a previous event whose devpath is a parent of, child of, or equivalent to the DEVPATH_OLD. 2. an event is blocked by a previous event whose DEVPATH_OLD is a parent of, child of, or equivalent to the devpath of the new event. I am not sure such check is really necessary. But, the cost of the check is expected to be extremely small, as device renaming does not occur so frequently. Hence, it should not introduce any significant performance regression. --- src/shared/udev-util.c | 13 +++++++++++++ src/shared/udev-util.h | 2 ++ src/test/test-udev-util.c | 13 +++++++++++++ src/udev/udevd.c | 31 +++---------------------------- 4 files changed, 31 insertions(+), 28 deletions(-) diff --git a/src/shared/udev-util.c b/src/shared/udev-util.c index f91ee3e1bdb..1eb5d8fb53f 100644 --- a/src/shared/udev-util.c +++ b/src/shared/udev-util.c @@ -568,6 +568,19 @@ int udev_resolve_subsys_kernel(const char *string, char *result, size_t maxsize, return 0; } +bool devpath_conflict(const char *a, const char *b) { + /* This returns true when two paths are equivalent, or one is a child of another. */ + + if (!a || !b) + return false; + + for (; *a != '\0' && *b != '\0'; a++, b++) + if (*a != *b) + return false; + + return *a == '/' || *b == '/' || *a == *b; +} + int udev_queue_is_empty(void) { return access("/run/udev/queue", F_OK) < 0 ? (errno == ENOENT ? true : -errno) : false; diff --git a/src/shared/udev-util.h b/src/shared/udev-util.h index ce016e88db3..43a1b846f48 100644 --- a/src/shared/udev-util.h +++ b/src/shared/udev-util.h @@ -50,6 +50,8 @@ size_t udev_replace_ifname(char *str); size_t udev_replace_chars(char *str, const char *allow); int udev_resolve_subsys_kernel(const char *string, char *result, size_t maxsize, bool read_value); +bool devpath_conflict(const char *a, const char *b); + int udev_queue_is_empty(void); int udev_queue_init(void); diff --git a/src/test/test-udev-util.c b/src/test/test-udev-util.c index d4139971895..02be7a0b3c6 100644 --- a/src/test/test-udev-util.c +++ b/src/test/test-udev-util.c @@ -155,4 +155,17 @@ TEST(udev_resolve_subsys_kernel) { test_udev_resolve_subsys_kernel_one("[net/lo]/address", true, 0, "00:00:00:00:00:00"); } +TEST(devpath_conflict) { + assert_se(!devpath_conflict(NULL, NULL)); + assert_se(!devpath_conflict(NULL, "/devices/pci0000:00/0000:00:1c.4")); + assert_se(!devpath_conflict("/devices/pci0000:00/0000:00:1c.4", NULL)); + assert_se(!devpath_conflict("/devices/pci0000:00/0000:00:1c.4", "/devices/pci0000:00/0000:00:00.0")); + assert_se(!devpath_conflict("/devices/virtual/net/veth99", "/devices/virtual/net/veth999")); + + assert_se(devpath_conflict("/devices/pci0000:00/0000:00:1c.4", "/devices/pci0000:00/0000:00:1c.4")); + assert_se(devpath_conflict("/devices/pci0000:00/0000:00:1c.4", "/devices/pci0000:00/0000:00:1c.4/0000:3c:00.0")); + assert_se(devpath_conflict("/devices/pci0000:00/0000:00:1c.4/0000:3c:00.0/nvme/nvme0/nvme0n1", + "/devices/pci0000:00/0000:00:1c.4/0000:3c:00.0/nvme/nvme0/nvme0n1/nvme0n1p1")); +} + DEFINE_TEST_MAIN(LOG_INFO); diff --git a/src/udev/udevd.c b/src/udev/udevd.c index 859b169f1b6..b2291c85cae 100644 --- a/src/udev/udevd.c +++ b/src/udev/udevd.c @@ -856,7 +856,6 @@ static int event_run(Event *event) { static int event_is_blocked(Event *event) { Event *loop_event = NULL; - size_t devpath_len; int r; /* lookup event for identical, parent, child device */ @@ -903,12 +902,8 @@ static int event_is_blocked(Event *event) { assert(loop_event->seqnum > event->blocker_seqnum && loop_event->seqnum < event->seqnum); - devpath_len = strlen(event->devpath); - /* check if queue contains events we depend on */ LIST_FOREACH(event, e, loop_event) { - size_t loop_devpath_len, common; - loop_event = e; /* found ourself, no later event can block us */ @@ -918,29 +913,9 @@ static int event_is_blocked(Event *event) { if (streq_ptr(loop_event->id, event->id)) break; - /* check our old name */ - if (event->devpath_old && streq(event->devpath_old, loop_event->devpath)) - break; - - loop_devpath_len = strlen(loop_event->devpath); - - /* compare devpath */ - common = MIN(devpath_len, loop_devpath_len); - - /* one devpath is contained in the other? */ - if (!strneq(event->devpath, loop_event->devpath, common)) - continue; - - /* identical device event found */ - if (devpath_len == loop_devpath_len) - break; - - /* parent device event found */ - if (event->devpath[common] == '/') - break; - - /* child device event found */ - if (loop_event->devpath[common] == '/') + if (devpath_conflict(event->devpath, loop_event->devpath) || + devpath_conflict(event->devpath, loop_event->devpath_old) || + devpath_conflict(event->devpath_old, loop_event->devpath)) break; } From 34458dbbe795c20442f6262875c74a679c5957e0 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 21 Apr 2022 17:47:51 +0900 Subject: [PATCH 3/3] udev: also make uevent blocked by events for the same device node Even if the device node is the same, devnum (thus, device ID) and syspath may be different. If a 'remove' and 'add' events for the same device node but with different devnum and syspath are queued, previously, we might process them in parallel. And, udev_watch_end() for the 'remove' event and udev_watch_begin() for the 'add' event may interfere each other. --- src/udev/udevd.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/udev/udevd.c b/src/udev/udevd.c index b2291c85cae..8a06d08c4a5 100644 --- a/src/udev/udevd.c +++ b/src/udev/udevd.c @@ -134,6 +134,7 @@ typedef struct Event { const char *id; const char *devpath; const char *devpath_old; + const char *devnode; usec_t retry_again_next_usec; usec_t retry_again_timeout_usec; @@ -917,6 +918,9 @@ static int event_is_blocked(Event *event) { devpath_conflict(event->devpath, loop_event->devpath_old) || devpath_conflict(event->devpath_old, loop_event->devpath)) break; + + if (event->devnode && streq_ptr(event->devnode, loop_event->devnode)) + break; } assert(loop_event); @@ -1064,7 +1068,7 @@ static int event_queue_assume_block_device_unlocked(Manager *manager, sd_device } static int event_queue_insert(Manager *manager, sd_device *dev) { - const char *devpath, *devpath_old = NULL, *id = NULL; + const char *devpath, *devpath_old = NULL, *id = NULL, *devnode = NULL; sd_device_action_t action; uint64_t seqnum; Event *event; @@ -1097,6 +1101,10 @@ static int event_queue_insert(Manager *manager, sd_device *dev) { if (r < 0 && r != -ENOENT) return r; + r = sd_device_get_devname(dev, &devnode); + if (r < 0 && r != -ENOENT) + return r; + event = new(Event, 1); if (!event) return -ENOMEM; @@ -1109,6 +1117,7 @@ static int event_queue_insert(Manager *manager, sd_device *dev) { .id = id, .devpath = devpath, .devpath_old = devpath_old, + .devnode = devnode, .state = EVENT_QUEUED, };