[PATCH AUTOSEL 4.19 133/191] efi: honour memory reservations passed via a linux specific config table

[PATCH AUTOSEL 4.19 133/191] efi: honour memory reservations passed via a linux specific config table

[Sasha Levin]

From: Ard Biesheuvel <ard.biesheuvel@linaro.org>

[ Upstream commit 71e0940d52e107748b270213a01d3b1546657d74 ]

In order to allow the OS to reserve memory persistently across a
kexec, introduce a Linux-specific UEFI configuration table that
points to the head of a linked list in memory, allowing each kernel
to add list items describing memory regions that the next kernel
should treat as reserved.

This is useful, e.g., for GICv3 based ARM systems that cannot disable
DMA access to the LPI tables, forcing them to reuse the same memory
region again after a kexec reboot.

Tested-by: Jeremy Linton <jeremy.linton@arm.com>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/firmware/efi/efi.c | 27 ++++++++++++++++++++++++++-
 include/linux/efi.h        |  8 ++++++++
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index d54fca902e64f..f265309859781 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -52,7 +52,8 @@ struct efi __read_mostly efi = {
 	.properties_table	= EFI_INVALID_TABLE_ADDR,
 	.mem_attr_table		= EFI_INVALID_TABLE_ADDR,
 	.rng_seed		= EFI_INVALID_TABLE_ADDR,
-	.tpm_log		= EFI_INVALID_TABLE_ADDR
+	.tpm_log		= EFI_INVALID_TABLE_ADDR,
+	.mem_reserve		= EFI_INVALID_TABLE_ADDR,
 };
 EXPORT_SYMBOL(efi);
 
@@ -487,6 +488,7 @@ static __initdata efi_config_table_type_t common_tables[] = {
 	{EFI_MEMORY_ATTRIBUTES_TABLE_GUID, "MEMATTR", &efi.mem_attr_table},
 	{LINUX_EFI_RANDOM_SEED_TABLE_GUID, "RNG", &efi.rng_seed},
 	{LINUX_EFI_TPM_EVENT_LOG_GUID, "TPMEventLog", &efi.tpm_log},
+	{LINUX_EFI_MEMRESERVE_TABLE_GUID, "MEMRESERVE", &efi.mem_reserve},
 	{NULL_GUID, NULL, NULL},
 };
 
@@ -594,6 +596,29 @@ int __init efi_config_parse_tables(void *config_tables, int count, int sz,
 		early_memunmap(tbl, sizeof(*tbl));
 	}
 
+	if (efi.mem_reserve != EFI_INVALID_TABLE_ADDR) {
+		unsigned long prsv = efi.mem_reserve;
+
+		while (prsv) {
+			struct linux_efi_memreserve *rsv;
+
+			/* reserve the entry itself */
+			memblock_reserve(prsv, sizeof(*rsv));
+
+			rsv = early_memremap(prsv, sizeof(*rsv));
+			if (rsv == NULL) {
+				pr_err("Could not map UEFI memreserve entry!\n");
+				return -ENOMEM;
+			}
+
+			if (rsv->size)
+				memblock_reserve(rsv->base, rsv->size);
+
+			prsv = rsv->next;
+			early_memunmap(rsv, sizeof(*rsv));
+		}
+	}
+
 	return 0;
 }
 
diff --git a/include/linux/efi.h b/include/linux/efi.h
index cc3391796c0b8..f43fc61fbe2c9 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -672,6 +672,7 @@ void efi_native_runtime_setup(void);
 #define LINUX_EFI_LOADER_ENTRY_GUID		EFI_GUID(0x4a67b082, 0x0a4c, 0x41cf,  0xb6, 0xc7, 0x44, 0x0b, 0x29, 0xbb, 0x8c, 0x4f)
 #define LINUX_EFI_RANDOM_SEED_TABLE_GUID	EFI_GUID(0x1ce1e5bc, 0x7ceb, 0x42f2,  0x81, 0xe5, 0x8a, 0xad, 0xf1, 0x80, 0xf5, 0x7b)
 #define LINUX_EFI_TPM_EVENT_LOG_GUID		EFI_GUID(0xb7799cb0, 0xeca2, 0x4943,  0x96, 0x67, 0x1f, 0xae, 0x07, 0xb7, 0x47, 0xfa)
