ACPICA regression for empty ResourceTemplates

ACPICA regression for empty ResourceTemplates

[Lukas Wunner]

Hi Robert,

ACPICA commit c8eac101 (= Linux commit a83019eb9f1f, "ACPICA: Update
resource descriptor handling") is causing a regression when parsing
an empty ResourceTemplate:

acpi_dev_get_resources() now returns -EIO because acpi_walk_resources()
returns AE_AML_NO_RESOURCE_END_TAG for a buffer than contains just a
lone end_tag.

The ACPI 6.1 spec, page 800 explicitly allows empty resource templates:

    ResourceTemplateTerm :=
        ResourceTemplate () {ResourceMacroList} => Buffer
    ResourceMacroList :=
        Nothing | <ResourceMacroTerm ResourceMacroList>
        ^^^^^^^

Thus a term such as "ResourceTemplate () {}" is legal and should not cause
an error to be returned.

The commit claims to revert Rafael's Linux commit 1315f01632da ("Revert
"ACPICA: Resources: Not a valid resource if buffer length too long"),
but in fact does something different.

Please fix at your earliest convenience.  The issue was discovered and
root-caused by Ronald Tschalär, please credit him in the fix, I'm merely
relaying the information:

Reported-by: Ronald Tschalär <ronald@innovation.ch>

Thanks,

Lukas

Re: ACPICA regression for empty ResourceTemplates

[Rafael J. Wysocki]

