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

[Heinrich Schuchardt]

On 10/19/2017 11:28 PM, Alexander Graf wrote:
> 
> 
> 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.

Seems we worked in parallel :)

> 
>> 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?

That is why I said we have to add read and write methods in struct
env_driver.

> 
>>
>> 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 :).

There are also env implementations in U-Boot writing to disk:
- env/ext4.c
- env/fat.c

> 
> 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.

According to the UEFI standard we should save the monotonic counter on
every boot.

Regards

Heinrich