From e59310adf5eebce108f78b6c47bb330aae2e1666 Mon Sep 17 00:00:00 2001 From: Seiji Aguchi Date: Fri, 11 Jan 2013 18:10:05 +0000 Subject: efi_pstore: Avoid deadlock in non-blocking paths [Issue] There is a scenario which efi_pstore may hang up: - cpuA grabs efivars->lock - cpuB panics and calls smp_send_stop - smp_send_stop sends IRQ to cpuA - after 1 second, cpuB gives up on cpuA and sends an NMI instead - cpuA is now in an NMI handler while still holding efivars->lock - cpuB is deadlocked This case may happen if a firmware has a bug and cpuA is stuck talking with it. [Solution] This patch changes a spin_lock to a spin_trylock in non-blocking paths. and if the spin_lock has already taken by another cpu, it returns without accessing to a firmware to avoid the deadlock. Signed-off-by: Seiji Aguchi Acked-by: Don Zickus Signed-off-by: Tony Luck --- drivers/firmware/efivars.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) (limited to 'drivers') diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c index 7b1c37497c9a..ef5070d86f88 100644 --- a/drivers/firmware/efivars.c +++ b/drivers/firmware/efivars.c @@ -1209,7 +1209,16 @@ static int efi_pstore_write(enum pstore_type_id type, u64 storage_space, remaining_space, max_variable_size; efi_status_t status = EFI_NOT_FOUND; - spin_lock(&efivars->lock); + if (pstore_cannot_block_path(reason)) { + /* + * If the lock is taken by another cpu in non-blocking path, + * this driver returns without entering firmware to avoid + * hanging up. + */ + if (!spin_trylock(&efivars->lock)) + return -EBUSY; + } else + spin_lock(&efivars->lock); /* * Check if there is a space enough to log. -- cgit v1.2.3 From 81fa4e581d9283f7992a0d8c534bb141eb840a14 Mon Sep 17 00:00:00 2001 From: Seiji Aguchi Date: Tue, 12 Feb 2013 12:59:07 -0800 Subject: efivars: Disable external interrupt while holding efivars->lock [Problem] There is a scenario which efi_pstore fails to log messages in a panic case. - CPUA holds an efi_var->lock in either efivarfs parts or efi_pstore with interrupt enabled. - CPUB panics and sends IPI to CPUA in smp_send_stop(). - CPUA stops with holding the lock. - CPUB kicks efi_pstore_write() via kmsg_dump(KSMG_DUMP_PANIC) but it returns without logging messages. [Patch Description] This patch disables an external interruption while holding efivars->lock as follows. In efi_pstore_write() and get_var_data(), spin_lock/spin_unlock is replaced by spin_lock_irqsave/spin_unlock_irqrestore because they may be called in an interrupt context. In other functions, they are replaced by spin_lock_irq/spin_unlock_irq. because they are all called from a process context. By applying this patch, we can avoid the problem above with a following senario. - CPUA holds an efi_var->lock with interrupt disabled. - CPUB panics and sends IPI to CPUA in smp_send_stop(). - CPUA receives the IPI after releasing the lock because it is disabling interrupt while holding the lock. - CPUB waits for one sec until CPUA releases the lock. - CPUB kicks efi_pstore_write() via kmsg_dump(KSMG_DUMP_PANIC) And it can hold the lock successfully. Signed-off-by: Seiji Aguchi Acked-by: Mike Waychison Acked-by: Matt Fleming Signed-off-by: Tony Luck --- drivers/firmware/efivars.c | 86 ++++++++++++++++++++++++---------------------- 1 file changed, 44 insertions(+), 42 deletions(-) (limited to 'drivers') diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c index ef5070d86f88..a64fb7bda365 100644 --- a/drivers/firmware/efivars.c +++ b/drivers/firmware/efivars.c @@ -405,10 +405,11 @@ static efi_status_t get_var_data(struct efivars *efivars, struct efi_variable *var) { efi_status_t status; + unsigned long flags; - spin_lock(&efivars->lock); + spin_lock_irqsave(&efivars->lock, flags); status = get_var_data_locked(efivars, var); - spin_unlock(&efivars->lock); + spin_unlock_irqrestore(&efivars->lock, flags); if (status != EFI_SUCCESS) { printk(KERN_WARNING "efivars: get_variable() failed 0x%lx!\n", @@ -537,14 +538,14 @@ efivar_store_raw(struct efivar_entry *entry, const char *buf, size_t count) return -EINVAL; } - spin_lock(&efivars->lock); + spin_lock_irq(&efivars->lock); status = efivars->ops->set_variable(new_var->VariableName, &new_var->VendorGuid, new_var->Attributes, new_var->DataSize, new_var->Data); - spin_unlock(&efivars->lock); + spin_unlock_irq(&efivars->lock); if (status != EFI_SUCCESS) { printk(KERN_WARNING "efivars: set_variable() failed: status=%lx\n", @@ -713,7 +714,7 @@ static ssize_t efivarfs_file_write(struct file *file, * amounts of memory. Pick a default size of 64K if * QueryVariableInfo() isn't supported by the firmware. */ - spin_lock(&efivars->lock); + spin_lock_irq(&efivars->lock); if (!efivars->ops->query_variable_info) status = EFI_UNSUPPORTED; @@ -723,7 +724,7 @@ static ssize_t efivarfs_file_write(struct file *file, &remaining_size, &max_size); } - spin_unlock(&efivars->lock); + spin_unlock_irq(&efivars->lock); if (status != EFI_SUCCESS) { if (status != EFI_UNSUPPORTED) @@ -754,7 +755,7 @@ static ssize_t efivarfs_file_write(struct file *file, * set_variable call, and removal of the variable from the efivars * list (in the case of an authenticated delete). */ - spin_lock(&efivars->lock); + spin_lock_irq(&efivars->lock); status = efivars->ops->set_variable(var->var.VariableName, &var->var.VendorGuid, @@ -762,7 +763,7 @@ static ssize_t efivarfs_file_write(struct file *file, data); if (status != EFI_SUCCESS) { - spin_unlock(&efivars->lock); + spin_unlock_irq(&efivars->lock); kfree(data); return efi_status_to_err(status); @@ -783,20 +784,20 @@ static ssize_t efivarfs_file_write(struct file *file, NULL); if (status == EFI_BUFFER_TOO_SMALL) { - spin_unlock(&efivars->lock); + spin_unlock_irq(&efivars->lock); mutex_lock(&inode->i_mutex); i_size_write(inode, newdatasize + sizeof(attributes)); mutex_unlock(&inode->i_mutex); } else if (status == EFI_NOT_FOUND) { list_del(&var->list); - spin_unlock(&efivars->lock); + spin_unlock_irq(&efivars->lock); efivar_unregister(var); drop_nlink(inode); dput(file->f_dentry); } else { - spin_unlock(&efivars->lock); + spin_unlock_irq(&efivars->lock); pr_warn("efivarfs: inconsistent EFI variable implementation? " "status = %lx\n", status); } @@ -818,11 +819,11 @@ static ssize_t efivarfs_file_read(struct file *file, char __user *userbuf, void *data; ssize_t size = 0; - spin_lock(&efivars->lock); + spin_lock_irq(&efivars->lock); status = efivars->ops->get_variable(var->var.VariableName, &var->var.VendorGuid, &attributes, &datasize, NULL); - spin_unlock(&efivars->lock); + spin_unlock_irq(&efivars->lock); if (status != EFI_BUFFER_TOO_SMALL) return efi_status_to_err(status); @@ -832,12 +833,12 @@ static ssize_t efivarfs_file_read(struct file *file, char __user *userbuf, if (!data) return -ENOMEM; - spin_lock(&efivars->lock); + spin_lock_irq(&efivars->lock); status = efivars->ops->get_variable(var->var.VariableName, &var->var.VendorGuid, &attributes, &datasize, (data + sizeof(attributes))); - spin_unlock(&efivars->lock); + spin_unlock_irq(&efivars->lock); if (status != EFI_SUCCESS) { size = efi_status_to_err(status); @@ -965,9 +966,9 @@ static int efivarfs_create(struct inode *dir, struct dentry *dentry, goto out; kobject_uevent(&var->kobj, KOBJ_ADD); - spin_lock(&efivars->lock); + spin_lock_irq(&efivars->lock); list_add(&var->list, &efivars->list); - spin_unlock(&efivars->lock); + spin_unlock_irq(&efivars->lock); d_instantiate(dentry, inode); dget(dentry); out: @@ -984,7 +985,7 @@ static int efivarfs_unlink(struct inode *dir, struct dentry *dentry) struct efivars *efivars = var->efivars; efi_status_t status; - spin_lock(&efivars->lock); + spin_lock_irq(&efivars->lock); status = efivars->ops->set_variable(var->var.VariableName, &var->var.VendorGuid, @@ -992,14 +993,14 @@ static int efivarfs_unlink(struct inode *dir, struct dentry *dentry) if (status == EFI_SUCCESS || status == EFI_NOT_FOUND) { list_del(&var->list); - spin_unlock(&efivars->lock); + spin_unlock_irq(&efivars->lock); efivar_unregister(var); drop_nlink(dir); dput(dentry); return 0; } - spin_unlock(&efivars->lock); + spin_unlock_irq(&efivars->lock); return -EINVAL; }; @@ -1065,13 +1066,13 @@ static int efivarfs_fill_super(struct super_block *sb, void *data, int silent) /* copied by the above to local storage in the dentry. */ kfree(name); - spin_lock(&efivars->lock); + spin_lock_irq(&efivars->lock); efivars->ops->get_variable(entry->var.VariableName, &entry->var.VendorGuid, &entry->var.Attributes, &size, NULL); - spin_unlock(&efivars->lock); + spin_unlock_irq(&efivars->lock); mutex_lock(&inode->i_mutex); inode->i_private = entry; @@ -1122,7 +1123,7 @@ static int efi_pstore_open(struct pstore_info *psi) { struct efivars *efivars = psi->data; - spin_lock(&efivars->lock); + spin_lock_irq(&efivars->lock); efivars->walk_entry = list_first_entry(&efivars->list, struct efivar_entry, list); return 0; @@ -1132,7 +1133,7 @@ static int efi_pstore_close(struct pstore_info *psi) { struct efivars *efivars = psi->data; - spin_unlock(&efivars->lock); + spin_unlock_irq(&efivars->lock); return 0; } @@ -1208,6 +1209,7 @@ static int efi_pstore_write(enum pstore_type_id type, int i, ret = 0; u64 storage_space, remaining_space, max_variable_size; efi_status_t status = EFI_NOT_FOUND; + unsigned long flags; if (pstore_cannot_block_path(reason)) { /* @@ -1215,10 +1217,10 @@ static int efi_pstore_write(enum pstore_type_id type, * this driver returns without entering firmware to avoid * hanging up. */ - if (!spin_trylock(&efivars->lock)) + if (!spin_trylock_irqsave(&efivars->lock, flags)) return -EBUSY; } else - spin_lock(&efivars->lock); + spin_lock_irqsave(&efivars->lock, flags); /* * Check if there is a space enough to log. @@ -1230,7 +1232,7 @@ static int efi_pstore_write(enum pstore_type_id type, &remaining_space, &max_variable_size); if (status || remaining_space < size + DUMP_NAME_LEN * 2) { - spin_unlock(&efivars->lock); + spin_unlock_irqrestore(&efivars->lock, flags); *id = part; return -ENOSPC; } @@ -1244,7 +1246,7 @@ static int efi_pstore_write(enum pstore_type_id type, efivars->ops->set_variable(efi_name, &vendor, PSTORE_EFI_ATTRIBUTES, size, psi->buf); - spin_unlock(&efivars->lock); + spin_unlock_irqrestore(&efivars->lock, flags); if (size) ret = efivar_create_sysfs_entry(efivars, @@ -1271,7 +1273,7 @@ static int efi_pstore_erase(enum pstore_type_id type, u64 id, int count, sprintf(name, "dump-type%u-%u-%d-%lu", type, (unsigned int)id, count, time.tv_sec); - spin_lock(&efivars->lock); + spin_lock_irq(&efivars->lock); for (i = 0; i < DUMP_NAME_LEN; i++) efi_name[i] = name[i]; @@ -1315,7 +1317,7 @@ static int efi_pstore_erase(enum pstore_type_id type, u64 id, int count, if (found) list_del(&found->list); - spin_unlock(&efivars->lock); + spin_unlock_irq(&efivars->lock); if (found) efivar_unregister(found); @@ -1385,7 +1387,7 @@ static ssize_t efivar_create(struct file *filp, struct kobject *kobj, return -EINVAL; } - spin_lock(&efivars->lock); + spin_lock_irq(&efivars->lock); /* * Does this variable already exist? @@ -1403,7 +1405,7 @@ static ssize_t efivar_create(struct file *filp, struct kobject *kobj, } } if (found) { - spin_unlock(&efivars->lock); + spin_unlock_irq(&efivars->lock); return -EINVAL; } @@ -1417,10 +1419,10 @@ static ssize_t efivar_create(struct file *filp, struct kobject *kobj, if (status != EFI_SUCCESS) { printk(KERN_WARNING "efivars: set_variable() failed: status=%lx\n", status); - spin_unlock(&efivars->lock); + spin_unlock_irq(&efivars->lock); return -EIO; } - spin_unlock(&efivars->lock); + spin_unlock_irq(&efivars->lock); /* Create the entry in sysfs. Locking is not required here */ status = efivar_create_sysfs_entry(efivars, @@ -1448,7 +1450,7 @@ static ssize_t efivar_delete(struct file *filp, struct kobject *kobj, if (!capable(CAP_SYS_ADMIN)) return -EACCES; - spin_lock(&efivars->lock); + spin_lock_irq(&efivars->lock); /* * Does this variable already exist? @@ -1466,7 +1468,7 @@ static ssize_t efivar_delete(struct file *filp, struct kobject *kobj, } } if (!found) { - spin_unlock(&efivars->lock); + spin_unlock_irq(&efivars->lock); return -EINVAL; } /* force the Attributes/DataSize to 0 to ensure deletion */ @@ -1482,12 +1484,12 @@ static ssize_t efivar_delete(struct file *filp, struct kobject *kobj, if (status != EFI_SUCCESS) { printk(KERN_WARNING "efivars: set_variable() failed: status=%lx\n", status); - spin_unlock(&efivars->lock); + spin_unlock_irq(&efivars->lock); return -EIO; } list_del(&search_efivar->list); /* We need to release this lock before unregistering. */ - spin_unlock(&efivars->lock); + spin_unlock_irq(&efivars->lock); efivar_unregister(search_efivar); /* It's dead Jim.... */ @@ -1602,9 +1604,9 @@ efivar_create_sysfs_entry(struct efivars *efivars, kfree(short_name); short_name = NULL; - spin_lock(&efivars->lock); + spin_lock_irq(&efivars->lock); list_add(&new_efivar->list, &efivars->list); - spin_unlock(&efivars->lock); + spin_unlock_irq(&efivars->lock); return 0; } @@ -1673,9 +1675,9 @@ void unregister_efivars(struct efivars *efivars) struct efivar_entry *entry, *n; list_for_each_entry_safe(entry, n, &efivars->list, list) { - spin_lock(&efivars->lock); + spin_lock_irq(&efivars->lock); list_del(&entry->list); - spin_unlock(&efivars->lock); + spin_unlock_irq(&efivars->lock); efivar_unregister(entry); } if (efivars->new_var) -- cgit v1.2.3 From a93bc0c6e07ed9bac44700280e65e2945d864fd4 Mon Sep 17 00:00:00 2001 From: Seiji Aguchi Date: Tue, 12 Feb 2013 13:04:41 -0800 Subject: efi_pstore: Introducing workqueue updating sysfs [Problem] efi_pstore creates sysfs entries, which enable users to access to NVRAM, in a write callback. If a kernel panic happens in an interrupt context, it may fail because it could sleep due to dynamic memory allocations during creating sysfs entries. [Patch Description] This patch removes sysfs operations from a write callback by introducing a workqueue updating sysfs entries which is scheduled after the write callback is called. Also, the workqueue is kicked in a just oops case. A system will go down in other cases such as panic, clean shutdown and emergency restart. And we don't need to create sysfs entries because there is no chance for users to access to them. efi_pstore will be robust against a kernel panic in an interrupt context with this patch. Signed-off-by: Seiji Aguchi Acked-by: Matt Fleming Signed-off-by: Tony Luck --- drivers/firmware/efivars.c | 85 +++++++++++++++++++++++++++++++++++++++++++--- include/linux/efi.h | 3 +- 2 files changed, 82 insertions(+), 6 deletions(-) (limited to 'drivers') diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c index a64fb7bda365..69225115304d 100644 --- a/drivers/firmware/efivars.c +++ b/drivers/firmware/efivars.c @@ -158,6 +158,13 @@ efivar_create_sysfs_entry(struct efivars *efivars, efi_char16_t *variable_name, efi_guid_t *vendor_guid); +/* + * Prototype for workqueue functions updating sysfs entry + */ + +static void efivar_update_sysfs_entries(struct work_struct *); +static DECLARE_WORK(efivar_work, efivar_update_sysfs_entries); + /* Return the number of unicode characters in data */ static unsigned long utf16_strnlen(efi_char16_t *s, size_t maxlength) @@ -1248,11 +1255,8 @@ static int efi_pstore_write(enum pstore_type_id type, spin_unlock_irqrestore(&efivars->lock, flags); - if (size) - ret = efivar_create_sysfs_entry(efivars, - utf16_strsize(efi_name, - DUMP_NAME_LEN * 2), - efi_name, &vendor); + if (reason == KMSG_DUMP_OOPS) + schedule_work(&efivar_work); *id = part; return ret; @@ -1496,6 +1500,75 @@ static ssize_t efivar_delete(struct file *filp, struct kobject *kobj, return count; } +static bool variable_is_present(efi_char16_t *variable_name, efi_guid_t *vendor) +{ + struct efivar_entry *entry, *n; + struct efivars *efivars = &__efivars; + unsigned long strsize1, strsize2; + bool found = false; + + strsize1 = utf16_strsize(variable_name, 1024); + list_for_each_entry_safe(entry, n, &efivars->list, list) { + strsize2 = utf16_strsize(entry->var.VariableName, 1024); + if (strsize1 == strsize2 && + !memcmp(variable_name, &(entry->var.VariableName), + strsize2) && + !efi_guidcmp(entry->var.VendorGuid, + *vendor)) { + found = true; + break; + } + } + return found; +} + +static void efivar_update_sysfs_entries(struct work_struct *work) +{ + struct efivars *efivars = &__efivars; + efi_guid_t vendor; + efi_char16_t *variable_name; + unsigned long variable_name_size = 1024; + efi_status_t status = EFI_NOT_FOUND; + bool found; + + /* Add new sysfs entries */ + while (1) { + variable_name = kzalloc(variable_name_size, GFP_KERNEL); + if (!variable_name) { + pr_err("efivars: Memory allocation failed.\n"); + return; + } + + spin_lock_irq(&efivars->lock); + found = false; + while (1) { + variable_name_size = 1024; + status = efivars->ops->get_next_variable( + &variable_name_size, + variable_name, + &vendor); + if (status != EFI_SUCCESS) { + break; + } else { + if (!variable_is_present(variable_name, + &vendor)) { + found = true; + break; + } + } + } + spin_unlock_irq(&efivars->lock); + + if (!found) { + kfree(variable_name); + break; + } else + efivar_create_sysfs_entry(efivars, + variable_name_size, + variable_name, &vendor); + } +} + /* * Let's not leave out systab information that snuck into * the efivars driver @@ -1833,6 +1906,8 @@ err_put: static void __exit efivars_exit(void) { + cancel_work_sync(&efivar_work); + if (efi_enabled) { unregister_efivars(&__efivars); kobject_put(efi_kobj); diff --git a/include/linux/efi.h b/include/linux/efi.h index 8b84916dc671..d0c68aee0925 100644 --- a/include/linux/efi.h +++ b/include/linux/efi.h @@ -728,7 +728,8 @@ struct efivars { * 1) ->list - adds, removals, reads, writes * 2) ops.[gs]et_variable() calls. * It must not be held when creating sysfs entries or calling kmalloc. - * ops.get_next_variable() is only called from register_efivars(), + * ops.get_next_variable() is only called from register_efivars() + * or efivar_update_sysfs_entries(), * which is protected by the BKL, so that path is safe. */ spinlock_t lock; -- cgit v1.2.3