RE: [PATCH] ACPI: fix vendor resource length computation

RE: [PATCH] ACPI: fix vendor resource length computation

[Moore, Robert]

I think the original code may have been attempting to subtract off the
size of the descriptor header to obtain the length of the vendor data
bytes. However, something like a global replace may have broken it.



> -----Original Message-----
> From: Bjorn Helgaas [mailto:bjorn.helgaas@hp.com]
> Sent: Tuesday, February 14, 2006 3:13 PM
> To: Andreas Schwab
> Cc: Thomas Renninger; Moore, Robert; Luck, Tony; Brown, Len; linux-
> acpi@vger.kernel.org; linux-ia64@vger.kernel.org; Andrew Morton;
> efocht@hpce.nec.com
> Subject: [PATCH] ACPI: fix vendor resource length computation
> 
> acpi_rs_get_list_length() needs to account for all the vendor-defined
> data bytes.  Failing to include these causes buffers to be sized too
> small, which causes slab corruption when we later convert AML to
> resources and run off the end of the buffer.
> 
> I'm no expert on this code, so please scrutinize this carefully.
> 
> This causes slab corruption on machines that use ACPI vendor-defined
> resources.  All HP ia64 machines do, and I'm told that some NEC
> machines may as well.  So if the fix is correct, it would be good
> to have it in 2.6.16.
> 
> Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
> 
> Index: work-mm4/drivers/acpi/resources/rscalc.c
> ===================================================================
> --- work-mm4.orig/drivers/acpi/resources/rscalc.c	2006-02-14
> 13:32:50.000000000 -0700
> +++ work-mm4/drivers/acpi/resources/rscalc.c	2006-02-14
> 13:33:25.000000000 -0700
> @@ -391,8 +391,7 @@
>  			 * Ensure a 32-bit boundary for the structure
>  			 */
>  			extra_struct_bytes =
> -			    ACPI_ROUND_UP_to_32_bITS(resource_length) -
> -			    resource_length;
> +			    ACPI_ROUND_UP_to_32_bITS(resource_length);
>  			break;
> 
>  		case ACPI_RESOURCE_NAME_END_TAG:
> @@ -408,8 +407,7 @@
>  			 * Add vendor data and ensure a 32-bit boundary
for the
> structure
>  			 */
>  			extra_struct_bytes =
> -			    ACPI_ROUND_UP_to_32_bITS(resource_length) -
> -			    resource_length;
> +			    ACPI_ROUND_UP_to_32_bITS(resource_length);
>  			break;
> 
>  		case ACPI_RESOURCE_NAME_ADDRESS32:

Re: [PATCH] ACPI: fix vendor resource length computation

[Thomas Renninger]

Moore, Robert wrote:
> I think the original code may have been attempting to subtract off the
> size of the descriptor header to obtain the length of the vendor data
> bytes. However, something like a global replace may have broken it.
> 
Thanks, this one fixes the mem corruption.
Unaligned access messages still remain...


   Thomas

RE: [PATCH] ACPI: fix vendor resource length computation

[Moore, Robert]

I recently added a new interface to ACPICA to support this kind of
debugging. It enables/disables debug for a specific control method:

ACPI_STATUS
AcpiDebugTrace (
    char                    *Name,
    UINT32                  DebugLevel,
    UINT32                  DebugLayer,
    UINT32                  Flags);

It would be nice to see this exposed in Linux somehow.

Bob


