when acpi=off is provided in bootarg, kernel crash with
[ 1.252739] BUG: kernel NULL pointer dereference, address: 0000000000000018
[ 1.258308] Call Trace:
[ 1.258490] ? acpi_walk_namespace+0x147/0x147
[ 1.258770] acpi_get_devices+0xe4/0x137
[ 1.258921] ? drm_core_init+0xc0/0xc0 [drm]
[ 1.259108] detect_thinkpad_privacy_screen+0x5e/0xa8 [drm]
[ 1.259337] drm_privacy_screen_lookup_init+0xe/0xe85 [drm]
The reason is that acpi_walk_namespace expects acpi related stuff
initialized but in fact it wouldn't when acpi is set to off. In this case
we should honor acpi=off in detect_thinkpad_privacy_screen().
Signed-off-by: Tong Zhang <[email protected]>
---
v2: fix typo in previous commit -- my keyboard is eating letters
drivers/gpu/drm/drm_privacy_screen_x86.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/drm_privacy_screen_x86.c b/drivers/gpu/drm/drm_privacy_screen_x86.c
index a2cafb294ca6..e7aa74ad0b24 100644
--- a/drivers/gpu/drm/drm_privacy_screen_x86.c
+++ b/drivers/gpu/drm/drm_privacy_screen_x86.c
@@ -33,6 +33,9 @@ static bool __init detect_thinkpad_privacy_screen(void)
unsigned long long output;
acpi_status status;
+ if (acpi_disabled)
+ return false;
+
/* Get embedded-controller handle */
status = acpi_get_devices("PNP0C09", acpi_set_handle, NULL, &ec_handle);
if (ACPI_FAILURE(status) || !ec_handle)
--
2.25.1
Hi All,
On 1/23/22 10:10, Tong Zhang wrote:
> when acpi=off is provided in bootarg, kernel crash with
>
> [ 1.252739] BUG: kernel NULL pointer dereference, address: 0000000000000018
> [ 1.258308] Call Trace:
> [ 1.258490] ? acpi_walk_namespace+0x147/0x147
> [ 1.258770] acpi_get_devices+0xe4/0x137
> [ 1.258921] ? drm_core_init+0xc0/0xc0 [drm]
> [ 1.259108] detect_thinkpad_privacy_screen+0x5e/0xa8 [drm]
> [ 1.259337] drm_privacy_screen_lookup_init+0xe/0xe85 [drm]
>
> The reason is that acpi_walk_namespace expects acpi related stuff
> initialized but in fact it wouldn't when acpi is set to off. In this case
> we should honor acpi=off in detect_thinkpad_privacy_screen().
>
> Signed-off-by: Tong Zhang <[email protected]>
Thank you for catching this and thank you for your patch. I was about to merge
this, but then I realized that this might not be the best way to fix this.
A quick grep shows 10 acpi_get_devices() calls outside of drivers/acpi,
and at a first glance about half of those are missing an acpi_disabled
check. IMHO it would be better to simply add an acpi_disabled check to
acpi_get_devices() itself.
Rafael, do you agree ?
Note the just added chrome privacy-screen check uses
acpi_dev_present(), this is also used in about 10 places outside
of drivers/acpi and AFAIK none of those do an acpi_disabled check.
acpi_dev_present() uses bus_find_device(&acpi_bus_type, ...)
but the acpi_bus_type does not get registered when acpi_disabled
is set. In the end this is fine though since bus_find_device
checks for the bus not being registered and then just returns
NULL.
Regards,
Hans
> ---
> v2: fix typo in previous commit -- my keyboard is eating letters
>
> drivers/gpu/drm/drm_privacy_screen_x86.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_privacy_screen_x86.c b/drivers/gpu/drm/drm_privacy_screen_x86.c
> index a2cafb294ca6..e7aa74ad0b24 100644
> --- a/drivers/gpu/drm/drm_privacy_screen_x86.c
> +++ b/drivers/gpu/drm/drm_privacy_screen_x86.c
> @@ -33,6 +33,9 @@ static bool __init detect_thinkpad_privacy_screen(void)
> unsigned long long output;
> acpi_status status;
>
> + if (acpi_disabled)
> + return false;
> +
> /* Get embedded-controller handle */
> status = acpi_get_devices("PNP0C09", acpi_set_handle, NULL, &ec_handle);
> if (ACPI_FAILURE(status) || !ec_handle)
Hi,
On 1/26/22 14:47, Hans de Goede wrote:
> Hi All,
>
> On 1/23/22 10:10, Tong Zhang wrote:
>> when acpi=off is provided in bootarg, kernel crash with
>>
>> [ 1.252739] BUG: kernel NULL pointer dereference, address: 0000000000000018
>> [ 1.258308] Call Trace:
>> [ 1.258490] ? acpi_walk_namespace+0x147/0x147
>> [ 1.258770] acpi_get_devices+0xe4/0x137
>> [ 1.258921] ? drm_core_init+0xc0/0xc0 [drm]
>> [ 1.259108] detect_thinkpad_privacy_screen+0x5e/0xa8 [drm]
>> [ 1.259337] drm_privacy_screen_lookup_init+0xe/0xe85 [drm]
>>
>> The reason is that acpi_walk_namespace expects acpi related stuff
>> initialized but in fact it wouldn't when acpi is set to off. In this case
>> we should honor acpi=off in detect_thinkpad_privacy_screen().
>>
>> Signed-off-by: Tong Zhang <[email protected]>
>
> Thank you for catching this and thank you for your patch. I was about to merge
> this, but then I realized that this might not be the best way to fix this.
>
> A quick grep shows 10 acpi_get_devices() calls outside of drivers/acpi,
> and at a first glance about half of those are missing an acpi_disabled
> check. IMHO it would be better to simply add an acpi_disabled check to
> acpi_get_devices() itself.
>
> Rafael, do you agree ?
Never mind I just saw that acpi_get_devices() is part of acpica, where
as the acpi_disabled flag is not. So callers need to check acpi_disabled
before calling acpi_get_devices().
I'll go and push this patch to drm-misc-fixes now.
Regards,
Hans
>> ---
>> v2: fix typo in previous commit -- my keyboard is eating letters
>>
>> drivers/gpu/drm/drm_privacy_screen_x86.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_privacy_screen_x86.c b/drivers/gpu/drm/drm_privacy_screen_x86.c
>> index a2cafb294ca6..e7aa74ad0b24 100644
>> --- a/drivers/gpu/drm/drm_privacy_screen_x86.c
>> +++ b/drivers/gpu/drm/drm_privacy_screen_x86.c
>> @@ -33,6 +33,9 @@ static bool __init detect_thinkpad_privacy_screen(void)
>> unsigned long long output;
>> acpi_status status;
>>
>> + if (acpi_disabled)
>> + return false;
>> +
>> /* Get embedded-controller handle */
>> status = acpi_get_devices("PNP0C09", acpi_set_handle, NULL, &ec_handle);
>> if (ACPI_FAILURE(status) || !ec_handle)
On Wed, Jan 26, 2022 at 2:47 PM Hans de Goede <[email protected]> wrote:
>
> Hi All,
>
> On 1/23/22 10:10, Tong Zhang wrote:
> > when acpi=off is provided in bootarg, kernel crash with
> >
> > [ 1.252739] BUG: kernel NULL pointer dereference, address: 0000000000000018
> > [ 1.258308] Call Trace:
> > [ 1.258490] ? acpi_walk_namespace+0x147/0x147
> > [ 1.258770] acpi_get_devices+0xe4/0x137
> > [ 1.258921] ? drm_core_init+0xc0/0xc0 [drm]
> > [ 1.259108] detect_thinkpad_privacy_screen+0x5e/0xa8 [drm]
> > [ 1.259337] drm_privacy_screen_lookup_init+0xe/0xe85 [drm]
> >
> > The reason is that acpi_walk_namespace expects acpi related stuff
> > initialized but in fact it wouldn't when acpi is set to off. In this case
> > we should honor acpi=off in detect_thinkpad_privacy_screen().
> >
> > Signed-off-by: Tong Zhang <[email protected]>
>
> Thank you for catching this and thank you for your patch. I was about to merge
> this, but then I realized that this might not be the best way to fix this.
>
> A quick grep shows 10 acpi_get_devices() calls outside of drivers/acpi,
> and at a first glance about half of those are missing an acpi_disabled
> check. IMHO it would be better to simply add an acpi_disabled check to
> acpi_get_devices() itself.
>
> Rafael, do you agree ?
Yes, I do.
> Note the just added chrome privacy-screen check uses
> acpi_dev_present(), this is also used in about 10 places outside
> of drivers/acpi and AFAIK none of those do an acpi_disabled check.
>
> acpi_dev_present() uses bus_find_device(&acpi_bus_type, ...)
> but the acpi_bus_type does not get registered when acpi_disabled
> is set. In the end this is fine though since bus_find_device
> checks for the bus not being registered and then just returns
> NULL.
Right.
> > ---
> > v2: fix typo in previous commit -- my keyboard is eating letters
> >
> > drivers/gpu/drm/drm_privacy_screen_x86.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_privacy_screen_x86.c b/drivers/gpu/drm/drm_privacy_screen_x86.c
> > index a2cafb294ca6..e7aa74ad0b24 100644
> > --- a/drivers/gpu/drm/drm_privacy_screen_x86.c
> > +++ b/drivers/gpu/drm/drm_privacy_screen_x86.c
> > @@ -33,6 +33,9 @@ static bool __init detect_thinkpad_privacy_screen(void)
> > unsigned long long output;
> > acpi_status status;
> >
> > + if (acpi_disabled)
> > + return false;
> > +
> > /* Get embedded-controller handle */
> > status = acpi_get_devices("PNP0C09", acpi_set_handle, NULL, &ec_handle);
> > if (ACPI_FAILURE(status) || !ec_handle)
>
On Wed, Jan 26, 2022 at 5:41 PM Hans de Goede <[email protected]> wrote:
>
> Hi,
>
> On 1/26/22 16:54, Rafael J. Wysocki wrote:
> > On Wed, Jan 26, 2022 at 2:47 PM Hans de Goede <[email protected]> wrote:
> >>
> >> Hi All,
> >>
> >> On 1/23/22 10:10, Tong Zhang wrote:
> >>> when acpi=off is provided in bootarg, kernel crash with
> >>>
> >>> [ 1.252739] BUG: kernel NULL pointer dereference, address: 0000000000000018
> >>> [ 1.258308] Call Trace:
> >>> [ 1.258490] ? acpi_walk_namespace+0x147/0x147
> >>> [ 1.258770] acpi_get_devices+0xe4/0x137
> >>> [ 1.258921] ? drm_core_init+0xc0/0xc0 [drm]
> >>> [ 1.259108] detect_thinkpad_privacy_screen+0x5e/0xa8 [drm]
> >>> [ 1.259337] drm_privacy_screen_lookup_init+0xe/0xe85 [drm]
> >>>
> >>> The reason is that acpi_walk_namespace expects acpi related stuff
> >>> initialized but in fact it wouldn't when acpi is set to off. In this case
> >>> we should honor acpi=off in detect_thinkpad_privacy_screen().
> >>>
> >>> Signed-off-by: Tong Zhang <[email protected]>
> >>
> >> Thank you for catching this and thank you for your patch. I was about to merge
> >> this, but then I realized that this might not be the best way to fix this.
> >>
> >> A quick grep shows 10 acpi_get_devices() calls outside of drivers/acpi,
> >> and at a first glance about half of those are missing an acpi_disabled
> >> check. IMHO it would be better to simply add an acpi_disabled check to
> >> acpi_get_devices() itself.
> >>
> >> Rafael, do you agree ?
> >
> > Yes, I do.
>
> Did you see my follow-up that that is not going to work because
> acpi_get_devices() is an acpica function ?
No, I didn't, but it is possible to add a wrapper doing the check
around it and convert all of the users.
Alternatively, the ACPICA function can check acpi_gbl_root_node
against NULL, like in the attached (untested) patch.
Hi,
On 1/26/22 16:54, Rafael J. Wysocki wrote:
> On Wed, Jan 26, 2022 at 2:47 PM Hans de Goede <[email protected]> wrote:
>>
>> Hi All,
>>
>> On 1/23/22 10:10, Tong Zhang wrote:
>>> when acpi=off is provided in bootarg, kernel crash with
>>>
>>> [ 1.252739] BUG: kernel NULL pointer dereference, address: 0000000000000018
>>> [ 1.258308] Call Trace:
>>> [ 1.258490] ? acpi_walk_namespace+0x147/0x147
>>> [ 1.258770] acpi_get_devices+0xe4/0x137
>>> [ 1.258921] ? drm_core_init+0xc0/0xc0 [drm]
>>> [ 1.259108] detect_thinkpad_privacy_screen+0x5e/0xa8 [drm]
>>> [ 1.259337] drm_privacy_screen_lookup_init+0xe/0xe85 [drm]
>>>
>>> The reason is that acpi_walk_namespace expects acpi related stuff
>>> initialized but in fact it wouldn't when acpi is set to off. In this case
>>> we should honor acpi=off in detect_thinkpad_privacy_screen().
>>>
>>> Signed-off-by: Tong Zhang <[email protected]>
>>
>> Thank you for catching this and thank you for your patch. I was about to merge
>> this, but then I realized that this might not be the best way to fix this.
>>
>> A quick grep shows 10 acpi_get_devices() calls outside of drivers/acpi,
>> and at a first glance about half of those are missing an acpi_disabled
>> check. IMHO it would be better to simply add an acpi_disabled check to
>> acpi_get_devices() itself.
>>
>> Rafael, do you agree ?
>
> Yes, I do.
Did you see my follow-up that that is not going to work because
acpi_get_devices() is an acpica function ?
Regards,
Hans
>>> ---
>>> v2: fix typo in previous commit -- my keyboard is eating letters
>>>
>>> drivers/gpu/drm/drm_privacy_screen_x86.c | 3 +++
>>> 1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_privacy_screen_x86.c b/drivers/gpu/drm/drm_privacy_screen_x86.c
>>> index a2cafb294ca6..e7aa74ad0b24 100644
>>> --- a/drivers/gpu/drm/drm_privacy_screen_x86.c
>>> +++ b/drivers/gpu/drm/drm_privacy_screen_x86.c
>>> @@ -33,6 +33,9 @@ static bool __init detect_thinkpad_privacy_screen(void)
>>> unsigned long long output;
>>> acpi_status status;
>>>
>>> + if (acpi_disabled)
>>> + return false;
>>> +
>>> /* Get embedded-controller handle */
>>> status = acpi_get_devices("PNP0C09", acpi_set_handle, NULL, &ec_handle);
>>> if (ACPI_FAILURE(status) || !ec_handle)
>>
>
Hi,
On 1/26/22 18:11, Rafael J. Wysocki wrote:
> On Wed, Jan 26, 2022 at 5:41 PM Hans de Goede <[email protected]> wrote:
>>
>> Hi,
>>
>> On 1/26/22 16:54, Rafael J. Wysocki wrote:
>>> On Wed, Jan 26, 2022 at 2:47 PM Hans de Goede <[email protected]> wrote:
>>>>
>>>> Hi All,
>>>>
>>>> On 1/23/22 10:10, Tong Zhang wrote:
>>>>> when acpi=off is provided in bootarg, kernel crash with
>>>>>
>>>>> [ 1.252739] BUG: kernel NULL pointer dereference, address: 0000000000000018
>>>>> [ 1.258308] Call Trace:
>>>>> [ 1.258490] ? acpi_walk_namespace+0x147/0x147
>>>>> [ 1.258770] acpi_get_devices+0xe4/0x137
>>>>> [ 1.258921] ? drm_core_init+0xc0/0xc0 [drm]
>>>>> [ 1.259108] detect_thinkpad_privacy_screen+0x5e/0xa8 [drm]
>>>>> [ 1.259337] drm_privacy_screen_lookup_init+0xe/0xe85 [drm]
>>>>>
>>>>> The reason is that acpi_walk_namespace expects acpi related stuff
>>>>> initialized but in fact it wouldn't when acpi is set to off. In this case
>>>>> we should honor acpi=off in detect_thinkpad_privacy_screen().
>>>>>
>>>>> Signed-off-by: Tong Zhang <[email protected]>
>>>>
>>>> Thank you for catching this and thank you for your patch. I was about to merge
>>>> this, but then I realized that this might not be the best way to fix this.
>>>>
>>>> A quick grep shows 10 acpi_get_devices() calls outside of drivers/acpi,
>>>> and at a first glance about half of those are missing an acpi_disabled
>>>> check. IMHO it would be better to simply add an acpi_disabled check to
>>>> acpi_get_devices() itself.
>>>>
>>>> Rafael, do you agree ?
>>>
>>> Yes, I do.
>>
>> Did you see my follow-up that that is not going to work because
>> acpi_get_devices() is an acpica function ?
>
> No, I didn't, but it is possible to add a wrapper doing the check
> around it and convert all of the users.
Yes I did think about that. Note that I've gone ahead and pushed
the fix which started this to drm-misc-fixes, to resolve the crash
for now.
If we add such a wrapper we can remove a bunch of acpi_disabled checks
from various callers.
> Alternatively, the ACPICA function can check acpi_gbl_root_node
> against NULL, like in the attached (untested) patch.
That is probably an even better idea, as that avoids the need
for a wrapper altogether. So I believe that that is the best
solution.
Regards,
Hans
On Thu, Jan 27, 2022 at 2:05 PM Hans de Goede <[email protected]> wrote:
>
> Hi,
>
> On 1/26/22 18:11, Rafael J. Wysocki wrote:
> > On Wed, Jan 26, 2022 at 5:41 PM Hans de Goede <[email protected]> wrote:
> >>
> >> Hi,
> >>
> >> On 1/26/22 16:54, Rafael J. Wysocki wrote:
> >>> On Wed, Jan 26, 2022 at 2:47 PM Hans de Goede <[email protected]> wrote:
> >>>>
> >>>> Hi All,
> >>>>
> >>>> On 1/23/22 10:10, Tong Zhang wrote:
> >>>>> when acpi=off is provided in bootarg, kernel crash with
> >>>>>
> >>>>> [ 1.252739] BUG: kernel NULL pointer dereference, address: 0000000000000018
> >>>>> [ 1.258308] Call Trace:
> >>>>> [ 1.258490] ? acpi_walk_namespace+0x147/0x147
> >>>>> [ 1.258770] acpi_get_devices+0xe4/0x137
> >>>>> [ 1.258921] ? drm_core_init+0xc0/0xc0 [drm]
> >>>>> [ 1.259108] detect_thinkpad_privacy_screen+0x5e/0xa8 [drm]
> >>>>> [ 1.259337] drm_privacy_screen_lookup_init+0xe/0xe85 [drm]
> >>>>>
> >>>>> The reason is that acpi_walk_namespace expects acpi related stuff
> >>>>> initialized but in fact it wouldn't when acpi is set to off. In this case
> >>>>> we should honor acpi=off in detect_thinkpad_privacy_screen().
> >>>>>
> >>>>> Signed-off-by: Tong Zhang <[email protected]>
> >>>>
> >>>> Thank you for catching this and thank you for your patch. I was about to merge
> >>>> this, but then I realized that this might not be the best way to fix this.
> >>>>
> >>>> A quick grep shows 10 acpi_get_devices() calls outside of drivers/acpi,
> >>>> and at a first glance about half of those are missing an acpi_disabled
> >>>> check. IMHO it would be better to simply add an acpi_disabled check to
> >>>> acpi_get_devices() itself.
> >>>>
> >>>> Rafael, do you agree ?
> >>>
> >>> Yes, I do.
> >>
> >> Did you see my follow-up that that is not going to work because
> >> acpi_get_devices() is an acpica function ?
> >
> > No, I didn't, but it is possible to add a wrapper doing the check
> > around it and convert all of the users.
>
> Yes I did think about that. Note that I've gone ahead and pushed
> the fix which started this to drm-misc-fixes, to resolve the crash
> for now.
OK
> If we add such a wrapper we can remove a bunch of acpi_disabled checks
> from various callers.
>
> > Alternatively, the ACPICA function can check acpi_gbl_root_node
> > against NULL, like in the attached (untested) patch.
>
> That is probably an even better idea, as that avoids the need
> for a wrapper altogether. So I believe that that is the best
> solution.
Allright, let me cut an analogous patch for the upstream ACPICA, then.
Hi,
On 1/27/22 14:33, Rafael J. Wysocki wrote:
> On Thu, Jan 27, 2022 at 2:05 PM Hans de Goede <[email protected]> wrote:
>>
>> Hi,
>>
>> On 1/26/22 18:11, Rafael J. Wysocki wrote:
>>> On Wed, Jan 26, 2022 at 5:41 PM Hans de Goede <[email protected]> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 1/26/22 16:54, Rafael J. Wysocki wrote:
>>>>> On Wed, Jan 26, 2022 at 2:47 PM Hans de Goede <[email protected]> wrote:
>>>>>>
>>>>>> Hi All,
>>>>>>
>>>>>> On 1/23/22 10:10, Tong Zhang wrote:
>>>>>>> when acpi=off is provided in bootarg, kernel crash with
>>>>>>>
>>>>>>> [ 1.252739] BUG: kernel NULL pointer dereference, address: 0000000000000018
>>>>>>> [ 1.258308] Call Trace:
>>>>>>> [ 1.258490] ? acpi_walk_namespace+0x147/0x147
>>>>>>> [ 1.258770] acpi_get_devices+0xe4/0x137
>>>>>>> [ 1.258921] ? drm_core_init+0xc0/0xc0 [drm]
>>>>>>> [ 1.259108] detect_thinkpad_privacy_screen+0x5e/0xa8 [drm]
>>>>>>> [ 1.259337] drm_privacy_screen_lookup_init+0xe/0xe85 [drm]
>>>>>>>
>>>>>>> The reason is that acpi_walk_namespace expects acpi related stuff
>>>>>>> initialized but in fact it wouldn't when acpi is set to off. In this case
>>>>>>> we should honor acpi=off in detect_thinkpad_privacy_screen().
>>>>>>>
>>>>>>> Signed-off-by: Tong Zhang <[email protected]>
>>>>>>
>>>>>> Thank you for catching this and thank you for your patch. I was about to merge
>>>>>> this, but then I realized that this might not be the best way to fix this.
>>>>>>
>>>>>> A quick grep shows 10 acpi_get_devices() calls outside of drivers/acpi,
>>>>>> and at a first glance about half of those are missing an acpi_disabled
>>>>>> check. IMHO it would be better to simply add an acpi_disabled check to
>>>>>> acpi_get_devices() itself.
>>>>>>
>>>>>> Rafael, do you agree ?
>>>>>
>>>>> Yes, I do.
>>>>
>>>> Did you see my follow-up that that is not going to work because
>>>> acpi_get_devices() is an acpica function ?
>>>
>>> No, I didn't, but it is possible to add a wrapper doing the check
>>> around it and convert all of the users.
>>
>> Yes I did think about that. Note that I've gone ahead and pushed
>> the fix which started this to drm-misc-fixes, to resolve the crash
>> for now.
>
> OK
>
>> If we add such a wrapper we can remove a bunch of acpi_disabled checks
>> from various callers.
>>
>>> Alternatively, the ACPICA function can check acpi_gbl_root_node
>>> against NULL, like in the attached (untested) patch.
>>
>> That is probably an even better idea, as that avoids the need
>> for a wrapper altogether. So I believe that that is the best
>> solution.
>
> Allright, let me cut an analogous patch for the upstream ACPICA, then.
Great, thank you.
I have added a note about checking for when this has found its way
into Linus' tree to my own TODO list, with the goal of doing
a cleanup series removing the then no longer needed acpi_disabled
checks in a bunch of places.
Regards,
Hans