[PATCH] romsignature/checksum cleanup

[PATCH] romsignature/checksum cleanup

[Rene Herman]

[-- Attachment #1: Type: text/plain, Size: 204 bytes --]

Hi Andrew.

Use adding __init to romsignature() (it's only called from probe_roms() 
which is itself __init) as an excuse to submit a pedantic cleanup.

Signed-off-by: Rene Herman <rene.herman@gmail.com>

[-- Attachment #2: romsignature-checksum-cleanup.diff --]
[-- Type: text/plain, Size: 916 bytes --]

diff --git a/arch/i386/kernel/e820.c b/arch/i386/kernel/e820.c
index f391abc..2565fac 100644
--- a/arch/i386/kernel/e820.c
+++ b/arch/i386/kernel/e820.c
@@ -156,21 +156,22 @@ static struct resource standard_io_resou
 	.flags	= IORESOURCE_BUSY | IORESOURCE_IO
 } };
 
-static int romsignature(const unsigned char *x)
+#define ROMSIGNATURE 0xaa55
+
+static int __init romsignature(const unsigned char *rom)
 {
 	unsigned short sig;
-	int ret = 0;
-	if (probe_kernel_address((const unsigned short *)x, sig) == 0)
-		ret = (sig == 0xaa55);
-	return ret;
+
+	return probe_kernel_address((const unsigned short *)rom, sig) == 0 &&
+	       sig == ROMSIGNATURE;
 }
 
 static int __init romchecksum(unsigned char *rom, unsigned long length)
 {
-	unsigned char *p, sum = 0;
+	unsigned char sum;
 
-	for (p = rom; p < rom + length; p++)
-		sum += *p;
+	for (sum = 0; length; length--)
+		sum += *rom++;
 	return sum == 0;
 }
 

Re: [PATCH] romsignature/checksum cleanup

[Rene Herman]

Rene Herman wrote:

> Use adding __init to romsignature() (it's only called from probe_roms() 
> which is itself __init) as an excuse to submit a pedantic cleanup.

Hmm, by the way, if romsignature() needs this probe_kernel_address() 
thing, why doesn't romchecksum()?

(Rusty in CC as author of bd472c794bbf6771c3fc1c58f188bc16c393d2fe)

Rene.

Re: [PATCH] romsignature/checksum cleanup

[Rusty Russell]

On Mon, 2006-12-25 at 01:53 +0100, Rene Herman wrote:
> Rene Herman wrote:
> 
> > Use adding __init to romsignature() (it's only called from probe_roms() 
> > which is itself __init) as an excuse to submit a pedantic cleanup.
> 
> Hmm, by the way, if romsignature() needs this probe_kernel_address() 
> thing, why doesn't romchecksum()?

I assume it's all in the same page, but CC'ing Zach is easier than
reading the code 8)

Rusty.

Re: [PATCH] romsignature/checksum cleanup

[Rene Herman]

Rusty Russell wrote:

> On Mon, 2006-12-25 at 01:53 +0100, Rene Herman wrote:

>> Hmm, by the way, if romsignature() needs this probe_kernel_address() 
>> thing, why doesn't romchecksum()?
> 
> I assume it's all in the same page, but CC'ing Zach is easier than
> reading the code 8)

If we're talking hardware pages here; the romchecksum() might be done 
over an area upto 0xff x 512 = 130560 bytes (there's also an acces to 
the length byte at rom[2] in probe_roms(). I assume that one's okay if 
romsignature() ensured that the first page is in).

Rene.

Re: [PATCH] romsignature/checksum cleanup

[Zachary Amsden]

Rusty Russell wrote:
> On Mon, 2006-12-25 at 01:53 +0100, Rene Herman wrote:
>   
>> Rene Herman wrote:
>>
>>     
>>> Use adding __init to romsignature() (it's only called from probe_roms() 
>>> which is itself __init) as an excuse to submit a pedantic cleanup.
>>>       
>> Hmm, by the way, if romsignature() needs this probe_kernel_address() 
>> thing, why doesn't romchecksum()?
>>     
>
> I assume it's all in the same page, but CC'ing Zach is easier than
> reading the code 8)
>   

