(Cc'ing Ard since he has recently been in this area)
On Fri, 18 Dec, at 11:29:50AM, sylvain.chouleur@xxxxxxxxx wrote:
> From: Sylvain Chouleur <sylvain.chouleur@xxxxxxxxx>
>
> All efivars operations are protected by a spinlock which prevents
> interruptions and preemption. This is too restricted, we just need a
> lock preventing concurency.
> 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.
>
> This patch also remove the efivars lock to use a single lock for the
> whole vars.c file. The goal of this lock is to protect concurrent calls
> to efi variable services, registering and unregistering.
> This allows to register new efivars operations without having
> in-progress call.
>
> Signed-off-by: Sylvain Chouleur <sylvain.chouleur@xxxxxxxxx>
> ---
> drivers/firmware/efi/efi-pstore.c | 34 +++++++---
> drivers/firmware/efi/efivars.c | 10 ++-
> drivers/firmware/efi/vars.c | 130 +++++++++++++++++++++++++-------------
> fs/efivarfs/inode.c | 5 +-
> fs/efivarfs/super.c | 8 ++-
> include/linux/efi.h | 12 +---
> 6 files changed, 130 insertions(+), 69 deletions(-)
This needs splitting into more than 1 patch.
You need separate patches to,
- Split out __efivars->lock into a file local lock
- Convert the lock to a semaphore
- Print a message when efi_operations are registered
Further comments below.
> diff --git a/drivers/firmware/efi/efivars.c b/drivers/firmware/efi/efivars.c
> index 756eca8c4cf8..3998373d92ef 100644
> --- a/drivers/firmware/efi/efivars.c
> +++ b/drivers/firmware/efi/efivars.c
> @@ -509,7 +509,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;
> @@ -582,7 +583,8 @@ 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))
> + return -EINTR;
>
> return 0;
> }
This looks like it's missing a call to efivar_unregister() in the
-EINTR case.
> @@ -697,7 +699,9 @@ 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;
> }
You now need to return early from efivars_sysfs_exit() if you hit the
error path in efivar_sysfs_destroy().
> diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
> index 70a0fb10517f..8a44351211e5 100644
> --- a/drivers/firmware/efi/vars.c
> +++ b/drivers/firmware/efi/vars.c
> @@ -37,6 +37,13 @@
> /* Private pointer to registered efivars */
> static struct efivars *__efivars;
>
> +/*
> + * ->lock protects two things:
> + * 1) efivarfs_list and efivars_sysfs_list
> + * 2) ->ops calls
> + */
> +DEFINE_SEMAPHORE(efivars_lock);
> +
Now it also protects registration of __efivars, so that needs to be
documented too.
> static bool efivar_wq_enabled = true;
> DECLARE_WORK(efivar_work, NULL);
> EXPORT_SYMBOL_GPL(efivar_work);
> @@ -376,7 +383,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
> @@ -392,7 +402,7 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
> switch (status) {
> case EFI_SUCCESS:
> if (!atomic)
> - spin_unlock_irq(&__efivars->lock);
> + up(&efivars_lock);
>
> variable_name_size = var_name_strnsize(variable_name,
> variable_name_size);
> @@ -410,7 +420,10 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
> dup_variable_bug(variable_name, &vendor_guid,
> variable_name_size);
> if (!atomic)
> - spin_lock_irq(&__efivars->lock);
> + if (down_interruptible(&efivars_lock)) {
> + err = -EINTR;
> + goto free;
> + }
>
> status = EFI_NOT_FOUND;
> break;
Add braces to the if(!atomic) clause please to help with readability.
> @@ -421,7 +434,10 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
> status = EFI_NOT_FOUND;
>
> if (!atomic)
> - spin_lock_irq(&__efivars->lock);
> + if (down_interruptible(&efivars_lock)) {
> + err = -EINTR;
> + goto free;
> + }
>
> break;
> case EFI_NOT_FOUND:
Ditto.
> @@ -533,12 +559,14 @@ 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);
> }
>
Please update the documentation for this function to note that we
return -EINTR if we can't grab the semaphore.
> @@ -576,10 +604,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;
> }
>
> @@ -588,7 +616,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);
>
Function documentation for the return values needs updating here too.
> @@ -1055,12 +1087,16 @@ int efivars_register(struct efivars *efivars,
> const struct efivar_operations *ops,
> struct kobject *kobject)
> {
> - spin_lock_init(&efivars->lock);
> + if (down_trylock(&efivars_lock))
> + return -EBUSY;
> +
Is this correct? I would have assumed that you'd want to return -EINTR
here, not -EBUSY since if an EFI variable operation is currently
running we should spin waiting for the semaphore to be released.
> efivars->ops = ops;
> efivars->kobject = kobject;
>
> __efivars = efivars;
>
> + up(&efivars_lock);
> +
> return 0;
> }
> EXPORT_SYMBOL_GPL(efivars_register);
> @@ -1076,6 +1112,9 @@ int efivars_unregister(struct efivars *efivars)
> {
> int rv;
>
> + if (down_trylock(&efivars_lock))
> + return -EBUSY;
> +
Same logic applies in the unregister case.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
