| Message ID | 1404295802-28030-2-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Wed, 02 Jul, at 12:10:02PM, Ard Biesheuvel wrote: > According to section 7.1 of the UEFI spec, Runtime Services are not fully > reentrant, and there are particular combinations of calls that need to be > serialized. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > drivers/firmware/efi/runtime-wrappers.c | 109 +++++++++++++++++++++++++++++--- > 1 file changed, 99 insertions(+), 10 deletions(-) Ard, what's going on with this one? I didn't see it resubmitted along with v3 of "efi/arm64: handle missing virtual mapping for UEFI System Table". Note that we already have a lock to serialize access to the UEFI variable services in the form of __efivars->lock, see drivers/firmware/efi/vars.c. It's a spinlock because of the context we may need to create variables in from efi_pstore_write().
On 7 July 2014 22:29, Matt Fleming <matt@console-pimps.org> wrote: > On Wed, 02 Jul, at 12:10:02PM, Ard Biesheuvel wrote: >> According to section 7.1 of the UEFI spec, Runtime Services are not fully >> reentrant, and there are particular combinations of calls that need to be >> serialized. >> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> --- >> drivers/firmware/efi/runtime-wrappers.c | 109 +++++++++++++++++++++++++++++--- >> 1 file changed, 99 insertions(+), 10 deletions(-) > > Ard, what's going on with this one? I didn't see it resubmitted along > with v3 of "efi/arm64: handle missing virtual mapping for UEFI System > Table". > > Note that we already have a lock to serialize access to the UEFI > variable services in the form of __efivars->lock, see > drivers/firmware/efi/vars.c. It's a spinlock because of the context we > may need to create variables in from efi_pstore_write(). > As the patch says, the UEFI spec is very clear on which combinations of calls are not reentrant. I don't think the rtc_lock and the efivars->lock quite cover that completely ...
On 7 July 2014 22:43, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 7 July 2014 22:29, Matt Fleming <matt@console-pimps.org> wrote: >> On Wed, 02 Jul, at 12:10:02PM, Ard Biesheuvel wrote: >>> According to section 7.1 of the UEFI spec, Runtime Services are not fully >>> reentrant, and there are particular combinations of calls that need to be >>> serialized. >>> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>> --- >>> drivers/firmware/efi/runtime-wrappers.c | 109 +++++++++++++++++++++++++++++--- >>> 1 file changed, 99 insertions(+), 10 deletions(-) >> >> Ard, what's going on with this one? I didn't see it resubmitted along >> with v3 of "efi/arm64: handle missing virtual mapping for UEFI System >> Table". >> >> Note that we already have a lock to serialize access to the UEFI >> variable services in the form of __efivars->lock, see >> drivers/firmware/efi/vars.c. It's a spinlock because of the context we >> may need to create variables in from efi_pstore_write(). >> > > As the patch says, the UEFI spec is very clear on which combinations > of calls are not reentrant. > I don't think the rtc_lock and the efivars->lock quite cover that completely ... 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. So the current status is: - get/set time calls are serialized with respect to one another using rtc_lock at the wrapper level - get/set variable calls are serialized using the efivars->lock in the efivars module - get_next_variable() calls use the 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? - 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?
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.
On 8 July 2014 11:29, Matt Fleming <matt@console-pimps.org> wrote: > 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. > Indeed. >> 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. > OK. That may be different on ARM, though ... >> - 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? > Well, that is what is says in the comment: * ops.get_next_variable() is only called from register_efivars() * or efivar_update_sysfs_entries(), * which is protected by the BKL, so that path is safe. >> 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. > Exactly. >> - 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. I agree. So shall I update my patch to add a single spin_lock that is used by all wrappers? We will likely need the wrappers on ARM as well, only ia64 needs another approach (if desired)
On Tue, 08 Jul, at 11:45:03AM, Ard Biesheuvel wrote: > > Well, that is what is says in the comment: > * ops.get_next_variable() is only called from register_efivars() > * or efivar_update_sysfs_entries(), > * which is protected by the BKL, so that path is safe. Oops, so it does. That's a stale comment. I'll update it. > I agree. So shall I update my patch to add a single spin_lock that is > used by all wrappers? > We will likely need the wrappers on ARM as well, only ia64 needs > another approach (if desired) Please do, that would be great!
diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c index 10daa4bbb258..6588d054af99 100644 --- a/drivers/firmware/efi/runtime-wrappers.c +++ b/drivers/firmware/efi/runtime-wrappers.c @@ -15,10 +15,50 @@ */ #include <linux/efi.h> -#include <linux/spinlock.h> /* spinlock_t */ +#include <linux/mutex.h> +#include <linux/spinlock.h> #include <asm/efi.h> /* + * According to section 7.1 of the UEFI spec, Runtime Services are not fully + * reentrant, and there are particular combinations of calls that need to be + * serialized. + * + * Table 31. Rules for Reentry Into Runtime Services + * +------------------------------------+-------------------------------+ + * | If previous call is busy in | Forbidden to call | + * +------------------------------------+-------------------------------+ + * | Any | SetVirtualAddressMap() | + * +------------------------------------+-------------------------------+ + * | ConvertPointer() | ConvertPointer() | + * +------------------------------------+-------------------------------+ + * | SetVariable() | ResetSystem() | + * | UpdateCapsule() | | + * | SetTime() | | + * | SetWakeupTime() | | + * | GetNextHighMonotonicCount() | | + * +------------------------------------+-------------------------------+ + * | GetVariable() | GetVariable() | + * | GetNextVariableName() | GetNextVariableName() | + * | SetVariable() | SetVariable() | + * | QueryVariableInfo() | QueryVariableInfo() | + * | UpdateCapsule() | UpdateCapsule() | + * | QueryCapsuleCapabilities() | QueryCapsuleCapabilities() | + * | GetNextHighMonotonicCount() | GetNextHighMonotonicCount() | + * +------------------------------------+-------------------------------+ + * | GetTime() | GetTime() | + * | SetTime() | SetTime() | + * | GetWakeupTime() | GetWakeupTime() | + * | SetWakeupTime() | SetWakeupTime() | + * +------------------------------------+-------------------------------+ + * + * The first two are disregarded here, as SetVirtualAddressMap() is called + * only once, and very early, and ConvertPointer() is not exposed at all. + */ +static DEFINE_MUTEX(var_ro_mutex); +static DEFINE_MUTEX(var_rw_mutex); + +/* * As per commit ef68c8f87ed1 ("x86: Serialize EFI time accesses on rtc_lock"), * the EFI specification requires that callers of the time related runtime * functions serialize with other CMOS accesses in the kernel, as the EFI time @@ -78,14 +118,25 @@ static efi_status_t virt_efi_get_variable(efi_char16_t *name, unsigned long *data_size, void *data) { - return efi_call_virt(get_variable, name, vendor, attr, data_size, data); + efi_status_t status; + + mutex_lock(&var_ro_mutex); + status = efi_call_virt(get_variable, name, vendor, attr, data_size, + data); + mutex_unlock(&var_ro_mutex); + return status; } static efi_status_t virt_efi_get_next_variable(unsigned long *name_size, efi_char16_t *name, efi_guid_t *vendor) { - return efi_call_virt(get_next_variable, name_size, name, vendor); + efi_status_t status; + + mutex_lock(&var_ro_mutex); + status = efi_call_virt(get_next_variable, name_size, name, vendor); + mutex_unlock(&var_ro_mutex); + return status; } static efi_status_t virt_efi_set_variable(efi_char16_t *name, @@ -94,7 +145,15 @@ static efi_status_t virt_efi_set_variable(efi_char16_t *name, unsigned long data_size, void *data) { - return efi_call_virt(set_variable, name, vendor, attr, data_size, data); + efi_status_t status; + + mutex_lock(&var_ro_mutex); + mutex_lock(&var_rw_mutex); + status = efi_call_virt(set_variable, name, vendor, attr, data_size, + data); + mutex_unlock(&var_rw_mutex); + mutex_unlock(&var_ro_mutex); + return status; } static efi_status_t virt_efi_query_variable_info(u32 attr, @@ -102,16 +161,28 @@ static efi_status_t virt_efi_query_variable_info(u32 attr, u64 *remaining_space, u64 *max_variable_size) { + efi_status_t status; + if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION) return EFI_UNSUPPORTED; - return efi_call_virt(query_variable_info, attr, storage_space, - remaining_space, max_variable_size); + mutex_lock(&var_ro_mutex); + status = efi_call_virt(query_variable_info, attr, storage_space, + remaining_space, max_variable_size); + mutex_unlock(&var_ro_mutex); + return status; } static efi_status_t virt_efi_get_next_high_mono_count(u32 *count) { - return efi_call_virt(get_next_high_mono_count, count); + efi_status_t status; + + mutex_lock(&var_ro_mutex); + mutex_lock(&var_rw_mutex); + status = efi_call_virt(get_next_high_mono_count, count); + mutex_unlock(&var_rw_mutex); + mutex_unlock(&var_ro_mutex); + return status; } static void virt_efi_reset_system(int reset_type, @@ -119,17 +190,30 @@ static void virt_efi_reset_system(int reset_type, unsigned long data_size, efi_char16_t *data) { + unsigned long flags; + + mutex_lock(&var_rw_mutex); + spin_lock_irqsave(&rtc_lock, flags); __efi_call_virt(reset_system, reset_type, status, data_size, data); + spin_unlock_irqrestore(&rtc_lock, flags); + mutex_unlock(&var_rw_mutex); } static efi_status_t virt_efi_update_capsule(efi_capsule_header_t **capsules, unsigned long count, unsigned long sg_list) { + efi_status_t status; + if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION) return EFI_UNSUPPORTED; - return efi_call_virt(update_capsule, capsules, count, sg_list); + mutex_lock(&var_ro_mutex); + mutex_lock(&var_rw_mutex); + status = efi_call_virt(update_capsule, capsules, count, sg_list); + mutex_unlock(&var_rw_mutex); + mutex_unlock(&var_ro_mutex); + return status; } static efi_status_t virt_efi_query_capsule_caps(efi_capsule_header_t **capsules, @@ -137,11 +221,16 @@ static efi_status_t virt_efi_query_capsule_caps(efi_capsule_header_t **capsules, u64 *max_size, int *reset_type) { + efi_status_t status; + if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION) return EFI_UNSUPPORTED; - return efi_call_virt(query_capsule_caps, capsules, count, max_size, - reset_type); + mutex_lock(&var_ro_mutex); + status = efi_call_virt(query_capsule_caps, capsules, count, max_size, + reset_type); + mutex_unlock(&var_ro_mutex); + return status; } void efi_native_runtime_setup(void)
According to section 7.1 of the UEFI spec, Runtime Services are not fully reentrant, and there are particular combinations of calls that need to be serialized. Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- drivers/firmware/efi/runtime-wrappers.c | 109 +++++++++++++++++++++++++++++--- 1 file changed, 99 insertions(+), 10 deletions(-)