boot: suppress XBOOTLDR if same device as ESP when enumerating entries

On my local system I linked up the ESP and XBOOTLDR partitions, and
ended up with duplicate entries being listed. Try hard to detect that
and only enumerate entries in the ESP if it turns out that both dirs
have the same dev_t.

This should detect both bind mounted and symlinked cases and should make
our list output less confusing.

diff --git a/src/basic/stat-util.h b/src/basic/stat-util.h
index f7d2f12aa9dcb7642b5b9b1e8ce45263cd8d131e..f9b3d2fbedaeb25364f8b269bd66c963abd2d47f 100644 (file)
--- a/src/basic/stat-util.h
+++ b/src/basic/stat-util.h
@@ -114,3 +114,9 @@ int statx_fallback(int dfd, const char *path, int flags, unsigned mask, struct s
                 struct new_statx nsx;           \
         } var
 #endif
+
+static inline bool devid_set_and_equal(dev_t a, dev_t b) {
+        /* Returns true if a and b definitely refer to the same device. If either is zero, this means "don't
+         * know" and we'll return false */
+        return a == b && a != 0;
+}

diff --git a/src/boot/bless-boot.c b/src/boot/bless-boot.c
index 9e4b0d1f72166128804a2826f134f9ee411886da..2cbabc6c59205cabdbb0960bdfa4f06980b4da7d 100644 (file)
--- a/src/boot/bless-boot.c
+++ b/src/boot/bless-boot.c
@@ -14,6 +14,7 @@
 #include "parse-util.h"
 #include "path-util.h"
 #include "pretty-print.h"
+#include "stat-util.h"
 #include "sync-util.h"
 #include "terminal-util.h"
 #include "util.h"