> -----Original Message-----
> From: Thomas Renninger [mailto:trenn@suse.de]
> Sent: Thursday, February 16, 2006 12:59 AM
> To: Luck, Tony
> Cc: Moore, Robert; Bjorn Helgaas; Andreas Schwab; Brown, Len; linux-
> acpi@vger.kernel.org; linux-ia64@vger.kernel.org; Andrew Morton;
> efocht@hpce.nec.com
> Subject: Re: [PATCH] ACPI: fix vendor resource length computation
> 
> Luck, Tony wrote:
> >> I'm working on the alignment issue also, should be a fix this week.
> >
> > With Bjorn's tip on using console=uart,mmio,0x{address} I can now
> > get a complete trace from boot using acpi_dbg_level = 0xffffff ...
> > well in theory I can, I started the boot two days ago and have
> > 2.2 million lines of output so far, and it is still going!  Now
> > I'm just curious as to whether this will actually complete, but
> > if it doesn't do so in the next few days I'll have to abort the
> > boot and get back to using that system for regression testing on
> > other patches.
> >
> > Sadly, I think that using the "console=uart,..." flag may have
> > caused the kernel to skip the bit of acpi probing that caused the
> > unaligned traps, as I don't see any "unaligned" messages in the
> > output so far ... so this might be a completely futile exercise.
> >
> Better don't use the boot parameters. Try to identify at which point
> the culprit might lie and then embed the code you like to
> debug in the sources into:
> acpi_dbg_level=0xXY (whatever debug level, avoid mutex and other
>                         unnecessary stuff you don't need)
> ... bad code to debug
> 
> acpi_dbg_level=0xF
> 
> I expect the output will never end, there will pop up an acpi
interrupt
> or some polling is done by ACPI and you are trapped in endless
output...
> 
>     Thomas

RE: [PATCH] ACPI: fix vendor resource length computation

[Luck, Tony]

> I'm working on the alignment issue also, should be a fix this week.

With Bjorn's tip on using console=uart,mmio,0x{address} I can now
get a complete trace from boot using acpi_dbg_level = 0xffffff ...
well in theory I can, I started the boot two days ago and have
2.2 million lines of output so far, and it is still going!  Now
I'm just curious as to whether this will actually complete, but
if it doesn't do so in the next few days I'll have to abort the
boot and get back to using that system for regression testing on
other patches.

Sadly, I think that using the "console=uart,..." flag may have
caused the kernel to skip the bit of acpi probing that caused the
unaligned traps, as I don't see any "unaligned" messages in the
output so far ... so this might be a completely futile exercise.

-Tony

Re: [PATCH] ACPI: fix vendor resource length computation

[Thomas Renninger]

Luck, Tony wrote:
>> I'm working on the alignment issue also, should be a fix this week.
> 
> With Bjorn's tip on using console=uart,mmio,0x{address} I can now
> get a complete trace from boot using acpi_dbg_level = 0xffffff ...
> well in theory I can, I started the boot two days ago and have
> 2.2 million lines of output so far, and it is still going!  Now
> I'm just curious as to whether this will actually complete, but
> if it doesn't do so in the next few days I'll have to abort the
> boot and get back to using that system for regression testing on
> other patches.
> 
> Sadly, I think that using the "console=uart,..." flag may have
> caused the kernel to skip the bit of acpi probing that caused the
> unaligned traps, as I don't see any "unaligned" messages in the
> output so far ... so this might be a completely futile exercise.
> 
Better don't use the boot parameters. Try to identify at which point
the culprit might lie and then embed the code you like to
debug in the sources into:
acpi_dbg_level=0xXY (whatever debug level, avoid mutex and other
                        unnecessary stuff you don't need)
... bad code to debug

acpi_dbg_level=0xF

I expect the output will never end, there will pop up an acpi interrupt
or some polling is done by ACPI and you are trapped in endless output...

    Thomas

RE: [PATCH] ACPI: fix vendor resource length computation

[Moore, Robert]

I'm working on the alignment issue also, should be a fix this week.
Bob


> -----Original Message-----
> From: Thomas Renninger [mailto:trenn@suse.de]
> Sent: Wednesday, February 15, 2006 9:50 AM
> To: Moore, Robert
> Cc: Bjorn Helgaas; Andreas Schwab; Luck, Tony; Brown, Len; linux-
> acpi@vger.kernel.org; linux-ia64@vger.kernel.org; Andrew Morton;
> efocht@hpce.nec.com
> Subject: Re: [PATCH] ACPI: fix vendor resource length computation
> 
> Moore, Robert wrote:
> > I think the original code may have been attempting to subtract off
the
> > size of the descriptor header to obtain the length of the vendor
data
> > bytes. However, something like a global replace may have broken it.
> >
> Thanks, this one fixes the mem corruption.
> Unaligned access messages still remain...
> 
> 
>    Thomas

RE: [PATCH] ACPI: fix vendor resource length computation

[Moore, Robert]

I'll look at this. 

Does this fix all of the slab corruptions issues we have been hearing
about?

Bob


> -----Original Message-----
> From: Bjorn Helgaas [mailto:bjorn.helgaas@hp.com]
> Sent: Tuesday, February 14, 2006 3:13 PM
> To: Andreas Schwab
> Cc: Thomas Renninger; Moore, Robert; Luck, Tony; Brown, Len; linux-
> acpi@vger.kernel.org; linux-ia64@vger.kernel.org; Andrew Morton;
> efocht@hpce.nec.com
> Subject: [PATCH] ACPI: fix vendor resource length computation
> 
> acpi_rs_get_list_length() needs to account for all the vendor-defined
> data bytes.  Failing to include these causes buffers to be sized too
> small, which causes slab corruption when we later convert AML to
> resources and run off the end of the buffer.
> 
> I'm no expert on this code, so please scrutinize this carefully.
> 
> This causes slab corruption on machines that use ACPI vendor-defined
> resources.  All HP ia64 machines do, and I'm told that some NEC
> machines may as well.  So if the fix is correct, it would be good
> to have it in 2.6.16.
> 
> Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
> 
> Index: work-mm4/drivers/acpi/resources/rscalc.c
> ===================================================================
> --- work-mm4.orig/drivers/acpi/resources/rscalc.c	2006-02-14
> 13:32:50.000000000 -0700
> +++ work-mm4/drivers/acpi/resources/rscalc.c	2006-02-14
> 13:33:25.000000000 -0700
> @@ -391,8 +391,7 @@
>  			 * Ensure a 32-bit boundary for the structure
>  			 */
>  			extra_struct_bytes =
> -			    ACPI_ROUND_UP_to_32_bITS(resource_length) -
> -			    resource_length;
> +			    ACPI_ROUND_UP_to_32_bITS(resource_length);
>  			break;
> 
>  		case ACPI_RESOURCE_NAME_END_TAG:
> @@ -408,8 +407,7 @@
>  			 * Add vendor data and ensure a 32-bit boundary
for the
> structure
>  			 */
>  			extra_struct_bytes =
> -			    ACPI_ROUND_UP_to_32_bITS(resource_length) -
> -			    resource_length;
> +			    ACPI_ROUND_UP_to_32_bITS(resource_length);
>  			break;
> 
>  		case ACPI_RESOURCE_NAME_ADDRESS32:

Re: [PATCH] ACPI: fix vendor resource length computation

[Bjorn Helgaas]

On Tuesday 14 February 2006 16:25, Moore, Robert wrote:
> Does this fix all of the slab corruptions issues we have been hearing
> about?

It fixes the one I saw with 2.6.16-rc3 on HP rx7620.  I'm pretty sure
that's the same as the one Thomas observed in pci_acpi_scan_root() and
acpi_sba_ioc_add().

But I haven't been paying attention to any other slab corruption issues,
so I can't speculate about them.

> > -----Original Message-----
> > From: Bjorn Helgaas [mailto:bjorn.helgaas@hp.com]
> > Sent: Tuesday, February 14, 2006 3:13 PM
> > To: Andreas Schwab
> > Cc: Thomas Renninger; Moore, Robert; Luck, Tony; Brown, Len; linux-
> > acpi@vger.kernel.org; linux-ia64@vger.kernel.org; Andrew Morton;
> > efocht@hpce.nec.com
> > Subject: [PATCH] ACPI: fix vendor resource length computation
> > 
> > acpi_rs_get_list_length() needs to account for all the vendor-defined
> > data bytes.  Failing to include these causes buffers to be sized too
> > small, which causes slab corruption when we later convert AML to
> > resources and run off the end of the buffer.
> > 
> > I'm no expert on this code, so please scrutinize this carefully.
> > 
> > This causes slab corruption on machines that use ACPI vendor-defined
> > resources.  All HP ia64 machines do, and I'm told that some NEC
> > machines may as well.  So if the fix is correct, it would be good
> > to have it in 2.6.16.
> > 
> > Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
> > 
> > Index: work-mm4/drivers/acpi/resources/rscalc.c
> > ===================================================================
> > --- work-mm4.orig/drivers/acpi/resources/rscalc.c	2006-02-14
> > 13:32:50.000000000 -0700
> > +++ work-mm4/drivers/acpi/resources/rscalc.c	2006-02-14
> > 13:33:25.000000000 -0700
> > @@ -391,8 +391,7 @@
> >  			 * Ensure a 32-bit boundary for the structure
> >  			 */
> >  			extra_struct_bytes =
> > -			    ACPI_ROUND_UP_to_32_bITS(resource_length) -
> > -			    resource_length;
> > +			    ACPI_ROUND_UP_to_32_bITS(resource_length);
> >  			break;
> > 
> >  		case ACPI_RESOURCE_NAME_END_TAG:
> > @@ -408,8 +407,7 @@
> >  			 * Add vendor data and ensure a 32-bit boundary
> for the
> > structure
> >  			 */
> >  			extra_struct_bytes =
> > -			    ACPI_ROUND_UP_to_32_bITS(resource_length) -
> > -			    resource_length;
> > +			    ACPI_ROUND_UP_to_32_bITS(resource_length);
> >  			break;
> > 
> >  		case ACPI_RESOURCE_NAME_ADDRESS32:
> 

RE: some new unaligned access while booting ia64 (HP rx2620)

[Moore, Robert]

This only contains the output of stores to the ACPI "debug" object, not
the full trace output. However, "_CRS 0" may help


i.e., like this:

> exstoren-0075 [18] ex_resolve_object     : ----Entry
> exstoren-0155 [18] ex_resolve_object     : ----Exit- AE_OK


> -----Original Message-----
> From: Luck, Tony
> Sent: Friday, February 10, 2006 3:25 PM
> To: Moore, Robert; 'Thomas Renninger'; Brown, Len
> Cc: 'linux-acpi@vger.kernel.org'; 'linux-ia64@vger.kernel.org'
> Subject: RE: some new unaligned access while booting ia64 (HP rx2620)
> 
> A 16MB log_buf was enough.
> 
> Here is the initial trace up until the unaligned access messages:
> 
> Linux version 2.6.16-rc2-zx1-smp (aegl@linux-t10) (gcc version 3.4.3
> [ACPI Debug]  Integer: 0x00000000
> [ACPI Debug]  String: [0x1F] "PARS.GUID: evaluating for COMP2"
> [ACPI Debug]  Integer: 0x00000001
> [ACPI Debug]  String: [0x12] "SERIAL DEV Ptr is "
> [ACPI Debug]  Integer: 0x3FEF99A0
> kernel unaligned access to 0xe000004040e7a434, ip=0xa0000001003c81a0
> kernel unaligned access to 0xe000004040e7a46c, ip=0xa0000001003c81a0
> kernel unaligned access to 0xe000004040e7a4bc, ip=0xa0000001003c81a0
> kernel unaligned access to 0xe000004040e7a42c, ip=0xa0000001003c6ca1
> kernel unaligned access to 0xe000004040e7a434, ip=0xa0000001003c6cb1
> [ACPI Debug]  String: [0x06] "_CRS 0"
> [ACPI Debug]  String: [0x0A] "CLIB.LBA 1"
> [ACPI Debug]  Integer: 0x00000000
> [ACPI Debug]  String: [0x0B] "PARS.GPTR 2"

Re: some new unaligned access while booting ia64 (HP rx2620)

[Andreas Schwab]

Bjorn Helgaas <bjorn.helgaas@hp.com> writes:

> I looked at both of those paths, and I don't see anything wrong.  I
> tried unsuccessfully to reproduce it with an rc1-mm4 kernel on an
> rx2600. 

Please try rx7620 or rx8640.  Apparently this only happens on the NUMA
systems.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
-
To unsubscribe from this list: send the line "unsubscribe linux-ia64" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: some new unaligned access while booting ia64 (HP rx2620)

[Bjorn Helgaas]

On Monday 13 February 2006 15:57, Andreas Schwab wrote:
> Bjorn Helgaas <bjorn.helgaas@hp.com> writes:
> > I looked at both of those paths, and I don't see anything wrong.  I
> > tried unsuccessfully to reproduce it with an rc1-mm4 kernel on an
> > rx2600. 
> 
> Please try rx7620 or rx8640.  Apparently this only happens on the NUMA
> systems.

While I was reading your mail, I was booting an rx7620 for something else.
When I looked at it again, it was spewing the slab errors :-)

I don't have time to debug it completely today, but it looks like the
conversion of an AML vendor resource is overrunning the buffer.  p1
and p2 are pointers to the redzones before and after the object.
0x170fc2a5 is the correct value:

acpi_rs_create_resource_list: p1 0xe0000007fc1518f0 p2 0xe0000007fc1519f8
acpi_rs_create_resource_list: *p1 0x170fc2a5 *p2 0x170fc2a5
acpi_rs_convert_aml_to_resources: p1 0xe0000007fc1518f0 p2 0xe0000007fc1519f8
acpi_rs_convert_aml_to_resources: *p1 0x170fc2a5 *p2 0x170fc2a5
acpi_rs_convert_aml_to_resources: before converting AML resource 0x88: *p1 0x170fc2a5 *p2 0x170fc2a5
acpi_rs_convert_aml_to_resources: before converting AML resource 0x8a: *p1 0x170fc2a5 *p2 0x170fc2a5
acpi_rs_convert_aml_to_resources: before converting AML resource 0x8a: *p1 0x170fc2a5 *p2 0x170fc2a5
acpi_rs_convert_aml_to_resources: before converting AML resource 0x84: *p1 0x170fc2a5 *p2 0x170fc2a5
acpi_rs_convert_aml_to_resources: before converting AML resource 0x84: *p1 0x170fc2a5 *p2 0x170fc2a5
acpi_rs_convert_aml_to_resources: before converting AML resource 0x79: *p1 0x170fc2a5 *p2 0xf901001a00000028

AML resource type 0x84 is a vendor-defined descriptor.  This makes sense
because rx7620 and rx8620 have large vendor-defined descriptors, while
the smaller boxes like rx2600 do not.

[PATCH] ACPI: fix vendor resource length computation

[Bjorn Helgaas]

acpi_rs_get_list_length() needs to account for all the vendor-defined
data bytes.  Failing to include these causes buffers to be sized too
small, which causes slab corruption when we later convert AML to
resources and run off the end of the buffer.

I'm no expert on this code, so please scrutinize this carefully.

This causes slab corruption on machines that use ACPI vendor-defined
resources.  All HP ia64 machines do, and I'm told that some NEC
machines may as well.  So if the fix is correct, it would be good
to have it in 2.6.16.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>

Index: work-mm4/drivers/acpi/resources/rscalc.c
===================================================================
--- work-mm4.orig/drivers/acpi/resources/rscalc.c	2006-02-14 13:32:50.000000000 -0700
+++ work-mm4/drivers/acpi/resources/rscalc.c	2006-02-14 13:33:25.000000000 -0700
@@ -391,8 +391,7 @@
 			 * Ensure a 32-bit boundary for the structure
 			 */
 			extra_struct_bytes =
-			    ACPI_ROUND_UP_to_32_bITS(resource_length) -
-			    resource_length;
+			    ACPI_ROUND_UP_to_32_bITS(resource_length);
 			break;
 
 		case ACPI_RESOURCE_NAME_END_TAG:
@@ -408,8 +407,7 @@
 			 * Add vendor data and ensure a 32-bit boundary for the structure
 			 */
 			extra_struct_bytes =
-			    ACPI_ROUND_UP_to_32_bITS(resource_length) -
-			    resource_length;
+			    ACPI_ROUND_UP_to_32_bITS(resource_length);
 			break;
 
 		case ACPI_RESOURCE_NAME_ADDRESS32:

Re: [PATCH] ACPI: fix vendor resource length computation

[Bjorn Helgaas]

On Tuesday 14 February 2006 16:13, Bjorn Helgaas wrote:
> acpi_rs_get_list_length() needs to account for all the vendor-defined
> data bytes.  Failing to include these causes buffers to be sized too
> small, which causes slab corruption when we later convert AML to
> resources and run off the end of the buffer.
> 
> I'm no expert on this code, so please scrutinize this carefully.
> 
> This causes slab corruption on machines that use ACPI vendor-defined
> resources.  All HP ia64 machines do, and I'm told that some NEC
> machines may as well.  So if the fix is correct, it would be good
> to have it in 2.6.16.
> 
> Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>

I forgot to mention that this patch may be used under either the GPL
or the BSD license used for the ACPI CA.

> Index: work-mm4/drivers/acpi/resources/rscalc.c
> ===================================================================
> --- work-mm4.orig/drivers/acpi/resources/rscalc.c	2006-02-14 13:32:50.000000000 -0700
> +++ work-mm4/drivers/acpi/resources/rscalc.c	2006-02-14 13:33:25.000000000 -0700
> @@ -391,8 +391,7 @@
>  			 * Ensure a 32-bit boundary for the structure
>  			 */
>  			extra_struct_bytes =
> -			    ACPI_ROUND_UP_to_32_bITS(resource_length) -
> -			    resource_length;
> +			    ACPI_ROUND_UP_to_32_bITS(resource_length);
>  			break;
>  
>  		case ACPI_RESOURCE_NAME_END_TAG:
> @@ -408,8 +407,7 @@
>  			 * Add vendor data and ensure a 32-bit boundary for the structure
>  			 */
>  			extra_struct_bytes =
> -			    ACPI_ROUND_UP_to_32_bITS(resource_length) -
> -			    resource_length;
> +			    ACPI_ROUND_UP_to_32_bITS(resource_length);
>  			break;
>  
>  		case ACPI_RESOURCE_NAME_ADDRESS32:
> -
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>