| Message ID | 20200923161404.17811-2-ardb@kernel.org |
|---|---|
| State | Accepted |
| Commit | 232f4eb6393f42f7f9418560ae9228e747fc6faf |
| Headers | show |
| Series | [1/7] efi: pstore: disentangle from deprecated efivars module | expand |
On Wed, Sep 23, 2020 at 06:13:58PM +0200, Ard Biesheuvel wrote: > The EFI pstore implementation relies on the 'efivars' abstraction, > which encapsulates the EFI variable store in a way that can be > overridden by other backing stores, like the Google SMI one. > > On top of that, the EFI pstore implementation also relies on the > efivars.ko module, which is a separate layer built on top of the > 'efivars' abstraction that exposes the [deprecated] sysfs entries > for each variable that exists in the backing store. > > Since the efivars.ko module is deprecated, and all users appear to > have moved to the efivarfs file system instead, let's prepare for > its removal, by removing EFI pstore's dependency on it. > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> With this and the other pstore patch, do the pstore self-tests still pass on an EFI system? If so, please consider both: Acked-by: Kees Cook <keescook@chromium.org> Thanks!
On Wed, 23 Sep 2020 at 20:41, Kees Cook <keescook@chromium.org> wrote: > > On Wed, Sep 23, 2020 at 06:13:58PM +0200, Ard Biesheuvel wrote: > > The EFI pstore implementation relies on the 'efivars' abstraction, > > which encapsulates the EFI variable store in a way that can be > > overridden by other backing stores, like the Google SMI one. > > > > On top of that, the EFI pstore implementation also relies on the > > efivars.ko module, which is a separate layer built on top of the > > 'efivars' abstraction that exposes the [deprecated] sysfs entries > > for each variable that exists in the backing store. > > > > Since the efivars.ko module is deprecated, and all users appear to > > have moved to the efivarfs file system instead, let's prepare for > > its removal, by removing EFI pstore's dependency on it. > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > With this and the other pstore patch, do the pstore self-tests still > pass on an EFI system? > > If so, please consider both: > > Acked-by: Kees Cook <keescook@chromium.org> > Selftests? Excellent! Are they documented too?
On Wed, Sep 23, 2020 at 08:43:21PM +0200, Ard Biesheuvel wrote: > On Wed, 23 Sep 2020 at 20:41, Kees Cook <keescook@chromium.org> wrote: > > > > On Wed, Sep 23, 2020 at 06:13:58PM +0200, Ard Biesheuvel wrote: > > > The EFI pstore implementation relies on the 'efivars' abstraction, > > > which encapsulates the EFI variable store in a way that can be > > > overridden by other backing stores, like the Google SMI one. > > > > > > On top of that, the EFI pstore implementation also relies on the > > > efivars.ko module, which is a separate layer built on top of the > > > 'efivars' abstraction that exposes the [deprecated] sysfs entries > > > for each variable that exists in the backing store. > > > > > > Since the efivars.ko module is deprecated, and all users appear to > > > have moved to the efivarfs file system instead, let's prepare for > > > its removal, by removing EFI pstore's dependency on it. > > > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > > > With this and the other pstore patch, do the pstore self-tests still > > pass on an EFI system? > > > > If so, please consider both: > > > > Acked-by: Kees Cook <keescook@chromium.org> > > > > Selftests? Excellent! Are they documented too? Not really, but they're pretty simple: cd tools/testing/selftests/pstore *double-check "config" against running kernel config* ./pstore_tests ./pstore_crash_test *wait for system to reboot* cd tools/testing/selftests/pstore ./pstore_post_reboot_tests (though please test before/after, just to make sure other deltas haven't broken things before your series -- I don't test EFI pstore with high frequency)
On Wed, 23 Sep 2020 at 23:02, Kees Cook <keescook@chromium.org> wrote: > > On Wed, Sep 23, 2020 at 08:43:21PM +0200, Ard Biesheuvel wrote: > > On Wed, 23 Sep 2020 at 20:41, Kees Cook <keescook@chromium.org> wrote: > > > > > > On Wed, Sep 23, 2020 at 06:13:58PM +0200, Ard Biesheuvel wrote: > > > > The EFI pstore implementation relies on the 'efivars' abstraction, > > > > which encapsulates the EFI variable store in a way that can be > > > > overridden by other backing stores, like the Google SMI one. > > > > > > > > On top of that, the EFI pstore implementation also relies on the > > > > efivars.ko module, which is a separate layer built on top of the > > > > 'efivars' abstraction that exposes the [deprecated] sysfs entries > > > > for each variable that exists in the backing store. > > > > > > > > Since the efivars.ko module is deprecated, and all users appear to > > > > have moved to the efivarfs file system instead, let's prepare for > > > > its removal, by removing EFI pstore's dependency on it. > > > > > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > > > > > With this and the other pstore patch, do the pstore self-tests still > > > pass on an EFI system? > > > > > > If so, please consider both: > > > > > > Acked-by: Kees Cook <keescook@chromium.org> > > > > > > > Selftests? Excellent! Are they documented too? > > Not really, but they're pretty simple: > > cd tools/testing/selftests/pstore > *double-check "config" against running kernel config* > ./pstore_tests > ./pstore_crash_test > *wait for system to reboot* > cd tools/testing/selftests/pstore > ./pstore_post_reboot_tests > > (though please test before/after, just to make sure other deltas haven't > broken things before your series -- I don't test EFI pstore with high > frequency) > I have done the 'before' test on three different EFI Linux systems (x86, arm64 and ARM), and they all give me something like the below === Pstore unit tests (pstore_tests) === UUID=109d02e6-9395-4274-9554-2c078e87a662 Checking pstore backend is registered ... ok backend=efi cmdline=BOOT_IMAGE=/vmlinuz-5.3.0-59-generic root=/dev/mapper/crypt-root ro quiet splash vt.handoff=1 Checking pstore console is registered ... FAIL Checking /dev/pmsg0 exists ... FAIL Writing unique string to /dev/pmsg0 ... FAIL So I'm not sure if there is any point to doing the 'after' test if this is the baseline.
On Thu, 24 Sep 2020 at 11:45, Ard Biesheuvel <ardb@kernel.org> wrote: > > On Wed, 23 Sep 2020 at 23:02, Kees Cook <keescook@chromium.org> wrote: > > > > On Wed, Sep 23, 2020 at 08:43:21PM +0200, Ard Biesheuvel wrote: > > > On Wed, 23 Sep 2020 at 20:41, Kees Cook <keescook@chromium.org> wrote: > > > > > > > > On Wed, Sep 23, 2020 at 06:13:58PM +0200, Ard Biesheuvel wrote: > > > > > The EFI pstore implementation relies on the 'efivars' abstraction, > > > > > which encapsulates the EFI variable store in a way that can be > > > > > overridden by other backing stores, like the Google SMI one. > > > > > > > > > > On top of that, the EFI pstore implementation also relies on the > > > > > efivars.ko module, which is a separate layer built on top of the > > > > > 'efivars' abstraction that exposes the [deprecated] sysfs entries > > > > > for each variable that exists in the backing store. > > > > > > > > > > Since the efivars.ko module is deprecated, and all users appear to > > > > > have moved to the efivarfs file system instead, let's prepare for > > > > > its removal, by removing EFI pstore's dependency on it. > > > > > > > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > > > > > > > With this and the other pstore patch, do the pstore self-tests still > > > > pass on an EFI system? > > > > > > > > If so, please consider both: > > > > > > > > Acked-by: Kees Cook <keescook@chromium.org> > > > > > > > > > > Selftests? Excellent! Are they documented too? > > > > Not really, but they're pretty simple: > > > > cd tools/testing/selftests/pstore > > *double-check "config" against running kernel config* > > ./pstore_tests > > ./pstore_crash_test > > *wait for system to reboot* > > cd tools/testing/selftests/pstore > > ./pstore_post_reboot_tests > > > > (though please test before/after, just to make sure other deltas haven't > > broken things before your series -- I don't test EFI pstore with high > > frequency) > > > > I have done the 'before' test on three different EFI Linux systems > (x86, arm64 and ARM), and they all give me something like the below > > === Pstore unit tests (pstore_tests) === > UUID=109d02e6-9395-4274-9554-2c078e87a662 > Checking pstore backend is registered ... ok > backend=efi > cmdline=BOOT_IMAGE=/vmlinuz-5.3.0-59-generic > root=/dev/mapper/crypt-root ro quiet splash vt.handoff=1 > Checking pstore console is registered ... FAIL > Checking /dev/pmsg0 exists ... FAIL > Writing unique string to /dev/pmsg0 ... FAIL > > So I'm not sure if there is any point to doing the 'after' test if > this is the baseline. In any case, I confirmed that the new efi-pstore module - exposes existing pstore dmesg entries correctly - captures oops and panic messages as dmesg-efi-xxxx entries as before
diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig index 3939699e62fe..dd8d10817bdf 100644 --- a/drivers/firmware/efi/Kconfig +++ b/drivers/firmware/efi/Kconfig @@ -26,7 +26,7 @@ config EFI_ESRT config EFI_VARS_PSTORE tristate "Register efivars backend for pstore" - depends on EFI_VARS && PSTORE + depends on PSTORE default y help Say Y here to enable use efivars as a backend to pstore. This diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c index feb7fe6f2da7..785f5e6b3a41 100644 --- a/drivers/firmware/efi/efi-pstore.c +++ b/drivers/firmware/efi/efi-pstore.c @@ -8,6 +8,8 @@ #define DUMP_NAME_LEN 66 +#define EFIVARS_DATA_SIZE_MAX 1024 + static bool efivars_pstore_disable = IS_ENABLED(CONFIG_EFI_VARS_PSTORE_DEFAULT_DISABLE); @@ -18,6 +20,8 @@ module_param_named(pstore_disable, efivars_pstore_disable, bool, 0644); EFI_VARIABLE_BOOTSERVICE_ACCESS | \ EFI_VARIABLE_RUNTIME_ACCESS) +static LIST_HEAD(efi_pstore_list); + static int efi_pstore_open(struct pstore_info *psi) { psi->data = NULL; @@ -126,7 +130,7 @@ static inline int __efi_pstore_scan_sysfs_exit(struct efivar_entry *entry, if (entry->deleting) { list_del(&entry->list); efivar_entry_iter_end(); - efivar_unregister(entry); + kfree(entry); if (efivar_entry_iter_begin()) return -EINTR; } else if (turn_off_scanning) @@ -169,7 +173,7 @@ static int efi_pstore_sysfs_entry_iter(struct pstore_record *record) { struct efivar_entry **pos = (struct efivar_entry **)&record->psi->data; struct efivar_entry *entry, *n; - struct list_head *head = &efivar_sysfs_list; + struct list_head *head = &efi_pstore_list; int size = 0; int ret; @@ -314,12 +318,12 @@ static int efi_pstore_erase_name(const char *name) if (efivar_entry_iter_begin()) return -EINTR; - found = __efivar_entry_iter(efi_pstore_erase_func, &efivar_sysfs_list, + found = __efivar_entry_iter(efi_pstore_erase_func, &efi_pstore_list, efi_name, &entry); efivar_entry_iter_end(); if (found && !entry->scanning) - efivar_unregister(entry); + kfree(entry); return found ? 0 : -ENOENT; } @@ -354,14 +358,76 @@ static struct pstore_info efi_pstore_info = { .erase = efi_pstore_erase, }; +static int efi_pstore_callback(efi_char16_t *name, efi_guid_t vendor, + unsigned long name_size, void *data) +{ + struct efivar_entry *entry; + int ret; + + entry = kzalloc(sizeof(*entry), GFP_KERNEL); + if (!entry) + return -ENOMEM; + + memcpy(entry->var.VariableName, name, name_size); + entry->var.VendorGuid = vendor; + + ret = efivar_entry_add(entry, &efi_pstore_list); + if (ret) + kfree(entry); + + return ret; +} + +static int efi_pstore_update_entry(efi_char16_t *name, efi_guid_t vendor, + unsigned long name_size, void *data) +{ + struct efivar_entry *entry = data; + + if (efivar_entry_find(name, vendor, &efi_pstore_list, false)) + return 0; + + memcpy(entry->var.VariableName, name, name_size); + memcpy(&(entry->var.VendorGuid), &vendor, sizeof(efi_guid_t)); + + return 1; +} + +static void efi_pstore_update_entries(struct work_struct *work) +{ + struct efivar_entry *entry; + int err; + + /* Add new sysfs entries */ + while (1) { + entry = kzalloc(sizeof(*entry), GFP_KERNEL); + if (!entry) + return; + + err = efivar_init(efi_pstore_update_entry, entry, + false, &efi_pstore_list); + if (!err) + break; + + efivar_entry_add(entry, &efi_pstore_list); + } + + kfree(entry); +} + static __init int efivars_pstore_init(void) { + int ret; + if (!efivars_kobject() || !efivar_supports_writes()) return 0; if (efivars_pstore_disable) return 0; + ret = efivar_init(efi_pstore_callback, NULL, true, &efi_pstore_list); + if (ret) + return ret; + efi_pstore_info.buf = kmalloc(4096, GFP_KERNEL); if (!efi_pstore_info.buf) return -ENOMEM; @@ -374,6 +440,8 @@ static __init int efivars_pstore_init(void) efi_pstore_info.bufsize = 0; } + INIT_WORK(&efivar_work, efi_pstore_update_entries); + return 0; } diff --git a/drivers/firmware/efi/efivars.c b/drivers/firmware/efi/efivars.c index dcea137142b3..f39321dbc29f 100644 --- a/drivers/firmware/efi/efivars.c +++ b/drivers/firmware/efi/efivars.c @@ -24,8 +24,7 @@ MODULE_LICENSE("GPL"); MODULE_VERSION(EFIVARS_VERSION); MODULE_ALIAS("platform:efivars"); -LIST_HEAD(efivar_sysfs_list); -EXPORT_SYMBOL_GPL(efivar_sysfs_list); +static LIST_HEAD(efivar_sysfs_list); static struct kset *efivars_kset; @@ -591,42 +590,6 @@ create_efivars_bin_attributes(void) return error; } -static int efivar_update_sysfs_entry(efi_char16_t *name, efi_guid_t vendor, - unsigned long name_size, void *data) -{ - struct efivar_entry *entry = data; - - if (efivar_entry_find(name, vendor, &efivar_sysfs_list, false)) - return 0; - - memcpy(entry->var.VariableName, name, name_size); - memcpy(&(entry->var.VendorGuid), &vendor, sizeof(efi_guid_t)); - - return 1; -} - -static void efivar_update_sysfs_entries(struct work_struct *work) -{ - struct efivar_entry *entry; - int err; - - /* Add new sysfs entries */ - while (1) { - entry = kzalloc(sizeof(*entry), GFP_KERNEL); - if (!entry) - return; - - err = efivar_init(efivar_update_sysfs_entry, entry, - false, &efivar_sysfs_list); - if (!err) - break; - - efivar_create_sysfs_entry(entry); - } - - kfree(entry); -} - static int efivars_sysfs_callback(efi_char16_t *name, efi_guid_t vendor, unsigned long name_size, void *data) { @@ -701,8 +664,6 @@ int efivars_sysfs_init(void) return error; } - INIT_WORK(&efivar_work, efivar_update_sysfs_entries); - return 0; } EXPORT_SYMBOL_GPL(efivars_sysfs_init); diff --git a/include/linux/efi.h b/include/linux/efi.h index 73db1ae04cef..a470256f5ee3 100644 --- a/include/linux/efi.h +++ b/include/linux/efi.h @@ -984,8 +984,6 @@ struct efivar_entry { bool deleting; }; -extern struct list_head efivar_sysfs_list; - static inline void efivar_unregister(struct efivar_entry *var) { @@ -1043,8 +1041,6 @@ void efivar_run_worker(void); #if defined(CONFIG_EFI_VARS) || defined(CONFIG_EFI_VARS_MODULE) int efivars_sysfs_init(void); -#define EFIVARS_DATA_SIZE_MAX 1024 - #endif /* CONFIG_EFI_VARS */ extern bool efi_capsule_pending(int *reset_type);
The EFI pstore implementation relies on the 'efivars' abstraction, which encapsulates the EFI variable store in a way that can be overridden by other backing stores, like the Google SMI one. On top of that, the EFI pstore implementation also relies on the efivars.ko module, which is a separate layer built on top of the 'efivars' abstraction that exposes the [deprecated] sysfs entries for each variable that exists in the backing store. Since the efivars.ko module is deprecated, and all users appear to have moved to the efivarfs file system instead, let's prepare for its removal, by removing EFI pstore's dependency on it. Signed-off-by: Ard Biesheuvel <ardb@kernel.org> --- drivers/firmware/efi/Kconfig | 2 +- drivers/firmware/efi/efi-pstore.c | 76 ++++++++++++++++++-- drivers/firmware/efi/efivars.c | 41 +---------- include/linux/efi.h | 4 -- 4 files changed, 74 insertions(+), 49 deletions(-)