[PATCH 0/1] x86/boot: clear some fields explicitly

[PATCH 0/1] x86/boot: clear some fields explicitly

[john.hubbard]

From: John Hubbard <jhubbard@nvidia.com>

Hi,

I just ran across this on the latest (well, any -rc1) linux.git, while
working on something else. It generates an ugly multiline warning:

In file included from arch/x86/kernel/head64.c:35:
In function ?sanitize_boot_params?,
    inlined from ?copy_bootdata? at arch/x86/kernel/head64.c:391:2:
./arch/x86/include/asm/bootparam_utils.h:40:3: warning:
?memset? offset [197, 448] from the object at ?boot_params? is out
of the bounds of referenced subobject ?ext_ramdisk_image? with type
?unsigned int? at offset 192 [-Warray-bounds]
   40 |   memset(&boot_params->ext_ramdisk_image, 0,
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   41 |          (char *)&boot_params->efi_info -
      |          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   42 |    (char *)&boot_params->ext_ramdisk_image);
      |    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./arch/x86/include/asm/bootparam_utils.h:43:3: warning: ?memset?
offset [493, 497] from the object at ?boot_params? is out of the
bounds of referenced subobject ?kbd_status? with type
?unsigned char? at offset 491 [-Warray-bounds]
   43 |   memset(&boot_params->kbd_status, 0,
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   44 |          (char *)&boot_params->hdr -
      |          ~~~~~~~~~~~~~~~~~~~~~~~~~~~
   45 |          (char *)&boot_params->kbd_status);
      |          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

It looks like this hasn't been touched since 2013, so maybe gcc is
just getting more agressive about warnings. Well, we know it is, actually.

Because struct boot_params is __packed__, normal variable
variable assignment will work just as well as a memset here.
Change three u32 fields to be cleared to zero that way, and
just memset the _pad4 field.

This clears up the build warnings for me.

Testing: just did a basic boot test on my x86_64 machine, no problems
seen. Not that that tests much, but it's something.

John Hubbard (1):
  x86/boot: clear some fields explicitly

 arch/x86/include/asm/bootparam_utils.h | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

-- 
2.22.0

[PATCH 1/1] x86/boot: clear some fields explicitly

[john.hubbard]

From: John Hubbard <jhubbard@nvidia.com>

Recent gcc compilers (gcc 9.1) generate warnings about an
out of bounds memset, if you trying memset across several fields
of a struct. This generated a couple of warnings on x86_64 builds.

Because struct boot_params is __packed__, normal variable
variable assignment will work just as well as a memset here.
Change three u32 fields to be cleared to zero that way, and
just memset the _pad4 field.

