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

[Alexander Graf]

On 19.10.17 23:15, Rob Clark wrote:
> On Thu, Oct 19, 2017 at 5:05 PM, Alexander Graf <agraf at suse.de> wrote:
>>
>>
>> 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.
>>
> 
> As I mentioned on #u-boot, I think we can just comment the
> env_save().. or perhaps make it a config option.  It will be mildly
> annoying, since it means every boot is "first boot", ie. falling back
> to fallback.efi to populate BootOrder/BootNNNN variables, and never
> using bootmgr.  But since we can't set EFI variables from userspace
> yet, it isn't really a regression (yet).

Ok, I sent a patch to remove the env_save() line.

> Longer term, maybe we need to split out EFI variables from u-boot env
> vars..  I thought it would be nice to store them together, since it
> gives a convenient way to set EFI variables from script or cmdline.
> But at least a subset of the EFI vars should be persisted to reach
> EBBR, and if that breaks expectations about u-boot env vars, then I
> guess we need to split them.

I really like the sharing of the env space. We just need to have a
different write mechanism for EFI variables: Instead of writing the
current env, we could for example reread the original env, copy all
current efi variables into that and write it back?

> 
> Side note, devices that can't repeatedly save env (even if we split
> out the EFI variables to be stored separately) might be problematic
> with EFI_LOADER.  Maybe we need a build option they can set to have a
> more crippled version of EFI_LOADER, so as to not penalize more
> capable devices.

Every flash device self-destroys :).

Really, it boils down to the amount of saves. Writing to flash on every
boot is a pretty bad idea. If we simply only write when a variable write
actually occurred (which in most cases won't), then we should be ok on
all devices I think.


Alex