RE: [PATCH] Ghost EDD devices in /sys again

RE: [PATCH] Ghost EDD devices in /sys again

[H. Peter Anvin]

I think this is closer to what we really want; after all, memory being written is the "first principles" test that applies here.

-- 
Sent from my mobile phone (pardon any lack of formatting)


-----Original Message-----
From: Andrey Borzenkov <arvidjaar@newmail.ru>
Sent: Saturday, September 06, 2008 10:46
To: H. Peter Anvin <hpa@zytor.com>
Cc: Andrew Morton <akpm@linux-foundation.org>; Rafael J. Wysocki <rjw@sisk.pl>; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; Ingo Molnar <mingo@elte.hu>
Subject: Re: [PATCH] Ghost EDD devices in /sys again

On Saturday 06 September 2008, H. Peter Anvin wrote:
> 
> Andrey Borzenkov wrote:
> > 
> > Was it really that simple?
> > 
> > Subject: [PATCH] Fix ghost devices under /sys/firmware/edd
> > From: Andrey Borzenkov <arvidjaar@mail.ru>
> > 
> > Some BIOSes do not always set CF on error before return from int13.
> > The patch adds additional check for status being zero (AH == 0).
> > 
> 
> Do you have any evidence that there aren't any BIOSes which leave AH != 
> 0 on legitimate success?  If so, you have turned a nuisance bug into 
> something that could cause install failures.
> 

Hmm ... if install depends on having valid MBR, it will break right
now as well, because it will have duplicated signatures. So at least
something like following is needed.

diff --git a/arch/x86/boot/edd.c b/arch/x86/boot/edd.c
index d93cbc6..4c20d31 100644
--- a/arch/x86/boot/edd.c
+++ b/arch/x86/boot/edd.c
@@ -58,6 +58,7 @@ static u32 read_mbr_sig(u8 devno, struct edd_info *ei, u32 *mbrsig)
        if (mbrbuf_end > (char *)(size_t)boot_params.hdr.heap_end_ptr)
                return -1;

+       memset(mbrbuf_ptr, 0, sector_size);
        if (read_mbr(devno, mbrbuf_ptr))
                return -1;

Re: [PATCH] Ghost EDD devices in /sys again

[Andrey Borzenkov]

[-- Attachment #1.1: Type: text/plain, Size: 572 bytes --]

On Sunday 07 September 2008, H. Peter Anvin wrote:
> 
> I think this is closer to what we really want; after all, memory being written is the "first principles" test that applies here.
> 

OK I have been using this for some time. I do not actually insist on
returning "failure" for zero signature; my main concern was duplicated
signature resulted from not clearing read buffer. Still I assume that
zero signature is the same as no signature; i.e. even if disk is physically
present but zeroed out there is no point in presenting mbr_signature in
sysfs either.

[-- Attachment #1.2: fix-edd-detection-v2 --]
[-- Type: text/x-diff, Size: 1168 bytes --]

Subject: [PATCH] Fix ghost entries under /sys/firmware/edd take 2
From: Andrey Borzenkov < arvidjaar@mail.ru>

Some BIOSes do not indicate error when trying to read from non-
existing device. Zero buffer before reading and check that we
actually got some data.

This was fixed in different way for edd.S in
http://marc.info/?l=linux-kernel&m=114087765422490&w=2, but lost
again when edd.S was rewritten in C.

Signed-off-by: Andrey Borzenkov < arvidjaar@mail.ru>

---

 arch/x86/boot/edd.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)


diff --git a/arch/x86/boot/edd.c b/arch/x86/boot/edd.c
index d93cbc6..0388234 100644
--- a/arch/x86/boot/edd.c
+++ b/arch/x86/boot/edd.c
@@ -58,11 +58,12 @@ static u32 read_mbr_sig(u8 devno, struct edd_info *ei, u32 *mbrsig)
 	if (mbrbuf_end > (char *)(size_t)boot_params.hdr.heap_end_ptr)
 		return -1;
 
+	memset(mbrbuf_ptr, 0, sector_size);
 	if (read_mbr(devno, mbrbuf_ptr))
 		return -1;
 
 	*mbrsig = *(u32 *)&mbrbuf_ptr[EDD_MBR_SIG_OFFSET];
-	return 0;
+	return -!*mbrsig;	/* treat zero signature as no sig */
 }
 
 static int get_edd_info(u8 devno, struct edd_info *ei)

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Ghost EDD devices in /sys again

[H. Peter Anvin]

