[PATCH 0/4] efi: run UEFI services with interrupts enabled

[PATCH 0/4] efi: run UEFI services with interrupts enabled

[Ard Biesheuvel]

The UEFI spec does not require interrupts to be disabled when invoking
runtime services. The reason we have been doing so is because EFI pstore
may call SetVariable() from interrupt context, which may result in deadlock
if another runtime services call was in progress on the same cpu.

The EFI pstore has already been updated to use a non-blocking path and fail
gracefully rather than spin forever, so we can updated the ordinary blocking
wrappers to run with interrupts enabled instead.

The first 3 patches fix a couple of bugs and remove a stale comment. Patch #4
actually implements the above.

Ard Biesheuvel (4):
  efi: expose non-blocking set_variable() wrapper to efivars
  efi: efivars: don't rely on blocking operations in non-blocking
    set_var()
  efi: runtime-wrappers: remove out of date comment regarding in_nmi()
  efi: runtime-wrappers: run UEFI Runtime Services with interrupts
    enabled

 drivers/firmware/efi/efi.c              |   1 +
 drivers/firmware/efi/runtime-wrappers.c | 114 +++++++++-----------
 drivers/firmware/efi/vars.c             |  12 +--
 3 files changed, 53 insertions(+), 74 deletions(-)

-- 
1.9.1

[PATCH 1/4] efi: expose non-blocking set_variable() wrapper to efivars

[Ard Biesheuvel]

