Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1233104 > unrolled thread

Re: [PATCH v4 0/5] Provide better MADT subtable sanity checks

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v4 0/5] Provide better MADT subtable sanity checks "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-09-26 01:10 +0200
    Re: [PATCH v4 0/5] Provide better MADT subtable sanity checks Al Stone <ahs3@redhat.com> - 2015-09-28 22:20 +0200
      Re: [PATCH v4 0/5] Provide better MADT subtable sanity checks "Rafael J. Wysocki" <rafael@kernel.org> - 2015-09-28 23:20 +0200

#1233104 — Re: [PATCH v4 0/5] Provide better MADT subtable sanity checks

On Wednesday, September 16, 2015 05:26:40 PM Al Stone wrote:
> Currently, the BAD_MADT_ENTRY macro is used to do a very simple sanity
> check on the various subtables that are defined for the MADT.  The check
> compares the size of the subtable data structure as defined by ACPICA to
> the length entry in the subtable.  If they are not the same, the assumption
> is that the subtable is incorrect.
> 
> Over time, the ACPI spec has allowed for MADT subtables where this can
> never be true (the local SAPIC subtable, for example).  Or, more recently,
> the spec has accumulated some minor flaws where there are three possible 
> sizes for a subtable, all of which are valid, but only for specific versions
> of the spec (the GICC subtable).  In both cases, BAD_MADT_ENTRY reports these
> subtables as bad when they are not.  In order to retain some sanity check
> on the MADT subtables, we now have to special case these subtables.  Of
> necessity, these special cases have ended up in arch-dependent code (arm64)
> or an arch has simply decided to forgo the check (ia64).
> 
> This patch set replaces the BAD_MADT_ENTRY macro with a function called
> bad_madt_entry().  This function uses a data set of details about the
> subtables to provide more sanity checking than before:
> 
> 	-- is the subtable legal for the version given in the FADT?
> 
> 	-- is the subtable legal for the revision of the MADT in use?
> 
> 	-- is the subtable of the proper length (including checking
> 	   on the one variable length subtable that is currently ignored),
> 	   given the FADT version and the MADT revision?
> 
> Further, this patch set adds in the call to bad_madt_entry() from the 
> acpi_table_parse_madt() function, allowing it to be used consistently
> by all architectures, for all subtables, and removing the need for each
> of the subtable traversal callback functions to use BAD_MADT_ENTRY.
> 
> In theory, as the ACPI specification changes, we would only have to add
> additional information to the data set describing the MADT subtables in
> order to continue providing sanity checks, even when new subtables are
> added.
> 
> These patches have been tested on an APM Mustang (arm64) and are known to
> work there.  They have also been cross-compiled for x86 and ia64 with no
> known failures.
> 
> Changes for v4:
>    -- Remove extraneous white space change (Graeme Gregory)
>    -- acpi_parse_entries() changes also needed a check to make sure that
>       only MADT entries used bad_madt_entry() (Sudeep Holla)
>    -- inadvertent use of 01day build noted that bad_madt_entry() can be
>       static, so added it (Sudeep Holla, Fengguang Wu)
> 
> Changes for v3:
>    -- Reviewed-and-tested-by from Sudeep Holla for arm64 parts
>    -- Clearer language in error messages (Graeme Gregory, Timur Tabi)
>    -- Double checked that inserting call to bad_madt_entry() into the
>       function acpi_parse_entries() does not impact current behavior
>       (Sudeep Holla)
>    
> Changes for v2:
>    -- Acked-by on 2/5 from Marc Zyngier and Catalin Marinas for ARM
>    -- Correct faulty end of loop test found by Timur Tabi
> 
> 
> Al Stone (5):
>   ACPI: add in a bad_madt_entry() function to eventually replace the
>     macro
>   ACPI / ARM64: remove usage of BAD_MADT_ENTRY/BAD_MADT_GICC_ENTRY
>   ACPI / IA64: remove usage of BAD_MADT_ENTRY
>   ACPI / X86: remove usage of BAD_MADT_ENTRY
>   ACPI: remove definition of BAD_MADT_ENTRY macro

I've queued this up for v4.4, but I had to rebase it on top of some previous
changes in my linux-next branch.

Can you please look at my bleeding-edge branch and see if the result of the
rebase is as intended?  In particular, I'm not sure if we really need to return
-EINVAL from acpi_parse_entries_array() when we find a bad MADT entry or it
will be sufficient to simply go to the next entry in that case?

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1234417

