bootspec: try harder to suppress duplicate enumerated entries

For testing purposes I run one of my system symlinking /boot/loader/ to
/efi/loader/. This triggers annoying behaviour in boot entry
enumeration: the code ends up iterating through the 'entries' subdir of
both, thinking one was actually in the ESP and the other in XBOOTLDR,
and thus distinct. This would result in duplicate entries.

Let's address that, and filter out duplicates via their inode numbers:
never process the same inode twice. This should protect us from any
confusion effectively, regardless which inodes are symlinked (or even
bind mounted).

diff --git a/src/shared/bootspec.c b/src/shared/bootspec.c
index b4365f80f6e7b8c54e37f2f40ef6f42933669714..6238517d9f292088ee0a71d189e06823ef48afe9 100644 (file)
--- a/src/shared/bootspec.c
+++ b/src/shared/bootspec.c
@@ -174,6 +174,8 @@ void boot_config_free(BootConfig *config) {
         for (size_t i = 0; i < config->n_entries; i++)
                 boot_entry_free(config->entries + i);
         free(config->entries);
+
+        set_free(config->inodes_seen);
 }
 
 static int boot_loader_read_conf(const char *path, BootConfig *config) {
@@ -274,6 +276,58 @@ static int boot_entry_compare(const BootEntry *a, const BootEntry *b) {
         return -strverscmp_improved(a->id, b->id);
 }
 
+static void inode_hash_func(const struct stat *q, struct siphash *state) {
+        siphash24_compress(&q->st_dev, sizeof(q->st_dev), state);
+        siphash24_compress(&q->st_ino, sizeof(q->st_ino), state);
+}
+
+static int inode_compare_func(const struct stat *a, const struct stat *b) {
+        int r;
+
+        r = CMP(a->st_dev, b->st_dev);
+        if (r != 0)
+                return r;
+
+        return CMP(a->st_ino, b->st_ino);
+}
+
+DEFINE_HASH_OPS_WITH_KEY_DESTRUCTOR(inode_hash_ops, struct stat, inode_hash_func, inode_compare_func, free);
+
+static int config_check_inode_relevant_and_unseen(BootConfig *config, int fd, const char *fname) {
+        _cleanup_free_ char *d = NULL;
+        struct stat st;
+
+        assert(config);
+        assert(fd >= 0);
+        assert(fname);
+
+        /* So, here's the thing: because of the mess around /efi/ vs. /boot/ vs. /boot/efi/ it might be that
+         * people have these dirs, or subdirs of them symlinked or bind mounted, and we might end up
+         * iterating though some dirs multiple times. Let's thus rather be safe than sorry, and track the
+         * inodes we already processed: let's ignore inodes we have seen already. This should be robust
+         * against any form of symlinking or bind mounting, and effectively suppress any such duplicates. */
+
+        if (fstat(fd, &st) < 0)
+                return log_error_errno(errno, "Failed to stat('%s'): %m", fname);
+        if (!S_ISREG(st.st_mode)) {
+                log_debug("File '%s' is not a reguar file, ignoring.", fname);
+                return false;
+        }
+
+        if (set_contains(config->inodes_seen, &st)) {
+                log_debug("Inode '%s' already seen before, ignoring.", fname);
+                return false;
+        }
+        d = memdup(&st, sizeof(st));
+        if (!d)
+                return log_oom();
+        if (set_ensure_put(&config->inodes_seen, &inode_hash_ops, d) < 0)
+                return log_oom();
+
+        TAKE_PTR(d);
+        return true;
+}
+
 static int boot_entries_find(
                 BootConfig *config,
                 const char *root,
@@ -309,20 +363,20 @@ static int boot_entries_find(
                 if (!endswith_no_case(de->d_name, ".conf"))
                         continue;
 
-                if (!GREEDY_REALLOC0(config->entries, config->n_entries + 1))
-                        return log_oom();
-
                 r = xfopenat(dir_fd, de->d_name, "re", 0, &f);
                 if (r < 0) {
                         log_warning_errno(r, "Failed to open %s/%s, ignoring: %m", dir, de->d_name);
                         continue;
                 }
 
-                r = fd_verify_regular(fileno(f));
-                if (r < 0) {
-                        log_warning_errno(r, "File %s/%s is not regular, ignoring: %m", dir, de->d_name);
+                r = config_check_inode_relevant_and_unseen(config, fileno(f), de->d_name);
+                if (r < 0)
+                        return r;
+                if (r == 0) /* inode already seen or otherwise not relevant */
                         continue;
-                }
+
+                if (!GREEDY_REALLOC0(config->entries, config->n_entries + 1))
+                        return log_oom();
 
                 r = boot_entry_load(f, root, dir, de->d_name, config->entries + config->n_entries);
                 if (r < 0)
@@ -575,14 +629,13 @@ static int boot_entries_find_unified(
                         continue;
                 }
 
-                r = fd_verify_regular(fd);
-                if (r < 0) {
-                        log_warning_errno(r, "File %s/%s is not regular, ignoring: %m", dir, de->d_name);
+                r = config_check_inode_relevant_and_unseen(config, fd, de->d_name);
+                if (r < 0)
+                        return r;
+                if (r == 0) /* inode already seen or otherwise not relevant */
                         continue;
-                }
 
-                r = find_sections(fd, &osrelease, &cmdline);
-                if (r < 0)
+                if (find_sections(fd, &osrelease, &cmdline) < 0)
                         continue;
 
                 j = path_join(dir, de->d_name);

diff --git a/src/shared/bootspec.h b/src/shared/bootspec.h
index 0f5e6c9ad50c21c3b096b32c4613906aeae79913..720aa0484f837afad012b8598462c59f3e68b709 100644 (file)
--- a/src/shared/bootspec.h
+++ b/src/shared/bootspec.h
@@ -7,6 +7,7 @@
 #include <stdbool.h>
 #include <sys/types.h>
 
+#include "set.h"
 #include "string-util.h"
 
 typedef enum BootEntryType {
@@ -57,6 +58,8 @@ typedef struct BootConfig {
         size_t n_entries;
         ssize_t default_entry;
         ssize_t selected_entry;
+
+        Set *inodes_seen;
 } BootConfig;
 
 static inline BootEntry* boot_config_find_entry(BootConfig *config, const char *id) {