Re: [PATCH 2/2] efi: implement mandatory locking for UEFI Runtime Services

[Matt Fleming <matt-HNK1S37rvNbeXh+fF434Mdi2O/JbrIOy@public.gmane.org>]

On Tue, 08 Jul, at 10:54:13AM, Ard Biesheuvel wrote:
> 
> After doing a bit more research, I still think there is work needed if
> we aim to adhere to the UEFI spec, or at least be safe from the
> hazards it points out.
 
Note that I never claimed there wasn't a need for an EFI runtime lock, I
was just pointing out that you need to consider the efi-pstore scenario,
and that a mutex isn't suitable in that case.

I did in fact make a half-arsed attempt at introducing a runtime lock
here,

  http://git.kernel.org/cgit/linux/kernel/git/mfleming/efi.git/commit/?h=capsule-blkdev&id=c0a88ac5b21f3c837121748be2e59e995126a6cb

Provided we can get away with a single EFI runtime lock like that patch,
your recent efi_call_virt() changes actually make the required patch
much simpler, at least for arm64 and x86.

> So the current status is:
> - get/set time calls are serialized with respect to one another using
> rtc_lock at the wrapper level

The time functions are completely unused on x86, which is why no proper
runtime locking exists. It's basically dead code.

> - get/set variable calls are serialized using the efivars->lock in the
> efivars module
> - get_next_variable() calls use the BKL

It uses __efivars->lock just like the other variable services. Is that
what you meant by BKL?
 
> The two things I am most concerned with are:
> - reset system while other calls are in flight; is this handled
> implicitly in some other way?

No, it isn't handled, so yeah, it needs fixing. I think on x86 we
actually wait for other cpus to shutdown before issuing the reset but
it's obviously not possible to make that guarantee across architectures.

> - things like settime()/setwakeuptime() and setvariable() poking into
> the flash at the same time.
> 
> Perhaps it would be sufficient to have a single spinlock cover all
> these cases? Or just let efivars grab the rtc_lock as well?

I think we need to introduce a separate lock, logically below all other
subsystem specific ones (rtc_lock, __efivars->lock, etc). It needs to be
the final lock you grab before invoking the runtime services.

I don't think the additional complexity of introducing multiple locks to
parallelise access to, say, GetTime() and GetVariable(), is really worth
the headache. Definitely not without someone making a really compelling
case for why they need to squeeze every ounce of performance out of that
scenario.

-- 
Matt Fleming, Intel Open Source Technology Center