Boot failure due to: x86/boot: Save fields explicitly, zero out everything else

Boot failure due to: x86/boot: Save fields explicitly, zero out everything else

[John Hubbard]

On 8/21/19 10:05 AM, Neil MacLeod wrote:
> Hi John
> 
> The following bug might be of interest:
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=204645
> 
> Let me know if I can be of any help.
> 

Hi Neil,

First of all, I'm pasting in the bug information so that it's directly available
here:

===================================================================
Description: Neil MacLeod 2019-08-21 16:29:19 UTC
System: Intel i5 Skylake NUC (NUC6i5SYH)

This system boots fine from internal M2 (128GB) drive with 5.3-rc4.

With 5.3-rc5, it does not boot from M2 and is stuck on the Intel splash screen (no other text is displayed, no panic etc.). It will boot 5.3-rc5 from a USB flash memory stick (via the F10 boot menu), but not from the internal M2.

Bisecting between 5.3-rc4 and 5.3-rc5, the bad commit is:

neil@nm-linux:~/projects/pullrequest_repos/torvalds-linux$ git bisect bad
a90118c445cc7f07781de26a9684d4ec58bfcfd1 is the first bad commit
commit a90118c445cc7f07781de26a9684d4ec58bfcfd1
Author: John Hubbard <jhubbard@nvidia.com>
Date:   Tue Jul 30 22:46:27 2019 -0700

     x86/boot: Save fields explicitly, zero out everything else

     Recent gcc compilers (gcc 9.1) generate warnings about an out of bounds
     memset, if the memset goes accross several fields of a struct. This
     generated a couple of warnings on x86_64 builds in sanitize_boot_params().

     Fix this by explicitly saving the fields in struct boot_params
     that are intended to be preserved, and zeroing all the rest.

     [ tglx: Tagged for stable as it breaks the warning free build there as well ]

     Suggested-by: Thomas Gleixner <tglx@linutronix.de>
     Suggested-by: H. Peter Anvin <hpa@zytor.com>
     Signed-off-by: John Hubbard <jhubbard@nvidia.com>
     Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
     Cc: stable@vger.kernel.org
     Link: https://lkml.kernel.org/r/20190731054627.5627-2-jhubbard@nvidia.com

:040000 040000 e0963edca990540dd759765a3d765af4698df892 d07e645eb3a500c31bd65526205e286ff6941187 M      arch

Comment 1 Neil MacLeod 2019-08-21 16:31:35 UTC
The kernel is built with gcc-9.2.0.

Comment 2 Neil MacLeod 2019-08-21 16:55:48 UTC

5.3-rc5 with "x86/boot: Save fields explicitly, zero out everything else" reverted will build with gcc-9.2.0, and boot from M2.
===================================================================

I'm also CC'ing the lists, so they know that the patch has caused a regression, and
also out of hope that they can help me choose the shortest path forward to debugging
this. My first reaction is that the list of BOOT_PARAM_PRESERVE() fields is
flawed--by which I include cases of  "the system improperly relied on a field that the spec said
should be zeroed". (After all, the boot_params->sentinel is set, which already means
the system is not really doing it right.)

So I'm going to cheat and ask right now if anyone notices either ommissions
or wrong entries here:

static void sanitize_boot_params(struct boot_params *boot_params)
{
...
		const struct boot_params_to_save to_save[] = {
			BOOT_PARAM_PRESERVE(screen_info),
			BOOT_PARAM_PRESERVE(apm_bios_info),
			BOOT_PARAM_PRESERVE(tboot_addr),
			BOOT_PARAM_PRESERVE(ist_info),
			BOOT_PARAM_PRESERVE(acpi_rsdp_addr),
			BOOT_PARAM_PRESERVE(hd0_info),
			BOOT_PARAM_PRESERVE(hd1_info),
			BOOT_PARAM_PRESERVE(sys_desc_table),
			BOOT_PARAM_PRESERVE(olpc_ofw_header),
			BOOT_PARAM_PRESERVE(efi_info),
			BOOT_PARAM_PRESERVE(alt_mem_k),
			BOOT_PARAM_PRESERVE(scratch),
			BOOT_PARAM_PRESERVE(e820_entries),
			BOOT_PARAM_PRESERVE(eddbuf_entries),
			BOOT_PARAM_PRESERVE(edd_mbr_sig_buf_entries),
			BOOT_PARAM_PRESERVE(edd_mbr_sig_buffer),
			BOOT_PARAM_PRESERVE(e820_table),
			BOOT_PARAM_PRESERVE(eddbuf),
		};

If not, then I think we need to bisect by...well, let's start with the
theory that we zeroed out too much, so we could start adding more fields
to preserve.  If that doesn't find the problem, then it's more complicated,
and might be better to go the other direction: starting without my commit,
and zeroing out fields until we see the failure.

Are you able to test patches? It would take some time, since there are quite a
few fields. Alternately, if you provide some more system information details
(especially if we have any other notes about other failures and passes), then
I might be able to borrow a Skylake system and attempt a repro.


thanks,
-- 
John Hubbard
NVIDIA

Re: Boot failure due to: x86/boot: Save fields explicitly, zero out everything else

[Neil MacLeod]

Hi John

I can test any patches given a link to the diff, and am happy to do so.

If I've understood your suggestion correctly, I will try commenting
out all of the entries, then add them back one-by-one until I get a
non-booting situation. I'll let you know how I get on.

The OS I'm testing is LibreELEC, which is a custom x86_64 build, and
this hasn't shown any problems on this Skylake i5 NUC in all the years
I've been using it as a test system (since at least 4.15.y). So far
5.3-rc has been trouble-free until 5.3-rc5. As I mentioned in the bug,
I've been able to boot 5.3-rc5 from a USB memory stick, but can't boot
5.3-rc5 from the internal M2 drive unless I revert this commit.

Regards
Neil

On Wed, 21 Aug 2019 at 19:20, John Hubbard <jhubbard@nvidia.com> wrote:
>
> On 8/21/19 10:05 AM, Neil MacLeod wrote:
> > Hi John
> >
> > The following bug might be of interest:
> >
> > https://bugzilla.kernel.org/show_bug.cgi?id=204645
> >
> > Let me know if I can be of any help.
> >
>
> Hi Neil,
>
> First of all, I'm pasting in the bug information so that it's directly available
> here:
>
> ===================================================================
> Description: Neil MacLeod 2019-08-21 16:29:19 UTC
> System: Intel i5 Skylake NUC (NUC6i5SYH)
>
> This system boots fine from internal M2 (128GB) drive with 5.3-rc4.
>
> With 5.3-rc5, it does not boot from M2 and is stuck on the Intel splash screen (no other text is displayed, no panic etc.). It will boot 5.3-rc5 from a USB flash memory stick (via the F10 boot menu), but not from the internal M2.
>
> Bisecting between 5.3-rc4 and 5.3-rc5, the bad commit is:
>
> neil@nm-linux:~/projects/pullrequest_repos/torvalds-linux$ git bisect bad
> a90118c445cc7f07781de26a9684d4ec58bfcfd1 is the first bad commit
> commit a90118c445cc7f07781de26a9684d4ec58bfcfd1
> Author: John Hubbard <jhubbard@nvidia.com>
> Date:   Tue Jul 30 22:46:27 2019 -0700
>
>      x86/boot: Save fields explicitly, zero out everything else
>
>      Recent gcc compilers (gcc 9.1) generate warnings about an out of bounds
>      memset, if the memset goes accross several fields of a struct. This
>      generated a couple of warnings on x86_64 builds in sanitize_boot_params().
>
>      Fix this by explicitly saving the fields in struct boot_params
>      that are intended to be preserved, and zeroing all the rest.
>
>      [ tglx: Tagged for stable as it breaks the warning free build there as well ]
>
>      Suggested-by: Thomas Gleixner <tglx@linutronix.de>
>      Suggested-by: H. Peter Anvin <hpa@zytor.com>
>      Signed-off-by: John Hubbard <jhubbard@nvidia.com>
>      Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
>      Cc: stable@vger.kernel.org
>      Link: https://lkml.kernel.org/r/20190731054627.5627-2-jhubbard@nvidia.com
>
> :040000 040000 e0963edca990540dd759765a3d765af4698df892 d07e645eb3a500c31bd65526205e286ff6941187 M      arch
>
> Comment 1 Neil MacLeod 2019-08-21 16:31:35 UTC
> The kernel is built with gcc-9.2.0.
>
> Comment 2 Neil MacLeod 2019-08-21 16:55:48 UTC
>
> 5.3-rc5 with "x86/boot: Save fields explicitly, zero out everything else" reverted will build with gcc-9.2.0, and boot from M2.
> ===================================================================
>
> I'm also CC'ing the lists, so they know that the patch has caused a regression, and
> also out of hope that they can help me choose the shortest path forward to debugging
> this. My first reaction is that the list of BOOT_PARAM_PRESERVE() fields is
> flawed--by which I include cases of  "the system improperly relied on a field that the spec said
> should be zeroed". (After all, the boot_params->sentinel is set, which already means
> the system is not really doing it right.)
>
> So I'm going to cheat and ask right now if anyone notices either ommissions
> or wrong entries here:
>
> static void sanitize_boot_params(struct boot_params *boot_params)
> {
> ...
>                 const struct boot_params_to_save to_save[] = {
>                         BOOT_PARAM_PRESERVE(screen_info),
>                         BOOT_PARAM_PRESERVE(apm_bios_info),
>                         BOOT_PARAM_PRESERVE(tboot_addr),
>                         BOOT_PARAM_PRESERVE(ist_info),
>                         BOOT_PARAM_PRESERVE(acpi_rsdp_addr),
>                         BOOT_PARAM_PRESERVE(hd0_info),
>                         BOOT_PARAM_PRESERVE(hd1_info),
>                         BOOT_PARAM_PRESERVE(sys_desc_table),
>                         BOOT_PARAM_PRESERVE(olpc_ofw_header),
>                         BOOT_PARAM_PRESERVE(efi_info),
>                         BOOT_PARAM_PRESERVE(alt_mem_k),
>                         BOOT_PARAM_PRESERVE(scratch),
>                         BOOT_PARAM_PRESERVE(e820_entries),
>                         BOOT_PARAM_PRESERVE(eddbuf_entries),
>                         BOOT_PARAM_PRESERVE(edd_mbr_sig_buf_entries),
>                         BOOT_PARAM_PRESERVE(edd_mbr_sig_buffer),
>                         BOOT_PARAM_PRESERVE(e820_table),
>                         BOOT_PARAM_PRESERVE(eddbuf),
>                 };
>
> If not, then I think we need to bisect by...well, let's start with the
> theory that we zeroed out too much, so we could start adding more fields
> to preserve.  If that doesn't find the problem, then it's more complicated,
> and might be better to go the other direction: starting without my commit,
> and zeroing out fields until we see the failure.
>
> Are you able to test patches? It would take some time, since there are quite a
> few fields. Alternately, if you provide some more system information details
> (especially if we have any other notes about other failures and passes), then
> I might be able to borrow a Skylake system and attempt a repro.
>
>
> thanks,
> --
> John Hubbard
> NVIDIA

Re: Boot failure due to: x86/boot: Save fields explicitly, zero out everything else

[John Hubbard]

On 8/21/19 11:32 AM, Neil MacLeod wrote:
> Hi John
> 
> I can test any patches given a link to the diff, and am happy to do so.
> 
> If I've understood your suggestion correctly, I will try commenting
> out all of the entries, then add them back one-by-one until I get a
> non-booting situation. I'll let you know how I get on.
> 

I was actually going the other direction. Note that the BOOT_PARAM_PRESERVE
entries indicate what *not* to zero out. So if you remove them all, then
everything gets zeroed, including lots of critical fields, and you
definitely won't boot up. (The tricky part is we don't yet know whether
fields are missing, need to be added--or both.)

