[U-Boot] Unexpected saving of all environment variables during EFI boot

[Alexander Graf]

On 19.10.17 22:40, Heinrich Schuchardt wrote:
> This was merged as
> ad644e7c18238026fecc647f62584d87d2c5b0a3
> efi_loader: efi variable support
> 
>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>> index ec40f41bcb..c406ff82ff 100644
>> --- a/lib/efi_loader/efi_boottime.c
>> +++ b/lib/efi_loader/efi_boottime.c
>> @@ -8,6 +8,7 @@
>>  
>>  #include <common.h>
>>  #include <efi_loader.h>
>> +#include <environment.h>
>>  #include <malloc.h>
>>  #include <asm/global_data.h>
>>  #include <libfdt_env.h>
>> @@ -942,6 +943,11 @@ static efi_status_t EFIAPI efi_exit_boot_services(void *image_handle,
>>  {
>>  	EFI_ENTRY("%p, %ld", image_handle, map_key);
>>  
>> +#if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_ENV_IS_NOWHERE)
>> +	/* save any EFI variables that have been written: */
>> +	env_save();
> 
> You save all changed variables and not specifically those bound to EFI.
> This is completely unexpected behavior for the user and not covered by
> the commit message.
> 
> Furthermore you call env_save even if no EFI variable has been touched
> at all.
> 
> This leads to writing to flash on every boot via EFI. Depending on
> technology this might ruin the flash after a few hundred reboots.

I agree that overwriting flash on every boot isn't a terribly smart idea.

Also saving random environment variables that are set while we are in a
distro loop to persistent storage isn't great either.

I agree that we should probably improve this code to only save efi
variables.

Rob, how bad would it be for Fedora if we remove the persistent variable
store call (just the env_save() line) for 2017.11 to not kill people's
storage devices? Then for the next version tackle it for real and only
save efi variable changes here and only if anything really did change.


Alex