| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAGTjWtBXCWi+gHJHO+_CO7mbTaruS0E2NfDCYPEY4gW2hBK9aA@mail.gmail.com>
Date: Mon, 20 Aug 2012 13:09:51 -0700
From: Mike Waychison <mikew@...gle.com>
To: Seiji Aguchi <seiji.aguchi@....com>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"Luck, Tony (tony.luck@...el.com)" <tony.luck@...el.com>,
"Matthew Garrett (mjg@...hat.com)" <mjg@...hat.com>,
"dzickus@...hat.com" <dzickus@...hat.com>,
"dle-develop@...ts.sourceforge.net"
<dle-develop@...ts.sourceforge.net>,
Satoru Moriya <satoru.moriya@....com>
Subject: Re: [RFC][PATCH v2 2/3] efi_pstore: Introducing workqueue updating
sysfs entries
On Mon, Aug 20, 2012 at 12:15 PM, Seiji Aguchi <seiji.aguchi@....com> wrote:
> [Problem]
> efi_pstore creates sysfs entries ,which enable users to access to NVRAM,
> in a write callback. If a kernel panic happens in interrupt contexts, pstore may
> fail because it could sleep due to dynamic memory allocations during creating sysfs entries.
>
> [Patch Description]
> This patch removes sysfs operations from write callback by introducing a workqueue
> updating sysfs entries which is scheduled after a write callback of efi_pstore is called.
> efi_pstore will be robust against a kernel panic in an interrupt context with it.
>
> Signed-off-by: Seiji Aguchi <seiji.aguchi@....com>
> ---
> drivers/firmware/efivars.c | 111 +++++++++++++++++++++++++++++++++++++++----
> include/linux/efi.h | 3 +-
> 2 files changed, 102 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
> index 25d6d35..8717ea9 100644
> --- a/drivers/firmware/efivars.c
> +++ b/drivers/firmware/efivars.c
> @@ -146,6 +146,13 @@ efivar_create_sysfs_entry(struct efivars *efivars,
> efi_char16_t *variable_name,
> efi_guid_t *vendor_guid);
>
> +/*
> + * Prototype for workqueue functions updating sysfs entry
> + */
> +
> +static void efivar_update_sysfs_entry(struct work_struct *);
> +static DECLARE_WORK(efivar_work, efivar_update_sysfs_entry);
> +
> /* Return the number of unicode characters in data */
> static unsigned long
> utf16_strnlen(efi_char16_t *s, size_t maxlength)
> @@ -732,9 +739,6 @@ static int efi_pstore_write(enum pstore_type_id type,
> 0, NULL);
> }
>
> - if (found)
> - list_del(&found->list);
> -
> for (i = 0; i < DUMP_NAME_LEN; i++)
> efi_name[i] = name[i];
>
> @@ -743,14 +747,7 @@ static int efi_pstore_write(enum pstore_type_id type,
>
> spin_unlock_irqrestore(&efivars->lock, flags);
>
> - if (found)
> - efivar_unregister(found);
> -
> - if (size)
> - ret = efivar_create_sysfs_entry(efivars,
> - utf16_strsize(efi_name,
> - DUMP_NAME_LEN * 2),
> - efi_name, &vendor);
> + schedule_work(&efivar_work);
>
> *id = part;
> return ret;
> @@ -1204,6 +1201,97 @@ EXPORT_SYMBOL_GPL(register_efivars);
> static struct efivars __efivars;
> static struct efivar_operations ops;
>
> +static void delete_sysfs_entry(void)
> +{
> + struct efivars *efivars = &__efivars;
> + struct efivar_entry *entry, *n;
> + efi_status_t status;
> + unsigned long flags;
> +
> + list_for_each_entry_safe(entry, n, &efivars->list, list) {
This ->lock here is protecting this list, so it isn't correct to grab
the lock within iteration. The lock should instead be grabbed before
iterating through the list, dropped if we need to unregister and
iteration should restart completely if the lock was dropped.
> + spin_lock_irqsave(&efivars->lock, flags);
> + status = get_var_data_locked(efivars, &entry->var);
> + if (status != EFI_SUCCESS) {
> + list_del(&entry->list);
> + spin_unlock_irqrestore(&efivars->lock, flags);
> + efivar_unregister(entry);
> + } else
> + spin_unlock_irqrestore(&efivars->lock, flags);
> + }
> +}
> +
> +static bool variable_is_present(efi_char16_t *variable_name, efi_guid_t *vendor)
> +{
> + struct efivar_entry *entry, *n;
> + struct efivars *efivars = &__efivars;
> + unsigned long strsize1, strsize2;
> + bool found = false;
> +
> + strsize1 = utf16_strsize(variable_name, 1024);
> + list_for_each_entry_safe(entry, n, &efivars->list, list) {
> + strsize2 = utf16_strsize(entry->var.VariableName, 1024);
> + if (strsize1 == strsize2 &&
> + !memcmp(variable_name, &(entry->var.VariableName),
> + strsize2) &&
> + !efi_guidcmp(entry->var.VendorGuid,
> + *vendor)) {
> + found = true;
> + break;
> + }
> + }
> + return found;
> +}
> +
> +static void efivar_update_sysfs_entry(struct work_struct *work)
> +{
> + struct efivars *efivars = &__efivars;
> + efi_guid_t vendor;
> + efi_char16_t *variable_name;
> + unsigned long variable_name_size = 1024, flags;
> + efi_status_t status = EFI_NOT_FOUND;
> + bool found;
> +
> + /* Delete unavailable sysfs entries */
> + delete_sysfs_entry();
This method needs a better name that reflects what it is doing.
delete_all_stale_sysfs_entries ?
> +
> + /* Add new sysfs entries */
> + while (1) {
> + variable_name = kzalloc(variable_name_size, GFP_KERNEL);
> + if (!variable_name) {
> + pr_err("efivars: Memory allocation failed.\n");
> + return;
> + }
> +
> + spin_lock_irqsave(&efivars->lock, flags);
> + found = false;
> + while (1) {
> + variable_name_size = 1024;
> + status = efivars->ops->get_next_variable(
> + &variable_name_size,
> + variable_name,
> + &vendor);
> + if (status != EFI_SUCCESS) {
> + break;
> + } else {
> + if (!variable_is_present(variable_name,
> + &vendor)) {
> + found = true;
> + break;
> + }
> + }
> + }
> + spin_unlock_irqrestore(&efivars->lock, flags);
> +
> + if (!found) {
> + kfree(variable_name);
> + break;
> + } else
> + efivar_create_sysfs_entry(efivars,
> + variable_name_size,
> + variable_name, &vendor);
> + }
> +}
> +
> /*
> * For now we register the efi subsystem with the firmware subsystem
> * and the vars subsystem with the efi subsystem. In the future, it
> @@ -1259,6 +1347,7 @@ static void __exit
> efivars_exit(void)
> {
> if (efi_enabled) {
> + cancel_work_sync(&efivar_work);
> unregister_efivars(&__efivars);
> kobject_put(efi_kobj);
> }
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 103adc6..cecdf58 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -641,7 +641,8 @@ struct efivars {
> * 1) ->list - adds, removals, reads, writes
> * 2) ops.[gs]et_variable() calls.
> * It must not be held when creating sysfs entries or calling kmalloc.
> - * ops.get_next_variable() is only called from register_efivars(),
> + * ops.get_next_variable() is only called from register_efivars()
> + * or efivar_update_sysfs_entry(),
> * which is protected by the BKL, so that path is safe.
> */
> spinlock_t lock;
> -- 1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists