Re: [PATCH] Skip UV runtime services mapping in the efi_runtime_disabled case

[Matt Fleming]

On Wed, 03 Aug, at 02:36:07PM, Alex Thorlton wrote:
> On Mon, Aug 01, 2016 at 09:28:06AM -0500, Alex Thorlton wrote:
> > So, it definitely needs to go in for v4.8, but it's kind of a toss-up
> > for the older kernels.  I'll discuss this with the other guys around
> > here to see what they think, and get back to you a bit later, if that's
> > alright?
> 
> We talked about this, and I think everyone here agrees that there's not
> much point in pulling this change back to the older kernels.  The only
> exception here would be that we'd definitely like this change on the
> older kernels *if* my other memmap fixes get ported back to those
> kernels, though I don't know what the chances are of those changes
> making it through stable.
> 
> So, unless you have a particular reason that you'd like to pull it back
> to the old kernels, or you think that my other fixes might make it back
> there, I don't see much point.
> 
> Let me know what you think!

Sound reasoning. I'll apply this to v4.8 only.

Re: [PATCH] Skip UV runtime services mapping in the efi_runtime_disabled case

[Matt Fleming]

On Wed, 03 Aug, at 02:36:07PM, Alex Thorlton wrote:
> On Mon, Aug 01, 2016 at 09:28:06AM -0500, Alex Thorlton wrote:
> > So, it definitely needs to go in for v4.8, but it's kind of a toss-up
> > for the older kernels.  I'll discuss this with the other guys around
> > here to see what they think, and get back to you a bit later, if that's
> > alright?
> 
> We talked about this, and I think everyone here agrees that there's not
> much point in pulling this change back to the older kernels.  The only
> exception here would be that we'd definitely like this change on the
> older kernels *if* my other memmap fixes get ported back to those
> kernels, though I don't know what the chances are of those changes
> making it through stable.
> 
> So, unless you have a particular reason that you'd like to pull it back
> to the old kernels, or you think that my other fixes might make it back
> there, I don't see much point.
> 
> Let me know what you think!

Sound reasoning. I'll apply this to v4.8 only.

Re: [PATCH] Skip UV runtime services mapping in the efi_runtime_disabled case

[Alex Thorlton]

On Mon, Aug 01, 2016 at 09:28:06AM -0500, Alex Thorlton wrote:
> So, it definitely needs to go in for v4.8, but it's kind of a toss-up
> for the older kernels.  I'll discuss this with the other guys around
> here to see what they think, and get back to you a bit later, if that's
> alright?

We talked about this, and I think everyone here agrees that there's not
much point in pulling this change back to the older kernels.  The only
exception here would be that we'd definitely like this change on the
older kernels *if* my other memmap fixes get ported back to those
kernels, though I don't know what the chances are of those changes
making it through stable.

So, unless you have a particular reason that you'd like to pull it back
to the old kernels, or you think that my other fixes might make it back
there, I don't see much point.

Let me know what you think!

- Alex

Re: [PATCH] Skip UV runtime services mapping in the efi_runtime_disabled case

[Alex Thorlton]

On Mon, Aug 01, 2016 at 09:28:06AM -0500, Alex Thorlton wrote:
> So, it definitely needs to go in for v4.8, but it's kind of a toss-up
> for the older kernels.  I'll discuss this with the other guys around
> here to see what they think, and get back to you a bit later, if that's
> alright?

We talked about this, and I think everyone here agrees that there's not
much point in pulling this change back to the older kernels.  The only
exception here would be that we'd definitely like this change on the
older kernels *if* my other memmap fixes get ported back to those
kernels, though I don't know what the chances are of those changes
making it through stable.

So, unless you have a particular reason that you'd like to pull it back
to the old kernels, or you think that my other fixes might make it back
there, I don't see much point.

Let me know what you think!

- Alex

Re: [PATCH] Skip UV runtime services mapping in the efi_runtime_disabled case

[Alex Thorlton]