On 09/25/2015 05:29 PM, Rafael J. Wysocki wrote:
> On Wednesday, September 16, 2015 05:26:40 PM Al Stone wrote:
>> Currently, the BAD_MADT_ENTRY macro is used to do a very simple sanity
>> check on the various subtables that are defined for the MADT.  The check
>> compares the size of the subtable data structure as defined by ACPICA to
>> the length entry in the subtable.  If they are not the same, the assumption
>> is that the subtable is incorrect.
>>
>> Over time, the ACPI spec has allowed for MADT subtables where this can
>> never be true (the local SAPIC subtable, for example).  Or, more recently,
>> the spec has accumulated some minor flaws where there are three possible 
>> sizes for a subtable, all of which are valid, but only for specific versions
>> of the spec (the GICC subtable).  In both cases, BAD_MADT_ENTRY reports these
>> subtables as bad when they are not.  In order to retain some sanity check
>> on the MADT subtables, we now have to special case these subtables.  Of
>> necessity, these special cases have ended up in arch-dependent code (arm64)
>> or an arch has simply decided to forgo the check (ia64).
>>
>> This patch set replaces the BAD_MADT_ENTRY macro with a function called
>> bad_madt_entry().  This function uses a data set of details about the
>> subtables to provide more sanity checking than before:
>>
>> 	-- is the subtable legal for the version given in the FADT?
>>
>> 	-- is the subtable legal for the revision of the MADT in use?
>>
>> 	-- is the subtable of the proper length (including checking
>> 	   on the one variable length subtable that is currently ignored),
>> 	   given the FADT version and the MADT revision?
>>
>> Further, this patch set adds in the call to bad_madt_entry() from the 
>> acpi_table_parse_madt() function, allowing it to be used consistently
>> by all architectures, for all subtables, and removing the need for each
>> of the subtable traversal callback functions to use BAD_MADT_ENTRY.
>>
>> In theory, as the ACPI specification changes, we would only have to add
>> additional information to the data set describing the MADT subtables in
>> order to continue providing sanity checks, even when new subtables are
>> added.
>>
>> These patches have been tested on an APM Mustang (arm64) and are known to
>> work there.  They have also been cross-compiled for x86 and ia64 with no
>> known failures.
>>
>> Changes for v4:
>>    -- Remove extraneous white space change (Graeme Gregory)
>>    -- acpi_parse_entries() changes also needed a check to make sure that
>>       only MADT entries used bad_madt_entry() (Sudeep Holla)
>>    -- inadvertent use of 01day build noted that bad_madt_entry() can be
>>       static, so added it (Sudeep Holla, Fengguang Wu)
>>
>> Changes for v3:
>>    -- Reviewed-and-tested-by from Sudeep Holla for arm64 parts
>>    -- Clearer language in error messages (Graeme Gregory, Timur Tabi)
>>    -- Double checked that inserting call to bad_madt_entry() into the
>>       function acpi_parse_entries() does not impact current behavior
>>       (Sudeep Holla)
>>    
>> Changes for v2:
>>    -- Acked-by on 2/5 from Marc Zyngier and Catalin Marinas for ARM
>>    -- Correct faulty end of loop test found by Timur Tabi
>>
>>
>> Al Stone (5):
>>   ACPI: add in a bad_madt_entry() function to eventually replace the
>>     macro
>>   ACPI / ARM64: remove usage of BAD_MADT_ENTRY/BAD_MADT_GICC_ENTRY
>>   ACPI / IA64: remove usage of BAD_MADT_ENTRY
>>   ACPI / X86: remove usage of BAD_MADT_ENTRY
>>   ACPI: remove definition of BAD_MADT_ENTRY macro
> 
> I've queued this up for v4.4, but I had to rebase it on top of some previous
> changes in my linux-next branch.
> 
> Can you please look at my bleeding-edge branch and see if the result of the
> rebase is as intended?  In particular, I'm not sure if we really need to return
> -EINVAL from acpi_parse_entries_array() when we find a bad MADT entry or it
> will be sufficient to simply go to the next entry in that case?
> 
> Thanks,
> Rafael

I see there being two options: (1) return -EINVAL and indicate that the tables
are incorrect, or (2) print a warning (or something more aggressive?), go to
the next entry, and hope for the best with the remainder of the MADT subtables.
The former is consistent with past behavior, I think, and the latter seems to
me a bit of a gamble.  So, my vote is for (1), the current method; what are you
thinking these days?

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1234476

On Mon, Sep 28, 2015 at 10:17 PM, Al Stone <ahs3@redhat.com> wrote:
> On 09/25/2015 05:29 PM, Rafael J. Wysocki wrote:
>> On Wednesday, September 16, 2015 05:26:40 PM Al Stone wrote:

[cut]

>> In particular, I'm not sure if we really need to return
>> -EINVAL from acpi_parse_entries_array() when we find a bad MADT entry or it
>> will be sufficient to simply go to the next entry in that case?
>>
>> Thanks,
>> Rafael
>
> I see there being two options: (1) return -EINVAL and indicate that the tables
> are incorrect, or (2) print a warning (or something more aggressive?), go to
> the next entry, and hope for the best with the remainder of the MADT subtables.
> The former is consistent with past behavior, I think, and the latter seems to
> me a bit of a gamble.  So, my vote is for (1), the current method; what are you
> thinking these days?

I would be for preserving the past behavior.

I'm a bit concerned that the new checks may trigger on systems where
the old ones didn't, but that is a separete problem.

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web