On Fri, 11 Jul, at 09:09:16AM, 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. Use a spinlock to serialize all Runtime Services with respect
> to all others, even if this is more than strictly needed.
It'd be good to include a point about why we're only using a spinlock,
namely that anything else introduces complication to code that is
unlikely to be performance critical.
[...]
> + *
> + * In order to prevent deadlocks under NMI, the wrappers for these functions
> + * only grab the efi_runtime_lock or rtc_lock spinlocks if !efi_in_nmi().
> + */
> +#ifndef efi_in_nmi
> +#define efi_in_nmi() (0)
> +#endif
It shouldn't be necessary to do the NMI checking for *all* runtime
services, unless you use, for instance, the timer functions on ARM64.
> +/*
> * 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
> @@ -30,10 +95,17 @@ static efi_status_t virt_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc)
> {
> unsigned long flags;
> efi_status_t status;
> + bool __in_nmi = efi_in_nmi();
>
> - spin_lock_irqsave(&rtc_lock, flags);
> + if (!__in_nmi) {
> + spin_lock_irqsave(&rtc_lock, flags);
> + spin_lock(&efi_runtime_lock);
> + }
> status = efi_call_virt(get_time, tm, tc);
> - spin_unlock_irqrestore(&rtc_lock, flags);
> + if (!__in_nmi) {
> + spin_unlock(&efi_runtime_lock);
> + spin_unlock_irqrestore(&rtc_lock, flags);
> + }
> return status;
> }
>
> @@ -42,8 +114,11 @@ static efi_status_t virt_efi_set_time(efi_time_t *tm)
> unsigned long flags;
> efi_status_t status;
>
> + BUG_ON(efi_in_nmi());
I think we can safely drop these BUG_ON()s. I would expect things to
explode pretty spectacularly anyway, even without them if we're in NMI
context. BUG_ON()s are usually reserved for "this is a fatal condition
and we cannot make any forward progress at all, so stop".
But also see my earlier point about how most of these functions aren't
called from NMI context.
> 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);
> + unsigned long flags;
> + efi_status_t status;
> + bool __in_nmi = efi_in_nmi();
> +
> + if (!__in_nmi)
> + spin_lock_irqsave(&efi_runtime_lock, flags);
> + status = efi_call_virt(get_next_variable, name_size, name, vendor);
> + if (!__in_nmi)
> + spin_unlock_irqrestore(&efi_runtime_lock, flags);
> + return status;
We shouldn't ever be in NMI context when calling get_next_variable(),
right?
> @@ -119,17 +245,33 @@ static void virt_efi_reset_system(int reset_type,
> unsigned long data_size,
> efi_char16_t *data)
> {
> + unsigned long flags;
> + bool __in_nmi = efi_in_nmi();
> +
> + if (!__in_nmi)
> + spin_lock_irqsave(&efi_runtime_lock, flags);
> __efi_call_virt(reset_system, reset_type, status, data_size, data);
> + if (!__in_nmi)
> + spin_unlock_irqrestore(&efi_runtime_lock, flags);
> }
Is it even possible to perform a reset through the usual machinery in
NMI context? I don't think this is possible for x86. What about for
arm64?
> static efi_status_t virt_efi_update_capsule(efi_capsule_header_t **capsules,
> unsigned long count,
> unsigned long sg_list)
> {
> + unsigned long flags;
> + efi_status_t status;
> + bool __in_nmi = efi_in_nmi();
> +
> if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
> return EFI_UNSUPPORTED;
>
> - return efi_call_virt(update_capsule, capsules, count, sg_list);
> + if (!__in_nmi)
> + spin_lock_irqsave(&efi_runtime_lock, flags);
> + status = efi_call_virt(update_capsule, capsules, count, sg_list);
> + if (!__in_nmi)
> + spin_unlock_irqrestore(&efi_runtime_lock, flags);
> + return status;
> }
I don't think we need to check for being in NMI context here.
> static efi_status_t virt_efi_query_capsule_caps(efi_capsule_header_t **capsules,
> @@ -137,11 +279,20 @@ static efi_status_t virt_efi_query_capsule_caps(efi_capsule_header_t **capsules,
> u64 *max_size,
> int *reset_type)
> {
> + unsigned long flags;
> + efi_status_t status;
> + bool __in_nmi = efi_in_nmi();
> +
> 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);
> + if (!__in_nmi)
> + spin_lock_irqsave(&efi_runtime_lock, flags);
> + status = efi_call_virt(query_capsule_caps, capsules, count, max_size,
> + reset_type);
> + if (!__in_nmi)
> + spin_unlock_irqrestore(&efi_runtime_lock, flags);
> + return status;
> }
Definitely should not be perfoming QueryCapsuleCapabilities() in NMI
context.
--
Matt Fleming, Intel Open Source Technology Center
--
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