This clears up the build warnings for me.

Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 arch/x86/include/asm/bootparam_utils.h | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/bootparam_utils.h b/arch/x86/include/asm/bootparam_utils.h
index 101eb944f13c..4df87d4a043b 100644
--- a/arch/x86/include/asm/bootparam_utils.h
+++ b/arch/x86/include/asm/bootparam_utils.h
@@ -37,12 +37,11 @@ static void sanitize_boot_params(struct boot_params *boot_params)
 	if (boot_params->sentinel) {
 		/* fields in boot_params are left uninitialized, clear them */
 		boot_params->acpi_rsdp_addr = 0;
-		memset(&boot_params->ext_ramdisk_image, 0,
-		       (char *)&boot_params->efi_info -
-			(char *)&boot_params->ext_ramdisk_image);
-		memset(&boot_params->kbd_status, 0,
-		       (char *)&boot_params->hdr -
-		       (char *)&boot_params->kbd_status);
+		boot_params->ext_ramdisk_image = 0;
+		boot_params->ext_ramdisk_size = 0;
+		boot_params->ext_cmd_line_ptr = 0;
+
+		memset(&boot_params->_pad4, 0, sizeof(boot_params->_pad4));
 		memset(&boot_params->_pad7[0], 0,
 		       (char *)&boot_params->edd_mbr_sig_buffer[0] -
 			(char *)&boot_params->_pad7[0]);
-- 
2.22.0

Re: [PATCH 1/1] x86/boot: clear some fields explicitly

[hpa]

On July 24, 2019 4:15:28 PM PDT, john.hubbard@gmail.com wrote:
>From: John Hubbard <jhubbard@nvidia.com>
>
>Recent gcc compilers (gcc 9.1) generate warnings about an
>out of bounds memset, if you trying memset across several fields
>of a struct. This generated a couple of warnings on x86_64 builds.
>
>Because struct boot_params is __packed__, normal variable
>variable assignment will work just as well as a memset here.
>Change three u32 fields to be cleared to zero that way, and
>just memset the _pad4 field.
>
>This clears up the build warnings for me.
>
>Signed-off-by: John Hubbard <jhubbard@nvidia.com>
>---
> arch/x86/include/asm/bootparam_utils.h | 11 +++++------
> 1 file changed, 5 insertions(+), 6 deletions(-)
>
>diff --git a/arch/x86/include/asm/bootparam_utils.h
>b/arch/x86/include/asm/bootparam_utils.h
>index 101eb944f13c..4df87d4a043b 100644
>--- a/arch/x86/include/asm/bootparam_utils.h
>+++ b/arch/x86/include/asm/bootparam_utils.h
>@@ -37,12 +37,11 @@ static void sanitize_boot_params(struct boot_params
>*boot_params)
> 	if (boot_params->sentinel) {
> 		/* fields in boot_params are left uninitialized, clear them */
> 		boot_params->acpi_rsdp_addr = 0;
>-		memset(&boot_params->ext_ramdisk_image, 0,
>-		       (char *)&boot_params->efi_info -
>-			(char *)&boot_params->ext_ramdisk_image);
>-		memset(&boot_params->kbd_status, 0,
>-		       (char *)&boot_params->hdr -
>-		       (char *)&boot_params->kbd_status);
>+		boot_params->ext_ramdisk_image = 0;
>+		boot_params->ext_ramdisk_size = 0;
>+		boot_params->ext_cmd_line_ptr = 0;
>+
>+		memset(&boot_params->_pad4, 0, sizeof(boot_params->_pad4));
> 		memset(&boot_params->_pad7[0], 0,
> 		       (char *)&boot_params->edd_mbr_sig_buffer[0] -
> 			(char *)&boot_params->_pad7[0]);

The problem with this is that it will break silently when changes are made to this structure.

So, that is a NAK from me.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH 1/1] x86/boot: clear some fields explicitly

[John Hubbard]

On 7/24/19 7:12 PM, hpa@zytor.com wrote:
> On July 24, 2019 4:15:28 PM PDT, john.hubbard@gmail.com wrote:
>> From: John Hubbard <jhubbard@nvidia.com>
...
>> +		boot_params->ext_ramdisk_image = 0;
>> +		boot_params->ext_ramdisk_size = 0;
>> +		boot_params->ext_cmd_line_ptr = 0;
>> +
>> +		memset(&boot_params->_pad4, 0, sizeof(boot_params->_pad4));
>> 		memset(&boot_params->_pad7[0], 0,
>> 		       (char *)&boot_params->edd_mbr_sig_buffer[0] -
>> 			(char *)&boot_params->_pad7[0]);
> 
> The problem with this is that it will break silently when changes are made to this structure.
> 
> So, that is a NAK from me.
> 

Understood. It occurs to me, though, that it would be trivial to
just add build time assertions to check a few struct member offset
values, and fail out if they changed. That would give us everything:
warnings-free builds, and no silent failures.

Thoughts?

thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH 1/1] x86/boot: clear some fields explicitly

[Thomas Gleixner]