Some hypervisors don't emulate the traditional physical layout of the 
first 1M of memory, so those pages might never get physical mappings 
established during the boot process, causing access to them to fault.  
Presumably, if the first page is there with a good signature, the entire 
ROM is mapped.  I think Jeremy added this for Xen, and it's harmless on 
native hardware.

Zach

Re: [PATCH] romsignature/checksum cleanup

[Rene Herman]

Zachary Amsden wrote:

>> Rusty Russell wrote:
>> 
>>> Rene Herman wrote:
>>> 
>>> Hmm, by the way, if romsignature() needs this
>>> probe_kernel_address() thing, why doesn't romchecksum()?
>> 
>> I assume it's all in the same page, but CC'ing Zach is easier than 
>> reading the code 8)
>> 
> 
> Some hypervisors don't emulate the traditional physical layout of the
> first 1M of memory, so those pages might never get physical mappings
> established during the boot process, causing access to them to
> fault. Presumably, if the first page is there with a good signature,
> the entire ROM is mapped.  I think Jeremy added this for Xen, and
> it's harmless on native hardware.

Jeremy? Is it okay to only check the signature word? The checksum will 
generally be done over more than 1 (hw) page... That "presumably" there 
seems a bit flaky?

Rene.

Re: [PATCH] romsignature/checksum cleanup

[Jeremy Fitzhardinge]

Rene Herman wrote:
> Jeremy? Is it okay to only check the signature word? The checksum will
> generally be done over more than 1 (hw) page... That "presumably"
> there seems a bit flaky? 

Well, in the Xen case, where the pages are simply not mapped, then the
signature simply won't exist.  In other cases, I guess its possible the
signature might exist but the rest of the ROM doesn't, but that won't
happen on normal hardware.

    J

Re: [PATCH] romsignature/checksum cleanup

[Rene Herman]

[-- Attachment #1: Type: text/plain, Size: 874 bytes --]

Jeremy Fitzhardinge wrote:

> Well, in the Xen case, where the pages are simply not mapped, then
> the signature simply won't exist.  In other cases, I guess its
> possible the signature might exist but the rest of the ROM doesn't,
> but that won't happen on normal hardware.

In your opinion, is the attached (versus 2.6.20-rc3) better? This uses 
probe_kernel_address() for all accesses. Or rather, an expanded version 
thereof. The set_fs() and pagefault_{disable,enable} calls are only done 
once in probe_roms().

Accessing the length byte at rom[2] with __get_user() is overkill after 
just checking the signature at 0 and 1 but direcly accessing only that 
makes for inconsistent code IMO. It's only a .fixup entry...

I can't say I'm all that sure that that pagefault_disable() call is 
still applicable now that it got expanded into the probe_roms() stage?

Rene.


[-- Attachment #2: probe_kernel_address.diff --]
[-- Type: text/plain, Size: 2542 bytes --]

commit f153a588097c08cefdb799f22123192a9975d273
Author: Rene Herman <rene.herman@gmail.com>
Date:   Sat Jan 6 04:09:32 2007 +0100

    Use __get_user() for ISA ROM accesses.
    
    In virtualized environments, the ISA ROMs may not be mapped so be careful
    about touching them.
    
    Signed-off-by: Rene Herman <rene.herman@gmail.com>

diff --git a/arch/i386/kernel/e820.c b/arch/i386/kernel/e820.c
index f391abc..8b54f65 100644
--- a/arch/i386/kernel/e820.c
+++ b/arch/i386/kernel/e820.c
@@ -156,29 +156,34 @@ static struct resource standard_io_resou
 	.flags	= IORESOURCE_BUSY | IORESOURCE_IO
 } };
 