+#define LINUX_EFI_MEMRESERVE_TABLE_GUID		EFI_GUID(0x888eb0c6, 0x8ede, 0x4ff5,  0xa8, 0xf0, 0x9a, 0xee, 0x5c, 0xb9, 0x77, 0xc2)
 
 typedef struct {
 	efi_guid_t guid;
@@ -957,6 +958,7 @@ extern struct efi {
 	unsigned long mem_attr_table;	/* memory attributes table */
 	unsigned long rng_seed;		/* UEFI firmware random seed */
 	unsigned long tpm_log;		/* TPM2 Event Log table */
+	unsigned long mem_reserve;	/* Linux EFI memreserve table */
 	efi_get_time_t *get_time;
 	efi_set_time_t *set_time;
 	efi_get_wakeup_time_t *get_wakeup_time;
@@ -1667,4 +1669,10 @@ extern int efi_tpm_eventlog_init(void);
 /* Workqueue to queue EFI Runtime Services */
 extern struct workqueue_struct *efi_rts_wq;
 
+struct linux_efi_memreserve {
+	phys_addr_t	next;
+	phys_addr_t	base;
+	phys_addr_t	size;
+};
+
 #endif /* _LINUX_EFI_H */
-- 
2.20.1

Re: [PATCH AUTOSEL 4.19 133/191] efi: honour memory reservations passed via a linux specific config table

[Ard Biesheuvel]

On Sun, 10 Nov 2019 at 03:44, Sasha Levin <sashal@kernel.org> wrote:
>
> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> [ Upstream commit 71e0940d52e107748b270213a01d3b1546657d74 ]
>
> In order to allow the OS to reserve memory persistently across a
> kexec, introduce a Linux-specific UEFI configuration table that
> points to the head of a linked list in memory, allowing each kernel
> to add list items describing memory regions that the next kernel
> should treat as reserved.
>
> This is useful, e.g., for GICv3 based ARM systems that cannot disable
> DMA access to the LPI tables, forcing them to reuse the same memory
> region again after a kexec reboot.
>
> Tested-by: Jeremy Linton <jeremy.linton@arm.com>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Sasha Levin <sashal@kernel.org>

NAK

This doesn't belong in -stable, and I'd be interested in understanding
how this got autoselected, and how I can prevent this from happening
again in the future.


> ---
>  drivers/firmware/efi/efi.c | 27 ++++++++++++++++++++++++++-
>  include/linux/efi.h        |  8 ++++++++
>  2 files changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index d54fca902e64f..f265309859781 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -52,7 +52,8 @@ struct efi __read_mostly efi = {
>         .properties_table       = EFI_INVALID_TABLE_ADDR,
>         .mem_attr_table         = EFI_INVALID_TABLE_ADDR,
>         .rng_seed               = EFI_INVALID_TABLE_ADDR,
> -       .tpm_log                = EFI_INVALID_TABLE_ADDR
> +       .tpm_log                = EFI_INVALID_TABLE_ADDR,
> +       .mem_reserve            = EFI_INVALID_TABLE_ADDR,
>  };
>  EXPORT_SYMBOL(efi);
>
> @@ -487,6 +488,7 @@ static __initdata efi_config_table_type_t common_tables[] = {
>         {EFI_MEMORY_ATTRIBUTES_TABLE_GUID, "MEMATTR", &efi.mem_attr_table},
>         {LINUX_EFI_RANDOM_SEED_TABLE_GUID, "RNG", &efi.rng_seed},
>         {LINUX_EFI_TPM_EVENT_LOG_GUID, "TPMEventLog", &efi.tpm_log},
> +       {LINUX_EFI_MEMRESERVE_TABLE_GUID, "MEMRESERVE", &efi.mem_reserve},
>         {NULL_GUID, NULL, NULL},
>  };
>
> @@ -594,6 +596,29 @@ int __init efi_config_parse_tables(void *config_tables, int count, int sz,
>                 early_memunmap(tbl, sizeof(*tbl));
>         }
>
> +       if (efi.mem_reserve != EFI_INVALID_TABLE_ADDR) {
> +               unsigned long prsv = efi.mem_reserve;
> +
> +               while (prsv) {
> +                       struct linux_efi_memreserve *rsv;
> +
> +                       /* reserve the entry itself */
> +                       memblock_reserve(prsv, sizeof(*rsv));
> +
> +                       rsv = early_memremap(prsv, sizeof(*rsv));
> +                       if (rsv == NULL) {
> +                               pr_err("Could not map UEFI memreserve entry!\n");
> +                               return -ENOMEM;
> +                       }
> +
> +                       if (rsv->size)
> +                               memblock_reserve(rsv->base, rsv->size);
> +
> +                       prsv = rsv->next;
> +                       early_memunmap(rsv, sizeof(*rsv));
> +               }
> +       }
> +
>         return 0;
>  }
>
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index cc3391796c0b8..f43fc61fbe2c9 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -672,6 +672,7 @@ void efi_native_runtime_setup(void);
>  #define LINUX_EFI_LOADER_ENTRY_GUID            EFI_GUID(0x4a67b082, 0x0a4c, 0x41cf,  0xb6, 0xc7, 0x44, 0x0b, 0x29, 0xbb, 0x8c, 0x4f)
>  #define LINUX_EFI_RANDOM_SEED_TABLE_GUID       EFI_GUID(0x1ce1e5bc, 0x7ceb, 0x42f2,  0x81, 0xe5, 0x8a, 0xad, 0xf1, 0x80, 0xf5, 0x7b)
>  #define LINUX_EFI_TPM_EVENT_LOG_GUID           EFI_GUID(0xb7799cb0, 0xeca2, 0x4943,  0x96, 0x67, 0x1f, 0xae, 0x07, 0xb7, 0x47, 0xfa)
> +#define LINUX_EFI_MEMRESERVE_TABLE_GUID                EFI_GUID(0x888eb0c6, 0x8ede, 0x4ff5,  0xa8, 0xf0, 0x9a, 0xee, 0x5c, 0xb9, 0x77, 0xc2)
>
>  typedef struct {
>         efi_guid_t guid;
> @@ -957,6 +958,7 @@ extern struct efi {
>         unsigned long mem_attr_table;   /* memory attributes table */
>         unsigned long rng_seed;         /* UEFI firmware random seed */
>         unsigned long tpm_log;          /* TPM2 Event Log table */
> +       unsigned long mem_reserve;      /* Linux EFI memreserve table */
>         efi_get_time_t *get_time;
>         efi_set_time_t *set_time;
>         efi_get_wakeup_time_t *get_wakeup_time;
> @@ -1667,4 +1669,10 @@ extern int efi_tpm_eventlog_init(void);
>  /* Workqueue to queue EFI Runtime Services */
>  extern struct workqueue_struct *efi_rts_wq;
>
> +struct linux_efi_memreserve {
> +       phys_addr_t     next;
> +       phys_addr_t     base;
> +       phys_addr_t     size;
> +};
> +
>  #endif /* _LINUX_EFI_H */
> --
> 2.20.1
>

Re: [PATCH AUTOSEL 4.19 133/191] efi: honour memory reservations passed via a linux specific config table

[Sasha Levin]

On Sun, Nov 10, 2019 at 08:33:47AM +0100, Ard Biesheuvel wrote:
>On Sun, 10 Nov 2019 at 03:44, Sasha Levin <sashal@kernel.org> wrote:
>>
>> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>
>> [ Upstream commit 71e0940d52e107748b270213a01d3b1546657d74 ]
>>
>> In order to allow the OS to reserve memory persistently across a
>> kexec, introduce a Linux-specific UEFI configuration table that
>> points to the head of a linked list in memory, allowing each kernel
>> to add list items describing memory regions that the next kernel
>> should treat as reserved.
>>
>> This is useful, e.g., for GICv3 based ARM systems that cannot disable
>> DMA access to the LPI tables, forcing them to reuse the same memory
>> region again after a kexec reboot.
>>
>> Tested-by: Jeremy Linton <jeremy.linton@arm.com>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Signed-off-by: Sasha Levin <sashal@kernel.org>
>
>NAK
>
>This doesn't belong in -stable, and I'd be interested in understanding
>how this got autoselected, and how I can prevent this from happening
>again in the future.

It was selected because it's part of a fix for a real issue reported by
users:

https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1806766

Besides ubuntu, it is also carried by:

SUSE: https://www.suse.com/support/update/announcement/2019/suse-su-20191530-1/
CentOS: https://koji.mbox.centos.org/koji/buildinfo?buildID=4558

As a way to resolve the reported bug.

Any reason this *shouldn't* be in stable? I'm aware that there might be
dependencies that are not obvious to me, but the solution here is to
take those dependencies as well rather than ignore the process
completely.

-- 
Thanks,
Sasha

Re: [PATCH AUTOSEL 4.19 133/191] efi: honour memory reservations passed via a linux specific config table

[Ard Biesheuvel]

On Sun, 10 Nov 2019 at 13:27, Sasha Levin <sashal@kernel.org> wrote:
>
> On Sun, Nov 10, 2019 at 08:33:47AM +0100, Ard Biesheuvel wrote:
> >On Sun, 10 Nov 2019 at 03:44, Sasha Levin <sashal@kernel.org> wrote:
> >>
> >> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >>
> >> [ Upstream commit 71e0940d52e107748b270213a01d3b1546657d74 ]
> >>
> >> In order to allow the OS to reserve memory persistently across a
> >> kexec, introduce a Linux-specific UEFI configuration table that
> >> points to the head of a linked list in memory, allowing each kernel
> >> to add list items describing memory regions that the next kernel
> >> should treat as reserved.
> >>
> >> This is useful, e.g., for GICv3 based ARM systems that cannot disable
> >> DMA access to the LPI tables, forcing them to reuse the same memory
> >> region again after a kexec reboot.
> >>
> >> Tested-by: Jeremy Linton <jeremy.linton@arm.com>
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> Signed-off-by: Sasha Levin <sashal@kernel.org>
> >
> >NAK
> >
> >This doesn't belong in -stable, and I'd be interested in understanding
> >how this got autoselected, and how I can prevent this from happening
> >again in the future.
>
> It was selected because it's part of a fix for a real issue reported by
> users:
>

For my understanding, are you saying your AI is reading launchpad bug
reports etc? Because it is marked AUTOSEL.

> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1806766
>

That pages mentions

"""
2 upstream patch series are required to fix this:
 https://<email address hidden>/msg10328.html
Which provides an EFI facility consumed by:
 https://lkml.org/lkml/2018/9/21/1066
There were also some follow-on fixes to deal with ARM-specific
problems associated with this usage:
 https://www.spinics.net/lists/arm-kernel/msg685751.html
"""

and without the other patches, we only add bugs and don't fix any.

> Besides ubuntu, it is also carried by:
>
> SUSE: https://www.suse.com/support/update/announcement/2019/suse-su-20191530-1/
> CentOS: https://koji.mbox.centos.org/koji/buildinfo?buildID=4558
>
> As a way to resolve the reported bug.
>

Backporting a feature/fix like this requires careful consideration of
the patches involved, and doing actual testing on hardware.

> Any reason this *shouldn't* be in stable?

Yes. By itself, it causes crashes at early boot and does not actually
solve the problem.

> I'm aware that there might be
> dependencies that are not obvious to me, but the solution here is to
> take those dependencies as well rather than ignore the process
> completely.
>

This is not a bugfix. kexec/kdump never worked correctly on the
hardware involved, and backporting a feature like that goes way beyond
what I am willing to accept for stable backports affecting the EFI
subsystem.

Re: [PATCH AUTOSEL 4.19 133/191] efi: honour memory reservations passed via a linux specific config table

[Sasha Levin]

On Sun, Nov 10, 2019 at 02:16:57PM +0000, Ard Biesheuvel wrote:
>On Sun, 10 Nov 2019 at 13:27, Sasha Levin <sashal@kernel.org> wrote:
>>
>> On Sun, Nov 10, 2019 at 08:33:47AM +0100, Ard Biesheuvel wrote:
>> >On Sun, 10 Nov 2019 at 03:44, Sasha Levin <sashal@kernel.org> wrote:
>> >>
>> >> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> >>
>> >> [ Upstream commit 71e0940d52e107748b270213a01d3b1546657d74 ]
>> >>
>> >> In order to allow the OS to reserve memory persistently across a
>> >> kexec, introduce a Linux-specific UEFI configuration table that
>> >> points to the head of a linked list in memory, allowing each kernel
>> >> to add list items describing memory regions that the next kernel
>> >> should treat as reserved.
>> >>
>> >> This is useful, e.g., for GICv3 based ARM systems that cannot disable
>> >> DMA access to the LPI tables, forcing them to reuse the same memory
>> >> region again after a kexec reboot.
>> >>
>> >> Tested-by: Jeremy Linton <jeremy.linton@arm.com>
>> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> >> Signed-off-by: Sasha Levin <sashal@kernel.org>
>> >
>> >NAK
>> >
>> >This doesn't belong in -stable, and I'd be interested in understanding
>> >how this got autoselected, and how I can prevent this from happening
>> >again in the future.
>>
>> It was selected because it's part of a fix for a real issue reported by
>> users:
>>
>
>For my understanding, are you saying your AI is reading launchpad bug
>reports etc? Because it is marked AUTOSEL.

Not quite. This review set was me feeding all the patches Ubuntu has on
top of stable trees into AUTOSEL, and sending out the output for review.
I doesn't look into launchpad bug reports on it's own, but in my
experience one can find a bug report for mostly everything AUTOSEL
considers to be a bug.

>> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1806766
>>
>
>That pages mentions
>
>"""
>2 upstream patch series are required to fix this:
> https://<email address hidden>/msg10328.html
>Which provides an EFI facility consumed by:
> https://lkml.org/lkml/2018/9/21/1066
>There were also some follow-on fixes to deal with ARM-specific
>problems associated with this usage:
> https://www.spinics.net/lists/arm-kernel/msg685751.html
>"""
>
>and without the other patches, we only add bugs and don't fix any.
>
>> Besides ubuntu, it is also carried by:
>>
>> SUSE: https://www.suse.com/support/update/announcement/2019/suse-su-20191530-1/
>> CentOS: https://koji.mbox.centos.org/koji/buildinfo?buildID=4558
>>
>> As a way to resolve the reported bug.
>>
>
>Backporting a feature/fix like this requires careful consideration of
>the patches involved, and doing actual testing on hardware.
>
>> Any reason this *shouldn't* be in stable?
>
>Yes. By itself, it causes crashes at early boot and does not actually
>solve the problem.

Sure, let's work on gathering all the needed patches then and testing it
out.

>> I'm aware that there might be
>> dependencies that are not obvious to me, but the solution here is to
>> take those dependencies as well rather than ignore the process
>> completely.
>>
>
>This is not a bugfix. kexec/kdump never worked correctly on the
>hardware involved, and backporting a feature like that goes way beyond
>what I am willing to accept for stable backports affecting the EFI
>subsystem.

I'm a bit confused. The bug report starts with:

	[Impact]
	kdump support isn't usable on HiSilicon D05 systems. This
	previously worked in bionic.

So it seems like it did use to work, but not anymore?

Either way, I understand that you want to keep the stable tree
conservative, but keep in mind that the flip side of not taking fixes
that users ask for means that distros end up having to carry them
anyway, which means that they don't get the review and testing they
need.

We can argue all we want around whether it's a fix or not, but if most
distros carry it then I think our argument is moot.

-- 
Thanks,
Sasha

Re: [PATCH AUTOSEL 4.19 133/191] efi: honour memory reservations passed via a linux specific config table

[Ard Biesheuvel]

(adding Marc, the GIC maintainer)

On Sun, 10 Nov 2019 at 15:57, Sasha Levin <sashal@kernel.org> wrote:
>
> On Sun, Nov 10, 2019 at 02:16:57PM +0000, Ard Biesheuvel wrote:
> >On Sun, 10 Nov 2019 at 13:27, Sasha Levin <sashal@kernel.org> wrote:
> >>
> >> On Sun, Nov 10, 2019 at 08:33:47AM +0100, Ard Biesheuvel wrote:
> >> >On Sun, 10 Nov 2019 at 03:44, Sasha Levin <sashal@kernel.org> wrote:
> >> >>
> >> >> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> >>
> >> >> [ Upstream commit 71e0940d52e107748b270213a01d3b1546657d74 ]
> >> >>
> >> >> In order to allow the OS to reserve memory persistently across a
> >> >> kexec, introduce a Linux-specific UEFI configuration table that
> >> >> points to the head of a linked list in memory, allowing each kernel
> >> >> to add list items describing memory regions that the next kernel
> >> >> should treat as reserved.
> >> >>
> >> >> This is useful, e.g., for GICv3 based ARM systems that cannot disable
> >> >> DMA access to the LPI tables, forcing them to reuse the same memory
> >> >> region again after a kexec reboot.
> >> >>
> >> >> Tested-by: Jeremy Linton <jeremy.linton@arm.com>
> >> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> >> Signed-off-by: Sasha Levin <sashal@kernel.org>
> >> >
> >> >NAK
> >> >
> >> >This doesn't belong in -stable, and I'd be interested in understanding
> >> >how this got autoselected, and how I can prevent this from happening
> >> >again in the future.
> >>
> >> It was selected because it's part of a fix for a real issue reported by
> >> users:
> >>
> >
> >For my understanding, are you saying your AI is reading launchpad bug
> >reports etc? Because it is marked AUTOSEL.
>
> Not quite. This review set was me feeding all the patches Ubuntu has on
> top of stable trees into AUTOSEL, and sending out the output for review.
> I doesn't look into launchpad bug reports on it's own, but in my
> experience one can find a bug report for mostly everything AUTOSEL
> considers to be a bug.
>

So the assumption is that taking an arbitrary subset of what Ubuntu
backported (and tested extensively), and letting that subset be chosen
by a bot is a process that improves the quality of stable trees? I'm
rather skeptical of that tbh.

> >> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1806766
> >>
> >
> >That pages mentions
> >
> >"""
> >2 upstream patch series are required to fix this:
> > https://<email address hidden>/msg10328.html
> >Which provides an EFI facility consumed by:
> > https://lkml.org/lkml/2018/9/21/1066
> >There were also some follow-on fixes to deal with ARM-specific
> >problems associated with this usage:
> > https://www.spinics.net/lists/arm-kernel/msg685751.html
> >"""
> >
> >and without the other patches, we only add bugs and don't fix any.
> >
> >> Besides ubuntu, it is also carried by:
> >>
> >> SUSE: https://www.suse.com/support/update/announcement/2019/suse-su-20191530-1/
> >> CentOS: https://koji.mbox.centos.org/koji/buildinfo?buildID=4558
> >>
> >> As a way to resolve the reported bug.
> >>
> >
> >Backporting a feature/fix like this requires careful consideration of
> >the patches involved, and doing actual testing on hardware.
> >
> >> Any reason this *shouldn't* be in stable?
> >
> >Yes. By itself, it causes crashes at early boot and does not actually
> >solve the problem.
>
> Sure, let's work on gathering all the needed patches then and testing it
> out.
>

No, let's not. This is a feature that was introduced to address a
shortcoming in some hardware that makes kexec/kdump problematic on
them. If you want kexec/kdump on that hardware, use a newer kernel.

> >> I'm aware that there might be
> >> dependencies that are not obvious to me, but the solution here is to
> >> take those dependencies as well rather than ignore the process
> >> completely.
> >>
> >
> >This is not a bugfix. kexec/kdump never worked correctly on the
> >hardware involved, and backporting a feature like that goes way beyond
> >what I am willing to accept for stable backports affecting the EFI
> >subsystem.
>
> I'm a bit confused. The bug report starts with:
>
>         [Impact]
>         kdump support isn't usable on HiSilicon D05 systems. This
>         previously worked in bionic.
>
> So it seems like it did use to work, but not anymore?
>

I have no idea what Ubuntu shipped in the previous kernel, but
labelling this as a software regression is dubious at least, and
wholly inaccurate for upstream.

> Either way, I understand that you want to keep the stable tree
> conservative, but keep in mind that the flip side of not taking fixes
> that users ask for means that distros end up having to carry them
> anyway, which means that they don't get the review and testing they
> need.
>

I'd say it is the opposite. At least the distros test their backports
on actual hardware. Taking any part of this set without testing it by
doing kexec/kdump on an affected ARM system, and regression testing it
on the hardware that got broken by it (with hundreds of cores IIRC) is
totally irresponsible, and I don't have the time or the hardware to do
the testing.

> We can argue all we want around whether it's a fix or not, but if most
> distros carry it then I think our argument is moot.
>

If someone cares enough to backport these as a coherent set, with boot
tests on the affected hardware etc, then I am not going to object.

Re: [PATCH AUTOSEL 4.19 133/191] efi: honour memory reservations passed via a linux specific config table

[Marc Zyngier]

On Sun, 10 Nov 2019 16:26:15 +0000,
Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> 
> (adding Marc, the GIC maintainer)
> 
> On Sun, 10 Nov 2019 at 15:57, Sasha Levin <sashal@kernel.org> wrote:
> >
> > On Sun, Nov 10, 2019 at 02:16:57PM +0000, Ard Biesheuvel wrote:
> > >On Sun, 10 Nov 2019 at 13:27, Sasha Levin <sashal@kernel.org> wrote:
> > >>
> > >> On Sun, Nov 10, 2019 at 08:33:47AM +0100, Ard Biesheuvel wrote:
> > >> >On Sun, 10 Nov 2019 at 03:44, Sasha Levin <sashal@kernel.org> wrote:
> > >> >>
> > >> >> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > >> >>
> > >> >> [ Upstream commit 71e0940d52e107748b270213a01d3b1546657d74 ]
> > >> >>
> > >> >> In order to allow the OS to reserve memory persistently across a
> > >> >> kexec, introduce a Linux-specific UEFI configuration table that
> > >> >> points to the head of a linked list in memory, allowing each kernel
> > >> >> to add list items describing memory regions that the next kernel
> > >> >> should treat as reserved.
> > >> >>
> > >> >> This is useful, e.g., for GICv3 based ARM systems that cannot disable
> > >> >> DMA access to the LPI tables, forcing them to reuse the same memory
> > >> >> region again after a kexec reboot.
> > >> >>
> > >> >> Tested-by: Jeremy Linton <jeremy.linton@arm.com>
> > >> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > >> >> Signed-off-by: Sasha Levin <sashal@kernel.org>
> > >> >
> > >> >NAK
> > >> >
> > >> >This doesn't belong in -stable, and I'd be interested in understanding
> > >> >how this got autoselected, and how I can prevent this from happening
> > >> >again in the future.
> > >>
> > >> It was selected because it's part of a fix for a real issue reported by
> > >> users:
> > >>
> > >
> > >For my understanding, are you saying your AI is reading launchpad bug
> > >reports etc? Because it is marked AUTOSEL.
> >
> > Not quite. This review set was me feeding all the patches Ubuntu has on
> > top of stable trees into AUTOSEL, and sending out the output for review.
> > I doesn't look into launchpad bug reports on it's own, but in my
> > experience one can find a bug report for mostly everything AUTOSEL
> > considers to be a bug.
> >
> 
> So the assumption is that taking an arbitrary subset of what Ubuntu
> backported (and tested extensively), and letting that subset be chosen
> by a bot is a process that improves the quality of stable trees? I'm
> rather skeptical of that tbh.
> 
> > >> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1806766
> > >>
> > >
> > >That pages mentions
> > >
> > >"""
> > >2 upstream patch series are required to fix this:
> > > https://<email address hidden>/msg10328.html
> > >Which provides an EFI facility consumed by:
> > > https://lkml.org/lkml/2018/9/21/1066
> > >There were also some follow-on fixes to deal with ARM-specific
> > >problems associated with this usage:
> > > https://www.spinics.net/lists/arm-kernel/msg685751.html
> > >"""
> > >
> > >and without the other patches, we only add bugs and don't fix any.
> > >
> > >> Besides ubuntu, it is also carried by:
> > >>
> > >> SUSE: https://www.suse.com/support/update/announcement/2019/suse-su-20191530-1/
> > >> CentOS: https://koji.mbox.centos.org/koji/buildinfo?buildID=4558
> > >>
> > >> As a way to resolve the reported bug.
> > >>
> > >
> > >Backporting a feature/fix like this requires careful consideration of
> > >the patches involved, and doing actual testing on hardware.
> > >
> > >> Any reason this *shouldn't* be in stable?
> > >
> > >Yes. By itself, it causes crashes at early boot and does not actually
> > >solve the problem.
> >
> > Sure, let's work on gathering all the needed patches then and testing it
> > out.
> >
> 
> No, let's not. This is a feature that was introduced to address a
> shortcoming in some hardware that makes kexec/kdump problematic on
> them. If you want kexec/kdump on that hardware, use a newer kernel.

That's my position as well. This isn't a bug fix at all, but a
workaround for a HW defect with complex dependencies. It wasn't cc'd
stable for good reasons, and I wish stable maintainers would respect
this decision.

> > >> I'm aware that there might be
> > >> dependencies that are not obvious to me, but the solution here is to
> > >> take those dependencies as well rather than ignore the process
> > >> completely.
> > >>
> > >
> > >This is not a bugfix. kexec/kdump never worked correctly on the
> > >hardware involved, and backporting a feature like that goes way beyond
> > >what I am willing to accept for stable backports affecting the EFI
> > >subsystem.
> >
> > I'm a bit confused. The bug report starts with:
> >
> >         [Impact]
> >         kdump support isn't usable on HiSilicon D05 systems. This
> >         previously worked in bionic.
> >
> > So it seems like it did use to work, but not anymore?
> >
> 
> I have no idea what Ubuntu shipped in the previous kernel, but
> labelling this as a software regression is dubious at least, and
> wholly inaccurate for upstream.

I have this exact machine keeping my feet warm, and kexec never worked
on it (nor on any GICv3 machine that is LPI-capable) before we added
this workaround.  Whatever distros carried as hacks to make it work at
the time, I don't want to know. What is more likely is that they always
kexec'd a kernel with the exact same allocation layout and that it
worked by luck (or maybe resulted in silent memory corruption).

If anything, I'd merge a patch *disabling* kexec altogether on these
systems, because pretending that it ever worked is a damn lie.

> > Either way, I understand that you want to keep the stable tree
> > conservative, but keep in mind that the flip side of not taking fixes
> > that users ask for means that distros end up having to carry them
> > anyway, which means that they don't get the review and testing they
> > need.
> >
> 
> I'd say it is the opposite. At least the distros test their backports
> on actual hardware. Taking any part of this set without testing it by
> doing kexec/kdump on an affected ARM system, and regression testing it
> on the hardware that got broken by it (with hundreds of cores IIRC) is
> totally irresponsible, and I don't have the time or the hardware to do
> the testing.
> 
> > We can argue all we want around whether it's a fix or not, but if most
> > distros carry it then I think our argument is moot.
> >
> 
> If someone cares enough to backport these as a coherent set, with boot
> tests on the affected hardware etc, then I am not going to object.

Ideally, I'd like the distros to test these backports, because only them
have access to the variety of HW that's required. But they'd have to
majorly up their game, if the above is anything to go by...

Thanks,

	M.

-- 
Jazz is not dead, it just smells funny.

[PATCH AUTOSEL 4.19 134/191] efi: Make efi_rts_work accessible to efi page fault handler

[Sasha Levin]

From: Sai Praneeth <sai.praneeth.prakhya@intel.com>

[ Upstream commit 9dbbedaa6171247c4c7c40b83f05b200a117c2e0 ]

After the kernel has booted, if any accesses by firmware causes a page
fault, the efi page fault handler would freeze efi_rts_wq and schedules
a new process. To do this, the efi page fault handler needs
efi_rts_work. Hence, make it accessible.

There will be no race conditions in accessing this structure, because
all the calls to efi runtime services are already serialized.

Tested-by: Bhupesh Sharma <bhsharma@redhat.com>
Suggested-by: Matt Fleming <matt@codeblueprint.co.uk>
Based-on-code-from: Ricardo Neri <ricardo.neri@intel.com>
Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/firmware/efi/runtime-wrappers.c | 53 +++++--------------------
 include/linux/efi.h                     | 36 +++++++++++++++++
 2 files changed, 45 insertions(+), 44 deletions(-)

diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index 1606abead22cc..b31e3d3729a6d 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -45,39 +45,7 @@
 #define __efi_call_virt(f, args...) \
 	__efi_call_virt_pointer(efi.systab->runtime, f, args)
 
-/* efi_runtime_service() function identifiers */
-enum efi_rts_ids {
-	GET_TIME,
-	SET_TIME,
-	GET_WAKEUP_TIME,
-	SET_WAKEUP_TIME,
-	GET_VARIABLE,
-	GET_NEXT_VARIABLE,
-	SET_VARIABLE,
-	QUERY_VARIABLE_INFO,
-	GET_NEXT_HIGH_MONO_COUNT,
-	UPDATE_CAPSULE,
-	QUERY_CAPSULE_CAPS,
-};
-
-/*
- * efi_runtime_work:	Details of EFI Runtime Service work
- * @arg<1-5>:		EFI Runtime Service function arguments
- * @status:		Status of executing EFI Runtime Service
- * @efi_rts_id:		EFI Runtime Service function identifier
- * @efi_rts_comp:	Struct used for handling completions
- */
-struct efi_runtime_work {
-	void *arg1;
-	void *arg2;
-	void *arg3;
-	void *arg4;
-	void *arg5;
-	efi_status_t status;
-	struct work_struct work;
-	enum efi_rts_ids efi_rts_id;
-	struct completion efi_rts_comp;
-};
+struct efi_runtime_work efi_rts_work;
 
 /*
  * efi_queue_work:	Queue efi_runtime_service() and wait until it's done
@@ -91,7 +59,6 @@ struct efi_runtime_work {
  */
 #define efi_queue_work(_rts, _arg1, _arg2, _arg3, _arg4, _arg5)		\
 ({									\
-	struct efi_runtime_work efi_rts_work;				\
 	efi_rts_work.status = EFI_ABORTED;				\
 									\
 	init_completion(&efi_rts_work.efi_rts_comp);			\
@@ -191,18 +158,16 @@ extern struct semaphore __efi_uv_runtime_lock __alias(efi_runtime_lock);
  */
 static void efi_call_rts(struct work_struct *work)
 {
-	struct efi_runtime_work *efi_rts_work;
 	void *arg1, *arg2, *arg3, *arg4, *arg5;
 	efi_status_t status = EFI_NOT_FOUND;
 
-	efi_rts_work = container_of(work, struct efi_runtime_work, work);
-	arg1 = efi_rts_work->arg1;
-	arg2 = efi_rts_work->arg2;
-	arg3 = efi_rts_work->arg3;
-	arg4 = efi_rts_work->arg4;
-	arg5 = efi_rts_work->arg5;
+	arg1 = efi_rts_work.arg1;
+	arg2 = efi_rts_work.arg2;
+	arg3 = efi_rts_work.arg3;
+	arg4 = efi_rts_work.arg4;
+	arg5 = efi_rts_work.arg5;
 
-	switch (efi_rts_work->efi_rts_id) {
+	switch (efi_rts_work.efi_rts_id) {
 	case GET_TIME:
 		status = efi_call_virt(get_time, (efi_time_t *)arg1,
 				       (efi_time_cap_t *)arg2);
@@ -260,8 +225,8 @@ static void efi_call_rts(struct work_struct *work)
 		 */
 		pr_err("Requested executing invalid EFI Runtime Service.\n");
 	}
-	efi_rts_work->status = status;
-	complete(&efi_rts_work->efi_rts_comp);
+	efi_rts_work.status = status;
+	complete(&efi_rts_work.efi_rts_comp);
 }
 
 static efi_status_t virt_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc)
diff --git a/include/linux/efi.h b/include/linux/efi.h
index f43fc61fbe2c9..9d4c25090fd04 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1666,6 +1666,42 @@ struct linux_efi_tpm_eventlog {
 
 extern int efi_tpm_eventlog_init(void);
 
+/* efi_runtime_service() function identifiers */
+enum efi_rts_ids {
+	GET_TIME,
+	SET_TIME,
+	GET_WAKEUP_TIME,
+	SET_WAKEUP_TIME,
+	GET_VARIABLE,
+	GET_NEXT_VARIABLE,
+	SET_VARIABLE,
+	QUERY_VARIABLE_INFO,
+	GET_NEXT_HIGH_MONO_COUNT,
+	UPDATE_CAPSULE,
+	QUERY_CAPSULE_CAPS,
+};
+
+/*
+ * efi_runtime_work:	Details of EFI Runtime Service work
+ * @arg<1-5>:		EFI Runtime Service function arguments
+ * @status:		Status of executing EFI Runtime Service
+ * @efi_rts_id:		EFI Runtime Service function identifier
+ * @efi_rts_comp:	Struct used for handling completions
+ */
+struct efi_runtime_work {
+	void *arg1;
+	void *arg2;
+	void *arg3;
+	void *arg4;
+	void *arg5;
+	efi_status_t status;
+	struct work_struct work;
+	enum efi_rts_ids efi_rts_id;
+	struct completion efi_rts_comp;
+};
+
+extern struct efi_runtime_work efi_rts_work;
+
 /* Workqueue to queue EFI Runtime Services */
 extern struct workqueue_struct *efi_rts_wq;
 
-- 
2.20.1

Re: [PATCH AUTOSEL 4.19 134/191] efi: Make efi_rts_work accessible to efi page fault handler

[Ard Biesheuvel]

On Sun, 10 Nov 2019 at 03:44, Sasha Levin <sashal@kernel.org> wrote:
>
> From: Sai Praneeth <sai.praneeth.prakhya@intel.com>
>
> [ Upstream commit 9dbbedaa6171247c4c7c40b83f05b200a117c2e0 ]
>
> After the kernel has booted, if any accesses by firmware causes a page
> fault, the efi page fault handler would freeze efi_rts_wq and schedules
> a new process. To do this, the efi page fault handler needs
> efi_rts_work. Hence, make it accessible.
>
> There will be no race conditions in accessing this structure, because
> all the calls to efi runtime services are already serialized.
>
> Tested-by: Bhupesh Sharma <bhsharma@redhat.com>
> Suggested-by: Matt Fleming <matt@codeblueprint.co.uk>
> Based-on-code-from: Ricardo Neri <ricardo.neri@intel.com>
> Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Sasha Levin <sashal@kernel.org>

NAK

Would it be possible to disregard all EFI patches for -stable unless
they have a fixes tag? EFI subtly depends on lots of firmware quirks
across various architectures, and so grabbing random patches and
backporting them is really not a good idea in general.

> ---
>  drivers/firmware/efi/runtime-wrappers.c | 53 +++++--------------------
>  include/linux/efi.h                     | 36 +++++++++++++++++
>  2 files changed, 45 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
> index 1606abead22cc..b31e3d3729a6d 100644
> --- a/drivers/firmware/efi/runtime-wrappers.c
> +++ b/drivers/firmware/efi/runtime-wrappers.c
> @@ -45,39 +45,7 @@
>  #define __efi_call_virt(f, args...) \
>         __efi_call_virt_pointer(efi.systab->runtime, f, args)
>
> -/* efi_runtime_service() function identifiers */
> -enum efi_rts_ids {
> -       GET_TIME,
> -       SET_TIME,
> -       GET_WAKEUP_TIME,
> -       SET_WAKEUP_TIME,
> -       GET_VARIABLE,
> -       GET_NEXT_VARIABLE,
> -       SET_VARIABLE,
> -       QUERY_VARIABLE_INFO,
> -       GET_NEXT_HIGH_MONO_COUNT,
> -       UPDATE_CAPSULE,
> -       QUERY_CAPSULE_CAPS,
> -};
> -
> -/*
> - * efi_runtime_work:   Details of EFI Runtime Service work
> - * @arg<1-5>:          EFI Runtime Service function arguments
> - * @status:            Status of executing EFI Runtime Service
> - * @efi_rts_id:                EFI Runtime Service function identifier
> - * @efi_rts_comp:      Struct used for handling completions
> - */
> -struct efi_runtime_work {
> -       void *arg1;
> -       void *arg2;
> -       void *arg3;
> -       void *arg4;
> -       void *arg5;
> -       efi_status_t status;
> -       struct work_struct work;
> -       enum efi_rts_ids efi_rts_id;
> -       struct completion efi_rts_comp;
> -};
> +struct efi_runtime_work efi_rts_work;
>
>  /*
>   * efi_queue_work:     Queue efi_runtime_service() and wait until it's done
> @@ -91,7 +59,6 @@ struct efi_runtime_work {
>   */
>  #define efi_queue_work(_rts, _arg1, _arg2, _arg3, _arg4, _arg5)                \
>  ({                                                                     \
> -       struct efi_runtime_work efi_rts_work;                           \
>         efi_rts_work.status = EFI_ABORTED;                              \
>                                                                         \
>         init_completion(&efi_rts_work.efi_rts_comp);                    \
> @@ -191,18 +158,16 @@ extern struct semaphore __efi_uv_runtime_lock __alias(efi_runtime_lock);
>   */
>  static void efi_call_rts(struct work_struct *work)
>  {
> -       struct efi_runtime_work *efi_rts_work;
>         void *arg1, *arg2, *arg3, *arg4, *arg5;
>         efi_status_t status = EFI_NOT_FOUND;
>
> -       efi_rts_work = container_of(work, struct efi_runtime_work, work);
> -       arg1 = efi_rts_work->arg1;
> -       arg2 = efi_rts_work->arg2;
> -       arg3 = efi_rts_work->arg3;
> -       arg4 = efi_rts_work->arg4;
> -       arg5 = efi_rts_work->arg5;
> +       arg1 = efi_rts_work.arg1;
> +       arg2 = efi_rts_work.arg2;
> +       arg3 = efi_rts_work.arg3;
> +       arg4 = efi_rts_work.arg4;
> +       arg5 = efi_rts_work.arg5;
>
> -       switch (efi_rts_work->efi_rts_id) {
> +       switch (efi_rts_work.efi_rts_id) {
>         case GET_TIME:
>                 status = efi_call_virt(get_time, (efi_time_t *)arg1,
>                                        (efi_time_cap_t *)arg2);
> @@ -260,8 +225,8 @@ static void efi_call_rts(struct work_struct *work)
>                  */
>                 pr_err("Requested executing invalid EFI Runtime Service.\n");
>         }
> -       efi_rts_work->status = status;
> -       complete(&efi_rts_work->efi_rts_comp);
> +       efi_rts_work.status = status;
> +       complete(&efi_rts_work.efi_rts_comp);
>  }
>
>  static efi_status_t virt_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc)
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index f43fc61fbe2c9..9d4c25090fd04 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1666,6 +1666,42 @@ struct linux_efi_tpm_eventlog {
>
>  extern int efi_tpm_eventlog_init(void);
>
> +/* efi_runtime_service() function identifiers */
> +enum efi_rts_ids {
> +       GET_TIME,
> +       SET_TIME,
> +       GET_WAKEUP_TIME,
> +       SET_WAKEUP_TIME,
> +       GET_VARIABLE,
> +       GET_NEXT_VARIABLE,
> +       SET_VARIABLE,
> +       QUERY_VARIABLE_INFO,
> +       GET_NEXT_HIGH_MONO_COUNT,
> +       UPDATE_CAPSULE,
> +       QUERY_CAPSULE_CAPS,
> +};
> +
> +/*
> + * efi_runtime_work:   Details of EFI Runtime Service work
> + * @arg<1-5>:          EFI Runtime Service function arguments
> + * @status:            Status of executing EFI Runtime Service
> + * @efi_rts_id:                EFI Runtime Service function identifier
> + * @efi_rts_comp:      Struct used for handling completions
> + */
> +struct efi_runtime_work {
> +       void *arg1;
> +       void *arg2;
> +       void *arg3;
> +       void *arg4;
> +       void *arg5;
> +       efi_status_t status;
> +       struct work_struct work;
> +       enum efi_rts_ids efi_rts_id;
> +       struct completion efi_rts_comp;
> +};
> +
> +extern struct efi_runtime_work efi_rts_work;
> +
>  /* Workqueue to queue EFI Runtime Services */
>  extern struct workqueue_struct *efi_rts_wq;
>
> --
> 2.20.1
>

[PATCH AUTOSEL 4.19 135/191] efi/x86: Handle page faults occurring while running EFI runtime services

[Sasha Levin]

From: Sai Praneeth <sai.praneeth.prakhya@intel.com>

[ Upstream commit 3425d934fc0312f62024163736a7afe4de20c10f ]

Memory accesses performed by UEFI runtime services should be limited to:
- reading/executing from EFI_RUNTIME_SERVICES_CODE memory regions
- reading/writing from/to EFI_RUNTIME_SERVICES_DATA memory regions
- reading/writing by-ref arguments
- reading/writing from/to the stack.

Accesses outside these regions may cause the kernel to hang because the
memory region requested by the firmware isn't mapped in efi_pgd, which
causes a page fault in ring 0 and the kernel fails to handle it, leading
to die(). To save kernel from hanging, add an EFI specific page fault
handler which recovers from such faults by
1. If the efi runtime service is efi_reset_system(), reboot the machine
   through BIOS.
2. If the efi runtime service is _not_ efi_reset_system(), then freeze
   efi_rts_wq and schedule a new process.

The EFI page fault handler offers us two advantages:
1. Avoid potential hangs caused by buggy firmware.
2. Shout loud that the firmware is buggy and hence is not a kernel bug.

Tested-by: Bhupesh Sharma <bhsharma@redhat.com>
Suggested-by: Matt Fleming <matt@codeblueprint.co.uk>
Based-on-code-from: Ricardo Neri <ricardo.neri@intel.com>
Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
[ardb: clarify commit log]
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 arch/x86/include/asm/efi.h              |  1 +
 arch/x86/mm/fault.c                     |  9 +++
 arch/x86/platform/efi/quirks.c          | 78 +++++++++++++++++++++++++
 drivers/firmware/efi/runtime-wrappers.c |  8 +++
 include/linux/efi.h                     |  8 ++-
 5 files changed, 103 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index baa549f8e9188..45864898f7e50 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -138,6 +138,7 @@ extern void __init efi_apply_memmap_quirks(void);
 extern int __init efi_reuse_config(u64 tables, int nr_tables);
 extern void efi_delete_dummy_variable(void);
 extern void efi_switch_mm(struct mm_struct *mm);
+extern void efi_recover_from_page_fault(unsigned long phys_addr);
 
 struct efi_setup_data {
 	u64 fw_vendor;
diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index 1bcb7242ad79a..53e69c7c5cd18 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -16,6 +16,7 @@
 #include <linux/prefetch.h>		/* prefetchw			*/
 #include <linux/context_tracking.h>	/* exception_enter(), ...	*/
 #include <linux/uaccess.h>		/* faulthandler_disabled()	*/
+#include <linux/efi.h>			/* efi_recover_from_page_fault()*/
 #include <linux/mm_types.h>
 
 #include <asm/cpufeature.h>		/* boot_cpu_has, ...		*/
@@ -25,6 +26,7 @@
 #include <asm/vsyscall.h>		/* emulate_vsyscall		*/
 #include <asm/vm86.h>			/* struct vm86			*/
 #include <asm/mmu_context.h>		/* vma_pkey()			*/
+#include <asm/efi.h>			/* efi_recover_from_page_fault()*/
 
 #define CREATE_TRACE_POINTS
 #include <asm/trace/exceptions.h>
@@ -783,6 +785,13 @@ no_context(struct pt_regs *regs, unsigned long error_code,
 	if (is_errata93(regs, address))
 		return;
 
+	/*
+	 * Buggy firmware could access regions which might page fault, try to
+	 * recover from such faults.
+	 */
+	if (IS_ENABLED(CONFIG_EFI))
+		efi_recover_from_page_fault(address);
+
 	/*
 	 * Oops. The kernel tried to access some bad page. We'll have to
 	 * terminate things with extreme prejudice:
diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index 844d31cb8a0c7..669babcaf245a 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -16,6 +16,7 @@
 #include <asm/efi.h>
 #include <asm/uv/uv.h>
 #include <asm/cpu_device_id.h>
+#include <asm/reboot.h>
 
 #define EFI_MIN_RESERVE 5120
 
@@ -654,3 +655,80 @@ int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
 }
 
 #endif
+
+/*
+ * If any access by any efi runtime service causes a page fault, then,
+ * 1. If it's efi_reset_system(), reboot through BIOS.
+ * 2. If any other efi runtime service, then
+ *    a. Return error status to the efi caller process.
+ *    b. Disable EFI Runtime Services forever and
+ *    c. Freeze efi_rts_wq and schedule new process.
+ *
+ * @return: Returns, if the page fault is not handled. This function
+ * will never return if the page fault is handled successfully.
+ */
+void efi_recover_from_page_fault(unsigned long phys_addr)
+{
+	if (!IS_ENABLED(CONFIG_X86_64))
+		return;
+
+	/*
+	 * Make sure that an efi runtime service caused the page fault.
+	 * "efi_mm" cannot be used to check if the page fault had occurred
+	 * in the firmware context because efi=old_map doesn't use efi_pgd.
+	 */
+	if (efi_rts_work.efi_rts_id == NONE)
+		return;
+
+	/*
+	 * Address range 0x0000 - 0x0fff is always mapped in the efi_pgd, so
+	 * page faulting on these addresses isn't expected.
+	 */
+	if (phys_addr >= 0x0000 && phys_addr <= 0x0fff)
+		return;
+
+	/*
+	 * Print stack trace as it might be useful to know which EFI Runtime
+	 * Service is buggy.
+	 */
+	WARN(1, FW_BUG "Page fault caused by firmware at PA: 0x%lx\n",
+	     phys_addr);
+
+	/*
+	 * Buggy efi_reset_system() is handled differently from other EFI
+	 * Runtime Services as it doesn't use efi_rts_wq. Although,
+	 * native_machine_emergency_restart() says that machine_real_restart()
+	 * could fail, it's better not to compilcate this fault handler
+	 * because this case occurs *very* rarely and hence could be improved
+	 * on a need by basis.
+	 */
+	if (efi_rts_work.efi_rts_id == RESET_SYSTEM) {
+		pr_info("efi_reset_system() buggy! Reboot through BIOS\n");
+		machine_real_restart(MRR_BIOS);
+		return;
+	}
+
+	/*
+	 * Before calling EFI Runtime Service, the kernel has switched the
+	 * calling process to efi_mm. Hence, switch back to task_mm.
+	 */
+	arch_efi_call_virt_teardown();
+
+	/* Signal error status to the efi caller process */
+	efi_rts_work.status = EFI_ABORTED;
+	complete(&efi_rts_work.efi_rts_comp);
+
+	clear_bit(EFI_RUNTIME_SERVICES, &efi.flags);
+	pr_info("Froze efi_rts_wq and disabled EFI Runtime Services\n");
+
+	/*
+	 * Call schedule() in an infinite loop, so that any spurious wake ups
+	 * will never run efi_rts_wq again.
+	 */
+	for (;;) {
+		set_current_state(TASK_IDLE);
+		schedule();
+	}
+
+	return;
+}
diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index b31e3d3729a6d..e2abfdb5cee6a 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -61,6 +61,11 @@ struct efi_runtime_work efi_rts_work;
 ({									\
 	efi_rts_work.status = EFI_ABORTED;				\
 									\
+	if (!efi_enabled(EFI_RUNTIME_SERVICES)) {			\
+		pr_warn_once("EFI Runtime Services are disabled!\n");	\
+		goto exit;						\
+	}								\
+									\
 	init_completion(&efi_rts_work.efi_rts_comp);			\
 	INIT_WORK(&efi_rts_work.work, efi_call_rts);			\
 	efi_rts_work.arg1 = _arg1;					\
@@ -79,6 +84,8 @@ struct efi_runtime_work efi_rts_work;
 	else								\
 		pr_err("Failed to queue work to efi_rts_wq.\n");	\
 									\
+exit:									\
+	efi_rts_work.efi_rts_id = NONE;					\
 	efi_rts_work.status;						\
 })
 
@@ -400,6 +407,7 @@ static void virt_efi_reset_system(int reset_type,
 			"could not get exclusive access to the firmware\n");
 		return;
 	}
+	efi_rts_work.efi_rts_id = RESET_SYSTEM;
 	__efi_call_virt(reset_system, reset_type, status, data_size, data);
 	up(&efi_runtime_lock);
 }
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 9d4c25090fd04..9a86f467b8911 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1666,8 +1666,13 @@ struct linux_efi_tpm_eventlog {
 
 extern int efi_tpm_eventlog_init(void);
 
-/* efi_runtime_service() function identifiers */
+/*
+ * efi_runtime_service() function identifiers.
+ * "NONE" is used by efi_recover_from_page_fault() to check if the page
+ * fault happened while executing an efi runtime service.
+ */
 enum efi_rts_ids {
+	NONE,
 	GET_TIME,
 	SET_TIME,
 	GET_WAKEUP_TIME,
@@ -1677,6 +1682,7 @@ enum efi_rts_ids {
 	SET_VARIABLE,
 	QUERY_VARIABLE_INFO,
 	GET_NEXT_HIGH_MONO_COUNT,
+	RESET_SYSTEM,
 	UPDATE_CAPSULE,
 	QUERY_CAPSULE_CAPS,
 };
-- 
2.20.1

Re: [PATCH AUTOSEL 4.19 135/191] efi/x86: Handle page faults occurring while running EFI runtime services

[Ard Biesheuvel]

On Sun, 10 Nov 2019 at 03:44, Sasha Levin <sashal@kernel.org> wrote:
>
> From: Sai Praneeth <sai.praneeth.prakhya@intel.com>
>
> [ Upstream commit 3425d934fc0312f62024163736a7afe4de20c10f ]
>
> Memory accesses performed by UEFI runtime services should be limited to:
> - reading/executing from EFI_RUNTIME_SERVICES_CODE memory regions
> - reading/writing from/to EFI_RUNTIME_SERVICES_DATA memory regions
> - reading/writing by-ref arguments
> - reading/writing from/to the stack.
>
> Accesses outside these regions may cause the kernel to hang because the
> memory region requested by the firmware isn't mapped in efi_pgd, which
> causes a page fault in ring 0 and the kernel fails to handle it, leading
> to die(). To save kernel from hanging, add an EFI specific page fault
> handler which recovers from such faults by
> 1. If the efi runtime service is efi_reset_system(), reboot the machine
>    through BIOS.
> 2. If the efi runtime service is _not_ efi_reset_system(), then freeze
>    efi_rts_wq and schedule a new process.
>
> The EFI page fault handler offers us two advantages:
> 1. Avoid potential hangs caused by buggy firmware.
> 2. Shout loud that the firmware is buggy and hence is not a kernel bug.
>
> Tested-by: Bhupesh Sharma <bhsharma@redhat.com>
> Suggested-by: Matt Fleming <matt@codeblueprint.co.uk>
> Based-on-code-from: Ricardo Neri <ricardo.neri@intel.com>
> Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
> [ardb: clarify commit log]
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Sasha Levin <sashal@kernel.org>

NAK

This backports a patch to -stable that is *known* to be broken, given
that the same series backports a patch that fixes it.


> ---
>  arch/x86/include/asm/efi.h              |  1 +
>  arch/x86/mm/fault.c                     |  9 +++
>  arch/x86/platform/efi/quirks.c          | 78 +++++++++++++++++++++++++
>  drivers/firmware/efi/runtime-wrappers.c |  8 +++
>  include/linux/efi.h                     |  8 ++-
>  5 files changed, 103 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
> index baa549f8e9188..45864898f7e50 100644
> --- a/arch/x86/include/asm/efi.h
> +++ b/arch/x86/include/asm/efi.h
> @@ -138,6 +138,7 @@ extern void __init efi_apply_memmap_quirks(void);
>  extern int __init efi_reuse_config(u64 tables, int nr_tables);
>  extern void efi_delete_dummy_variable(void);
>  extern void efi_switch_mm(struct mm_struct *mm);
> +extern void efi_recover_from_page_fault(unsigned long phys_addr);
>
>  struct efi_setup_data {
>         u64 fw_vendor;
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index 1bcb7242ad79a..53e69c7c5cd18 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -16,6 +16,7 @@
>  #include <linux/prefetch.h>            /* prefetchw                    */
>  #include <linux/context_tracking.h>    /* exception_enter(), ...       */
>  #include <linux/uaccess.h>             /* faulthandler_disabled()      */
> +#include <linux/efi.h>                 /* efi_recover_from_page_fault()*/
>  #include <linux/mm_types.h>
>
>  #include <asm/cpufeature.h>            /* boot_cpu_has, ...            */
> @@ -25,6 +26,7 @@
>  #include <asm/vsyscall.h>              /* emulate_vsyscall             */
>  #include <asm/vm86.h>                  /* struct vm86                  */
>  #include <asm/mmu_context.h>           /* vma_pkey()                   */
> +#include <asm/efi.h>                   /* efi_recover_from_page_fault()*/
>
>  #define CREATE_TRACE_POINTS
>  #include <asm/trace/exceptions.h>
> @@ -783,6 +785,13 @@ no_context(struct pt_regs *regs, unsigned long error_code,
>         if (is_errata93(regs, address))
>                 return;
>
> +       /*
> +        * Buggy firmware could access regions which might page fault, try to
> +        * recover from such faults.
> +        */
> +       if (IS_ENABLED(CONFIG_EFI))
> +               efi_recover_from_page_fault(address);
> +
>         /*
>          * Oops. The kernel tried to access some bad page. We'll have to
>          * terminate things with extreme prejudice:
> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
> index 844d31cb8a0c7..669babcaf245a 100644
> --- a/arch/x86/platform/efi/quirks.c
> +++ b/arch/x86/platform/efi/quirks.c
> @@ -16,6 +16,7 @@
>  #include <asm/efi.h>
>  #include <asm/uv/uv.h>
>  #include <asm/cpu_device_id.h>
> +#include <asm/reboot.h>
>
>  #define EFI_MIN_RESERVE 5120
>
> @@ -654,3 +655,80 @@ int efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff,
>  }
>
>  #endif
> +
> +/*
> + * If any access by any efi runtime service causes a page fault, then,
> + * 1. If it's efi_reset_system(), reboot through BIOS.
> + * 2. If any other efi runtime service, then
> + *    a. Return error status to the efi caller process.
> + *    b. Disable EFI Runtime Services forever and
> + *    c. Freeze efi_rts_wq and schedule new process.
> + *
> + * @return: Returns, if the page fault is not handled. This function
> + * will never return if the page fault is handled successfully.
> + */
> +void efi_recover_from_page_fault(unsigned long phys_addr)
> +{
> +       if (!IS_ENABLED(CONFIG_X86_64))
> +               return;
> +
> +       /*
> +        * Make sure that an efi runtime service caused the page fault.
> +        * "efi_mm" cannot be used to check if the page fault had occurred
> +        * in the firmware context because efi=old_map doesn't use efi_pgd.
> +        */
> +       if (efi_rts_work.efi_rts_id == NONE)
> +               return;
> +
> +       /*
> +        * Address range 0x0000 - 0x0fff is always mapped in the efi_pgd, so
> +        * page faulting on these addresses isn't expected.
> +        */
> +       if (phys_addr >= 0x0000 && phys_addr <= 0x0fff)
> +               return;
> +
> +       /*
> +        * Print stack trace as it might be useful to know which EFI Runtime
> +        * Service is buggy.
> +        */
> +       WARN(1, FW_BUG "Page fault caused by firmware at PA: 0x%lx\n",
> +            phys_addr);
> +
> +       /*
> +        * Buggy efi_reset_system() is handled differently from other EFI
> +        * Runtime Services as it doesn't use efi_rts_wq. Although,
> +        * native_machine_emergency_restart() says that machine_real_restart()
> +        * could fail, it's better not to compilcate this fault handler
> +        * because this case occurs *very* rarely and hence could be improved
> +        * on a need by basis.
> +        */
> +       if (efi_rts_work.efi_rts_id == RESET_SYSTEM) {
> +               pr_info("efi_reset_system() buggy! Reboot through BIOS\n");
> +               machine_real_restart(MRR_BIOS);
> +               return;
> +       }
> +
> +       /*
> +        * Before calling EFI Runtime Service, the kernel has switched the
> +        * calling process to efi_mm. Hence, switch back to task_mm.
> +        */
> +       arch_efi_call_virt_teardown();
> +
> +       /* Signal error status to the efi caller process */
> +       efi_rts_work.status = EFI_ABORTED;
> +       complete(&efi_rts_work.efi_rts_comp);
> +
> +       clear_bit(EFI_RUNTIME_SERVICES, &efi.flags);
> +       pr_info("Froze efi_rts_wq and disabled EFI Runtime Services\n");
> +
> +       /*
> +        * Call schedule() in an infinite loop, so that any spurious wake ups
> +        * will never run efi_rts_wq again.
> +        */
> +       for (;;) {
> +               set_current_state(TASK_IDLE);
> +               schedule();
> +       }
> +
> +       return;
> +}
> diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
> index b31e3d3729a6d..e2abfdb5cee6a 100644
> --- a/drivers/firmware/efi/runtime-wrappers.c
> +++ b/drivers/firmware/efi/runtime-wrappers.c
> @@ -61,6 +61,11 @@ struct efi_runtime_work efi_rts_work;
>  ({                                                                     \
>         efi_rts_work.status = EFI_ABORTED;                              \
>                                                                         \
> +       if (!efi_enabled(EFI_RUNTIME_SERVICES)) {                       \
> +               pr_warn_once("EFI Runtime Services are disabled!\n");   \
> +               goto exit;                                              \
> +       }                                                               \
> +                                                                       \
>         init_completion(&efi_rts_work.efi_rts_comp);                    \
>         INIT_WORK(&efi_rts_work.work, efi_call_rts);                    \
>         efi_rts_work.arg1 = _arg1;                                      \
> @@ -79,6 +84,8 @@ struct efi_runtime_work efi_rts_work;
>         else                                                            \
>                 pr_err("Failed to queue work to efi_rts_wq.\n");        \
>                                                                         \
> +exit:                                                                  \
> +       efi_rts_work.efi_rts_id = NONE;                                 \
>         efi_rts_work.status;                                            \
>  })
>
> @@ -400,6 +407,7 @@ static void virt_efi_reset_system(int reset_type,
>                         "could not get exclusive access to the firmware\n");
>                 return;
>         }
> +       efi_rts_work.efi_rts_id = RESET_SYSTEM;
>         __efi_call_virt(reset_system, reset_type, status, data_size, data);
>         up(&efi_runtime_lock);
>  }
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 9d4c25090fd04..9a86f467b8911 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1666,8 +1666,13 @@ struct linux_efi_tpm_eventlog {
>
>  extern int efi_tpm_eventlog_init(void);
>
> -/* efi_runtime_service() function identifiers */
> +/*
> + * efi_runtime_service() function identifiers.
> + * "NONE" is used by efi_recover_from_page_fault() to check if the page
> + * fault happened while executing an efi runtime service.
> + */
>  enum efi_rts_ids {
> +       NONE,
>         GET_TIME,
>         SET_TIME,
>         GET_WAKEUP_TIME,
> @@ -1677,6 +1682,7 @@ enum efi_rts_ids {
>         SET_VARIABLE,
>         QUERY_VARIABLE_INFO,
>         GET_NEXT_HIGH_MONO_COUNT,
> +       RESET_SYSTEM,
>         UPDATE_CAPSULE,
>         QUERY_CAPSULE_CAPS,
>  };
> --
> 2.20.1
>

[PATCH AUTOSEL 4.19 191/191] x86/efi: fix a -Wtype-limits compilation warning

[Sasha Levin]

From: Qian Cai <cai@lca.pw>

[ Upstream commit 919aef44d73d5d0c04213cb1bc31149cc074e65e ]

Compiling a kernel with W=1 generates this warning,

arch/x86/platform/efi/quirks.c:731:16: warning: comparison of unsigned
expression >= 0 is always true [-Wtype-limits]

Fixes: 3425d934fc03 ("efi/x86: Handle page faults occurring while running ...")
Signed-off-by: Qian Cai <cai@lca.pw>
Acked-by: "Prakhya, Sai Praneeth" <sai.praneeth.prakhya@intel.com>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 arch/x86/platform/efi/quirks.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index 669babcaf245a..c75d5ba732f18 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -684,7 +684,7 @@ void efi_recover_from_page_fault(unsigned long phys_addr)
 	 * Address range 0x0000 - 0x0fff is always mapped in the efi_pgd, so
 	 * page faulting on these addresses isn't expected.
 	 */
-	if (phys_addr >= 0x0000 && phys_addr <= 0x0fff)
+	if (phys_addr <= 0x0fff)
 		return;
 
 	/*
-- 
2.20.1