On Wed, 24 Jul 2019, hpa@zytor.com wrote:
> On July 24, 2019 4:15:28 PM PDT, john.hubbard@gmail.com wrote:
> >From: John Hubbard <jhubbard@nvidia.com>
> >
> >Recent gcc compilers (gcc 9.1) generate warnings about an
> >out of bounds memset, if you trying memset across several fields
> >of a struct. This generated a couple of warnings on x86_64 builds.
> >
> >Because struct boot_params is __packed__, normal variable
> >variable assignment will work just as well as a memset here.
> >Change three u32 fields to be cleared to zero that way, and
> >just memset the _pad4 field.
> >
> >This clears up the build warnings for me.
> >
> >Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> >---
> > arch/x86/include/asm/bootparam_utils.h | 11 +++++------
> > 1 file changed, 5 insertions(+), 6 deletions(-)
> >
> >diff --git a/arch/x86/include/asm/bootparam_utils.h
> >b/arch/x86/include/asm/bootparam_utils.h
> >index 101eb944f13c..4df87d4a043b 100644
> >--- a/arch/x86/include/asm/bootparam_utils.h
> >+++ b/arch/x86/include/asm/bootparam_utils.h
> >@@ -37,12 +37,11 @@ static void sanitize_boot_params(struct boot_params
> >*boot_params)
> > 	if (boot_params->sentinel) {
> > 		/* fields in boot_params are left uninitialized, clear them */
> > 		boot_params->acpi_rsdp_addr = 0;
> >-		memset(&boot_params->ext_ramdisk_image, 0,
> >-		       (char *)&boot_params->efi_info -
> >-			(char *)&boot_params->ext_ramdisk_image);
> >-		memset(&boot_params->kbd_status, 0,
> >-		       (char *)&boot_params->hdr -
> >-		       (char *)&boot_params->kbd_status);
> >+		boot_params->ext_ramdisk_image = 0;
> >+		boot_params->ext_ramdisk_size = 0;
> >+		boot_params->ext_cmd_line_ptr = 0;
> >+
> >+		memset(&boot_params->_pad4, 0, sizeof(boot_params->_pad4));
> > 		memset(&boot_params->_pad7[0], 0,
> > 		       (char *)&boot_params->edd_mbr_sig_buffer[0] -
> > 			(char *)&boot_params->_pad7[0]);
> 
> The problem with this is that it will break silently when changes are
> made to this structure.

That's not really the worst problem. Changes to that struct which touch any
of the to be cleared ranges will break anyway if not handled correctly in
the sanitizer function.

What's worse is that the patch is broken.  It 'clears' the build warnings,
but not all the fields which have been cleared before:

It removes the clearing of the range between kbd_status and hdr without any
replacement. It neither clears edid_info.

The above approach is doomed and if we have to handle this GCC0.1 madness
then this needs to be done smarter. Something like the completely untested
thing below.

Thanks,

	tglx

8<--------------
diff --git a/arch/x86/include/asm/bootparam_utils.h b/arch/x86/include/asm/bootparam_utils.h
index 101eb944f13c..f5bc4c01b66b 100644
--- a/arch/x86/include/asm/bootparam_utils.h
+++ b/arch/x86/include/asm/bootparam_utils.h
@@ -9,6 +9,18 @@
  * add completing #includes to make it standalone.
  */
 
+struct boot_params_clear {
+	unsigned int	offs;
+	unsigned int	len;
+};
+
+#define BOOT_PARAM_CLEAR(start, end)				\
+{								\
+	.offs	= offsetof(struct boot_params, start),		\
+	.len	= offsetof(struct boot_params, end) -		\
+		  offsetof(struct boot_params, start),		\
+}
+
 /*
  * Deal with bootloaders which fail to initialize unknown fields in
  * boot_params to zero.  The list fields in this list are taken from
@@ -20,7 +32,19 @@
  */
 static void sanitize_boot_params(struct boot_params *boot_params)
 {
-	/* 
+	const struct boot_params_clear toclear[] = {
+		BOOT_PARAM_CLEAR(acpi_rdsp_addr, _pad3),
+		BOOT_PARAM_CLEAR(ext_ramdisk_image, efi_info),
+		BOOT_PARAM_CLEAR(kbd_status, hdr),
+		BOOT_PARAM_CLEAR(_pad7, edd_mbr_sig_buffer),
+		BOOT_PARAM_CLEAR(_pad8, eddbuf),
+		{
+			.offs	= offsetof(struct boot_params, _pad9),
+			.len	= sizeof(boot_params->_pad9),
+		},
+	}
+
+	/*
 	 * IMPORTANT NOTE TO BOOTLOADER AUTHORS: do not simply clear
 	 * this field.  The purpose of this field is to guarantee
 	 * compliance with the x86 boot spec located in
@@ -36,20 +60,11 @@ static void sanitize_boot_params(struct boot_params *boot_params)
 	 */
 	if (boot_params->sentinel) {
 		/* fields in boot_params are left uninitialized, clear them */
-		boot_params->acpi_rsdp_addr = 0;
-		memset(&boot_params->ext_ramdisk_image, 0,
-		       (char *)&boot_params->efi_info -
-			(char *)&boot_params->ext_ramdisk_image);
-		memset(&boot_params->kbd_status, 0,
-		       (char *)&boot_params->hdr -
-		       (char *)&boot_params->kbd_status);
-		memset(&boot_params->_pad7[0], 0,
-		       (char *)&boot_params->edd_mbr_sig_buffer[0] -
-			(char *)&boot_params->_pad7[0]);
-		memset(&boot_params->_pad8[0], 0,
-		       (char *)&boot_params->eddbuf[0] -
-			(char *)&boot_params->_pad8[0]);
-		memset(&boot_params->_pad9[0], 0, sizeof(boot_params->_pad9));
+		char *p = (char *) boot_params;
+		int i;
+
+		for (i = 0; i < ARRAY_SIZE(toclear); i++)
+			memset(p + toclear[i].start, 0, toclear[i].len);
 	}
 }
 

Re: [PATCH 1/1] x86/boot: clear some fields explicitly

[John Hubbard]

On 7/25/19 12:22 AM, Thomas Gleixner wrote:
> On Wed, 24 Jul 2019, hpa@zytor.com wrote:
>> On July 24, 2019 4:15:28 PM PDT, john.hubbard@gmail.com wrote:
>>> From: John Hubbard <jhubbard@nvidia.com>
>>>
>>> Recent gcc compilers (gcc 9.1) generate warnings about an
>>> out of bounds memset, if you trying memset across several fields
>>> of a struct. This generated a couple of warnings on x86_64 builds.
>>>
>>> Because struct boot_params is __packed__, normal variable
>>> variable assignment will work just as well as a memset here.
>>> Change three u32 fields to be cleared to zero that way, and
>>> just memset the _pad4 field.
>>>
>>> This clears up the build warnings for me.
>>>
>>> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
>>> ---
>>> arch/x86/include/asm/bootparam_utils.h | 11 +++++------
>>> 1 file changed, 5 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/arch/x86/include/asm/bootparam_utils.h
>>> b/arch/x86/include/asm/bootparam_utils.h
>>> index 101eb944f13c..4df87d4a043b 100644
>>> --- a/arch/x86/include/asm/bootparam_utils.h
>>> +++ b/arch/x86/include/asm/bootparam_utils.h
>>> @@ -37,12 +37,11 @@ static void sanitize_boot_params(struct boot_params
>>> *boot_params)
>>> 	if (boot_params->sentinel) {
>>> 		/* fields in boot_params are left uninitialized, clear them */
>>> 		boot_params->acpi_rsdp_addr = 0;
>>> -		memset(&boot_params->ext_ramdisk_image, 0,
>>> -		       (char *)&boot_params->efi_info -
>>> -			(char *)&boot_params->ext_ramdisk_image);
>>> -		memset(&boot_params->kbd_status, 0,
>>> -		       (char *)&boot_params->hdr -
>>> -		       (char *)&boot_params->kbd_status);
>>> +		boot_params->ext_ramdisk_image = 0;
>>> +		boot_params->ext_ramdisk_size = 0;
>>> +		boot_params->ext_cmd_line_ptr = 0;
>>> +
>>> +		memset(&boot_params->_pad4, 0, sizeof(boot_params->_pad4));
>>> 		memset(&boot_params->_pad7[0], 0,
>>> 		       (char *)&boot_params->edd_mbr_sig_buffer[0] -
>>> 			(char *)&boot_params->_pad7[0]);
>>
>> The problem with this is that it will break silently when changes are
>> made to this structure.
> 
> That's not really the worst problem. Changes to that struct which touch any
> of the to be cleared ranges will break anyway if not handled correctly in
> the sanitizer function.
> 
> What's worse is that the patch is broken.  It 'clears' the build warnings,
> but not all the fields which have been cleared before:
> 
> It removes the clearing of the range between kbd_status and hdr without any
> replacement. It neither clears edid_info.


Yes. Somehow I left that chunk out. Not my finest hour. 

> 
> The above approach is doomed and if we have to handle this GCC0.1 madness
> then this needs to be done smarter. Something like the completely untested
> thing below.
> 
> Thanks,
> 
> 	tglx
> 
> 8<--------------
> diff --git a/arch/x86/include/asm/bootparam_utils.h b/arch/x86/include/asm/bootparam_utils.h
> index 101eb944f13c..f5bc4c01b66b 100644
> --- a/arch/x86/include/asm/bootparam_utils.h
> +++ b/arch/x86/include/asm/bootparam_utils.h
> @@ -9,6 +9,18 @@
>   * add completing #includes to make it standalone.
>   */
>  
> +struct boot_params_clear {
> +	unsigned int	offs;
> +	unsigned int	len;
> +};
> +
> +#define BOOT_PARAM_CLEAR(start, end)				\
> +{								\
> +	.offs	= offsetof(struct boot_params, start),		\
> +	.len	= offsetof(struct boot_params, end) -		\
> +		  offsetof(struct boot_params, start),		\
> +}
> +
>  /*
>   * Deal with bootloaders which fail to initialize unknown fields in
>   * boot_params to zero.  The list fields in this list are taken from
> @@ -20,7 +32,19 @@
>   */
>  static void sanitize_boot_params(struct boot_params *boot_params)
>  {
> -	/* 
> +	const struct boot_params_clear toclear[] = {
> +		BOOT_PARAM_CLEAR(acpi_rdsp_addr, _pad3),
> +		BOOT_PARAM_CLEAR(ext_ramdisk_image, efi_info),
> +		BOOT_PARAM_CLEAR(kbd_status, hdr),
> +		BOOT_PARAM_CLEAR(_pad7, edd_mbr_sig_buffer),
> +		BOOT_PARAM_CLEAR(_pad8, eddbuf),
> +		{
> +			.offs	= offsetof(struct boot_params, _pad9),
> +			.len	= sizeof(boot_params->_pad9),
> +		},
> +	}
> +
> +	/*
>  	 * IMPORTANT NOTE TO BOOTLOADER AUTHORS: do not simply clear
>  	 * this field.  The purpose of this field is to guarantee
>  	 * compliance with the x86 boot spec located in
> @@ -36,20 +60,11 @@ static void sanitize_boot_params(struct boot_params *boot_params)
>  	 */
>  	if (boot_params->sentinel) {
>  		/* fields in boot_params are left uninitialized, clear them */
> -		boot_params->acpi_rsdp_addr = 0;
> -		memset(&boot_params->ext_ramdisk_image, 0,
> -		       (char *)&boot_params->efi_info -
> -			(char *)&boot_params->ext_ramdisk_image);
> -		memset(&boot_params->kbd_status, 0,
> -		       (char *)&boot_params->hdr -
> -		       (char *)&boot_params->kbd_status);
> -		memset(&boot_params->_pad7[0], 0,
> -		       (char *)&boot_params->edd_mbr_sig_buffer[0] -
> -			(char *)&boot_params->_pad7[0]);
> -		memset(&boot_params->_pad8[0], 0,
> -		       (char *)&boot_params->eddbuf[0] -
> -			(char *)&boot_params->_pad8[0]);
> -		memset(&boot_params->_pad9[0], 0, sizeof(boot_params->_pad9));
> +		char *p = (char *) boot_params;
> +		int i;
> +
> +		for (i = 0; i < ARRAY_SIZE(toclear); i++)
> +			memset(p + toclear[i].start, 0, toclear[i].len);
>  	}
>  }


Looks nice.


thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH 1/1] x86/boot: clear some fields explicitly

[H. Peter Anvin]

On 7/25/19 12:22 AM, Thomas Gleixner wrote:
>>
>> The problem with this is that it will break silently when changes are
>> made to this structure.
> 
> That's not really the worst problem. Changes to that struct which touch any
> of the to be cleared ranges will break anyway if not handled correctly in
> the sanitizer function.
> 

Not really... that's kind of the point (the cleared ranges are cleared
explicitly because the boot loader failed to do so, so zeroing them is what
the boot loader should have done.)

