Skip to contentSkip to conversation

romsignature/checksum cleanup

PATCH In review Rene Herman 4 participants 14 messages Dec 24, 2006 – Jan 8, 2007

lore
Share & tools
lore

Rene Herman

author
#0 lore raw

PATCH 1/1 romsignature/checksum cleanup

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 <[email protected]>
arch/i386/kernel/e820.c +9 -8
index f391abc..2565fac 100644
@@ -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;
 }
author
#1 lore raw
Quoted 1 line · Rene Herman

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.

#2 lore raw
Quoted 7 lines · Rene Herman
Quoted 2 lines · Rene Herman

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.

author
#3 lore raw
Quoted 6 lines · Rusty Russell
Quoted 1 line · Rene Herman

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.

#4 lore raw
In reply to Rusty Russell
Quoted 5 lines · Rusty Russell
Quoted 3 lines · Rene Herman

Rene Herman wrote:

Quoted 8 lines
Quoted 4 lines
Quoted 2 lines

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

author
#5 lore raw
Quoted 10 lines · Zachary Amsden
Quoted 9 lines
Quoted 4 lines · Rusty Russell

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)

Quoted 6 lines

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.

#6 lore raw
Quoted 3 lines · Rene Herman

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

author
#7 lore raw
Quoted 4 lines · Jeremy Fitzhardinge

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.

commit f153a588097c08cefdb799f22123192a9975d273

Author: Rene Herman <[email protected]>
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 <[email protected]>
arch/i386/kernel/e820.c +28 -14
index f391abc..8b54f65 100644
@@ -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);
 }
 
 /*
#8 lore raw
Quoted 11 lines · Rene Herman

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

author
#9 lore raw
Quoted 10 lines · Jeremy Fitzhardinge
Quoted 5 lines · Rene Herman

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.

#10 lore raw
Quoted 6 lines · Rene Herman

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

author
#11 lore raw
Quoted 11 lines · Jeremy Fitzhardinge
Quoted 6 lines · Rene Herman

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).

Quoted 3 lines

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.

commit f153a588097c08cefdb799f22123192a9975d273

Author: Rene Herman <[email protected]>
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 <[email protected]>
arch/i386/kernel/e820.c +28 -14
index f391abc..8b54f65 100644
@@ -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);
 }
 
 /*
#12 lore raw
Quoted 2 lines · Rene Herman

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.

Quoted 5 lines

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()".

Quoted 3 lines

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

author
#13 lore raw

PATCH 1/1 romsignature/checksum cleanup

Quoted 6 lines · Jeremy Fitzhardinge
Quoted 2 lines · Rene Herman

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 <[email protected]>
Not-strongly-objected-to-by Jeremy Fitzhardinge <[email protected]>

Rene.

arch/i386/kernel/e820.c +22 -14
index f391abc..8b8741f 100644
@@ -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))