Bug 11539

Summary: empty filename in /sys/firmware/acpi/tables
Product: ACPI Reporter: Peter Gruber (nokos)
Component: OtherAssignee: Zhang Rui (rui.zhang)
Status: CLOSED PATCH_ALREADY_AVAILABLE    
Severity: low CC: acpi-bugzilla
Priority: P1    
Hardware: All   
OS: Linux   
Kernel Version: 2.6.26 Subsystem:
Regression: --- Bisected commit-id:
Attachments: patch: acpi tables changes, including the fix for this issue
refreshed patch
patch: acpi tables changes, refreshed version
patch: acpi tables changes, refreshed version, fixed a suspend/resume issue

Description Peter Gruber 2008-09-11 02:59:49 UTC
Hardware Environment:
A system with an acpi table with an empty signature

Problem Description:
Since commit bc45b1d39a925b56796bebf8a397a0491489d85c acpi tables are allowed
to have an empty signature and /sys/firmware/acpi/tables uses the signature as filename. Applications using naive recursion through /sys loop forever. A possible solution would be:
(replacing the zero length filename with the string "NULL")


diff a/drivers/acpi/system.c b/drivers/acpi/system.c
--- a/drivers/acpi/system.c
+++ b/drivers/acpi/system.c
@@ -78,9 +78,15 @@ static ssize_t acpi_table_show(struct kobject *kobj,
            container_of(bin_attr, struct acpi_table_attr, attr);
        struct acpi_table_header *table_header = NULL;
        acpi_status status;
+       char name[ACPI_NAME_SIZE];
+
+       if (strncmp(table_attr->name, "NULL", 4))
+               memcpy(name, table_attr->name, ACPI_NAME_SIZE);
+       else
+               memcpy(name, "\0\0\0\0", 4);
 
        status =
-           acpi_get_table(table_attr->name, table_attr->instance,
+           acpi_get_table(name, table_attr->instance,
                           &table_header);
        if (ACPI_FAILURE(status))
                return -ENODEV;
@@ -95,21 +101,24 @@ static void acpi_table_attr_init(struct acpi_table_attr *table_attr,
        struct acpi_table_header *header = NULL;
        struct acpi_table_attr *attr = NULL;
 
-       memcpy(table_attr->name, table_header->signature, ACPI_NAME_SIZE);
+       if (table_header->signature[0] != '\0')
+               memcpy(table_attr->name, table_header->signature,
+                      ACPI_NAME_SIZE);
+       else
+               memcpy(table_attr->name, "NULL", 4);
 
        list_for_each_entry(attr, &acpi_table_attr_list, node) {
-               if (!memcmp(table_header->signature, attr->name,
-                           ACPI_NAME_SIZE))
+               if (!memcmp(table_attr->name, attr->name, ACPI_NAME_SIZE))
                        if (table_attr->instance < attr->instance)
                                table_attr->instance = attr->instance;
        }
        table_attr->instance++;
 
        if (table_attr->instance > 1 || (table_attr->instance == 1 &&
-                                        !acpi_get_table(table_header->
-                                                        signature, 2,
-                                                        &header)))
-               sprintf(table_attr->name + 4, "%d", table_attr->instance);
+                                        !acpi_get_table
+                                        (table_header->signature, 2, &header)))
+               sprintf(table_attr->name + ACPI_NAME_SIZE, "%d",
+                       table_attr->instance);
 
        table_attr->attr.size = 0;
        table_attr->attr.read = acpi_table_show;
Comment 1 Anonymous Emailer 2008-09-11 12:34:23 UTC
Reply-To: akpm@linux-foundation.org

On Thu, 11 Sep 2008 02:59:49 -0700 (PDT)
bugme-daemon@bugzilla.kernel.org wrote:

> http://bugzilla.kernel.org/show_bug.cgi?id=11539
> 
>            Summary: empty filename in /sys/firmware/acpi/tables
>            Product: ACPI
>            Version: 2.5
>      KernelVersion: 2.6.26
>           Platform: All
>         OS/Version: Linux
>               Tree: Mainline
>             Status: NEW
>           Severity: low
>           Priority: P1
>          Component: Other
>         AssignedTo: acpi_other@kernel-bugs.osdl.org
>         ReportedBy: nokos@gmx.net
> 
> 
> Hardware Environment:
> A system with an acpi table with an empty signature
> 
> Problem Description:
> Since commit bc45b1d39a925b56796bebf8a397a0491489d85c acpi tables are allowed
> to have an empty signature and /sys/firmware/acpi/tables uses the signature
> as
> filename. Applications using naive recursion through /sys loop forever. A
> possible solution would be:
> (replacing the zero length filename with the string "NULL")
> 
> 
> diff a/drivers/acpi/system.c b/drivers/acpi/system.c
> --- a/drivers/acpi/system.c
> +++ b/drivers/acpi/system.c

(argh)


Please don't submit patches via bugzilla, especially not copy-n-paste
ones.  It gets wordwrapped and tabs get replaced with spaces and
similar crap.  Patches should be submitted via email.

I cleaned this patch up and applied it locally.

Please send a Signed-off-by: for this patch as per
Documentation/SubmittingPatches, section 12.

This patch fixes what appears to be a regression in 2.6.26 and looks
like it is applicable to both 2.6.26.x and to 2.6.27.  Len, Bob: please
review asap - I can merge it with your ack if you'd like.



From: Peter Gruber <nokos@gmx.net>

Taken from http://bugzilla.kernel.org/show_bug.cgi?id=11539

Since commit bc45b1d39a925b56796bebf8a397a0491489d85c acpi tables are
allowed to have an empty signature and /sys/firmware/acpi/tables uses the
signature as filename.  Applications using naive recursion through /sys
loop forever.  A possible solution would be: (replacing the zero length
filename with the string "NULL")

Cc: Bob Moore <robert.moore@intel.com>
Cc: Lin Ming <ming.m.lin@intel.com>
Cc: Len Brown <len.brown@intel.com>
Cc: <stable@kernel.org>		[2.6.26.x]
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 drivers/acpi/system.c |   25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff -puN drivers/acpi/system.c~acpi-dont-create-empty-filenames-in-sys-firmware-acpi-tables drivers/acpi/system.c
--- a/drivers/acpi/system.c~acpi-dont-create-empty-filenames-in-sys-firmware-acpi-tables
+++ a/drivers/acpi/system.c
@@ -78,9 +78,15 @@ static ssize_t acpi_table_show(struct ko
 	    container_of(bin_attr, struct acpi_table_attr, attr);
 	struct acpi_table_header *table_header = NULL;
 	acpi_status status;
+	char name[ACPI_NAME_SIZE];
+
+	if (strncmp(table_attr->name, "NULL", 4))
+		memcpy(name, table_attr->name, ACPI_NAME_SIZE);
+	else
+		memcpy(name, "\0\0\0\0", 4);
 
 	status =
-	    acpi_get_table(table_attr->name, table_attr->instance,
+	    acpi_get_table(name, table_attr->instance,
 			   &table_header);
 	if (ACPI_FAILURE(status))
 		return -ENODEV;
@@ -95,21 +101,24 @@ static void acpi_table_attr_init(struct 
 	struct acpi_table_header *header = NULL;
 	struct acpi_table_attr *attr = NULL;
 
-	memcpy(table_attr->name, table_header->signature, ACPI_NAME_SIZE);
+	if (table_header->signature[0] != '\0')
+		memcpy(table_attr->name, table_header->signature,
+			ACPI_NAME_SIZE);
+	else
+		memcpy(table_attr->name, "NULL", 4);
 
 	list_for_each_entry(attr, &acpi_table_attr_list, node) {
-		if (!memcmp(table_header->signature, attr->name,
-			    ACPI_NAME_SIZE))
+		if (!memcmp(table_attr->name, attr->name, ACPI_NAME_SIZE))
 			if (table_attr->instance < attr->instance)
 				table_attr->instance = attr->instance;
 	}
 	table_attr->instance++;
 
 	if (table_attr->instance > 1 || (table_attr->instance == 1 &&
-					 !acpi_get_table(table_header->
-							 signature, 2,
-							 &header)))
-		sprintf(table_attr->name + 4, "%d", table_attr->instance);
+					!acpi_get_table
+					(table_header->signature, 2, &header)))
+		sprintf(table_attr->name + ACPI_NAME_SIZE, "%d",
+			table_attr->instance);
 
 	table_attr->attr.size = 0;
 	table_attr->attr.read = acpi_table_show;
_
Comment 2 Zhang Rui 2008-09-11 18:09:09 UTC
On Thu, 2008-09-11 at 12:33 -0700, Andrew Morton wrote:
> On Thu, 11 Sep 2008 02:59:49 -0700 (PDT)
> bugme-daemon@bugzilla.kernel.org wrote:
> 
> > http://bugzilla.kernel.org/show_bug.cgi?id=11539
> > 
> >            Summary: empty filename in /sys/firmware/acpi/tables
> >            Product: ACPI
> >            Version: 2.5
> >      KernelVersion: 2.6.26
> >           Platform: All
> >         OS/Version: Linux
> >               Tree: Mainline
> >             Status: NEW
> >           Severity: low
> >           Priority: P1
> >          Component: Other
> >         AssignedTo: acpi_other@kernel-bugs.osdl.org
> >         ReportedBy: nokos@gmx.net
> > 
> > 
> > Hardware Environment:
> > A system with an acpi table with an empty signature
> > 
> > Problem Description:
> > Since commit bc45b1d39a925b56796bebf8a397a0491489d85c acpi tables are
> allowed
> > to have an empty signature and /sys/firmware/acpi/tables uses the signature
> as
> > filename. Applications using naive recursion through /sys loop forever. A
> > possible solution would be:
> > (replacing the zero length filename with the string "NULL")
> > 

Acked-by: Zhang Rui <rui.zhang@intel.com>

thanks,
rui
 
> > diff a/drivers/acpi/system.c b/drivers/acpi/system.c
> > --- a/drivers/acpi/system.c
> > +++ b/drivers/acpi/system.c
> 
> (argh)
> 
> 
> Please don't submit patches via bugzilla, especially not copy-n-paste
> ones.  It gets wordwrapped and tabs get replaced with spaces and
> similar crap.  Patches should be submitted via email.
> 
> I cleaned this patch up and applied it locally.
> 
> Please send a Signed-off-by: for this patch as per
> Documentation/SubmittingPatches, section 12.
> 
> This patch fixes what appears to be a regression in 2.6.26 and looks
> like it is applicable to both 2.6.26.x and to 2.6.27.  Len, Bob: please
> review asap - I can merge it with your ack if you'd like.
> 
> 
> 
> From: Peter Gruber <nokos@gmx.net>
> 
> Taken from http://bugzilla.kernel.org/show_bug.cgi?id=11539
> 
> Since commit bc45b1d39a925b56796bebf8a397a0491489d85c acpi tables are
> allowed to have an empty signature and /sys/firmware/acpi/tables uses the
> signature as filename.  Applications using naive recursion through /sys
> loop forever.  A possible solution would be: (replacing the zero length
> filename with the string "NULL")
> 
> Cc: Bob Moore <robert.moore@intel.com>
> Cc: Lin Ming <ming.m.lin@intel.com>
> Cc: Len Brown <len.brown@intel.com>
> Cc: <stable@kernel.org>               [2.6.26.x]
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  drivers/acpi/system.c |   25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)
> 
> diff -puN
> drivers/acpi/system.c~acpi-dont-create-empty-filenames-in-sys-firmware-acpi-tables
> drivers/acpi/system.c
> ---
> a/drivers/acpi/system.c~acpi-dont-create-empty-filenames-in-sys-firmware-acpi-tables
> +++ a/drivers/acpi/system.c
> @@ -78,9 +78,15 @@ static ssize_t acpi_table_show(struct ko
>           container_of(bin_attr, struct acpi_table_attr, attr);
>       struct acpi_table_header *table_header = NULL;
>       acpi_status status;
> +     char name[ACPI_NAME_SIZE];
> +
> +     if (strncmp(table_attr->name, "NULL", 4))
> +             memcpy(name, table_attr->name, ACPI_NAME_SIZE);
> +     else
> +             memcpy(name, "\0\0\0\0", 4);
>  
>       status =
> -         acpi_get_table(table_attr->name, table_attr->instance,
> +         acpi_get_table(name, table_attr->instance,
>                          &table_header);
>       if (ACPI_FAILURE(status))
>               return -ENODEV;
> @@ -95,21 +101,24 @@ static void acpi_table_attr_init(struct 
>       struct acpi_table_header *header = NULL;
>       struct acpi_table_attr *attr = NULL;
>  
> -     memcpy(table_attr->name, table_header->signature, ACPI_NAME_SIZE);
> +     if (table_header->signature[0] != '\0')
> +             memcpy(table_attr->name, table_header->signature,
> +                     ACPI_NAME_SIZE);
> +     else
> +             memcpy(table_attr->name, "NULL", 4);
>  
>       list_for_each_entry(attr, &acpi_table_attr_list, node) {
> -             if (!memcmp(table_header->signature, attr->name,
> -                         ACPI_NAME_SIZE))
> +             if (!memcmp(table_attr->name, attr->name, ACPI_NAME_SIZE))
>                       if (table_attr->instance < attr->instance)
>                               table_attr->instance = attr->instance;
>       }
>       table_attr->instance++;
>  
>       if (table_attr->instance > 1 || (table_attr->instance == 1 &&
> -                                      !acpi_get_table(table_header->
> -                                                      signature, 2,
> -                                                      &header)))
> -             sprintf(table_attr->name + 4, "%d", table_attr->instance);
> +                                     !acpi_get_table
> +                                     (table_header->signature, 2, &header)))
> +             sprintf(table_attr->name + ACPI_NAME_SIZE, "%d",
> +                     table_attr->instance);
>  
>       table_attr->attr.size = 0;
>       table_attr->attr.read = acpi_table_show;
> _
> 
> --
> 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
Comment 3 Len Brown 2008-10-05 21:22:38 UTC
What happens when we encouter a 2nd table w/ a null signature,
won't we then attempt to create two sysfs files with the same name?
Comment 4 Zhang Rui 2008-10-05 22:31:04 UTC
well,
we will try to create a duplicate sysfs files with the same name in this case.
this doesn't sounds good.
I'll refresh the patch here and send it out soon.
Comment 5 Zhang Rui 2008-10-07 00:53:26 UTC
Created attachment 18185 [details]
patch: acpi tables changes, including the fix for this issue

hi, peter,

I made several changes of ACPI table management, including the one for this bug.
could you please give it a try?
note it's on top of len's acpi test branch. :)
how to get the acpi test branch can be found at:
http://www.lesswatts.org/projects/acpi/git.php
Comment 6 Zhang Rui 2008-10-07 01:38:38 UTC
Created attachment 18186 [details]
refreshed patch
Comment 7 Peter Gruber 2008-10-07 07:20:37 UTC
(In reply to comment #3)
> What happens when we encouter a 2nd table w/ a null signature,
> won't we then attempt to create two sysfs files with the same name?

this is taken care of in the standard way (adding an index, NULL1, NULL2, ..) like in the case when there are more tahn one SSDT table (SSDT1, SSDT2, ...)

I can only think of one (somewhat) unhandled case: when there are more than one table signature starting with \0 but continuing differently (say '\0\0\0\0' and '\0FOO'). In this case multiple files with the same name are probably created. 
Comment 8 Peter Gruber 2008-10-09 12:04:22 UTC
(In reply to comment #5)
> hi, peter,
> 
> I made several changes of ACPI table management, including the one for this
> bug.
> could you please give it a try?

with your patch I get the following kernel panic at boot:

BUG: unable to handle kernel paging request at ffffc20000039018
IP: [<ffffffff808db96d>] acpi_initialize_inactive_tables+0x1e2/0x280
PGD 12f804067 PUD 12f805067 PMD 12f806067 PTE 0
Oops: 0000 [1] SMP
CPU 0
Modules linked in:
Pid: 1, comm: swapper Tainted: G        W 2.6.27-rc8 #2
RIP: 0010:[<ffffffff808db96d>]  [<ffffffff808db96d>] acpi_initialize_inactive_tables+0x1e2/0x280
RSP: 0018:ffff88012f85be10  EFLAGS: 00010282
RAX: ffffc20000038f8c RBX: 0000000000000000 RCX: ffff88012f85bce0
RDX: ffffc20000038000 RSI: 0000000000000000 RDI: ffffc20000038f8c
RBP: ffff88012f85be50 R08: 0000000000000000 R09: 00000000000000d0
R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
R13: ffffc2000003875c R14: ffffc2000003870c R15: 0000000000000050
FS:  0000000000000000(0000) GS: fffffff808a3340(0000) knlGS:0000000000000000
CS:  0010 DS: 0018 ES: 0018 CR0: 000000008005003b
CR2: ffffc20000038018 CR3: 0000000000201000 CR4: 00000000000006e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process swapper (pid: 1, threadinfo ffff88012f85a000, task ffff88012f860000)
Stack: 00000000000092fe0 0000000000000000 0000000000000000 ffffffff809073e8
 0000000000000000 0000000000000000 0000000000092fe0 0000000000000000
 ffff88012f85bea0 ffffffff808dcf32 ffffffff808dcc45 0000000000000000
Call Trace:
 [<ffffffff808dcf32>] acpi_system_init+0xa7/0x208
 [<ffffffff808dcc45>] ? acpi_power_init+0x0/0x77
 [<ffffffff808dce8b>] ? acpi_system_init+0x0/0x208
 [<ffffffff8020903c>] _stext+0x3c/0x170
 [<ffffffff80284103>] ? register_irq_proc+0xd3/0xf0
 [<ffffffff80300000>] ? vfs_set_dqblk+0x230/0x360
 [<ffffffff808b96b3>] kernel_init+0x141/0x197
 [<ffffffff802413f7>] ? schedule_tail+0x27/0x70
 [<ffffffff8020d9e9>] child_rip+0xa/0x11
 [<ffffffff808b9572>] ? kernel_init+0x0/0x197
 [<ffffffff8020d9df>] ? child_rip+0x0/0x11

Code: 0c 00 48 01 da 8b 42 14 3b 05 8a 74 ee ff 0f 85 01 00 00 00 48 8b 3a 8b 72 10 e8 fb cc da ff 48 89 c7 48 85 c0 0f 84 85 00 00 00 <48> 8b 97 8c 00 00 00 48 8b 0d c5 62 0c 00 8b 05 c7 62 0c 00 48
RIP [<fffffffff808db96d>] acpi_initialize_inactive_tables+0x1e2/0x280
 RSP <ffff88012f85be10>
CR2: ffffc20000039010
---[ end trace 4eaa2a86a8e2da22 ]---
Comment 9 Zhang Rui 2008-10-09 18:01:24 UTC
Created attachment 18235 [details]
patch: acpi tables changes, refreshed version

sorry, would you please try this one please?
Comment 10 Zhang Rui 2008-10-09 23:37:11 UTC
Created attachment 18237 [details]
patch: acpi tables changes, refreshed version, fixed a suspend/resume issue
Comment 11 Peter Gruber 2008-10-12 14:47:33 UTC
(In reply to comment #10)
> Created an attachment (id=18237) [details]
> patch: acpi tables changes, refreshed version, fixed a suspend/resume issue

ok ... this one works,
what do the _inactive, _dynamic mean?
Comment 12 Zhang Rui 2008-10-12 17:48:39 UTC
_inactive tables are the tables that exported by the inactive root table.
i.e. if Linux is using XSDT on this laptop, tables from RSDT are inactive tables.
_dynamic tables are the ACPI tables that loaded at runtime, which is different from the tables that are mapped at the boot time(tables from RSDT/XSDT).
Comment 13 Len Brown 2008-10-27 21:02:33 UTC
applied akpm's update to peter's patch to acpi tree.
I plan to send it to .26.stable and .27.stable
as soon as it hits upstream.
Comment 14 Len Brown 2008-11-12 21:18:19 UTC
shipped in 2.6.28-rc4-git3

commit 4feba70a2c1a1a0c96909f657f48b2e11e682370
Author: Peter Gruber <nokos@gmx.net>
Date:   Mon Oct 27 23:59:36 2008 -0400

    ACPI: avoid empty file name in sysfs

e-mailed to 2.6.26-stable, 2.6.27-stable.

closed.