The most correct way to address this would be to have an explicit list of
members to be *preserved* even if the sentinel triggers.

The easy way would be to put in a suitable cast to clear the warning -- I
would not be surprised if an explicit cast to something like (void *) would
quiet the warning, or else (yuck) put in an explicit (well-commented) #pragma
to shut it up.

	-hpa

Re: [PATCH 1/1] x86/boot: clear some fields explicitly

[John Hubbard]

On 7/25/19 1:38 PM, H. Peter Anvin wrote:
> On 7/25/19 12:22 AM, Thomas Gleixner wrote:

> The easy way would be to put in a suitable cast to clear the warning -- I
> would not be surprised if an explicit cast to something like (void *) would
> quiet the warning, or else (yuck) put in an explicit (well-commented) #pragma
> to shut it up.
> 

I wish. The first thing I tried was a (void*) cast, and the second thing
was declaring a void pointer instead. But the new compiler is fiendishly 
clever, and figured me out in both cases. 

The #pragma I haven't tried, it seems like a bit too far.

thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH 1/1] x86/boot: clear some fields explicitly

[Thomas Gleixner]

On Thu, 25 Jul 2019, John Hubbard wrote:
> On 7/25/19 12:22 AM, Thomas Gleixner wrote:
> > It removes the clearing of the range between kbd_status and hdr without any
> > replacement. It neither clears edid_info.
> 
> 
> Yes. Somehow I left that chunk out. Not my finest hour. 

