* [PATCH 1/3] efi: Determine how much space is used by boot services-only variables @ 2013-04-10 2:41 Matthew Garrett 2013-04-10 2:41 ` [PATCH 2/3] Revert "x86, efivars: firmware bug workarounds should be in platform code" Matthew Garrett ` (4 more replies) 0 siblings, 5 replies; 30+ messages in thread From: Matthew Garrett @ 2013-04-10 2:41 UTC (permalink / raw) To: matt.fleming; +Cc: linux-efi, linux-kernel, x86, Matthew Garrett EFI variables can be flagged as being accessible only within boot services. This makes it awkward for us to figure out how much space they use at runtime. In theory we could figure this out by simply comparing the results from QueryVariableInfo() to the space used by all of our variables, but that fails if the platform doesn't garbage collect on every boot. Thankfully, calling QueryVariableInfo() while still inside boot services gives a more reliable answer. This patch passes that information from the EFI boot stub up to the efivars code, letting us calculate a reasonably accurate value. Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com> --- arch/x86/boot/compressed/eboot.c | 47 +++++++++++++++++++++++++++++++++++ arch/x86/include/asm/efi.h | 10 ++++++++ arch/x86/include/uapi/asm/bootparam.h | 1 + arch/x86/platform/efi/efi.c | 24 ++++++++++++++++++ drivers/firmware/efivars.c | 29 +++++++++++++++++++++ 5 files changed, 111 insertions(+) diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c index c205035..8615f75 100644 --- a/arch/x86/boot/compressed/eboot.c +++ b/arch/x86/boot/compressed/eboot.c @@ -251,6 +251,51 @@ static void find_bits(unsigned long mask, u8 *pos, u8 *size) *size = len; } +static efi_status_t setup_efi_vars(struct boot_params *params) +{ + struct setup_data *data; + struct efi_var_bootdata *efidata; + u64 store_size, remaining_size, var_size; + efi_status_t status; + + if (!sys_table->runtime->query_variable_info) + return EFI_UNSUPPORTED; + + data = (struct setup_data *)(unsigned long)params->hdr.setup_data; + + while (data && data->next) + data = (struct setup_data *)(unsigned long)data->next; + + status = efi_call_phys4(sys_table->runtime->query_variable_info, + EFI_VARIABLE_NON_VOLATILE | + EFI_VARIABLE_BOOTSERVICE_ACCESS | + EFI_VARIABLE_RUNTIME_ACCESS, &store_size, + &remaining_size, &var_size); + + if (status != EFI_SUCCESS) + return status; + + status = efi_call_phys3(sys_table->boottime->allocate_pool, + EFI_LOADER_DATA, sizeof(*efidata), &efidata); + + if (status != EFI_SUCCESS) + return status; + + efidata->data.type = SETUP_EFI_VARS; + efidata->data.len = sizeof(struct efi_var_bootdata) - + sizeof(struct setup_data); + efidata->data.next = 0; + efidata->store_size = store_size; + efidata->remaining_size = remaining_size; + efidata->max_var_size = var_size; + + if (data) + data->next = (unsigned long)efidata; + else + params->hdr.setup_data = (unsigned long)efidata; + +} + static efi_status_t setup_efi_pci(struct boot_params *params) { efi_pci_io_protocol *pci; @@ -1157,6 +1202,8 @@ struct boot_params *efi_main(void *handle, efi_system_table_t *_table, setup_graphics(boot_params); + setup_efi_vars(boot_params); + setup_efi_pci(boot_params); status = efi_call_phys3(sys_table->boottime->allocate_pool, diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h index 60c89f3..6c3a154 100644 --- a/arch/x86/include/asm/efi.h +++ b/arch/x86/include/asm/efi.h @@ -93,6 +93,9 @@ extern void __iomem *efi_ioremap(unsigned long addr, unsigned long size, #endif /* CONFIG_X86_32 */ +extern u64 efi_var_store_size; +extern u64 efi_var_remaining_size; +extern u64 efi_var_max_var_size; extern int add_efi_memmap; extern unsigned long x86_efi_facility; extern void efi_set_executable(efi_memory_desc_t *md, bool executable); @@ -102,6 +105,13 @@ extern void efi_call_phys_epilog(void); extern void efi_unmap_memmap(void); extern void efi_memory_uc(u64 addr, unsigned long size); +struct efi_var_bootdata { + struct setup_data data; + u64 store_size; + u64 remaining_size; + u64 max_var_size; +}; + #ifdef CONFIG_EFI static inline bool efi_is_native(void) diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h index c15ddaf..0874424 100644 --- a/arch/x86/include/uapi/asm/bootparam.h +++ b/arch/x86/include/uapi/asm/bootparam.h @@ -6,6 +6,7 @@ #define SETUP_E820_EXT 1 #define SETUP_DTB 2 #define SETUP_PCI 3 +#define SETUP_EFI_VARS 4 /* ram_size flags */ #define RAMDISK_IMAGE_START_MASK 0x07FF diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c index c89c245..659da48 100644 --- a/arch/x86/platform/efi/efi.c +++ b/arch/x86/platform/efi/efi.c @@ -71,6 +71,13 @@ static efi_system_table_t efi_systab __initdata; unsigned long x86_efi_facility; +u64 efi_var_store_size; +EXPORT_SYMBOL(efi_var_store_size); +u64 efi_var_remaining_size; +EXPORT_SYMBOL(efi_var_remaining_size); +u64 efi_var_max_var_size; +EXPORT_SYMBOL(efi_var_max_var_size); + /* * Returns 1 if 'facility' is enabled, 0 otherwise. */ @@ -682,6 +689,9 @@ void __init efi_init(void) char vendor[100] = "unknown"; int i = 0; void *tmp; + struct setup_data *data; + struct efi_var_bootdata *efi_var_data; + u64 pa_data; #ifdef CONFIG_X86_32 if (boot_params.efi_info.efi_systab_hi || @@ -699,6 +709,20 @@ void __init efi_init(void) if (efi_systab_init(efi_phys.systab)) return; + pa_data = boot_params.hdr.setup_data; + while (pa_data) { + data = early_ioremap(pa_data, sizeof(*efi_var_data)); + if (data->type == SETUP_EFI_VARS) { + efi_var_data = (struct efi_var_bootdata *)data; + + efi_var_store_size = efi_var_data->store_size; + efi_var_remaining_size = efi_var_data->remaining_size; + efi_var_max_var_size = efi_var_data->max_var_size; + } + pa_data = data->next; + early_iounmap(data, sizeof(*efi_var_data)); + } + set_bit(EFI_SYSTEM_TABLES, &x86_efi_facility); /* diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c index bf15d81..684a118 100644 --- a/drivers/firmware/efivars.c +++ b/drivers/firmware/efivars.c @@ -85,6 +85,7 @@ #include <linux/ramfs.h> #include <linux/pagemap.h> +#include <asm/efi.h> #include <asm/uaccess.h> #define EFIVARS_VERSION "0.08" @@ -139,6 +140,8 @@ struct efivar_attribute { static struct efivars __efivars; static struct efivar_operations ops; +static u64 boot_var_size; +static u64 boot_used_size; #define PSTORE_EFI_ATTRIBUTES \ (EFI_VARIABLE_NON_VOLATILE | \ @@ -2002,6 +2005,9 @@ int register_efivars(struct efivars *efivars, efi_char16_t *variable_name; unsigned long variable_name_size = 1024; int error = 0; + struct efivar_entry *entry; + struct efi_variable *var; + u64 active_size = 0; variable_name = kzalloc(variable_name_size, GFP_KERNEL); if (!variable_name) { @@ -2074,6 +2080,25 @@ int register_efivars(struct efivars *efivars, } } while (status != EFI_NOT_FOUND); + if (boot_used_size) { + list_for_each_entry(entry, &efivars->list, list) { + var = &entry->var; + status = get_var_data(efivars, var); + + if (status || + !(var->Attributes & EFI_VARIABLE_NON_VOLATILE)) + continue; + + active_size += var->DataSize; + active_size += utf16_strsize(var->VariableName, 1024); + } + + if (active_size < boot_used_size) + boot_var_size = boot_used_size - active_size; + else + printk(KERN_WARNING FW_BUG "efivars: Inconsistent initial sizes\n"); + } + error = create_efivars_bin_attributes(efivars); if (error) unregister_efivars(efivars); @@ -2121,6 +2146,10 @@ efivars_init(void) ops.get_next_variable = efi.get_next_variable; ops.query_variable_store = efi_query_variable_store; +#ifdef CONFIG_X86 + boot_used_size = efi_var_store_size - efi_var_remaining_size; +#endif + error = register_efivars(&__efivars, &ops, efi_kobj); if (error) goto err_put; -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 2/3] Revert "x86, efivars: firmware bug workarounds should be in platform code" 2013-04-10 2:41 [PATCH 1/3] efi: Determine how much space is used by boot services-only variables Matthew Garrett @ 2013-04-10 2:41 ` Matthew Garrett 2013-04-10 2:41 ` [PATCH 3/3] efi: Distinguish between "remaining space" and actually used space Matthew Garrett ` (3 subsequent siblings) 4 siblings, 0 replies; 30+ messages in thread From: Matthew Garrett @ 2013-04-10 2:41 UTC (permalink / raw) To: matt.fleming; +Cc: linux-efi, linux-kernel, x86, Matthew Garrett This reverts commit a6e4d5a03e9e3587e88aba687d8f225f4f04c792. Doing this workaround properly requires us to work within the variable code. Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com> --- arch/x86/platform/efi/efi.c | 25 ------------------------- drivers/firmware/efivars.c | 18 +++++++++++++++--- include/linux/efi.h | 9 +-------- 3 files changed, 16 insertions(+), 36 deletions(-) diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c index 659da48..fdc5074 100644 --- a/arch/x86/platform/efi/efi.c +++ b/arch/x86/platform/efi/efi.c @@ -1023,28 +1023,3 @@ u64 efi_mem_attributes(unsigned long phys_addr) } return 0; } - -/* - * Some firmware has serious problems when using more than 50% of the EFI - * variable store, i.e. it triggers bugs that can brick machines. Ensure that - * we never use more than this safe limit. - * - * Return EFI_SUCCESS if it is safe to write 'size' bytes to the variable - * store. - */ -efi_status_t efi_query_variable_store(u32 attributes, unsigned long size) -{ - efi_status_t status; - u64 storage_size, remaining_size, max_size; - - status = efi.query_variable_info(attributes, &storage_size, - &remaining_size, &max_size); - if (status != EFI_SUCCESS) - return status; - - if (!storage_size || size > remaining_size || size > max_size || - (remaining_size - size) < (storage_size / 2)) - return EFI_OUT_OF_RESOURCES; - - return EFI_SUCCESS; -} diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c index 684a118..60e7d8f 100644 --- a/drivers/firmware/efivars.c +++ b/drivers/firmware/efivars.c @@ -439,12 +439,24 @@ static efi_status_t check_var_size_locked(struct efivars *efivars, u32 attributes, unsigned long size) { + u64 storage_size, remaining_size, max_size; + efi_status_t status; const struct efivar_operations *fops = efivars->ops; - if (!efivars->ops->query_variable_store) + if (!efivars->ops->query_variable_info) return EFI_UNSUPPORTED; - return fops->query_variable_store(attributes, size); + status = fops->query_variable_info(attributes, &storage_size, + &remaining_size, &max_size); + + if (status != EFI_SUCCESS) + return status; + + if (!storage_size || size > remaining_size || size > max_size || + (remaining_size - size) < (storage_size / 2)) + return EFI_OUT_OF_RESOURCES; + + return status; } @@ -2144,7 +2156,7 @@ efivars_init(void) ops.get_variable = efi.get_variable; ops.set_variable = efi.set_variable; ops.get_next_variable = efi.get_next_variable; - ops.query_variable_store = efi_query_variable_store; + ops.query_variable_info = efi.query_variable_info; #ifdef CONFIG_X86 boot_used_size = efi_var_store_size - efi_var_remaining_size; diff --git a/include/linux/efi.h b/include/linux/efi.h index 3d7df3d..9bf2f1f 100644 --- a/include/linux/efi.h +++ b/include/linux/efi.h @@ -333,7 +333,6 @@ typedef efi_status_t efi_query_capsule_caps_t(efi_capsule_header_t **capsules, unsigned long count, u64 *max_size, int *reset_type); -typedef efi_status_t efi_query_variable_store_t(u32 attributes, unsigned long size); /* * EFI Configuration Table and GUID definitions @@ -576,15 +575,9 @@ extern void efi_enter_virtual_mode (void); /* switch EFI to virtual mode, if pos #ifdef CONFIG_X86 extern void efi_late_init(void); extern void efi_free_boot_services(void); -extern efi_status_t efi_query_variable_store(u32 attributes, unsigned long size); #else static inline void efi_late_init(void) {} static inline void efi_free_boot_services(void) {} - -static inline efi_status_t efi_query_variable_store(u32 attributes, unsigned long size) -{ - return EFI_SUCCESS; -} #endif extern void __iomem *efi_lookup_mapped_addr(u64 phys_addr); extern u64 efi_get_iobase (void); @@ -738,7 +731,7 @@ struct efivar_operations { efi_get_variable_t *get_variable; efi_get_next_variable_t *get_next_variable; efi_set_variable_t *set_variable; - efi_query_variable_store_t *query_variable_store; + efi_query_variable_info_t *query_variable_info; }; struct efivars { -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 3/3] efi: Distinguish between "remaining space" and actually used space 2013-04-10 2:41 [PATCH 1/3] efi: Determine how much space is used by boot services-only variables Matthew Garrett 2013-04-10 2:41 ` [PATCH 2/3] Revert "x86, efivars: firmware bug workarounds should be in platform code" Matthew Garrett @ 2013-04-10 2:41 ` Matthew Garrett 2013-04-10 6:02 ` Lingzhu Xiang 2013-04-10 17:46 ` [PATCH V4 1/3] efi: Determine how much space is used by boot services-only variables Matthew Garrett ` (2 subsequent siblings) 4 siblings, 1 reply; 30+ messages in thread From: Matthew Garrett @ 2013-04-10 2:41 UTC (permalink / raw) To: matt.fleming; +Cc: linux-efi, linux-kernel, x86, Matthew Garrett EFI implementations distinguish between space that is actively used by a variable and space that merely hasn't been garbage collected yet. Space that hasn't yet been garbage collected isn't available for use and so isn't counted in the remaining_space field returned by QueryVariableInfo(). Combined with commit 68d9298 this can cause problems. Some implementations don't garbage collect until the remaining space is smaller than the maximum variable size, and as a result check_var_size() will always fail once more than 50% of the variable store has been used even if most of that space is marked as available for garbage collection. The user is unable to create new variables, and deleting variables doesn't increase the remaining space. The problem that 68d9298 was attempting to avoid was one where certain platforms fail if the actively used space is greater than 50% of the available storage space. We should be able to calculate that by simply summing the size of each available variable and subtracting that from the total storage space. With luck this will fix the problem described in https://bugzilla.kernel.org/show_bug.cgi?id=55471 without permitting damage to occur to the machines 68d9298 was attempting to fix. Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com> --- drivers/firmware/efivars.c | 104 +++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 101 insertions(+), 3 deletions(-) diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c index 60e7d8f..f5a4e87 100644 --- a/drivers/firmware/efivars.c +++ b/drivers/firmware/efivars.c @@ -104,6 +104,13 @@ MODULE_VERSION(EFIVARS_VERSION); */ #define GUID_LEN 36 +/* + * There's some additional metadata associated with each + * variable. Intel's reference implementation is 60 bytes - bump that + * to account for potential alignment constraints + */ +#define VAR_METADATA_SIZE 64 + static bool efivars_pstore_disable = IS_ENABLED(CONFIG_EFI_VARS_PSTORE_DEFAULT_DISABLE); @@ -405,6 +412,52 @@ validate_var(struct efi_variable *var, u8 *data, unsigned long len) } static efi_status_t +get_var_size_locked(struct efivars *efivars, struct efi_variable *var) +{ + efi_status_t status; + void *dummy; + + var->DataSize = 0; + status = efivars->ops->get_variable(var->VariableName, + &var->VendorGuid, + &var->Attributes, + &var->DataSize, + var->Data); + + if (status != EFI_BUFFER_TOO_SMALL) + return status; + + dummy = kmalloc(var->DataSize, GFP_ATOMIC); + + status = efivars->ops->get_variable(var->VariableName, + &var->VendorGuid, + &var->Attributes, + &var->DataSize, + dummy); + + kfree(dummy); + + return status; +} + +static efi_status_t +get_var_size(struct efivars *efivars, struct efi_variable *var) +{ + efi_status_t status; + unsigned long flags; + + spin_lock_irqsave(&efivars->lock, flags); + status = get_var_size_locked(efivars, var); + spin_unlock_irqrestore(&efivars->lock, flags); + + if (status != EFI_SUCCESS) { + printk(KERN_WARNING "efivars: Failed to get var size 0x%lx!\n", + status); + } + return status; +} + +static efi_status_t get_var_data_locked(struct efivars *efivars, struct efi_variable *var) { efi_status_t status; @@ -415,6 +468,10 @@ get_var_data_locked(struct efivars *efivars, struct efi_variable *var) &var->Attributes, &var->DataSize, var->Data); + + if (status != EFI_SUCCESS) + var->DataSize = 0; + return status; } @@ -440,8 +497,18 @@ check_var_size_locked(struct efivars *efivars, u32 attributes, unsigned long size) { u64 storage_size, remaining_size, max_size; + struct efivar_entry *entry; + struct efi_variable *var; efi_status_t status; const struct efivar_operations *fops = efivars->ops; + unsigned long active_size = 0; + + /* + * Any writes other than EFI_VARIABLE_NON_VOLATILE will only hit + * RAM, not flash, so ignore them. + */ + if (!(attributes & EFI_VARIABLE_NON_VOLATILE)) + return EFI_SUCCESS; if (!efivars->ops->query_variable_info) return EFI_UNSUPPORTED; @@ -452,8 +519,39 @@ check_var_size_locked(struct efivars *efivars, u32 attributes, if (status != EFI_SUCCESS) return status; - if (!storage_size || size > remaining_size || size > max_size || - (remaining_size - size) < (storage_size / 2)) + list_for_each_entry(entry, &efivars->list, list) { + var = &entry->var; + status = EFI_SUCCESS; + + if (var->DataSize == 0) + status = get_var_size_locked(efivars, var); + + if (status || !(var->Attributes & EFI_VARIABLE_NON_VOLATILE)) + continue; + + active_size += var->DataSize; + active_size += utf16_strsize(var->VariableName, 1024); + active_size += VAR_METADATA_SIZE; + } + + /* + * Add on the size of any boot services-only variables + */ + active_size += boot_var_size; + + /* + * Some firmware implementations refuse to boot if there's insufficient + * space in the variable store. We account for that by refusing the + * write if permitting it would reduce the available space to under + * 50%. However, some firmware won't reclaim variable space until + * after the used (not merely the actively used) space drops below + * a threshold. We can approximate that case with the value calculated + * above. If both the firmware and our calculations indicate that the + * available space would drop below 50%, refuse the write. + */ + if (!storage_size || size > remaining_size || + ((active_size + size + VAR_METADATA_SIZE > storage_size / 2) && + (remaining_size - size - VAR_METADATA_SIZE < storage_size / 2))) return EFI_OUT_OF_RESOURCES; return status; @@ -2095,7 +2193,7 @@ int register_efivars(struct efivars *efivars, if (boot_used_size) { list_for_each_entry(entry, &efivars->list, list) { var = &entry->var; - status = get_var_data(efivars, var); + status = get_var_size(efivars, var); if (status || !(var->Attributes & EFI_VARIABLE_NON_VOLATILE)) -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 3/3] efi: Distinguish between "remaining space" and actually used space 2013-04-10 2:41 ` [PATCH 3/3] efi: Distinguish between "remaining space" and actually used space Matthew Garrett @ 2013-04-10 6:02 ` Lingzhu Xiang 0 siblings, 0 replies; 30+ messages in thread From: Lingzhu Xiang @ 2013-04-10 6:02 UTC (permalink / raw) To: Matthew Garrett; +Cc: matt.fleming, linux-efi, linux-kernel, x86 On 04/10/2013 10:41 AM, Matthew Garrett wrote: > + if (!storage_size || size > remaining_size || > + ((active_size + size + VAR_METADATA_SIZE > storage_size / 2) && > + (remaining_size - size - VAR_METADATA_SIZE < storage_size / 2))) This could overflow. (u64)32768 - (u64)32768 - VAR_METADATA_SIZE < (u64)65536 / 2 == false ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH V4 1/3] efi: Determine how much space is used by boot services-only variables 2013-04-10 2:41 [PATCH 1/3] efi: Determine how much space is used by boot services-only variables Matthew Garrett 2013-04-10 2:41 ` [PATCH 2/3] Revert "x86, efivars: firmware bug workarounds should be in platform code" Matthew Garrett 2013-04-10 2:41 ` [PATCH 3/3] efi: Distinguish between "remaining space" and actually used space Matthew Garrett @ 2013-04-10 17:46 ` Matthew Garrett 2013-04-10 17:46 ` [PATCH V4 2/3] Revert "x86, efivars: firmware bug workarounds should be in platform code" Matthew Garrett ` (3 more replies) 2013-04-15 15:53 ` [PATCH V5 1/2] efi: Pass boot services variable info to runtime code Matthew Garrett 2013-04-15 20:09 ` Fix UEFI variable paranoia Matthew Garrett 4 siblings, 4 replies; 30+ messages in thread From: Matthew Garrett @ 2013-04-10 17:46 UTC (permalink / raw) To: matt.fleming; +Cc: linux-efi, x86, linux-kernel, Matthew Garrett EFI variables can be flagged as being accessible only within boot services. This makes it awkward for us to figure out how much space they use at runtime. In theory we could figure this out by simply comparing the results from QueryVariableInfo() to the space used by all of our variables, but that fails if the platform doesn't garbage collect on every boot. Thankfully, calling QueryVariableInfo() while still inside boot services gives a more reliable answer. This patch passes that information from the EFI boot stub up to the efivars code, letting us calculate a reasonably accurate value. Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com> --- arch/x86/boot/compressed/eboot.c | 47 +++++++++++++++++++++++++++++++++++ arch/x86/include/asm/efi.h | 10 ++++++++ arch/x86/include/uapi/asm/bootparam.h | 1 + arch/x86/platform/efi/efi.c | 24 ++++++++++++++++++ drivers/firmware/efivars.c | 29 +++++++++++++++++++++ 5 files changed, 111 insertions(+) diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c index c205035..8615f75 100644 --- a/arch/x86/boot/compressed/eboot.c +++ b/arch/x86/boot/compressed/eboot.c @@ -251,6 +251,51 @@ static void find_bits(unsigned long mask, u8 *pos, u8 *size) *size = len; } +static efi_status_t setup_efi_vars(struct boot_params *params) +{ + struct setup_data *data; + struct efi_var_bootdata *efidata; + u64 store_size, remaining_size, var_size; + efi_status_t status; + + if (!sys_table->runtime->query_variable_info) + return EFI_UNSUPPORTED; + + data = (struct setup_data *)(unsigned long)params->hdr.setup_data; + + while (data && data->next) + data = (struct setup_data *)(unsigned long)data->next; + + status = efi_call_phys4(sys_table->runtime->query_variable_info, + EFI_VARIABLE_NON_VOLATILE | + EFI_VARIABLE_BOOTSERVICE_ACCESS | + EFI_VARIABLE_RUNTIME_ACCESS, &store_size, + &remaining_size, &var_size); + + if (status != EFI_SUCCESS) + return status; + + status = efi_call_phys3(sys_table->boottime->allocate_pool, + EFI_LOADER_DATA, sizeof(*efidata), &efidata); + + if (status != EFI_SUCCESS) + return status; + + efidata->data.type = SETUP_EFI_VARS; + efidata->data.len = sizeof(struct efi_var_bootdata) - + sizeof(struct setup_data); + efidata->data.next = 0; + efidata->store_size = store_size; + efidata->remaining_size = remaining_size; + efidata->max_var_size = var_size; + + if (data) + data->next = (unsigned long)efidata; + else + params->hdr.setup_data = (unsigned long)efidata; + +} + static efi_status_t setup_efi_pci(struct boot_params *params) { efi_pci_io_protocol *pci; @@ -1157,6 +1202,8 @@ struct boot_params *efi_main(void *handle, efi_system_table_t *_table, setup_graphics(boot_params); + setup_efi_vars(boot_params); + setup_efi_pci(boot_params); status = efi_call_phys3(sys_table->boottime->allocate_pool, diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h index 60c89f3..6c3a154 100644 --- a/arch/x86/include/asm/efi.h +++ b/arch/x86/include/asm/efi.h @@ -93,6 +93,9 @@ extern void __iomem *efi_ioremap(unsigned long addr, unsigned long size, #endif /* CONFIG_X86_32 */ +extern u64 efi_var_store_size; +extern u64 efi_var_remaining_size; +extern u64 efi_var_max_var_size; extern int add_efi_memmap; extern unsigned long x86_efi_facility; extern void efi_set_executable(efi_memory_desc_t *md, bool executable); @@ -102,6 +105,13 @@ extern void efi_call_phys_epilog(void); extern void efi_unmap_memmap(void); extern void efi_memory_uc(u64 addr, unsigned long size); +struct efi_var_bootdata { + struct setup_data data; + u64 store_size; + u64 remaining_size; + u64 max_var_size; +}; + #ifdef CONFIG_EFI static inline bool efi_is_native(void) diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h index c15ddaf..0874424 100644 --- a/arch/x86/include/uapi/asm/bootparam.h +++ b/arch/x86/include/uapi/asm/bootparam.h @@ -6,6 +6,7 @@ #define SETUP_E820_EXT 1 #define SETUP_DTB 2 #define SETUP_PCI 3 +#define SETUP_EFI_VARS 4 /* ram_size flags */ #define RAMDISK_IMAGE_START_MASK 0x07FF diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c index c89c245..659da48 100644 --- a/arch/x86/platform/efi/efi.c +++ b/arch/x86/platform/efi/efi.c @@ -71,6 +71,13 @@ static efi_system_table_t efi_systab __initdata; unsigned long x86_efi_facility; +u64 efi_var_store_size; +EXPORT_SYMBOL(efi_var_store_size); +u64 efi_var_remaining_size; +EXPORT_SYMBOL(efi_var_remaining_size); +u64 efi_var_max_var_size; +EXPORT_SYMBOL(efi_var_max_var_size); + /* * Returns 1 if 'facility' is enabled, 0 otherwise. */ @@ -682,6 +689,9 @@ void __init efi_init(void) char vendor[100] = "unknown"; int i = 0; void *tmp; + struct setup_data *data; + struct efi_var_bootdata *efi_var_data; + u64 pa_data; #ifdef CONFIG_X86_32 if (boot_params.efi_info.efi_systab_hi || @@ -699,6 +709,20 @@ void __init efi_init(void) if (efi_systab_init(efi_phys.systab)) return; + pa_data = boot_params.hdr.setup_data; + while (pa_data) { + data = early_ioremap(pa_data, sizeof(*efi_var_data)); + if (data->type == SETUP_EFI_VARS) { + efi_var_data = (struct efi_var_bootdata *)data; + + efi_var_store_size = efi_var_data->store_size; + efi_var_remaining_size = efi_var_data->remaining_size; + efi_var_max_var_size = efi_var_data->max_var_size; + } + pa_data = data->next; + early_iounmap(data, sizeof(*efi_var_data)); + } + set_bit(EFI_SYSTEM_TABLES, &x86_efi_facility); /* diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c index bf15d81..684a118 100644 --- a/drivers/firmware/efivars.c +++ b/drivers/firmware/efivars.c @@ -85,6 +85,7 @@ #include <linux/ramfs.h> #include <linux/pagemap.h> +#include <asm/efi.h> #include <asm/uaccess.h> #define EFIVARS_VERSION "0.08" @@ -139,6 +140,8 @@ struct efivar_attribute { static struct efivars __efivars; static struct efivar_operations ops; +static u64 boot_var_size; +static u64 boot_used_size; #define PSTORE_EFI_ATTRIBUTES \ (EFI_VARIABLE_NON_VOLATILE | \ @@ -2002,6 +2005,9 @@ int register_efivars(struct efivars *efivars, efi_char16_t *variable_name; unsigned long variable_name_size = 1024; int error = 0; + struct efivar_entry *entry; + struct efi_variable *var; + u64 active_size = 0; variable_name = kzalloc(variable_name_size, GFP_KERNEL); if (!variable_name) { @@ -2074,6 +2080,25 @@ int register_efivars(struct efivars *efivars, } } while (status != EFI_NOT_FOUND); + if (boot_used_size) { + list_for_each_entry(entry, &efivars->list, list) { + var = &entry->var; + status = get_var_data(efivars, var); + + if (status || + !(var->Attributes & EFI_VARIABLE_NON_VOLATILE)) + continue; + + active_size += var->DataSize; + active_size += utf16_strsize(var->VariableName, 1024); + } + + if (active_size < boot_used_size) + boot_var_size = boot_used_size - active_size; + else + printk(KERN_WARNING FW_BUG "efivars: Inconsistent initial sizes\n"); + } + error = create_efivars_bin_attributes(efivars); if (error) unregister_efivars(efivars); @@ -2121,6 +2146,10 @@ efivars_init(void) ops.get_next_variable = efi.get_next_variable; ops.query_variable_store = efi_query_variable_store; +#ifdef CONFIG_X86 + boot_used_size = efi_var_store_size - efi_var_remaining_size; +#endif + error = register_efivars(&__efivars, &ops, efi_kobj); if (error) goto err_put; -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH V4 2/3] Revert "x86, efivars: firmware bug workarounds should be in platform code" 2013-04-10 17:46 ` [PATCH V4 1/3] efi: Determine how much space is used by boot services-only variables Matthew Garrett @ 2013-04-10 17:46 ` Matthew Garrett 2013-04-11 13:24 ` Matt Fleming 2013-04-10 17:46 ` [PATCH V4 3/3] efi: Distinguish between "remaining space" and actually used space Matthew Garrett ` (2 subsequent siblings) 3 siblings, 1 reply; 30+ messages in thread From: Matthew Garrett @ 2013-04-10 17:46 UTC (permalink / raw) To: matt.fleming; +Cc: linux-efi, x86, linux-kernel, Matthew Garrett This reverts commit a6e4d5a03e9e3587e88aba687d8f225f4f04c792. Doing this workaround properly requires us to work within the variable code. Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com> --- arch/x86/platform/efi/efi.c | 25 ------------------------- drivers/firmware/efivars.c | 18 +++++++++++++++--- include/linux/efi.h | 9 +-------- 3 files changed, 16 insertions(+), 36 deletions(-) diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c index 659da48..fdc5074 100644 --- a/arch/x86/platform/efi/efi.c +++ b/arch/x86/platform/efi/efi.c @@ -1023,28 +1023,3 @@ u64 efi_mem_attributes(unsigned long phys_addr) } return 0; } - -/* - * Some firmware has serious problems when using more than 50% of the EFI - * variable store, i.e. it triggers bugs that can brick machines. Ensure that - * we never use more than this safe limit. - * - * Return EFI_SUCCESS if it is safe to write 'size' bytes to the variable - * store. - */ -efi_status_t efi_query_variable_store(u32 attributes, unsigned long size) -{ - efi_status_t status; - u64 storage_size, remaining_size, max_size; - - status = efi.query_variable_info(attributes, &storage_size, - &remaining_size, &max_size); - if (status != EFI_SUCCESS) - return status; - - if (!storage_size || size > remaining_size || size > max_size || - (remaining_size - size) < (storage_size / 2)) - return EFI_OUT_OF_RESOURCES; - - return EFI_SUCCESS; -} diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c index 684a118..60e7d8f 100644 --- a/drivers/firmware/efivars.c +++ b/drivers/firmware/efivars.c @@ -439,12 +439,24 @@ static efi_status_t check_var_size_locked(struct efivars *efivars, u32 attributes, unsigned long size) { + u64 storage_size, remaining_size, max_size; + efi_status_t status; const struct efivar_operations *fops = efivars->ops; - if (!efivars->ops->query_variable_store) + if (!efivars->ops->query_variable_info) return EFI_UNSUPPORTED; - return fops->query_variable_store(attributes, size); + status = fops->query_variable_info(attributes, &storage_size, + &remaining_size, &max_size); + + if (status != EFI_SUCCESS) + return status; + + if (!storage_size || size > remaining_size || size > max_size || + (remaining_size - size) < (storage_size / 2)) + return EFI_OUT_OF_RESOURCES; + + return status; } @@ -2144,7 +2156,7 @@ efivars_init(void) ops.get_variable = efi.get_variable; ops.set_variable = efi.set_variable; ops.get_next_variable = efi.get_next_variable; - ops.query_variable_store = efi_query_variable_store; + ops.query_variable_info = efi.query_variable_info; #ifdef CONFIG_X86 boot_used_size = efi_var_store_size - efi_var_remaining_size; diff --git a/include/linux/efi.h b/include/linux/efi.h index 3d7df3d..9bf2f1f 100644 --- a/include/linux/efi.h +++ b/include/linux/efi.h @@ -333,7 +333,6 @@ typedef efi_status_t efi_query_capsule_caps_t(efi_capsule_header_t **capsules, unsigned long count, u64 *max_size, int *reset_type); -typedef efi_status_t efi_query_variable_store_t(u32 attributes, unsigned long size); /* * EFI Configuration Table and GUID definitions @@ -576,15 +575,9 @@ extern void efi_enter_virtual_mode (void); /* switch EFI to virtual mode, if pos #ifdef CONFIG_X86 extern void efi_late_init(void); extern void efi_free_boot_services(void); -extern efi_status_t efi_query_variable_store(u32 attributes, unsigned long size); #else static inline void efi_late_init(void) {} static inline void efi_free_boot_services(void) {} - -static inline efi_status_t efi_query_variable_store(u32 attributes, unsigned long size) -{ - return EFI_SUCCESS; -} #endif extern void __iomem *efi_lookup_mapped_addr(u64 phys_addr); extern u64 efi_get_iobase (void); @@ -738,7 +731,7 @@ struct efivar_operations { efi_get_variable_t *get_variable; efi_get_next_variable_t *get_next_variable; efi_set_variable_t *set_variable; - efi_query_variable_store_t *query_variable_store; + efi_query_variable_info_t *query_variable_info; }; struct efivars { -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH V4 2/3] Revert "x86, efivars: firmware bug workarounds should be in platform code" 2013-04-10 17:46 ` [PATCH V4 2/3] Revert "x86, efivars: firmware bug workarounds should be in platform code" Matthew Garrett @ 2013-04-11 13:24 ` Matt Fleming 2013-04-11 13:30 ` Matthew Garrett 0 siblings, 1 reply; 30+ messages in thread From: Matt Fleming @ 2013-04-11 13:24 UTC (permalink / raw) To: Matthew Garrett; +Cc: matt.fleming, linux-efi, x86, linux-kernel On 10/04/13 18:46, Matthew Garrett wrote: > This reverts commit a6e4d5a03e9e3587e88aba687d8f225f4f04c792. Doing this > workaround properly requires us to work within the variable code. > > Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com> > --- > arch/x86/platform/efi/efi.c | 25 ------------------------- > drivers/firmware/efivars.c | 18 +++++++++++++++--- > include/linux/efi.h | 9 +-------- > 3 files changed, 16 insertions(+), 36 deletions(-) Does it really? Why can't you just hook into the get_next_variable() and set_variable() functions in arch/x86/platform/efi/efi.c? -- Matt Fleming, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V4 2/3] Revert "x86, efivars: firmware bug workarounds should be in platform code" 2013-04-11 13:24 ` Matt Fleming @ 2013-04-11 13:30 ` Matthew Garrett 0 siblings, 0 replies; 30+ messages in thread From: Matthew Garrett @ 2013-04-11 13:30 UTC (permalink / raw) To: Matt Fleming; +Cc: matt.fleming, linux-efi, x86, linux-kernel [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1011 bytes --] On Thu, 2013-04-11 at 14:24 +0100, Matt Fleming wrote: > On 10/04/13 18:46, Matthew Garrett wrote: > > This reverts commit a6e4d5a03e9e3587e88aba687d8f225f4f04c792. Doing this > > workaround properly requires us to work within the variable code. > > > > Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com> > > --- > > arch/x86/platform/efi/efi.c | 25 ------------------------- > > drivers/firmware/efivars.c | 18 +++++++++++++++--- > > include/linux/efi.h | 9 +-------- > > 3 files changed, 16 insertions(+), 36 deletions(-) > > Does it really? Why can't you just hook into the get_next_variable() and > set_variable() functions in arch/x86/platform/efi/efi.c? struct efi_variable isn't exported from efivars.c, and chunks of the workaround need to be called at efivars init anyway. -- Matthew Garrett | mjg59@srcf.ucam.org ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥ ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH V4 3/3] efi: Distinguish between "remaining space" and actually used space 2013-04-10 17:46 ` [PATCH V4 1/3] efi: Determine how much space is used by boot services-only variables Matthew Garrett 2013-04-10 17:46 ` [PATCH V4 2/3] Revert "x86, efivars: firmware bug workarounds should be in platform code" Matthew Garrett @ 2013-04-10 17:46 ` Matthew Garrett 2013-04-12 10:16 ` [PATCH V4 1/3] efi: Determine how much space is used by boot services-only variables Lingzhu Xiang 2013-04-12 12:19 ` Lingzhu Xiang 3 siblings, 0 replies; 30+ messages in thread From: Matthew Garrett @ 2013-04-10 17:46 UTC (permalink / raw) To: matt.fleming; +Cc: linux-efi, x86, linux-kernel, Matthew Garrett EFI implementations distinguish between space that is actively used by a variable and space that merely hasn't been garbage collected yet. Space that hasn't yet been garbage collected isn't available for use and so isn't counted in the remaining_space field returned by QueryVariableInfo(). Combined with commit 68d9298 this can cause problems. Some implementations don't garbage collect until the remaining space is smaller than the maximum variable size, and as a result check_var_size() will always fail once more than 50% of the variable store has been used even if most of that space is marked as available for garbage collection. The user is unable to create new variables, and deleting variables doesn't increase the remaining space. The problem that 68d9298 was attempting to avoid was one where certain platforms fail if the actively used space is greater than 50% of the available storage space. We should be able to calculate that by simply summing the size of each available variable and subtracting that from the total storage space. With luck this will fix the problem described in https://bugzilla.kernel.org/show_bug.cgi?id=55471 without permitting damage to occur to the machines 68d9298 was attempting to fix. Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com> --- drivers/firmware/efivars.c | 104 +++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 101 insertions(+), 3 deletions(-) diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c index 60e7d8f..33a2be0 100644 --- a/drivers/firmware/efivars.c +++ b/drivers/firmware/efivars.c @@ -104,6 +104,13 @@ MODULE_VERSION(EFIVARS_VERSION); */ #define GUID_LEN 36 +/* + * There's some additional metadata associated with each + * variable. Intel's reference implementation is 60 bytes - bump that + * to account for potential alignment constraints + */ +#define VAR_METADATA_SIZE 64 + static bool efivars_pstore_disable = IS_ENABLED(CONFIG_EFI_VARS_PSTORE_DEFAULT_DISABLE); @@ -405,6 +412,52 @@ validate_var(struct efi_variable *var, u8 *data, unsigned long len) } static efi_status_t +get_var_size_locked(struct efivars *efivars, struct efi_variable *var) +{ + efi_status_t status; + void *dummy; + + var->DataSize = 0; + status = efivars->ops->get_variable(var->VariableName, + &var->VendorGuid, + &var->Attributes, + &var->DataSize, + var->Data); + + if (status != EFI_BUFFER_TOO_SMALL) + return status; + + dummy = kmalloc(var->DataSize, GFP_ATOMIC); + + status = efivars->ops->get_variable(var->VariableName, + &var->VendorGuid, + &var->Attributes, + &var->DataSize, + dummy); + + kfree(dummy); + + return status; +} + +static efi_status_t +get_var_size(struct efivars *efivars, struct efi_variable *var) +{ + efi_status_t status; + unsigned long flags; + + spin_lock_irqsave(&efivars->lock, flags); + status = get_var_size_locked(efivars, var); + spin_unlock_irqrestore(&efivars->lock, flags); + + if (status != EFI_SUCCESS) { + printk(KERN_WARNING "efivars: Failed to get var size 0x%lx!\n", + status); + } + return status; +} + +static efi_status_t get_var_data_locked(struct efivars *efivars, struct efi_variable *var) { efi_status_t status; @@ -415,6 +468,10 @@ get_var_data_locked(struct efivars *efivars, struct efi_variable *var) &var->Attributes, &var->DataSize, var->Data); + + if (status != EFI_SUCCESS) + var->DataSize = 0; + return status; } @@ -440,8 +497,18 @@ check_var_size_locked(struct efivars *efivars, u32 attributes, unsigned long size) { u64 storage_size, remaining_size, max_size; + struct efivar_entry *entry; + struct efi_variable *var; efi_status_t status; const struct efivar_operations *fops = efivars->ops; + unsigned long active_size = 0; + + /* + * Any writes other than EFI_VARIABLE_NON_VOLATILE will only hit + * RAM, not flash, so ignore them. + */ + if (!(attributes & EFI_VARIABLE_NON_VOLATILE)) + return EFI_SUCCESS; if (!efivars->ops->query_variable_info) return EFI_UNSUPPORTED; @@ -452,8 +519,39 @@ check_var_size_locked(struct efivars *efivars, u32 attributes, if (status != EFI_SUCCESS) return status; - if (!storage_size || size > remaining_size || size > max_size || - (remaining_size - size) < (storage_size / 2)) + list_for_each_entry(entry, &efivars->list, list) { + var = &entry->var; + status = EFI_SUCCESS; + + if (var->DataSize == 0) + status = get_var_size_locked(efivars, var); + + if (status || !(var->Attributes & EFI_VARIABLE_NON_VOLATILE)) + continue; + + active_size += var->DataSize; + active_size += utf16_strsize(var->VariableName, 1024); + active_size += VAR_METADATA_SIZE; + } + + /* + * Add on the size of any boot services-only variables + */ + active_size += boot_var_size; + + /* + * Some firmware implementations refuse to boot if there's insufficient + * space in the variable store. We account for that by refusing the + * write if permitting it would reduce the available space to under + * 50%. However, some firmware won't reclaim variable space until + * after the used (not merely the actively used) space drops below + * a threshold. We can approximate that case with the value calculated + * above. If both the firmware and our calculations indicate that the + * available space would drop below 50%, refuse the write. + */ + if (!storage_size || size > remaining_size || + ((active_size + size + VAR_METADATA_SIZE > storage_size / 2) && + (remaining_size - size < storage_size / 2))) return EFI_OUT_OF_RESOURCES; return status; @@ -2095,7 +2193,7 @@ int register_efivars(struct efivars *efivars, if (boot_used_size) { list_for_each_entry(entry, &efivars->list, list) { var = &entry->var; - status = get_var_data(efivars, var); + status = get_var_size(efivars, var); if (status || !(var->Attributes & EFI_VARIABLE_NON_VOLATILE)) -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH V4 1/3] efi: Determine how much space is used by boot services-only variables 2013-04-10 17:46 ` [PATCH V4 1/3] efi: Determine how much space is used by boot services-only variables Matthew Garrett 2013-04-10 17:46 ` [PATCH V4 2/3] Revert "x86, efivars: firmware bug workarounds should be in platform code" Matthew Garrett 2013-04-10 17:46 ` [PATCH V4 3/3] efi: Distinguish between "remaining space" and actually used space Matthew Garrett @ 2013-04-12 10:16 ` Lingzhu Xiang 2013-04-12 10:22 ` Matt Fleming 2013-04-12 12:19 ` Lingzhu Xiang 3 siblings, 1 reply; 30+ messages in thread From: Lingzhu Xiang @ 2013-04-12 10:16 UTC (permalink / raw) To: Matthew Garrett; +Cc: matt.fleming, linux-efi, x86, linux-kernel On 04/11/2013 01:46 AM, Matthew Garrett wrote: > ops.query_variable_store = efi_query_variable_store; Can't apply here. ops.query_variable_info = efi.query_variable_info? ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V4 1/3] efi: Determine how much space is used by boot services-only variables 2013-04-12 10:16 ` [PATCH V4 1/3] efi: Determine how much space is used by boot services-only variables Lingzhu Xiang @ 2013-04-12 10:22 ` Matt Fleming 0 siblings, 0 replies; 30+ messages in thread From: Matt Fleming @ 2013-04-12 10:22 UTC (permalink / raw) To: Lingzhu Xiang; +Cc: Matthew Garrett, linux-efi, x86, linux-kernel On 12/04/13 11:16, Lingzhu Xiang wrote: > On 04/11/2013 01:46 AM, Matthew Garrett wrote: >> ops.query_variable_store = efi_query_variable_store; > > Can't apply here. ops.query_variable_info = efi.query_variable_info? It's against the 'urgent' branch at, git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V4 1/3] efi: Determine how much space is used by boot services-only variables 2013-04-10 17:46 ` [PATCH V4 1/3] efi: Determine how much space is used by boot services-only variables Matthew Garrett ` (2 preceding siblings ...) 2013-04-12 10:16 ` [PATCH V4 1/3] efi: Determine how much space is used by boot services-only variables Lingzhu Xiang @ 2013-04-12 12:19 ` Lingzhu Xiang 3 siblings, 0 replies; 30+ messages in thread From: Lingzhu Xiang @ 2013-04-12 12:19 UTC (permalink / raw) To: Matthew Garrett; +Cc: matt.fleming, linux-efi, x86, linux-kernel On 04/11/2013 01:46 AM, Matthew Garrett wrote: > EFI variables can be flagged as being accessible only within boot services. > This makes it awkward for us to figure out how much space they use at > runtime. In theory we could figure this out by simply comparing the results > from QueryVariableInfo() to the space used by all of our variables, but > that fails if the platform doesn't garbage collect on every boot. Thankfully, > calling QueryVariableInfo() while still inside boot services gives a more > reliable answer. This patch passes that information from the EFI boot stub > up to the efivars code, letting us calculate a reasonably accurate value. > > Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com> I just tried this 3.9-rc6 with this patchset with pstore fulling up space and various attempts to manually fulling up space. So far I've been able to delete variables and continue creating variables. Since QueryVariableInfo is no longer trusted and the accounting is done by the kernel, I'm somewhat concerned that variables can be repeatedly created and deleted until nvram is full of garbage to collect and the firmware hits EFI_OUT_OF_RESOURCES. Could this be any kind of problem? Lingzhu Xiang ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH V5 1/2] efi: Pass boot services variable info to runtime code 2013-04-10 2:41 [PATCH 1/3] efi: Determine how much space is used by boot services-only variables Matthew Garrett ` (2 preceding siblings ...) 2013-04-10 17:46 ` [PATCH V4 1/3] efi: Determine how much space is used by boot services-only variables Matthew Garrett @ 2013-04-15 15:53 ` Matthew Garrett 2013-04-15 15:53 ` [PATCH V5 2/2] efi: Distinguish between "remaining space" and actually used space Matthew Garrett 2013-04-15 20:09 ` Fix UEFI variable paranoia Matthew Garrett 4 siblings, 1 reply; 30+ messages in thread From: Matthew Garrett @ 2013-04-15 15:53 UTC (permalink / raw) To: matt.fleming; +Cc: linux-efi, x86, linux-kernel, Matthew Garrett EFI variables can be flagged as being accessible only within boot services. This makes it awkward for us to figure out how much space they use at runtime. In theory we could figure this out by simply comparing the results from QueryVariableInfo() to the space used by all of our variables, but that fails if the platform doesn't garbage collect on every boot. Thankfully, calling QueryVariableInfo() while still inside boot services gives a more reliable answer. This patch passes that information from the EFI boot stub up to the efi platform code. Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com> --- arch/x86/boot/compressed/eboot.c | 47 +++++++++++++++++++++++++++++++++++ arch/x86/include/asm/efi.h | 7 ++++++ arch/x86/include/uapi/asm/bootparam.h | 1 + arch/x86/platform/efi/efi.c | 21 ++++++++++++++++ 4 files changed, 76 insertions(+) diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c index c205035..8615f75 100644 --- a/arch/x86/boot/compressed/eboot.c +++ b/arch/x86/boot/compressed/eboot.c @@ -251,6 +251,51 @@ static void find_bits(unsigned long mask, u8 *pos, u8 *size) *size = len; } +static efi_status_t setup_efi_vars(struct boot_params *params) +{ + struct setup_data *data; + struct efi_var_bootdata *efidata; + u64 store_size, remaining_size, var_size; + efi_status_t status; + + if (!sys_table->runtime->query_variable_info) + return EFI_UNSUPPORTED; + + data = (struct setup_data *)(unsigned long)params->hdr.setup_data; + + while (data && data->next) + data = (struct setup_data *)(unsigned long)data->next; + + status = efi_call_phys4(sys_table->runtime->query_variable_info, + EFI_VARIABLE_NON_VOLATILE | + EFI_VARIABLE_BOOTSERVICE_ACCESS | + EFI_VARIABLE_RUNTIME_ACCESS, &store_size, + &remaining_size, &var_size); + + if (status != EFI_SUCCESS) + return status; + + status = efi_call_phys3(sys_table->boottime->allocate_pool, + EFI_LOADER_DATA, sizeof(*efidata), &efidata); + + if (status != EFI_SUCCESS) + return status; + + efidata->data.type = SETUP_EFI_VARS; + efidata->data.len = sizeof(struct efi_var_bootdata) - + sizeof(struct setup_data); + efidata->data.next = 0; + efidata->store_size = store_size; + efidata->remaining_size = remaining_size; + efidata->max_var_size = var_size; + + if (data) + data->next = (unsigned long)efidata; + else + params->hdr.setup_data = (unsigned long)efidata; + +} + static efi_status_t setup_efi_pci(struct boot_params *params) { efi_pci_io_protocol *pci; @@ -1157,6 +1202,8 @@ struct boot_params *efi_main(void *handle, efi_system_table_t *_table, setup_graphics(boot_params); + setup_efi_vars(boot_params); + setup_efi_pci(boot_params); status = efi_call_phys3(sys_table->boottime->allocate_pool, diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h index 60c89f3..2fb5d58 100644 --- a/arch/x86/include/asm/efi.h +++ b/arch/x86/include/asm/efi.h @@ -102,6 +102,13 @@ extern void efi_call_phys_epilog(void); extern void efi_unmap_memmap(void); extern void efi_memory_uc(u64 addr, unsigned long size); +struct efi_var_bootdata { + struct setup_data data; + u64 store_size; + u64 remaining_size; + u64 max_var_size; +}; + #ifdef CONFIG_EFI static inline bool efi_is_native(void) diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h index c15ddaf..0874424 100644 --- a/arch/x86/include/uapi/asm/bootparam.h +++ b/arch/x86/include/uapi/asm/bootparam.h @@ -6,6 +6,7 @@ #define SETUP_E820_EXT 1 #define SETUP_DTB 2 #define SETUP_PCI 3 +#define SETUP_EFI_VARS 4 /* ram_size flags */ #define RAMDISK_IMAGE_START_MASK 0x07FF diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c index c89c245..e844d82 100644 --- a/arch/x86/platform/efi/efi.c +++ b/arch/x86/platform/efi/efi.c @@ -69,6 +69,10 @@ struct efi_memory_map memmap; static struct efi efi_phys __initdata; static efi_system_table_t efi_systab __initdata; +static u64 efi_var_store_size; +static u64 efi_var_remaining_size; +static u64 efi_var_max_var_size; + unsigned long x86_efi_facility; /* @@ -682,6 +686,9 @@ void __init efi_init(void) char vendor[100] = "unknown"; int i = 0; void *tmp; + struct setup_data *data; + struct efi_var_bootdata *efi_var_data; + u64 pa_data; #ifdef CONFIG_X86_32 if (boot_params.efi_info.efi_systab_hi || @@ -699,6 +706,20 @@ void __init efi_init(void) if (efi_systab_init(efi_phys.systab)) return; + pa_data = boot_params.hdr.setup_data; + while (pa_data) { + data = early_ioremap(pa_data, sizeof(*efi_var_data)); + if (data->type == SETUP_EFI_VARS) { + efi_var_data = (struct efi_var_bootdata *)data; + + efi_var_store_size = efi_var_data->store_size; + efi_var_remaining_size = efi_var_data->remaining_size; + efi_var_max_var_size = efi_var_data->max_var_size; + } + pa_data = data->next; + early_iounmap(data, sizeof(*efi_var_data)); + } + set_bit(EFI_SYSTEM_TABLES, &x86_efi_facility); /* -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH V5 2/2] efi: Distinguish between "remaining space" and actually used space 2013-04-15 15:53 ` [PATCH V5 1/2] efi: Pass boot services variable info to runtime code Matthew Garrett @ 2013-04-15 15:53 ` Matthew Garrett 0 siblings, 0 replies; 30+ messages in thread From: Matthew Garrett @ 2013-04-15 15:53 UTC (permalink / raw) To: matt.fleming; +Cc: linux-efi, x86, linux-kernel, Matthew Garrett EFI implementations distinguish between space that is actively used by a variable and space that merely hasn't been garbage collected yet. Space that hasn't yet been garbage collected isn't available for use and so isn't counted in the remaining_space field returned by QueryVariableInfo(). Combined with commit 68d9298 this can cause problems. Some implementations don't garbage collect until the remaining space is smaller than the maximum variable size, and as a result check_var_size() will always fail once more than 50% of the variable store has been used even if most of that space is marked as available for garbage collection. The user is unable to create new variables, and deleting variables doesn't increase the remaining space. The problem that 68d9298 was attempting to avoid was one where certain platforms fail if the actively used space is greater than 50% of the available storage space. We should be able to calculate that by simply summing the size of each available variable and subtracting that from the total storage space. With luck this will fix the problem described in https://bugzilla.kernel.org/show_bug.cgi?id=55471 without permitting damage to occur to the machines 68d9298 was attempting to fix. Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com> --- arch/x86/platform/efi/efi.c | 109 +++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 102 insertions(+), 7 deletions(-) diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c index e844d82..a3f03cd 100644 --- a/arch/x86/platform/efi/efi.c +++ b/arch/x86/platform/efi/efi.c @@ -41,6 +41,7 @@ #include <linux/io.h> #include <linux/reboot.h> #include <linux/bcd.h> +#include <linux/ucs2_string.h> #include <asm/setup.h> #include <asm/efi.h> @@ -51,6 +52,13 @@ #define EFI_DEBUG 1 +/* + * There's some additional metadata associated with each + * variable. Intel's reference implementation is 60 bytes - bump that + * to account for potential alignment constraints + */ +#define VAR_METADATA_SIZE 64 + struct efi __read_mostly efi = { .mps = EFI_INVALID_TABLE_ADDR, .acpi = EFI_INVALID_TABLE_ADDR, @@ -72,6 +80,9 @@ static efi_system_table_t efi_systab __initdata; static u64 efi_var_store_size; static u64 efi_var_remaining_size; static u64 efi_var_max_var_size; +static u64 boot_used_size; +static u64 boot_var_size; +static u64 active_size; unsigned long x86_efi_facility; @@ -166,8 +177,53 @@ static efi_status_t virt_efi_get_next_variable(unsigned long *name_size, efi_char16_t *name, efi_guid_t *vendor) { - return efi_call_virt3(get_next_variable, - name_size, name, vendor); + efi_status_t status; + static bool finished = false; + static u64 var_size; + + status = efi_call_virt3(get_next_variable, + name_size, name, vendor); + + if (status == EFI_NOT_FOUND) { + finished = true; + if (var_size < boot_used_size) { + boot_var_size = boot_used_size - var_size; + active_size += boot_var_size; + } else { + printk(KERN_WARNING FW_BUG "efi: Inconsistent initial sizes\n"); + } + } + + if (boot_used_size && !finished) { + unsigned long size; + u32 attr; + efi_status_t s; + void *tmp; + + s = virt_efi_get_variable(name, vendor, &attr, &size, NULL); + + if (s != EFI_BUFFER_TOO_SMALL || !size) + return status; + + tmp = kmalloc(size, GFP_ATOMIC); + + if (!tmp) + return status; + + s = virt_efi_get_variable(name, vendor, &attr, &size, tmp); + + if (s == EFI_SUCCESS && (attr & EFI_VARIABLE_NON_VOLATILE)) { + var_size += size; + var_size += ucs2_strsize(name, 1024); + active_size += size; + active_size += VAR_METADATA_SIZE; + active_size += ucs2_strsize(name, 1024); + } + + kfree(tmp); + } + + return status; } static efi_status_t virt_efi_set_variable(efi_char16_t *name, @@ -176,9 +232,34 @@ static efi_status_t virt_efi_set_variable(efi_char16_t *name, unsigned long data_size, void *data) { - return efi_call_virt5(set_variable, - name, vendor, attr, - data_size, data); + efi_status_t status; + u32 orig_attr = 0; + unsigned long orig_size = 0; + + status = virt_efi_get_variable(name, vendor, &orig_attr, &orig_size, + NULL); + + if (status != EFI_BUFFER_TOO_SMALL) + orig_size = 0; + + status = efi_call_virt5(set_variable, + name, vendor, attr, + data_size, data); + + if (status == EFI_SUCCESS) { + if (orig_size) { + active_size -= orig_size; + active_size -= ucs2_strsize(name, 1024); + active_size -= VAR_METADATA_SIZE; + } + if (data_size) { + active_size += data_size; + active_size += ucs2_strsize(name, 1024); + active_size += VAR_METADATA_SIZE; + } + } + + return status; } static efi_status_t virt_efi_query_variable_info(u32 attr, @@ -720,6 +801,8 @@ void __init efi_init(void) early_iounmap(data, sizeof(*efi_var_data)); } + boot_used_size = efi_var_store_size - efi_var_remaining_size; + set_bit(EFI_SYSTEM_TABLES, &x86_efi_facility); /* @@ -1039,8 +1122,20 @@ efi_status_t efi_query_variable_store(u32 attributes, unsigned long size) if (status != EFI_SUCCESS) return status; - if (!storage_size || size > remaining_size || size > max_size || - (remaining_size - size) < (storage_size / 2)) + /* + * Some firmware implementations refuse to boot if there's insufficient + * space in the variable store. We account for that by refusing the + * write if permitting it would reduce the available space to under + * 50%. However, some firmware won't reclaim variable space until + * after the used (not merely the actively used) space drops below + * a threshold. We can approximate that case with the value calculated + * above. If both the firmware and our calculations indicate that the + * available space would drop below 50%, refuse the write. + */ + + if (!storage_size || size > remaining_size || + ((active_size + size + VAR_METADATA_SIZE > storage_size / 2) && + (remaining_size - size < storage_size / 2))) return EFI_OUT_OF_RESOURCES; return EFI_SUCCESS; -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Fix UEFI variable paranoia 2013-04-10 2:41 [PATCH 1/3] efi: Determine how much space is used by boot services-only variables Matthew Garrett ` (3 preceding siblings ...) 2013-04-15 15:53 ` [PATCH V5 1/2] efi: Pass boot services variable info to runtime code Matthew Garrett @ 2013-04-15 20:09 ` Matthew Garrett 2013-04-15 20:09 ` [PATCH V6 1/3] Move utf16 functions to kernel core and rename Matthew Garrett ` (3 more replies) 4 siblings, 4 replies; 30+ messages in thread From: Matthew Garrett @ 2013-04-15 20:09 UTC (permalink / raw) To: matt.fleming; +Cc: linux-efi, x86, linux-kernel Identical to V5, except with the UCS2 helper functions included this time. ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH V6 1/3] Move utf16 functions to kernel core and rename 2013-04-15 20:09 ` Fix UEFI variable paranoia Matthew Garrett @ 2013-04-15 20:09 ` Matthew Garrett 2013-04-15 20:09 ` [PATCH V6 2/3] efi: Pass boot services variable info to runtime code Matthew Garrett ` (2 subsequent siblings) 3 siblings, 0 replies; 30+ messages in thread From: Matthew Garrett @ 2013-04-15 20:09 UTC (permalink / raw) To: matt.fleming; +Cc: linux-efi, x86, linux-kernel, Matthew Garrett We want to be able to use the utf16 functions that are currently present in the EFI variables code in platform-specific code as well. Move them to the kernel core, and in the process rename them to accurately describe what they do - they don't handle UTF16, only UCS2. Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com> --- drivers/firmware/Kconfig | 1 + drivers/firmware/efivars.c | 80 ++++++++++----------------------------------- include/linux/ucs2_string.h | 14 ++++++++ lib/Kconfig | 3 ++ lib/Makefile | 2 ++ lib/ucs2_string.c | 51 +++++++++++++++++++++++++++++ 6 files changed, 89 insertions(+), 62 deletions(-) create mode 100644 include/linux/ucs2_string.h create mode 100644 lib/ucs2_string.c diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig index 42c759a..3e53200 100644 --- a/drivers/firmware/Kconfig +++ b/drivers/firmware/Kconfig @@ -39,6 +39,7 @@ config FIRMWARE_MEMMAP config EFI_VARS tristate "EFI Variable Support via sysfs" depends on EFI + select UCS2_STRING default n help If you say Y here, you are able to get EFI (Extensible Firmware diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c index bf15d81..182ce94 100644 --- a/drivers/firmware/efivars.c +++ b/drivers/firmware/efivars.c @@ -80,6 +80,7 @@ #include <linux/slab.h> #include <linux/pstore.h> #include <linux/ctype.h> +#include <linux/ucs2_string.h> #include <linux/fs.h> #include <linux/ramfs.h> @@ -172,51 +173,6 @@ static void efivar_update_sysfs_entries(struct work_struct *); static DECLARE_WORK(efivar_work, efivar_update_sysfs_entries); static bool efivar_wq_enabled = true; -/* Return the number of unicode characters in data */ -static unsigned long -utf16_strnlen(efi_char16_t *s, size_t maxlength) -{ - unsigned long length = 0; - - while (*s++ != 0 && length < maxlength) - length++; - return length; -} - -static inline unsigned long -utf16_strlen(efi_char16_t *s) -{ - return utf16_strnlen(s, ~0UL); -} - -/* - * Return the number of bytes is the length of this string - * Note: this is NOT the same as the number of unicode characters - */ -static inline unsigned long -utf16_strsize(efi_char16_t *data, unsigned long maxlength) -{ - return utf16_strnlen(data, maxlength/sizeof(efi_char16_t)) * sizeof(efi_char16_t); -} - -static inline int -utf16_strncmp(const efi_char16_t *a, const efi_char16_t *b, size_t len) -{ - while (1) { - if (len == 0) - return 0; - if (*a < *b) - return -1; - if (*a > *b) - return 1; - if (*a == 0) /* implies *b == 0 */ - return 0; - a++; - b++; - len--; - } -} - static bool validate_device_path(struct efi_variable *var, int match, u8 *buffer, unsigned long len) @@ -268,7 +224,7 @@ validate_load_option(struct efi_variable *var, int match, u8 *buffer, u16 filepathlength; int i, desclength = 0, namelen; - namelen = utf16_strnlen(var->VariableName, sizeof(var->VariableName)); + namelen = ucs2_strnlen(var->VariableName, sizeof(var->VariableName)); /* Either "Boot" or "Driver" followed by four digits of hex */ for (i = match; i < match+4; i++) { @@ -291,7 +247,7 @@ validate_load_option(struct efi_variable *var, int match, u8 *buffer, * There's no stored length for the description, so it has to be * found by hand */ - desclength = utf16_strsize((efi_char16_t *)(buffer + 6), len - 6) + 2; + desclength = ucs2_strsize((efi_char16_t *)(buffer + 6), len - 6) + 2; /* Each boot entry must have a descriptor */ if (!desclength) @@ -581,7 +537,7 @@ efivar_store_raw(struct efivar_entry *entry, const char *buf, size_t count) spin_lock_irq(&efivars->lock); status = check_var_size_locked(efivars, new_var->Attributes, - new_var->DataSize + utf16_strsize(new_var->VariableName, 1024)); + new_var->DataSize + ucs2_strsize(new_var->VariableName, 1024)); if (status == EFI_SUCCESS || status == EFI_UNSUPPORTED) status = efivars->ops->set_variable(new_var->VariableName, @@ -759,7 +715,7 @@ static ssize_t efivarfs_file_write(struct file *file, * QueryVariableInfo() isn't supported by the firmware. */ - varsize = datasize + utf16_strsize(var->var.VariableName, 1024); + varsize = datasize + ucs2_strsize(var->var.VariableName, 1024); status = check_var_size(efivars, attributes, varsize); if (status != EFI_SUCCESS) { @@ -1211,7 +1167,7 @@ static int efivarfs_fill_super(struct super_block *sb, void *data, int silent) inode = NULL; - len = utf16_strlen(entry->var.VariableName); + len = ucs2_strlen(entry->var.VariableName); /* name, plus '-', plus GUID, plus NUL*/ name = kmalloc(len + 1 + GUID_LEN + 1, GFP_ATOMIC); @@ -1469,8 +1425,8 @@ static int efi_pstore_erase(enum pstore_type_id type, u64 id, int count, if (efi_guidcmp(entry->var.VendorGuid, vendor)) continue; - if (utf16_strncmp(entry->var.VariableName, efi_name, - utf16_strlen(efi_name))) { + if (ucs2_strncmp(entry->var.VariableName, efi_name, + ucs2_strlen(efi_name))) { /* * Check if an old format, * which doesn't support holding @@ -1482,8 +1438,8 @@ static int efi_pstore_erase(enum pstore_type_id type, u64 id, int count, for (i = 0; i < DUMP_NAME_LEN; i++) efi_name_old[i] = name_old[i]; - if (utf16_strncmp(entry->var.VariableName, efi_name_old, - utf16_strlen(efi_name_old))) + if (ucs2_strncmp(entry->var.VariableName, efi_name_old, + ucs2_strlen(efi_name_old))) continue; } @@ -1561,8 +1517,8 @@ static ssize_t efivar_create(struct file *filp, struct kobject *kobj, * Does this variable already exist? */ list_for_each_entry_safe(search_efivar, n, &efivars->list, list) { - strsize1 = utf16_strsize(search_efivar->var.VariableName, 1024); - strsize2 = utf16_strsize(new_var->VariableName, 1024); + strsize1 = ucs2_strsize(search_efivar->var.VariableName, 1024); + strsize2 = ucs2_strsize(new_var->VariableName, 1024); if (strsize1 == strsize2 && !memcmp(&(search_efivar->var.VariableName), new_var->VariableName, strsize1) && @@ -1578,7 +1534,7 @@ static ssize_t efivar_create(struct file *filp, struct kobject *kobj, } status = check_var_size_locked(efivars, new_var->Attributes, - new_var->DataSize + utf16_strsize(new_var->VariableName, 1024)); + new_var->DataSize + ucs2_strsize(new_var->VariableName, 1024)); if (status && status != EFI_UNSUPPORTED) { spin_unlock_irq(&efivars->lock); @@ -1602,7 +1558,7 @@ static ssize_t efivar_create(struct file *filp, struct kobject *kobj, /* Create the entry in sysfs. Locking is not required here */ status = efivar_create_sysfs_entry(efivars, - utf16_strsize(new_var->VariableName, + ucs2_strsize(new_var->VariableName, 1024), new_var->VariableName, &new_var->VendorGuid); @@ -1632,8 +1588,8 @@ static ssize_t efivar_delete(struct file *filp, struct kobject *kobj, * Does this variable already exist? */ list_for_each_entry_safe(search_efivar, n, &efivars->list, list) { - strsize1 = utf16_strsize(search_efivar->var.VariableName, 1024); - strsize2 = utf16_strsize(del_var->VariableName, 1024); + strsize1 = ucs2_strsize(search_efivar->var.VariableName, 1024); + strsize2 = ucs2_strsize(del_var->VariableName, 1024); if (strsize1 == strsize2 && !memcmp(&(search_efivar->var.VariableName), del_var->VariableName, strsize1) && @@ -1679,9 +1635,9 @@ static bool variable_is_present(efi_char16_t *variable_name, efi_guid_t *vendor) unsigned long strsize1, strsize2; bool found = false; - strsize1 = utf16_strsize(variable_name, 1024); + strsize1 = ucs2_strsize(variable_name, 1024); list_for_each_entry_safe(entry, n, &efivars->list, list) { - strsize2 = utf16_strsize(entry->var.VariableName, 1024); + strsize2 = ucs2_strsize(entry->var.VariableName, 1024); if (strsize1 == strsize2 && !memcmp(variable_name, &(entry->var.VariableName), strsize2) && diff --git a/include/linux/ucs2_string.h b/include/linux/ucs2_string.h new file mode 100644 index 0000000..cbb20af --- /dev/null +++ b/include/linux/ucs2_string.h @@ -0,0 +1,14 @@ +#ifndef _LINUX_UCS2_STRING_H_ +#define _LINUX_UCS2_STRING_H_ + +#include <linux/types.h> /* for size_t */ +#include <linux/stddef.h> /* for NULL */ + +typedef u16 ucs2_char_t; + +unsigned long ucs2_strnlen(const ucs2_char_t *s, size_t maxlength); +unsigned long ucs2_strlen(const ucs2_char_t *s); +unsigned long ucs2_strsize(const ucs2_char_t *data, unsigned long maxlength); +int ucs2_strncmp(const ucs2_char_t *a, const ucs2_char_t *b, size_t len); + +#endif /* _LINUX_UCS2_STRING_H_ */ diff --git a/lib/Kconfig b/lib/Kconfig index 3958dc4..fe01d41 100644 --- a/lib/Kconfig +++ b/lib/Kconfig @@ -404,4 +404,7 @@ config OID_REGISTRY help Enable fast lookup object identifier registry. +config UCS2_STRING + tristate + endmenu diff --git a/lib/Makefile b/lib/Makefile index d7946ff..6e2cc56 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -174,3 +174,5 @@ quiet_cmd_build_OID_registry = GEN $@ cmd_build_OID_registry = perl $(srctree)/$(src)/build_OID_registry $< $@ clean-files += oid_registry_data.c + +obj-$(CONFIG_UCS2_STRING) += ucs2_string.o diff --git a/lib/ucs2_string.c b/lib/ucs2_string.c new file mode 100644 index 0000000..6f500ef --- /dev/null +++ b/lib/ucs2_string.c @@ -0,0 +1,51 @@ +#include <linux/ucs2_string.h> +#include <linux/module.h> + +/* Return the number of unicode characters in data */ +unsigned long +ucs2_strnlen(const ucs2_char_t *s, size_t maxlength) +{ + unsigned long length = 0; + + while (*s++ != 0 && length < maxlength) + length++; + return length; +} +EXPORT_SYMBOL(ucs2_strnlen); + +unsigned long +ucs2_strlen(const ucs2_char_t *s) +{ + return ucs2_strnlen(s, ~0UL); +} +EXPORT_SYMBOL(ucs2_strlen); + +/* + * Return the number of bytes is the length of this string + * Note: this is NOT the same as the number of unicode characters + */ +unsigned long +ucs2_strsize(const ucs2_char_t *data, unsigned long maxlength) +{ + return ucs2_strnlen(data, maxlength/sizeof(ucs2_char_t)) * sizeof(ucs2_char_t); +} +EXPORT_SYMBOL(ucs2_strsize); + +int +ucs2_strncmp(const ucs2_char_t *a, const ucs2_char_t *b, size_t len) +{ + while (1) { + if (len == 0) + return 0; + if (*a < *b) + return -1; + if (*a > *b) + return 1; + if (*a == 0) /* implies *b == 0 */ + return 0; + a++; + b++; + len--; + } +} +EXPORT_SYMBOL(ucs2_strncmp); -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH V6 2/3] efi: Pass boot services variable info to runtime code 2013-04-15 20:09 ` Fix UEFI variable paranoia Matthew Garrett 2013-04-15 20:09 ` [PATCH V6 1/3] Move utf16 functions to kernel core and rename Matthew Garrett @ 2013-04-15 20:09 ` Matthew Garrett 2013-04-22 15:03 ` Paul Bolle 2013-04-15 20:09 ` [PATCH V6 3/3] efi: Distinguish between "remaining space" and actually used space Matthew Garrett 2013-04-16 10:15 ` Fix UEFI variable paranoia Matt Fleming 3 siblings, 1 reply; 30+ messages in thread From: Matthew Garrett @ 2013-04-15 20:09 UTC (permalink / raw) To: matt.fleming; +Cc: linux-efi, x86, linux-kernel, Matthew Garrett EFI variables can be flagged as being accessible only within boot services. This makes it awkward for us to figure out how much space they use at runtime. In theory we could figure this out by simply comparing the results from QueryVariableInfo() to the space used by all of our variables, but that fails if the platform doesn't garbage collect on every boot. Thankfully, calling QueryVariableInfo() while still inside boot services gives a more reliable answer. This patch passes that information from the EFI boot stub up to the efi platform code. Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com> --- arch/x86/boot/compressed/eboot.c | 47 +++++++++++++++++++++++++++++++++++ arch/x86/include/asm/efi.h | 7 ++++++ arch/x86/include/uapi/asm/bootparam.h | 1 + arch/x86/platform/efi/efi.c | 21 ++++++++++++++++ 4 files changed, 76 insertions(+) diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c index c205035..8615f75 100644 --- a/arch/x86/boot/compressed/eboot.c +++ b/arch/x86/boot/compressed/eboot.c @@ -251,6 +251,51 @@ static void find_bits(unsigned long mask, u8 *pos, u8 *size) *size = len; } +static efi_status_t setup_efi_vars(struct boot_params *params) +{ + struct setup_data *data; + struct efi_var_bootdata *efidata; + u64 store_size, remaining_size, var_size; + efi_status_t status; + + if (!sys_table->runtime->query_variable_info) + return EFI_UNSUPPORTED; + + data = (struct setup_data *)(unsigned long)params->hdr.setup_data; + + while (data && data->next) + data = (struct setup_data *)(unsigned long)data->next; + + status = efi_call_phys4(sys_table->runtime->query_variable_info, + EFI_VARIABLE_NON_VOLATILE | + EFI_VARIABLE_BOOTSERVICE_ACCESS | + EFI_VARIABLE_RUNTIME_ACCESS, &store_size, + &remaining_size, &var_size); + + if (status != EFI_SUCCESS) + return status; + + status = efi_call_phys3(sys_table->boottime->allocate_pool, + EFI_LOADER_DATA, sizeof(*efidata), &efidata); + + if (status != EFI_SUCCESS) + return status; + + efidata->data.type = SETUP_EFI_VARS; + efidata->data.len = sizeof(struct efi_var_bootdata) - + sizeof(struct setup_data); + efidata->data.next = 0; + efidata->store_size = store_size; + efidata->remaining_size = remaining_size; + efidata->max_var_size = var_size; + + if (data) + data->next = (unsigned long)efidata; + else + params->hdr.setup_data = (unsigned long)efidata; + +} + static efi_status_t setup_efi_pci(struct boot_params *params) { efi_pci_io_protocol *pci; @@ -1157,6 +1202,8 @@ struct boot_params *efi_main(void *handle, efi_system_table_t *_table, setup_graphics(boot_params); + setup_efi_vars(boot_params); + setup_efi_pci(boot_params); status = efi_call_phys3(sys_table->boottime->allocate_pool, diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h index 60c89f3..2fb5d58 100644 --- a/arch/x86/include/asm/efi.h +++ b/arch/x86/include/asm/efi.h @@ -102,6 +102,13 @@ extern void efi_call_phys_epilog(void); extern void efi_unmap_memmap(void); extern void efi_memory_uc(u64 addr, unsigned long size); +struct efi_var_bootdata { + struct setup_data data; + u64 store_size; + u64 remaining_size; + u64 max_var_size; +}; + #ifdef CONFIG_EFI static inline bool efi_is_native(void) diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h index c15ddaf..0874424 100644 --- a/arch/x86/include/uapi/asm/bootparam.h +++ b/arch/x86/include/uapi/asm/bootparam.h @@ -6,6 +6,7 @@ #define SETUP_E820_EXT 1 #define SETUP_DTB 2 #define SETUP_PCI 3 +#define SETUP_EFI_VARS 4 /* ram_size flags */ #define RAMDISK_IMAGE_START_MASK 0x07FF diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c index c89c245..e844d82 100644 --- a/arch/x86/platform/efi/efi.c +++ b/arch/x86/platform/efi/efi.c @@ -69,6 +69,10 @@ struct efi_memory_map memmap; static struct efi efi_phys __initdata; static efi_system_table_t efi_systab __initdata; +static u64 efi_var_store_size; +static u64 efi_var_remaining_size; +static u64 efi_var_max_var_size; + unsigned long x86_efi_facility; /* @@ -682,6 +686,9 @@ void __init efi_init(void) char vendor[100] = "unknown"; int i = 0; void *tmp; + struct setup_data *data; + struct efi_var_bootdata *efi_var_data; + u64 pa_data; #ifdef CONFIG_X86_32 if (boot_params.efi_info.efi_systab_hi || @@ -699,6 +706,20 @@ void __init efi_init(void) if (efi_systab_init(efi_phys.systab)) return; + pa_data = boot_params.hdr.setup_data; + while (pa_data) { + data = early_ioremap(pa_data, sizeof(*efi_var_data)); + if (data->type == SETUP_EFI_VARS) { + efi_var_data = (struct efi_var_bootdata *)data; + + efi_var_store_size = efi_var_data->store_size; + efi_var_remaining_size = efi_var_data->remaining_size; + efi_var_max_var_size = efi_var_data->max_var_size; + } + pa_data = data->next; + early_iounmap(data, sizeof(*efi_var_data)); + } + set_bit(EFI_SYSTEM_TABLES, &x86_efi_facility); /* -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH V6 2/3] efi: Pass boot services variable info to runtime code 2013-04-15 20:09 ` [PATCH V6 2/3] efi: Pass boot services variable info to runtime code Matthew Garrett @ 2013-04-22 15:03 ` Paul Bolle 0 siblings, 0 replies; 30+ messages in thread From: Paul Bolle @ 2013-04-22 15:03 UTC (permalink / raw) To: Matthew Garrett, Matt Fleming; +Cc: linux-efi, x86, linux-kernel On Mon, 2013-04-15 at 13:09 -0700, Matthew Garrett wrote: > EFI variables can be flagged as being accessible only within boot services. > This makes it awkward for us to figure out how much space they use at > runtime. In theory we could figure this out by simply comparing the results > from QueryVariableInfo() to the space used by all of our variables, but > that fails if the platform doesn't garbage collect on every boot. Thankfully, > calling QueryVariableInfo() while still inside boot services gives a more > reliable answer. This patch passes that information from the EFI boot stub > up to the efi platform code. > > Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com> This seems to be the patch that has become commit cc5a080c5d40c36089bb08a8a16fa3fc7047fe0f ("efi: Pass boot services variable info to runtime code"), which is part of v3.9-rc8. It triggers this GCC warning (on Fedora 17, 32 bit): arch/x86/boot/compressed/eboot.c: In function ‘setup_efi_vars’: arch/x86/boot/compressed/eboot.c:269:2: warning: passing argument 1 of ‘efi_call_phys’ makes pointer from integer without a cast [enabled by default] In file included from arch/x86/boot/compressed/eboot.c:12:0: [...]/arch/x86/include/asm/efi.h:8:33: note: expected ‘void *’ but argument is of type ‘long unsigned int’ > --- a/arch/x86/boot/compressed/eboot.c > +++ b/arch/x86/boot/compressed/eboot.c > @@ -251,6 +251,51 @@ static void find_bits(unsigned long mask, u8 *pos, u8 *size) > *size = len; > } > > +static efi_status_t setup_efi_vars(struct boot_params *params) > +{ > + struct setup_data *data; > + struct efi_var_bootdata *efidata; > + u64 store_size, remaining_size, var_size; > + efi_status_t status; > + > + if (!sys_table->runtime->query_variable_info) > + return EFI_UNSUPPORTED; > + > + data = (struct setup_data *)(unsigned long)params->hdr.setup_data; > + > + while (data && data->next) > + data = (struct setup_data *)(unsigned long)data->next; > + > + status = efi_call_phys4(sys_table->runtime->query_variable_info, Casting to void * here makes the warning go away. But the code is sufficiently complicated (for me) to make it unobvious whether that is the right thing to do. Besides, I don't think I have hardware to test this. > + EFI_VARIABLE_NON_VOLATILE | > + EFI_VARIABLE_BOOTSERVICE_ACCESS | > + EFI_VARIABLE_RUNTIME_ACCESS, &store_size, > + &remaining_size, &var_size); > + > + if (status != EFI_SUCCESS) > + return status; > + > + status = efi_call_phys3(sys_table->boottime->allocate_pool, > + EFI_LOADER_DATA, sizeof(*efidata), &efidata); > + > + if (status != EFI_SUCCESS) > + return status; > + > + efidata->data.type = SETUP_EFI_VARS; > + efidata->data.len = sizeof(struct efi_var_bootdata) - > + sizeof(struct setup_data); > + efidata->data.next = 0; > + efidata->store_size = store_size; > + efidata->remaining_size = remaining_size; > + efidata->max_var_size = var_size; > + > + if (data) > + data->next = (unsigned long)efidata; > + else > + params->hdr.setup_data = (unsigned long)efidata; > + > +} > + > static efi_status_t setup_efi_pci(struct boot_params *params) > { > efi_pci_io_protocol *pci; Paul Bolle ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH V6 3/3] efi: Distinguish between "remaining space" and actually used space 2013-04-15 20:09 ` Fix UEFI variable paranoia Matthew Garrett 2013-04-15 20:09 ` [PATCH V6 1/3] Move utf16 functions to kernel core and rename Matthew Garrett 2013-04-15 20:09 ` [PATCH V6 2/3] efi: Pass boot services variable info to runtime code Matthew Garrett @ 2013-04-15 20:09 ` Matthew Garrett 2013-04-16 14:31 ` [PATCH 1/2] x86/Kconfig: Make EFI select UCS2_STRING Sergey Vlasov ` (2 more replies) 2013-04-16 10:15 ` Fix UEFI variable paranoia Matt Fleming 3 siblings, 3 replies; 30+ messages in thread From: Matthew Garrett @ 2013-04-15 20:09 UTC (permalink / raw) To: matt.fleming; +Cc: linux-efi, x86, linux-kernel, Matthew Garrett EFI implementations distinguish between space that is actively used by a variable and space that merely hasn't been garbage collected yet. Space that hasn't yet been garbage collected isn't available for use and so isn't counted in the remaining_space field returned by QueryVariableInfo(). Combined with commit 68d9298 this can cause problems. Some implementations don't garbage collect until the remaining space is smaller than the maximum variable size, and as a result check_var_size() will always fail once more than 50% of the variable store has been used even if most of that space is marked as available for garbage collection. The user is unable to create new variables, and deleting variables doesn't increase the remaining space. The problem that 68d9298 was attempting to avoid was one where certain platforms fail if the actively used space is greater than 50% of the available storage space. We should be able to calculate that by simply summing the size of each available variable and subtracting that from the total storage space. With luck this will fix the problem described in https://bugzilla.kernel.org/show_bug.cgi?id=55471 without permitting damage to occur to the machines 68d9298 was attempting to fix. Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com> --- arch/x86/platform/efi/efi.c | 109 +++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 102 insertions(+), 7 deletions(-) diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c index e844d82..a3f03cd 100644 --- a/arch/x86/platform/efi/efi.c +++ b/arch/x86/platform/efi/efi.c @@ -41,6 +41,7 @@ #include <linux/io.h> #include <linux/reboot.h> #include <linux/bcd.h> +#include <linux/ucs2_string.h> #include <asm/setup.h> #include <asm/efi.h> @@ -51,6 +52,13 @@ #define EFI_DEBUG 1 +/* + * There's some additional metadata associated with each + * variable. Intel's reference implementation is 60 bytes - bump that + * to account for potential alignment constraints + */ +#define VAR_METADATA_SIZE 64 + struct efi __read_mostly efi = { .mps = EFI_INVALID_TABLE_ADDR, .acpi = EFI_INVALID_TABLE_ADDR, @@ -72,6 +80,9 @@ static efi_system_table_t efi_systab __initdata; static u64 efi_var_store_size; static u64 efi_var_remaining_size; static u64 efi_var_max_var_size; +static u64 boot_used_size; +static u64 boot_var_size; +static u64 active_size; unsigned long x86_efi_facility; @@ -166,8 +177,53 @@ static efi_status_t virt_efi_get_next_variable(unsigned long *name_size, efi_char16_t *name, efi_guid_t *vendor) { - return efi_call_virt3(get_next_variable, - name_size, name, vendor); + efi_status_t status; + static bool finished = false; + static u64 var_size; + + status = efi_call_virt3(get_next_variable, + name_size, name, vendor); + + if (status == EFI_NOT_FOUND) { + finished = true; + if (var_size < boot_used_size) { + boot_var_size = boot_used_size - var_size; + active_size += boot_var_size; + } else { + printk(KERN_WARNING FW_BUG "efi: Inconsistent initial sizes\n"); + } + } + + if (boot_used_size && !finished) { + unsigned long size; + u32 attr; + efi_status_t s; + void *tmp; + + s = virt_efi_get_variable(name, vendor, &attr, &size, NULL); + + if (s != EFI_BUFFER_TOO_SMALL || !size) + return status; + + tmp = kmalloc(size, GFP_ATOMIC); + + if (!tmp) + return status; + + s = virt_efi_get_variable(name, vendor, &attr, &size, tmp); + + if (s == EFI_SUCCESS && (attr & EFI_VARIABLE_NON_VOLATILE)) { + var_size += size; + var_size += ucs2_strsize(name, 1024); + active_size += size; + active_size += VAR_METADATA_SIZE; + active_size += ucs2_strsize(name, 1024); + } + + kfree(tmp); + } + + return status; } static efi_status_t virt_efi_set_variable(efi_char16_t *name, @@ -176,9 +232,34 @@ static efi_status_t virt_efi_set_variable(efi_char16_t *name, unsigned long data_size, void *data) { - return efi_call_virt5(set_variable, - name, vendor, attr, - data_size, data); + efi_status_t status; + u32 orig_attr = 0; + unsigned long orig_size = 0; + + status = virt_efi_get_variable(name, vendor, &orig_attr, &orig_size, + NULL); + + if (status != EFI_BUFFER_TOO_SMALL) + orig_size = 0; + + status = efi_call_virt5(set_variable, + name, vendor, attr, + data_size, data); + + if (status == EFI_SUCCESS) { + if (orig_size) { + active_size -= orig_size; + active_size -= ucs2_strsize(name, 1024); + active_size -= VAR_METADATA_SIZE; + } + if (data_size) { + active_size += data_size; + active_size += ucs2_strsize(name, 1024); + active_size += VAR_METADATA_SIZE; + } + } + + return status; } static efi_status_t virt_efi_query_variable_info(u32 attr, @@ -720,6 +801,8 @@ void __init efi_init(void) early_iounmap(data, sizeof(*efi_var_data)); } + boot_used_size = efi_var_store_size - efi_var_remaining_size; + set_bit(EFI_SYSTEM_TABLES, &x86_efi_facility); /* @@ -1039,8 +1122,20 @@ efi_status_t efi_query_variable_store(u32 attributes, unsigned long size) if (status != EFI_SUCCESS) return status; - if (!storage_size || size > remaining_size || size > max_size || - (remaining_size - size) < (storage_size / 2)) + /* + * Some firmware implementations refuse to boot if there's insufficient + * space in the variable store. We account for that by refusing the + * write if permitting it would reduce the available space to under + * 50%. However, some firmware won't reclaim variable space until + * after the used (not merely the actively used) space drops below + * a threshold. We can approximate that case with the value calculated + * above. If both the firmware and our calculations indicate that the + * available space would drop below 50%, refuse the write. + */ + + if (!storage_size || size > remaining_size || + ((active_size + size + VAR_METADATA_SIZE > storage_size / 2) && + (remaining_size - size < storage_size / 2))) return EFI_OUT_OF_RESOURCES; return EFI_SUCCESS; -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 1/2] x86/Kconfig: Make EFI select UCS2_STRING 2013-04-15 20:09 ` [PATCH V6 3/3] efi: Distinguish between "remaining space" and actually used space Matthew Garrett @ 2013-04-16 14:31 ` Sergey Vlasov 2013-04-16 14:31 ` [PATCH 2/2] efi: Export efi_query_variable_store() for efivars.ko Sergey Vlasov 2013-04-16 16:39 ` [PATCH 1/2] x86/Kconfig: Make EFI select UCS2_STRING Matt Fleming 2013-04-17 10:49 ` [PATCH V6 3/3] efi: Distinguish between "remaining space" and actually used space Lingzhu Xiang 2013-04-24 10:08 ` joeyli 2 siblings, 2 replies; 30+ messages in thread From: Sergey Vlasov @ 2013-04-16 14:31 UTC (permalink / raw) To: Matthew Garrett; +Cc: matt.fleming, linux-efi, x86, linux-kernel, Sergey Vlasov The commit "efi: Distinguish between "remaining space" and actually used space" added usage of ucs2_*() functions to arch/x86/platform/efi/efi.c, but the only thing which selected UCS2_STRING was EFI_VARS, which is technically optional and can be built as a module. Signed-off-by: Sergey Vlasov <vsu@altlinux.ru> --- arch/x86/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index a4f24f5..01af853 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -1549,6 +1549,7 @@ config X86_SMAP config EFI bool "EFI runtime service support" depends on ACPI + select UCS2_STRING ---help--- This enables the kernel to use EFI runtime services that are available (such as the EFI variable services). -- 1.8.1.1 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 2/2] efi: Export efi_query_variable_store() for efivars.ko 2013-04-16 14:31 ` [PATCH 1/2] x86/Kconfig: Make EFI select UCS2_STRING Sergey Vlasov @ 2013-04-16 14:31 ` Sergey Vlasov 2013-04-16 16:39 ` Matt Fleming 2013-04-16 16:39 ` [PATCH 1/2] x86/Kconfig: Make EFI select UCS2_STRING Matt Fleming 1 sibling, 1 reply; 30+ messages in thread From: Sergey Vlasov @ 2013-04-16 14:31 UTC (permalink / raw) To: Matthew Garrett; +Cc: matt.fleming, linux-efi, x86, linux-kernel, Sergey Vlasov Fixes build with CONFIG_EFI_VARS=m which was broken after the commit "x86, efivars: firmware bug workarounds should be in platform code". Signed-off-by: Sergey Vlasov <vsu@altlinux.ru> --- arch/x86/platform/efi/efi.c | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c index 4959e3f..4f364c7 100644 --- a/arch/x86/platform/efi/efi.c +++ b/arch/x86/platform/efi/efi.c @@ -1144,3 +1144,4 @@ efi_status_t efi_query_variable_store(u32 attributes, unsigned long size) return EFI_SUCCESS; } +EXPORT_SYMBOL_GPL(efi_query_variable_store); -- 1.8.1.1 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] efi: Export efi_query_variable_store() for efivars.ko 2013-04-16 14:31 ` [PATCH 2/2] efi: Export efi_query_variable_store() for efivars.ko Sergey Vlasov @ 2013-04-16 16:39 ` Matt Fleming 0 siblings, 0 replies; 30+ messages in thread From: Matt Fleming @ 2013-04-16 16:39 UTC (permalink / raw) To: Sergey Vlasov; +Cc: Matthew Garrett, linux-efi, x86, linux-kernel On 16/04/13 15:31, Sergey Vlasov wrote: > Fixes build with CONFIG_EFI_VARS=m which was broken after the commit > "x86, efivars: firmware bug workarounds should be in platform code". > > Signed-off-by: Sergey Vlasov <vsu@altlinux.ru> > --- > arch/x86/platform/efi/efi.c | 1 + > 1 file changed, 1 insertion(+) Applied, thanks! ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] x86/Kconfig: Make EFI select UCS2_STRING 2013-04-16 14:31 ` [PATCH 1/2] x86/Kconfig: Make EFI select UCS2_STRING Sergey Vlasov 2013-04-16 14:31 ` [PATCH 2/2] efi: Export efi_query_variable_store() for efivars.ko Sergey Vlasov @ 2013-04-16 16:39 ` Matt Fleming 1 sibling, 0 replies; 30+ messages in thread From: Matt Fleming @ 2013-04-16 16:39 UTC (permalink / raw) To: Sergey Vlasov; +Cc: Matthew Garrett, linux-efi, x86, linux-kernel On 16/04/13 15:31, Sergey Vlasov wrote: > The commit "efi: Distinguish between "remaining space" and actually used > space" added usage of ucs2_*() functions to arch/x86/platform/efi/efi.c, > but the only thing which selected UCS2_STRING was EFI_VARS, which is > technically optional and can be built as a module. > > Signed-off-by: Sergey Vlasov <vsu@altlinux.ru> > --- > arch/x86/Kconfig | 1 + > 1 file changed, 1 insertion(+) Applied, thanks! ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V6 3/3] efi: Distinguish between "remaining space" and actually used space 2013-04-15 20:09 ` [PATCH V6 3/3] efi: Distinguish between "remaining space" and actually used space Matthew Garrett 2013-04-16 14:31 ` [PATCH 1/2] x86/Kconfig: Make EFI select UCS2_STRING Sergey Vlasov @ 2013-04-17 10:49 ` Lingzhu Xiang 2013-04-24 10:08 ` joeyli 2 siblings, 0 replies; 30+ messages in thread From: Lingzhu Xiang @ 2013-04-17 10:49 UTC (permalink / raw) To: Matthew Garrett; +Cc: matt.fleming, linux-efi, x86, linux-kernel On 04/16/2013 04:09 AM, Matthew Garrett wrote: > EFI implementations distinguish between space that is actively used by a > variable and space that merely hasn't been garbage collected yet. Space > that hasn't yet been garbage collected isn't available for use and so isn't > counted in the remaining_space field returned by QueryVariableInfo(). > > Combined with commit 68d9298 this can cause problems. Some implementations > don't garbage collect until the remaining space is smaller than the maximum > variable size, and as a result check_var_size() will always fail once more > than 50% of the variable store has been used even if most of that space is > marked as available for garbage collection. The user is unable to create > new variables, and deleting variables doesn't increase the remaining space. > > The problem that 68d9298 was attempting to avoid was one where certain > platforms fail if the actively used space is greater than 50% of the > available storage space. We should be able to calculate that by simply > summing the size of each available variable and subtracting that from > the total storage space. With luck this will fix the problem described in > https://bugzilla.kernel.org/show_bug.cgi?id=55471 without permitting > damage to occur to the machines 68d9298 was attempting to fix. > > Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com> > --- > arch/x86/platform/efi/efi.c | 109 +++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 102 insertions(+), 7 deletions(-) > > diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c > index e844d82..a3f03cd 100644 > --- a/arch/x86/platform/efi/efi.c > +++ b/arch/x86/platform/efi/efi.c > @@ -41,6 +41,7 @@ > #include <linux/io.h> > #include <linux/reboot.h> > #include <linux/bcd.h> > +#include <linux/ucs2_string.h> > > #include <asm/setup.h> > #include <asm/efi.h> > @@ -51,6 +52,13 @@ > > #define EFI_DEBUG 1 > > +/* > + * There's some additional metadata associated with each > + * variable. Intel's reference implementation is 60 bytes - bump that > + * to account for potential alignment constraints > + */ > +#define VAR_METADATA_SIZE 64 > + > struct efi __read_mostly efi = { > .mps = EFI_INVALID_TABLE_ADDR, > .acpi = EFI_INVALID_TABLE_ADDR, > @@ -72,6 +80,9 @@ static efi_system_table_t efi_systab __initdata; > static u64 efi_var_store_size; > static u64 efi_var_remaining_size; > static u64 efi_var_max_var_size; > +static u64 boot_used_size; > +static u64 boot_var_size; > +static u64 active_size; > > unsigned long x86_efi_facility; > > @@ -166,8 +177,53 @@ static efi_status_t virt_efi_get_next_variable(unsigned long *name_size, > efi_char16_t *name, > efi_guid_t *vendor) > { > - return efi_call_virt3(get_next_variable, > - name_size, name, vendor); > + efi_status_t status; > + static bool finished = false; > + static u64 var_size; > + > + status = efi_call_virt3(get_next_variable, > + name_size, name, vendor); > + > + if (status == EFI_NOT_FOUND) { > + finished = true; > + if (var_size < boot_used_size) { > + boot_var_size = boot_used_size - var_size; > + active_size += boot_var_size; Doesn't work sometimes. Here, if garbage is not collected, then boot_used_size will contain the size of garbage, and get added into active_size. This defeats the purpose of active_size. I added a big printk before "return EFI_OUT_OF_RESOURCES", here are the results (Dell XPS 8500, Secure Boot capable): Failed to create variables in efivarfs, printk: size=22 storage_size=131072 remaining_size=5230 max_size=5204 efi_var_store_size=131072 efi_var_remaining_size=5378 efi_var_max_var_size=5352 boot_used_size=125694 boot_var_size=125694 active_size=125694 After a few reboots, EFI shell, QueryVariableInfo.efi: MaximumVariableStorageSize=131072 RemainingVariableStorageSize=102113 MaximumVariableSize=65509 After several more pstore crash dumps, printk: size=22 storage_size=131072 remaining_size=53064 max_size=53038 efi_var_store_size=131072 efi_var_remaining_size=53212 efi_var_max_var_size=53186 boot_used_size=77860 boot_var_size=77860 active_size=77860 After reboot, EFI shell, QueryVariableInfo.efi: MaximumVariableStorageSize=131072 RemainingVariableStorageSize=50456 MaximumVariableSize=50430 -> reset ... RemainingVariableStorageSize=47922 MaximumVariableSize=47896 -> reset ... RemainingVariableStorageSize=45462 MaximumVariableSize=45436 -> reset ... RemainingVariableStorageSize=43002 MaximumVariableSize=42976 -> reset ... RemainingVariableStorageSize=40542 MaximumVariableSize=40516 Each reboot will consume some nvram? This is consistent with http://article.gmane.org/gmane.linux.kernel.stable/47156 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V6 3/3] efi: Distinguish between "remaining space" and actually used space 2013-04-15 20:09 ` [PATCH V6 3/3] efi: Distinguish between "remaining space" and actually used space Matthew Garrett 2013-04-16 14:31 ` [PATCH 1/2] x86/Kconfig: Make EFI select UCS2_STRING Sergey Vlasov 2013-04-17 10:49 ` [PATCH V6 3/3] efi: Distinguish between "remaining space" and actually used space Lingzhu Xiang @ 2013-04-24 10:08 ` joeyli 2013-04-24 10:14 ` Matthew Garrett 2 siblings, 1 reply; 30+ messages in thread From: joeyli @ 2013-04-24 10:08 UTC (permalink / raw) To: Matthew Garrett; +Cc: matt.fleming, linux-efi, x86, linux-kernel Hi all, 於 一,2013-04-15 於 13:09 -0700,Matthew Garrett 提到: > EFI implementations distinguish between space that is actively used by a > variable and space that merely hasn't been garbage collected yet. Space > that hasn't yet been garbage collected isn't available for use and so isn't > counted in the remaining_space field returned by QueryVariableInfo(). > > Combined with commit 68d9298 this can cause problems. Some implementations > don't garbage collect until the remaining space is smaller than the maximum > variable size, and as a result check_var_size() will always fail once more > than 50% of the variable store has been used even if most of that space is > marked as available for garbage collection. The user is unable to create > new variables, and deleting variables doesn't increase the remaining space. > > The problem that 68d9298 was attempting to avoid was one where certain > platforms fail if the actively used space is greater than 50% of the > available storage space. We should be able to calculate that by simply > summing the size of each available variable and subtracting that from > the total storage space. With luck this will fix the problem described in > https://bugzilla.kernel.org/show_bug.cgi?id=55471 without permitting > damage to occur to the machines 68d9298 was attempting to fix. > > Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com> > --- > arch/x86/platform/efi/efi.c | 109 +++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 102 insertions(+), 7 deletions(-) > > diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c > index e844d82..a3f03cd 100644 > --- a/arch/x86/platform/efi/efi.c > +++ b/arch/x86/platform/efi/efi.c ... > @@ -1039,8 +1122,20 @@ efi_status_t efi_query_variable_store(u32 attributes, unsigned long size) > if (status != EFI_SUCCESS) > return status; > > - if (!storage_size || size > remaining_size || size > max_size || > - (remaining_size - size) < (storage_size / 2)) > + /* > + * Some firmware implementations refuse to boot if there's insufficient > + * space in the variable store. We account for that by refusing the > + * write if permitting it would reduce the available space to under > + * 50%. However, some firmware won't reclaim variable space until > + * after the used (not merely the actively used) space drops below > + * a threshold. We can approximate that case with the value calculated > + * above. If both the firmware and our calculations indicate that the > + * available space would drop below 50%, refuse the write. > + */ > + > + if (!storage_size || size > remaining_size || > + ((active_size + size + VAR_METADATA_SIZE > storage_size / 2) && > + (remaining_size - size < storage_size / 2))) I am afraid it could not completely avoid to brick Samsung machines when binding active_size and remaining_size logic with AND. I don't have machine for testing, but my concern is we can run delete/create variables many cycles at runtime for trigger brick Samsung machines. It causes the garbage size increased and remaining_size decreased. But we still can create new variable because active_size doesn't increase due to we delete variable before create new. So, the condition "remaining_size - size < storage_size / 2" will not really hit because active_size condition is pass. And, here also can not use OR for binding active_size and remaining_size logic, that causes many machines will not trigger garbage collection because remaining_size will never tight. Please let me know if I lost any other conditions or background information. Thanks a lot! Joey Lee ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V6 3/3] efi: Distinguish between "remaining space" and actually used space 2013-04-24 10:08 ` joeyli @ 2013-04-24 10:14 ` Matthew Garrett 2013-04-24 10:59 ` joeyli 0 siblings, 1 reply; 30+ messages in thread From: Matthew Garrett @ 2013-04-24 10:14 UTC (permalink / raw) To: joeyli; +Cc: matt.fleming, linux-efi, x86, linux-kernel [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="utf-8", Size: 728 bytes --] On Wed, 2013-04-24 at 18:08 +0800, joeyli wrote: > It causes the garbage size increased and remaining_size decreased. But > we still can create new variable because active_size doesn't increase > due to we delete variable before create new. So, the condition > "remaining_size - size < storage_size / 2" will not really hit because > active_size condition is pass. That's fine - the (limited) information we have from Samsung is that there's no problem if all the space is dirty, only if all the space is marked as active. -- Matthew Garrett | mjg59@srcf.ucam.org ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥ ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V6 3/3] efi: Distinguish between "remaining space" and actually used space 2013-04-24 10:14 ` Matthew Garrett @ 2013-04-24 10:59 ` joeyli 2013-04-24 11:57 ` Matthew Garrett 0 siblings, 1 reply; 30+ messages in thread From: joeyli @ 2013-04-24 10:59 UTC (permalink / raw) To: Matthew Garrett; +Cc: matt.fleming, linux-efi, x86, linux-kernel 於 三,2013-04-24 於 10:14 +0000,Matthew Garrett 提到: > On Wed, 2013-04-24 at 18:08 +0800, joeyli wrote: > > > It causes the garbage size increased and remaining_size decreased. But > > we still can create new variable because active_size doesn't increase > > due to we delete variable before create new. So, the condition > > "remaining_size - size < storage_size / 2" will not really hit because > > active_size condition is pass. > > That's fine - the (limited) information we have from Samsung is that > there's no problem if all the space is dirty, only if all the space is > marked as active. > > -- > Matthew Garrett | mjg59@srcf.ucam.org Got it! Thanks for your explanation! Then why we don't just remove the "remaining_size" condition but only monitor the active_size should not larger then 1/2 storage_size? Does that because we allow one and only one VAR_METADATA_SIZE beyond the 1/2 storage? If so, why we don't just give ( 1/2 storage_size + VAR_METADATA_SIZ) space for active_size using? Thanks a lot! Joey Lee ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V6 3/3] efi: Distinguish between "remaining space" and actually used space 2013-04-24 10:59 ` joeyli @ 2013-04-24 11:57 ` Matthew Garrett 2013-04-24 13:23 ` joeyli 0 siblings, 1 reply; 30+ messages in thread From: Matthew Garrett @ 2013-04-24 11:57 UTC (permalink / raw) To: joeyli; +Cc: matt.fleming, linux-efi, x86, linux-kernel [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="utf-8", Size: 583 bytes --] On Wed, 2013-04-24 at 18:59 +0800, joeyli wrote: > Then why we don't just remove the "remaining_size" condition but only > monitor the active_size should not larger then 1/2 storage_size? If we calculate active_size as using more than 50% of the storage space but remaining_size says we still have more than 50% available, we should trust remaining_size instead of active_size. -- Matthew Garrett | mjg59@srcf.ucam.org ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥ ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH V6 3/3] efi: Distinguish between "remaining space" and actually used space 2013-04-24 11:57 ` Matthew Garrett @ 2013-04-24 13:23 ` joeyli 0 siblings, 0 replies; 30+ messages in thread From: joeyli @ 2013-04-24 13:23 UTC (permalink / raw) To: Matthew Garrett; +Cc: matt.fleming, linux-efi, x86, linux-kernel 於 三,2013-04-24 於 11:57 +0000,Matthew Garrett 提到: > On Wed, 2013-04-24 at 18:59 +0800, joeyli wrote: > > > Then why we don't just remove the "remaining_size" condition but only > > monitor the active_size should not larger then 1/2 storage_size? > > If we calculate active_size as using more than 50% of the storage space > but remaining_size says we still have more than 50% available, we should > trust remaining_size instead of active_size. > > -- > Matthew Garrett | mjg59@srcf.ucam.org That makes sense because BIOS base on remaining_size to trigger garbage collection. Thanks for your clarification! Joey Lee ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Fix UEFI variable paranoia 2013-04-15 20:09 ` Fix UEFI variable paranoia Matthew Garrett ` (2 preceding siblings ...) 2013-04-15 20:09 ` [PATCH V6 3/3] efi: Distinguish between "remaining space" and actually used space Matthew Garrett @ 2013-04-16 10:15 ` Matt Fleming 3 siblings, 0 replies; 30+ messages in thread From: Matt Fleming @ 2013-04-16 10:15 UTC (permalink / raw) To: Matthew Garrett; +Cc: matt.fleming, linux-efi, x86, linux-kernel On 15/04/13 21:09, Matthew Garrett wrote: > Identical to V5, except with the UCS2 helper functions included this time. Thanks these looks good to me. I've applied all three patches to the 'urgent' branch. -- Matt Fleming, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2013-04-24 13:25 UTC | newest] Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-04-10 2:41 [PATCH 1/3] efi: Determine how much space is used by boot services-only variables Matthew Garrett 2013-04-10 2:41 ` [PATCH 2/3] Revert "x86, efivars: firmware bug workarounds should be in platform code" Matthew Garrett 2013-04-10 2:41 ` [PATCH 3/3] efi: Distinguish between "remaining space" and actually used space Matthew Garrett 2013-04-10 6:02 ` Lingzhu Xiang 2013-04-10 17:46 ` [PATCH V4 1/3] efi: Determine how much space is used by boot services-only variables Matthew Garrett 2013-04-10 17:46 ` [PATCH V4 2/3] Revert "x86, efivars: firmware bug workarounds should be in platform code" Matthew Garrett 2013-04-11 13:24 ` Matt Fleming 2013-04-11 13:30 ` Matthew Garrett 2013-04-10 17:46 ` [PATCH V4 3/3] efi: Distinguish between "remaining space" and actually used space Matthew Garrett 2013-04-12 10:16 ` [PATCH V4 1/3] efi: Determine how much space is used by boot services-only variables Lingzhu Xiang 2013-04-12 10:22 ` Matt Fleming 2013-04-12 12:19 ` Lingzhu Xiang 2013-04-15 15:53 ` [PATCH V5 1/2] efi: Pass boot services variable info to runtime code Matthew Garrett 2013-04-15 15:53 ` [PATCH V5 2/2] efi: Distinguish between "remaining space" and actually used space Matthew Garrett 2013-04-15 20:09 ` Fix UEFI variable paranoia Matthew Garrett 2013-04-15 20:09 ` [PATCH V6 1/3] Move utf16 functions to kernel core and rename Matthew Garrett 2013-04-15 20:09 ` [PATCH V6 2/3] efi: Pass boot services variable info to runtime code Matthew Garrett 2013-04-22 15:03 ` Paul Bolle 2013-04-15 20:09 ` [PATCH V6 3/3] efi: Distinguish between "remaining space" and actually used space Matthew Garrett 2013-04-16 14:31 ` [PATCH 1/2] x86/Kconfig: Make EFI select UCS2_STRING Sergey Vlasov 2013-04-16 14:31 ` [PATCH 2/2] efi: Export efi_query_variable_store() for efivars.ko Sergey Vlasov 2013-04-16 16:39 ` Matt Fleming 2013-04-16 16:39 ` [PATCH 1/2] x86/Kconfig: Make EFI select UCS2_STRING Matt Fleming 2013-04-17 10:49 ` [PATCH V6 3/3] efi: Distinguish between "remaining space" and actually used space Lingzhu Xiang 2013-04-24 10:08 ` joeyli 2013-04-24 10:14 ` Matthew Garrett 2013-04-24 10:59 ` joeyli 2013-04-24 11:57 ` Matthew Garrett 2013-04-24 13:23 ` joeyli 2013-04-16 10:15 ` Fix UEFI variable paranoia Matt Fleming
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).