efi: vars: Don't drop lock in the middle of efivar_init()

Even though the efivars_lock lock is documented as protecting the
efivars->ops pointer (among other things), efivar_init() happily
releases and reacquires the lock for every EFI variable that it
enumerates. This used to be needed because the lock was originally a
spinlock, which prevented the callback that is invoked for every
variable from being able to sleep. However, releasing the lock could
potentially invalidate the ops pointer, but more importantly, it might
allow a SetVariable() runtime service call to take place concurrently,
and the UEFI spec does not define how this affects an enumeration that
is running in parallel using the GetNextVariable() runtime service,
which is what efivar_init() uses.

In the meantime, the lock has been converted into a semaphore, and the
only reason we need to drop the lock is because the efivarfs pseudo
filesystem driver will otherwise deadlock when it invokes the efivars
API from the callback to create the efivar_entry items and insert them
into the linked list. (EFI pstore is affected in a similar way)

So let's switch to helpers that can be used while the lock is already
taken. This way, we can hold on to the lock throughout the enumeration.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c
index 7e771c56c13c6194cbda0ac31a1359dd9d2d8d8f..0d80cc7ff6ca79057d058ac7f9d29546b46e4b0b 100644 (file)
--- a/drivers/firmware/efi/efi-pstore.c
+++ b/drivers/firmware/efi/efi-pstore.c
@@ -364,7 +364,6 @@ static int efi_pstore_callback(efi_char16_t *name, efi_guid_t vendor,
                               unsigned long name_size, void *data)
 {
        struct efivar_entry *entry;
-       int ret;
 
        entry = kzalloc(sizeof(*entry), GFP_KERNEL);
        if (!entry)
@@ -373,11 +372,9 @@ static int efi_pstore_callback(efi_char16_t *name, efi_guid_t vendor,
        memcpy(entry->var.VariableName, name, name_size);
        entry->var.VendorGuid = vendor;
 
-       ret = efivar_entry_add(entry, &efi_pstore_list);
-       if (ret)
-               kfree(entry);
+       __efivar_entry_add(entry, &efi_pstore_list);
 
-       return ret;
+       return 0;
 }
 
 static int efi_pstore_update_entry(efi_char16_t *name, efi_guid_t vendor,

diff --git a/drivers/firmware/efi/efivars.c b/drivers/firmware/efi/efivars.c
index ea0bc39dc96576e88ce4c7279d090693d0fc5661..c19db0b35c0de5294e3ce33c031e409b27ca5166 100644 (file)
--- a/drivers/firmware/efi/efivars.c
+++ b/drivers/firmware/efi/efivars.c
@@ -527,10 +527,7 @@ efivar_create_sysfs_entry(struct efivar_entry *new_var)
        }
 
        kobject_uevent(&new_var->kobj, KOBJ_ADD);
-       if (efivar_entry_add(new_var, &efivar_sysfs_list)) {
-               efivar_unregister(new_var);
-               return -EINTR;
-       }
+       __efivar_entry_add(new_var, &efivar_sysfs_list);
 
        return 0;
 }

diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index cae590bd08f27c3c769459802d3546d1542c2ff8..146360e2f1cb542ce3fc0b0c27c3bba3d1427d1f 100644 (file)
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -450,9 +450,6 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
                                                &vendor_guid);
                switch (status) {
                case EFI_SUCCESS:
-                       if (duplicates)
-                               up(&efivars_lock);
-
                        variable_name_size = var_name_strnsize(variable_name,
                                                               variable_name_size);
 
@@ -476,14 +473,6 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
                                if (err)
                                        status = EFI_NOT_FOUND;
                        }
-
-                       if (duplicates) {
-                               if (down_interruptible(&efivars_lock)) {
-                                       err = -EINTR;
-                                       goto free;
-                               }
-                       }
-
                        break;
                case EFI_UNSUPPORTED:
                        err = -EOPNOTSUPP;
@@ -526,6 +515,17 @@ int efivar_entry_add(struct efivar_entry *entry, struct list_head *head)
 }
 EXPORT_SYMBOL_GPL(efivar_entry_add);
 
+/**
+ * __efivar_entry_add - add entry to variable list
+ * @entry: entry to add to list
+ * @head: list head
+ */
+void __efivar_entry_add(struct efivar_entry *entry, struct list_head *head)
+{
+       list_add(&entry->list, head);
+}
+EXPORT_SYMBOL_GPL(__efivar_entry_add);
+
 /**
  * efivar_entry_remove - remove entry from variable list
  * @entry: entry to remove from list

diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
index 15880a68faadc58ba34d1a3f68ac412416202f2e..09dfa8362f507b34d95daf23531eab4789b4ce16 100644 (file)
--- a/fs/efivarfs/super.c
+++ b/fs/efivarfs/super.c
@@ -155,10 +155,8 @@ static int efivarfs_callback(efi_char16_t *name16, efi_guid_t vendor,
                goto fail_inode;
        }
 
-       efivar_entry_size(entry, &size);
-       err = efivar_entry_add(entry, &efivarfs_list);
-       if (err)
-               goto fail_inode;
+       __efivar_entry_get(entry, NULL, &size, NULL);
+       __efivar_entry_add(entry, &efivarfs_list);
 
        /* copied by the above to local storage in the dentry. */
        kfree(name);

diff --git a/include/linux/efi.h b/include/linux/efi.h
index 53f64c14a525be2de747693f3e36533a4c8cb959..56f04b6daeb0c9863356c3e873d6a703076d0fec 100644 (file)
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1064,6 +1064,7 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
                void *data, bool duplicates, struct list_head *head);
 
 int efivar_entry_add(struct efivar_entry *entry, struct list_head *head);
+void __efivar_entry_add(struct efivar_entry *entry, struct list_head *head);
 int efivar_entry_remove(struct efivar_entry *entry);
 
 int __efivar_entry_delete(struct efivar_entry *entry);