Menu
▾
▴
dle-develop — Dependable Linux Effort mailing list for developers
You can subscribe to this list here.
| 2009 |
Jan
|
Feb
|
Mar
|
Apr
|
May
(32) |
Jun
(66) |
Jul
(102) |
Aug
(78) |
Sep
(106) |
Oct
(137) |
Nov
(147) |
Dec
(147) |
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 2010 |
Jan
(71) |
Feb
(139) |
Mar
(86) |
Apr
(76) |
May
(57) |
Jun
(10) |
Jul
(12) |
Aug
(6) |
Sep
(8) |
Oct
(12) |
Nov
(12) |
Dec
(18) |
| 2011 |
Jan
(16) |
Feb
(19) |
Mar
(3) |
Apr
(1) |
May
(16) |
Jun
(17) |
Jul
(74) |
Aug
(22) |
Sep
(18) |
Oct
(24) |
Nov
(21) |
Dec
(30) |
| 2012 |
Jan
(31) |
Feb
(16) |
Mar
(22) |
Apr
(25) |
May
(18) |
Jun
(13) |
Jul
(83) |
Aug
(49) |
Sep
(20) |
Oct
(60) |
Nov
(35) |
Dec
(28) |
| 2013 |
Jan
(39) |
Feb
(61) |
Mar
(35) |
Apr
(21) |
May
(45) |
Jun
(56) |
Jul
(20) |
Aug
(9) |
Sep
(10) |
Oct
(31) |
Nov
(8) |
Dec
(4) |
| 2014 |
Jan
(6) |
Feb
(7) |
Mar
(7) |
Apr
(6) |
May
(4) |
Jun
(8) |
Jul
(5) |
Aug
(2) |
Sep
(4) |
Oct
(4) |
Nov
(11) |
Dec
(5) |
| 2015 |
Jan
(4) |
Feb
(4) |
Mar
(3) |
Apr
(4) |
May
(9) |
Jun
(4) |
Jul
(15) |
Aug
(8) |
Sep
(16) |
Oct
(18) |
Nov
(15) |
Dec
(7) |
| 2016 |
Jan
(20) |
Feb
(9) |
Mar
(15) |
Apr
(24) |
May
(16) |
Jun
(28) |
Jul
(22) |
Aug
(23) |
Sep
(18) |
Oct
(30) |
Nov
(40) |
Dec
(9) |
| 2017 |
Jan
(1) |
Feb
(8) |
Mar
(37) |
Apr
(26) |
May
(25) |
Jun
(46) |
Jul
(24) |
Aug
(9) |
Sep
|
Oct
|
Nov
|
Dec
|
|
From: Seiji A. <sei...@hd...> - 2012-08-21 16:53:03
|
Changelog
v2 -> v3
- Patch 1/3
Replace spin_lock_irqsave/spin_unlock_irqrestore with spin_lock_irq/spin_unlock_irq in efivars_unregister(),
efivar_create(), efivar_store_raw() and efivar_delete() which are called in a process context.
- Patch 2/3
Change a name of delete_sysfs_entry() to delete_all_stale_sysfs_entries().
Also, don't release an efivar->lock while searching efivar->list in delete_all_stale_sysfs_entries().
- Patch 3/3
Remove a logic in efi_pstore_erase() which freshly created in patch v2.
v1 -> v2
- Patch 1/3
Add spin_lock_irq/spin_unlock_irq to open/close callbacks of efi_pstore
instead of moving spin_locks to a read callback.
- Patch 2/3
Replace a periodical timer with schedule_work().
- Patch 3/3
freshly create to kick a workqueue in oops case only.
[Problem]
There are following problems related to an interrupt context in efivar/efi_pstore.
Currently, efivars enables interrupt while taking efivars->lock.
So, there is a risk to be deadlocking in a write callback of efi_pstore if kernel panics
in interrupt context while taking efi_lock.
Also, 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.
To resolve the problems above, a goal of this patchset is making efivars/efi_pstore interrupt-safe.
[Patch Description]
Patch 1/3 efivars: Disable external interrupt while holding efivars->lock
This patch replaces spin_lock/spin_unlock with spin_lock_irqsave/spin_lock_irqrestore to make efivars interrupt safe
Patch 2/3 efi_pstore: Introducing workqueue updating sysfs entries
This patch removes sysfs operations from write callback by introducing a workqueue updating sysfs entries
Patch 3/3 efi_pstore: Skiping scheduling a workqueue in cases other than oops
This patch restricts a schedule of a workqueue in case where users erase entries or oops happen which is truly needed for users.
drivers/firmware/efivars.c | 166 +++++++++++++++++++++++++++++++++++--------
include/linux/efi.h | 3 +-
2 files changed, 137 insertions(+), 32 deletions(-)
|
|
From: Seiji A. <sei...@hd...> - 2012-08-20 22:08:48
|
> -----Original Message-----
> From: Mike Waychison [mailto:mi...@go...]
> Sent: Monday, August 20, 2012 3:17 PM
> To: Seiji Aguchi
> Cc: lin...@vg...; Luck, Tony (ton...@in...); Matthew Garrett (mj...@re...); dz...@re...; dle-
> de...@li...; Satoru Moriya
> Subject: Re: [RFC][PATCH v2 1/3] efivars: Disable external interrupt while holding efivars->lock
>
> Acked-by: Mike Waychison <mi...@go...>
>
> > @@ -1101,11 +1107,12 @@ out_free:
> > void unregister_efivars(struct efivars *efivars) {
> > struct efivar_entry *entry, *n;
> > + unsigned long flags;
> >
> > list_for_each_entry_safe(entry, n, &efivars->list, list) {
> > - spin_lock(&efivars->lock);
> > + spin_lock_irqsave(&efivars->lock, flags);
> > list_del(&entry->list);
> > - spin_unlock(&efivars->lock);
> > + spin_unlock_irqrestore(&efivars->lock, flags);
> > efivar_unregister(entry);
> > }
> > if (efivars->new_var)
>
> Feel free to remove any other uses of flags where you know that you
> are being called from process context.
OK. I will remove the flags from unregister_efivars(), efivar_store_raw(), efivar_create() and efivar_delete().
If I'm missing something, please let me know.
Seiji
|
|
From: Seiji A. <sei...@hd...> - 2012-08-20 21:28:19
|
> > Also, a logic of erase callback is freshly created instead of sharing
> > with a write callback because the sysfs operation is kicked in just oops case in a write callback and an erase callback doesn't work
> without any change.
> > By separating a logic to a write callback, an erase callback works correctly.
>
> I don't understand why this part of the patch is required.
If I have to keep sharing the code of an erase callback with a write callback,
I can change my patch as follows.
I just thought the code would be ieasy to read by separating them.
<snip>
efi_pstore_write()
{
+ /*
+ * The user may want to update when they erases an entry or see it for this write in sysfs in oops case.
+ */
+ if (!size || reason == KMSG_DUMP_OOPS)
+ schedule_work(&efivar_work);
}
<snip>
Seiji
|
|
From: Seiji A. <sei...@hd...> - 2012-08-20 20:56:34
|
> > +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.
>
Thank you for your comment.
If my understanding is correct, the code should be changed as follows.
+ while (1) {
+ found = NULL;
+ spin_lock_irqsave(&efivars->lock, flags);
+ list_for_each_entry(entry, &efivars->list, list) {
+ status = get_var_data_locked(efivars, &entry->var);
+ if (status != EFI_SUCCESS) {
+ found = entry;
+ list_del(&entry->list);
+ break;
+ }
+ }
+ spin_unlock_irqrestore(&efivars->lock, flags);
+ if (found)
+ efivar_unregister(entry);
+ else
+ break;
+ }
> > + 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);
> > + }
> > +}
> > + /* Delete unavailable sysfs entries */
> > + delete_sysfs_entry();
>
> This method needs a better name that reflects what it is doing.
> delete_all_stale_sysfs_entries ?
OK. I will change the name to it.
Seiji
|
|
From: Mike W. <mi...@go...> - 2012-08-20 20:17:26
|
On Mon, Aug 20, 2012 at 12:15 PM, Seiji Aguchi <sei...@hd...> 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 <sei...@hd...>
> ---
> 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
|
|
From: Mike W. <mi...@go...> - 2012-08-20 20:13:56
|
On Mon, Aug 20, 2012 at 12:15 PM, Seiji Aguchi <sei...@hd...> wrote:
> [Problem]
> efi_pstore creates sysfs files when logging kernel messages to NVRAM.
> Currently, the sysfs files are updated in a workqueue which is registered in a write callback.
>
> On the other hand, a situation which users need the sysfs files is just oops case because a system will
> be down and they can't access to sysfs files in other cases like panic, reboot or emergency_restart.
>
> Also, if kernel panics due to a bug of workqueue operations and a write callback of efi_pstore is called in
> panic case, efi_pstore may fail due to a failure of schedule_work().
> And panic_notifier_chain()/emergency_restart() is not kicked if efi_pstore fails.
> This may cause user's unwanted result.
>
> [Patch Description]
> This patch skips a registration of a workqueue in a write callback in cases other than oops.
>
> Also, a logic of erase callback is freshly created instead of sharing with a write callback because the sysfs
> operation is kicked in just oops case in a write callback and an erase callback doesn't work without any change.
> By separating a logic to a write callback, an erase callback works correctly.
I don't understand why this part of the patch is required.
>
> Signed-off-by: Seiji Aguchi <sei...@hd...>
> ---
> drivers/firmware/efivars.c | 53 ++++++++++++++++++++++++++++++++++++++++++-
> 1 files changed, 51 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
> index 8717ea9..4811abb 100644
> --- a/drivers/firmware/efivars.c
> +++ b/drivers/firmware/efivars.c
> @@ -747,7 +747,11 @@ static int efi_pstore_write(enum pstore_type_id type,
>
> spin_unlock_irqrestore(&efivars->lock, flags);
>
> - schedule_work(&efivar_work);
> + /*
> + * The user may want to see an entry for this write in sysfs.
> + */
> + if (reason == KMSG_DUMP_OOPS)
> + schedule_work(&efivar_work);
>
> *id = part;
> return ret;
> @@ -756,7 +760,52 @@ static int efi_pstore_write(enum pstore_type_id type,
> static int efi_pstore_erase(enum pstore_type_id type, u64 id,
> struct pstore_info *psi)
> {
> - efi_pstore_write(type, 0, &id, (unsigned int)id, 0, psi);
> + char stub_name[DUMP_NAME_LEN];
> + efi_char16_t efi_name[DUMP_NAME_LEN];
> + efi_guid_t vendor = LINUX_EFI_CRASH_GUID;
> + struct efivars *efivars = psi->data;
> + struct efivar_entry *entry, *found = NULL;
> + int i;
> + unsigned long flags;
> +
> + sprintf(stub_name, "dump-type%u-%llu-", type, id);
> +
> + spin_lock_irqsave(&efivars->lock, flags);
> +
> + for (i = 0; i < DUMP_NAME_LEN; i++)
> + efi_name[i] = stub_name[i];
> +
> + /*
> + * Clean up any entries with the same name
> + */
> +
> + list_for_each_entry(entry, &efivars->list, list) {
> + get_var_data_locked(efivars, &entry->var);
> +
> + if (efi_guidcmp(entry->var.VendorGuid, vendor))
> + continue;
> + if (utf16_strncmp(entry->var.VariableName, efi_name,
> + utf16_strlen(efi_name)))
> + continue;
> + /* Needs to be a prefix */
> + if (entry->var.VariableName[utf16_strlen(efi_name)] == 0)
> + continue;
> +
> + /* found */
> + found = entry;
> + efivars->ops->set_variable(entry->var.VariableName,
> + &entry->var.VendorGuid,
> + PSTORE_EFI_ATTRIBUTES,
> + 0, NULL);
> + }
> +
> + if (found)
> + list_del(&found->list);
> +
> + spin_unlock_irqrestore(&efivars->lock, flags);
> +
> + if (found)
> + efivar_unregister(found);
>
> return 0;
> }
> -- 1.7.1
|
|
From: Mike W. <mi...@go...> - 2012-08-20 19:17:34
|
Acked-by: Mike Waychison <mi...@go...>
> @@ -1101,11 +1107,12 @@ out_free:
> void unregister_efivars(struct efivars *efivars)
> {
> struct efivar_entry *entry, *n;
> + unsigned long flags;
>
> list_for_each_entry_safe(entry, n, &efivars->list, list) {
> - spin_lock(&efivars->lock);
> + spin_lock_irqsave(&efivars->lock, flags);
> list_del(&entry->list);
> - spin_unlock(&efivars->lock);
> + spin_unlock_irqrestore(&efivars->lock, flags);
> efivar_unregister(entry);
> }
> if (efivars->new_var)
Feel free to remove any other uses of flags where you know that you
are being called from process context.
|
|
From: Seiji A. <sei...@hd...> - 2012-08-20 19:16:04
|
[Problem]
efi_pstore creates sysfs files when logging kernel messages to NVRAM.
Currently, the sysfs files are updated in a workqueue which is registered in a write callback.
On the other hand, a situation which users need the sysfs files is just oops case because a system will
be down and they can't access to sysfs files in other cases like panic, reboot or emergency_restart.
Also, if kernel panics due to a bug of workqueue operations and a write callback of efi_pstore is called in
panic case, efi_pstore may fail due to a failure of schedule_work().
And panic_notifier_chain()/emergency_restart() is not kicked if efi_pstore fails.
This may cause user's unwanted result.
[Patch Description]
This patch skips a registration of a workqueue in a write callback in cases other than oops.
Also, a logic of erase callback is freshly created instead of sharing with a write callback because the sysfs
operation is kicked in just oops case in a write callback and an erase callback doesn't work without any change.
By separating a logic to a write callback, an erase callback works correctly.
Signed-off-by: Seiji Aguchi <sei...@hd...>
---
drivers/firmware/efivars.c | 53 ++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 51 insertions(+), 2 deletions(-)
diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 8717ea9..4811abb 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -747,7 +747,11 @@ static int efi_pstore_write(enum pstore_type_id type,
spin_unlock_irqrestore(&efivars->lock, flags);
- schedule_work(&efivar_work);
+ /*
+ * The user may want to see an entry for this write in sysfs.
+ */
+ if (reason == KMSG_DUMP_OOPS)
+ schedule_work(&efivar_work);
*id = part;
return ret;
@@ -756,7 +760,52 @@ static int efi_pstore_write(enum pstore_type_id type,
static int efi_pstore_erase(enum pstore_type_id type, u64 id,
struct pstore_info *psi)
{
- efi_pstore_write(type, 0, &id, (unsigned int)id, 0, psi);
+ char stub_name[DUMP_NAME_LEN];
+ efi_char16_t efi_name[DUMP_NAME_LEN];
+ efi_guid_t vendor = LINUX_EFI_CRASH_GUID;
+ struct efivars *efivars = psi->data;
+ struct efivar_entry *entry, *found = NULL;
+ int i;
+ unsigned long flags;
+
+ sprintf(stub_name, "dump-type%u-%llu-", type, id);
+
+ spin_lock_irqsave(&efivars->lock, flags);
+
+ for (i = 0; i < DUMP_NAME_LEN; i++)
+ efi_name[i] = stub_name[i];
+
+ /*
+ * Clean up any entries with the same name
+ */
+
+ list_for_each_entry(entry, &efivars->list, list) {
+ get_var_data_locked(efivars, &entry->var);
+
+ if (efi_guidcmp(entry->var.VendorGuid, vendor))
+ continue;
+ if (utf16_strncmp(entry->var.VariableName, efi_name,
+ utf16_strlen(efi_name)))
+ continue;
+ /* Needs to be a prefix */
+ if (entry->var.VariableName[utf16_strlen(efi_name)] == 0)
+ continue;
+
+ /* found */
+ found = entry;
+ efivars->ops->set_variable(entry->var.VariableName,
+ &entry->var.VendorGuid,
+ PSTORE_EFI_ATTRIBUTES,
+ 0, NULL);
+ }
+
+ if (found)
+ list_del(&found->list);
+
+ spin_unlock_irqrestore(&efivars->lock, flags);
+
+ if (found)
+ efivar_unregister(found);
return 0;
}
-- 1.7.1
|
|
From: Seiji A. <sei...@hd...> - 2012-08-20 19:15:33
|
[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 <sei...@hd...>
---
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) {
+ 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();
+
+ /* 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
|
|
From: Seiji A. <sei...@hd...> - 2012-08-20 19:14:43
|
[Problem]
Currently, efivars doesn't disable interrupt while taking efivars->lock.
So, there is a risk to be deadlocking in a write callback of efi_pstore
if kernel panics in interrupt context while taking efivars->lock.
[Patch Description]
This patch disables an external interruption while holding efivars->lock by
replacing spin_lock/spin_unlock with spin_lock_irqsave/spin_unlock_irqrestore.
As for spin_locks in open/close callbacks of efi_pstore which works in process
context, they are replaced with spin_lock_irq/spin_unlock_irq.
Signed-off-by: Seiji Aguchi <sei...@hd...>
---
drivers/firmware/efivars.c | 47 +++++++++++++++++++++++++------------------
1 files changed, 27 insertions(+), 20 deletions(-)
diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 47408e8..25d6d35 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -393,10 +393,11 @@ static efi_status_t
get_var_data(struct efivars *efivars, struct efi_variable *var)
{
efi_status_t status;
+ unsigned long flags;
- spin_lock(&efivars->lock);
+ spin_lock_irqsave(&efivars->lock, flags);
status = get_var_data_locked(efivars, var);
- spin_unlock(&efivars->lock);
+ spin_unlock_irqrestore(&efivars->lock, flags);
if (status != EFI_SUCCESS) {
printk(KERN_WARNING "efivars: get_variable() failed 0x%lx!\n",
@@ -488,6 +489,7 @@ efivar_store_raw(struct efivar_entry *entry, const char *buf, size_t count)
struct efi_variable *new_var, *var = &entry->var;
struct efivars *efivars = entry->efivars;
efi_status_t status = EFI_NOT_FOUND;
+ unsigned long flags;
if (count != sizeof(struct efi_variable))
return -EINVAL;
@@ -514,14 +516,14 @@ efivar_store_raw(struct efivar_entry *entry, const char *buf, size_t count)
return -EINVAL;
}
- spin_lock(&efivars->lock);
+ spin_lock_irqsave(&efivars->lock, flags);
status = efivars->ops->set_variable(new_var->VariableName,
&new_var->VendorGuid,
new_var->Attributes,
new_var->DataSize,
new_var->Data);
- spin_unlock(&efivars->lock);
+ spin_unlock_irqrestore(&efivars->lock, flags);
if (status != EFI_SUCCESS) {
printk(KERN_WARNING "efivars: set_variable() failed: status=%lx\n",
@@ -632,7 +634,7 @@ static int efi_pstore_open(struct pstore_info *psi)
{
struct efivars *efivars = psi->data;
- spin_lock(&efivars->lock);
+ spin_lock_irq(&efivars->lock);
efivars->walk_entry = list_first_entry(&efivars->list,
struct efivar_entry, list);
return 0;
@@ -642,7 +644,7 @@ static int efi_pstore_close(struct pstore_info *psi)
{
struct efivars *efivars = psi->data;
- spin_unlock(&efivars->lock);
+ spin_unlock_irq(&efivars->lock);
return 0;
}
@@ -696,11 +698,12 @@ static int efi_pstore_write(enum pstore_type_id type,
struct efivars *efivars = psi->data;
struct efivar_entry *entry, *found = NULL;
int i, ret = 0;
+ unsigned long flags;
sprintf(stub_name, "dump-type%u-%u-", type, part);
sprintf(name, "%s%lu", stub_name, get_seconds());
- spin_lock(&efivars->lock);
+ spin_lock_irqsave(&efivars->lock, flags);
for (i = 0; i < DUMP_NAME_LEN; i++)
efi_name[i] = stub_name[i];
@@ -738,7 +741,7 @@ static int efi_pstore_write(enum pstore_type_id type,
efivars->ops->set_variable(efi_name, &vendor, PSTORE_EFI_ATTRIBUTES,
size, psi->buf);
- spin_unlock(&efivars->lock);
+ spin_unlock_irqrestore(&efivars->lock, flags);
if (found)
efivar_unregister(found);
@@ -812,6 +815,7 @@ static ssize_t efivar_create(struct file *filp, struct kobject *kobj,
unsigned long strsize1, strsize2;
efi_status_t status = EFI_NOT_FOUND;
int found = 0;
+ unsigned long flags;
if (!capable(CAP_SYS_ADMIN))
return -EACCES;
@@ -822,7 +826,7 @@ static ssize_t efivar_create(struct file *filp, struct kobject *kobj,
return -EINVAL;
}
- spin_lock(&efivars->lock);
+ spin_lock_irqsave(&efivars->lock, flags);
/*
* Does this variable already exist?
@@ -840,7 +844,7 @@ static ssize_t efivar_create(struct file *filp, struct kobject *kobj,
}
}
if (found) {
- spin_unlock(&efivars->lock);
+ spin_unlock_irqrestore(&efivars->lock, flags);
return -EINVAL;
}
@@ -854,10 +858,10 @@ static ssize_t efivar_create(struct file *filp, struct kobject *kobj,
if (status != EFI_SUCCESS) {
printk(KERN_WARNING "efivars: set_variable() failed: status=%lx\n",
status);
- spin_unlock(&efivars->lock);
+ spin_unlock_irqrestore(&efivars->lock, flags);
return -EIO;
}
- spin_unlock(&efivars->lock);
+ spin_unlock_irqrestore(&efivars->lock, flags);
/* Create the entry in sysfs. Locking is not required here */
status = efivar_create_sysfs_entry(efivars,
@@ -881,11 +885,12 @@ static ssize_t efivar_delete(struct file *filp, struct kobject *kobj,
unsigned long strsize1, strsize2;
efi_status_t status = EFI_NOT_FOUND;
int found = 0;
+ unsigned long flags;
if (!capable(CAP_SYS_ADMIN))
return -EACCES;
- spin_lock(&efivars->lock);
+ spin_lock_irqsave(&efivars->lock, flags);
/*
* Does this variable already exist?
@@ -903,7 +908,7 @@ static ssize_t efivar_delete(struct file *filp, struct kobject *kobj,
}
}
if (!found) {
- spin_unlock(&efivars->lock);
+ spin_unlock_irqrestore(&efivars->lock, flags);
return -EINVAL;
}
/* force the Attributes/DataSize to 0 to ensure deletion */
@@ -919,12 +924,12 @@ static ssize_t efivar_delete(struct file *filp, struct kobject *kobj,
if (status != EFI_SUCCESS) {
printk(KERN_WARNING "efivars: set_variable() failed: status=%lx\n",
status);
- spin_unlock(&efivars->lock);
+ spin_unlock_irqrestore(&efivars->lock, flags);
return -EIO;
}
list_del(&search_efivar->list);
/* We need to release this lock before unregistering. */
- spin_unlock(&efivars->lock);
+ spin_unlock_irqrestore(&efivars->lock, flags);
efivar_unregister(search_efivar);
/* It's dead Jim.... */
@@ -993,6 +998,7 @@ efivar_create_sysfs_entry(struct efivars *efivars,
int i, short_name_size = variable_name_size / sizeof(efi_char16_t) + 38;
char *short_name;
struct efivar_entry *new_efivar;
+ unsigned long flags;
short_name = kzalloc(short_name_size + 1, GFP_KERNEL);
new_efivar = kzalloc(sizeof(struct efivar_entry), GFP_KERNEL);
@@ -1032,9 +1038,9 @@ efivar_create_sysfs_entry(struct efivars *efivars,
kfree(short_name);
short_name = NULL;
- spin_lock(&efivars->lock);
+ spin_lock_irqsave(&efivars->lock, flags);
list_add(&new_efivar->list, &efivars->list);
- spin_unlock(&efivars->lock);
+ spin_unlock_irqrestore(&efivars->lock, flags);
return 0;
}
@@ -1101,11 +1107,12 @@ out_free:
void unregister_efivars(struct efivars *efivars)
{
struct efivar_entry *entry, *n;
+ unsigned long flags;
list_for_each_entry_safe(entry, n, &efivars->list, list) {
- spin_lock(&efivars->lock);
+ spin_lock_irqsave(&efivars->lock, flags);
list_del(&entry->list);
- spin_unlock(&efivars->lock);
+ spin_unlock_irqrestore(&efivars->lock, flags);
efivar_unregister(entry);
}
if (efivars->new_var)
-- 1.7.1
|
|
From: Seiji A. <sei...@hd...> - 2012-08-20 19:13:49
|
Changelog
- Patch 1/3
Add spin_lock_irq/spin_unlock_irq to open/close callbacks of efi_pstore
instead of moving spin_locks to a read callback.
- Patch 2/3
Replace a periodical timer with schedule_work().
- Patch 3/3
freshly create to kick a workqueue in oops case only.
[Problem]
There are following problems related to an interrupt context in efivar/efi_pstore.
Currently, efivars enables interrupt while taking efivars->lock.
So, there is a risk to be deadlocking in a write callback of efi_pstore if kernel panics
in interrupt context while taking efi_lock.
Also, 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.
To resolve the problems above, a goal of this patchset is making efivars/efi_pstore interrupt-safe.
[Patch Description]
Patch 1/3 efivars: Disable external interrupt while holding efivars->lock
This patch replaces spin_lock/spin_unlock with spin_lock_irqsave/spin_lock_irqrestore to make efivars interrupt safe
Patch 2/3 efi_pstore: Introducing workqueue updating sysfs entries
This patch removes sysfs operations from write callback by introducing a workqueue updating sysfs entries
Patch 3/3 efi_pstore: Skip scheduling a workqueue in cases other than oops
This patch restricts a schedule of a workqueue in just oops case which is truly needed for users.
Also, a logic of erase callback is freshly created instead of sharing with a write callback so that
its sysfs update operation works correctly.
drivers/firmware/efivars.c | 209 +++++++++++++++++++++++++++++++++++++-------
include/linux/efi.h | 3 +-
2 files changed, 179 insertions(+), 33 deletions(-)
|
|
From: Seiji A. <sei...@hd...> - 2012-08-17 21:42:11
|
> You could, but why not always just schedule_work()? If we are hosed by broken workqueue/scheduler locking, the user isn't going to > see those files in sysfs either way :) I'm not concern about failure of sysfs operations. In panic function call, panic_notifier_chain is kicked. Also users may expect the system reboots in panic case by specifying /proc/sys/kernel/panic. I just would like to avoid failures of those operations without adding unnecessary calls in the write callback. > If you are going to insist that we shouldn't schedule_work() in the other cases, I'd prefer: > > /* The user may want to see an entry for this write in sysfs. */ if (reason == KMSG_DUMP_OOPS) > schedule_work(...); > Thank you for your suggestion. I will update my patch by adding this logic above. |
|
From: Mike W. <mi...@go...> - 2012-08-17 21:28:07
|
On Fri, Aug 17, 2012 at 2:15 PM, Seiji Aguchi <sei...@hd...> wrote:
>
>> I'm not a fan of creating a periodic timer that wakes up here to check for an event that should be considered very rare.
>>
>> Can this just become scheduled work? Scheduling work itself is a very lightweight process and should be relatively safe to do from a
>> pstore write.
>
> I agree that the periodic timer is heavy a bit.
> But I would like to keep a write callback simple as much as possible in panic situation.
> For example, I'm concerned that efi_pstore hangs up due to some spin_locks related workqueue like gcwq->lock.
>
> Also, a situation which this workqueue is needed is just oops case because, system will be down and users can't access to sysfs files
> in other cases, panic, reboot and emergency_restart.
>
> So, Can I call schedule_work in oops case only as follows?
>
> efi_pstore_write()
> {
> <write log to NVRAM>
>
> if (reason != KMSG_DUMP_OOPS)
> return;
>
> schedule_work()
You could, but why not always just schedule_work()? If we are hosed
by broken workqueue/scheduler locking, the user isn't going to see
those files in sysfs either way :) If you are going to insist that we
shouldn't schedule_work() in the other cases, I'd prefer:
/* The user may want to see an entry for this write in sysfs. */
if (reason == KMSG_DUMP_OOPS)
schedule_work(...);
>
> }
|
|
From: Seiji A. <sei...@hd...> - 2012-08-17 21:16:09
|
> I'm not a fan of creating a periodic timer that wakes up here to check for an event that should be considered very rare.
>
> Can this just become scheduled work? Scheduling work itself is a very lightweight process and should be relatively safe to do from a
> pstore write.
I agree that the periodic timer is heavy a bit.
But I would like to keep a write callback simple as much as possible in panic situation.
For example, I'm concerned that efi_pstore hangs up due to some spin_locks related workqueue like gcwq->lock.
Also, a situation which this workqueue is needed is just oops case because, system will be down and users can't access to sysfs files
in other cases, panic, reboot and emergency_restart.
So, Can I call schedule_work in oops case only as follows?
efi_pstore_write()
{
<write log to NVRAM>
if (reason != KMSG_DUMP_OOPS)
return;
schedule_work()
}
|
|
From: Mike W. <mi...@go...> - 2012-08-17 20:56:30
|
On Fri, Aug 17, 2012 at 12:43 PM, Seiji Aguchi <sei...@hd...> 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 kicked after a write callback of efi_pstore is called.
> efi_pstore will be robust against a kernel panic in an interrupt context with it.
> The workqueue works as follows.
>
> - A timer is registered during an initialization of efivars module.
> - A flag, update_sysfs_entries, is checked periodically with the timer.
> - The flag is set in a write callback, efi_pstore_write().
> - Operation updating sysfs entries is kicked if the flag is set.
>
> Any operations ,like registering a timer, should not be added in a write callback because it may run
> in panic case and fail due to operations related to workqueue.
I'm not a fan of creating a periodic timer that wakes up here to check
for an event that should be considered very rare.
Can this just become scheduled work? Scheduling work itself is a very
lightweight process and should be relatively safe to do from a pstore
write.
>
> To make efi_pstore work in panic case, a write callback should just log to NVRAM.
>
> Signed-off-by: Seiji Aguchi <sei...@hd...>
> ---
> drivers/firmware/efivars.c | 127 ++++++++++++++++++++++++++++++++++++++++----
> include/linux/efi.h | 3 +-
> 2 files changed, 118 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
> index b6a813e..9f27ef6 100644
> --- a/drivers/firmware/efivars.c
> +++ b/drivers/firmware/efivars.c
> @@ -146,6 +146,17 @@ 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 *);
> +#define EFIVAR_INTERVAL (60 * HZ)
> +static void efivar_timefunc(unsigned long);
> +static DEFINE_TIMER(efivar_timer, efivar_timefunc, 0, 0);
> +static DECLARE_WORK(efivar_work, efivar_update_sysfs_entry);
> +static int update_sysfs_entries;
> +
> /* Return the number of unicode characters in data */
> static unsigned long
> utf16_strnlen(efi_char16_t *s, size_t maxlength)
> @@ -739,9 +750,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];
>
> @@ -750,14 +758,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);
> + update_sysfs_entries = 1;
>
> *id = part;
> return ret;
> @@ -1201,6 +1202,8 @@ int register_efivars(struct efivars *efivars,
> pstore_register(&efivars->efi_pstore_info);
> }
>
> + add_timer(&efivar_timer);
> +
> out:
> kfree(variable_name);
>
> @@ -1211,6 +1214,107 @@ 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) {
> + 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();
> +
> + /* 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);
> + }
> +}
> +
> +static void efivar_timefunc(unsigned long dummy)
> +{
> + if (update_sysfs_entries) {
> + update_sysfs_entries = 0;
> + schedule_work(&efivar_work);
> + }
> +
> + mod_timer(&efivar_timer, jiffies + EFIVAR_INTERVAL);
> +}
> +
> /*
> * For now we register the efi subsystem with the firmware subsystem
> * and the vars subsystem with the efi subsystem. In the future, it
> @@ -1266,6 +1370,7 @@ static void __exit
> efivars_exit(void)
> {
> if (efi_enabled) {
> + del_timer_sync(&efivar_timer);
> unregister_efivars(&__efivars);
> kobject_put(efi_kobj);
> }
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index f1f5703..3e0c37a 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
|
|
From: Seiji A. <sei...@hd...> - 2012-08-17 20:43:26
|
> This isn't necessary (nor desirable). If the API here is that we get called from process context for ->open and ->close(), just use the > spin_lock_irq and spin_unlock_irq variants for these paths (avoiding the need to remove the lock acquire in ->open and ->close. OK. I will update my patch based on your comment above. Thank you for your quick response. Seiji |
|
From: Mike W. <mi...@go...> - 2012-08-17 20:18:31
|
On Fri, Aug 17, 2012 at 12:42 PM, Seiji Aguchi <sei...@hd...> wrote:
> [Problem]
> Currently, efivars doesn't disable interrupt while taking efivars->lock.
> So, there is a risk to be deadlocking in a write callback of efi_pstore
> if kernel panics in interrupt context while taking efivars->lock.
>
> [Patch Description]
> This patch disables an external interruption while holding efivars->lock by
> replacing spin_lock/spin_unlock with spin_lock_irqsave/spin_unlock_irqrestore.
>
> As for spin_locks in open/close callbacks of efi_pstore, they moved to a read callback
> to pass a flags argument to spin_lock_irqsave/spin_unlock_irqrestore.
This isn't necessary (nor desirable). If the API here is that we get
called from process context for ->open and ->close(), just use the
spin_lock_irq and spin_unlock_irq variants for these paths (avoiding
the need to remove the lock acquire in ->open and ->close.
>
> Even if spin_locks in open/close callbacks are erased, a whole process of open/read/close
> is still serialized by psinfo->read_mutex which is taken in a pstore layer.
>
> Also, initialization of efivars->walk_entry moved to a read callback to protect list_first_entry()
> with efivars->lock. And some comments about efivars->walk_entry are added to avoid using it
> without taking psinfo->read_mutex.
>
> Signed-off-by: Seiji Aguchi <sei...@hd...>
> ---
> drivers/firmware/efivars.c | 66 ++++++++++++++++++++++++++-----------------
> include/linux/efi.h | 3 ++
> 2 files changed, 43 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
> index 47408e8..b6a813e 100644
> --- a/drivers/firmware/efivars.c
> +++ b/drivers/firmware/efivars.c
> @@ -393,10 +393,11 @@ static efi_status_t
> get_var_data(struct efivars *efivars, struct efi_variable *var)
> {
> efi_status_t status;
> + unsigned long flags;
>
> - spin_lock(&efivars->lock);
> + spin_lock_irqsave(&efivars->lock, flags);
> status = get_var_data_locked(efivars, var);
> - spin_unlock(&efivars->lock);
> + spin_unlock_irqrestore(&efivars->lock, flags);
>
> if (status != EFI_SUCCESS) {
> printk(KERN_WARNING "efivars: get_variable() failed 0x%lx!\n",
> @@ -488,6 +489,7 @@ efivar_store_raw(struct efivar_entry *entry, const char *buf, size_t count)
> struct efi_variable *new_var, *var = &entry->var;
> struct efivars *efivars = entry->efivars;
> efi_status_t status = EFI_NOT_FOUND;
> + unsigned long flags;
>
> if (count != sizeof(struct efi_variable))
> return -EINVAL;
> @@ -514,14 +516,14 @@ efivar_store_raw(struct efivar_entry *entry, const char *buf, size_t count)
> return -EINVAL;
> }
>
> - spin_lock(&efivars->lock);
> + spin_lock_irqsave(&efivars->lock, flags);
> status = efivars->ops->set_variable(new_var->VariableName,
> &new_var->VendorGuid,
> new_var->Attributes,
> new_var->DataSize,
> new_var->Data);
>
> - spin_unlock(&efivars->lock);
> + spin_unlock_irqrestore(&efivars->lock, flags);
>
> if (status != EFI_SUCCESS) {
> printk(KERN_WARNING "efivars: set_variable() failed: status=%lx\n",
> @@ -630,19 +632,11 @@ efivar_unregister(struct efivar_entry *var)
>
> static int efi_pstore_open(struct pstore_info *psi)
> {
> - struct efivars *efivars = psi->data;
> -
> - spin_lock(&efivars->lock);
> - efivars->walk_entry = list_first_entry(&efivars->list,
> - struct efivar_entry, list);
> return 0;
> }
>
> static int efi_pstore_close(struct pstore_info *psi)
> {
> - struct efivars *efivars = psi->data;
> -
> - spin_unlock(&efivars->lock);
> return 0;
> }
>
> @@ -656,6 +650,17 @@ static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type,
> int i;
> unsigned int part, size;
> unsigned long time;
> + static unsigned long flags;
> +
> + /*
> + * efivars->walk_entry is protected by pstore_info.read_mutex
> + */
> + if (!efivars->walk_entry) {
> + spin_lock_irqsave(&efivars->lock, flags);
> + efivars->walk_entry = list_first_entry(&efivars->list,
> + struct efivar_entry,
> + list);
> + }
>
> while (&efivars->walk_entry->list != &efivars->list) {
> if (!efi_guidcmp(efivars->walk_entry->var.VendorGuid,
> @@ -682,6 +687,10 @@ static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type,
> efivars->walk_entry = list_entry(efivars->walk_entry->list.next,
> struct efivar_entry, list);
> }
> +
> + efivars->walk_entry = NULL;
> + spin_unlock_irqrestore(&efivars->lock, flags);
This doesn't look right. Why are we only conditionally locking above?
Having the iterator state live in efivars->walk_entry seems wrong.
Any reason we shouldn't move this into pstore_info (a void *private
would do).
> +
> return 0;
> }
>
> @@ -696,11 +705,12 @@ static int efi_pstore_write(enum pstore_type_id type,
> struct efivars *efivars = psi->data;
> struct efivar_entry *entry, *found = NULL;
> int i, ret = 0;
> + unsigned long flags;
>
> sprintf(stub_name, "dump-type%u-%u-", type, part);
> sprintf(name, "%s%lu", stub_name, get_seconds());
>
> - spin_lock(&efivars->lock);
> + spin_lock_irqsave(&efivars->lock, flags);
>
> for (i = 0; i < DUMP_NAME_LEN; i++)
> efi_name[i] = stub_name[i];
> @@ -738,7 +748,7 @@ static int efi_pstore_write(enum pstore_type_id type,
> efivars->ops->set_variable(efi_name, &vendor, PSTORE_EFI_ATTRIBUTES,
> size, psi->buf);
>
> - spin_unlock(&efivars->lock);
> + spin_unlock_irqrestore(&efivars->lock, flags);
>
> if (found)
> efivar_unregister(found);
> @@ -812,6 +822,7 @@ static ssize_t efivar_create(struct file *filp, struct kobject *kobj,
> unsigned long strsize1, strsize2;
> efi_status_t status = EFI_NOT_FOUND;
> int found = 0;
> + unsigned long flags;
>
> if (!capable(CAP_SYS_ADMIN))
> return -EACCES;
> @@ -822,7 +833,7 @@ static ssize_t efivar_create(struct file *filp, struct kobject *kobj,
> return -EINVAL;
> }
>
> - spin_lock(&efivars->lock);
> + spin_lock_irqsave(&efivars->lock, flags);
>
> /*
> * Does this variable already exist?
> @@ -840,7 +851,7 @@ static ssize_t efivar_create(struct file *filp, struct kobject *kobj,
> }
> }
> if (found) {
> - spin_unlock(&efivars->lock);
> + spin_unlock_irqrestore(&efivars->lock, flags);
> return -EINVAL;
> }
>
> @@ -854,10 +865,10 @@ static ssize_t efivar_create(struct file *filp, struct kobject *kobj,
> if (status != EFI_SUCCESS) {
> printk(KERN_WARNING "efivars: set_variable() failed: status=%lx\n",
> status);
> - spin_unlock(&efivars->lock);
> + spin_unlock_irqrestore(&efivars->lock, flags);
> return -EIO;
> }
> - spin_unlock(&efivars->lock);
> + spin_unlock_irqrestore(&efivars->lock, flags);
>
> /* Create the entry in sysfs. Locking is not required here */
> status = efivar_create_sysfs_entry(efivars,
> @@ -881,11 +892,12 @@ static ssize_t efivar_delete(struct file *filp, struct kobject *kobj,
> unsigned long strsize1, strsize2;
> efi_status_t status = EFI_NOT_FOUND;
> int found = 0;
> + unsigned long flags;
>
> if (!capable(CAP_SYS_ADMIN))
> return -EACCES;
>
> - spin_lock(&efivars->lock);
> + spin_lock_irqsave(&efivars->lock, flags);
>
> /*
> * Does this variable already exist?
> @@ -903,7 +915,7 @@ static ssize_t efivar_delete(struct file *filp, struct kobject *kobj,
> }
> }
> if (!found) {
> - spin_unlock(&efivars->lock);
> + spin_unlock_irqrestore(&efivars->lock, flags);
> return -EINVAL;
> }
> /* force the Attributes/DataSize to 0 to ensure deletion */
> @@ -919,12 +931,12 @@ static ssize_t efivar_delete(struct file *filp, struct kobject *kobj,
> if (status != EFI_SUCCESS) {
> printk(KERN_WARNING "efivars: set_variable() failed: status=%lx\n",
> status);
> - spin_unlock(&efivars->lock);
> + spin_unlock_irqrestore(&efivars->lock, flags);
> return -EIO;
> }
> list_del(&search_efivar->list);
> /* We need to release this lock before unregistering. */
> - spin_unlock(&efivars->lock);
> + spin_unlock_irqrestore(&efivars->lock, flags);
> efivar_unregister(search_efivar);
>
> /* It's dead Jim.... */
> @@ -993,6 +1005,7 @@ efivar_create_sysfs_entry(struct efivars *efivars,
> int i, short_name_size = variable_name_size / sizeof(efi_char16_t) + 38;
> char *short_name;
> struct efivar_entry *new_efivar;
> + unsigned long flags;
>
> short_name = kzalloc(short_name_size + 1, GFP_KERNEL);
> new_efivar = kzalloc(sizeof(struct efivar_entry), GFP_KERNEL);
> @@ -1032,9 +1045,9 @@ efivar_create_sysfs_entry(struct efivars *efivars,
> kfree(short_name);
> short_name = NULL;
>
> - spin_lock(&efivars->lock);
> + spin_lock_irqsave(&efivars->lock, flags);
> list_add(&new_efivar->list, &efivars->list);
> - spin_unlock(&efivars->lock);
> + spin_unlock_irqrestore(&efivars->lock, flags);
>
> return 0;
> }
> @@ -1101,11 +1114,12 @@ out_free:
> void unregister_efivars(struct efivars *efivars)
> {
> struct efivar_entry *entry, *n;
> + unsigned long flags;
>
> list_for_each_entry_safe(entry, n, &efivars->list, list) {
> - spin_lock(&efivars->lock);
> + spin_lock_irqsave(&efivars->lock, flags);
> list_del(&entry->list);
> - spin_unlock(&efivars->lock);
> + spin_unlock_irqrestore(&efivars->lock, flags);
> efivar_unregister(entry);
> }
> if (efivars->new_var)
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 103adc6..f1f5703 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -649,6 +649,9 @@ struct efivars {
> struct kset *kset;
> struct bin_attribute *new_var, *del_var;
> const struct efivar_operations *ops;
> + /*
> + * walk_entry is protected by pstore_info.read_mutex
> + */
> struct efivar_entry *walk_entry;
> struct pstore_info efi_pstore_info;
> };
> -- 1.7.1
|
|
From: Seiji A. <sei...@hd...> - 2012-08-17 19:43:14
|
[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 kicked after a write callback of efi_pstore is called.
efi_pstore will be robust against a kernel panic in an interrupt context with it.
The workqueue works as follows.
- A timer is registered during an initialization of efivars module.
- A flag, update_sysfs_entries, is checked periodically with the timer.
- The flag is set in a write callback, efi_pstore_write().
- Operation updating sysfs entries is kicked if the flag is set.
Any operations ,like registering a timer, should not be added in a write callback because it may run
in panic case and fail due to operations related to workqueue.
To make efi_pstore work in panic case, a write callback should just log to NVRAM.
Signed-off-by: Seiji Aguchi <sei...@hd...>
---
drivers/firmware/efivars.c | 127 ++++++++++++++++++++++++++++++++++++++++----
include/linux/efi.h | 3 +-
2 files changed, 118 insertions(+), 12 deletions(-)
diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index b6a813e..9f27ef6 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -146,6 +146,17 @@ 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 *);
+#define EFIVAR_INTERVAL (60 * HZ)
+static void efivar_timefunc(unsigned long);
+static DEFINE_TIMER(efivar_timer, efivar_timefunc, 0, 0);
+static DECLARE_WORK(efivar_work, efivar_update_sysfs_entry);
+static int update_sysfs_entries;
+
/* Return the number of unicode characters in data */
static unsigned long
utf16_strnlen(efi_char16_t *s, size_t maxlength)
@@ -739,9 +750,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];
@@ -750,14 +758,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);
+ update_sysfs_entries = 1;
*id = part;
return ret;
@@ -1201,6 +1202,8 @@ int register_efivars(struct efivars *efivars,
pstore_register(&efivars->efi_pstore_info);
}
+ add_timer(&efivar_timer);
+
out:
kfree(variable_name);
@@ -1211,6 +1214,107 @@ 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) {
+ 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();
+
+ /* 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);
+ }
+}
+
+static void efivar_timefunc(unsigned long dummy)
+{
+ if (update_sysfs_entries) {
+ update_sysfs_entries = 0;
+ schedule_work(&efivar_work);
+ }
+
+ mod_timer(&efivar_timer, jiffies + EFIVAR_INTERVAL);
+}
+
/*
* For now we register the efi subsystem with the firmware subsystem
* and the vars subsystem with the efi subsystem. In the future, it
@@ -1266,6 +1370,7 @@ static void __exit
efivars_exit(void)
{
if (efi_enabled) {
+ del_timer_sync(&efivar_timer);
unregister_efivars(&__efivars);
kobject_put(efi_kobj);
}
diff --git a/include/linux/efi.h b/include/linux/efi.h
index f1f5703..3e0c37a 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
|
|
From: Seiji A. <sei...@hd...> - 2012-08-17 19:42:32
|
[Problem]
Currently, efivars doesn't disable interrupt while taking efivars->lock.
So, there is a risk to be deadlocking in a write callback of efi_pstore
if kernel panics in interrupt context while taking efivars->lock.
[Patch Description]
This patch disables an external interruption while holding efivars->lock by
replacing spin_lock/spin_unlock with spin_lock_irqsave/spin_unlock_irqrestore.
As for spin_locks in open/close callbacks of efi_pstore, they moved to a read callback
to pass a flags argument to spin_lock_irqsave/spin_unlock_irqrestore.
Even if spin_locks in open/close callbacks are erased, a whole process of open/read/close
is still serialized by psinfo->read_mutex which is taken in a pstore layer.
Also, initialization of efivars->walk_entry moved to a read callback to protect list_first_entry()
with efivars->lock. And some comments about efivars->walk_entry are added to avoid using it
without taking psinfo->read_mutex.
Signed-off-by: Seiji Aguchi <sei...@hd...>
---
drivers/firmware/efivars.c | 66 ++++++++++++++++++++++++++-----------------
include/linux/efi.h | 3 ++
2 files changed, 43 insertions(+), 26 deletions(-)
diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 47408e8..b6a813e 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -393,10 +393,11 @@ static efi_status_t
get_var_data(struct efivars *efivars, struct efi_variable *var)
{
efi_status_t status;
+ unsigned long flags;
- spin_lock(&efivars->lock);
+ spin_lock_irqsave(&efivars->lock, flags);
status = get_var_data_locked(efivars, var);
- spin_unlock(&efivars->lock);
+ spin_unlock_irqrestore(&efivars->lock, flags);
if (status != EFI_SUCCESS) {
printk(KERN_WARNING "efivars: get_variable() failed 0x%lx!\n",
@@ -488,6 +489,7 @@ efivar_store_raw(struct efivar_entry *entry, const char *buf, size_t count)
struct efi_variable *new_var, *var = &entry->var;
struct efivars *efivars = entry->efivars;
efi_status_t status = EFI_NOT_FOUND;
+ unsigned long flags;
if (count != sizeof(struct efi_variable))
return -EINVAL;
@@ -514,14 +516,14 @@ efivar_store_raw(struct efivar_entry *entry, const char *buf, size_t count)
return -EINVAL;
}
- spin_lock(&efivars->lock);
+ spin_lock_irqsave(&efivars->lock, flags);
status = efivars->ops->set_variable(new_var->VariableName,
&new_var->VendorGuid,
new_var->Attributes,
new_var->DataSize,
new_var->Data);
- spin_unlock(&efivars->lock);
+ spin_unlock_irqrestore(&efivars->lock, flags);
if (status != EFI_SUCCESS) {
printk(KERN_WARNING "efivars: set_variable() failed: status=%lx\n",
@@ -630,19 +632,11 @@ efivar_unregister(struct efivar_entry *var)
static int efi_pstore_open(struct pstore_info *psi)
{
- struct efivars *efivars = psi->data;
-
- spin_lock(&efivars->lock);
- efivars->walk_entry = list_first_entry(&efivars->list,
- struct efivar_entry, list);
return 0;
}
static int efi_pstore_close(struct pstore_info *psi)
{
- struct efivars *efivars = psi->data;
-
- spin_unlock(&efivars->lock);
return 0;
}
@@ -656,6 +650,17 @@ static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type,
int i;
unsigned int part, size;
unsigned long time;
+ static unsigned long flags;
+
+ /*
+ * efivars->walk_entry is protected by pstore_info.read_mutex
+ */
+ if (!efivars->walk_entry) {
+ spin_lock_irqsave(&efivars->lock, flags);
+ efivars->walk_entry = list_first_entry(&efivars->list,
+ struct efivar_entry,
+ list);
+ }
while (&efivars->walk_entry->list != &efivars->list) {
if (!efi_guidcmp(efivars->walk_entry->var.VendorGuid,
@@ -682,6 +687,10 @@ static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type,
efivars->walk_entry = list_entry(efivars->walk_entry->list.next,
struct efivar_entry, list);
}
+
+ efivars->walk_entry = NULL;
+ spin_unlock_irqrestore(&efivars->lock, flags);
+
return 0;
}
@@ -696,11 +705,12 @@ static int efi_pstore_write(enum pstore_type_id type,
struct efivars *efivars = psi->data;
struct efivar_entry *entry, *found = NULL;
int i, ret = 0;
+ unsigned long flags;
sprintf(stub_name, "dump-type%u-%u-", type, part);
sprintf(name, "%s%lu", stub_name, get_seconds());
- spin_lock(&efivars->lock);
+ spin_lock_irqsave(&efivars->lock, flags);
for (i = 0; i < DUMP_NAME_LEN; i++)
efi_name[i] = stub_name[i];
@@ -738,7 +748,7 @@ static int efi_pstore_write(enum pstore_type_id type,
efivars->ops->set_variable(efi_name, &vendor, PSTORE_EFI_ATTRIBUTES,
size, psi->buf);
- spin_unlock(&efivars->lock);
+ spin_unlock_irqrestore(&efivars->lock, flags);
if (found)
efivar_unregister(found);
@@ -812,6 +822,7 @@ static ssize_t efivar_create(struct file *filp, struct kobject *kobj,
unsigned long strsize1, strsize2;
efi_status_t status = EFI_NOT_FOUND;
int found = 0;
+ unsigned long flags;
if (!capable(CAP_SYS_ADMIN))
return -EACCES;
@@ -822,7 +833,7 @@ static ssize_t efivar_create(struct file *filp, struct kobject *kobj,
return -EINVAL;
}
- spin_lock(&efivars->lock);
+ spin_lock_irqsave(&efivars->lock, flags);
/*
* Does this variable already exist?
@@ -840,7 +851,7 @@ static ssize_t efivar_create(struct file *filp, struct kobject *kobj,
}
}
if (found) {
- spin_unlock(&efivars->lock);
+ spin_unlock_irqrestore(&efivars->lock, flags);
return -EINVAL;
}
@@ -854,10 +865,10 @@ static ssize_t efivar_create(struct file *filp, struct kobject *kobj,
if (status != EFI_SUCCESS) {
printk(KERN_WARNING "efivars: set_variable() failed: status=%lx\n",
status);
- spin_unlock(&efivars->lock);
+ spin_unlock_irqrestore(&efivars->lock, flags);
return -EIO;
}
- spin_unlock(&efivars->lock);
+ spin_unlock_irqrestore(&efivars->lock, flags);
/* Create the entry in sysfs. Locking is not required here */
status = efivar_create_sysfs_entry(efivars,
@@ -881,11 +892,12 @@ static ssize_t efivar_delete(struct file *filp, struct kobject *kobj,
unsigned long strsize1, strsize2;
efi_status_t status = EFI_NOT_FOUND;
int found = 0;
+ unsigned long flags;
if (!capable(CAP_SYS_ADMIN))
return -EACCES;
- spin_lock(&efivars->lock);
+ spin_lock_irqsave(&efivars->lock, flags);
/*
* Does this variable already exist?
@@ -903,7 +915,7 @@ static ssize_t efivar_delete(struct file *filp, struct kobject *kobj,
}
}
if (!found) {
- spin_unlock(&efivars->lock);
+ spin_unlock_irqrestore(&efivars->lock, flags);
return -EINVAL;
}
/* force the Attributes/DataSize to 0 to ensure deletion */
@@ -919,12 +931,12 @@ static ssize_t efivar_delete(struct file *filp, struct kobject *kobj,
if (status != EFI_SUCCESS) {
printk(KERN_WARNING "efivars: set_variable() failed: status=%lx\n",
status);
- spin_unlock(&efivars->lock);
+ spin_unlock_irqrestore(&efivars->lock, flags);
return -EIO;
}
list_del(&search_efivar->list);
/* We need to release this lock before unregistering. */
- spin_unlock(&efivars->lock);
+ spin_unlock_irqrestore(&efivars->lock, flags);
efivar_unregister(search_efivar);
/* It's dead Jim.... */
@@ -993,6 +1005,7 @@ efivar_create_sysfs_entry(struct efivars *efivars,
int i, short_name_size = variable_name_size / sizeof(efi_char16_t) + 38;
char *short_name;
struct efivar_entry *new_efivar;
+ unsigned long flags;
short_name = kzalloc(short_name_size + 1, GFP_KERNEL);
new_efivar = kzalloc(sizeof(struct efivar_entry), GFP_KERNEL);
@@ -1032,9 +1045,9 @@ efivar_create_sysfs_entry(struct efivars *efivars,
kfree(short_name);
short_name = NULL;
- spin_lock(&efivars->lock);
+ spin_lock_irqsave(&efivars->lock, flags);
list_add(&new_efivar->list, &efivars->list);
- spin_unlock(&efivars->lock);
+ spin_unlock_irqrestore(&efivars->lock, flags);
return 0;
}
@@ -1101,11 +1114,12 @@ out_free:
void unregister_efivars(struct efivars *efivars)
{
struct efivar_entry *entry, *n;
+ unsigned long flags;
list_for_each_entry_safe(entry, n, &efivars->list, list) {
- spin_lock(&efivars->lock);
+ spin_lock_irqsave(&efivars->lock, flags);
list_del(&entry->list);
- spin_unlock(&efivars->lock);
+ spin_unlock_irqrestore(&efivars->lock, flags);
efivar_unregister(entry);
}
if (efivars->new_var)
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 103adc6..f1f5703 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -649,6 +649,9 @@ struct efivars {
struct kset *kset;
struct bin_attribute *new_var, *del_var;
const struct efivar_operations *ops;
+ /*
+ * walk_entry is protected by pstore_info.read_mutex
+ */
struct efivar_entry *walk_entry;
struct pstore_info efi_pstore_info;
};
-- 1.7.1
|
|
From: Seiji A. <sei...@hd...> - 2012-08-17 19:41:32
|
[Problem] There are following problems related to an interrupt context in efivar/efi_pstore. Currently, efivars enables interrupt while taking efivars->lock. So, there is a risk to be deadlocking in a write callback of efi_pstore if kernel panics in interrupt context while taking efi_lock. Also, 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. To resolve the problems above, a goal of this patchset is making efivars/efi_pstore interrupt-safe. [Patch Description] Patch 1/2 efivars: Disable external interrupt while holding efivars->lock This patch replaces spin_lock/spin_unlock with spin_lock_irqsave/spin_lock_irqrestore to make efivars interrupt safe Patch 2/2 efi_pstore: Introducing workqueue updating sysfs entries This patch removes sysfs operations from write callback by introducing a workqueue updating sysfs entries drivers/firmware/efivars.c | 193 +++++++++++++++++++++++++++++++++++--------- include/linux/efi.h | 6 +- 2 files changed, 161 insertions(+), 38 deletions(-) |
|
From: Matt F. <mat...@in...> - 2012-08-17 10:32:01
|
On Thu, 2012-08-16 at 14:43 -0700, H. Peter Anvin wrote:
> On 08/16/2012 12:53 AM, Matt Fleming wrote:
> > On Mon, 2012-08-06 at 18:37 +0000, Seiji Aguchi wrote:
> >> A value of efi.runtime_version is checked before calling
> >> update_capsule()/query_variable_info() as follows.
> >> But it isn't initialized anywhere.
> >> <snip>
> >> static efi_status_t virt_efi_query_variable_info(u32 attr,
> >> u64 *storage_space,
> >> u64 *remaining_space,
> >> u64 *max_variable_size) {
> >> if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
> >> return EFI_UNSUPPORTED;
> >> <snip>
> >>
> >> This patch initializes a value of efi.runtime_version at boot time.
> >>
> >> Acked-by: Matthew Garrett <mj...@re...>
> >> Signed-off-by: Seiji Aguchi <sei...@hd...>
> >> ---
> >> arch/x86/platform/efi/efi.c | 1 +
> >> 1 files changed, 1 insertions(+), 0 deletions(-)
> >
> > Thanks, I picked this up just so it doesn't get lost. It will probably
> > go to Linus via Peter Anvin.
> >
>
> Should I take that as an Acked-by: on your part?
Yep.
|
|
From: H. P. A. <hp...@li...> - 2012-08-16 21:44:10
|
On 08/16/2012 12:53 AM, Matt Fleming wrote:
> On Mon, 2012-08-06 at 18:37 +0000, Seiji Aguchi wrote:
>> A value of efi.runtime_version is checked before calling
>> update_capsule()/query_variable_info() as follows.
>> But it isn't initialized anywhere.
>> <snip>
>> static efi_status_t virt_efi_query_variable_info(u32 attr,
>> u64 *storage_space,
>> u64 *remaining_space,
>> u64 *max_variable_size) {
>> if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
>> return EFI_UNSUPPORTED;
>> <snip>
>>
>> This patch initializes a value of efi.runtime_version at boot time.
>>
>> Acked-by: Matthew Garrett <mj...@re...>
>> Signed-off-by: Seiji Aguchi <sei...@hd...>
>> ---
>> arch/x86/platform/efi/efi.c | 1 +
>> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> Thanks, I picked this up just so it doesn't get lost. It will probably
> go to Linus via Peter Anvin.
>
Should I take that as an Acked-by: on your part?
-hpa
|
|
From: Matt F. <mat...@in...> - 2012-08-16 07:53:48
|
On Mon, 2012-08-06 at 18:37 +0000, Seiji Aguchi wrote:
> A value of efi.runtime_version is checked before calling
> update_capsule()/query_variable_info() as follows.
> But it isn't initialized anywhere.
> <snip>
> static efi_status_t virt_efi_query_variable_info(u32 attr,
> u64 *storage_space,
> u64 *remaining_space,
> u64 *max_variable_size) {
> if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
> return EFI_UNSUPPORTED;
> <snip>
>
> This patch initializes a value of efi.runtime_version at boot time.
>
> Acked-by: Matthew Garrett <mj...@re...>
> Signed-off-by: Seiji Aguchi <sei...@hd...>
> ---
> arch/x86/platform/efi/efi.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
Thanks, I picked this up just so it doesn't get lost. It will probably
go to Linus via Peter Anvin.
|
|
From: Seiji A. <sei...@hd...> - 2012-08-14 21:38:29
|
>
> Something like this (pseudo) may work:
>
> /* Loop until we have all entries in efivars. */ while (1) {
> variable_name = kzalloc(1024, GFP_KERNEL );
> spin_lock_irqsave(&efivars->lock);
> bool found = false;
> while (1) {
> ret = ops->get_next_variable(variable_name)
> if (ret == EFI_NOT_FOUND)
> break;
> if (!variable_is_already_present(variable, &efivars->list) {
> found = true;
> break;
> }
> }
> if (!found) {
> kfree(variable_name);
> break;
> }
> <Register in sysfs>
> }
>
Thanks. It seems to work.
I will create a patch based on your pseudo code above.
>
> If we are calling into pstore_info ops though from interrupt context, it seems we'll need to fix the immediate problem of updating the
> locking throughout to be interrupt safe.
>
I agree with you. It should be fixing. I will update the locking to be interrupt safe as well.
Seiji
|
|
From: Mike W. <mi...@go...> - 2012-08-14 20:48:02
|
On Tue, Aug 14, 2012 at 12:29 PM, Seiji Aguchi <sei...@hd...> wrote:
> Mike,
>
> Previous pseudo code was not correct.
> More precisely, it is as follows.
> But, we still need to alloc memory dynamically in the part of <Save information about new entry>
> because a workqueue can't know how many new entries in advance.
>
> spin_lock_irqsave(&efivars_lock);
>
> do {
> get_next_variable();
> if (found new entry)
> <Save information about new entry>
If the work item is changed to instead do a loop, storage for this may
be saved elsewhere, either the stack (though 1KiB is sorta pushing it)
or on the heap.
>
> } while()
>
> spin_lock_irqrestore(&efivars_lock);
>
> efivars_create_sysfs_entries();
Something like this (pseudo) may work:
/* Loop until we have all entries in efivars. */
while (1) {
variable_name = kzalloc(1024, GFP_KERNEL );
spin_lock_irqsave(&efivars->lock);
bool found = false;
while (1) {
ret = ops->get_next_variable(variable_name)
if (ret == EFI_NOT_FOUND)
break;
if (!variable_is_already_present(variable, &efivars->list) {
found = true;
break;
}
}
if (!found) {
kfree(variable_name);
break;
}
<Register in sysfs>
}
If we are calling into pstore_info ops though from interrupt context,
it seems we'll need to fix the immediate problem of updating the
locking throughout to be interrupt safe.
>
> Seiji
>
>> -----Original Message-----
>> From: lin...@vg... [mailto:lin...@vg...] On Behalf Of Seiji Aguchi
>> Sent: Tuesday, August 14, 2012 2:52 PM
>> To: Mike Waychison
>> Cc: lin...@vg...; Luck, Tony (ton...@in...); Matthew Garrett (mj...@re...); dz...@re...; dle-
>> de...@li...; Satoru Moriya
>> Subject: RE: efi_pstore: question about how to remove create_sysfs_entry() from a write callback.
>>
>> > Can we not serialize this with &efivars->lock if it is updated to
>> > disable interrupts? A loop in the workqueue that locks, iterates
>> > through
>> > ->get_next_variable, and compares against searches in
>> > efivars->list should work, no?
>>
>> If my understanding is correct, your pseudo code is as follows.
>>
>> spin_lock_irqsave(&efivars_lock);
>>
>> do {
>> get_next_variable();
>> if (found new entry)
>> efivars_create_sysfs_entries();
>>
>> } while()
>>
>> spin_lock_irqrestore(&efivars_lock);
>>
>> But, memory is allocated dynamically with kzalloc() in efivars_create_sysfs_entries().
>> I don't want to allocate memory while holding spin_lock.
>>
>> Please let me know if I'm missing something.
>>
>> Seiji
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to maj...@vg... More
>> majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
|
454 messages has been excluded from this view by a project administrator.