Commit 6d80dba1c9fe ("efi: Provide a non-blocking SetVariable()
operation") implemented a non-blocking alternative for the UEFI
SetVariable() invocation performed by efivars, since it may occur
in atomic context. However, this version of the function was never
exposed via the efivars struct, so the non-blocking versions was
not actually callable. Fix that.

Fixes: 6d80dba1c9fe ("efi: Provide a non-blocking SetVariable() operation")
Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/firmware/efi/efi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 027ca212179f..3b52677f459a 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -180,6 +180,7 @@ static int generic_ops_register(void)
 {
 	generic_ops.get_variable = efi.get_variable;
 	generic_ops.set_variable = efi.set_variable;
+	generic_ops.set_variable_nonblocking = efi.set_variable_nonblocking;
 	generic_ops.get_next_variable = efi.get_next_variable;
 	generic_ops.query_variable_store = efi_query_variable_store;
 
-- 
1.9.1

[PATCH 2/4] efi: efivars: don't rely on blocking operations in non-blocking set_var()

[Ard Biesheuvel]

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.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 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);
 
@@ -652,9 +646,6 @@ int efivar_entry_set_safe(efi_char16_t *name, efi_guid_t vendor, u32 attributes,
 	unsigned long flags;
 	efi_status_t status;
 
-	if (!ops->query_variable_store)
-		return -ENOSYS;
-
 	/*
 	 * If the EFI variable backend provides a non-blocking
 	 * ->set_variable() operation and we're in a context where we
@@ -669,6 +660,9 @@ int efivar_entry_set_safe(efi_char16_t *name, efi_guid_t vendor, u32 attributes,
 		return efivar_entry_set_nonblocking(name, vendor, attributes,
 						    size, data);
 
+	if (!ops->query_variable_store)
+		return -ENOSYS;
+
 	if (!block) {
 		if (!spin_trylock_irqsave(&__efivars->lock, flags))
 			return -EBUSY;
-- 
1.9.1

[PATCH 3/4] efi: runtime-wrappers: remove out of date comment regarding in_nmi()

[Ard Biesheuvel]

This code is long gone, so remove the comment as well.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/firmware/efi/runtime-wrappers.c | 26 --------------------
 1 file changed, 26 deletions(-)

diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index 228bbf910461..a624f7445d11 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -62,32 +62,6 @@
 static DEFINE_SPINLOCK(efi_runtime_lock);
 
 /*
- * Some runtime services calls can be reentrant under NMI, even if the table
- * above says they are not. (source: UEFI Specification v2.4A)
- *
- * Table 32. Functions that may be called after Machine Check, INIT and NMI
- * +----------------------------+------------------------------------------+
- * | Function			| Called after Machine Check, INIT and NMI |
- * +----------------------------+------------------------------------------+
- * | GetTime()			| Yes, even if previously busy.		   |
- * | GetVariable()		| Yes, even if previously busy		   |
- * | GetNextVariableName()	| Yes, even if previously busy		   |
- * | QueryVariableInfo()	| Yes, even if previously busy		   |
- * | SetVariable()		| Yes, even if previously busy		   |
- * | UpdateCapsule()		| Yes, even if previously busy		   |
- * | QueryCapsuleCapabilities()	| Yes, even if previously busy		   |
- * | ResetSystem()		| Yes, even if previously busy		   |
- * +----------------------------+------------------------------------------+
- *
- * In order to prevent deadlocks under NMI, the wrappers for these functions
- * may only grab the efi_runtime_lock or rtc_lock spinlocks if !efi_in_nmi().
- * However, not all of the services listed are reachable through NMI code paths,
- * so the the special handling as suggested by the UEFI spec is only implemented
- * for QueryVariableInfo() and SetVariable(), as these can be reached in NMI
- * context through efi_pstore_write().
- */
-
-/*
  * 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
-- 
1.9.1

[PATCH 4/4] efi: runtime-wrappers: run UEFI Runtime Services with interrupts enabled

[Ard Biesheuvel]

The UEFI spec allows Runtime Services to be invoked with interrupts
enabled. The only reason we were disabling interrupts was to prevent
recursive calls into the services on the same CPU, which will lead to
deadlock. However, the only context where such invocations may occur
legally is from efi-pstore via efivars, and that code has been updated
to call a non-blocking alternative when invoked from a non-interruptible
context.

So instead, update the ordinary, blocking UEFI Runtime Services wrappers
to execute with interrupts enabled.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/firmware/efi/runtime-wrappers.c | 88 +++++++++++---------
 1 file changed, 49 insertions(+), 39 deletions(-)

diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index a624f7445d11..31e131f09530 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -71,27 +71,29 @@ __weak DEFINE_SPINLOCK(rtc_lock);
 
 static efi_status_t virt_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc)
 {
-	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&rtc_lock, flags);
+	BUG_ON(in_irq());
+
+	spin_lock(&rtc_lock);
 	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(get_time, tm, tc);
 	spin_unlock(&efi_runtime_lock);
-	spin_unlock_irqrestore(&rtc_lock, flags);
+	spin_unlock(&rtc_lock);
 	return status;
 }
 
 static efi_status_t virt_efi_set_time(efi_time_t *tm)
 {
-	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&rtc_lock, flags);
+	BUG_ON(in_irq());
+
+	spin_lock(&rtc_lock);
 	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(set_time, tm);
 	spin_unlock(&efi_runtime_lock);
-	spin_unlock_irqrestore(&rtc_lock, flags);
+	spin_unlock(&rtc_lock);
 	return status;
 }
 
@@ -99,27 +101,29 @@ static efi_status_t virt_efi_get_wakeup_time(efi_bool_t *enabled,
 					     efi_bool_t *pending,
 					     efi_time_t *tm)
 {
-	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&rtc_lock, flags);
+	BUG_ON(in_irq());
+
+	spin_lock(&rtc_lock);
 	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(get_wakeup_time, enabled, pending, tm);
 	spin_unlock(&efi_runtime_lock);
-	spin_unlock_irqrestore(&rtc_lock, flags);
+	spin_unlock(&rtc_lock);
 	return status;
 }
 
 static efi_status_t virt_efi_set_wakeup_time(efi_bool_t enabled, efi_time_t *tm)
 {
-	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&rtc_lock, flags);
+	BUG_ON(in_irq());
+
+	spin_lock(&rtc_lock);
 	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(set_wakeup_time, enabled, tm);
 	spin_unlock(&efi_runtime_lock);
-	spin_unlock_irqrestore(&rtc_lock, flags);
+	spin_unlock(&rtc_lock);
 	return status;
 }
 
@@ -129,13 +133,14 @@ static efi_status_t virt_efi_get_variable(efi_char16_t *name,
 					  unsigned long *data_size,
 					  void *data)
 {
-	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	BUG_ON(in_irq());
+
+	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(get_variable, name, vendor, attr, data_size,
 			       data);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
@@ -143,12 +148,13 @@ static efi_status_t virt_efi_get_next_variable(unsigned long *name_size,
 					       efi_char16_t *name,
 					       efi_guid_t *vendor)
 {
-	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	BUG_ON(in_irq());
+
+	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(get_next_variable, name_size, name, vendor);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
@@ -158,13 +164,14 @@ static efi_status_t virt_efi_set_variable(efi_char16_t *name,
 					  unsigned long data_size,
 					  void *data)
 {
-	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	BUG_ON(in_irq());
+
+	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(set_variable, name, vendor, attr, data_size,
 			       data);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
@@ -173,15 +180,14 @@ virt_efi_set_variable_nonblocking(efi_char16_t *name, efi_guid_t *vendor,
 				  u32 attr, unsigned long data_size,
 				  void *data)
 {
-	unsigned long flags;
 	efi_status_t status;
 
-	if (!spin_trylock_irqsave(&efi_runtime_lock, flags))
+	if (!spin_trylock(&efi_runtime_lock))
 		return EFI_NOT_READY;
 
 	status = efi_call_virt(set_variable, name, vendor, attr, data_size,
 			       data);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
@@ -191,27 +197,29 @@ static efi_status_t virt_efi_query_variable_info(u32 attr,
 						 u64 *remaining_space,
 						 u64 *max_variable_size)
 {
-	unsigned long flags;
 	efi_status_t status;
 
+	BUG_ON(in_irq());
+
 	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(query_variable_info, attr, storage_space,
 			       remaining_space, max_variable_size);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
 static efi_status_t virt_efi_get_next_high_mono_count(u32 *count)
 {
-	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	BUG_ON(in_irq());
+
+	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(get_next_high_mono_count, count);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
@@ -220,26 +228,27 @@ static void virt_efi_reset_system(int reset_type,
 				  unsigned long data_size,
 				  efi_char16_t *data)
 {
-	unsigned long flags;
+	BUG_ON(in_irq());
 
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	spin_lock(&efi_runtime_lock);
 	__efi_call_virt(reset_system, reset_type, status, data_size, data);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 }
 
 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;
 
+	BUG_ON(in_irq());
+
 	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(update_capsule, capsules, count, sg_list);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
@@ -248,16 +257,17 @@ 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;
 
+	BUG_ON(in_irq());
+
 	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(query_capsule_caps, capsules, count, max_size,
 			       reset_type);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
-- 
1.9.1

Re: [PATCH 2/4] efi: efivars: don't rely on blocking operations in non-blocking set_var()

[Matt Fleming]

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.

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  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?

Re: [PATCH 1/4] efi: expose non-blocking set_variable() wrapper to efivars

[Matt Fleming]

On Thu, 19 Nov, at 02:36:28PM, Ard Biesheuvel wrote:
> Commit 6d80dba1c9fe ("efi: Provide a non-blocking SetVariable()
> operation") implemented a non-blocking alternative for the UEFI
> SetVariable() invocation performed by efivars, since it may occur
> in atomic context. However, this version of the function was never
> exposed via the efivars struct, so the non-blocking versions was
> not actually callable. Fix that.
> 
> Fixes: 6d80dba1c9fe ("efi: Provide a non-blocking SetVariable() operation")
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/firmware/efi/efi.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 027ca212179f..3b52677f459a 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -180,6 +180,7 @@ static int generic_ops_register(void)
>  {
>  	generic_ops.get_variable = efi.get_variable;
>  	generic_ops.set_variable = efi.set_variable;
> +	generic_ops.set_variable_nonblocking = efi.set_variable_nonblocking;
>  	generic_ops.get_next_variable = efi.get_next_variable;
>  	generic_ops.query_variable_store = efi_query_variable_store;
>  

What a mess. Thanks for the fix Ard.

Re: [PATCH 3/4] efi: runtime-wrappers: remove out of date comment regarding in_nmi()

[Matt Fleming]

On Thu, 19 Nov, at 02:36:30PM, Ard Biesheuvel wrote:
> This code is long gone, so remove the comment as well.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/firmware/efi/runtime-wrappers.c | 26 --------------------
>  1 file changed, 26 deletions(-)

Looks good, thanks.

Re: [PATCH 4/4] efi: runtime-wrappers: run UEFI Runtime Services with interrupts enabled

[Matt Fleming]

On Thu, 19 Nov, at 02:36:31PM, Ard Biesheuvel wrote:
> The UEFI spec allows Runtime Services to be invoked with interrupts
> enabled. The only reason we were disabling interrupts was to prevent
> recursive calls into the services on the same CPU, which will lead to
> deadlock. However, the only context where such invocations may occur
> legally is from efi-pstore via efivars, and that code has been updated
> to call a non-blocking alternative when invoked from a non-interruptible
> context.
> 
> So instead, update the ordinary, blocking UEFI Runtime Services wrappers
> to execute with interrupts enabled.
 
Please document the justification for this change; why do you *need*
interrupts enabled as opposed to doing it because the spec allows it.

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/firmware/efi/runtime-wrappers.c | 88 +++++++++++---------
>  1 file changed, 49 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
> index a624f7445d11..31e131f09530 100644
> --- a/drivers/firmware/efi/runtime-wrappers.c
> +++ b/drivers/firmware/efi/runtime-wrappers.c
> @@ -71,27 +71,29 @@ __weak DEFINE_SPINLOCK(rtc_lock);
>  
>  static efi_status_t virt_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc)
>  {
> -	unsigned long flags;
>  	efi_status_t status;
>  
> -	spin_lock_irqsave(&rtc_lock, flags);
> +	BUG_ON(in_irq());
> +
> +	spin_lock(&rtc_lock);
>  	spin_lock(&efi_runtime_lock);
>  	status = efi_call_virt(get_time, tm, tc);
>  	spin_unlock(&efi_runtime_lock);
> -	spin_unlock_irqrestore(&rtc_lock, flags);
> +	spin_unlock(&rtc_lock);
>  	return status;
>  }

This looks wrong. rtc_lock can be grabbed from IRQ context. Maybe now
is the time to rip out rtc_lock? It's __weak for arm64 anyway and x86
doesn't even use these functions. Though I notice that arm also has an
rtc_lock.

>  static efi_status_t virt_efi_set_time(efi_time_t *tm)
>  {
> -	unsigned long flags;
>  	efi_status_t status;
>  
> -	spin_lock_irqsave(&rtc_lock, flags);
> +	BUG_ON(in_irq());
> +
> +	spin_lock(&rtc_lock);

The thing about using BUG_ON() here is that you get no LOCKDEP info if
you trigger it, nor any stack traces from the softlockup detector.
It's much better to leave spin_lock() alone to do its thing.

Re: [PATCH 2/4] efi: efivars: don't rely on blocking operations in non-blocking set_var()

[Ard Biesheuvel]

On 26 November 2015 at 10:54, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> 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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>  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.

Re: [PATCH 4/4] efi: runtime-wrappers: run UEFI Runtime Services with interrupts enabled

[Ard Biesheuvel]

On 26 November 2015 at 11:23, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
> On Thu, 19 Nov, at 02:36:31PM, Ard Biesheuvel wrote:
>> The UEFI spec allows Runtime Services to be invoked with interrupts
>> enabled. The only reason we were disabling interrupts was to prevent
>> recursive calls into the services on the same CPU, which will lead to
>> deadlock. However, the only context where such invocations may occur
>> legally is from efi-pstore via efivars, and that code has been updated
>> to call a non-blocking alternative when invoked from a non-interruptible
>> context.
>>
>> So instead, update the ordinary, blocking UEFI Runtime Services wrappers
>> to execute with interrupts enabled.
>
> Please document the justification for this change; why do you *need*
> interrupts enabled as opposed to doing it because the spec allows it.
>

The primary concern is EFI writes on a UP system to a variable store
that is backed by RPMB/MMC storage through some marshalling layer in
the secure OS. I will add that to the commit log for v2

>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>  drivers/firmware/efi/runtime-wrappers.c | 88 +++++++++++---------
>>  1 file changed, 49 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
>> index a624f7445d11..31e131f09530 100644
>> --- a/drivers/firmware/efi/runtime-wrappers.c
>> +++ b/drivers/firmware/efi/runtime-wrappers.c
>> @@ -71,27 +71,29 @@ __weak DEFINE_SPINLOCK(rtc_lock);
>>
>>  static efi_status_t virt_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc)
>>  {
>> -     unsigned long flags;
>>       efi_status_t status;
>>
>> -     spin_lock_irqsave(&rtc_lock, flags);
>> +     BUG_ON(in_irq());
>> +
>> +     spin_lock(&rtc_lock);
>>       spin_lock(&efi_runtime_lock);
>>       status = efi_call_virt(get_time, tm, tc);
>>       spin_unlock(&efi_runtime_lock);
>> -     spin_unlock_irqrestore(&rtc_lock, flags);
>> +     spin_unlock(&rtc_lock);
>>       return status;
>>  }
>
> This looks wrong. rtc_lock can be grabbed from IRQ context. Maybe now
> is the time to rip out rtc_lock? It's __weak for arm64 anyway and x86
> doesn't even use these functions. Though I notice that arm also has an
> rtc_lock.
>
>>  static efi_status_t virt_efi_set_time(efi_time_t *tm)
>>  {
>> -     unsigned long flags;
>>       efi_status_t status;
>>
>> -     spin_lock_irqsave(&rtc_lock, flags);
>> +     BUG_ON(in_irq());
>> +
>> +     spin_lock(&rtc_lock);
>
> The thing about using BUG_ON() here is that you get no LOCKDEP info if
> you trigger it, nor any stack traces from the softlockup detector.
> It's much better to leave spin_lock() alone to do its thing.

Re: [PATCH 2/4] efi: efivars: don't rely on blocking operations in non-blocking set_var()

[Matt Fleming]

On Thu, 26 Nov, at 01:06:27PM, Ard Biesheuvel wrote:
> 
> 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.

I *think* that'd be OK. The thing I wanted to avoid was a
proliferation of nonblocking versions of the standard EFI calls, but
limiting it to adding query_variable_info() doesn't seem too bad.

Let me think about it over the weekend. 

Re: [PATCH 2/4] efi: efivars: don't rely on blocking operations in non-blocking set_var()

[Matt Fleming]

On Fri, 27 Nov, at 09:18:36PM, Matt Fleming wrote:
> 
> I *think* that'd be OK. The thing I wanted to avoid was a
> proliferation of nonblocking versions of the standard EFI calls, but
> limiting it to adding query_variable_info() doesn't seem too bad.
> 
> Let me think about it over the weekend. 

I couldn't come up with anything better. Go for it!