-static int romsignature(const unsigned char *x)
+#define ROM_SIG 0xaa55
+
+static int __init romsignature(const unsigned char *rom)
 {
 	unsigned short sig;
-	int ret = 0;
-	if (probe_kernel_address((const unsigned short *)x, sig) == 0)
-		ret = (sig == 0xaa55);
-	return ret;
+	
+	return !__get_user(sig, (const unsigned short *)rom) && sig == ROM_SIG;
 }
 
-static int __init romchecksum(unsigned char *rom, unsigned long length)
+static int __init romchecksum(const unsigned char *rom, unsigned long length)
 {
-	unsigned char *p, sum = 0;
+	unsigned char sum, c;
 
-	for (p = rom; p < rom + length; p++)
-		sum += *p;
-	return sum == 0;
+	for (sum = 0; length && !__get_user(c, rom); rom++, length--)
+		sum += c;
+	return !length && !sum;
 }
 
 static void __init probe_roms(void)
 {
+	const unsigned char *rom;
 	unsigned long start, length, upper;
-	unsigned char *rom;
-	int	      i;
+	unsigned char c;
+	int i;
+	mm_segment_t old_fs = get_fs();
+
+	set_fs(KERNEL_DS);
+	pagefault_disable();
 
 	/* video rom */
 	upper = adapter_rom_resources[0].start;
@@ -189,8 +194,11 @@ static void __init probe_roms(void)
 
 		video_rom_resource.start = start;
 
+		if (__get_user(c, rom + 2))
+			continue;
+
 		/* 0 < length <= 0x7f * 512, historically */
-		length = rom[2] * 512;
+		length = c * 512;
 
 		/* if checksum okay, trust length byte */
 		if (length && romchecksum(rom, length))
@@ -224,8 +232,11 @@ static void __init probe_roms(void)
 		if (!romsignature(rom))
 			continue;
 
+		if (__get_user(c, rom + 2))
+			continue;
+
 		/* 0 < length <= 0x7f * 512, historically */
-		length = rom[2] * 512;
+		length = c * 512;
 
 		/* but accept any length that fits if checksum okay */
 		if (!length || start + length > upper || !romchecksum(rom, length))
@@ -237,6 +248,9 @@ static void __init probe_roms(void)
 
 		start = adapter_rom_resources[i++].end & ~2047UL;
 	}
