On 26 November 2015 at 10:54, Matt Fleming <matt@xxxxxxxxxxxxxxxxxxx> wrote:
> On Thu, 19 Nov, at 02:36:29PM, Ard Biesheuvel wrote:
>> The non-blocking version of efivar_entry_set() gives up directly on
>> failure to acquire __efivars->lock. However, it also calls check_var_size(),
>> whose implementation calls the ordinary query_variable_info() and
>> set_variable() runtime service wrappers, meaning we could still deadlock
>> if efivar_entry_set_nonblocking() is called from an atomic context that
>> interrupted a runtime service already in progress on the same CPU.
>>
>> So drop the call to check_var_size(). This is potentially unsafe on some
>> UEFI implementations that fail to boot if the varstore fills up, but
>> those systems are unlikely to be using efi-pstore in the first place.
>
> There is simply no way you can make that assumption. The whole "my
> NVRAM is full, now my machine is bricked" issue arose because
> efi-pstore was filling the NVRAM with crash logs; that's how we
> triggered the problem in the first place.
>
> The locking here is non-trivial, so it's worth spelling out. There are
> two spinlocks we need to concern ourselves with,
>
> 1. __efivars->lock
> 2. efi_runtime_lock
>
> All EFI variable operations are performed with __efivars->lock held,
> apart from the x86 efi_delete_dummy_variable() call during boot when
> we're UP anyway. For all intents and purposes, when we're SMP,
> __efivars->lock is held for all EFI variable operations.
>
> So the only potential for deadlock if CPU A is doing variable calls is
> if CPU A or CPU B is doing efi_reset() or efi_capsule_*(). That's not
> an impossible scenario (particularly on arm64 where efi_reset() is
> used more frequently), but it gives you some clue as to when deadlock
> might be a problem.
>
The likelihood for deadlock is low considering the existence of
__efivars->lock, even on arm64, but I would still like to get this
straightened out one way or the other.
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
>> ---
>> drivers/firmware/efi/vars.c | 12 +++---------
>> 1 file changed, 3 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
>> index 70a0fb10517f..caccdbffc1d0 100644
>> --- a/drivers/firmware/efi/vars.c
>> +++ b/drivers/firmware/efi/vars.c
>> @@ -615,12 +615,6 @@ efivar_entry_set_nonblocking(efi_char16_t *name, efi_guid_t vendor,
>> if (!spin_trylock_irqsave(&__efivars->lock, flags))
>> return -EBUSY;
>>
>> - status = check_var_size(attributes, size + ucs2_strsize(name, 1024));
>> - if (status != EFI_SUCCESS) {
>> - spin_unlock_irqrestore(&__efivars->lock, flags);
>> - return -ENOSPC;
>> - }
>> -
>> status = ops->set_variable_nonblocking(name, &vendor, attributes,
>> size, data);
>
> We still need some validation to occur if efi_no_storage_paranoia is
> unset, i.e. "Be paranoid about how much NVRAM we use".
>
> However, efi_query_variable_store() actually does two things,
>
> 1. Check if the write pushes free NVRAM space below EFI_MIN_RESERVE
> 2. If it does, attempt to trigger garbage collection
>
> Now, if you're in the belly of a kdump handler, attempting to trigger
> garbage collection to ensure the write succeeds should be the last
> thing on your mind. Everything you do is a last ditch attempt to
> record the reason your machine is going down.
>
> I think the best solution would be to only perform step 1. above in
> the efi_pstore_write() code path, and keep a cached copy of
> query_variable_info() that we can perform lockless queries on. We can
> initially grab it on boot and update it with every ->set_variable()
> call.
>
> Thoughts?
How would this be affected by things like suspend/resume, hibernation
or variable accesses made in SMM context (i.e., which the kernel knows
nothing about)
If we need to take all of that into account as well, we're probably
better off adding a nonblocking query_variable_info() call after all,
and wiring it up to efi_query_variable_store(), i.e., make it take a
'bool nonblocking' argument and choose between the two versions of
each of the hooks it uses.
--
Ard.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