S*** happens

> > +		char *p = (char *) boot_params;
> > +		int i;
> > +
> > +		for (i = 0; i < ARRAY_SIZE(toclear); i++)
> > +			memset(p + toclear[i].start, 0, toclear[i].len);
> >  	}
> >  }
> 
> Looks nice.

I have no idea whether it works and I have no cycles either, so I would
appreciate it if you could polish it up so we can handle that new fangled
GCC "feature" nicely.

Alternatively file a bug report to the GCC folks :)

But seriously I think it's not completely insane what they are doing and
the table based approach is definitely more readable and maintainable than
the existing stuff.

Thanks,

	tglx

Re: [PATCH 1/1] x86/boot: clear some fields explicitly

[hpa]

On July 25, 2019 2:48:30 PM PDT, Thomas Gleixner <tglx@linutronix.de> wrote:
>On Thu, 25 Jul 2019, John Hubbard wrote:
>> On 7/25/19 12:22 AM, Thomas Gleixner wrote:
>> > It removes the clearing of the range between kbd_status and hdr
>without any
>> > replacement. It neither clears edid_info.
>> 
>> 
>> Yes. Somehow I left that chunk out. Not my finest hour. 
>
>S*** happens
>
>> > +		char *p = (char *) boot_params;
>> > +		int i;
>> > +
>> > +		for (i = 0; i < ARRAY_SIZE(toclear); i++)
>> > +			memset(p + toclear[i].start, 0, toclear[i].len);
>> >  	}
>> >  }
>> 
>> Looks nice.
>
>I have no idea whether it works and I have no cycles either, so I would
>appreciate it if you could polish it up so we can handle that new
>fangled
>GCC "feature" nicely.
>
>Alternatively file a bug report to the GCC folks :)
>
>But seriously I think it's not completely insane what they are doing
>and
>the table based approach is definitely more readable and maintainable
>than
>the existing stuff.
>
>Thanks,
>
>	tglx

