Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1450955 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH] Skip UV runtime services mapping in the efi_runtime_disabled case Alex Thorlton <athorlton@sgi.com> - 2016-07-27 00:50 +0200
    Re: [PATCH] Skip UV runtime services mapping in the  efi_runtime_disabled case Matt Fleming <matt@codeblueprint.co.uk> - 2016-08-01 16:00 +0200
      Re: [PATCH] Skip UV runtime services mapping in the  efi_runtime_disabled case Alex Thorlton <athorlton@sgi.com> - 2016-08-01 16:30 +0200
        Re: [PATCH] Skip UV runtime services mapping in the  efi_runtime_disabled case Alex Thorlton <athorlton@sgi.com> - 2016-08-03 21:40 +0200
          Re: [PATCH] Skip UV runtime services mapping in the  efi_runtime_disabled case Matt Fleming <matt@codeblueprint.co.uk> - 2016-08-04 11:30 +0200

#1450955 — [PATCH] Skip UV runtime services mapping in the efi_runtime_disabled case

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 0000000000000880
[    0.346276] IP: [<ffffffff8105df8d>] 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 <athorlton@sgi.com>
Cc: Russ Anderson <rja@sgi.com>
Cc: Mike Travis <travis@sgi.com>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Borislav Petkov <bp@suse.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@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;
 	}
-- 
1.8.5.6

[toc] | [next] | [standalone]

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

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 0000000000000880
> [    0.346276] IP: [<ffffffff8105df8d>] 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 <athorlton@sgi.com>
> Cc: Russ Anderson <rja@sgi.com>
> Cc: Mike Travis <travis@sgi.com>
> Cc: Matt Fleming <matt@codeblueprint.co.uk>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: x86@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?

[toc] | [prev] | [next] | [standalone]

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

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 0000000000000880
> > [    0.346276] IP: [<ffffffff8105df8d>] 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 <athorlton@sgi.com>
> > Cc: Russ Anderson <rja@sgi.com>
> > Cc: Mike Travis <travis@sgi.com>
> > Cc: Matt Fleming <matt@codeblueprint.co.uk>
> > Cc: Borislav Petkov <bp@suse.de>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: "H. Peter Anvin" <hpa@zytor.com>
> > Cc: x86@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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web