From 30ced6a8c742e1c798fff439b28a9800ca43f3e7 Mon Sep 17 00:00:00 2001

From: Lennart Poettering <lennart@poettering.net>

Date: Fri, 27 Feb 2015 21:55:08 +0100

Subject: [PATCH] core: rework device state logic

This change introduces a new state "tentative" for device units. Device

units are considered "plugged" when udev announced them, "dead" when

they are not available in the kernel, and "tentative" when they are

referenced in /proc/self/mountinfo or /proc/swaps but not (yet)

announced via udev.

This should fix a race when device nodes (like loop devices) are created

and immediately mounted. Previously, systemd might end up seeing the

mount unit before the device, and would thus pull down the mount because

its BindTo dependency on the device would not be fulfilled.

(cherry picked from commit 628c89cc68ab96fce2de7ebba5933725d147aecc)

---

 src/core/device.c | 368 ++++++++++++++++++++++++++++------------------

 src/core/device.h |  14 +-

 src/core/mount.c  |  46 +++---

 src/core/swap.c   |  32 ++--

 src/core/swap.h   |   4 +-

 src/core/unit.c   |   1 -

 6 files changed, 285 insertions(+), 180 deletions(-)

diff --git a/src/core/device.c b/src/core/device.c

index d3deac393..75b9a4628 100644

--- a/src/core/device.c

+++ b/src/core/device.c

@@ -36,7 +36,8 @@

 static const UnitActiveState state_translation_table[_DEVICE_STATE_MAX] = {

         [DEVICE_DEAD] = UNIT_INACTIVE,

-        [DEVICE_PLUGGED] = UNIT_ACTIVE

+        [DEVICE_TENTATIVE] = UNIT_ACTIVATING,

+        [DEVICE_PLUGGED] = UNIT_ACTIVE,

 };

 static int device_dispatch_io(sd_event_source *source, int fd, uint32_t revents, void *userdata);

@@ -65,6 +66,41 @@ static void device_unset_sysfs(Device *d) {

         d->sysfs = NULL;

 }

+static int device_set_sysfs(Device *d, const char *sysfs) {

+        Device *first;

+        char *copy;

+        int r;

+

+        assert(d);

+

+        if (streq_ptr(d->sysfs, sysfs))

+                return 0;

+

+        r = hashmap_ensure_allocated(&UNIT(d)->manager->devices_by_sysfs, &string_hash_ops);

+        if (r < 0)

+                return r;

+

+        copy = strdup(sysfs);

+        if (!copy)

+                return -ENOMEM;

+

+        device_unset_sysfs(d);

+

+        first = hashmap_get(UNIT(d)->manager->devices_by_sysfs, sysfs);

+        LIST_PREPEND(same_sysfs, first, d);

+

+        r = hashmap_replace(UNIT(d)->manager->devices_by_sysfs, copy, first);

+        if (r < 0) {

+                LIST_REMOVE(same_sysfs, first, d);

+                free(copy);

+                return r;

+        }

+

+        d->sysfs = copy;

+

+        return 0;

+}

