[PATCH] Fix race in efi variable delete code.

[PATCH] Fix race in efi variable delete code.

[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);
 

Re: [PATCH] Fix race in efi variable delete code.

[Matt Domsch]

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

[PATCH] Fix race in efi variable delete code

[Matt Domsch]

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);

Re: [PATCH] Fix race in efi variable delete code

[Andrew Morton]

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...