Andrey Borzenkov wrote:
> On Sunday 07 September 2008, H. Peter Anvin wrote:
>> I think this is closer to what we really want; after all, memory being written is the "first principles" test that applies here.
>>
> 
> OK I have been using this for some time. I do not actually insist on
> returning "failure" for zero signature; my main concern was duplicated
> signature resulted from not clearing read buffer. Still I assume that
> zero signature is the same as no signature; i.e. even if disk is physically
> present but zeroed out there is no point in presenting mbr_signature in
> sysfs either.
> 

Right, I think that's a much better patch.

The really Right Thing[TM] to do is probably to pre-clear the buffer and 
then look for the 0xaa55 signature at offset 510; it is required for 
sector 0 to be a valid MBR-format partition table and hence for the MBR 
*signature* to be valid.

I'll write that up tomorrow (I'm travelling today), or you can send me a 
patch.

	-hpa

Ghost EDD devices in /sys again

[Andrey Borzenkov]

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

This is regression but old enough. Apparently I had for whatever reasons
EDD turned off till recently. This is 2.6.27-rc5 just in case.

In 2006 I fixed ghost devices due to buggy BIOS:

http://marc.info/?l=linux-kernel&m=114087765422490&w=2

Later edd.S has been rewritten in C, and apparently this patch has been
lost:

{pts/1}% ls /sys/firmware/edd
int13_dev80/  int13_dev84/  int13_dev88/  int13_dev8c/
int13_dev81/  int13_dev85/  int13_dev89/  int13_dev8d/
int13_dev82/  int13_dev86/  int13_dev8a/  int13_dev8e/
int13_dev83/  int13_dev87/  int13_dev8b/  int13_dev8f/

But I have just a single disk. This is the same system BTW.

Unfortunately I do not speak asm language of C; if somebody could provide
equivalent fix for current edd.c, I am more than ready to test it.

Thank you

-andrey

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[PATCH] Ghost EDD devices in /sys again

[Andrey Borzenkov]

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

On Friday 05 September 2008, Andrey Borzenkov wrote:
> This is regression but old enough. Apparently I had for whatever reasons
> EDD turned off till recently. This is 2.6.27-rc5 just in case.
> 
> In 2006 I fixed ghost devices due to buggy BIOS:
> 
> http://marc.info/?l=linux-kernel&m=114087765422490&w=2
> 
> Later edd.S has been rewritten in C, and apparently this patch has been
> lost:
> 
> {pts/1}% ls /sys/firmware/edd
> int13_dev80/  int13_dev84/  int13_dev88/  int13_dev8c/
> int13_dev81/  int13_dev85/  int13_dev89/  int13_dev8d/
> int13_dev82/  int13_dev86/  int13_dev8a/  int13_dev8e/
> int13_dev83/  int13_dev87/  int13_dev8b/  int13_dev8f/
> 
> But I have just a single disk. This is the same system BTW.
> 
> Unfortunately I do not speak asm language of C; if somebody could provide
> equivalent fix for current edd.c, I am more than ready to test it.

Was it really that simple?

Subject: [PATCH] Fix ghost devices under /sys/firmware/edd
From: Andrey Borzenkov <arvidjaar@mail.ru>

Some BIOSes do not always set CF on error before return from int13.
The patch adds additional check for status being zero (AH == 0).

This was fixed for edd.S in
http://marc.info/?l=linux-kernel&m=114087765422490&w=2, but lost
again when edd.S was rewritten in C.

Signed-off-by: Andrey Borzenkov <arvidjaar@mail.ru>

---

 arch/x86/boot/edd.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)


diff --git a/arch/x86/boot/edd.c b/arch/x86/boot/edd.c
index d93cbc6..bf4ae6f 100644
--- a/arch/x86/boot/edd.c
+++ b/arch/x86/boot/edd.c
@@ -32,7 +32,9 @@ static int read_mbr(u8 devno, void *buf)
 		     : "+a" (ax), "+c" (cx), "+d" (dx), "+b" (bx)
 		     : : "esi", "edi", "memory");
 
-	return -(u8)ax;		/* 0 or -1 */
+	/* Some BIOSes do not set carry flag on error but still return
+	 * error in AH. The condition below is expected to catch both */
+	return -!!ax;		/* 0 or -1 */
 }
 
 static u32 read_mbr_sig(u8 devno, struct edd_info *ei, u32 *mbrsig)

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Ghost EDD devices in /sys again

[Ingo Molnar]

* Andrey Borzenkov <arvidjaar@newmail.ru> wrote:

> On Friday 05 September 2008, Andrey Borzenkov wrote:
> > This is regression but old enough. Apparently I had for whatever reasons
> > EDD turned off till recently. This is 2.6.27-rc5 just in case.
> > 
> > In 2006 I fixed ghost devices due to buggy BIOS:
> > 
> > http://marc.info/?l=linux-kernel&m=114087765422490&w=2
> > 
> > Later edd.S has been rewritten in C, and apparently this patch has been
> > lost:
> > 
> > {pts/1}% ls /sys/firmware/edd
> > int13_dev80/  int13_dev84/  int13_dev88/  int13_dev8c/
> > int13_dev81/  int13_dev85/  int13_dev89/  int13_dev8d/
> > int13_dev82/  int13_dev86/  int13_dev8a/  int13_dev8e/
> > int13_dev83/  int13_dev87/  int13_dev8b/  int13_dev8f/
> > 
> > But I have just a single disk. This is the same system BTW.
> > 
> > Unfortunately I do not speak asm language of C; if somebody could provide
> > equivalent fix for current edd.c, I am more than ready to test it.
> 
> Was it really that simple?
> 
> Subject: [PATCH] Fix ghost devices under /sys/firmware/edd
> From: Andrey Borzenkov <arvidjaar@mail.ru>
> 
> Some BIOSes do not always set CF on error before return from int13.
> The patch adds additional check for status being zero (AH == 0).
> 
> This was fixed for edd.S in
> http://marc.info/?l=linux-kernel&m=114087765422490&w=2, but lost
> again when edd.S was rewritten in C.
> 
> Signed-off-by: Andrey Borzenkov <arvidjaar@mail.ru>
> 
> ---
> 
>  arch/x86/boot/edd.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> 
> diff --git a/arch/x86/boot/edd.c b/arch/x86/boot/edd.c
> index d93cbc6..bf4ae6f 100644
> --- a/arch/x86/boot/edd.c
> +++ b/arch/x86/boot/edd.c
> @@ -32,7 +32,9 @@ static int read_mbr(u8 devno, void *buf)
>  		     : "+a" (ax), "+c" (cx), "+d" (dx), "+b" (bx)
>  		     : : "esi", "edi", "memory");
>  
> -	return -(u8)ax;		/* 0 or -1 */
> +	/* Some BIOSes do not set carry flag on error but still return
> +	 * error in AH. The condition below is expected to catch both */
> +	return -!!ax;		/* 0 or -1 */
>  }
>  
>  static u32 read_mbr_sig(u8 devno, struct edd_info *ei, u32 *mbrsig)

looks good to me - applied it to tip/x86/urgent. Thanks Andrey!

	Ingo

Re: [PATCH] Ghost EDD devices in /sys again

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> looks good to me - applied it to tip/x86/urgent. Thanks Andrey!

ok - Peter says he'd rather like to investigate this some more and have 
the fix in v2.6.28 instead - so i moved it from tip/x86/urgent to 
tip/x86/setup.

	Ingo

Re: [PATCH] Ghost EDD devices in /sys again

[H. Peter Anvin]

Andrey Borzenkov wrote:
> 
> Was it really that simple?
> 
> Subject: [PATCH] Fix ghost devices under /sys/firmware/edd
> From: Andrey Borzenkov <arvidjaar@mail.ru>
> 
> Some BIOSes do not always set CF on error before return from int13.
> The patch adds additional check for status being zero (AH == 0).
> 

Do you have any evidence that there aren't any BIOSes which leave AH != 
0 on legitimate success?  If so, you have turned a nuisance bug into 
something that could cause install failures.

	-hpa

Re: [PATCH] Ghost EDD devices in /sys again

[Andrey Borzenkov]

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

On Saturday 06 September 2008, H. Peter Anvin wrote:
> 
> Andrey Borzenkov wrote:
> > 
> > Was it really that simple?
> > 
> > Subject: [PATCH] Fix ghost devices under /sys/firmware/edd
> > From: Andrey Borzenkov <arvidjaar@mail.ru>
> > 
> > Some BIOSes do not always set CF on error before return from int13.
> > The patch adds additional check for status being zero (AH == 0).
> > 
> 
> Do you have any evidence that there aren't any BIOSes which leave AH != 
> 0 on legitimate success?  If so, you have turned a nuisance bug into 
> something that could cause install failures.
> 

Hmm ... if install depends on having valid MBR, it will break right
now as well, because it will have duplicated signatures. So at least
something like following is needed.

diff --git a/arch/x86/boot/edd.c b/arch/x86/boot/edd.c
index d93cbc6..4c20d31 100644
--- a/arch/x86/boot/edd.c
+++ b/arch/x86/boot/edd.c
@@ -58,6 +58,7 @@ static u32 read_mbr_sig(u8 devno, struct edd_info *ei, u32 *mbrsig)
        if (mbrbuf_end > (char *)(size_t)boot_params.hdr.heap_end_ptr)
                return -1;

+       memset(mbrbuf_ptr, 0, sector_size);
        if (read_mbr(devno, mbrbuf_ptr))
                return -1;


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]