@@ -98,17 +99,18 @@ static int parse_argv(int argc, char *argv[]) {
 
 static int acquire_path(void) {
         _cleanup_free_ char *esp_path = NULL, *xbootldr_path = NULL;
+        dev_t esp_devid = 0, xbootldr_devid = 0;
         char **a;
         int r;
 
         if (!strv_isempty(arg_path))
                 return 0;
 
-        r = find_esp_and_warn(NULL, false, &esp_path, NULL, NULL, NULL, NULL);
+        r = find_esp_and_warn(NULL, /* unprivileged_mode= */ false, &esp_path, NULL, NULL, NULL, NULL, &esp_devid);
         if (r < 0 && r != -ENOKEY) /* ENOKEY means not found, and is the only error the function won't log about on its own */
                 return r;
 
-        r = find_xbootldr_and_warn(NULL, false, &xbootldr_path, NULL);
+        r = find_xbootldr_and_warn(NULL, /* unprivileged_mode= */ false, &xbootldr_path, NULL, &xbootldr_devid);
         if (r < 0 && r != -ENOKEY)
                 return r;
 
@@ -117,8 +119,10 @@ static int acquire_path(void) {
                                        "Couldn't find $BOOT partition. It is recommended to mount it to /boot.\n"
                                        "Alternatively, use --path= to specify path to mount point.");
 
-        if (esp_path)
+        if (esp_path && xbootldr_path && !devid_set_and_equal(esp_devid, xbootldr_devid)) /* in case the two paths refer to the same inode, suppress one */
                 a = strv_new(esp_path, xbootldr_path);
+        else if (esp_path)
+                a = strv_new(esp_path);
         else
                 a = strv_new(xbootldr_path);
         if (!a)
@@ -130,7 +134,7 @@ static int acquire_path(void) {
                 _cleanup_free_ char *j = NULL;
 
                 j = strv_join(arg_path, ":");
-                log_debug("Using %s as boot loader drop-in search path.", j);
+                log_debug("Using %s as boot loader drop-in search path.", strna(j));
         }
 
         return 0;

diff --git a/src/boot/bootctl.c b/src/boot/bootctl.c
index ab514d28ee07da46260997a1df1cca9c6bd82e91..c81411c85797e62179c62fa7be66e67ad313638e 100644 (file)
--- a/src/boot/bootctl.c
+++ b/src/boot/bootctl.c
@@ -75,7 +75,8 @@ static int acquire_esp(
                 uint32_t *ret_part,
                 uint64_t *ret_pstart,
                 uint64_t *ret_psize,
-                sd_id128_t *ret_uuid) {
+                sd_id128_t *ret_uuid,
+                dev_t *ret_devid) {
 
         char *np;
         int r;
@@ -86,7 +87,7 @@ static int acquire_esp(
          * we simply eat up the error here, so that --list and --status work too, without noise about
          * this). */
 
-        r = find_esp_and_warn(arg_esp_path, unprivileged_mode, &np, ret_part, ret_pstart, ret_psize, ret_uuid);
+        r = find_esp_and_warn(arg_esp_path, unprivileged_mode, &np, ret_part, ret_pstart, ret_psize, ret_uuid, ret_devid);
         if (r == -ENOKEY) {
                 if (graceful)
                         return log_info_errno(r, "Couldn't find EFI system partition, skipping.");
@@ -104,16 +105,23 @@ static int acquire_esp(
         return 1;
 }
 
-static int acquire_xbootldr(bool unprivileged_mode, sd_id128_t *ret_uuid) {
+static int acquire_xbootldr(
+                bool unprivileged_mode,
+                sd_id128_t *ret_uuid,
+                dev_t *ret_devid) {
+
         char *np;
         int r;
 
-        r = find_xbootldr_and_warn(arg_xbootldr_path, unprivileged_mode, &np, ret_uuid);
+        r = find_xbootldr_and_warn(arg_xbootldr_path, unprivileged_mode, &np, ret_uuid, ret_devid);
         if (r == -ENOKEY) {
                 log_debug_errno(r, "Didn't find an XBOOTLDR partition, using the ESP as $BOOT.");
+                arg_xbootldr_path = mfree(arg_xbootldr_path);
+
                 if (ret_uuid)
                         *ret_uuid = SD_ID128_NULL;
-                arg_xbootldr_path = mfree(arg_xbootldr_path);
+                if (ret_devid)
+                        *ret_devid = 0;
                 return 0;
         }
         if (r < 0)
@@ -1436,7 +1444,7 @@ static void print_yes_no_line(bool first, bool good, const char *name) {
 static int are_we_installed(void) {
         int r;
 
-        r = acquire_esp(/* privileged_mode= */ false, /* graceful= */ false, NULL, NULL, NULL, NULL);
+        r = acquire_esp(/* privileged_mode= */ false, /* graceful= */ false, NULL, NULL, NULL, NULL, NULL);
         if (r < 0)
                 return r;
 
@@ -1468,9 +1476,10 @@ static int are_we_installed(void) {
 
 static int verb_status(int argc, char *argv[], void *userdata) {
         sd_id128_t esp_uuid = SD_ID128_NULL, xbootldr_uuid = SD_ID128_NULL;
+        dev_t esp_devid = 0, xbootldr_devid = 0;
         int r, k;
 
-        r = acquire_esp(/* unprivileged_mode= */ geteuid() != 0, /* graceful= */ false, NULL, NULL, NULL, &esp_uuid);
+        r = acquire_esp(/* unprivileged_mode= */ geteuid() != 0, /* graceful= */ false, NULL, NULL, NULL, &esp_uuid, &esp_devid);
         if (arg_print_esp_path) {
                 if (r == -EACCES) /* If we couldn't acquire the ESP path, log about access errors (which is the only
                                    * error the find_esp_and_warn() won't log on its own) */
@@ -1481,7 +1490,7 @@ static int verb_status(int argc, char *argv[], void *userdata) {
                 puts(arg_esp_path);
         }
 
-        r = acquire_xbootldr(/* unprivileged_mode= */ geteuid() != 0, &xbootldr_uuid);
+        r = acquire_xbootldr(/* unprivileged_mode= */ geteuid() != 0, &xbootldr_uuid, &xbootldr_devid);
         if (arg_print_dollar_boot_path) {
                 if (r == -EACCES)
                         return log_error_errno(r, "Failed to determine XBOOTLDR location: %m");
@@ -1615,7 +1624,14 @@ static int verb_status(int argc, char *argv[], void *userdata) {
         }
 
         if (arg_esp_path || arg_xbootldr_path) {
-                k = status_entries(arg_esp_path, esp_uuid, arg_xbootldr_path, xbootldr_uuid);
+                /* If XBOOTLDR and ESP actually refer to the same block device, suppress XBOOTLDR, since it would find the same entries twice */
+                bool same = arg_esp_path && arg_xbootldr_path && devid_set_and_equal(esp_devid, xbootldr_devid);
+
+                k = status_entries(
+                                arg_esp_path,
+                                esp_uuid,
+                                same ? NULL : arg_xbootldr_path,
+                                same ? SD_ID128_NULL : xbootldr_uuid);
                 if (k < 0)
                         r = k;
         }
@@ -1626,25 +1642,29 @@ static int verb_status(int argc, char *argv[], void *userdata) {
 static int verb_list(int argc, char *argv[], void *userdata) {
         _cleanup_(boot_config_free) BootConfig config = {};
         _cleanup_strv_free_ char **efi_entries = NULL;
+        dev_t esp_devid = 0, xbootldr_devid = 0;
         int r;
 
         /* If we lack privileges we invoke find_esp_and_warn() in "unprivileged mode" here, which does two things: turn
          * off logging about access errors and turn off potentially privileged device probing. Here we're interested in
          * the latter but not the former, hence request the mode, and log about EACCES. */
 
-        r = acquire_esp(/* unprivileged_mode= */ geteuid() != 0, /* graceful= */ false, NULL, NULL, NULL, NULL);
+        r = acquire_esp(/* unprivileged_mode= */ geteuid() != 0, /* graceful= */ false, NULL, NULL, NULL, NULL, &esp_devid);
         if (r == -EACCES) /* We really need the ESP path for this call, hence also log about access errors */
                 return log_error_errno(r, "Failed to determine ESP: %m");
         if (r < 0)
                 return r;
 
-        r = acquire_xbootldr(/* unprivileged_mode= */ geteuid() != 0, NULL);
+        r = acquire_xbootldr(/* unprivileged_mode= */ geteuid() != 0, NULL, &xbootldr_devid);
         if (r == -EACCES)
                 return log_error_errno(r, "Failed to determine XBOOTLDR partition: %m");
         if (r < 0)
                 return r;
 
-        r = boot_entries_load_config(arg_esp_path, arg_xbootldr_path, &config);
+        /* If XBOOTLDR and ESP actually refer to the same block device, suppress XBOOTLDR, since it would find the same entries twice */
+        bool same = arg_esp_path && arg_xbootldr_path && devid_set_and_equal(esp_devid, xbootldr_devid);
+
+        r = boot_entries_load_config(arg_esp_path, same ? NULL : arg_xbootldr_path, &config);
         if (r < 0)
                 return r;
 
@@ -1840,7 +1860,7 @@ static int verb_install(int argc, char *argv[], void *userdata) {
         install = streq(argv[0], "install");
         graceful = !install && arg_graceful; /* support graceful mode for updates */
 
-        r = acquire_esp(/* unprivileged_mode= */ false, graceful, &part, &pstart, &psize, &uuid);
+        r = acquire_esp(/* unprivileged_mode= */ false, graceful, &part, &pstart, &psize, &uuid, NULL);
         if (graceful && r == -ENOKEY)
                 return 0; /* If --graceful is specified and we can't find an ESP, handle this cleanly */
         if (r < 0)
@@ -1857,7 +1877,7 @@ static int verb_install(int argc, char *argv[], void *userdata) {
                 }
         }
 
-        r = acquire_xbootldr(/* unprivileged_mode= */ false, NULL);
+        r = acquire_xbootldr(/* unprivileged_mode= */ false, NULL, NULL);
         if (r < 0)
                 return r;
 
@@ -1917,11 +1937,11 @@ static int verb_remove(int argc, char *argv[], void *userdata) {
         sd_id128_t uuid = SD_ID128_NULL;
         int r, q;
 
-        r = acquire_esp(/* unprivileged_mode= */ false, /* graceful= */ false, NULL, NULL, NULL, &uuid);
+        r = acquire_esp(/* unprivileged_mode= */ false, /* graceful= */ false, NULL, NULL, NULL, &uuid, NULL);
         if (r < 0)
                 return r;
 
-        r = acquire_xbootldr(/* unprivileged_mode= */ false, NULL);
+        r = acquire_xbootldr(/* unprivileged_mode= */ false, NULL, NULL);
         if (r < 0)
                 return r;
 
@@ -2130,7 +2150,7 @@ static int verb_set_efivar(int argc, char *argv[], void *userdata) {
 static int verb_random_seed(int argc, char *argv[], void *userdata) {
         int r;
 
-        r = find_esp_and_warn(arg_esp_path, false, &arg_esp_path, NULL, NULL, NULL, NULL);
+        r = find_esp_and_warn(arg_esp_path, false, &arg_esp_path, NULL, NULL, NULL, NULL, NULL);
         if (r == -ENOKEY) {
                 /* find_esp_and_warn() doesn't warn about ENOKEY, so let's do that on our own */
                 if (!arg_graceful)

diff --git a/src/shared/bootspec.c b/src/shared/bootspec.c
index 7c74cf6f182ca26b85c0cbefb718da5a80a50564..1dbeda23d9dc5c3817f67f6e02b8b4356f7ad7d8 100644 (file)
--- a/src/shared/bootspec.c
+++ b/src/shared/bootspec.c
@@ -780,6 +780,7 @@ int boot_entries_load_config_auto(
                 BootConfig *config) {
 
         _cleanup_free_ char *esp_where = NULL, *xbootldr_where = NULL;
+        dev_t esp_devid = 0, xbootldr_devid = 0;
         int r;
 
         assert(config);
@@ -800,14 +801,18 @@ int boot_entries_load_config_auto(
                                                "Failed to determine whether /run/boot-loader-entries/ exists: %m");
         }
 
-        r = find_esp_and_warn(override_esp_path, false, &esp_where, NULL, NULL, NULL, NULL);
+        r = find_esp_and_warn(override_esp_path, /* unprivileged_mode= */ false, &esp_where, NULL, NULL, NULL, NULL, &esp_devid);
         if (r < 0) /* we don't log about ENOKEY here, but propagate it, leaving it to the caller to log */
                 return r;
 
-        r = find_xbootldr_and_warn(override_xbootldr_path, false, &xbootldr_where, NULL);
+        r = find_xbootldr_and_warn(override_xbootldr_path, /* unprivileged_mode= */ false, &xbootldr_where, NULL, &xbootldr_devid);
         if (r < 0 && r != -ENOKEY)
                 return r; /* It's fine if the XBOOTLDR partition doesn't exist, hence we ignore ENOKEY here */
 
+        /* If both paths actually refer to the same inode, suppress the xbootldr path */
+        if (esp_where && xbootldr_where && devid_set_and_equal(esp_devid, xbootldr_devid))
+                xbootldr_where = mfree(xbootldr_where);
+
         return boot_entries_load_config(esp_where, xbootldr_where, config);
 }
 
@@ -1162,7 +1167,8 @@ static int verify_esp(
                 uint32_t *ret_part,
                 uint64_t *ret_pstart,
                 uint64_t *ret_psize,
-                sd_id128_t *ret_uuid) {
+                sd_id128_t *ret_uuid,
+                dev_t *ret_devid) {
 
         bool relax_checks;
         dev_t devid;
@@ -1212,9 +1218,16 @@ static int verify_esp(
          * however blkid can't work if we have no privileges to access block devices directly, which is why
          * we use udev in that case. */
         if (unprivileged_mode)
-                return verify_esp_udev(devid, searching, ret_part, ret_pstart, ret_psize, ret_uuid);
+                r = verify_esp_udev(devid, searching, ret_part, ret_pstart, ret_psize, ret_uuid);
         else
-                return verify_esp_blkid(devid, searching, ret_part, ret_pstart, ret_psize, ret_uuid);
+                r = verify_esp_blkid(devid, searching, ret_part, ret_pstart, ret_psize, ret_uuid);
+        if (r < 0)
+                return r;
+
+        if (ret_devid)
+                *ret_devid = devid;
+
+        return 0;
 
 finish:
         if (ret_part)
@@ -1225,6 +1238,8 @@ finish:
                 *ret_psize = 0;
         if (ret_uuid)
                 *ret_uuid = SD_ID128_NULL;
+        if (ret_devid)
+                *ret_devid = 0;
 
         return 0;
 }
@@ -1236,7 +1251,8 @@ int find_esp_and_warn(
                 uint32_t *ret_part,
                 uint64_t *ret_pstart,
                 uint64_t *ret_psize,
-                sd_id128_t *ret_uuid) {
+                sd_id128_t *ret_uuid,
+                dev_t *ret_devid) {
 
         int r;
 
@@ -1247,7 +1263,7 @@ int find_esp_and_warn(
          */
 
         if (path) {
-                r = verify_esp(path, false, unprivileged_mode, ret_part, ret_pstart, ret_psize, ret_uuid);
+                r = verify_esp(path, /* searching= */ false, unprivileged_mode, ret_part, ret_pstart, ret_psize, ret_uuid, ret_devid);
                 if (r < 0)
                         return r;
 
@@ -1256,13 +1272,21 @@ int find_esp_and_warn(
 
         path = getenv("SYSTEMD_ESP_PATH");
         if (path) {
+                struct stat st;
+
                 if (!path_is_valid(path) || !path_is_absolute(path))
                         return log_error_errno(SYNTHETIC_ERRNO(EINVAL),
                                                "$SYSTEMD_ESP_PATH does not refer to absolute path, refusing to use it: %s",
                                                path);
 
-                /* Note: when the user explicitly configured things with an env var we won't validate the mount
-                 * point. After all we want this to be useful for testing. */
+                /* Note: when the user explicitly configured things with an env var we won't validate the
+                 * path beyond checking it refers to a directory. After all we want this to be useful for
+                 * testing. */
+
+                if (stat(path, &st) < 0)
+                        return log_error_errno(errno, "Failed to stat '%s': %m", path);
+                if (!S_ISDIR(st.st_mode))
+                        return log_error_errno(SYNTHETIC_ERRNO(ENOTDIR), "ESP path '%s' is not a directory.", path);
 
                 if (ret_part)
                         *ret_part = 0;
@@ -1272,13 +1296,15 @@ int find_esp_and_warn(
                         *ret_psize = 0;
                 if (ret_uuid)
                         *ret_uuid = SD_ID128_NULL;
+                if (ret_devid)
+                        *ret_devid = st.st_dev;
 
                 goto found;
         }
 
         FOREACH_STRING(path, "/efi", "/boot", "/boot/efi") {
 
-                r = verify_esp(path, true, unprivileged_mode, ret_part, ret_pstart, ret_psize, ret_uuid);
+                r = verify_esp(path, /* searching= */ true, unprivileged_mode, ret_part, ret_pstart, ret_psize, ret_uuid, ret_devid);
                 if (r >= 0)
                         goto found;
                 if (!IN_SET(r, -ENOENT, -EADDRNOTAVAIL)) /* This one is not it */
@@ -1445,7 +1471,8 @@ static int verify_xbootldr(
                 const char *p,
                 bool searching,
                 bool unprivileged_mode,
-                sd_id128_t *ret_uuid) {
+                sd_id128_t *ret_uuid,
+                dev_t *ret_devid) {
 
         bool relax_checks;
         dev_t devid;
@@ -1463,13 +1490,22 @@ static int verify_xbootldr(
                 goto finish;
 
         if (unprivileged_mode)
-                return verify_xbootldr_udev(devid, searching, ret_uuid);
+                r = verify_xbootldr_udev(devid, searching, ret_uuid);
         else
-                return verify_xbootldr_blkid(devid, searching, ret_uuid);
+                r = verify_xbootldr_blkid(devid, searching, ret_uuid);
+        if (r < 0)
+                return r;
+
+        if (ret_devid)
+                *ret_devid = devid;
+
+        return 0;
 
 finish:
         if (ret_uuid)
                 *ret_uuid = SD_ID128_NULL;
+        if (ret_devid)
+                *ret_devid = 0;
 
         return 0;
 }
@@ -1478,14 +1514,15 @@ int find_xbootldr_and_warn(
                 const char *path,
                 bool unprivileged_mode,
                 char **ret_path,
-                sd_id128_t *ret_uuid) {
+                sd_id128_t *ret_uuid,
+                dev_t *ret_devid) {
 
         int r;
 
         /* Similar to find_esp_and_warn(), but finds the XBOOTLDR partition. Returns the same errors. */
 
         if (path) {
-                r = verify_xbootldr(path, false, unprivileged_mode, ret_uuid);
+                r = verify_xbootldr(path, /* searching= */ false, unprivileged_mode, ret_uuid, ret_devid);
                 if (r < 0)
                         return r;
 
@@ -1494,18 +1531,27 @@ int find_xbootldr_and_warn(
 
         path = getenv("SYSTEMD_XBOOTLDR_PATH");
         if (path) {
+                struct stat st;
+
                 if (!path_is_valid(path) || !path_is_absolute(path))
                         return log_error_errno(SYNTHETIC_ERRNO(EINVAL),
                                                "$SYSTEMD_XBOOTLDR_PATH does not refer to absolute path, refusing to use it: %s",
                                                path);
 
+                if (stat(path, &st) < 0)
+                        return log_error_errno(errno, "Failed to stat '%s': %m", path);
+                if (!S_ISDIR(st.st_mode))
+                        return log_error_errno(SYNTHETIC_ERRNO(ENOTDIR), "XBOOTLDR path '%s' is not a directory.", path);
+
                 if (ret_uuid)
                         *ret_uuid = SD_ID128_NULL;
+                if (ret_devid)
+                        *ret_devid = st.st_dev;
 
                 goto found;
         }
 
-        r = verify_xbootldr("/boot", true, unprivileged_mode, ret_uuid);
+        r = verify_xbootldr("/boot", true, unprivileged_mode, ret_uuid, ret_devid);
         if (r >= 0) {
                 path = "/boot";
                 goto found;

diff --git a/src/shared/bootspec.h b/src/shared/bootspec.h
index 6f1014db4a48138d0db02a54840650b392a0d08a..16353746aed64460ee60eac7c04406c9f3a14fe7 100644 (file)
--- a/src/shared/bootspec.h
+++ b/src/shared/bootspec.h
@@ -91,5 +91,5 @@ static inline const char* boot_entry_title(const BootEntry *entry) {
         return entry->show_title ?: entry->title ?: entry->id;
 }
 
-int find_esp_and_warn(const char *path, bool unprivileged_mode, char **ret_path, uint32_t *ret_part, uint64_t *ret_pstart, uint64_t *ret_psize, sd_id128_t *ret_uuid);
-int find_xbootldr_and_warn(const char *path, bool unprivileged_mode, char **ret_path,sd_id128_t *ret_uuid);
+int find_esp_and_warn(const char *path, bool unprivileged_mode, char **ret_path, uint32_t *ret_part, uint64_t *ret_pstart, uint64_t *ret_psize, sd_id128_t *ret_uuid, dev_t *ret_devid);
+int find_xbootldr_and_warn(const char *path, bool unprivileged_mode, char **ret_path,sd_id128_t *ret_uuid, dev_t *ret_devid);