[BUG] acpica: ltp_acpi test case causes kernel crash at acpi_ns_walk_namespace

[BUG] acpica: ltp_acpi test case causes kernel crash at acpi_ns_walk_namespace

[Vadim Lomovtsev]

Hi guys,

While running LTP tests I've faced kernel crash caused by ltp_acpi test case.
I have ACPI support enabled in kernel but kernel is boot with FDT having ACPI
disabled. The ltp_acpi test case application is built along with ltp_acpi_cmds
module to run ACPI tests.

So my question is - should we update acpica implementation at kernel side by
adding 'acpi_disabled' variable checking to the 'acpi_get_devices' function (see
patch next to this email, maybe not a good approach) or this should be fixed at LTP
side so the ltp_acpi_cmds should be updated in order to check if acpi is enabled
before running tests ?

Please check for kernel trace below:

[  928.815454] Modules linked in: ltp_acpi_cmds(OE) ip6t_rpfilter ipt_REJECT
[ .. ]
2 05:12:36 ...
 kernel:Internal error: Oops: 96000007 [#1] SMP
 [  928.903937]  ipmi_devintf mdio_thunder thunderx_edac mdio_cavium ipmi_msghandler dm_mirror dm_region_hash dm_log dm_mod dax
 [  928.917581] CPU: 32 PID: 27679 Comm: ltp_acpi Tainted: G           OE    4.15.0-rc3+ #1
 [  928.928154] Hardware name: Gigabyte MT60-SC0-00_24SATA_02/MT60-SC0-00_24SATA_02, BIOS ThunderX-Firmware-Release-1.22.18-Build_02 Oct 17 2017
 [  928.946235] pstate: 60000005 (nZCv daif -PAN -UAO)
 [  928.953764] pc : acpi_ns_walk_namespace+0x64/0x1cc
 [  928.961296] lr : acpi_get_devices+0x74/0x90
 [  928.968159] sp : fffffc004d50fba0
 [  928.974170] x29: fffffc004d50fba0 x28: 0000000000000001
 [  928.982234] x27: fffffc00088a1000 x26: 0000000000000001
 [  928.990308] x25: 0000000000000000 x24: fffffc004d50fc58
 [  928.998413] x23: 0000000000000000 x22: 0000000000000001
 [  929.006536] x21: 0000000000000000 x20: fffffc00084fb2c4
 [  929.014680] x19: 0000000000000000 x18: 000003ffe27a5d70
 [  929.022848] x17: 0000000000000000 x16: 0000000000000000
 [  929.031030] x15: 0000000000005788 x14: 0e200e200e200e20
 [  929.039231] x13: 0000000000000008 x12: 0000000000000001
 [  929.047449] x11: 00000000ffffffff x10: ffffff00044e39a0
 [  929.055695] x9 : 00000000000009eb x8 : 0000000000000001
 [  929.063972] x7 : 0000000000000000 x6 : fffffc004d50fc58
 [  929.072272] x5 : 0000000000000000 x4 : fffffc00084fb2c4
 [  929.080583] x3 : 0000000000000001 x2 : 00000000ffffffff
 [  929.088915] x1 : ffffffffffffffff x0 : fffffc00095fa000
 [  929.097284] Process ltp_acpi (pid: 27679, stack limit = 0x0000000060c25f0d)
 [  929.107389] Call trace:
 [  929.112989]  acpi_ns_walk_namespace+0x64/0x1cc
 [  929.120648]  acpi_get_devices+0x74/0x90
 [  929.127639]  sys_tcase+0x158/0x618 [ltp_acpi_cmds]
 [  929.135610]  dev_attr_store+0x40/0x54
 [  929.142396]  sysfs_kf_write+0x5c/0x6c
 [  929.149123]  kernfs_fop_write+0xc8/0x1d0
 [  929.156122]  __vfs_write+0x48/0x150
 [  929.162658]  vfs_write+0xa8/0x1a0
 [  929.168994]  SyS_write+0x54/0xb0
 [  929.175234]  __sys_trace_return+0x0/0x4
 [  929.182099] Code: f9433c13 5280003c 52800017 0a1c007a (f9400e76)
 [  929.191352] ---[ end trace ae3e06a2af30d07c ]---
 [  929.199061] Kernel panic - not syncing: Fatal exception
 [  929.207409] SMP: stopping secondary CPUs
 [  929.214374] Kernel Offset: disabled
 [  929.220781] CPU features: 0x100108
 [  929.227004] Memory Limit: none
 [  929.232805] ---[ end Kernel panic - not syncing: Fatal exception

WBR,
Vadim

[PATCH] acpi: acpica: add acpi status check prior walking through namespace

[Vadim Lomovtsev]

From: Vadim Lomovtsev <vadim.lomovtsev@cavium.com>

While having kernel built with ACPI support enabled and booted over FDT,
the ltp_acpi test from LTP suite causes kernel crash while calling
acpi_ns_walk_namespace(). The acpi_get_devices is high level wrapper for
it, so we need to protect kernel from crashes by adding acpi status
check before walking through namespace which is not loaded because of
acpi is disabled.

Signed-off-by: Vadim Lomovtsev <vadim.lomovtsev@cavium.com>
---
 drivers/acpi/acpica/nsxfeval.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/acpi/acpica/nsxfeval.c b/drivers/acpi/acpica/nsxfeval.c
index 783f4c8..e0eb9ae 100644
--- a/drivers/acpi/acpica/nsxfeval.c
+++ b/drivers/acpi/acpica/nsxfeval.c
@@ -52,6 +52,8 @@
 #define _COMPONENT          ACPI_NAMESPACE
 ACPI_MODULE_NAME("nsxfeval")
 
+extern int acpi_disabled;
+
 /* Local prototypes */
 static void acpi_ns_resolve_references(struct acpi_evaluate_info *info);
 
@@ -812,6 +814,11 @@ static void acpi_ns_resolve_references(struct acpi_evaluate_info *info)
 
 	ACPI_FUNCTION_TRACE(acpi_get_devices);
 
+	/* check if ACPI disabled to prevent kernel crash later */
+	if (acpi_disabled) {
+		return_ACPI_STATUS(AE_NOT_CONFIGURED);
+	}
+
 	/* Parameter validation */
 
 	if (!user_function) {
-- 
1.8.3.1

RE: [PATCH] acpi: acpica: add acpi status check prior walking through namespace

[Moore, Robert]

Another way to look at this is that the kernel should not be calling ACPI interfaces if ACPI is disabled.

> -----Original Message-----
> From: Vadim Lomovtsev [mailto:Vadim.Lomovtsev@caviumnetworks.com]
> Sent: Tuesday, December 12, 2017 7:59 AM
> To: Moore, Robert <robert.moore@intel.com>; Zheng, Lv
> <lv.zheng@intel.com>; Wysocki, Rafael J <rafael.j.wysocki@intel.com>;
> lenb@kernel.org; linux-acpi@vger.kernel.org; devel@acpica.org; linux-
> kernel@vger.kernel.org
> Cc: vadim.lomovtsev@cavium.com
> Subject: [PATCH] acpi: acpica: add acpi status check prior walking
> through namespace
> 
> From: Vadim Lomovtsev <vadim.lomovtsev@cavium.com>
> 
> While having kernel built with ACPI support enabled and booted over FDT,
> the ltp_acpi test from LTP suite causes kernel crash while calling
> acpi_ns_walk_namespace(). The acpi_get_devices is high level wrapper for
> it, so we need to protect kernel from crashes by adding acpi status
> check before walking through namespace which is not loaded because of
> acpi is disabled.
> 
> Signed-off-by: Vadim Lomovtsev <vadim.lomovtsev@cavium.com>
> ---
>  drivers/acpi/acpica/nsxfeval.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/acpi/acpica/nsxfeval.c
> b/drivers/acpi/acpica/nsxfeval.c index 783f4c8..e0eb9ae 100644
> --- a/drivers/acpi/acpica/nsxfeval.c
> +++ b/drivers/acpi/acpica/nsxfeval.c
> @@ -52,6 +52,8 @@
>  #define _COMPONENT          ACPI_NAMESPACE
>  ACPI_MODULE_NAME("nsxfeval")
> 
> +extern int acpi_disabled;
> +
>  /* Local prototypes */
>  static void acpi_ns_resolve_references(struct acpi_evaluate_info
> *info);
> 
> @@ -812,6 +814,11 @@ static void acpi_ns_resolve_references(struct
> acpi_evaluate_info *info)
> 
>  	ACPI_FUNCTION_TRACE(acpi_get_devices);
> 
> +	/* check if ACPI disabled to prevent kernel crash later */
> +	if (acpi_disabled) {
> +		return_ACPI_STATUS(AE_NOT_CONFIGURED);
> +	}
> +
>  	/* Parameter validation */
> 
>  	if (!user_function) {
> --
> 1.8.3.1

Re: [PATCH] acpi: acpica: add acpi status check prior walking through namespace

[Vadim Lomovtsev]

On Tue, Dec 12, 2017 at 09:52:21PM +0000, Moore, Robert wrote:
> Another way to look at this is that the kernel should not be calling ACPI interfaces if ACPI is disabled.

Yes, I agree. So in this case the ltp_acpi test case has to be updated with such checking
before calling ACPI interfaces. However, it seems that such calls was put there intentionally,
without ACPI state check, as part of kernel testing strategy.

WBR,
Vadim

> 
> > -----Original Message-----
> > From: Vadim Lomovtsev [mailto:Vadim.Lomovtsev@caviumnetworks.com]
> > Sent: Tuesday, December 12, 2017 7:59 AM
> > To: Moore, Robert <robert.moore@intel.com>; Zheng, Lv
> > <lv.zheng@intel.com>; Wysocki, Rafael J <rafael.j.wysocki@intel.com>;
> > lenb@kernel.org; linux-acpi@vger.kernel.org; devel@acpica.org; linux-
> > kernel@vger.kernel.org
> > Cc: vadim.lomovtsev@cavium.com
> > Subject: [PATCH] acpi: acpica: add acpi status check prior walking
> > through namespace
> > 
> > From: Vadim Lomovtsev <vadim.lomovtsev@cavium.com>
> > 
> > While having kernel built with ACPI support enabled and booted over FDT,
> > the ltp_acpi test from LTP suite causes kernel crash while calling
> > acpi_ns_walk_namespace(). The acpi_get_devices is high level wrapper for
> > it, so we need to protect kernel from crashes by adding acpi status
> > check before walking through namespace which is not loaded because of
> > acpi is disabled.
> > 
> > Signed-off-by: Vadim Lomovtsev <vadim.lomovtsev@cavium.com>
> > ---
> >  drivers/acpi/acpica/nsxfeval.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/acpi/acpica/nsxfeval.c
> > b/drivers/acpi/acpica/nsxfeval.c index 783f4c8..e0eb9ae 100644
> > --- a/drivers/acpi/acpica/nsxfeval.c
> > +++ b/drivers/acpi/acpica/nsxfeval.c
> > @@ -52,6 +52,8 @@
> >  #define _COMPONENT          ACPI_NAMESPACE
> >  ACPI_MODULE_NAME("nsxfeval")
> > 
> > +extern int acpi_disabled;
> > +
> >  /* Local prototypes */
> >  static void acpi_ns_resolve_references(struct acpi_evaluate_info
> > *info);
> > 
> > @@ -812,6 +814,11 @@ static void acpi_ns_resolve_references(struct
> > acpi_evaluate_info *info)
> > 
> >  	ACPI_FUNCTION_TRACE(acpi_get_devices);
> > 
> > +	/* check if ACPI disabled to prevent kernel crash later */
> > +	if (acpi_disabled) {
> > +		return_ACPI_STATUS(AE_NOT_CONFIGURED);
> > +	}
> > +
> >  	/* Parameter validation */
> > 
> >  	if (!user_function) {
> > --
> > 1.8.3.1
> 

Re: [PATCH] acpi: acpica: add acpi status check prior walking through namespace

[Rafael J. Wysocki]

On Wed, Dec 13, 2017 at 3:52 PM, Vadim Lomovtsev
<Vadim.Lomovtsev@caviumnetworks.com> wrote:
> On Tue, Dec 12, 2017 at 09:52:21PM +0000, Moore, Robert wrote:
>> Another way to look at this is that the kernel should not be calling ACPI interfaces if ACPI is disabled.
>
> Yes, I agree. So in this case the ltp_acpi test case has to be updated with such checking
> before calling ACPI interfaces. However, it seems that such calls was put there intentionally,
> without ACPI state check, as part of kernel testing strategy.

Not really.

Thanks,
Rafael

Re: [PATCH] acpi: acpica: add acpi status check prior walking through namespace

[Vadim Lomovtsev]

On Wed, Dec 13, 2017 at 05:48:49PM +0100, Rafael J. Wysocki wrote:
> On Wed, Dec 13, 2017 at 3:52 PM, Vadim Lomovtsev
> <Vadim.Lomovtsev@caviumnetworks.com> wrote:
> > On Tue, Dec 12, 2017 at 09:52:21PM +0000, Moore, Robert wrote:
> >> Another way to look at this is that the kernel should not be calling ACPI interfaces if ACPI is disabled.
> >
> > Yes, I agree. So in this case the ltp_acpi test case has to be updated with such checking
> > before calling ACPI interfaces. However, it seems that such calls was put there intentionally,
> > without ACPI state check, as part of kernel testing strategy.
> 
> Not really.

Ok, see your point.
Thank you for your time.

WBR,
Vadim

> 
> Thanks,
> Rafael

Re: [BUG] acpica: ltp_acpi test case causes kernel crash at acpi_ns_walk_namespace

[Rafael J. Wysocki]

On Tuesday, December 12, 2017 4:59:19 PM CET Vadim Lomovtsev wrote:
> Hi guys,
> 
> While running LTP tests I've faced kernel crash caused by ltp_acpi test case.
> I have ACPI support enabled in kernel but kernel is boot with FDT having ACPI
> disabled. The ltp_acpi test case application is built along with ltp_acpi_cmds
> module to run ACPI tests.
> 
> So my question is - should we update acpica implementation at kernel side by
> adding 'acpi_disabled' variable checking to the 'acpi_get_devices' function (see
> patch next to this email, maybe not a good approach) or this should be fixed at LTP
> side so the ltp_acpi_cmds should be updated in order to check if acpi is enabled
> before running tests ?

There should be a check preventing acpi_get_devices() from being called in the
acpi_disabled case.

acpi_disabled is Linux-specific and the ACPICA code isn't, so the code calling
ACPICA functions should check acpi_disabled when necessary.

Thanks,
Rafael

Re: [BUG] acpica: ltp_acpi test case causes kernel crash at acpi_ns_walk_namespace

[Vadim Lomovtsev]

On Wed, Dec 13, 2017 at 12:45:50AM +0100, Rafael J. Wysocki wrote:
> On Tuesday, December 12, 2017 4:59:19 PM CET Vadim Lomovtsev wrote:
> > Hi guys,
> > 
> > While running LTP tests I've faced kernel crash caused by ltp_acpi test case.
> > I have ACPI support enabled in kernel but kernel is boot with FDT having ACPI
> > disabled. The ltp_acpi test case application is built along with ltp_acpi_cmds
> > module to run ACPI tests.
> > 
> > So my question is - should we update acpica implementation at kernel side by
> > adding 'acpi_disabled' variable checking to the 'acpi_get_devices' function (see
> > patch next to this email, maybe not a good approach) or this should be fixed at LTP
> > side so the ltp_acpi_cmds should be updated in order to check if acpi is enabled
> > before running tests ?
> 
> There should be a check preventing acpi_get_devices() from being called in the
> acpi_disabled case.

In my case I have to update ltp_acpi code then.

> 
> acpi_disabled is Linux-specific and the ACPICA code isn't, so the code calling
> ACPICA functions should check acpi_disabled when necessary.

Agree. However getting back to LTP tests it looks like such calls were implemented
intentionally without checking of aspi_disabled value.

Don't we have any self-testing stuff in acpica to prevent such scenarious ?

WBR,
Vadim

> 
> Thanks,
> Rafael
> 

Re: [BUG] acpica: ltp_acpi test case causes kernel crash at acpi_ns_walk_namespace

[Rafael J. Wysocki]

On Wed, Dec 13, 2017 at 3:55 PM, Vadim Lomovtsev
<Vadim.Lomovtsev@caviumnetworks.com> wrote:
> On Wed, Dec 13, 2017 at 12:45:50AM +0100, Rafael J. Wysocki wrote:
>> On Tuesday, December 12, 2017 4:59:19 PM CET Vadim Lomovtsev wrote:
>> > Hi guys,
>> >
>> > While running LTP tests I've faced kernel crash caused by ltp_acpi test case.
>> > I have ACPI support enabled in kernel but kernel is boot with FDT having ACPI
>> > disabled. The ltp_acpi test case application is built along with ltp_acpi_cmds
>> > module to run ACPI tests.
>> >
>> > So my question is - should we update acpica implementation at kernel side by
>> > adding 'acpi_disabled' variable checking to the 'acpi_get_devices' function (see
>> > patch next to this email, maybe not a good approach) or this should be fixed at LTP
>> > side so the ltp_acpi_cmds should be updated in order to check if acpi is enabled
>> > before running tests ?
>>
>> There should be a check preventing acpi_get_devices() from being called in the
>> acpi_disabled case.
>
> In my case I have to update ltp_acpi code then.

RIght.

>>
>> acpi_disabled is Linux-specific and the ACPICA code isn't, so the code calling
>> ACPICA functions should check acpi_disabled when necessary.
>
> Agree. However getting back to LTP tests it looks like such calls were implemented
> intentionally without checking of aspi_disabled value.
>
> Don't we have any self-testing stuff in acpica to prevent such scenarious ?

ACPICA doesn't know anything about acpi_disabled as I said already.

I would argue that testing unsupported use cases in LTP is not very useful.

Thanks,
Rafael

Re: [BUG] acpica: ltp_acpi test case causes kernel crash at acpi_ns_walk_namespace

[Vadim Lomovtsev]

On Wed, Dec 13, 2017 at 05:52:38PM +0100, Rafael J. Wysocki wrote:
> On Wed, Dec 13, 2017 at 3:55 PM, Vadim Lomovtsev
> <Vadim.Lomovtsev@caviumnetworks.com> wrote:
> > On Wed, Dec 13, 2017 at 12:45:50AM +0100, Rafael J. Wysocki wrote:
> >> On Tuesday, December 12, 2017 4:59:19 PM CET Vadim Lomovtsev wrote:
> >> > Hi guys,
> >> >
> >> > While running LTP tests I've faced kernel crash caused by ltp_acpi test case.
> >> > I have ACPI support enabled in kernel but kernel is boot with FDT having ACPI
> >> > disabled. The ltp_acpi test case application is built along with ltp_acpi_cmds
> >> > module to run ACPI tests.
> >> >
> >> > So my question is - should we update acpica implementation at kernel side by
> >> > adding 'acpi_disabled' variable checking to the 'acpi_get_devices' function (see
> >> > patch next to this email, maybe not a good approach) or this should be fixed at LTP
> >> > side so the ltp_acpi_cmds should be updated in order to check if acpi is enabled
> >> > before running tests ?
> >>
> >> There should be a check preventing acpi_get_devices() from being called in the
> >> acpi_disabled case.
> >
> > In my case I have to update ltp_acpi code then.
> 
> RIght.
> 
> >>
> >> acpi_disabled is Linux-specific and the ACPICA code isn't, so the code calling
> >> ACPICA functions should check acpi_disabled when necessary.
> >
> > Agree. However getting back to LTP tests it looks like such calls were implemented
> > intentionally without checking of aspi_disabled value.
> >
> > Don't we have any self-testing stuff in acpica to prevent such scenarious ?
> 
> ACPICA doesn't know anything about acpi_disabled as I said already.

And I got it from the first time. And assuming that I said that suggested patch
'maybe not a good approach' in the issue description. Just wanted to
highlight the issue and initiate discussion.

> 
> I would argue that testing unsupported use cases in LTP is not very useful.
>

The ltp_acpi test in particular shows that there is no any protection from such actions
so I wouldn't said that it is useless use case. From another hand, those calls were intialted
from kernel module (which is part of test case) so the solution here is to place ACPI
state check there - into kernel module.

But again, see your point. Thank you for your time.

> Thanks,
> Rafael

WBR,
Vadim