Note: this is largely a rework of the final patch from v2 [2], which now has a
per-arch component (and hence additional patches). The rest of v2 has already
been picked up, and hence dropped from this posting.
Some firmware erroneously unmask IRQs (and potentially other architecture
specific exceptions) during runtime services functions, in violation of both
common sense and the UEFI specification. This can result in a number of issues
if said exceptions are taken when they are expected to be masked, and
additionally can confuse IRQ tracing if the original mask state is not
restored prior to returning from firmware.
In practice it's difficult to check that firmware never unmasks exceptions, but
we can at least check that the IRQ flags are at least consistent upon entry to
and return from a runtime services function call. This series implements said
check in the shared EFI runtime wrappers code, after an initial round of
refactoring (patches 1-5 of [2]).
I have left ia64 as-is, without this check, as ia64 doesn't currently use the
generic runtime wrappers, has many special cases for the runtime calls which
don't fit well with the generic code, and I don't expect a new, buggy ia64
firmware to appear soon.
The first time corruption of the IRQ flags is detected, we dump a stack trace,
and set TAINT_FIRMWARE_WORKAROUND. Additionally, and in all subsequent cases,
we log (with ratelimiting) the specific corruption of the flags, and restore
the expected flags to avoid redundant warnings elsewhere.
Since v1 [1]:
* Fix thinko: s/local_irq_save/local_save_flags/
* Remove ifdefs after conversion
* Remove reundant semicolon from x86 patch
* Move efi_call_virt_check_flags before first use
* Add Acked-bys and Reviewed-bys
Since v2 [2]:
* Drop the refactoring patches (1-5), which Matt has queued
* Add per-arch ARCH_EFI_IRQ_FLAGS_MASK, as discussed for v2 [3,4]
As with v2, this has been build-tested for each target, but other than arm64 I
don't have a good platform for testing. Hopefully this causes fewer explosions
than v2.
Thanks,
Mark.
[1] https://lkml.org/lkml/2016/4/21/260
[2] https://lkml.org/lkml/2016/4/22/542
[3] https://lkml.org/lkml/2016/4/25/230
[4] https://lkml.org/lkml/2016/4/25/243
Mark Rutland (5):
efi/runtime-wrappers: detect FW irq flag corruption
arm64/efi: enable runtime call flag checking
arm/efi: enable runtime call flag checking
x86/efi: enable runtime call flag checking
efi/runtime-wrappers: remove ARCH_EFI_IRQ_FLAGS_MASK ifdef
arch/arm/include/asm/efi.h | 5 +++++
arch/arm64/include/asm/efi.h | 3 +++
arch/x86/include/asm/efi.h | 4 +++-
drivers/firmware/efi/runtime-wrappers.c | 25 +++++++++++++++++++++++++
4 files changed, 36 insertions(+), 1 deletion(-)
--
1.9.1
The UEFI spec allows runtime services to be called with interrupts
masked or unmasked, and if a runtime service function needs to mask
interrupts, it must restore the mask to its original state before
returning (i.e. from the PoV of the OS, this does not change across a
call). Firmware should never unmask exceptions, as these may then be
taken by the OS unexpectedly.
Unfortunately, some firmware has been seen to unmask IRQs (and
potentially other maskable exceptions) across runtime services calls,
leaving irq flags corrupted after returning from a runtime services
function call. This may be detected by the IRQ tracing code, but often
goes unnoticed, leaving a potentially disastrous bug hidden.
This patch detects when the irq flags are corrupted by an EFI runtime
services call, logging the call and specific corruption to the console.
While restoring the expected value of the flags is insufficient to avoid
problems, we do so to avoid redundant warnings from elsewhere (e.g. IRQ
tracing).
The set of bits in flags which we want to check is architecture-specific
(e.g. we want to check FIQ on arm64, but not the zero flag on x86), so
each arch must provide ARCH_EFI_IRQ_FLAGS_MASK to describe those. In the
absence of this mask, the check is a no-op, and we redundantly save the
flags twice, but that will be short-lived as subsequent patches
will implement this and remove the scaffolding.
Signed-off-by: Mark Rutland <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Cc: Matt Fleming <[email protected]>
Cc: [email protected]
---
drivers/firmware/efi/runtime-wrappers.c | 32 ++++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index a2c8e70..1f0277e 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -16,23 +16,55 @@
#include <linux/bug.h>
#include <linux/efi.h>
+#include <linux/irqflags.h>
#include <linux/mutex.h>
#include <linux/spinlock.h>
+#include <linux/stringify.h>
#include <asm/efi.h>
+/*
+ * Temporary scaffolding until all users provide ARCH_EFI_IRQ_FLAGS_MASK.
+ */
+#ifdef ARCH_EFI_IRQ_FLAGS_MASK
+static void efi_call_virt_check_flags(unsigned long flags, const char *call)
+{
+ unsigned long cur_flags;
+ bool mismatch;
+
+ local_save_flags(cur_flags);
+
+ mismatch = !!((cur_flags ^ flags) & ARCH_EFI_IRQ_FLAGS_MASK);
+ if (!WARN_ON_ONCE(mismatch))
+ return;
+
+ add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_NOW_UNRELIABLE);
+ pr_err_ratelimited(FW_BUG "IRQ flags corrupted (0x%08lx=>0x%08lx) by EFI %s\n",
+ flags, cur_flags, call);
+ local_irq_restore(flags);
+}
+#else /* ARCH_EFI_IRQ_FLAGS_MASK */
+static inline void efi_call_virt_check_flags(unsigned long flags, const char *call) {}
+#endif /* ARCH_EFI_IRQ_FLAGS_MASK */
+
#define efi_call_virt(f, args...) \
({ \
efi_status_t __s; \
+ unsigned long flags; \
arch_efi_call_virt_setup(); \
+ local_save_flags(flags); \
__s = arch_efi_call_virt(f, args); \
+ efi_call_virt_check_flags(flags, __stringify(f)); \
arch_efi_call_virt_teardown(); \
__s; \
})
#define __efi_call_virt(f, args...) \
({ \
+ unsigned long flags; \
arch_efi_call_virt_setup(); \
+ local_save_flags(flags); \
arch_efi_call_virt(f, args); \
+ efi_call_virt_check_flags(flags, __stringify(f)); \
arch_efi_call_virt_teardown(); \
})
--
1.9.1
Now that arm, arm64, and x86 all provide ARCH_EFI_IRQ_FLAGS_MASK, we can
get rid of the trivial and now unused implementation of
efi_call_virt_check_flags.
Signed-off-by: Mark Rutland <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Cc: Matt Fleming <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/firmware/efi/runtime-wrappers.c | 7 -------
1 file changed, 7 deletions(-)
diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index 1f0277e..a51b2c3 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -22,10 +22,6 @@
#include <linux/stringify.h>
#include <asm/efi.h>
-/*
- * Temporary scaffolding until all users provide ARCH_EFI_IRQ_FLAGS_MASK.
- */
-#ifdef ARCH_EFI_IRQ_FLAGS_MASK
static void efi_call_virt_check_flags(unsigned long flags, const char *call)
{
unsigned long cur_flags;
@@ -42,9 +38,6 @@ static void efi_call_virt_check_flags(unsigned long flags, const char *call)
flags, cur_flags, call);
local_irq_restore(flags);
}
-#else /* ARCH_EFI_IRQ_FLAGS_MASK */
-static inline void efi_call_virt_check_flags(unsigned long flags, const char *call) {}
-#endif /* ARCH_EFI_IRQ_FLAGS_MASK */
#define efi_call_virt(f, args...) \
({ \
--
1.9.1
Define ARCH_EFI_IRQ_FLAGS_MASK for arm, which will enable the generic
runtime wrapper code to detect when firmware erroneously modifies flags
over a runtime services function call.
We check all allocated flags, barring those which firmware has
legitimate reason to modify (condition flags and IT state). While in
practice corruption of some flags (e.g. J) would already be fatal, we
include these for consistency and documentation purposes.
Signed-off-by: Mark Rutland <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Cc: Leif Lindholm <[email protected]>
Cc: Matt Fleming <[email protected]>
Cc: Russell King <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
arch/arm/include/asm/efi.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
index e1461fc..b05ff68 100644
--- a/arch/arm/include/asm/efi.h
+++ b/arch/arm/include/asm/efi.h
@@ -17,6 +17,7 @@
#include <asm/mach/map.h>
#include <asm/mmu_context.h>
#include <asm/pgtable.h>
+#include <asm/ptrace.h>
#ifdef CONFIG_EFI
void efi_init(void);
@@ -33,6 +34,10 @@ int efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md);
__f(args); \
})
+#define ARCH_EFI_IRQ_FLAGS_MASK \
+ (PSR_J_BIT | PSR_E_BIT | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT | \
+ PSR_T_BIT | MODE_MASK)
+
static inline void efi_set_pgd(struct mm_struct *mm)
{
check_and_switch_context(mm, NULL);
--
1.9.1
Define ARCH_EFI_IRQ_FLAGS_MASK for x86, which will enable the generic
runtime wrapper code to detect when firmware erroneously modifies flags
over a runtime services function call.
For x86 (both 32-bit and 64-bit), we only need check the interrupt flag.
Signed-off-by: Mark Rutland <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Cc: H. Peter Anvin <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Matt Fleming <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
arch/x86/include/asm/efi.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 34bd38b..c6d8ad1 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -3,6 +3,7 @@
#include <asm/fpu/api.h>
#include <asm/pgtable.h>
+#include <asm/processor-flags.h>
#include <asm/tlb.h>
/*
@@ -28,8 +29,9 @@
#define MAX_CMDLINE_ADDRESS UINT_MAX
-#ifdef CONFIG_X86_32
+#define ARCH_EFI_IRQ_FLAGS_MASK X86_EFLAGS_IF
+#ifdef CONFIG_X86_32
extern unsigned long asmlinkage efi_call_phys(void *, ...);
--
1.9.1
Define ARCH_EFI_IRQ_FLAGS_MASK for arm64, which will enable the generic
runtime wrapper code to detect when firmware erroneously modifies flags
over a runtime services function call.
Signed-off-by: Mark Rutland <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Leif Lindholm <[email protected]>
Cc: Matt Fleming <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
arch/arm64/include/asm/efi.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
index f4f71224..b44603e 100644
--- a/arch/arm64/include/asm/efi.h
+++ b/arch/arm64/include/asm/efi.h
@@ -4,6 +4,7 @@
#include <asm/io.h>
#include <asm/mmu_context.h>
#include <asm/neon.h>
+#include <asm/ptrace.h>
#include <asm/tlbflush.h>
#ifdef CONFIG_EFI
@@ -33,6 +34,8 @@ int efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md);
kernel_neon_end(); \
})
+#define ARCH_EFI_IRQ_FLAGS_MASK (PSR_D_BIT | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT)
+
/* arch specific definitions used by the stub code */
/*
--
1.9.1
On Mon, Apr 25, 2016 at 02:46:31PM +0100, Mark Rutland wrote:
> Define ARCH_EFI_IRQ_FLAGS_MASK for arm64, which will enable the generic
> runtime wrapper code to detect when firmware erroneously modifies flags
> over a runtime services function call.
>
> Signed-off-by: Mark Rutland <[email protected]>
> Cc: Ard Biesheuvel <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Leif Lindholm <[email protected]>
> Cc: Matt Fleming <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> arch/arm64/include/asm/efi.h | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
> index f4f71224..b44603e 100644
> --- a/arch/arm64/include/asm/efi.h
> +++ b/arch/arm64/include/asm/efi.h
> @@ -4,6 +4,7 @@
> #include <asm/io.h>
> #include <asm/mmu_context.h>
> #include <asm/neon.h>
> +#include <asm/ptrace.h>
> #include <asm/tlbflush.h>
>
> #ifdef CONFIG_EFI
> @@ -33,6 +34,8 @@ int efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md);
> kernel_neon_end(); \
> })
>
> +#define ARCH_EFI_IRQ_FLAGS_MASK (PSR_D_BIT | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT)
I look forward to the day when the firmware returns in big-endian AArch32 ;)
Anyway, this looks good to me:
Acked-by: Will Deacon <[email protected]>
Will
Hi Mark,
On 25/04/16 14:46, Mark Rutland wrote:
> The UEFI spec allows runtime services to be called with interrupts
> masked or unmasked, and if a runtime service function needs to mask
> interrupts, it must restore the mask to its original state before
> returning (i.e. from the PoV of the OS, this does not change across a
> call). Firmware should never unmask exceptions, as these may then be
> taken by the OS unexpectedly.
>
> Unfortunately, some firmware has been seen to unmask IRQs (and
> potentially other maskable exceptions) across runtime services calls,
> leaving irq flags corrupted after returning from a runtime services
> function call. This may be detected by the IRQ tracing code, but often
> goes unnoticed, leaving a potentially disastrous bug hidden.
>
> This patch detects when the irq flags are corrupted by an EFI runtime
> services call, logging the call and specific corruption to the console.
> While restoring the expected value of the flags is insufficient to avoid
> problems, we do so to avoid redundant warnings from elsewhere (e.g. IRQ
> tracing).
>
> The set of bits in flags which we want to check is architecture-specific
> (e.g. we want to check FIQ on arm64, but not the zero flag on x86), so
> each arch must provide ARCH_EFI_IRQ_FLAGS_MASK to describe those. In the
> absence of this mask, the check is a no-op, and we redundantly save the
> flags twice, but that will be short-lived as subsequent patches
> will implement this and remove the scaffolding.
>
> Signed-off-by: Mark Rutland <[email protected]>
> Cc: Ard Biesheuvel <[email protected]>
> Cc: Matt Fleming <[email protected]>
> Cc: [email protected]
> ---
> drivers/firmware/efi/runtime-wrappers.c | 32 ++++++++++++++++++++++++++++++++
> 1 file changed, 32 insertions(+)
>
> diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
> index a2c8e70..1f0277e 100644
> --- a/drivers/firmware/efi/runtime-wrappers.c
> +++ b/drivers/firmware/efi/runtime-wrappers.c
> @@ -16,23 +16,55 @@
>
> #include <linux/bug.h>
> #include <linux/efi.h>
> +#include <linux/irqflags.h>
> #include <linux/mutex.h>
> #include <linux/spinlock.h>
> +#include <linux/stringify.h>
> #include <asm/efi.h>
>
> +/*
> + * Temporary scaffolding until all users provide ARCH_EFI_IRQ_FLAGS_MASK.
> + */
> +#ifdef ARCH_EFI_IRQ_FLAGS_MASK
> +static void efi_call_virt_check_flags(unsigned long flags, const char *call)
> +{
> + unsigned long cur_flags;
> + bool mismatch;
> +
> + local_save_flags(cur_flags);
> +
> + mismatch = !!((cur_flags ^ flags) & ARCH_EFI_IRQ_FLAGS_MASK);
nit: the assignment itself is already a conversion to bool, so the
excitement is redundant here.
Robin.
> + if (!WARN_ON_ONCE(mismatch))
> + return;
> +
> + add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_NOW_UNRELIABLE);
> + pr_err_ratelimited(FW_BUG "IRQ flags corrupted (0x%08lx=>0x%08lx) by EFI %s\n",
> + flags, cur_flags, call);
> + local_irq_restore(flags);
> +}
> +#else /* ARCH_EFI_IRQ_FLAGS_MASK */
> +static inline void efi_call_virt_check_flags(unsigned long flags, const char *call) {}
> +#endif /* ARCH_EFI_IRQ_FLAGS_MASK */
> +
> #define efi_call_virt(f, args...) \
> ({ \
> efi_status_t __s; \
> + unsigned long flags; \
> arch_efi_call_virt_setup(); \
> + local_save_flags(flags); \
> __s = arch_efi_call_virt(f, args); \
> + efi_call_virt_check_flags(flags, __stringify(f)); \
> arch_efi_call_virt_teardown(); \
> __s; \
> })
>
> #define __efi_call_virt(f, args...) \
> ({ \
> + unsigned long flags; \
> arch_efi_call_virt_setup(); \
> + local_save_flags(flags); \
> arch_efi_call_virt(f, args); \
> + efi_call_virt_check_flags(flags, __stringify(f)); \
> arch_efi_call_virt_teardown(); \
> })
>
>
On Mon, 25 Apr, at 03:12:01PM, Robin Murphy wrote:
> >+static void efi_call_virt_check_flags(unsigned long flags, const char *call)
> >+{
> >+ unsigned long cur_flags;
> >+ bool mismatch;
> >+
> >+ local_save_flags(cur_flags);
> >+
> >+ mismatch = !!((cur_flags ^ flags) & ARCH_EFI_IRQ_FLAGS_MASK);
>
> nit: the assignment itself is already a conversion to bool, so the
> excitement is redundant here.
This was intentional. I asked Mark to make this change so that it's
explicit for the developer that we're performing the type conversion.
On 25 April 2016 at 16:15, Matt Fleming <[email protected]> wrote:
> On Mon, 25 Apr, at 03:12:01PM, Robin Murphy wrote:
>> >+static void efi_call_virt_check_flags(unsigned long flags, const char *call)
>> >+{
>> >+ unsigned long cur_flags;
>> >+ bool mismatch;
>> >+
>> >+ local_save_flags(cur_flags);
>> >+
>> >+ mismatch = !!((cur_flags ^ flags) & ARCH_EFI_IRQ_FLAGS_MASK);
>>
>> nit: the assignment itself is already a conversion to bool, so the
>> excitement is redundant here.
>
> This was intentional. I asked Mark to make this change so that it's
> explicit for the developer that we're performing the type conversion.
But replacing an implicit boolean cast with an explicit one makes
little sense, no? Don't we simply want '!= 0' here if you need a
boolean expression?
On Mon, 25 Apr, at 04:18:41PM, Ard Biesheuvel wrote:
> On 25 April 2016 at 16:15, Matt Fleming <[email protected]> wrote:
> > On Mon, 25 Apr, at 03:12:01PM, Robin Murphy wrote:
> >> >+static void efi_call_virt_check_flags(unsigned long flags, const char *call)
> >> >+{
> >> >+ unsigned long cur_flags;
> >> >+ bool mismatch;
> >> >+
> >> >+ local_save_flags(cur_flags);
> >> >+
> >> >+ mismatch = !!((cur_flags ^ flags) & ARCH_EFI_IRQ_FLAGS_MASK);
> >>
> >> nit: the assignment itself is already a conversion to bool, so the
> >> excitement is redundant here.
> >
> > This was intentional. I asked Mark to make this change so that it's
> > explicit for the developer that we're performing the type conversion.
>
> But replacing an implicit boolean cast with an explicit one makes
> little sense, no? Don't we simply want '!= 0' here if you need a
> boolean expression?
Aha but '!!' is fewer characters to type!!
I'm not that bothered as long as we don't stuff an int into a bool
without giving the programmer some idea we're doing that. It's not
about the compiler getting it wrong, more about a developer
introducing a bug when they change the code in the future.
Unless anyone objects, I'll fix this up to use '!= 0' when I apply it.
On Mon, Apr 25, 2016 at 03:24:35PM +0100, Matt Fleming wrote:
> On Mon, 25 Apr, at 04:18:41PM, Ard Biesheuvel wrote:
> > On 25 April 2016 at 16:15, Matt Fleming <[email protected]> wrote:
> > > On Mon, 25 Apr, at 03:12:01PM, Robin Murphy wrote:
> > >> >+static void efi_call_virt_check_flags(unsigned long flags, const char *call)
> > >> >+{
> > >> >+ unsigned long cur_flags;
> > >> >+ bool mismatch;
> > >> >+
> > >> >+ local_save_flags(cur_flags);
> > >> >+
> > >> >+ mismatch = !!((cur_flags ^ flags) & ARCH_EFI_IRQ_FLAGS_MASK);
> > >>
> > >> nit: the assignment itself is already a conversion to bool, so the
> > >> excitement is redundant here.
> > >
> > > This was intentional. I asked Mark to make this change so that it's
> > > explicit for the developer that we're performing the type conversion.
> >
> > But replacing an implicit boolean cast with an explicit one makes
> > little sense, no? Don't we simply want '!= 0' here if you need a
> > boolean expression?
>
> Aha but '!!' is fewer characters to type!!
>
> I'm not that bothered as long as we don't stuff an int into a bool
> without giving the programmer some idea we're doing that. It's not
> about the compiler getting it wrong, more about a developer
> introducing a bug when they change the code in the future.
>
> Unless anyone objects, I'll fix this up to use '!= 0' when I apply it.
I have no strong preference so long as the code is correct.
Another option is to get rid of the bool entirely:
flags ^= cur_flags;
if (!WARN_ON(flags & ARCH_EFI_IRQ_FLAGS_MASK))
return;
Mark.
On 25/04/16 15:24, Matt Fleming wrote:
> On Mon, 25 Apr, at 04:18:41PM, Ard Biesheuvel wrote:
>> On 25 April 2016 at 16:15, Matt Fleming <[email protected]> wrote:
>>> On Mon, 25 Apr, at 03:12:01PM, Robin Murphy wrote:
>>>>> +static void efi_call_virt_check_flags(unsigned long flags, const char *call)
>>>>> +{
>>>>> + unsigned long cur_flags;
>>>>> + bool mismatch;
>>>>> +
>>>>> + local_save_flags(cur_flags);
>>>>> +
>>>>> + mismatch = !!((cur_flags ^ flags) & ARCH_EFI_IRQ_FLAGS_MASK);
>>>>
>>>> nit: the assignment itself is already a conversion to bool, so the
>>>> excitement is redundant here.
>>>
>>> This was intentional. I asked Mark to make this change so that it's
>>> explicit for the developer that we're performing the type conversion.
>>
>> But replacing an implicit boolean cast with an explicit one makes
>> little sense, no? Don't we simply want '!= 0' here if you need a
>> boolean expression?
>
> Aha but '!!' is fewer characters to type!!
>
> I'm not that bothered as long as we don't stuff an int into a bool
> without giving the programmer some idea we're doing that. It's not
> about the compiler getting it wrong, more about a developer
> introducing a bug when they change the code in the future.
>
> Unless anyone objects, I'll fix this up to use '!= 0' when I apply it.
Agreed - the belt and braces approach isn't necessarily bad if the cost
of cocking it up is significant, and !=0 is as explicit as you can get.
After all, if Joe Random Hacker can't infer the behaviour from looking 4
lines up to see the variable definition, then I wouldn't count on him
understanding !! either ;)
Thanks,
Robin.
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
On Mon, 25 Apr, at 03:27:35PM, Mark Rutland wrote:
>
> I have no strong preference so long as the code is correct.
>
> Another option is to get rid of the bool entirely:
>
> flags ^= cur_flags;
> if (!WARN_ON(flags & ARCH_EFI_IRQ_FLAGS_MASK))
> return;
OK, let's do the following because we need flags to be preserved for
printing,
---
unsigned long cur_flags, mismatch;
local_save_flags(cur_flags);
mismatch = flags ^ cur_flags;
if (!WARN_ON_ONCE(mismatch & ARCH_EFI_IRQ_FLAGS_MASK))
return;
On Mon, 25 Apr, at 02:46:29PM, Mark Rutland wrote:
> Note: this is largely a rework of the final patch from v2 [2], which now has a
> per-arch component (and hence additional patches). The rest of v2 has already
> been picked up, and hence dropped from this posting.
>
> Some firmware erroneously unmask IRQs (and potentially other architecture
> specific exceptions) during runtime services functions, in violation of both
> common sense and the UEFI specification. This can result in a number of issues
> if said exceptions are taken when they are expected to be masked, and
> additionally can confuse IRQ tracing if the original mask state is not
> restored prior to returning from firmware.
>
> In practice it's difficult to check that firmware never unmasks exceptions, but
> we can at least check that the IRQ flags are at least consistent upon entry to
> and return from a runtime services function call. This series implements said
> check in the shared EFI runtime wrappers code, after an initial round of
> refactoring (patches 1-5 of [2]).
>
> I have left ia64 as-is, without this check, as ia64 doesn't currently use the
> generic runtime wrappers, has many special cases for the runtime calls which
> don't fit well with the generic code, and I don't expect a new, buggy ia64
> firmware to appear soon.
>
> The first time corruption of the IRQ flags is detected, we dump a stack trace,
> and set TAINT_FIRMWARE_WORKAROUND. Additionally, and in all subsequent cases,
> we log (with ratelimiting) the specific corruption of the flags, and restore
> the expected flags to avoid redundant warnings elsewhere.
Thanks Mark. I've picked up the series and applied it to the v4.7
queue.
On Mon, Apr 25, 2016 at 04:59:22PM +0100, Matt Fleming wrote:
> On Mon, 25 Apr, at 03:27:35PM, Mark Rutland wrote:
> >
> > I have no strong preference so long as the code is correct.
> >
> > Another option is to get rid of the bool entirely:
> >
> > flags ^= cur_flags;
> > if (!WARN_ON(flags & ARCH_EFI_IRQ_FLAGS_MASK))
> > return;
>
> OK, let's do the following because we need flags to be preserved for
> printing,
>
> ---
>
> unsigned long cur_flags, mismatch;
>
> local_save_flags(cur_flags);
>
> mismatch = flags ^ cur_flags;
> if (!WARN_ON_ONCE(mismatch & ARCH_EFI_IRQ_FLAGS_MASK))
> return;
>
Sure; that looks good to me.
Cheers,
Mark.