So I was heading toward adding most (but not all--important) of these fields,
as BOOT_PARAM_PRESERVE entries, as a first bisect step. Let me work that up
and post a patch for that.

struct boot_params {
	struct screen_info screen_info;			/* 0x000 */
	struct apm_bios_info apm_bios_info;		/* 0x040 */
	__u8  _pad2[4];					/* 0x054 */
	__u64  tboot_addr;				/* 0x058 */
	struct ist_info ist_info;			/* 0x060 */
	__u64 acpi_rsdp_addr;				/* 0x070 */
	__u8  _pad3[8];					/* 0x078 */
	__u8  hd0_info[16];	/* obsolete! */		/* 0x080 */
	__u8  hd1_info[16];	/* obsolete! */		/* 0x090 */
	struct sys_desc_table sys_desc_table; /* obsolete! */	/* 0x0a0 */
	struct olpc_ofw_header olpc_ofw_header;		/* 0x0b0 */
	__u32 ext_ramdisk_image;			/* 0x0c0 */
	__u32 ext_ramdisk_size;				/* 0x0c4 */
	__u32 ext_cmd_line_ptr;				/* 0x0c8 */
	__u8  _pad4[116];				/* 0x0cc */
	struct edid_info edid_info;			/* 0x140 */
	struct efi_info efi_info;			/* 0x1c0 */
	__u32 alt_mem_k;				/* 0x1e0 */
	__u32 scratch;		/* Scratch field! */	/* 0x1e4 */
	__u8  e820_entries;				/* 0x1e8 */
	__u8  eddbuf_entries;				/* 0x1e9 */
	__u8  edd_mbr_sig_buf_entries;			/* 0x1ea */
	__u8  kbd_status;				/* 0x1eb */
	__u8  secure_boot;				/* 0x1ec */
	__u8  _pad5[2];					/* 0x1ed */
	/*
	 * The sentinel is set to a nonzero value (0xff) in header.S.
	 *
	 * A bootloader is supposed to only take setup_header and put
	 * it into a clean boot_params buffer. If it turns out that
	 * it is clumsy or too generous with the buffer, it most
	 * probably will pick up the sentinel variable too. The fact
	 * that this variable then is still 0xff will let kernel
	 * know that some variables in boot_params are invalid and
	 * kernel should zero out certain portions of boot_params.
	 */
	__u8  sentinel;					/* 0x1ef */
	__u8  _pad6[1];					/* 0x1f0 */
	struct setup_header hdr;    /* setup header */	/* 0x1f1 */
	__u8  _pad7[0x290-0x1f1-sizeof(struct setup_header)];
	__u32 edd_mbr_sig_buffer[EDD_MBR_SIG_MAX];	/* 0x290 */
	struct boot_e820_entry e820_table[E820_MAX_ENTRIES_ZEROPAGE]; /* 0x2d0 */
	__u8  _pad8[48];				/* 0xcd0 */
	struct edd_info eddbuf[EDDMAXNR];		/* 0xd00 */
	__u8  _pad9[276];				/* 0xeec */
} __attribute__((packed));