On Mon, Jun 26, 2017 at 10:38 AM, Lukas Wunner <lukas@wunner.de> wrote:
> Hi Robert,
>
> ACPICA commit c8eac101 (= Linux commit a83019eb9f1f, "ACPICA: Update
> resource descriptor handling") is causing a regression when parsing
> an empty ResourceTemplate:

This is a linux-next commit, right?

Thanks,
Rafael

Re: ACPICA regression for empty ResourceTemplates

[Lukas Wunner]

On Mon, Jun 26, 2017 at 01:49:03PM +0200, Rafael J. Wysocki wrote:
> On Mon, Jun 26, 2017 at 10:38 AM, Lukas Wunner <lukas@wunner.de> wrote:
> > Hi Robert,
> >
> > ACPICA commit c8eac101 (= Linux commit a83019eb9f1f, "ACPICA: Update
> > resource descriptor handling") is causing a regression when parsing
> > an empty ResourceTemplate:
> 
> This is a linux-next commit, right?

Yes.

Lukas

Re: ACPICA regression for empty ResourceTemplates

[Rafael J. Wysocki]

On Mon, Jun 26, 2017 at 2:38 PM, Lukas Wunner <lukas@wunner.de> wrote:
> On Mon, Jun 26, 2017 at 01:49:03PM +0200, Rafael J. Wysocki wrote:
>> On Mon, Jun 26, 2017 at 10:38 AM, Lukas Wunner <lukas@wunner.de> wrote:
>> > Hi Robert,
>> >
>> > ACPICA commit c8eac101 (= Linux commit a83019eb9f1f, "ACPICA: Update
>> > resource descriptor handling") is causing a regression when parsing
>> > an empty ResourceTemplate:
>>
>> This is a linux-next commit, right?
>
> Yes.

OK, let's give Bob and Lv some time to respond.  Worst case I'll drop
it from linux-next.

Thanks,
Rafael

RE: ACPICA regression for empty ResourceTemplates

[Zheng, Lv]

Hi,

> -----Original Message-----
> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Lukas
> Wunner
> Sent: Monday, June 26, 2017 8:39 PM
> To: Rafael J. Wysocki <rafael@kernel.org>
> Cc: Moore, Robert <robert.moore@intel.com>; Zheng, Lv <lv.zheng@intel.com>; Wysocki, Rafael J
> <rafael.j.wysocki@intel.com>; Ronald Tschalaer <ronald@innovation.ch>; ACPI Devel Maling List <linux-
> acpi@vger.kernel.org>
> Subject: Re: ACPICA regression for empty ResourceTemplates
> 
> On Mon, Jun 26, 2017 at 01:49:03PM +0200, Rafael J. Wysocki wrote:
> > On Mon, Jun 26, 2017 at 10:38 AM, Lukas Wunner <lukas@wunner.de> wrote:
> > > Hi Robert,
> > >
> > > ACPICA commit c8eac101 (= Linux commit a83019eb9f1f, "ACPICA: Update
> > > resource descriptor handling") is causing a regression when parsing
> > > an empty ResourceTemplate:
> >
> > This is a linux-next commit, right?
> 
> Yes.

Can it be fixed by using this logic:
if (AmlLength < sizeof (AML_RESOURCE_END_TAG) && AmlLength)
Instead of the logic introduced in commit a83019eb9f1f?
if (AmlLength < sizeof (AML_RESOURCE_END_TAG))

If so, could you please offer a fix for upstream?

Thanks and best regards
Lv

> 
> Lukas
> --
> 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

Re: ACPICA regression for empty ResourceTemplates

[Lukas Wunner]

On Tue, Jun 27, 2017 at 01:19:31AM +0000, Zheng, Lv wrote:
> > On Mon, Jun 26, 2017 at 01:49:03PM +0200, Rafael J. Wysocki wrote:
> > > On Mon, Jun 26, 2017 at 10:38 AM, Lukas Wunner <lukas@wunner.de> wrote:
> > > > Hi Robert,
> > > >
> > > > ACPICA commit c8eac101 (= Linux commit a83019eb9f1f, "ACPICA: Update
> > > > resource descriptor handling") is causing a regression when parsing
> > > > an empty ResourceTemplate:
> 
> Can it be fixed by using this logic:
> if (AmlLength < sizeof (AML_RESOURCE_END_TAG) && AmlLength)
> Instead of the logic introduced in commit a83019eb9f1f?
> if (AmlLength < sizeof (AML_RESOURCE_END_TAG))

Unfortunately not, AmlLength is not 0 but 2 in this case
(= sizeof(struct aml_resource_end_tag)).


> If so, could you please offer a fix for upstream?

I'd suggest just reverting the commit, a buffer containing
a lone end tag seems to be legal according to the spec, so
returning an error seems wrong.

Thanks,

Lukas

RE: ACPICA regression for empty ResourceTemplates

[Moore, Robert]

> -----Original Message-----
> From: Lukas Wunner [mailto:lukas@wunner.de]
> Sent: Tuesday, June 27, 2017 2:24 AM
> To: Zheng, Lv <lv.zheng@intel.com>
> Cc: Rafael J. Wysocki <rafael@kernel.org>; Moore, Robert
> <robert.moore@intel.com>; Wysocki, Rafael J
> <rafael.j.wysocki@intel.com>; Ronald Tschalaer <ronald@innovation.ch>;
> ACPI Devel Maling List <linux-acpi@vger.kernel.org>
> Subject: Re: ACPICA regression for empty ResourceTemplates
> 
> On Tue, Jun 27, 2017 at 01:19:31AM +0000, Zheng, Lv wrote:
> > > On Mon, Jun 26, 2017 at 01:49:03PM +0200, Rafael J. Wysocki wrote:
> > > > On Mon, Jun 26, 2017 at 10:38 AM, Lukas Wunner <lukas@wunner.de>
> wrote:
> > > > > Hi Robert,
> > > > >
> > > > > ACPICA commit c8eac101 (= Linux commit a83019eb9f1f, "ACPICA:
> > > > > Update resource descriptor handling") is causing a regression
> > > > > when parsing an empty ResourceTemplate:
> >
> > Can it be fixed by using this logic:
> > if (AmlLength < sizeof (AML_RESOURCE_END_TAG) && AmlLength) Instead of
> > the logic introduced in commit a83019eb9f1f?
> > if (AmlLength < sizeof (AML_RESOURCE_END_TAG))
> 
> Unfortunately not, AmlLength is not 0 but 2 in this case (=
> sizeof(struct aml_resource_end_tag)).
> 
> 
> > If so, could you please offer a fix for upstream?
[Moore, Robert] 

Is the problem seen with something like this?

    Name (BUF1, ResourceTemplate ()
    {
    })

When BUF1 is evaluated, the exception happens?

I think we have all of the ResourceTemplate issues resolved for the next release of ACPICA.
Bob



> 
> I'd suggest just reverting the commit, a buffer containing a lone end
> tag seems to be legal according to the spec, so returning an error seems
> wrong.
> 
> Thanks,
> 
> Lukas

Re: ACPICA regression for empty ResourceTemplates

[Rafael J. Wysocki]

On Tue, Jun 27, 2017 at 5:08 PM, Moore, Robert <robert.moore@intel.com> wrote:
>
>
>> -----Original Message-----
>> From: Lukas Wunner [mailto:lukas@wunner.de]
>> Sent: Tuesday, June 27, 2017 2:24 AM
>> To: Zheng, Lv <lv.zheng@intel.com>
>> Cc: Rafael J. Wysocki <rafael@kernel.org>; Moore, Robert
>> <robert.moore@intel.com>; Wysocki, Rafael J
>> <rafael.j.wysocki@intel.com>; Ronald Tschalaer <ronald@innovation.ch>;
>> ACPI Devel Maling List <linux-acpi@vger.kernel.org>
>> Subject: Re: ACPICA regression for empty ResourceTemplates
>>
>> On Tue, Jun 27, 2017 at 01:19:31AM +0000, Zheng, Lv wrote:
>> > > On Mon, Jun 26, 2017 at 01:49:03PM +0200, Rafael J. Wysocki wrote:
>> > > > On Mon, Jun 26, 2017 at 10:38 AM, Lukas Wunner <lukas@wunner.de>
>> wrote:
>> > > > > Hi Robert,
>> > > > >
>> > > > > ACPICA commit c8eac101 (= Linux commit a83019eb9f1f, "ACPICA:
>> > > > > Update resource descriptor handling") is causing a regression
>> > > > > when parsing an empty ResourceTemplate:
>> >
>> > Can it be fixed by using this logic:
>> > if (AmlLength < sizeof (AML_RESOURCE_END_TAG) && AmlLength) Instead of
>> > the logic introduced in commit a83019eb9f1f?
>> > if (AmlLength < sizeof (AML_RESOURCE_END_TAG))
>>
>> Unfortunately not, AmlLength is not 0 but 2 in this case (=
>> sizeof(struct aml_resource_end_tag)).
>>
>>
>> > If so, could you please offer a fix for upstream?
> [Moore, Robert]
>
> Is the problem seen with something like this?
>
>     Name (BUF1, ResourceTemplate ()
>     {
>     })
>
> When BUF1 is evaluated, the exception happens?
>
> I think we have all of the ResourceTemplate issues resolved for the next release of ACPICA.

OK

So I will drop this commit for now and I will add it again along with
the next release material to avoid triggering regressions in the
meantime.

Thanks,
Rafael

Re: ACPICA regression for empty ResourceTemplates

[Life is hard, and then you die]

On Tue, Jun 27, 2017 at 11:24:25AM +0200, Lukas Wunner wrote:
> On Tue, Jun 27, 2017 at 01:19:31AM +0000, Zheng, Lv wrote:
> > > On Mon, Jun 26, 2017 at 01:49:03PM +0200, Rafael J. Wysocki wrote:
> > > > On Mon, Jun 26, 2017 at 10:38 AM, Lukas Wunner <lukas@wunner.de> wrote:
> > > > > Hi Robert,
> > > > >
> > > > > ACPICA commit c8eac101 (= Linux commit a83019eb9f1f, "ACPICA: Update
> > > > > resource descriptor handling") is causing a regression when parsing
> > > > > an empty ResourceTemplate:
> > 
> > Can it be fixed by using this logic:
> > if (AmlLength < sizeof (AML_RESOURCE_END_TAG) && AmlLength)
> > Instead of the logic introduced in commit a83019eb9f1f?
> > if (AmlLength < sizeof (AML_RESOURCE_END_TAG))
> 
> Unfortunately not, AmlLength is not 0 but 2 in this case
> (= sizeof(struct aml_resource_end_tag)).

Actually, that will work here (the use of '<' ensures that). But it's
unclear whether this is semantically the correct thing, in particular
because a83019eb9f1f widened the cases where NO_RESOURCE_END_TAG is
returned, but the above suggestion narrows them. Unfortunately commit
a83019eb9f1f is quite vague about why it reverted back to '<='.


  Cheers,

  Ronald

RE: ACPICA regression for empty ResourceTemplates

[Moore, Robert]

> -----Original Message-----
> From: Life is hard, and then you die [mailto:ronald@innovation.ch]
> Sent: Tuesday, June 27, 2017 11:59 AM
> To: Lukas Wunner <lukas@wunner.de>
> Cc: Zheng, Lv <lv.zheng@intel.com>; Rafael J. Wysocki
> <rafael@kernel.org>; Moore, Robert <robert.moore@intel.com>; Wysocki,
> Rafael J <rafael.j.wysocki@intel.com>; ACPI Devel Maling List <linux-
> acpi@vger.kernel.org>
> Subject: Re: ACPICA regression for empty ResourceTemplates
> 
> 
> On Tue, Jun 27, 2017 at 11:24:25AM +0200, Lukas Wunner wrote:
> > On Tue, Jun 27, 2017 at 01:19:31AM +0000, Zheng, Lv wrote:
> > > > On Mon, Jun 26, 2017 at 01:49:03PM +0200, Rafael J. Wysocki wrote:
> > > > > On Mon, Jun 26, 2017 at 10:38 AM, Lukas Wunner <lukas@wunner.de>
> wrote:
> > > > > > Hi Robert,
> > > > > >
> > > > > > ACPICA commit c8eac101 (= Linux commit a83019eb9f1f, "ACPICA:
> > > > > > Update resource descriptor handling") is causing a regression
> > > > > > when parsing an empty ResourceTemplate:
> > >
> > > Can it be fixed by using this logic:
> > > if (AmlLength < sizeof (AML_RESOURCE_END_TAG) && AmlLength) Instead
> > > of the logic introduced in commit a83019eb9f1f?
> > > if (AmlLength < sizeof (AML_RESOURCE_END_TAG))
> >
> > Unfortunately not, AmlLength is not 0 but 2 in this case (=
> > sizeof(struct aml_resource_end_tag)).
> 
> Actually, that will work here (the use of '<' ensures that). But it's
> unclear whether this is semantically the correct thing, in particular
> because a83019eb9f1f widened the cases where NO_RESOURCE_END_TAG is
> returned, but the above suggestion narrows them. Unfortunately commit
> a83019eb9f1f is quite vague about why it reverted back to '<='.
> 
>
[Moore, Robert] 

Yes, the <= is wrong and should be <

There was an odd corner case that was solved by the <=, even though it looks stupid (and is of course incorrect for most cases.)

We have not been able to reproduce this corner case again (yet), so we will revert the <= change back to the <

Bob

>   Cheers,
> 
>   Ronald

Re: ACPICA regression for empty ResourceTemplates

[Lukas Wunner]

On Tue, Jun 27, 2017 at 11:59:17AM -0700, Life is hard, and then you die wrote:
> On Tue, Jun 27, 2017 at 11:24:25AM +0200, Lukas Wunner wrote:
> > On Tue, Jun 27, 2017 at 01:19:31AM +0000, Zheng, Lv wrote:
> > > > On Mon, Jun 26, 2017 at 10:38 AM, Lukas Wunner <lukas@wunner.de> wrote:
> > > > > ACPICA commit c8eac101 (= Linux commit a83019eb9f1f, "ACPICA: Update
> > > > > resource descriptor handling") is causing a regression when parsing
> > > > > an empty ResourceTemplate:
> > > 
> > > Can it be fixed by using this logic:
> > > if (AmlLength < sizeof (AML_RESOURCE_END_TAG) && AmlLength)
> > > Instead of the logic introduced in commit a83019eb9f1f?
> > > if (AmlLength < sizeof (AML_RESOURCE_END_TAG))
> > 
> > Unfortunately not, AmlLength is not 0 but 2 in this case
> > (= sizeof(struct aml_resource_end_tag)).
> 
> Actually, that will work here (the use of '<' ensures that).

Ugh, right, I had missed that the = was gone in Lv's suggestion, sorry.


On Tue, Jun 27, 2017 at 05:31:52PM +0200, Rafael J. Wysocki wrote:
> So I will drop this commit for now and I will add it again along with
> the next release material to avoid triggering regressions in the
> meantime.

Okay, thanks for the speedy response Rafael and Robert!

Lukas