+
+	pagefault_enable();
+	set_fs(old_fs);
 }
 
 /*

Re: [PATCH] romsignature/checksum cleanup

[Jeremy Fitzhardinge]

Rene Herman wrote:
> In your opinion, is the attached (versus 2.6.20-rc3) better? This uses
> probe_kernel_address() for all accesses. Or rather, an expanded
> version thereof. The set_fs() and pagefault_{disable,enable} calls are
> only done once in probe_roms().
>
> Accessing the length byte at rom[2] with __get_user() is overkill
> after just checking the signature at 0 and 1 but direcly accessing
> only that makes for inconsistent code IMO. It's only a .fixup entry...
>
> I can't say I'm all that sure that that pagefault_disable() call is
> still applicable now that it got expanded into the probe_roms() stage? 

I don't think this is worthwhile.  Its hardly a performance-critical
piece of code, and I think its better to use the straightforward
interface rather than complicating it for some nominal extra efficiency.

    J

Re: [PATCH] romsignature/checksum cleanup

[Rene Herman]

On 01/07/2007 09:59 AM, Jeremy Fitzhardinge wrote:

> Rene Herman wrote:

>> In your opinion, is the attached (versus 2.6.20-rc3) better? This
>> uses probe_kernel_address() for all accesses. Or rather, an
>> expanded version thereof. The set_fs() and
>> pagefault_{disable,enable} calls are only done once in
>> probe_roms().
> 
> I don't think this is worthwhile.  Its hardly a performance-critical 
> piece of code, and I think its better to use the straightforward 
> interface rather than complicating it for some nominal extra
> efficiency.

How is it for efficiency? I thought it was for correctness. romsignature 
is using probe_kernel_adress() while all other accesses to the ROMs 
there aren't.

If nothing else, anyone reading that code is likely to ask himself the 
very same question -- why the one, and not the others.

Rene.

Re: [PATCH] romsignature/checksum cleanup

[Jeremy Fitzhardinge]

Rene Herman wrote:
> How is it for efficiency? I thought it was for correctness.
> romsignature is using probe_kernel_adress() while all other accesses
> to the ROMs there aren't.
>
> If nothing else, anyone reading that code is likely to ask himself the
> very same question -- why the one, and not the others.

Well, I was wondering about all the uses of __get_user; why not
probe_kernel_address() everywhere?

I think its reasonable to assume that if the signature is mapped and
correct, then everything else is mapped.  That's certainly the case for
Xen, which is why I added it.  If you think this is unclear, then I
think a comment to explain this rather than code changes is the
appropriate fix.

    J

Re: [PATCH] romsignature/checksum cleanup

[Rene Herman]

[-- Attachment #1: Type: text/plain, Size: 2865 bytes --]

On 01/07/2007 11:20 AM, Jeremy Fitzhardinge wrote:

> Rene Herman wrote:

>> How is it for efficiency? I thought it was for correctness.
>> romsignature is using probe_kernel_adress() while all other accesses
>> to the ROMs there aren't.
>>
>> If nothing else, anyone reading that code is likely to ask himself the
>> very same question -- why the one, and not the others.
> 
> Well, I was wondering about all the uses of __get_user; why not
> probe_kernel_address() everywhere?

It's just a manual version of probe_kernel_adress():

#define probe_kernel_address(addr, retval)              \
         ({                                              \
                 long ret;                               \
                 mm_segment_t old_fs = get_fs();         \
                                                         \
                 set_fs(KERNEL_DS);                      \
                 pagefault_disable();                    \
                 ret = __get_user(retval, [ ... ]);      \
                 pagefault_enable();                     \
                 set_fs(old_fs);                         \
                 ret;                                    \
         })

Doing the set_fs() and pagefault_{disable,enable} calls for every single 
byte during the checksum seems rather silly. The patch as posted has the 
set_fs() and pagefault_ calls only once in probe_roms() (as said when 
posted, I'm not sure the pagefault calls are still useful now that it's 
no longer a generic function/macro, but used directly at probe_roms() time).

> I think its reasonable to assume that if the signature is mapped and 
> correct, then everything else is mapped.  That's certainly the case
> for Xen, which is why I added it.  If you think this is unclear, then
> I think a comment to explain this rather than code changes is the 
> appropriate fix.

I disagree I'm afraid. Given what __get_user compiles to (nothing more 
than a .fixup entry, basically) they're largely "free" and it makes the 
code completely obvious: "If you're touching this, do so via __get_user 
and not directly" and frees it from any assumptions, however reasonable 
or unreasonable.

Would you _mind_ if I submit it? If not, if you could comment on whether 
or not these pagefault calls are still useful, that would be great. The 
comment at probe_kernel_address() says:

  * We ensure that the __get_user() is executed in atomic context so that
  * do_page_fault() doesn't attempt to take mmap_sem. This makes
  * probe_kernel_address() suitable for use within regions where the
  * caller already holds mmap_sem, or other locks which nest inside
  * mmap_sem

This sounds like it might be nonsensical at probe_roms() time, but I'm 
not familiar with these virtualized environments -- I do not know which 
assumptions break.

Patch attached again for reference...

Rene.

[-- Attachment #2: probe_kernel_address.diff --]
[-- Type: text/plain, Size: 2542 bytes --]

commit f153a588097c08cefdb799f22123192a9975d273
Author: Rene Herman <rene.herman@gmail.com>
Date:   Sat Jan 6 04:09:32 2007 +0100

    Use __get_user() for ISA ROM accesses.
    
    In virtualized environments, the ISA ROMs may not be mapped so be careful
    about touching them.
    
    Signed-off-by: Rene Herman <rene.herman@gmail.com>

diff --git a/arch/i386/kernel/e820.c b/arch/i386/kernel/e820.c
index f391abc..8b54f65 100644
--- a/arch/i386/kernel/e820.c
+++ b/arch/i386/kernel/e820.c
@@ -156,29 +156,34 @@ static struct resource standard_io_resou
 	.flags	= IORESOURCE_BUSY | IORESOURCE_IO
 } };
 
-static int romsignature(const unsigned char *x)
+#define ROM_SIG 0xaa55
+
+static int __init romsignature(const unsigned char *rom)
 {
 	unsigned short sig;
-	int ret = 0;
-	if (probe_kernel_address((const unsigned short *)x, sig) == 0)
-		ret = (sig == 0xaa55);
-	return ret;
+	
+	return !__get_user(sig, (const unsigned short *)rom) && sig == ROM_SIG;
 }
 
-static int __init romchecksum(unsigned char *rom, unsigned long length)
+static int __init romchecksum(const unsigned char *rom, unsigned long length)
 {
-	unsigned char *p, sum = 0;
+	unsigned char sum, c;
 
-	for (p = rom; p < rom + length; p++)
-		sum += *p;
-	return sum == 0;
+	for (sum = 0; length && !__get_user(c, rom); rom++, length--)
+		sum += c;
+	return !length && !sum;
 }
 
 static void __init probe_roms(void)
 {
+	const unsigned char *rom;
 	unsigned long start, length, upper;
-	unsigned char *rom;
-	int	      i;
+	unsigned char c;
+	int i;
+	mm_segment_t old_fs = get_fs();
+
+	set_fs(KERNEL_DS);
+	pagefault_disable();
 
 	/* video rom */
 	upper = adapter_rom_resources[0].start;