Doing this table based does seem like a good idea.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH 1/1] x86/boot: clear some fields explicitly

[Thomas Gleixner]

On Thu, 25 Jul 2019, hpa@zytor.com wrote:
> On July 25, 2019 2:48:30 PM PDT, Thomas Gleixner <tglx@linutronix.de> wrote:
> > 
> > But seriously I think it's not completely insane what they are doing
> > and the table based approach is definitely more readable and maintainable
> > than the existing stuff.
> 
> Doing this table based does seem like a good idea.

The question is whether we use a 'toclear' table or a 'preserve' table. I'd
argue that the 'preserve' approach is saner.

> Sent from my Android device with K-9 Mail. Please excuse my brevity.

I surely excuse the brevity, but the formatting mess which that brevity app
creates is not excusable.

Thanks,

	tglx
---
Sent from a sane mail client. Nothing to excuse here. All failures are intentional.

Re: [PATCH 1/1] x86/boot: clear some fields explicitly

[H. Peter Anvin]

On 7/25/19 3:03 PM, Thomas Gleixner wrote:
> On Thu, 25 Jul 2019, hpa@zytor.com wrote:
>> On July 25, 2019 2:48:30 PM PDT, Thomas Gleixner <tglx@linutronix.de> wrote:
>>>
>>> But seriously I think it's not completely insane what they are doing
>>> and the table based approach is definitely more readable and maintainable
>>> than the existing stuff.
>>
>> Doing this table based does seem like a good idea.
> 
> The question is whether we use a 'toclear' table or a 'preserve' table. I'd
> argue that the 'preserve' approach is saner.
> 

