From b8380cc67a738cb7ec68ac03bb128b90d542d53a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 3 Sep 2020 18:27:53 +0200 Subject: [PATCH] udev: make sure to install an inotify watch whenever we find a block device locked This fixes a race where a block device that pops up and immediately is locked (such as a loopback device in preparation) might result in udev never run any rules for it, and thus never turn on inotify watching for it (as inotify watching is controlled via an option set via udev rules), thus not noticing when the device is unlocked/closed again (which is noticed via IN_CLOSE_WRITE inotify events). This changes two things: 1. Whenever we encounter a locked block device we'll now inotify watch it, so that it is guaranteed we'll notice when the BSD lock fd is closed again, and will reprobe. 2. We'll now turn off inotify watching again once we realise the udev rules don't actually want that. Previously, once watching a device was enabled via a udev rule, it would be watched forever until the device disappeared, even if the option was dropped by the rules for later events. Together this will make sure that we'll watch the device via inotify in both of the following cases: a) The block device has been BSD locked when udev wanted to look at it b) The udev rules run for the last seen event for the device say so In all other cases inotify is off for block devices. This new behaviour both fixes the race, but also makes the most sense, as the rules (when they are run) actually really control the watch state now. And if someone BSD locks a block device then it should be OK to inotify watch it briefly until the lock is released again as the user this way more or less opts into the locking protocol. --- src/udev/udevd.c | 42 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 40 insertions(+), 2 deletions(-) diff --git a/src/udev/udevd.c b/src/udev/udevd.c index d6415ad57e..18079d363d 100644 --- a/src/udev/udevd.c +++ b/src/udev/udevd.c @@ -459,6 +459,43 @@ static int worker_process_device(Manager *manager, sd_device *dev) { return -ENOMEM; r = worker_lock_block_device(dev, &fd_lock); + if (r == -EAGAIN) { + /* So this is a block device and the device is locked currently via the BSD advisory locks — + * someone else is exclusively using it. This means we don't run our udev rules now, to not + * interfere. However we want to know when the device is unlocked again, and retrigger the + * device again then, so that the rules are run eventually. For that we use IN_CLOSE_WRITE + * inotify watches (which isn't exactly the same as waiting for the BSD locks to release, but + * not totally off, as long as unlock+close() is done together, as it usually is). + * + * (The user-facing side of this: https://systemd.io/BLOCK_DEVICE_LOCKING) + * + * There's a bit of a chicken and egg problem here for this however: inotify watching is + * supposed to be enabled via an option set via udev rules (OPTIONS+="watch"). If we skip the + * udev rules here however (as we just said we do), we would thus never see that specific + * udev rule, and thus never turn on inotify watching. But in order to catch up eventually + * and run them we we need the inotify watching: hence a classic chicken and egg problem. + * + * Our way out here: if we see the block device locked, unconditionally watch the device via + * inotify, regardless of any explicit request via OPTIONS+="watch". Thus, a device that is + * currently locked via the BSD file locks will be treated as if we ran a single udev rule + * only for it: the one that turns on inotify watching for it. If we eventually see the + * inotify IN_CLOSE_WRITE event, and then run the rules after all and we then realize that + * this wasn't actually requested (i.e. no OPTIONS+="watch" set) we'll simply turn off the + * watching again (see below). Effectively this means: inotify watching is now enabled either + * a) when the udev rules say so, or b) while the device is locked. + * + * Worst case scenario hence: in the (unlikely) case someone locked the device and we clash + * with that we might do inotify watching for a brief moment for a device where we actually + * weren't supposed to. But that shouldn't be too bad, in particular as BSD locks being taken + * on a block device is kinda an indication that the inotify logic is desired too, to some + * degree — they go hand-in-hand after all. */ + + log_device_debug(dev, "Block device is currently locked, installing watch to wait until the lock is released."); + (void) udev_watch_begin(dev); + + /* Now the watch is installed, let's lock the device again, maybe in the meantime things changed */ + r = worker_lock_block_device(dev, &fd_lock); + } if (r < 0) return r; @@ -475,13 +512,14 @@ static int worker_process_device(Manager *manager, sd_device *dev) { /* in case rtnl was initialized */ manager->rtnl = sd_netlink_ref(udev_event->rtnl); - /* apply/restore inotify watch */ + /* apply/restore/end inotify watch */ if (udev_event->inotify_watch) { (void) udev_watch_begin(dev); r = device_update_db(dev); if (r < 0) return log_device_debug_errno(dev, r, "Failed to update database under /run/udev/data/: %m"); - } + } else + (void) udev_watch_end(dev); log_device_debug(dev, "Device (SEQNUM=%"PRIu64", ACTION=%s) processed", seqnum, device_action_to_string(action));