@@ -189,8 +194,11 @@ static void __init probe_roms(void)
 
 		video_rom_resource.start = start;
 
+		if (__get_user(c, rom + 2))
+			continue;
+
 		/* 0 < length <= 0x7f * 512, historically */
-		length = rom[2] * 512;
+		length = c * 512;
 
 		/* if checksum okay, trust length byte */
 		if (length && romchecksum(rom, length))
@@ -224,8 +232,11 @@ static void __init probe_roms(void)
 		if (!romsignature(rom))
 			continue;
 
+		if (__get_user(c, rom + 2))
+			continue;
+
 		/* 0 < length <= 0x7f * 512, historically */
-		length = rom[2] * 512;
+		length = c * 512;
 
 		/* but accept any length that fits if checksum okay */
 		if (!length || start + length > upper || !romchecksum(rom, length))
@@ -237,6 +248,9 @@ static void __init probe_roms(void)
 
 		start = adapter_rom_resources[i++].end & ~2047UL;
 	}
+
+	pagefault_enable();
+	set_fs(old_fs);
 }
 
 /*

Re: [PATCH] romsignature/checksum cleanup

[Jeremy Fitzhardinge]

Rene Herman wrote:
> Doing the set_fs() and pagefault_{disable,enable} calls for every
> single byte during the checksum seems rather silly.

Why?  It's a bit of a performance hit, but that doesn't matter here. 
probe_kernel_address() is semantically the right thing to be using;
open-coding its contents to avoid a few fairly cheap operations is a
backwards step.

> I disagree I'm afraid. Given what __get_user compiles to (nothing more
> than a .fixup entry, basically) they're largely "free" and it makes
> the code completely obvious: "If you're touching this, do so via
> __get_user and not directly" and frees it from any assumptions,
> however reasonable or unreasonable.

My point is that "__get_user" doesn't make much semantic sense here:
we're not talking about usermode pages.  We used to use it quite often
for cases where an access may or may not fault, but now we spell that
"probe_kernel_address()".

> Would you _mind_ if I submit it? If not, if you could comment on
> whether or not these pagefault calls are still useful, that would be
> great.

I don't strongly object to using probe_kernel_address() for all ROM
memory accesses if it makes you feel happier, but I think putting an
open-coded implementation in here is definitely the wrong thing to do.

    J

[PATCH] romsignature/checksum cleanup

[Rene Herman]

[-- Attachment #1: Type: text/plain, Size: 730 bytes --]

On 01/07/2007 07:07 PM, Jeremy Fitzhardinge wrote:

> Rene Herman wrote:

>> Doing the set_fs() and pagefault_{disable,enable} calls for every
>> single byte during the checksum seems rather silly.
> 
> Why?

Because it makes for dumb code. But oh well, given that it all compiles 
to basically nothing I guess I'll stop objecting.

Andrew: The attached removes the assumption that if the first page of an 
ISA ROM is mapped, it'll all be mapped. This'll also stop people reading 
this code from wondering if they're looking at a bug.

This replaces "romsignature-checksum-cleanup.patch" in current -mm.

Signed-off-by: Rene Herman <rene.herman@gmail.com>
Not-strongly-objected-to-by: Jeremy Fitzhardinge <jeremy@goop.org>

Rene.

[-- Attachment #2: probe_kernel_address.diff --]
[-- Type: text/plain, Size: 2058 bytes --]

diff --git a/arch/i386/kernel/e820.c b/arch/i386/kernel/e820.c
index f391abc..8b8741f 100644
--- a/arch/i386/kernel/e820.c
+++ b/arch/i386/kernel/e820.c
@@ -156,29 +156,31 @@ static struct resource standard_io_resou
 	.flags	= IORESOURCE_BUSY | IORESOURCE_IO
 } };
 
-static int romsignature(const unsigned char *x)
+#define ROMSIGNATURE 0xaa55
+
+static int __init romsignature(const unsigned char *rom)
 {
+	const unsigned short * const ptr = (const unsigned short *)rom;
 	unsigned short sig;
-	int ret = 0;
-	if (probe_kernel_address((const unsigned short *)x, sig) == 0)
-		ret = (sig == 0xaa55);
-	return ret;
+	
+	return probe_kernel_address(ptr, sig) == 0 && sig == ROMSIGNATURE;
 }
 
-static int __init romchecksum(unsigned char *rom, unsigned long length)
+static int __init romchecksum(const unsigned char *rom, unsigned long length)
 {
-	unsigned char *p, sum = 0;
+	unsigned char sum, c;
 
-	for (p = rom; p < rom + length; p++)
-		sum += *p;
-	return sum == 0;
+	for (sum = 0; length && probe_kernel_address(rom++, c) == 0; length--)
+		sum += c;
+	return !length && !sum;
 }
 
 static void __init probe_roms(void)
 {
+	const unsigned char *rom;
 	unsigned long start, length, upper;
-	unsigned char *rom;
-	int	      i;
+	unsigned char c;
+	int i;
 
 	/* video rom */
 	upper = adapter_rom_resources[0].start;
@@ -189,8 +191,11 @@ static void __init probe_roms(void)
 
 		video_rom_resource.start = start;
 
+		if (probe_kernel_address(rom + 2, c) != 0)
+			continue;
+
 		/* 0 < length <= 0x7f * 512, historically */
-		length = rom[2] * 512;
+		length = c * 512;
 
 		/* if checksum okay, trust length byte */
 		if (length && romchecksum(rom, length))
@@ -224,8 +229,11 @@ static void __init probe_roms(void)
 		if (!romsignature(rom))
 			continue;
 
+		if (probe_kernel_address(rom + 2, c) != 0)
+			continue;
+
 		/* 0 < length <= 0x7f * 512, historically */
-		length = rom[2] * 512;
+		length = c * 512;
 
 		/* but accept any length that fits if checksum okay */
 		if (!length || start + length > upper || !romchecksum(rom, length))