I agree.

>> Sent from my Android device with K-9 Mail. Please excuse my brevity.
> 
> I surely excuse the brevity, but the formatting mess which that brevity app
> creates is not excusable.
> 

I'll try to improve it...

	-hpa

Re: [PATCH 1/1] x86/boot: clear some fields explicitly

[Thomas Gleixner]

On Thu, 25 Jul 2019, H. Peter Anvin wrote:
> On 7/25/19 3:03 PM, Thomas Gleixner wrote:
> > On Thu, 25 Jul 2019, hpa@zytor.com wrote:
> >> On July 25, 2019 2:48:30 PM PDT, Thomas Gleixner <tglx@linutronix.de> wrote:
> >>>
> >>> But seriously I think it's not completely insane what they are doing
> >>> and the table based approach is definitely more readable and maintainable
> >>> than the existing stuff.
> >>
> >> Doing this table based does seem like a good idea.
> > 
> > The question is whether we use a 'toclear' table or a 'preserve' table. I'd
> > argue that the 'preserve' approach is saner.
> > 
> I agree.

Now we just need to volunteer someone to do that :)

> >> Sent from my Android device with K-9 Mail. Please excuse my brevity.
> > 
> > I surely excuse the brevity, but the formatting mess which that brevity app
> > creates is not excusable.
> > 
> 
> I'll try to improve it...

SCNR ...

Re: [PATCH 1/1] x86/boot: clear some fields explicitly

[John Hubbard]