+

 static void device_init(Unit *u) {

         Device *d = DEVICE(u);

@@ -112,8 +148,13 @@ static int device_coldplug(Unit *u) {

         assert(d);

         assert(d->state == DEVICE_DEAD);

-        if (d->sysfs)

+        if (d->found & DEVICE_FOUND_UDEV)

+                /* If udev says the device is around, it's around */

                 device_set_state(d, DEVICE_PLUGGED);

+        else if (d->found != DEVICE_NOT_FOUND)

+                /* If a device is found in /proc/self/mountinfo or

+                 * /proc/swaps, it's "tentatively" around. */

+                device_set_state(d, DEVICE_TENTATIVE);

         return 0;

 }

@@ -142,49 +183,9 @@ _pure_ static const char *device_sub_state_to_string(Unit *u) {

         return device_state_to_string(DEVICE(u)->state);

 }

-static int device_add_escaped_name(Unit *u, const char *dn) {

-        _cleanup_free_ char *e = NULL;

-        int r;

-

-        assert(u);

-        assert(dn);

-        assert(dn[0] == '/');

-

-        e = unit_name_from_path(dn, ".device");

-        if (!e)

-                return -ENOMEM;

-

-        r = unit_add_name(u, e);

-        if (r < 0 && r != -EEXIST)

-                return r;

-

-        return 0;

-}

-

-static int device_find_escape_name(Manager *m, const char *dn, Unit **_u) {

-        _cleanup_free_ char *e = NULL;

-        Unit *u;

-

-        assert(m);

-        assert(dn);

-        assert(dn[0] == '/');

-        assert(_u);

-

-        e = unit_name_from_path(dn, ".device");

-        if (!e)

-                return -ENOMEM;

-

-        u = manager_get_unit(m, e);

-        if (u) {

-                *_u = u;

-                return 1;

-        }

-

-        return 0;

-}

-

-static int device_make_description(Unit *u, struct udev_device *dev, const char *path) {

+static int device_update_description(Unit *u, struct udev_device *dev, const char *path) {

         const char *model;

+        int r;

         assert(u);

         assert(dev);

@@ -209,13 +210,16 @@ static int device_make_description(Unit *u, struct udev_device *dev, const char

                         j = strjoin(model, " ", label, NULL);

                         if (j)

-                                return unit_set_description(u, j);

-                }

+                                r = unit_set_description(u, j);

+                } else

+                        r = unit_set_description(u, model);

+        } else

+                r = unit_set_description(u, path);

-                return unit_set_description(u, model);

-        }

+        if (r < 0)

+                log_unit_error_errno(u->id, r, "Failed to set device description: %m");

-        return unit_set_description(u, path);

+        return r;

 }

 static int device_add_udev_wants(Unit *u, struct udev_device *dev) {

@@ -242,20 +246,20 @@ static int device_add_udev_wants(Unit *u, struct udev_device *dev) {

                 n = unit_name_mangle(e, MANGLE_NOGLOB);

                 if (!n)

-                        return -ENOMEM;

+                        return log_oom();

                 r = unit_add_dependency_by_name(u, UNIT_WANTS, n, NULL, true);

                 if (r < 0)

-                        return r;

+                        return log_unit_error_errno(u->id, r, "Failed to add wants dependency: %m");

         }

         if (!isempty(state))

-                log_unit_warning(u->id, "Property %s on %s has trailing garbage, ignoring.",

-                                 property, strna(udev_device_get_syspath(dev)));

+                log_unit_warning(u->id, "Property %s on %s has trailing garbage, ignoring.", property, strna(udev_device_get_syspath(dev)));

         return 0;

 }

