Fix race in efi variable delete code.

PATCH In review Prarit Bhargava 3 participants 4 messages Jan 22, 2007 – Jan 26, 2007

Share & tools

Prarit Bhargava

author

2007-01-22 15:22 UTC #0 lore raw

PATCH 1/1 Fix race in efi variable delete code.

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 <[email protected]>

drivers/firmware/efivars.c +5 -4

index 5ab5e39..bf2ca97 100644
@@ -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);

Matt Domsch

2007-01-25 20:34 UTC #1 lore raw

Quoted 5 lines · 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 <[email protected]>

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

Signature · 4 lines

Matt Domsch
Software Architect
Dell Linux Solutions linux.dell.com & www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com

Matt Domsch

2007-01-25 22:20 UTC #2 lore raw

PATCH 1/1 Fix race in efi variable delete code

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 <[email protected]>

Signed-off-by Matt Domsch <[email protected]>

Signature · 97 lines

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

Andrew Morton

2007-01-26 06:00 UTC #3 lore raw

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

Quoted 7 lines · On Thu, 25 Jan 2007 16:20:56 -0600 Matt Domsch <[email protected]> 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.

Quoted 4 lines

static void __exit
efivars_exit(void)
{
(quoted diff) +0 -1
-	struct list_head *pos, *n;

Review

Quoted 10 lines

(quoted diff) +7 -2

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