From 21b3ddd39feecd2f4d6c52bcd30f0a4fa14f125a Mon Sep 17 00:00:00 2001 From: Sylvain Chouleur Date: Fri, 15 Jul 2016 21:36:30 +0200 Subject: [PATCH] efi: Don't use spinlocks for efi vars All efivars operations are protected by a spinlock which prevents interruptions and preemption. This is too restricted, we just need a lock preventing concurrency. The idea is to use a semaphore of count 1 and to have two ways of locking, depending on the context: - In interrupt context, we call down_trylock(), if it fails we return an error - In normal context, we call down_interruptible() We don't use a mutex here because the mutex_trylock() function must not be called from interrupt context, whereas the down_trylock() can. Signed-off-by: Sylvain Chouleur Signed-off-by: Ard Biesheuvel Cc: Leif Lindholm Cc: Mark Rutland Cc: Sylvain Chouleur Signed-off-by: Matt Fleming --- drivers/firmware/efi/efi-pstore.c | 36 ++++++-- drivers/firmware/efi/efivars.c | 22 ++++- drivers/firmware/efi/vars.c | 137 +++++++++++++++++------------- fs/efivarfs/inode.c | 5 +- fs/efivarfs/super.c | 9 +- include/linux/efi.h | 6 +- 6 files changed, 139 insertions(+), 76 deletions(-) diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c index 30a24d09ea6cf..1c33d7469e4a2 100644 --- a/drivers/firmware/efi/efi-pstore.c +++ b/drivers/firmware/efi/efi-pstore.c @@ -125,16 +125,19 @@ static void efi_pstore_scan_sysfs_enter(struct efivar_entry *pos, * @entry: deleting entry * @turn_off_scanning: Check if a scanning flag should be turned off */ -static inline void __efi_pstore_scan_sysfs_exit(struct efivar_entry *entry, +static inline int __efi_pstore_scan_sysfs_exit(struct efivar_entry *entry, bool turn_off_scanning) { if (entry->deleting) { list_del(&entry->list); efivar_entry_iter_end(); efivar_unregister(entry); - efivar_entry_iter_begin(); + if (efivar_entry_iter_begin()) + return -EINTR; } else if (turn_off_scanning) entry->scanning = false; + + return 0; } /** @@ -144,13 +147,18 @@ static inline void __efi_pstore_scan_sysfs_exit(struct efivar_entry *entry, * @head: list head * @stop: a flag checking if scanning will stop */ -static void efi_pstore_scan_sysfs_exit(struct efivar_entry *pos, +static int efi_pstore_scan_sysfs_exit(struct efivar_entry *pos, struct efivar_entry *next, struct list_head *head, bool stop) { - __efi_pstore_scan_sysfs_exit(pos, true); + int ret = __efi_pstore_scan_sysfs_exit(pos, true); + + if (ret) + return ret; + if (stop) - __efi_pstore_scan_sysfs_exit(next, &next->list != head); + ret = __efi_pstore_scan_sysfs_exit(next, &next->list != head); + return ret; } /** @@ -172,13 +180,17 @@ static int efi_pstore_sysfs_entry_iter(void *data, struct efivar_entry **pos) struct efivar_entry *entry, *n; struct list_head *head = &efivar_sysfs_list; int size = 0; + int ret; if (!*pos) { list_for_each_entry_safe(entry, n, head, list) { efi_pstore_scan_sysfs_enter(entry, n, head); size = efi_pstore_read_func(entry, data); - efi_pstore_scan_sysfs_exit(entry, n, head, size < 0); + ret = efi_pstore_scan_sysfs_exit(entry, n, head, + size < 0); + if (ret) + return ret; if (size) break; } @@ -190,7 +202,9 @@ static int efi_pstore_sysfs_entry_iter(void *data, struct efivar_entry **pos) efi_pstore_scan_sysfs_enter((*pos), n, head); size = efi_pstore_read_func((*pos), data); - efi_pstore_scan_sysfs_exit((*pos), n, head, size < 0); + ret = efi_pstore_scan_sysfs_exit((*pos), n, head, size < 0); + if (ret) + return ret; if (size) break; } @@ -232,7 +246,10 @@ static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type, if (!*data.buf) return -ENOMEM; - efivar_entry_iter_begin(); + if (efivar_entry_iter_begin()) { + kfree(*data.buf); + return -EINTR; + } size = efi_pstore_sysfs_entry_iter(&data, (struct efivar_entry **)&psi->data); efivar_entry_iter_end(); @@ -347,7 +364,8 @@ static int efi_pstore_erase(enum pstore_type_id type, u64 id, int count, edata.time = time; edata.name = efi_name; - efivar_entry_iter_begin(); + if (efivar_entry_iter_begin()) + return -EINTR; found = __efivar_entry_iter(efi_pstore_erase_func, &efivar_sysfs_list, &edata, &entry); if (found && !entry->scanning) { diff --git a/drivers/firmware/efi/efivars.c b/drivers/firmware/efi/efivars.c index 116b244dee682..3e626fd9bd4e1 100644 --- a/drivers/firmware/efi/efivars.c +++ b/drivers/firmware/efi/efivars.c @@ -510,7 +510,8 @@ static ssize_t efivar_delete(struct file *filp, struct kobject *kobj, vendor = del_var->VendorGuid; } - efivar_entry_iter_begin(); + if (efivar_entry_iter_begin()) + return -EINTR; entry = efivar_entry_find(name, vendor, &efivar_sysfs_list, true); if (!entry) err = -EINVAL; @@ -575,7 +576,10 @@ efivar_create_sysfs_entry(struct efivar_entry *new_var) return ret; kobject_uevent(&new_var->kobj, KOBJ_ADD); - efivar_entry_add(new_var, &efivar_sysfs_list); + if (efivar_entry_add(new_var, &efivar_sysfs_list)) { + efivar_unregister(new_var); + return -EINTR; + } return 0; } @@ -690,7 +694,10 @@ static int efivars_sysfs_callback(efi_char16_t *name, efi_guid_t vendor, static int efivar_sysfs_destroy(struct efivar_entry *entry, void *data) { - efivar_entry_remove(entry); + int err = efivar_entry_remove(entry); + + if (err) + return err; efivar_unregister(entry); return 0; } @@ -698,7 +705,14 @@ static int efivar_sysfs_destroy(struct efivar_entry *entry, void *data) static void efivars_sysfs_exit(void) { /* Remove all entries and destroy */ - __efivar_entry_iter(efivar_sysfs_destroy, &efivar_sysfs_list, NULL, NULL); + int err; + + err = __efivar_entry_iter(efivar_sysfs_destroy, &efivar_sysfs_list, + NULL, NULL); + if (err) { + pr_err("efivars: Failed to destroy sysfs entries\n"); + return; + } if (efivars_new_var) sysfs_remove_bin_file(&efivars_kset->kobj, efivars_new_var); diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c index d0d807e1287e2..9336ffdf6e2c6 100644 --- a/drivers/firmware/efi/vars.c +++ b/drivers/firmware/efi/vars.c @@ -43,7 +43,7 @@ static struct efivars *__efivars; * 2) ->ops calls * 3) (un)registration of __efivars */ -static DEFINE_SPINLOCK(efivars_lock); +static DEFINE_SEMAPHORE(efivars_lock); static bool efivar_wq_enabled = true; DECLARE_WORK(efivar_work, NULL); @@ -442,7 +442,10 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *), return -ENOMEM; } - spin_lock_irq(&efivars_lock); + if (down_interruptible(&efivars_lock)) { + err = -EINTR; + goto free; + } /* * Per EFI spec, the maximum storage allocated for both @@ -458,7 +461,7 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *), switch (status) { case EFI_SUCCESS: if (duplicates) - spin_unlock_irq(&efivars_lock); + up(&efivars_lock); variable_name_size = var_name_strnsize(variable_name, variable_name_size); @@ -484,8 +487,12 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *), status = EFI_NOT_FOUND; } - if (duplicates) - spin_lock_irq(&efivars_lock); + if (duplicates) { + if (down_interruptible(&efivars_lock)) { + err = -EINTR; + goto free; + } + } break; case EFI_NOT_FOUND: @@ -499,8 +506,8 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *), } while (status != EFI_NOT_FOUND); - spin_unlock_irq(&efivars_lock); - + up(&efivars_lock); +free: kfree(variable_name); return err; @@ -511,24 +518,34 @@ EXPORT_SYMBOL_GPL(efivar_init); * efivar_entry_add - add entry to variable list * @entry: entry to add to list * @head: list head + * + * Returns 0 on success, or a kernel error code on failure. */ -void efivar_entry_add(struct efivar_entry *entry, struct list_head *head) +int efivar_entry_add(struct efivar_entry *entry, struct list_head *head) { - spin_lock_irq(&efivars_lock); + if (down_interruptible(&efivars_lock)) + return -EINTR; list_add(&entry->list, head); - spin_unlock_irq(&efivars_lock); + up(&efivars_lock); + + return 0; } EXPORT_SYMBOL_GPL(efivar_entry_add); /** * efivar_entry_remove - remove entry from variable list * @entry: entry to remove from list + * + * Returns 0 on success, or a kernel error code on failure. */ -void efivar_entry_remove(struct efivar_entry *entry) +int efivar_entry_remove(struct efivar_entry *entry) { - spin_lock_irq(&efivars_lock); + if (down_interruptible(&efivars_lock)) + return -EINTR; list_del(&entry->list); - spin_unlock_irq(&efivars_lock); + up(&efivars_lock); + + return 0; } EXPORT_SYMBOL_GPL(efivar_entry_remove); @@ -545,10 +562,8 @@ EXPORT_SYMBOL_GPL(efivar_entry_remove); */ static void efivar_entry_list_del_unlock(struct efivar_entry *entry) { - lockdep_assert_held(&efivars_lock); - list_del(&entry->list); - spin_unlock_irq(&efivars_lock); + up(&efivars_lock); } /** @@ -571,8 +586,6 @@ int __efivar_entry_delete(struct efivar_entry *entry) const struct efivar_operations *ops = __efivars->ops; efi_status_t status; - lockdep_assert_held(&efivars_lock); - status = ops->set_variable(entry->var.VariableName, &entry->var.VendorGuid, 0, 0, NULL); @@ -589,20 +602,22 @@ EXPORT_SYMBOL_GPL(__efivar_entry_delete); * variable list. It is the caller's responsibility to free @entry * once we return. * - * Returns 0 on success, or a converted EFI status code if - * set_variable() fails. + * Returns 0 on success, -EINTR if we can't grab the semaphore, + * converted EFI status code if set_variable() fails. */ int efivar_entry_delete(struct efivar_entry *entry) { const struct efivar_operations *ops = __efivars->ops; efi_status_t status; - spin_lock_irq(&efivars_lock); + if (down_interruptible(&efivars_lock)) + return -EINTR; + status = ops->set_variable(entry->var.VariableName, &entry->var.VendorGuid, 0, 0, NULL); if (!(status == EFI_SUCCESS || status == EFI_NOT_FOUND)) { - spin_unlock_irq(&efivars_lock); + up(&efivars_lock); return efi_status_to_err(status); } @@ -628,9 +643,9 @@ EXPORT_SYMBOL_GPL(efivar_entry_delete); * If @head is not NULL a lookup is performed to determine whether * the entry is already on the list. * - * Returns 0 on success, -EEXIST if a lookup is performed and the entry - * already exists on the list, or a converted EFI status code if - * set_variable() fails. + * Returns 0 on success, -EINTR if we can't grab the semaphore, + * -EEXIST if a lookup is performed and the entry already exists on + * the list, or a converted EFI status code if set_variable() fails. */ int efivar_entry_set(struct efivar_entry *entry, u32 attributes, unsigned long size, void *data, struct list_head *head) @@ -640,10 +655,10 @@ int efivar_entry_set(struct efivar_entry *entry, u32 attributes, efi_char16_t *name = entry->var.VariableName; efi_guid_t vendor = entry->var.VendorGuid; - spin_lock_irq(&efivars_lock); - + if (down_interruptible(&efivars_lock)) + return -EINTR; if (head && efivar_entry_find(name, vendor, head, false)) { - spin_unlock_irq(&efivars_lock); + up(&efivars_lock); return -EEXIST; } @@ -652,7 +667,7 @@ int efivar_entry_set(struct efivar_entry *entry, u32 attributes, status = ops->set_variable(name, &vendor, attributes, size, data); - spin_unlock_irq(&efivars_lock); + up(&efivars_lock); return efi_status_to_err(status); @@ -673,23 +688,22 @@ efivar_entry_set_nonblocking(efi_char16_t *name, efi_guid_t vendor, u32 attributes, unsigned long size, void *data) { const struct efivar_operations *ops = __efivars->ops; - unsigned long flags; efi_status_t status; - if (!spin_trylock_irqsave(&efivars_lock, flags)) + if (down_trylock(&efivars_lock)) return -EBUSY; status = check_var_size_nonblocking(attributes, size + ucs2_strsize(name, 1024)); if (status != EFI_SUCCESS) { - spin_unlock_irqrestore(&efivars_lock, flags); + up(&efivars_lock); return -ENOSPC; } status = ops->set_variable_nonblocking(name, &vendor, attributes, size, data); - spin_unlock_irqrestore(&efivars_lock, flags); + up(&efivars_lock); return efi_status_to_err(status); } @@ -714,7 +728,6 @@ int efivar_entry_set_safe(efi_char16_t *name, efi_guid_t vendor, u32 attributes, bool block, unsigned long size, void *data) { const struct efivar_operations *ops = __efivars->ops; - unsigned long flags; efi_status_t status; if (!ops->query_variable_store) @@ -735,21 +748,22 @@ int efivar_entry_set_safe(efi_char16_t *name, efi_guid_t vendor, u32 attributes, size, data); if (!block) { - if (!spin_trylock_irqsave(&efivars_lock, flags)) + if (down_trylock(&efivars_lock)) return -EBUSY; } else { - spin_lock_irqsave(&efivars_lock, flags); + if (down_interruptible(&efivars_lock)) + return -EINTR; } status = check_var_size(attributes, size + ucs2_strsize(name, 1024)); if (status != EFI_SUCCESS) { - spin_unlock_irqrestore(&efivars_lock, flags); + up(&efivars_lock); return -ENOSPC; } status = ops->set_variable(name, &vendor, attributes, size, data); - spin_unlock_irqrestore(&efivars_lock, flags); + up(&efivars_lock); return efi_status_to_err(status); } @@ -779,8 +793,6 @@ struct efivar_entry *efivar_entry_find(efi_char16_t *name, efi_guid_t guid, int strsize1, strsize2; bool found = false; - lockdep_assert_held(&efivars_lock); - list_for_each_entry_safe(entry, n, head, list) { strsize1 = ucs2_strsize(name, 1024); strsize2 = ucs2_strsize(entry->var.VariableName, 1024); @@ -822,10 +834,11 @@ int efivar_entry_size(struct efivar_entry *entry, unsigned long *size) *size = 0; - spin_lock_irq(&efivars_lock); + if (down_interruptible(&efivars_lock)) + return -EINTR; status = ops->get_variable(entry->var.VariableName, &entry->var.VendorGuid, NULL, size, NULL); - spin_unlock_irq(&efivars_lock); + up(&efivars_lock); if (status != EFI_BUFFER_TOO_SMALL) return efi_status_to_err(status); @@ -851,8 +864,6 @@ int __efivar_entry_get(struct efivar_entry *entry, u32 *attributes, const struct efivar_operations *ops = __efivars->ops; efi_status_t status; - lockdep_assert_held(&efivars_lock); - status = ops->get_variable(entry->var.VariableName, &entry->var.VendorGuid, attributes, size, data); @@ -874,11 +885,12 @@ int efivar_entry_get(struct efivar_entry *entry, u32 *attributes, const struct efivar_operations *ops = __efivars->ops; efi_status_t status; - spin_lock_irq(&efivars_lock); + if (down_interruptible(&efivars_lock)) + return -EINTR; status = ops->get_variable(entry->var.VariableName, &entry->var.VendorGuid, attributes, size, data); - spin_unlock_irq(&efivars_lock); + up(&efivars_lock); return efi_status_to_err(status); } @@ -925,7 +937,8 @@ int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes, * set_variable call, and removal of the variable from the efivars * list (in the case of an authenticated delete). */ - spin_lock_irq(&efivars_lock); + if (down_interruptible(&efivars_lock)) + return -EINTR; /* * Ensure that the available space hasn't shrunk below the safe level @@ -965,7 +978,7 @@ int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes, if (status == EFI_NOT_FOUND) efivar_entry_list_del_unlock(entry); else - spin_unlock_irq(&efivars_lock); + up(&efivars_lock); if (status && status != EFI_BUFFER_TOO_SMALL) return efi_status_to_err(status); @@ -973,7 +986,7 @@ int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes, return 0; out: - spin_unlock_irq(&efivars_lock); + up(&efivars_lock); return err; } @@ -986,9 +999,9 @@ EXPORT_SYMBOL_GPL(efivar_entry_set_get_size); * efivar_entry_iter_end() is called. This function is usually used in * conjunction with __efivar_entry_iter() or efivar_entry_iter(). */ -void efivar_entry_iter_begin(void) +int efivar_entry_iter_begin(void) { - spin_lock_irq(&efivars_lock); + return down_interruptible(&efivars_lock); } EXPORT_SYMBOL_GPL(efivar_entry_iter_begin); @@ -999,7 +1012,7 @@ EXPORT_SYMBOL_GPL(efivar_entry_iter_begin); */ void efivar_entry_iter_end(void) { - spin_unlock_irq(&efivars_lock); + up(&efivars_lock); } EXPORT_SYMBOL_GPL(efivar_entry_iter_end); @@ -1075,7 +1088,9 @@ int efivar_entry_iter(int (*func)(struct efivar_entry *, void *), { int err = 0; - efivar_entry_iter_begin(); + err = efivar_entry_iter_begin(); + if (err) + return err; err = __efivar_entry_iter(func, head, data, NULL); efivar_entry_iter_end(); @@ -1120,12 +1135,17 @@ int efivars_register(struct efivars *efivars, const struct efivar_operations *ops, struct kobject *kobject) { - spin_lock_irq(&efivars_lock); + if (down_interruptible(&efivars_lock)) + return -EINTR; + efivars->ops = ops; efivars->kobject = kobject; __efivars = efivars; - spin_unlock_irq(&efivars_lock); + + pr_info("Registered efivars operations\n"); + + up(&efivars_lock); return 0; } @@ -1142,7 +1162,9 @@ int efivars_unregister(struct efivars *efivars) { int rv; - spin_lock_irq(&efivars_lock); + if (down_interruptible(&efivars_lock)) + return -EINTR; + if (!__efivars) { printk(KERN_ERR "efivars not registered\n"); rv = -EINVAL; @@ -1154,11 +1176,12 @@ int efivars_unregister(struct efivars *efivars) goto out; } + pr_info("Unregistered efivars operations\n"); __efivars = NULL; rv = 0; out: - spin_unlock_irq(&efivars_lock); + up(&efivars_lock); return rv; } EXPORT_SYMBOL_GPL(efivars_unregister); diff --git a/fs/efivarfs/inode.c b/fs/efivarfs/inode.c index 1d73fc6dba13b..cbb50cadcffc0 100644 --- a/fs/efivarfs/inode.c +++ b/fs/efivarfs/inode.c @@ -105,7 +105,10 @@ static int efivarfs_create(struct inode *dir, struct dentry *dentry, inode->i_private = var; - efivar_entry_add(var, &efivarfs_list); + err = efivar_entry_add(var, &efivarfs_list); + if (err) + goto out; + d_instantiate(dentry, inode); dget(dentry); out: diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c index 688ccc16b702d..01e3d6e53944b 100644 --- a/fs/efivarfs/super.c +++ b/fs/efivarfs/super.c @@ -161,7 +161,9 @@ static int efivarfs_callback(efi_char16_t *name16, efi_guid_t vendor, kfree(name); efivar_entry_size(entry, &size); - efivar_entry_add(entry, &efivarfs_list); + err = efivar_entry_add(entry, &efivarfs_list); + if (err) + goto fail_inode; inode_lock(inode); inode->i_private = entry; @@ -182,7 +184,10 @@ static int efivarfs_callback(efi_char16_t *name16, efi_guid_t vendor, static int efivarfs_destroy(struct efivar_entry *entry, void *data) { - efivar_entry_remove(entry); + int err = efivar_entry_remove(entry); + + if (err) + return err; kfree(entry); return 0; } diff --git a/include/linux/efi.h b/include/linux/efi.h index deecb29027157..4d6da7b66c19d 100644 --- a/include/linux/efi.h +++ b/include/linux/efi.h @@ -1297,8 +1297,8 @@ struct kobject *efivars_kobject(void); int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *), void *data, bool duplicates, struct list_head *head); -void efivar_entry_add(struct efivar_entry *entry, struct list_head *head); -void efivar_entry_remove(struct efivar_entry *entry); +int 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); int efivar_entry_delete(struct efivar_entry *entry); @@ -1315,7 +1315,7 @@ int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes, int efivar_entry_set_safe(efi_char16_t *name, efi_guid_t vendor, u32 attributes, bool block, unsigned long size, void *data); -void efivar_entry_iter_begin(void); +int efivar_entry_iter_begin(void); void efivar_entry_iter_end(void); int __efivar_entry_iter(int (*func)(struct efivar_entry *, void *),