* [PATCH] Fix race in efi variable delete code.
@ 2007-01-22 15:22 Prarit Bhargava
2007-01-25 20:34 ` Matt Domsch
0 siblings, 1 reply; 4+ messages in thread
From: Prarit Bhargava @ 2007-01-22 15:22 UTC (permalink / raw)
To: linux-kernel
Cc: matt_domsch, matthew.e.tolentino, anil.s.keshavamurthy, Prarit Bhargava
Fix race when deleting an EFI variable and issuing another EFI command on the
same variable. The removal of the variable from the efivars_list should be
done in efivar_delete and not delayed until the kprobes release.
Signed-off-by: Prarit Bhargava <prarit@redhat.com>
diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 5ab5e39..bf2ca97 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -385,10 +385,8 @@ static struct sysfs_ops efivar_attr_ops = {
static void efivar_release(struct kobject *kobj)
{
- struct efivar_entry *var = container_of(kobj, struct efivar_entry, kobj);
- spin_lock(&efivars_lock);
- list_del(&var->list);
- spin_unlock(&efivars_lock);
+ struct efivar_entry *var = container_of(kobj, struct efivar_entry,
+ kobj);
kfree(var);
}
@@ -537,6 +535,9 @@ efivar_delete(struct subsystem *sub, const char *buf, size_t count)
spin_unlock(&efivars_lock);
return -EIO;
}
+
+ list_del(&search_efivar->list);
+
/* We need to release this lock before unregistering. */
spin_unlock(&efivars_lock);
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] Fix race in efi variable delete code.
2007-01-22 15:22 [PATCH] Fix race in efi variable delete code Prarit Bhargava
@ 2007-01-25 20:34 ` Matt Domsch
2007-01-25 22:20 ` Matt Domsch
0 siblings, 1 reply; 4+ messages in thread
From: Matt Domsch @ 2007-01-25 20:34 UTC (permalink / raw)
To: Prarit Bhargava; +Cc: linux-kernel, matthew.e.tolentino, anil.s.keshavamurthy
On Mon, Jan 22, 2007 at 10:22:09AM -0500, Prarit Bhargava wrote:
> Fix race when deleting an EFI variable and issuing another EFI command on the
> same variable. The removal of the variable from the efivars_list should be
> done in efivar_delete and not delayed until the kprobes release.
>
> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
But then we'll leave it dangling on the list at module unload time.
Let me rework this and send you something to try.
Thanks,
Matt
--
Matt Domsch
Software Architect
Dell Linux Solutions linux.dell.com & www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] Fix race in efi variable delete code
2007-01-25 20:34 ` Matt Domsch
@ 2007-01-25 22:20 ` Matt Domsch
2007-01-26 6:00 ` Andrew Morton
0 siblings, 1 reply; 4+ messages in thread
From: Matt Domsch @ 2007-01-25 22:20 UTC (permalink / raw)
To: Prarit Bhargava
Cc: linux-kernel, matthew.e.tolentino, anil.s.keshavamurthy, akpm
Fix race when deleting an EFI variable and issuing another EFI command on the
same variable. The removal of the variable from the efivars_list should be
done in efivar_delete and not delayed until the kobject release.
Furthermore, remove the item from the list at module unload time, and
use list_for_each_entry_safe() rather than list_for_each_safe() for readability.
Tested on ia64.
Signed-off-by: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: Matt Domsch <Matt_Domsch@dell.com>
--
Matt Domsch
Software Architect
Dell Linux Solutions linux.dell.com & www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com
--- linux-2.6/drivers/firmware/efivars.c.orig Thu Jan 25 14:35:05 2007
+++ linux-2.6/drivers/firmware/efivars.c Thu Jan 25 15:00:45 2007
@@ -122,8 +122,6 @@ struct efivar_entry {
struct kobject kobj;
};
-#define get_efivar_entry(n) list_entry(n, struct efivar_entry, list)
-
struct efivar_attribute {
struct attribute attr;
ssize_t (*show) (struct efivar_entry *entry, char *buf);
@@ -386,9 +384,6 @@ static struct sysfs_ops efivar_attr_ops
static void efivar_release(struct kobject *kobj)
{
struct efivar_entry *var = container_of(kobj, struct efivar_entry, kobj);
- spin_lock(&efivars_lock);
- list_del(&var->list);
- spin_unlock(&efivars_lock);
kfree(var);
}
@@ -430,9 +425,8 @@ static ssize_t
efivar_create(struct subsystem *sub, const char *buf, size_t count)
{
struct efi_variable *new_var = (struct efi_variable *)buf;
- struct efivar_entry *search_efivar = NULL;
+ struct efivar_entry *search_efivar, *n;
unsigned long strsize1, strsize2;
- struct list_head *pos, *n;
efi_status_t status = EFI_NOT_FOUND;
int found = 0;
@@ -444,8 +438,7 @@ efivar_create(struct subsystem *sub, con
/*
* Does this variable already exist?
*/
- list_for_each_safe(pos, n, &efivar_list) {
- search_efivar = get_efivar_entry(pos);
+ list_for_each_entry_safe(search_efivar, n, &efivar_list, list) {
strsize1 = utf8_strsize(search_efivar->var.VariableName, 1024);
strsize2 = utf8_strsize(new_var->VariableName, 1024);
if (strsize1 == strsize2 &&
@@ -490,9 +483,8 @@ static ssize_t
efivar_delete(struct subsystem *sub, const char *buf, size_t count)
{
struct efi_variable *del_var = (struct efi_variable *)buf;
- struct efivar_entry *search_efivar = NULL;
+ struct efivar_entry *search_efivar, *n;
unsigned long strsize1, strsize2;
- struct list_head *pos, *n;
efi_status_t status = EFI_NOT_FOUND;
int found = 0;
@@ -504,8 +496,7 @@ efivar_delete(struct subsystem *sub, con
/*
* Does this variable already exist?
*/
- list_for_each_safe(pos, n, &efivar_list) {
- search_efivar = get_efivar_entry(pos);
+ list_for_each_entry_safe(search_efivar, n, &efivar_list, list) {
strsize1 = utf8_strsize(search_efivar->var.VariableName, 1024);
strsize2 = utf8_strsize(del_var->VariableName, 1024);
if (strsize1 == strsize2 &&
@@ -537,9 +528,9 @@ efivar_delete(struct subsystem *sub, con
spin_unlock(&efivars_lock);
return -EIO;
}
+ list_del(&search_efivar->list);
/* We need to release this lock before unregistering. */
spin_unlock(&efivars_lock);
-
efivar_unregister(search_efivar);
/* It's dead Jim.... */
@@ -768,10 +759,14 @@ out_free:
static void __exit
efivars_exit(void)
{
- struct list_head *pos, *n;
+ struct efivar_entry *entry, *n;
- list_for_each_safe(pos, n, &efivar_list)
- efivar_unregister(get_efivar_entry(pos));
+ list_for_each_entry_safe(entry, n, &efivar_list, list) {
+ spin_lock(&efivars_lock);
+ list_del(&entry->list);
+ spin_unlock(&efivars_lock);
+ efivar_unregister(entry);
+ }
subsystem_unregister(&vars_subsys);
firmware_unregister(&efi_subsys);
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Fix race in efi variable delete code
2007-01-25 22:20 ` Matt Domsch
@ 2007-01-26 6:00 ` Andrew Morton
0 siblings, 0 replies; 4+ messages in thread
From: Andrew Morton @ 2007-01-26 6:00 UTC (permalink / raw)
To: Matt Domsch
Cc: Prarit Bhargava, linux-kernel, matthew.e.tolentino, anil.s.keshavamurthy
On Thu, 25 Jan 2007 16:20:56 -0600
Matt Domsch <Matt_Domsch@dell.com> wrote:
> Fix race when deleting an EFI variable and issuing another EFI command on the
> same variable. The removal of the variable from the efivars_list should be
> done in efivar_delete and not delayed until the kobject release.
>
> Furthermore, remove the item from the list at module unload time, and
> use list_for_each_entry_safe() rather than list_for_each_safe() for readability.
>
Does it actually need to use the _safe variant? That's only needed if the
body of the loop can do list_del() and afaict that doesn't happen here.
> static void __exit
> efivars_exit(void)
> {
> - struct list_head *pos, *n;
> + struct efivar_entry *entry, *n;
>
> - list_for_each_safe(pos, n, &efivar_list)
> - efivar_unregister(get_efivar_entry(pos));
> + list_for_each_entry_safe(entry, n, &efivar_list, list) {
> + spin_lock(&efivars_lock);
> + list_del(&entry->list);
> + spin_unlock(&efivars_lock);
> + efivar_unregister(entry);
> + }
That's not exactly a thing of beauty, sorry ;)
Given that the code is single-threaded here, there's nothing to race
against and I don't think we strictly need any locking at all. But
consistency is OK. Given the locking here I'm not sure that the code would
be safe against concurrent removes anyway.
A more idiomatic implementation would do:
while (!list_empty(&efivar_list)) {
struct efivar_entry *entry = list_entry(...);
list_del(...)
}
Anyway. Stuff to think about on a rainy day...
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2007-01-26 6:00 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-01-22 15:22 [PATCH] Fix race in efi variable delete code Prarit Bhargava
2007-01-25 20:34 ` Matt Domsch
2007-01-25 22:20 ` Matt Domsch
2007-01-26 6:00 ` Andrew Morton
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.