On 7/25/19 3:28 PM, H. Peter Anvin wrote:
> On 7/25/19 3:03 PM, Thomas Gleixner wrote:
>> On Thu, 25 Jul 2019, hpa@zytor.com wrote:
>>> On July 25, 2019 2:48:30 PM PDT, Thomas Gleixner <tglx@linutronix.de> wrote:
>>>>
>>>> But seriously I think it's not completely insane what they are doing
>>>> and the table based approach is definitely more readable and maintainable
>>>> than the existing stuff.
>>>
>>> Doing this table based does seem like a good idea.
>>
>> The question is whether we use a 'toclear' table or a 'preserve' table. I'd
>> argue that the 'preserve' approach is saner.
>>
> 
> I agree.
> 

OK, I can polish up something and post it, if you can help me with one more
quick question: how did you want "to preserve" to work?

a) copy out fields to preserve, memset the area to zero, copy back preserved
fields? This seems like it would have the same gcc warnings as we have now,
due to the requirement to memset a range of a struct... 

b) Iterate through all fields, memsetting to zero items that are *not*
marked "to preserve"?

c) Something else? Sorry for the naivete here.  I really did read 
Documentation/x86/boot.rst, honest. :)



thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH 1/1] x86/boot: clear some fields explicitly

[John Hubbard]

On 7/25/19 3:37 PM, Thomas Gleixner wrote:
> On Thu, 25 Jul 2019, H. Peter Anvin wrote:
>> On 7/25/19 3:03 PM, Thomas Gleixner wrote:
>>> On Thu, 25 Jul 2019, hpa@zytor.com wrote:
>>>> On July 25, 2019 2:48:30 PM PDT, Thomas Gleixner <tglx@linutronix.de> wrote:
>>>>>
>>>>> But seriously I think it's not completely insane what they are doing
>>>>> and the table based approach is definitely more readable and maintainable
>>>>> than the existing stuff.
>>>>
>>>> Doing this table based does seem like a good idea.
>>>
>>> The question is whether we use a 'toclear' table or a 'preserve' table. I'd
>>> argue that the 'preserve' approach is saner.
>>>
>> I agree.
> 
> Now we just need to volunteer someone to do that :)
> 

Happy to jump in and do that, since I have an easy repro of the warning here.

In case you missed an earlier response [1], I did have a lingering question 
about what you had in mind for the "to preserve" approach:

[1] https://lore.kernel.org/r/ffd7a9b6-8017-2d2c-c4f7-65563094ccd0@nvidia.com

thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH 1/1] x86/boot: clear some fields explicitly

[Thomas Gleixner]

On Thu, 25 Jul 2019, John Hubbard wrote:
> On 7/25/19 3:28 PM, H. Peter Anvin wrote:
> > On 7/25/19 3:03 PM, Thomas Gleixner wrote:
> >> On Thu, 25 Jul 2019, hpa@zytor.com wrote:
> >>> On July 25, 2019 2:48:30 PM PDT, Thomas Gleixner <tglx@linutronix.de> wrote:
> >>>>
> >>>> But seriously I think it's not completely insane what they are doing
> >>>> and the table based approach is definitely more readable and maintainable
> >>>> than the existing stuff.
> >>>
> >>> Doing this table based does seem like a good idea.
> >>
> >> The question is whether we use a 'toclear' table or a 'preserve' table. I'd
> >> argue that the 'preserve' approach is saner.
> >>
> > 
> > I agree.
> > 
> 
> OK, I can polish up something and post it, if you can help me with one more
> quick question: how did you want "to preserve" to work?
> 
> a) copy out fields to preserve, memset the area to zero, copy back preserved
> fields? This seems like it would have the same gcc warnings as we have now,
> due to the requirement to memset a range of a struct... 

Use the same trick I used for the toclear variant.

#define PRESERVE(m)			\
	{				\
		.start = offsetof(m),	\
		.len   = sizeof(m),	\
	}

sanitize_boot_params(bp, scratch)
{
	char *p1 = bp, *p2 = scratch;

	preserve[] = {
		PRESERVE(member1),
		...
		PRESERVE(memberN),
	};

	for_each_preserve(pr)
		memcpy(p2 + pr->start, p1 + pr->start, pr->len)
	memcpy(bp, scratch, ...);
}


Thanks,

	tglx