thanks,
-- 
John Hubbard
NVIDIA


> The OS I'm testing is LibreELEC, which is a custom x86_64 build, and
> this hasn't shown any problems on this Skylake i5 NUC in all the years
> I've been using it as a test system (since at least 4.15.y). So far
> 5.3-rc has been trouble-free until 5.3-rc5. As I mentioned in the bug,
> I've been able to boot 5.3-rc5 from a USB memory stick, but can't boot
> 5.3-rc5 from the internal M2 drive unless I revert this commit.
> 

Re: Boot failure due to: x86/boot: Save fields explicitly, zero out everything else

[Thomas Gleixner]

On Wed, 21 Aug 2019, John Hubbard wrote:
> On 8/21/19 10:05 AM, Neil MacLeod wrote:
> static void sanitize_boot_params(struct boot_params *boot_params)
> {
> ...
> 		const struct boot_params_to_save to_save[] = {
> 			BOOT_PARAM_PRESERVE(screen_info),
> 			BOOT_PARAM_PRESERVE(apm_bios_info),
> 			BOOT_PARAM_PRESERVE(tboot_addr),
> 			BOOT_PARAM_PRESERVE(ist_info),
> 			BOOT_PARAM_PRESERVE(acpi_rsdp_addr),
> 			BOOT_PARAM_PRESERVE(hd0_info),
> 			BOOT_PARAM_PRESERVE(hd1_info),
> 			BOOT_PARAM_PRESERVE(sys_desc_table),
> 			BOOT_PARAM_PRESERVE(olpc_ofw_header),
> 			BOOT_PARAM_PRESERVE(efi_info),
> 			BOOT_PARAM_PRESERVE(alt_mem_k),
> 			BOOT_PARAM_PRESERVE(scratch),
> 			BOOT_PARAM_PRESERVE(e820_entries),
> 			BOOT_PARAM_PRESERVE(eddbuf_entries),
> 			BOOT_PARAM_PRESERVE(edd_mbr_sig_buf_entries),
> 			BOOT_PARAM_PRESERVE(edd_mbr_sig_buffer),
> 			BOOT_PARAM_PRESERVE(e820_table),
> 			BOOT_PARAM_PRESERVE(eddbuf),
> 		};

I think I spotted it:

-               boot_params->acpi_rsdp_addr = 0;

+ 			BOOT_PARAM_PRESERVE(acpi_rsdp_addr),

And it does not preserve 'hdr'

Grr. I surely was too tired when staring at this last time.

Thanks,

	tglx

Re: Boot failure due to: x86/boot: Save fields explicitly, zero out everything else

[John Hubbard]

On 8/21/19 11:51 AM, Thomas Gleixner wrote:
> On Wed, 21 Aug 2019, John Hubbard wrote:
>> On 8/21/19 10:05 AM, Neil MacLeod wrote:
>> static void sanitize_boot_params(struct boot_params *boot_params)
>> {
>> ...
>> 		const struct boot_params_to_save to_save[] = {
>> 			BOOT_PARAM_PRESERVE(screen_info),
>> 			BOOT_PARAM_PRESERVE(apm_bios_info),
>> 			BOOT_PARAM_PRESERVE(tboot_addr),
>> 			BOOT_PARAM_PRESERVE(ist_info),
>> 			BOOT_PARAM_PRESERVE(acpi_rsdp_addr),
>> 			BOOT_PARAM_PRESERVE(hd0_info),
>> 			BOOT_PARAM_PRESERVE(hd1_info),
>> 			BOOT_PARAM_PRESERVE(sys_desc_table),
>> 			BOOT_PARAM_PRESERVE(olpc_ofw_header),
>> 			BOOT_PARAM_PRESERVE(efi_info),
>> 			BOOT_PARAM_PRESERVE(alt_mem_k),
>> 			BOOT_PARAM_PRESERVE(scratch),
>> 			BOOT_PARAM_PRESERVE(e820_entries),
>> 			BOOT_PARAM_PRESERVE(eddbuf_entries),
>> 			BOOT_PARAM_PRESERVE(edd_mbr_sig_buf_entries),
>> 			BOOT_PARAM_PRESERVE(edd_mbr_sig_buffer),
>> 			BOOT_PARAM_PRESERVE(e820_table),
>> 			BOOT_PARAM_PRESERVE(eddbuf),
>> 		};
> 
> I think I spotted it:
> 
> -               boot_params->acpi_rsdp_addr = 0;
> 
> + 			BOOT_PARAM_PRESERVE(acpi_rsdp_addr),
> 
> And it does not preserve 'hdr'
> 
> Grr. I surely was too tired when staring at this last time.
> 

ohhh man, that's embarrassing. Especially hdr, which was the center of
the whole thing...sigh. Patch coming shortly.


thanks,
-- 
John Hubbard
NVIDIA

Re: Boot failure due to: x86/boot: Save fields explicitly, zero out everything else

[Neil MacLeod]

The fix looks good - many thanks for the quick turnaround!

Neil

On Wed, 21 Aug 2019 at 19:56, John Hubbard <jhubbard@nvidia.com> wrote:
>
> On 8/21/19 11:51 AM, Thomas Gleixner wrote:
> > On Wed, 21 Aug 2019, John Hubbard wrote:
> >> On 8/21/19 10:05 AM, Neil MacLeod wrote:
> >> static void sanitize_boot_params(struct boot_params *boot_params)
> >> {
> >> ...
> >>              const struct boot_params_to_save to_save[] = {
> >>                      BOOT_PARAM_PRESERVE(screen_info),
> >>                      BOOT_PARAM_PRESERVE(apm_bios_info),
> >>                      BOOT_PARAM_PRESERVE(tboot_addr),
> >>                      BOOT_PARAM_PRESERVE(ist_info),
> >>                      BOOT_PARAM_PRESERVE(acpi_rsdp_addr),
> >>                      BOOT_PARAM_PRESERVE(hd0_info),
> >>                      BOOT_PARAM_PRESERVE(hd1_info),
> >>                      BOOT_PARAM_PRESERVE(sys_desc_table),
> >>                      BOOT_PARAM_PRESERVE(olpc_ofw_header),
> >>                      BOOT_PARAM_PRESERVE(efi_info),
> >>                      BOOT_PARAM_PRESERVE(alt_mem_k),
> >>                      BOOT_PARAM_PRESERVE(scratch),
> >>                      BOOT_PARAM_PRESERVE(e820_entries),
> >>                      BOOT_PARAM_PRESERVE(eddbuf_entries),
> >>                      BOOT_PARAM_PRESERVE(edd_mbr_sig_buf_entries),
> >>                      BOOT_PARAM_PRESERVE(edd_mbr_sig_buffer),
> >>                      BOOT_PARAM_PRESERVE(e820_table),
> >>                      BOOT_PARAM_PRESERVE(eddbuf),
> >>              };
> >
> > I think I spotted it:
> >
> > -               boot_params->acpi_rsdp_addr = 0;
> >
> > +                     BOOT_PARAM_PRESERVE(acpi_rsdp_addr),
> >
> > And it does not preserve 'hdr'
> >
> > Grr. I surely was too tired when staring at this last time.
> >
>
> ohhh man, that's embarrassing. Especially hdr, which was the center of
> the whole thing...sigh. Patch coming shortly.
>
>
> thanks,
> --
> John Hubbard
> NVIDIA

Re: Boot failure due to: x86/boot: Save fields explicitly, zero out everything else

[John Hubbard]

On 8/21/19 12:49 PM, Neil MacLeod wrote:
> The fix looks good - many thanks for the quick turnaround!
> 

Great news, and thanks for the bug report!

thanks,
-- 
John Hubbard
NVIDIA

> 
> On Wed, 21 Aug 2019 at 19:56, John Hubbard <jhubbard@nvidia.com> wrote:
>>
>> On 8/21/19 11:51 AM, Thomas Gleixner wrote:
>>> On Wed, 21 Aug 2019, John Hubbard wrote:
>>>> On 8/21/19 10:05 AM, Neil MacLeod wrote:
>>>> static void sanitize_boot_params(struct boot_params *boot_params)
>>>> {
>>>> ...
>>>>               const struct boot_params_to_save to_save[] = {
>>>>                       BOOT_PARAM_PRESERVE(screen_info),
>>>>                       BOOT_PARAM_PRESERVE(apm_bios_info),
>>>>                       BOOT_PARAM_PRESERVE(tboot_addr),
>>>>                       BOOT_PARAM_PRESERVE(ist_info),
>>>>                       BOOT_PARAM_PRESERVE(acpi_rsdp_addr),
>>>>                       BOOT_PARAM_PRESERVE(hd0_info),
>>>>                       BOOT_PARAM_PRESERVE(hd1_info),
>>>>                       BOOT_PARAM_PRESERVE(sys_desc_table),
>>>>                       BOOT_PARAM_PRESERVE(olpc_ofw_header),
>>>>                       BOOT_PARAM_PRESERVE(efi_info),
>>>>                       BOOT_PARAM_PRESERVE(alt_mem_k),
>>>>                       BOOT_PARAM_PRESERVE(scratch),
>>>>                       BOOT_PARAM_PRESERVE(e820_entries),
>>>>                       BOOT_PARAM_PRESERVE(eddbuf_entries),
>>>>                       BOOT_PARAM_PRESERVE(edd_mbr_sig_buf_entries),
>>>>                       BOOT_PARAM_PRESERVE(edd_mbr_sig_buffer),
>>>>                       BOOT_PARAM_PRESERVE(e820_table),
>>>>                       BOOT_PARAM_PRESERVE(eddbuf),
>>>>               };
>>>
>>> I think I spotted it:
>>>
>>> -               boot_params->acpi_rsdp_addr = 0;
>>>
>>> +                     BOOT_PARAM_PRESERVE(acpi_rsdp_addr),
>>>
>>> And it does not preserve 'hdr'
>>>
>>> Grr. I surely was too tired when staring at this last time.
>>>
>>
>> ohhh man, that's embarrassing. Especially hdr, which was the center of
>> the whole thing...sigh. Patch coming shortly.
>>
>>
>> thanks,
>> --
>> John Hubbard
>> NVIDIA

Re: Boot failure due to: x86/boot: Save fields explicitly, zero out everything else

[Neil MacLeod]

John & Thomas

Thanks both - if you can ping me a suitable patch I'll test it and let
you all know ASAP!

Neil

On Wed, 21 Aug 2019 at 19:51, Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Wed, 21 Aug 2019, John Hubbard wrote:
> > On 8/21/19 10:05 AM, Neil MacLeod wrote:
> > static void sanitize_boot_params(struct boot_params *boot_params)
> > {
> > ...
> >               const struct boot_params_to_save to_save[] = {
> >                       BOOT_PARAM_PRESERVE(screen_info),
> >                       BOOT_PARAM_PRESERVE(apm_bios_info),
> >                       BOOT_PARAM_PRESERVE(tboot_addr),
> >                       BOOT_PARAM_PRESERVE(ist_info),
> >                       BOOT_PARAM_PRESERVE(acpi_rsdp_addr),
> >                       BOOT_PARAM_PRESERVE(hd0_info),
> >                       BOOT_PARAM_PRESERVE(hd1_info),
> >                       BOOT_PARAM_PRESERVE(sys_desc_table),
> >                       BOOT_PARAM_PRESERVE(olpc_ofw_header),
> >                       BOOT_PARAM_PRESERVE(efi_info),
> >                       BOOT_PARAM_PRESERVE(alt_mem_k),
> >                       BOOT_PARAM_PRESERVE(scratch),
> >                       BOOT_PARAM_PRESERVE(e820_entries),
> >                       BOOT_PARAM_PRESERVE(eddbuf_entries),
> >                       BOOT_PARAM_PRESERVE(edd_mbr_sig_buf_entries),
> >                       BOOT_PARAM_PRESERVE(edd_mbr_sig_buffer),
> >                       BOOT_PARAM_PRESERVE(e820_table),
> >                       BOOT_PARAM_PRESERVE(eddbuf),
> >               };
>
> I think I spotted it:
>
> -               boot_params->acpi_rsdp_addr = 0;
>
> +                       BOOT_PARAM_PRESERVE(acpi_rsdp_addr),
>
> And it does not preserve 'hdr'
>
> Grr. I surely was too tired when staring at this last time.
>
> Thanks,
>
>         tglx

[PATCH] x86/boot: Fix boot failure regression

[John Hubbard]

commit a90118c445cc ("x86/boot: Save fields explicitly, zero out
everything else") had two errors:

    * It preserved boot_params.acpi_rsdp_addr, and
    * It failed to preserve boot_params.hdr

Therefore, zero out acpi_rsdp_addr, and preserve hdr.

Fixes: a90118c445cc ("x86/boot: Save fields explicitly, zero out everything else")
Reported-by: Neil MacLeod <neil@nmacleod.com>
Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: stable@vger.kernel.org
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 arch/x86/include/asm/bootparam_utils.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/bootparam_utils.h b/arch/x86/include/asm/bootparam_utils.h
index f5e90a849bca..9e5f3c722c33 100644
--- a/arch/x86/include/asm/bootparam_utils.h
+++ b/arch/x86/include/asm/bootparam_utils.h
@@ -59,7 +59,6 @@ static void sanitize_boot_params(struct boot_params *boot_params)
 			BOOT_PARAM_PRESERVE(apm_bios_info),
 			BOOT_PARAM_PRESERVE(tboot_addr),
 			BOOT_PARAM_PRESERVE(ist_info),
-			BOOT_PARAM_PRESERVE(acpi_rsdp_addr),
 			BOOT_PARAM_PRESERVE(hd0_info),
 			BOOT_PARAM_PRESERVE(hd1_info),
 			BOOT_PARAM_PRESERVE(sys_desc_table),
@@ -71,6 +70,7 @@ static void sanitize_boot_params(struct boot_params *boot_params)
 			BOOT_PARAM_PRESERVE(eddbuf_entries),
 			BOOT_PARAM_PRESERVE(edd_mbr_sig_buf_entries),
 			BOOT_PARAM_PRESERVE(edd_mbr_sig_buffer),
+			BOOT_PARAM_PRESERVE(hdr),
 			BOOT_PARAM_PRESERVE(e820_table),
 			BOOT_PARAM_PRESERVE(eddbuf),
 		};
-- 
2.22.1

Re: [PATCH] x86/boot: Fix boot failure regression

[Neil MacLeod]

I can confirm 5.3-rc5 is booting again from internal M2 drive on
Skylake i5 NUC with this commit - many thanks!

Regards
Neil

On Wed, 21 Aug 2019 at 20:25, John Hubbard <jhubbard@nvidia.com> wrote:
>
> commit a90118c445cc ("x86/boot: Save fields explicitly, zero out
> everything else") had two errors:
>
>     * It preserved boot_params.acpi_rsdp_addr, and
>     * It failed to preserve boot_params.hdr
>
> Therefore, zero out acpi_rsdp_addr, and preserve hdr.
>
> Fixes: a90118c445cc ("x86/boot: Save fields explicitly, zero out everything else")
> Reported-by: Neil MacLeod <neil@nmacleod.com>
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: H. Peter Anvin <hpa@zytor.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> ---
>  arch/x86/include/asm/bootparam_utils.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/bootparam_utils.h b/arch/x86/include/asm/bootparam_utils.h
> index f5e90a849bca..9e5f3c722c33 100644
> --- a/arch/x86/include/asm/bootparam_utils.h
> +++ b/arch/x86/include/asm/bootparam_utils.h
> @@ -59,7 +59,6 @@ static void sanitize_boot_params(struct boot_params *boot_params)
>                         BOOT_PARAM_PRESERVE(apm_bios_info),
>                         BOOT_PARAM_PRESERVE(tboot_addr),
>                         BOOT_PARAM_PRESERVE(ist_info),
> -                       BOOT_PARAM_PRESERVE(acpi_rsdp_addr),
>                         BOOT_PARAM_PRESERVE(hd0_info),
>                         BOOT_PARAM_PRESERVE(hd1_info),
>                         BOOT_PARAM_PRESERVE(sys_desc_table),
> @@ -71,6 +70,7 @@ static void sanitize_boot_params(struct boot_params *boot_params)
>                         BOOT_PARAM_PRESERVE(eddbuf_entries),
>                         BOOT_PARAM_PRESERVE(edd_mbr_sig_buf_entries),
>                         BOOT_PARAM_PRESERVE(edd_mbr_sig_buffer),
> +                       BOOT_PARAM_PRESERVE(hdr),
>                         BOOT_PARAM_PRESERVE(e820_table),
>                         BOOT_PARAM_PRESERVE(eddbuf),
>                 };
> --
> 2.22.1
>

Re: [PATCH] x86/boot: Fix boot failure regression

[Thomas Gleixner]

On Wed, 21 Aug 2019, Neil MacLeod wrote:

> I can confirm 5.3-rc5 is booting again from internal M2 drive on
> Skylake i5 NUC with this commit - many thanks!

I've queued that in x86/urgent and it's en route for rc6 and stable.

Thanks!

	tglx

Re: [PATCH] x86/boot: Fix boot failure regression

[Greg KH]

On Wed, Aug 21, 2019 at 11:24:28PM +0200, Thomas Gleixner wrote:
> On Wed, 21 Aug 2019, Neil MacLeod wrote:
> 
> > I can confirm 5.3-rc5 is booting again from internal M2 drive on
> > Skylake i5 NUC with this commit - many thanks!
> 
> I've queued that in x86/urgent and it's en route for rc6 and stable.

I've dropped the original patch from the stable trees now to wait for
this fix to hit Linus's tree.

Also the original doesn't seem to have fixed the build warning I'm
seeing on my Fedora test build systems, which is odd, I need to dig into
that...

thanks,

greg k-h

[tip: x86/urgent] x86/boot: Fix boot regression caused by bootparam sanitizing

[tip-bot2 for John Hubbard]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     7846f58fba964af7cb8cf77d4d13c33254725211
Gitweb:        https://git.kernel.org/tip/7846f58fba964af7cb8cf77d4d13c33254725211
Author:        John Hubbard <jhubbard@nvidia.com>
AuthorDate:    Wed, 21 Aug 2019 12:25:13 -07:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Wed, 21 Aug 2019 22:37:09 +02:00

x86/boot: Fix boot regression caused by bootparam sanitizing

commit a90118c445cc ("x86/boot: Save fields explicitly, zero out everything
else") had two errors:

    * It preserved boot_params.acpi_rsdp_addr, and
    * It failed to preserve boot_params.hdr

Therefore, zero out acpi_rsdp_addr, and preserve hdr.

Fixes: a90118c445cc ("x86/boot: Save fields explicitly, zero out everything else")
Reported-by: Neil MacLeod <neil@nmacleod.com>
Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Neil MacLeod <neil@nmacleod.com>
Cc: stable@vger.kernel.org
Link: https://lkml.kernel.org/r/20190821192513.20126-1-jhubbard@nvidia.com
---
 arch/x86/include/asm/bootparam_utils.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/bootparam_utils.h b/arch/x86/include/asm/bootparam_utils.h
index 9e5f3c7..f5e90a8 100644
--- a/arch/x86/include/asm/bootparam_utils.h
+++ b/arch/x86/include/asm/bootparam_utils.h
@@ -59,6 +59,7 @@ static void sanitize_boot_params(struct boot_params *boot_params)
 			BOOT_PARAM_PRESERVE(apm_bios_info),
 			BOOT_PARAM_PRESERVE(tboot_addr),
 			BOOT_PARAM_PRESERVE(ist_info),
+			BOOT_PARAM_PRESERVE(acpi_rsdp_addr),
 			BOOT_PARAM_PRESERVE(hd0_info),
 			BOOT_PARAM_PRESERVE(hd1_info),
 			BOOT_PARAM_PRESERVE(sys_desc_table),
@@ -70,7 +71,6 @@ static void sanitize_boot_params(struct boot_params *boot_params)
 			BOOT_PARAM_PRESERVE(eddbuf_entries),
 			BOOT_PARAM_PRESERVE(edd_mbr_sig_buf_entries),
 			BOOT_PARAM_PRESERVE(edd_mbr_sig_buffer),
-			BOOT_PARAM_PRESERVE(hdr),
 			BOOT_PARAM_PRESERVE(e820_table),
 			BOOT_PARAM_PRESERVE(eddbuf),
 		};