From 0d1506d4a8e100c7ba311d77840b11afc4b76dd6 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 23 Mar 2022 16:08:36 +0100 Subject: [PATCH 01/10] bootctl: optionally, output entries in JSON format Replaces: #18387 Fixes: #18094 --- man/bootctl.xml | 6 +++++- src/boot/bootctl.c | 51 +++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 55 insertions(+), 2 deletions(-) diff --git a/man/bootctl.xml b/man/bootctl.xml index ff49a8043c..8d5b1d3d2c 100644 --- a/man/bootctl.xml +++ b/man/bootctl.xml @@ -92,7 +92,10 @@ url="https://systemd.io/BOOT_LOADER_SPECIFICATION">Boot Loader Specification, as well as any other entries discovered or automatically generated by a boot loader implementing the Boot Loader - Interface. + Interface. + + JSON output may be requested with . + @@ -315,6 +318,7 @@ + diff --git a/src/boot/bootctl.c b/src/boot/bootctl.c index cc79f51655..8596939ff3 100644 --- a/src/boot/bootctl.c +++ b/src/boot/bootctl.c @@ -70,6 +70,7 @@ static enum { ARG_ENTRY_TOKEN_AUTO, } arg_entry_token_type = ARG_ENTRY_TOKEN_AUTO; static char *arg_entry_token = NULL; +static JsonFormatFlags arg_json_format_flags = JSON_FORMAT_OFF; STATIC_DESTRUCTOR_REGISTER(arg_esp_path, freep); STATIC_DESTRUCTOR_REGISTER(arg_xbootldr_path, freep); @@ -1420,6 +1421,8 @@ static int help(int argc, char *argv[], void *userdata) { " Create $BOOT/ENTRY-TOKEN/ directory\n" " --entry-token=machine-id|os-id|os-image-id|auto|literal:…\n" " Entry token to use for this installation\n" + " --json=pretty|short|off\n" + " Generate JSON output\n" "\nSee the %2$s for details.\n", program_invocation_short_name, link, @@ -1441,6 +1444,7 @@ static int parse_argv(int argc, char *argv[]) { ARG_GRACEFUL, ARG_MAKE_ENTRY_DIRECTORY, ARG_ENTRY_TOKEN, + ARG_JSON, }; static const struct option options[] = { @@ -1458,6 +1462,7 @@ static int parse_argv(int argc, char *argv[]) { { "make-entry-directory", required_argument, NULL, ARG_MAKE_ENTRY_DIRECTORY }, { "make-machine-id-directory", required_argument, NULL, ARG_MAKE_ENTRY_DIRECTORY }, /* Compatibility alias */ { "entry-token", required_argument, NULL, ARG_ENTRY_TOKEN }, + { "json", required_argument, NULL, ARG_JSON }, {} }; @@ -1552,6 +1557,13 @@ static int parse_argv(int argc, char *argv[]) { } break; + case ARG_JSON: + r = parse_json_argument(optarg, &arg_json_format_flags); + if (r <= 0) + return r; + + break; + case '?': return -EINVAL; @@ -1812,7 +1824,44 @@ static int verb_list(int argc, char *argv[], void *userdata) { else (void) boot_entries_augment_from_loader(&config, efi_entries, /* only_auto= */ false); - if (config.n_entries == 0) + if (!FLAGS_SET(arg_json_format_flags, JSON_FORMAT_OFF)) { + + pager_open(arg_pager_flags); + + for (size_t i = 0; i < config.n_entries; i++) { + _cleanup_free_ char *opts = NULL; + BootEntry *e = config.entries + i; + _cleanup_(json_variant_unrefp) JsonVariant *v = NULL; + + if (!strv_isempty(e->options)) { + opts = strv_join(e->options, " "); + if (!opts) + return log_oom(); + } + + r = json_build(&v, JSON_BUILD_OBJECT( + JSON_BUILD_PAIR_CONDITION(e->id, "id", JSON_BUILD_STRING(e->id)), + JSON_BUILD_PAIR_CONDITION(e->path, "path", JSON_BUILD_STRING(e->path)), + JSON_BUILD_PAIR_CONDITION(e->root, "root", JSON_BUILD_STRING(e->root)), + JSON_BUILD_PAIR_CONDITION(e->title, "title", JSON_BUILD_STRING(e->title)), + JSON_BUILD_PAIR_CONDITION(boot_entry_title(e), "showTitle", JSON_BUILD_STRING(boot_entry_title(e))), + JSON_BUILD_PAIR_CONDITION(e->sort_key, "sortKey", JSON_BUILD_STRING(e->sort_key)), + JSON_BUILD_PAIR_CONDITION(e->version, "version", JSON_BUILD_STRING(e->version)), + JSON_BUILD_PAIR_CONDITION(e->machine_id, "machineId", JSON_BUILD_STRING(e->machine_id)), + JSON_BUILD_PAIR_CONDITION(e->architecture, "architecture", JSON_BUILD_STRING(e->architecture)), + JSON_BUILD_PAIR_CONDITION(opts, "options", JSON_BUILD_STRING(opts)), + JSON_BUILD_PAIR_CONDITION(e->kernel, "linux", JSON_BUILD_STRING(e->kernel)), + JSON_BUILD_PAIR_CONDITION(e->efi, "efi", JSON_BUILD_STRING(e->efi)), + JSON_BUILD_PAIR_CONDITION(!strv_isempty(e->initrd), "initrd", JSON_BUILD_STRV(e->initrd)), + JSON_BUILD_PAIR_CONDITION(e->device_tree, "devicetree", JSON_BUILD_STRING(e->device_tree)), + JSON_BUILD_PAIR_CONDITION(!strv_isempty(e->device_tree_overlay), "devicetreeOverlay", JSON_BUILD_STRV(e->device_tree_overlay)))); + if (r < 0) + return log_oom(); + + json_variant_dump(v, arg_json_format_flags, stdout, NULL); + } + + } else if (config.n_entries == 0) log_info("No boot loader entries found."); else { pager_open(arg_pager_flags); From d04f0331118175f302508fdee7b842fc65aee506 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 23 Mar 2022 16:09:08 +0100 Subject: [PATCH 02/10] bootspec: don't use conf_files_list() for finding type #1 entries We can't really use conf_files_list() for finding type #1 entries, since it is case-sensitive, but type #1 entries are typically placed on VFAT, i.e. are case-insensitive. hence, use readdir_all() instead, which is quite similar, but gives us all files, and allows us to do a case-insensitive check. While we are at it, use openat() on the open dir to open the file, and pass that around, to make things a tiny bit more race-free. --- src/shared/bootspec.c | 86 +++++++++++++++++++++++++++++-------------- 1 file changed, 59 insertions(+), 27 deletions(-) diff --git a/src/shared/bootspec.c b/src/shared/bootspec.c index 53e079bdd7..47a42912d3 100644 --- a/src/shared/bootspec.c +++ b/src/shared/bootspec.c @@ -13,6 +13,7 @@ #include "find-esp.h" #include "path-util.h" #include "pe-header.h" +#include "recurse-dir.h" #include "sort-util.h" #include "stat-util.h" #include "strv.h" @@ -40,39 +41,44 @@ static void boot_entry_free(BootEntry *entry) { } static int boot_entry_load( + FILE *f, const char *root, - const char *path, + const char *dir, + const char *id, BootEntry *entry) { _cleanup_(boot_entry_free) BootEntry tmp = { .type = BOOT_ENTRY_CONF, }; - _cleanup_fclose_ FILE *f = NULL; unsigned line = 1; char *c; int r; + assert(f); assert(root); - assert(path); + assert(dir); + assert(id); assert(entry); - r = path_extract_filename(path, &tmp.id); - if (r < 0) - return log_error_errno(r, "Failed to extract file name from path '%s': %m", path); + /* Loads a Type #1 boot menu entry from the specified FILE* object */ - c = endswith_no_case(tmp.id, ".conf"); + if (!efi_loader_entry_name_valid(id)) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Invalid loader entry name: %s", id); + + c = endswith_no_case(id, ".conf"); if (!c) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Invalid loader entry file suffix: %s", tmp.id); + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Invalid loader entry file suffix: %s", id); - if (!efi_loader_entry_name_valid(tmp.id)) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Invalid loader entry name: %s", tmp.id); + tmp.id = strdup(id); + if (!tmp.id) + return log_oom(); - tmp.id_old = strndup(tmp.id, c - tmp.id); + tmp.id_old = strndup(id, c - id); /* Without .conf suffix */ if (!tmp.id_old) return log_oom(); - tmp.path = strdup(path); + tmp.path = path_join(dir, id); if (!tmp.path) return log_oom(); @@ -80,10 +86,6 @@ static int boot_entry_load( if (!tmp.root) return log_oom(); - f = fopen(path, "re"); - if (!f) - return log_error_errno(errno, "Failed to open \"%s\": %m", path); - for (;;) { _cleanup_free_ char *buf = NULL, *field = NULL; const char *p; @@ -92,9 +94,9 @@ static int boot_entry_load( if (r == 0) break; if (r == -ENOBUFS) - return log_error_errno(r, "%s:%u: Line too long", path, line); + return log_error_errno(r, "%s:%u: Line too long", tmp.path, line); if (r < 0) - return log_error_errno(r, "%s:%u: Error while reading: %m", path, line); + return log_error_errno(r, "%s:%u: Error while reading: %m", tmp.path, line); line++; @@ -104,11 +106,11 @@ static int boot_entry_load( p = buf; r = extract_first_word(&p, &field, " \t", 0); if (r < 0) { - log_error_errno(r, "Failed to parse config file %s line %u: %m", path, line); + log_error_errno(r, "Failed to parse config file %s line %u: %m", tmp.path, line); continue; } if (r == 0) { - log_warning("%s:%u: Bad syntax", path, line); + log_warning("%s:%u: Bad syntax", tmp.path, line); continue; } @@ -141,11 +143,11 @@ static int boot_entry_load( r = strv_extend_strv(&tmp.device_tree_overlay, l, false); } else { - log_notice("%s:%u: Unknown line \"%s\", ignoring.", path, line, field); + log_notice("%s:%u: Unknown line \"%s\", ignoring.", tmp.path, line, field); continue; } if (r < 0) - return log_error_errno(r, "%s:%u: Error while reading: %m", path, line); + return log_error_errno(r, "%s:%u: Error while reading: %m", tmp.path, line); } *entry = tmp; @@ -278,7 +280,8 @@ static int boot_entries_find( BootEntry **entries, size_t *n_entries) { - _cleanup_strv_free_ char **files = NULL; + _cleanup_free_ DirectoryEntries *dentries = NULL; + _cleanup_close_ int dir_fd = -1; int r; assert(root); @@ -286,15 +289,44 @@ static int boot_entries_find( assert(entries); assert(n_entries); - r = conf_files_list(&files, ".conf", NULL, 0, dir); + dir_fd = open(dir, O_DIRECTORY|O_CLOEXEC); + if (dir_fd < 0) { + if (errno == ENOENT) + return 0; + + return log_error_errno(errno, "Failed to open '%s': %m", dir); + } + + r = readdir_all(dir_fd, RECURSE_DIR_IGNORE_DOT, &dentries); if (r < 0) - return log_error_errno(r, "Failed to list files in \"%s\": %m", dir); + return log_error_errno(r, "Failed to read directory '%s': %m", dir); + + for (size_t i = 0; i < dentries->n_entries; i++) { + const struct dirent *de = dentries->entries[i]; + _cleanup_fclose_ FILE *f = NULL; + + if (!dirent_is_file(de)) + continue; + + if (!endswith_no_case(de->d_name, ".conf")) + continue; - STRV_FOREACH(f, files) { if (!GREEDY_REALLOC0(*entries, *n_entries + 1)) return log_oom(); - r = boot_entry_load(root, *f, *entries + *n_entries); + 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); + continue; + } + + r = boot_entry_load(f, root, dir, de->d_name, *entries + *n_entries); if (r < 0) continue; From 85f4ae2f50c516cf2dc2343013ff61cdc08762f7 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 23 Mar 2022 16:19:07 +0100 Subject: [PATCH 03/10] bootspec: pass around BootConfig struct instead of entries/n_entries fields only This simplifies the parameter lists quite a bit, at zero change in behaviour. --- src/shared/bootspec.c | 37 ++++++++++++++++--------------------- 1 file changed, 16 insertions(+), 21 deletions(-) diff --git a/src/shared/bootspec.c b/src/shared/bootspec.c index 47a42912d3..b4365f80f6 100644 --- a/src/shared/bootspec.c +++ b/src/shared/bootspec.c @@ -275,19 +275,17 @@ static int boot_entry_compare(const BootEntry *a, const BootEntry *b) { } static int boot_entries_find( + BootConfig *config, const char *root, - const char *dir, - BootEntry **entries, - size_t *n_entries) { + const char *dir) { _cleanup_free_ DirectoryEntries *dentries = NULL; _cleanup_close_ int dir_fd = -1; int r; + assert(config); assert(root); assert(dir); - assert(entries); - assert(n_entries); dir_fd = open(dir, O_DIRECTORY|O_CLOEXEC); if (dir_fd < 0) { @@ -311,7 +309,7 @@ static int boot_entries_find( if (!endswith_no_case(de->d_name, ".conf")) continue; - if (!GREEDY_REALLOC0(*entries, *n_entries + 1)) + if (!GREEDY_REALLOC0(config->entries, config->n_entries + 1)) return log_oom(); r = xfopenat(dir_fd, de->d_name, "re", 0, &f); @@ -326,11 +324,11 @@ static int boot_entries_find( continue; } - r = boot_entry_load(f, root, dir, de->d_name, *entries + *n_entries); + r = boot_entry_load(f, root, dir, de->d_name, config->entries + config->n_entries); if (r < 0) continue; - (*n_entries) ++; + config->n_entries++; } return 0; @@ -540,18 +538,15 @@ static int find_sections( } static int boot_entries_find_unified( + BootConfig *config, const char *root, - const char *dir, - BootEntry **entries, - size_t *n_entries) { + const char *dir) { _cleanup_(closedirp) DIR *d = NULL; int r; - assert(root); + assert(config); assert(dir); - assert(entries); - assert(n_entries); d = opendir(dir); if (!d) { @@ -571,7 +566,7 @@ static int boot_entries_find_unified( if (!endswith_no_case(de->d_name, ".efi")) continue; - if (!GREEDY_REALLOC0(*entries, *n_entries + 1)) + if (!GREEDY_REALLOC0(config->entries, config->n_entries + 1)) return log_oom(); fd = openat(dirfd(d), de->d_name, O_RDONLY|O_CLOEXEC|O_NONBLOCK); @@ -594,11 +589,11 @@ static int boot_entries_find_unified( if (!j) return log_oom(); - r = boot_entry_load_unified(root, j, osrelease, cmdline, *entries + *n_entries); + r = boot_entry_load_unified(root, j, osrelease, cmdline, config->entries + config->n_entries); if (r < 0) continue; - (*n_entries) ++; + config->n_entries++; } return 0; @@ -791,24 +786,24 @@ int boot_entries_load_config( return r; p = strjoina(esp_path, "/loader/entries"); - r = boot_entries_find(esp_path, p, &config->entries, &config->n_entries); + r = boot_entries_find(config, esp_path, p); if (r < 0) return r; p = strjoina(esp_path, "/EFI/Linux/"); - r = boot_entries_find_unified(esp_path, p, &config->entries, &config->n_entries); + r = boot_entries_find_unified(config, esp_path, p); if (r < 0) return r; } if (xbootldr_path) { p = strjoina(xbootldr_path, "/loader/entries"); - r = boot_entries_find(xbootldr_path, p, &config->entries, &config->n_entries); + r = boot_entries_find(config, xbootldr_path, p); if (r < 0) return r; p = strjoina(xbootldr_path, "/EFI/Linux/"); - r = boot_entries_find_unified(xbootldr_path, p, &config->entries, &config->n_entries); + r = boot_entries_find_unified(config, xbootldr_path, p); if (r < 0) return r; } From d486a0eaaa435e50597327fa2f90947545a0312b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 23 Mar 2022 17:00:24 +0100 Subject: [PATCH 04/10] 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). --- src/shared/bootspec.c | 81 +++++++++++++++++++++++++++++++++++-------- src/shared/bootspec.h | 3 ++ 2 files changed, 70 insertions(+), 14 deletions(-) diff --git a/src/shared/bootspec.c b/src/shared/bootspec.c index b4365f80f6..6238517d9f 100644 --- 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); - continue; - } - - r = find_sections(fd, &osrelease, &cmdline); + 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; + + 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 0f5e6c9ad5..720aa0484f 100644 --- a/src/shared/bootspec.h +++ b/src/shared/bootspec.h @@ -7,6 +7,7 @@ #include #include +#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) { From 85e17916d3c0237d8c01bc7d01febd4b725466c9 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 23 Mar 2022 17:06:50 +0100 Subject: [PATCH 05/10] bootspec: rename type1 parsers to say "type1" explicitly in the name This just got too confusing for me. With this change we'll now have _unified as common suffix for stuff loading unified kernels (i.e. type 1), and _type1 as common suffix for type1 entries. Just some renaming. --- src/shared/bootspec.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/shared/bootspec.c b/src/shared/bootspec.c index 6238517d9f..448d65f7e4 100644 --- a/src/shared/bootspec.c +++ b/src/shared/bootspec.c @@ -40,7 +40,7 @@ static void boot_entry_free(BootEntry *entry) { strv_free(entry->device_tree_overlay); } -static int boot_entry_load( +static int boot_entry_load_type1( FILE *f, const char *root, const char *dir, @@ -328,7 +328,7 @@ static int config_check_inode_relevant_and_unseen(BootConfig *config, int fd, co return true; } -static int boot_entries_find( +static int boot_entries_find_type1( BootConfig *config, const char *root, const char *dir) { @@ -378,7 +378,7 @@ static int boot_entries_find( 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); + r = boot_entry_load_type1(f, root, dir, de->d_name, config->entries + config->n_entries); if (r < 0) continue; @@ -839,7 +839,7 @@ int boot_entries_load_config( return r; p = strjoina(esp_path, "/loader/entries"); - r = boot_entries_find(config, esp_path, p); + r = boot_entries_find_type1(config, esp_path, p); if (r < 0) return r; @@ -851,7 +851,7 @@ int boot_entries_load_config( if (xbootldr_path) { p = strjoina(xbootldr_path, "/loader/entries"); - r = boot_entries_find(config, xbootldr_path, p); + r = boot_entries_find_type1(config, xbootldr_path, p); if (r < 0) return r; From 92067ab672f1fcdfd31dfd41f6bca37e2fcc9e95 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 23 Mar 2022 17:15:23 +0100 Subject: [PATCH 06/10] bootspec: normalize oom handling in boot_load_efi_entry_pointers() OOM should usually be fatal, hence make it so here, too. --- src/shared/bootspec.c | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/src/shared/bootspec.c b/src/shared/bootspec.c index 448d65f7e4..52a1612f3d 100644 --- a/src/shared/bootspec.c +++ b/src/shared/bootspec.c @@ -799,25 +799,22 @@ static int boot_load_efi_entry_pointers(BootConfig *config) { /* Loads the three "pointers" to boot loader entries from their EFI variables */ r = efi_get_variable_string(EFI_LOADER_VARIABLE(LoaderEntryOneShot), &config->entry_oneshot); - if (r < 0 && !IN_SET(r, -ENOENT, -ENODATA)) { - log_warning_errno(r, "Failed to read EFI variable \"LoaderEntryOneShot\": %m"); - if (r == -ENOMEM) - return r; - } + if (r == -ENOMEM) + return log_oom(); + if (r < 0 && !IN_SET(r, -ENOENT, -ENODATA)) + log_warning_errno(r, "Failed to read EFI variable \"LoaderEntryOneShot\", ignoring: %m"); r = efi_get_variable_string(EFI_LOADER_VARIABLE(LoaderEntryDefault), &config->entry_default); - if (r < 0 && !IN_SET(r, -ENOENT, -ENODATA)) { - log_warning_errno(r, "Failed to read EFI variable \"LoaderEntryDefault\": %m"); - if (r == -ENOMEM) - return r; - } + if (r == -ENOMEM) + return log_oom(); + if (r < 0 && !IN_SET(r, -ENOENT, -ENODATA)) + log_warning_errno(r, "Failed to read EFI variable \"LoaderEntryDefault\", ignoring: %m"); r = efi_get_variable_string(EFI_LOADER_VARIABLE(LoaderEntrySelected), &config->entry_selected); - if (r < 0 && !IN_SET(r, -ENOENT, -ENODATA)) { - log_warning_errno(r, "Failed to read EFI variable \"LoaderEntrySelected\": %m"); - if (r == -ENOMEM) - return r; - } + if (r == -ENOMEM) + return log_oom(); + if (r < 0 && !IN_SET(r, -ENOENT, -ENODATA)) + log_warning_errno(r, "Failed to read EFI variable \"LoaderEntrySelected\", ignoring: %m"); return 1; } From f7a7a5e2675fbcf227700e952c053411200d5d15 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 23 Mar 2022 17:28:44 +0100 Subject: [PATCH 07/10] bootspec: assess default/selected entries *after* we augmented entry list with entries from loader Fixes: #22580 --- src/boot/bootctl.c | 12 ++++++++++-- src/login/logind-dbus.c | 4 ++-- src/shared/bootspec.c | 22 +++++++++++++++------- src/shared/bootspec.h | 8 ++++++++ src/systemctl/systemctl-start-special.c | 6 +++++- src/test/test-bootspec.c | 2 +- 6 files changed, 41 insertions(+), 13 deletions(-) diff --git a/src/boot/bootctl.c b/src/boot/bootctl.c index 8596939ff3..5c1d2533d7 100644 --- a/src/boot/bootctl.c +++ b/src/boot/bootctl.c @@ -687,7 +687,7 @@ static int status_entries( const char *xbootldr_path, sd_id128_t xbootldr_partition_uuid) { - _cleanup_(boot_config_free) BootConfig config = {}; + _cleanup_(boot_config_free) BootConfig config = BOOT_CONFIG_NULL; sd_id128_t dollar_boot_partition_uuid; const char *dollar_boot_path; int r; @@ -713,6 +713,10 @@ static int status_entries( if (r < 0) return r; + r = boot_config_select_special_entries(&config); + if (r < 0) + return r; + if (config.default_entry < 0) printf("%zu entries, no entry could be determined as default.\n", config.n_entries); else { @@ -1788,7 +1792,7 @@ 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_(boot_config_free) BootConfig config = BOOT_CONFIG_NULL; _cleanup_strv_free_ char **efi_entries = NULL; dev_t esp_devid = 0, xbootldr_devid = 0; int r; @@ -1824,6 +1828,10 @@ static int verb_list(int argc, char *argv[], void *userdata) { else (void) boot_entries_augment_from_loader(&config, efi_entries, /* only_auto= */ false); + r = boot_config_select_special_entries(&config); + if (r < 0) + return r; + if (!FLAGS_SET(arg_json_format_flags, JSON_FORMAT_OFF)) { pager_open(arg_pager_flags); diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c index 7767096619..04a72bd9b3 100644 --- a/src/login/logind-dbus.c +++ b/src/login/logind-dbus.c @@ -3002,7 +3002,7 @@ static int property_get_reboot_to_boot_loader_entry( } static int boot_loader_entry_exists(Manager *m, const char *id) { - _cleanup_(boot_config_free) BootConfig config = {}; + _cleanup_(boot_config_free) BootConfig config = BOOT_CONFIG_NULL; int r; assert(m); @@ -3157,7 +3157,7 @@ static int property_get_boot_loader_entries( void *userdata, sd_bus_error *error) { - _cleanup_(boot_config_free) BootConfig config = {}; + _cleanup_(boot_config_free) BootConfig config = BOOT_CONFIG_NULL; Manager *m = userdata; size_t i; int r; diff --git a/src/shared/bootspec.c b/src/shared/bootspec.c index 52a1612f3d..188ab1d663 100644 --- a/src/shared/bootspec.c +++ b/src/shared/bootspec.c @@ -819,6 +819,21 @@ static int boot_load_efi_entry_pointers(BootConfig *config) { return 1; } +int boot_config_select_special_entries(BootConfig *config) { + int r; + + assert(config); + + r = boot_load_efi_entry_pointers(config); + if (r < 0) + return r; + + config->default_entry = boot_entries_select_default(config); + config->selected_entry = boot_entries_select_selected(config); + + return 0; +} + int boot_entries_load_config( const char *esp_path, const char *xbootldr_path, @@ -864,13 +879,6 @@ int boot_entries_load_config( if (r < 0) return log_error_errno(r, "Failed to uniquify boot entries: %m"); - r = boot_load_efi_entry_pointers(config); - if (r < 0) - return r; - - config->default_entry = boot_entries_select_default(config); - config->selected_entry = boot_entries_select_selected(config); - return 0; } diff --git a/src/shared/bootspec.h b/src/shared/bootspec.h index 720aa0484f..5c4b211ee4 100644 --- a/src/shared/bootspec.h +++ b/src/shared/bootspec.h @@ -62,6 +62,12 @@ typedef struct BootConfig { Set *inodes_seen; } BootConfig; +#define BOOT_CONFIG_NULL \ + { \ + .default_entry = -1, \ + .selected_entry = -1, \ + } + static inline BootEntry* boot_config_find_entry(BootConfig *config, const char *id) { assert(config); assert(id); @@ -88,6 +94,8 @@ int boot_entries_load_config(const char *esp_path, const char *xbootldr_path, Bo int boot_entries_load_config_auto(const char *override_esp_path, const char *override_xbootldr_path, BootConfig *config); int boot_entries_augment_from_loader(BootConfig *config, char **list, bool only_auto); +int boot_config_select_special_entries(BootConfig *config); + static inline const char* boot_entry_title(const BootEntry *entry) { assert(entry); diff --git a/src/systemctl/systemctl-start-special.c b/src/systemctl/systemctl-start-special.c index da2336d3a2..6b41f4f298 100644 --- a/src/systemctl/systemctl-start-special.c +++ b/src/systemctl/systemctl-start-special.c @@ -18,7 +18,7 @@ #include "systemctl.h" static int load_kexec_kernel(void) { - _cleanup_(boot_config_free) BootConfig config = {}; + _cleanup_(boot_config_free) BootConfig config = BOOT_CONFIG_NULL; _cleanup_free_ char *kernel = NULL, *initrd = NULL, *options = NULL; const BootEntry *e; pid_t pid; @@ -43,6 +43,10 @@ static int load_kexec_kernel(void) { if (r < 0) return r; + r = boot_config_select_special_entries(&config); + if (r < 0) + return r; + e = boot_config_default_entry(&config); if (!e) return log_error_errno(SYNTHETIC_ERRNO(ENOENT), diff --git a/src/test/test-bootspec.c b/src/test/test-bootspec.c index 7ba44744ba..f52b53c5f8 100644 --- a/src/test/test-bootspec.c +++ b/src/test/test-bootspec.c @@ -61,7 +61,7 @@ TEST_RET(bootspec_sort) { }; _cleanup_(rm_rf_physical_and_freep) char *d = NULL; - _cleanup_(boot_config_free) BootConfig config = {}; + _cleanup_(boot_config_free) BootConfig config = BOOT_CONFIG_NULL; assert_se(mkdtemp_malloc("/tmp/bootspec-testXXXXXX", &d) >= 0); From d412691a91fc29e2a8204fe4b3494426fbb76cac Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 24 Mar 2022 17:06:16 +0100 Subject: [PATCH 08/10] bootctl: use boot_config_default_entry() where appropriate --- src/boot/bootctl.c | 2 +- src/shared/bootspec.h | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/boot/bootctl.c b/src/boot/bootctl.c index 5c1d2533d7..2766c8452f 100644 --- a/src/boot/bootctl.c +++ b/src/boot/bootctl.c @@ -723,7 +723,7 @@ static int status_entries( printf("Default Boot Loader Entry:\n"); r = boot_entry_show( - config.entries + config.default_entry, + boot_config_default_entry(&config), /* show_as_default= */ false, /* show_as_selected= */ false, /* show_discovered= */ false); diff --git a/src/shared/bootspec.h b/src/shared/bootspec.h index 5c4b211ee4..0f199d5ee9 100644 --- a/src/shared/bootspec.h +++ b/src/shared/bootspec.h @@ -56,6 +56,7 @@ typedef struct BootConfig { BootEntry *entries; size_t n_entries; + ssize_t default_entry; ssize_t selected_entry; @@ -86,10 +87,12 @@ static inline BootEntry* boot_config_default_entry(BootConfig *config) { if (config->default_entry < 0) return NULL; + assert((size_t) config->default_entry < config->n_entries); return config->entries + config->default_entry; } void boot_config_free(BootConfig *config); + int boot_entries_load_config(const char *esp_path, const char *xbootldr_path, BootConfig *config); int boot_entries_load_config_auto(const char *override_esp_path, const char *override_xbootldr_path, BootConfig *config); int boot_entries_augment_from_loader(BootConfig *config, char **list, bool only_auto); From 3f8e42c03828ff2da0392111b394fac6f5d3c3da Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 24 Mar 2022 17:08:09 +0100 Subject: [PATCH 09/10] bootspec: don't needlessly inline boot_config_find_entry() the function contains a loop and if expressions and whatnot. Let's define it as regular function, to make the header easier to read and let the compiler more freedom on whether to inline this or not. --- src/shared/bootspec.c | 12 ++++++++++++ src/shared/bootspec.h | 12 +----------- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/src/shared/bootspec.c b/src/shared/bootspec.c index 188ab1d663..fd08e22bf9 100644 --- a/src/shared/bootspec.c +++ b/src/shared/bootspec.c @@ -986,3 +986,15 @@ int boot_entries_augment_from_loader( return 0; } + +BootEntry* boot_config_find_entry(BootConfig *config, const char *id) { + assert(config); + assert(id); + + for (size_t j = 0; j < config->n_entries; j++) + if (streq_ptr(config->entries[j].id, id) || + streq_ptr(config->entries[j].id_old, id)) + return config->entries + j; + + return NULL; +} diff --git a/src/shared/bootspec.h b/src/shared/bootspec.h index 0f199d5ee9..02ccd90aee 100644 --- a/src/shared/bootspec.h +++ b/src/shared/bootspec.h @@ -69,17 +69,7 @@ typedef struct BootConfig { .selected_entry = -1, \ } -static inline BootEntry* boot_config_find_entry(BootConfig *config, const char *id) { - assert(config); - assert(id); - - for (size_t j = 0; j < config->n_entries; j++) - if (streq_ptr(config->entries[j].id, id) || - streq_ptr(config->entries[j].id_old, id)) - return config->entries + j; - - return NULL; -} +BootEntry* boot_config_find_entry(BootConfig *config, const char *id); static inline BootEntry* boot_config_default_entry(BootConfig *config) { assert(config); From af9ae7502614741601565ba94569570363d9ac39 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 24 Mar 2022 17:15:50 +0100 Subject: [PATCH 10/10] bootspec: normalize function names/parameter lists This normalizes naming of functions operating on BootConfig objects. Let's always call them boot_config_xyz(), like our usual way to name stuff. moreover, move the BootConfig parameter to the beginning, as it's not a return value (which we typically move to the end of the parameter list), but simply an object, that even happens to be initialized already. With these changes the functions are more like our usual way to call things, and less surprises are good. --- src/boot/bootctl.c | 6 +++--- src/login/logind-dbus.c | 8 ++++---- src/shared/bootspec.c | 18 +++++++++--------- src/shared/bootspec.h | 6 +++--- src/systemctl/systemctl-start-special.c | 2 +- src/test/test-bootspec.c | 2 +- 6 files changed, 21 insertions(+), 21 deletions(-) diff --git a/src/boot/bootctl.c b/src/boot/bootctl.c index 2766c8452f..759b3ca86a 100644 --- a/src/boot/bootctl.c +++ b/src/boot/bootctl.c @@ -709,7 +709,7 @@ static int status_entries( SD_ID128_FORMAT_VAL(dollar_boot_partition_uuid)); printf("\n\n"); - r = boot_entries_load_config(esp_path, xbootldr_path, &config); + r = boot_config_load(&config, esp_path, xbootldr_path); if (r < 0) return r; @@ -1816,7 +1816,7 @@ static int verb_list(int argc, char *argv[], void *userdata) { /* 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); + r = boot_config_load(&config, arg_esp_path, same ? NULL : arg_xbootldr_path); if (r < 0) return r; @@ -1826,7 +1826,7 @@ static int verb_list(int argc, char *argv[], void *userdata) { else if (r < 0) log_warning_errno(r, "Failed to determine entries reported by boot loader, ignoring: %m"); else - (void) boot_entries_augment_from_loader(&config, efi_entries, /* only_auto= */ false); + (void) boot_config_augment_from_loader(&config, efi_entries, /* only_auto= */ false); r = boot_config_select_special_entries(&config); if (r < 0) diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c index 04a72bd9b3..1212a1ac53 100644 --- a/src/login/logind-dbus.c +++ b/src/login/logind-dbus.c @@ -3008,13 +3008,13 @@ static int boot_loader_entry_exists(Manager *m, const char *id) { assert(m); assert(id); - r = boot_entries_load_config_auto(NULL, NULL, &config); + r = boot_config_load_auto(&config, NULL, NULL); if (r < 0 && r != -ENOKEY) /* don't complain if no GPT is found, hence skip ENOKEY */ return r; r = manager_read_efi_boot_loader_entries(m); if (r >= 0) - (void) boot_entries_augment_from_loader(&config, m->efi_boot_loader_entries, /* auto_only= */ true); + (void) boot_config_augment_from_loader(&config, m->efi_boot_loader_entries, /* auto_only= */ true); return !!boot_config_find_entry(&config, id); } @@ -3166,13 +3166,13 @@ static int property_get_boot_loader_entries( assert(reply); assert(m); - r = boot_entries_load_config_auto(NULL, NULL, &config); + r = boot_config_load_auto(&config, NULL, NULL); if (r < 0 && r != -ENOKEY) /* don't complain if there's no GPT found */ return r; r = manager_read_efi_boot_loader_entries(m); if (r >= 0) - (void) boot_entries_augment_from_loader(&config, m->efi_boot_loader_entries, /* auto_only= */ true); + (void) boot_config_augment_from_loader(&config, m->efi_boot_loader_entries, /* auto_only= */ true); r = sd_bus_message_open_container(reply, 'a', "s"); if (r < 0) diff --git a/src/shared/bootspec.c b/src/shared/bootspec.c index fd08e22bf9..7016f3840e 100644 --- a/src/shared/bootspec.c +++ b/src/shared/bootspec.c @@ -834,10 +834,10 @@ int boot_config_select_special_entries(BootConfig *config) { return 0; } -int boot_entries_load_config( +int boot_config_load( + BootConfig *config, const char *esp_path, - const char *xbootldr_path, - BootConfig *config) { + const char *xbootldr_path) { const char *p; int r; @@ -882,10 +882,10 @@ int boot_entries_load_config( return 0; } -int boot_entries_load_config_auto( +int boot_config_load_auto( + BootConfig *config, const char *override_esp_path, - const char *override_xbootldr_path, - BootConfig *config) { + const char *override_xbootldr_path) { _cleanup_free_ char *esp_where = NULL, *xbootldr_where = NULL; dev_t esp_devid = 0, xbootldr_devid = 0; @@ -902,7 +902,7 @@ int boot_entries_load_config_auto( if (!override_esp_path && !override_xbootldr_path) { if (access("/run/boot-loader-entries/", F_OK) >= 0) - return boot_entries_load_config("/run/boot-loader-entries/", NULL, config); + return boot_config_load(config, "/run/boot-loader-entries/", NULL); if (errno != ENOENT) return log_error_errno(errno, @@ -921,10 +921,10 @@ int boot_entries_load_config_auto( 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); + return boot_config_load(config, esp_where, xbootldr_where); } -int boot_entries_augment_from_loader( +int boot_config_augment_from_loader( BootConfig *config, char **found_by_loader, bool only_auto) { diff --git a/src/shared/bootspec.h b/src/shared/bootspec.h index 02ccd90aee..99bc5f69a4 100644 --- a/src/shared/bootspec.h +++ b/src/shared/bootspec.h @@ -83,9 +83,9 @@ static inline BootEntry* boot_config_default_entry(BootConfig *config) { void boot_config_free(BootConfig *config); -int boot_entries_load_config(const char *esp_path, const char *xbootldr_path, BootConfig *config); -int boot_entries_load_config_auto(const char *override_esp_path, const char *override_xbootldr_path, BootConfig *config); -int boot_entries_augment_from_loader(BootConfig *config, char **list, bool only_auto); +int boot_config_load(BootConfig *config, const char *esp_path, const char *xbootldr_path); +int boot_config_load_auto(BootConfig *config, const char *override_esp_path, const char *override_xbootldr_path); +int boot_config_augment_from_loader(BootConfig *config, char **list, bool only_auto); int boot_config_select_special_entries(BootConfig *config); diff --git a/src/systemctl/systemctl-start-special.c b/src/systemctl/systemctl-start-special.c index 6b41f4f298..7a6cef3c5e 100644 --- a/src/systemctl/systemctl-start-special.c +++ b/src/systemctl/systemctl-start-special.c @@ -32,7 +32,7 @@ static int load_kexec_kernel(void) { if (access(KEXEC, X_OK) < 0) return log_error_errno(errno, KEXEC" is not available: %m"); - r = boot_entries_load_config_auto(NULL, NULL, &config); + r = boot_config_load_auto(&config, NULL, NULL); if (r == -ENOKEY) /* The call doesn't log about ENOKEY, let's do so here. */ return log_error_errno(r, diff --git a/src/test/test-bootspec.c b/src/test/test-bootspec.c index f52b53c5f8..46acd60e0d 100644 --- a/src/test/test-bootspec.c +++ b/src/test/test-bootspec.c @@ -74,7 +74,7 @@ TEST_RET(bootspec_sort) { assert_se(write_string_file(j, entries[i].contents, WRITE_STRING_FILE_CREATE|WRITE_STRING_FILE_MKDIR_0755) >= 0); } - assert_se(boot_entries_load_config(d, NULL, &config) >= 0); + assert_se(boot_config_load(&config, d, NULL) >= 0); assert_se(config.n_entries == 6);