-static int device_update_unit(Manager *m, struct udev_device *dev, const char *path, bool main) {

+static int device_setup_unit(Manager *m, struct udev_device *dev, const char *path, bool main) {

+        _cleanup_free_ char *e = NULL;

         const char *sysfs;

         Unit *u = NULL;

         bool delete;

@@ -269,12 +273,18 @@ static int device_update_unit(Manager *m, struct udev_device *dev, const char *p

         if (!sysfs)

                 return 0;

-        r = device_find_escape_name(m, path, &u);

-        if (r < 0)

-                return r;

+        e = unit_name_from_path(path, ".device");

+        if (!e)

+                return log_oom();

+

+        u = manager_get_unit(m, e);

-        if (u && DEVICE(u)->sysfs && !path_equal(DEVICE(u)->sysfs, sysfs))

+        if (u &&

+            DEVICE(u)->sysfs &&

+            !path_equal(DEVICE(u)->sysfs, sysfs)) {

+                log_unit_error(u->id, "Device %s appeared twice with different sysfs paths %s and %s", e, DEVICE(u)->sysfs, sysfs);

                 return -EEXIST;

+        }

         if (!u) {

                 delete = true;

@@ -283,7 +293,7 @@ static int device_update_unit(Manager *m, struct udev_device *dev, const char *p

                 if (!u)

                         return log_oom();

-                r = device_add_escaped_name(u, path);

+                r = unit_add_name(u, e);

                 if (r < 0)

                         goto fail;

@@ -295,37 +305,16 @@ static int device_update_unit(Manager *m, struct udev_device *dev, const char *p

          * actually been seen yet ->sysfs will not be

          * initialized. Hence initialize it if necessary. */

-        if (!DEVICE(u)->sysfs) {

-                Device *first;

-

-                DEVICE(u)->sysfs = strdup(sysfs);

-                if (!DEVICE(u)->sysfs) {

-                        r = -ENOMEM;

-                        goto fail;

-                }

-

-                r = hashmap_ensure_allocated(&m->devices_by_sysfs, &string_hash_ops);

-                if (r < 0)

-                        goto fail;

-

-                first = hashmap_get(m->devices_by_sysfs, sysfs);

-                LIST_PREPEND(same_sysfs, first, DEVICE(u));

-

-                r = hashmap_replace(m->devices_by_sysfs, DEVICE(u)->sysfs, first);

-                if (r < 0)

-                        goto fail;

-        }

-

-        device_make_description(u, dev, path);

+        r = device_set_sysfs(DEVICE(u), sysfs);

+        if (r < 0)

+                goto fail;

-        if (main) {

-                /* The additional systemd udev properties we only

-                 * interpret for the main object */

+        (void) device_update_description(u, dev, path);

-                r = device_add_udev_wants(u, dev);

-                if (r < 0)

-                        goto fail;

-        }

+        /* The additional systemd udev properties we only interpret

+         * for the main object */

+        if (main)

+                (void) device_add_udev_wants(u, dev);

         /* Note that this won't dispatch the load queue, the caller

          * has to do that if needed and appropriate */

@@ -334,7 +323,7 @@ static int device_update_unit(Manager *m, struct udev_device *dev, const char *p

         return 0;

 fail:

-        log_warning_errno(r, "Failed to load device unit: %m");

+        log_unit_warning_errno(u->id, r, "Failed to set up device unit: %m");

         if (delete && u)

                 unit_free(u);

@@ -342,7 +331,7 @@ fail:

         return r;

 }

-static int device_process_new_device(Manager *m, struct udev_device *dev) {

+static int device_process_new(Manager *m, struct udev_device *dev) {

         const char *sysfs, *dn, *alias;

         struct udev_list_entry *item = NULL, *first = NULL;

         int r;

@@ -354,14 +343,14 @@ static int device_process_new_device(Manager *m, struct udev_device *dev) {

                 return 0;

         /* Add the main unit named after the sysfs path */

-        r = device_update_unit(m, dev, sysfs, true);

+        r = device_setup_unit(m, dev, sysfs, true);

         if (r < 0)

                 return r;

         /* Add an additional unit for the device node */

         dn = udev_device_get_devnode(dev);

         if (dn)

-                device_update_unit(m, dev, dn, false);

+                (void) device_setup_unit(m, dev, dn, false);

         /* Add additional units for all symlinks */

         first = udev_device_get_devlinks_list_entry(dev);

@@ -388,7 +377,7 @@ static int device_process_new_device(Manager *m, struct udev_device *dev) {

                             st.st_rdev != udev_device_get_devnum(dev))

                                 continue;

-                device_update_unit(m, dev, p, false);

+                (void) device_setup_unit(m, dev, p, false);

         }

         /* Add additional units for all explicitly configured

@@ -405,7 +394,7 @@ static int device_process_new_device(Manager *m, struct udev_device *dev) {

                         e[l] = 0;

                         if (path_is_absolute(e))

-                                device_update_unit(m, dev, e, false);

+                                (void) device_setup_unit(m, dev, e, false);

                         else

                                 log_warning("SYSTEMD_ALIAS for %s is not an absolute path, ignoring: %s", sysfs, e);

                 }

@@ -416,39 +405,62 @@ static int device_process_new_device(Manager *m, struct udev_device *dev) {

         return 0;

 }

-static void device_set_path_plugged(Manager *m, struct udev_device *dev) {

-        const char *sysfs;

+static void device_update_found_one(Device *d, bool add, DeviceFound found, bool now) {

+        DeviceFound n;

+

+        assert(d);

+

+        n = add ? (d->found | found) : (d->found & ~found);

+        if (n == d->found)

+                return;

+

+        d->found = n;

+

+        if (now) {

+                if (d->found & DEVICE_FOUND_UDEV)

+                        device_set_state(d, DEVICE_PLUGGED);

+                else if (d->found != DEVICE_NOT_FOUND)

+                        device_set_state(d, DEVICE_TENTATIVE);

+                else

+                        device_set_state(d, DEVICE_DEAD);

+        }

+}

+

+static int device_update_found_by_sysfs(Manager *m, const char *sysfs, bool add, DeviceFound found, bool now) {

         Device *d, *l;

         assert(m);

-        assert(dev);

+        assert(sysfs);

-        sysfs = udev_device_get_syspath(dev);

-        if (!sysfs)

-                return;

+        if (found == DEVICE_NOT_FOUND)

+                return 0;

         l = hashmap_get(m->devices_by_sysfs, sysfs);

         LIST_FOREACH(same_sysfs, d, l)

-                device_set_state(d, DEVICE_PLUGGED);

+                device_update_found_one(d, add, found, now);

+

+        return 0;

 }

-static int device_process_removed_device(Manager *m, struct udev_device *dev) {

-        const char *sysfs;

-        Device *d;

+static int device_update_found_by_name(Manager *m, const char *path, bool add, DeviceFound found, bool now) {

+        _cleanup_free_ char *e = NULL;

+        Unit *u;

         assert(m);

-        assert(dev);

+        assert(path);

-        sysfs = udev_device_get_syspath(dev);

-        if (!sysfs)

-                return -ENOMEM;

+        if (found == DEVICE_NOT_FOUND)

+                return 0;

-        /* Remove all units of this sysfs path */

-        while ((d = hashmap_get(m->devices_by_sysfs, sysfs))) {

-                device_unset_sysfs(d);

-                device_set_state(d, DEVICE_DEAD);

-        }

+        e = unit_name_from_path(path, ".device");

+        if (!e)

+                return log_oom();

+        u = manager_get_unit(m, e);

+        if (!u)

+                return 0;

+

+        device_update_found_one(DEVICE(u), add, found, now);

         return 0;

 }

@@ -464,22 +476,6 @@ static bool device_is_ready(struct udev_device *dev) {

         return parse_boolean(ready) != 0;

 }

-static int device_process_new_path(Manager *m, const char *path) {

-        _cleanup_udev_device_unref_ struct udev_device *dev = NULL;

-

-        assert(m);

-        assert(path);

-

-        dev = udev_device_new_from_syspath(m->udev, path);

-        if (!dev)

-                return log_oom();

-

-        if (!device_is_ready(dev))

-                return 0;

-

-        return device_process_new_device(m, dev);

-}

-

 static Unit *device_following(Unit *u) {

         Device *d = DEVICE(u);

         Device *other, *first = NULL;

@@ -606,12 +602,31 @@ static int device_enumerate(Manager *m) {

                 goto fail;

         first = udev_enumerate_get_list_entry(e);

-        udev_list_entry_foreach(item, first)

-                device_process_new_path(m, udev_list_entry_get_name(item));

+        udev_list_entry_foreach(item, first) {

+                _cleanup_udev_device_unref_ struct udev_device *dev = NULL;

+                const char *sysfs;

+

+                sysfs = udev_list_entry_get_name(item);

+

+                dev = udev_device_new_from_syspath(m->udev, sysfs);

+                if (!dev) {

+                        log_oom();

+                        continue;

+                }

+

+                if (!device_is_ready(dev))

+                        continue;

+

+                (void) device_process_new(m, dev);

+

+                device_update_found_by_sysfs(m, sysfs, true, DEVICE_FOUND_UDEV, false);

+        }

         return 0;

 fail:

+        log_error_errno(r, "Failed to enumerate devices: %m");

+

         device_shutdown(m);

         return r;

 }

@@ -619,7 +634,7 @@ fail:

 static int device_dispatch_io(sd_event_source *source, int fd, uint32_t revents, void *userdata) {

         _cleanup_udev_device_unref_ struct udev_device *dev = NULL;

         Manager *m = userdata;

-        const char *action;

+        const char *action, *sysfs;

         int r;

         assert(m);

@@ -641,33 +656,47 @@ static int device_dispatch_io(sd_event_source *source, int fd, uint32_t revents,

         if (!dev)

                 return 0;

+        sysfs = udev_device_get_syspath(dev);

+        if (!sysfs) {

+                log_error("Failed to get udev sys path.");

+                return 0;

+        }

+

         action = udev_device_get_action(dev);

         if (!action) {

                 log_error("Failed to get udev action string.");

                 return 0;

         }

-        if (streq(action, "remove") || !device_is_ready(dev))  {

-                r = device_process_removed_device(m, dev);

-                if (r < 0)

-                        log_error_errno(r, "Failed to process device remove event: %m");

-

-                r = swap_process_removed_device(m, dev);

+        if (streq(action, "remove"))  {

+                r = swap_process_device_remove(m, dev);

                 if (r < 0)

                         log_error_errno(r, "Failed to process swap device remove event: %m");

-        } else {

-                r = device_process_new_device(m, dev);

-                if (r < 0)

-                        log_error_errno(r, "Failed to process device new event: %m");

+                /* If we get notified that a device was removed by

+                 * udev, then it's completely gone, hence unset all

+                 * found bits */

+                device_update_found_by_sysfs(m, sysfs, false, DEVICE_FOUND_UDEV|DEVICE_FOUND_MOUNT|DEVICE_FOUND_SWAP, true);

-                r = swap_process_new_device(m, dev);

+        } else if (device_is_ready(dev)) {

+

+                (void) device_process_new(m, dev);

+

+                r = swap_process_device_new(m, dev);

                 if (r < 0)

                         log_error_errno(r, "Failed to process swap device new event: %m");

                 manager_dispatch_load_queue(m);

-                device_set_path_plugged(m, dev);

+                /* The device is found now, set the udev found bit */

+                device_update_found_by_sysfs(m, sysfs, true, DEVICE_FOUND_UDEV, true);

+

+        } else {

+                /* The device is nominally around, but not ready for

+                 * us. Hence unset the udev bit, but leave the rest

+                 * around. */

+

+                device_update_found_by_sysfs(m, sysfs, false, DEVICE_FOUND_UDEV, true);

         }

         return 0;

@@ -686,9 +715,58 @@ static bool device_supported(Manager *m) {

         return read_only <= 0;

 }

+int device_found_node(Manager *m, const char *node, bool add, DeviceFound found, bool now) {

+        _cleanup_udev_device_unref_ struct udev_device *dev = NULL;

+        struct stat st;

+

+        assert(m);

+        assert(node);

+

+        /* This is called whenever we find a device referenced in

+         * /proc/swaps or /proc/self/mounts. Such a device might be

+         * mounted/enabled at a time where udev has not finished

+         * probing it yet, and we thus haven't learned about it

+         * yet. In this case we will set the device unit to

+         * "tentative" state. */

+

+        if (add) {

+                if (!path_startswith(node, "/dev"))

+                        return 0;

+

+                if (stat(node, &st) < 0) {

+                        if (errno == ENOENT)

+                                return 0;

+

+                        return log_error_errno(errno, "Failed to stat device node file %s: %m", node);

+                }

+

+                if (!S_ISBLK(st.st_mode) && !S_ISCHR(st.st_mode))

+                        return 0;

+

+                dev = udev_device_new_from_devnum(m->udev, S_ISBLK(st.st_mode) ? 'b' : 'c', st.st_rdev);

+                if (!dev) {

+                        if (errno == ENOENT)

+                                return 0;

+

+                        return log_oom();

+                }

+

+                /* If the device is known in the kernel and newly

+                 * appeared, then we'll create a device unit for it,

+                 * under the name referenced in /proc/swaps or

+                 * /proc/self/mountinfo. */

+

+                (void) device_setup_unit(m, dev, node, false);

+        }

+

+        /* Update the device unit's state, should it exist */

+        return device_update_found_by_name(m, node, add, found, now);

+}

+

 static const char* const device_state_table[_DEVICE_STATE_MAX] = {

         [DEVICE_DEAD] = "dead",

-        [DEVICE_PLUGGED] = "plugged"

+        [DEVICE_TENTATIVE] = "tentative",

+        [DEVICE_PLUGGED] = "plugged",

 };

 DEFINE_STRING_TABLE_LOOKUP(device_state, DeviceState);

diff --git a/src/core/device.h b/src/core/device.h

index bb7ae0783..0609b20fd 100644

--- a/src/core/device.h

+++ b/src/core/device.h

@@ -29,20 +29,28 @@ typedef struct Device Device;

  * simplifies the state engine greatly */

 typedef enum DeviceState {

         DEVICE_DEAD,

-        DEVICE_PLUGGED,

+        DEVICE_TENTATIVE, /* mounted or swapped, but not (yet) announced by udev */

+        DEVICE_PLUGGED,   /* announced by udev */

         _DEVICE_STATE_MAX,

         _DEVICE_STATE_INVALID = -1

 } DeviceState;

+typedef enum DeviceFound {

+        DEVICE_NOT_FOUND = 0,

+        DEVICE_FOUND_UDEV = 1,

+        DEVICE_FOUND_MOUNT = 2,

+        DEVICE_FOUND_SWAP = 4,

+} DeviceFound;

+

 struct Device {

         Unit meta;

         char *sysfs;

+        DeviceFound found;

         /* In order to be able to distinguish dependencies on

         different device nodes we might end up creating multiple

         devices for the same sysfs path. We chain them up here. */

-

         LIST_FIELDS(struct Device, same_sysfs);

         DeviceState state;

@@ -52,3 +60,5 @@ extern const UnitVTable device_vtable;

 const char* device_state_to_string(DeviceState i) _const_;

 DeviceState device_state_from_string(const char *s) _pure_;

+

+int device_found_node(Manager *m, const char *node, bool add, DeviceFound found, bool now);

diff --git a/src/core/mount.c b/src/core/mount.c

index f3977e62d..c971330af 100644

--- a/src/core/mount.c

+++ b/src/core/mount.c

@@ -1391,7 +1391,7 @@ static int mount_dispatch_timer(sd_event_source *source, usec_t usec, void *user

         return 0;

 }

-static int mount_add_one(

+static int mount_setup_unit(

                 Manager *m,

                 const char *what,

                 const char *where,

@@ -1434,7 +1434,7 @@ static int mount_add_one(

                 u = unit_new(m, sizeof(Mount));

                 if (!u)

-                        return -ENOMEM;

+                        return log_oom();

                 r = unit_add_name(u, e);

                 if (r < 0)

@@ -1547,6 +1547,8 @@ static int mount_add_one(

         return 0;

 fail:

+        log_warning_errno(r, "Failed to set up mount unit: %m");

+

         if (delete && u)

                 unit_free(u);

@@ -1554,33 +1556,36 @@ fail:

 }

 static int mount_load_proc_self_mountinfo(Manager *m, bool set_flags) {

-        _cleanup_(mnt_free_tablep) struct libmnt_table *tb = NULL;

-        _cleanup_(mnt_free_iterp) struct libmnt_iter *itr = NULL;

-        struct libmnt_fs *fs;

+        _cleanup_(mnt_free_tablep) struct libmnt_table *t = NULL;

+        _cleanup_(mnt_free_iterp) struct libmnt_iter *i = NULL;

         int r = 0;

         assert(m);

-        tb = mnt_new_table();

-        itr = mnt_new_iter(MNT_ITER_FORWARD);

-        if (!tb || !itr)

+        t = mnt_new_table();

+        if (!t)

                 return log_oom();

-        r = mnt_table_parse_mtab(tb, NULL);

+        i = mnt_new_iter(MNT_ITER_FORWARD);

+        if (!i)

+                return log_oom();

+

+        r = mnt_table_parse_mtab(t, NULL);

         if (r < 0)

-                return r;

+                return log_error_errno(r, "Failed to parse /proc/self/mountinfo: %m");

         r = 0;

         for (;;) {

                 const char *device, *path, *options, *fstype;

                 _cleanup_free_ const char *d = NULL, *p = NULL;

+                struct libmnt_fs *fs;

                 int k;

-                k = mnt_table_next_fs(tb, itr, &fs);

+                k = mnt_table_next_fs(t, i, &fs);

                 if (k == 1)

                         break;

-                else if (k < 0)

-                        return log_error_errno(k, "Failed to get next entry from /etc/fstab: %m");

+                if (k < 0)

+                        return log_error_errno(k, "Failed to get next entry from /proc/self/mountinfo: %m");

                 device = mnt_fs_get_source(fs);

                 path = mnt_fs_get_target(fs);

@@ -1588,11 +1593,16 @@ static int mount_load_proc_self_mountinfo(Manager *m, bool set_flags) {

                 fstype = mnt_fs_get_fstype(fs);