On Mon, Aug 01, 2016 at 02:49:57PM +0100, Matt Fleming wrote:
> On Tue, 26 Jul, at 05:38:33PM, Alex Thorlton wrote:
> > This problem has actually been in the UV code for a while, but we didn't
> > catch it until recently, because we had been relying on EFI_OLD_MEMMAP
> > to allow our systems to boot for a period of time.  We noticed the issue
> > when trying to kexec a recent community kernel, where we hit this NULL
> > pointer dereference in efi_sync_low_kernel_mappings:
> > 
> > [0.337515] BUG: unable to handle kernel NULL pointer dereference at 
> > 0880
> > [0.346276] IP: [] 
> > efi_sync_low_kernel_mappings+0x5d/0x1b0
> > 
> > The problem doesn't show up with EFI_OLD_MEMMAP because we skip the
> > chunk of setup_efi_state that sets the efi_loader_signature for the
> > kexec'd kernel.  When the kexec'd kernel boots, it won't set EFI_BOOT in
> > setup_arch, so we completely avoid the bug.
> > 
> > We always kexec with noefi on the command line, so this shouldn't be an
> > issue, but since we're not actually checking for efi_runtime_disabled in
> > uv_bios_init, we end up trying to do EFI runtime callbacks when we
> > shouldn't be. This patch just adds a check for efi_runtime_disabled in
> > uv_bios_init so that we don't map in uv_systab when runtime_disabled ==
> > true.
> > 
> > Signed-off-by: Alex Thorlton 
> > Cc: Russ Anderson 
> > Cc: Mike Travis 
> > Cc: Matt Fleming 
> > Cc: Borislav Petkov 
> > Cc: Thomas Gleixner 
> > Cc: Ingo Molnar 
> > Cc: "H. Peter Anvin" 
> > Cc: x...@kernel.org
> > ---
> >  arch/x86/platform/uv/bios_uv.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/platform/uv/bios_uv.c b/arch/x86/platform/uv/bios_uv.c
> > index 66b2166..0df8a03 100644
> > --- a/arch/x86/platform/uv/bios_uv.c
> > +++ b/arch/x86/platform/uv/bios_uv.c
> > @@ -187,7 +187,8 @@ EXPORT_SYMBOL_GPL(uv_bios_set_legacy_vga_target);
> >  void uv_bios_init(void)
> >  {
> > uv_systab = NULL;
> > -   if ((efi.uv_systab == EFI_INVALID_TABLE_ADDR) || !efi.uv_systab) {
> > +   if ((efi.uv_systab == EFI_INVALID_TABLE_ADDR) ||
> > +   !efi.uv_systab || efi_runtime_disabled()) {
> > pr_crit("UV: UVsystab: missing\n");
> > return;
> > }
> 
> The fix looks fine, but I'm losing track of which kernels this patch
> should be applied to. Does it just need to be applied for v4.8 or
> earlier kernels too?

Well, we *have* to boot v4.6 and v4.7 with efi=old_map, which will avoid
our kexec problem entirely, so while the patch would apply just fine on
those kernels, and achieve the desired effect, we wouldn't really get
any benefit from it.

So, it definitely needs to go in for v4.8, but it's kind of a toss-up
for the older kernels.  I'll discuss this with the other guys around
here to see what they think, and get back to you a bit later, if that's
alright?

- Alex

Re: [PATCH] Skip UV runtime services mapping in the efi_runtime_disabled case

[Alex Thorlton]

On Mon, Aug 01, 2016 at 02:49:57PM +0100, Matt Fleming wrote:
> On Tue, 26 Jul, at 05:38:33PM, Alex Thorlton wrote:
> > This problem has actually been in the UV code for a while, but we didn't
> > catch it until recently, because we had been relying on EFI_OLD_MEMMAP
> > to allow our systems to boot for a period of time.  We noticed the issue
> > when trying to kexec a recent community kernel, where we hit this NULL
> > pointer dereference in efi_sync_low_kernel_mappings:
> > 
> > [0.337515] BUG: unable to handle kernel NULL pointer dereference at 
> > 0880
> > [0.346276] IP: [] 
> > efi_sync_low_kernel_mappings+0x5d/0x1b0
> > 
> > The problem doesn't show up with EFI_OLD_MEMMAP because we skip the
> > chunk of setup_efi_state that sets the efi_loader_signature for the
> > kexec'd kernel.  When the kexec'd kernel boots, it won't set EFI_BOOT in
> > setup_arch, so we completely avoid the bug.
> > 
> > We always kexec with noefi on the command line, so this shouldn't be an
> > issue, but since we're not actually checking for efi_runtime_disabled in
> > uv_bios_init, we end up trying to do EFI runtime callbacks when we
> > shouldn't be. This patch just adds a check for efi_runtime_disabled in
> > uv_bios_init so that we don't map in uv_systab when runtime_disabled ==
> > true.
> > 
> > Signed-off-by: Alex Thorlton 
> > Cc: Russ Anderson 
> > Cc: Mike Travis 
> > Cc: Matt Fleming 
> > Cc: Borislav Petkov 
> > Cc: Thomas Gleixner 
> > Cc: Ingo Molnar 
> > Cc: "H. Peter Anvin" 
> > Cc: x...@kernel.org
> > ---
> >  arch/x86/platform/uv/bios_uv.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/platform/uv/bios_uv.c b/arch/x86/platform/uv/bios_uv.c
> > index 66b2166..0df8a03 100644
> > --- a/arch/x86/platform/uv/bios_uv.c
> > +++ b/arch/x86/platform/uv/bios_uv.c
> > @@ -187,7 +187,8 @@ EXPORT_SYMBOL_GPL(uv_bios_set_legacy_vga_target);
> >  void uv_bios_init(void)
> >  {
> > uv_systab = NULL;
> > -   if ((efi.uv_systab == EFI_INVALID_TABLE_ADDR) || !efi.uv_systab) {
> > +   if ((efi.uv_systab == EFI_INVALID_TABLE_ADDR) ||
> > +   !efi.uv_systab || efi_runtime_disabled()) {
> > pr_crit("UV: UVsystab: missing\n");
> > return;
> > }
> 
> The fix looks fine, but I'm losing track of which kernels this patch
> should be applied to. Does it just need to be applied for v4.8 or
> earlier kernels too?

Well, we *have* to boot v4.6 and v4.7 with efi=old_map, which will avoid
our kexec problem entirely, so while the patch would apply just fine on
those kernels, and achieve the desired effect, we wouldn't really get
any benefit from it.

So, it definitely needs to go in for v4.8, but it's kind of a toss-up
for the older kernels.  I'll discuss this with the other guys around
here to see what they think, and get back to you a bit later, if that's
alright?

- Alex

Re: [PATCH] Skip UV runtime services mapping in the efi_runtime_disabled case

[Matt Fleming]

On Tue, 26 Jul, at 05:38:33PM, Alex Thorlton wrote:
> This problem has actually been in the UV code for a while, but we didn't
> catch it until recently, because we had been relying on EFI_OLD_MEMMAP
> to allow our systems to boot for a period of time.  We noticed the issue
> when trying to kexec a recent community kernel, where we hit this NULL
> pointer dereference in efi_sync_low_kernel_mappings:
> 
> [0.337515] BUG: unable to handle kernel NULL pointer dereference at 
> 0880
> [0.346276] IP: [] 
> efi_sync_low_kernel_mappings+0x5d/0x1b0
> 
> The problem doesn't show up with EFI_OLD_MEMMAP because we skip the
> chunk of setup_efi_state that sets the efi_loader_signature for the
> kexec'd kernel.  When the kexec'd kernel boots, it won't set EFI_BOOT in
> setup_arch, so we completely avoid the bug.
> 
> We always kexec with noefi on the command line, so this shouldn't be an
> issue, but since we're not actually checking for efi_runtime_disabled in
> uv_bios_init, we end up trying to do EFI runtime callbacks when we
> shouldn't be. This patch just adds a check for efi_runtime_disabled in
> uv_bios_init so that we don't map in uv_systab when runtime_disabled ==
> true.
> 
> Signed-off-by: Alex Thorlton 
> Cc: Russ Anderson 
> Cc: Mike Travis 
> Cc: Matt Fleming 
> Cc: Borislav Petkov 
> Cc: Thomas Gleixner 
> Cc: Ingo Molnar 
> Cc: "H. Peter Anvin" 
> Cc: x...@kernel.org
> ---
>  arch/x86/platform/uv/bios_uv.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/platform/uv/bios_uv.c b/arch/x86/platform/uv/bios_uv.c
> index 66b2166..0df8a03 100644
> --- a/arch/x86/platform/uv/bios_uv.c
> +++ b/arch/x86/platform/uv/bios_uv.c
> @@ -187,7 +187,8 @@ EXPORT_SYMBOL_GPL(uv_bios_set_legacy_vga_target);
>  void uv_bios_init(void)
>  {
>   uv_systab = NULL;
> - if ((efi.uv_systab == EFI_INVALID_TABLE_ADDR) || !efi.uv_systab) {
> + if ((efi.uv_systab == EFI_INVALID_TABLE_ADDR) ||
> + !efi.uv_systab || efi_runtime_disabled()) {
>   pr_crit("UV: UVsystab: missing\n");
>   return;
>   }

The fix looks fine, but I'm losing track of which kernels this patch
should be applied to. Does it just need to be applied for v4.8 or
earlier kernels too?

Re: [PATCH] Skip UV runtime services mapping in the efi_runtime_disabled case

[Matt Fleming]

On Tue, 26 Jul, at 05:38:33PM, Alex Thorlton wrote:
> This problem has actually been in the UV code for a while, but we didn't
> catch it until recently, because we had been relying on EFI_OLD_MEMMAP
> to allow our systems to boot for a period of time.  We noticed the issue
> when trying to kexec a recent community kernel, where we hit this NULL
> pointer dereference in efi_sync_low_kernel_mappings:
> 
> [0.337515] BUG: unable to handle kernel NULL pointer dereference at 
> 0880
> [0.346276] IP: [] 
> efi_sync_low_kernel_mappings+0x5d/0x1b0
> 
> The problem doesn't show up with EFI_OLD_MEMMAP because we skip the
> chunk of setup_efi_state that sets the efi_loader_signature for the
> kexec'd kernel.  When the kexec'd kernel boots, it won't set EFI_BOOT in
> setup_arch, so we completely avoid the bug.
> 
> We always kexec with noefi on the command line, so this shouldn't be an
> issue, but since we're not actually checking for efi_runtime_disabled in
> uv_bios_init, we end up trying to do EFI runtime callbacks when we
> shouldn't be. This patch just adds a check for efi_runtime_disabled in
> uv_bios_init so that we don't map in uv_systab when runtime_disabled ==
> true.
> 
> Signed-off-by: Alex Thorlton 
> Cc: Russ Anderson 
> Cc: Mike Travis 
> Cc: Matt Fleming 
> Cc: Borislav Petkov 
> Cc: Thomas Gleixner 
> Cc: Ingo Molnar 
> Cc: "H. Peter Anvin" 
> Cc: x...@kernel.org
> ---
>  arch/x86/platform/uv/bios_uv.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/platform/uv/bios_uv.c b/arch/x86/platform/uv/bios_uv.c
> index 66b2166..0df8a03 100644
> --- a/arch/x86/platform/uv/bios_uv.c
> +++ b/arch/x86/platform/uv/bios_uv.c
> @@ -187,7 +187,8 @@ EXPORT_SYMBOL_GPL(uv_bios_set_legacy_vga_target);
>  void uv_bios_init(void)
>  {
>   uv_systab = NULL;
> - if ((efi.uv_systab == EFI_INVALID_TABLE_ADDR) || !efi.uv_systab) {
> + if ((efi.uv_systab == EFI_INVALID_TABLE_ADDR) ||
> + !efi.uv_systab || efi_runtime_disabled()) {
>   pr_crit("UV: UVsystab: missing\n");
>   return;
>   }

The fix looks fine, but I'm losing track of which kernels this patch
should be applied to. Does it just need to be applied for v4.8 or
earlier kernels too?