Re: [PATCH] ACPICA: cleanup method properly on error
On 07/29/2016 06:47 AM, Zheng, Lv wrote: Hi, Vegard From: linux-acpi-ow...@vger.kernel.org [mailto:linux-acpi- ow...@vger.kernel.org] On Behalf Of Vegard Nossum Subject: [PATCH] ACPICA: cleanup method properly on error If the call to acpi_ds_init_aml_walk() fails, then we have to undo the walk state push done by acpi_ds_create_walk_state(). Otherwise, the new walk state (which has just been freed) will remain on the thread's walk_state_list and be dereferenced in acpi_ps_parse_aml() when we try to get the new state. [Lv Zheng] I haven't looked into the detail. Let me first ask simple questions and present simple concerns in order to move this discussion on. You can observe this when reading e.g. /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C0F:01/status [Lv Zheng] Do you mean you have real issues related to this? If so, could provide the .config and dmesg for us? Yes, I wouldn't send a patch otherwise. I don't have an issue as in "my kernel is throwing these errors and I don't know why", but I found it with KASAN + trinity + fault injection for slab. So in order to trigger the bug you really have to run out of memory (which in theory could happen on a real running system, right? Otherwise why do we check the kmalloc() return values.) == BUG: KASAN: use-after-free in acpi_ps_parse_aml+0x264/0x692 at addr 88004f255b98 Read of size 8 by task trinity-c2/5482 = BUG kmalloc-1024 (Not tainted): kasan: bad access detected - Disabling lock debugging due to kernel taint INFO: Allocated in acpi_ds_create_walk_state+0x74/0x1ff age=572 cpu=3 pid=5482 ___slab_alloc+0x480/0x4b0 __slab_alloc.isra.57+0x56/0x80 kmem_cache_alloc+0x1af/0x1e0 acpi_ds_create_walk_state+0x74/0x1ff acpi_ds_call_control_method+0xc3/0x3ef acpi_ps_parse_aml+0x20f/0x692 acpi_ps_execute_method+0x498/0x4f6 acpi_ns_evaluate+0x662/0x858 acpi_evaluate_object+0x37d/0x6d5 acpi_evaluate_integer+0x9e/0xfb status_show+0x91/0xda dev_attr_show+0x40/0xc0 sysfs_kf_seq_show+0x23c/0x490 kernfs_seq_show+0x113/0x1a0 traverse+0x2b9/0x850 seq_read+0x7cc/0x1180 INFO: Freed in acpi_ds_delete_walk_state+0x1da/0x1e5 age=95 cpu=3 pid=5482 __slab_free+0x17a/0x250 kfree+0x128/0x140 acpi_ds_delete_walk_state+0x1da/0x1e5 acpi_ds_call_control_method+0x3dd/0x3ef acpi_ps_parse_aml+0x20f/0x692 acpi_ps_execute_method+0x498/0x4f6 acpi_ns_evaluate+0x662/0x858 acpi_evaluate_object+0x37d/0x6d5 acpi_evaluate_integer+0x9e/0xfb status_show+0x91/0xda dev_attr_show+0x40/0xc0 sysfs_kf_seq_show+0x23c/0x490 kernfs_seq_show+0x113/0x1a0 traverse+0x2b9/0x850 seq_read+0x7cc/0x1180 kernfs_fop_read+0x331/0x4d0 INFO: Slab 0xea00013c9400 objects=24 used=24 fp=0x (null) flags=0x2004080 INFO: Object 0x88004f255830 @offset=22576 fp=0x88005fec5648 Bytes b4 88004f255820: 00 00 00 00 00 00 00 00 31 9e 4b 00 01 00 00 00 1.K. Object 88004f255830: 48 56 ec 5f 00 88 ff ff 00 02 00 00 00 00 ad de HV._ Object 88004f255840: 40 58 25 4f 00 88 ff ff 40 58 25 4f 00 88 ff ff @X%O@X%O Object 88004f255850: 50 58 25 4f 00 88 ff ff 50 58 25 4f 00 88 ff ff PX%OPX%O Object 88004f255860: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 Object 88004f255870: 00 00 00 00 00 00 00 00 00 02 00 00 00 00 ad de Object 88004f255880: 80 58 25 4f 00 88 ff ff 80 58 25 4f 00 88 ff ff .X%O.X%O Object 88004f255890: 00 00 00 00 01 00 00 00 30 58 25 4f 00 88 ff ff 0X%O Object 88004f2558a0: c0 78 ab 83 ff ff ff ff 00 00 00 00 00 00 00 00 .x.. Object 88004f2558b0: fd ff ff ff 04 00 00 00 00 00 00 00 00 00 00 00 Object 88004f2558c0: 00 00 00 00 00 00 00 00 fa 0d 5c 00 00 00 00 00 ..\. Object 88004f2558d0: fa 0d 5c 00 00 00 00 00 00 00 00 00 00 00 00 00 ..\. Object 88004f2558e0: 00 00 00 00 00 00 00 00 fa 0d 5c 00 00 00 00 00 ..\. Object 88004f2558f0: e1 e5 b2 db 37 fb ff ff 01 00 00 00 70 00 00 00 7...p... Object 88004f255900: 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 Object 88004f255910: 00 00 00 00 00 00 00 00 05 00 00 00 00 00 00 00 Object 88004f255920: ae 79 93 03 00 00 00 00 00 00 00 00 00 00 00 00 .y.. Object 88004f255930: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 Object 88004f255940: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 .
RE: [PATCH] ACPICA: cleanup method properly on error
Hi, Vegard > From: linux-acpi-ow...@vger.kernel.org [mailto:linux-acpi- > ow...@vger.kernel.org] On Behalf Of Vegard Nossum > Subject: [PATCH] ACPICA: cleanup method properly on error > > If the call to acpi_ds_init_aml_walk() fails, then we have to undo the > walk state push done by acpi_ds_create_walk_state(). Otherwise, the new > walk state (which has just been freed) will remain on the thread's > walk_state_list and be dereferenced in acpi_ps_parse_aml() when we try > to get the new state. [Lv Zheng] I haven't looked into the detail. Let me first ask simple questions and present simple concerns in order to move this discussion on. > > You can observe this when reading e.g. > > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C0F:01/status [Lv Zheng] Do you mean you have real issues related to this? If so, could provide the .config and dmesg for us? > > Cc: sta...@vger.kernel.org > Signed-off-by: Vegard Nossum > --- > drivers/acpi/acpica/dsmethod.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/acpi/acpica/dsmethod.c > b/drivers/acpi/acpica/dsmethod.c > index 47c7b52..44b50a6 100644 > --- a/drivers/acpi/acpica/dsmethod.c > +++ b/drivers/acpi/acpica/dsmethod.c > @@ -596,6 +596,8 @@ cleanup: > /* On error, we must terminate the method properly */ > > acpi_ds_terminate_control_method(obj_desc, next_walk_state); > + if (thread) > + acpi_ds_pop_walk_state(thread); > acpi_ds_delete_walk_state(next_walk_state); [Lv Zheng] It seems, if acpi_ds_create_walk_state() fails, acpi_ds_delete_walk_state() will be invoked. So they are paired. Fixing this in acpi_ds_delete_walk_state() could help to fix all of them. Given the fix is useful, why don't you do this in acpi_ds_delete_walk_state()? Thanks and best regards -Lv
Re: [PATCH] ACPICA: cleanup method properly on error
On 07/26/2016 10:28 PM, Moore, Robert wrote:
/* Put the new state at the head of the walk list */
if (Thread)
{
AcpiDsPushWalkState (WalkState, Thread);
}
Is there any chance that Thread could be zero?
I'm not sure if this question was for me or not, but that code (modulo
the capitalisation differences) is the reason why I also used 'if
(thread)' in my patch.
Vegard
-Original Message-
From: Devel [mailto:devel-boun...@acpica.org] On Behalf Of Moore, Robert
Sent: Tuesday, July 26, 2016 7:40 AM
To: Vegard Nossum ; Zheng, Lv
; Wysocki, Rafael J
Cc: linux-a...@vger.kernel.org; linux-kernel@vger.kernel.org;
sta...@vger.kernel.org; de...@acpica.org
Subject: Re: [Devel] [PATCH] ACPICA: cleanup method properly on error
We'll look at this.
Thanks.
-Original Message-
From: Vegard Nossum [mailto:vegard.nos...@oracle.com]
Sent: Friday, July 22, 2016 8:35 AM
To: Moore, Robert ; Zheng, Lv
; Wysocki, Rafael J
Cc: linux-a...@vger.kernel.org; de...@acpica.org; linux-
ker...@vger.kernel.org; Vegard Nossum ;
sta...@vger.kernel.org
Subject: [PATCH] ACPICA: cleanup method properly on error
If the call to acpi_ds_init_aml_walk() fails, then we have to undo the
walk state push done by acpi_ds_create_walk_state(). Otherwise, the
new walk state (which has just been freed) will remain on the thread's
walk_state_list and be dereferenced in acpi_ps_parse_aml() when we try
to get the new state.
You can observe this when reading e.g.
/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C0F:01/status
Cc: sta...@vger.kernel.org
Signed-off-by: Vegard Nossum
---
drivers/acpi/acpica/dsmethod.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/acpi/acpica/dsmethod.c
b/drivers/acpi/acpica/dsmethod.c index 47c7b52..44b50a6 100644
--- a/drivers/acpi/acpica/dsmethod.c
+++ b/drivers/acpi/acpica/dsmethod.c
@@ -596,6 +596,8 @@ cleanup:
/* On error, we must terminate the method properly */
acpi_ds_terminate_control_method(obj_desc, next_walk_state);
+ if (thread)
+ acpi_ds_pop_walk_state(thread);
acpi_ds_delete_walk_state(next_walk_state);
return_ACPI_STATUS(status);
--
1.9.1
___
Devel mailing list
de...@acpica.org
https://lists.acpica.org/mailman/listinfo/devel
RE: [PATCH] ACPICA: cleanup method properly on error
/* Put the new state at the head of the walk list */
if (Thread)
{
AcpiDsPushWalkState (WalkState, Thread);
}
Is there any chance that Thread could be zero?
> -Original Message-
> From: Devel [mailto:devel-boun...@acpica.org] On Behalf Of Moore, Robert
> Sent: Tuesday, July 26, 2016 7:40 AM
> To: Vegard Nossum ; Zheng, Lv
> ; Wysocki, Rafael J
> Cc: linux-a...@vger.kernel.org; linux-kernel@vger.kernel.org;
> sta...@vger.kernel.org; de...@acpica.org
> Subject: Re: [Devel] [PATCH] ACPICA: cleanup method properly on error
>
> We'll look at this.
> Thanks.
>
>
> > -Original Message-
> > From: Vegard Nossum [mailto:vegard.nos...@oracle.com]
> > Sent: Friday, July 22, 2016 8:35 AM
> > To: Moore, Robert ; Zheng, Lv
> > ; Wysocki, Rafael J
> > Cc: linux-a...@vger.kernel.org; de...@acpica.org; linux-
> > ker...@vger.kernel.org; Vegard Nossum ;
> > sta...@vger.kernel.org
> > Subject: [PATCH] ACPICA: cleanup method properly on error
> >
> > If the call to acpi_ds_init_aml_walk() fails, then we have to undo the
> > walk state push done by acpi_ds_create_walk_state(). Otherwise, the
> > new walk state (which has just been freed) will remain on the thread's
> > walk_state_list and be dereferenced in acpi_ps_parse_aml() when we try
> > to get the new state.
> >
> > You can observe this when reading e.g.
> >
> > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C0F:01/status
> >
> > Cc: sta...@vger.kernel.org
> > Signed-off-by: Vegard Nossum
> > ---
> > drivers/acpi/acpica/dsmethod.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/acpi/acpica/dsmethod.c
> > b/drivers/acpi/acpica/dsmethod.c index 47c7b52..44b50a6 100644
> > --- a/drivers/acpi/acpica/dsmethod.c
> > +++ b/drivers/acpi/acpica/dsmethod.c
> > @@ -596,6 +596,8 @@ cleanup:
> > /* On error, we must terminate the method properly */
> >
> > acpi_ds_terminate_control_method(obj_desc, next_walk_state);
> > + if (thread)
> > + acpi_ds_pop_walk_state(thread);
> > acpi_ds_delete_walk_state(next_walk_state);
> >
> > return_ACPI_STATUS(status);
> > --
> > 1.9.1
>
> ___
> Devel mailing list
> de...@acpica.org
> https://lists.acpica.org/mailman/listinfo/devel
RE: [PATCH] ACPICA: cleanup method properly on error
We'll look at this. Thanks. > -Original Message- > From: Vegard Nossum [mailto:vegard.nos...@oracle.com] > Sent: Friday, July 22, 2016 8:35 AM > To: Moore, Robert ; Zheng, Lv > ; Wysocki, Rafael J > Cc: linux-a...@vger.kernel.org; de...@acpica.org; linux- > ker...@vger.kernel.org; Vegard Nossum ; > sta...@vger.kernel.org > Subject: [PATCH] ACPICA: cleanup method properly on error > > If the call to acpi_ds_init_aml_walk() fails, then we have to undo the > walk state push done by acpi_ds_create_walk_state(). Otherwise, the new > walk state (which has just been freed) will remain on the thread's > walk_state_list and be dereferenced in acpi_ps_parse_aml() when we try > to get the new state. > > You can observe this when reading e.g. > > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C0F:01/status > > Cc: sta...@vger.kernel.org > Signed-off-by: Vegard Nossum > --- > drivers/acpi/acpica/dsmethod.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/acpi/acpica/dsmethod.c > b/drivers/acpi/acpica/dsmethod.c index 47c7b52..44b50a6 100644 > --- a/drivers/acpi/acpica/dsmethod.c > +++ b/drivers/acpi/acpica/dsmethod.c > @@ -596,6 +596,8 @@ cleanup: > /* On error, we must terminate the method properly */ > > acpi_ds_terminate_control_method(obj_desc, next_walk_state); > + if (thread) > + acpi_ds_pop_walk_state(thread); > acpi_ds_delete_walk_state(next_walk_state); > > return_ACPI_STATUS(status); > -- > 1.9.1
[PATCH] ACPICA: cleanup method properly on error
If the call to acpi_ds_init_aml_walk() fails, then we have to undo the walk state push done by acpi_ds_create_walk_state(). Otherwise, the new walk state (which has just been freed) will remain on the thread's walk_state_list and be dereferenced in acpi_ps_parse_aml() when we try to get the new state. You can observe this when reading e.g. /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C0F:01/status Cc: sta...@vger.kernel.org Signed-off-by: Vegard Nossum --- drivers/acpi/acpica/dsmethod.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/acpi/acpica/dsmethod.c b/drivers/acpi/acpica/dsmethod.c index 47c7b52..44b50a6 100644 --- a/drivers/acpi/acpica/dsmethod.c +++ b/drivers/acpi/acpica/dsmethod.c @@ -596,6 +596,8 @@ cleanup: /* On error, we must terminate the method properly */ acpi_ds_terminate_control_method(obj_desc, next_walk_state); + if (thread) + acpi_ds_pop_walk_state(thread); acpi_ds_delete_walk_state(next_walk_state); return_ACPI_STATUS(status); -- 1.9.1
