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

Groups > linux.kernel > #1435637 > unrolled thread

[PATCH 3/3] ACPI: fix acpi_parse_entries_array() so it reports overflow correctly

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

  [PATCH 3/3] ACPI: fix acpi_parse_entries_array() so it reports overflow correctly Al Stone <ahs3@redhat.com> - 2016-07-01 23:30 +0200
    Re: [PATCH 3/3] ACPI: fix acpi_parse_entries_array() so it reports  overflow correctly Al Stone <ahs3@redhat.com> - 2016-07-01 23:50 +0200
      Re: [PATCH 3/3] ACPI: fix acpi_parse_entries_array() so it reports  overflow correctly "Rafael J. Wysocki" <rafael@kernel.org> - 2016-07-02 00:00 +0200
        Re: [PATCH 3/3] ACPI: fix acpi_parse_entries_array() so it reports  overflow correctly Al Stone <ahs3@redhat.com> - 2016-07-02 00:40 +0200
          Re: [PATCH 3/3] ACPI: fix acpi_parse_entries_array() so it reports  overflow correctly "Rafael J. Wysocki" <rafael@kernel.org> - 2016-07-02 00:50 +0200
    Re: [PATCH 3/3] ACPI: fix acpi_parse_entries_array() so it reports  overflow correctly "Rafael J. Wysocki" <rafael@kernel.org> - 2016-07-01 23:50 +0200

#1435637 — [PATCH 3/3] ACPI: fix acpi_parse_entries_array() so it reports overflow correctly

The function acpi_parse_entries_array() has a limiting parameter,
max_entries, which tells the function to stop looking at subtables
once that limit has been reached.  Further, if the limit is reached,
it is reported.  However, the logic is incorrect in that the loop
to examine all subtables will always stop when exactly max_entries
have been found, regardless of whether or not there are still subtables
to examine, and it will always report that zero subtables have been
ignored.  This change allows the loop to continue to look at all
subtables and count all the ones of interest; if we have already
reached the number of max_entries, though, we will not invoke the
callback functions.  If the max_entries limit has been exceeded,
report on that, as before, but more accurately, listing how many
subtables of interest there are in total (as was meant), and how
many entries each subtable type occupied.

Signed-off-by: Al Stone <ahs3@redhat.com>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
---
 drivers/acpi/tables.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index 76c07ed..227312d 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -272,12 +272,11 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
 
 	while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) <
 	       table_end) {
-		if (max_entries && count >= max_entries)
-			break;
-
 		for (i = 0; i < proc_num; i++) {
 			if (entry->type != proc[i].id)
 				continue;
+			if (max_entries && count >= max_entries)
+				break;
 			if (!proc[i].handler ||
 			     proc[i].handler(entry, table_end)) {
 				errs_found++;
@@ -304,8 +303,11 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
 	}
 
 	if (max_entries && count > max_entries) {
-		pr_warn("[%4.4s:0x%02x] ignored %i entries of %i found\n",
-			id, proc->id, count - max_entries, count);
+		pr_warn("[%4.4s] ignored %i entries of %i found\n",
+			id, count - max_entries, count);
+		for (i = 0; i < proc_num; i++)
+			pr_warn("[%4.4s] subtable 0x%02x used %i entries\n",
+				id, proc[i].id, proc[i].count);
 	}
 
 	return (errs_found) ? -EINVAL: count;
-- 
2.7.4

[toc] | [next] | [standalone]

#1435650 — Re: [PATCH 3/3] ACPI: fix acpi_parse_entries_array() so it reports overflow correctly

On 07/01/2016 03:40 PM, Rafael J. Wysocki wrote:
> On Fri, Jul 1, 2016 at 11:21 PM, Al Stone <ahs3@redhat.com> wrote:
>> The function acpi_parse_entries_array() has a limiting parameter,
>> max_entries, which tells the function to stop looking at subtables
>> once that limit has been reached.  Further, if the limit is reached,
>> it is reported.  However, the logic is incorrect in that the loop
>> to examine all subtables will always stop when exactly max_entries
>> have been found, regardless of whether or not there are still subtables
>> to examine, and it will always report that zero subtables have been
>> ignored.  This change allows the loop to continue to look at all
>> subtables and count all the ones of interest; if we have already
>> reached the number of max_entries, though, we will not invoke the
>> callback functions.  If the max_entries limit has been exceeded,
>> report on that, as before, but more accurately, listing how many
>> subtables of interest there are in total (as was meant), and how
>> many entries each subtable type occupied.
> 
> The problem appears to be that, if max_entries has been reached, it
> prints "ignored 0", although it should count all of the entries in
> that case too in principle.  Do I think correctly?
> 

Exactly.  That's how I interpreted the comments.  And it fit what I
needed it to do if the comments were correct.

Of course, it could be the code was correct and the comments were
wrong :).  I preferred not to think that.

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

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

#1435659 — Re: [PATCH 3/3] ACPI: fix acpi_parse_entries_array() so it reports overflow correctly

On Fri, Jul 1, 2016 at 11:44 PM, Al Stone <ahs3@redhat.com> wrote:
> On 07/01/2016 03:40 PM, Rafael J. Wysocki wrote:
>> On Fri, Jul 1, 2016 at 11:21 PM, Al Stone <ahs3@redhat.com> wrote:
>>> The function acpi_parse_entries_array() has a limiting parameter,
>>> max_entries, which tells the function to stop looking at subtables
>>> once that limit has been reached.  Further, if the limit is reached,
>>> it is reported.  However, the logic is incorrect in that the loop
>>> to examine all subtables will always stop when exactly max_entries
>>> have been found, regardless of whether or not there are still subtables
>>> to examine, and it will always report that zero subtables have been
>>> ignored.  This change allows the loop to continue to look at all
>>> subtables and count all the ones of interest; if we have already
>>> reached the number of max_entries, though, we will not invoke the
>>> callback functions.  If the max_entries limit has been exceeded,
>>> report on that, as before, but more accurately, listing how many
>>> subtables of interest there are in total (as was meant), and how
>>> many entries each subtable type occupied.
>>
>> The problem appears to be that, if max_entries has been reached, it
>> prints "ignored 0", although it should count all of the entries in
>> that case too in principle.  Do I think correctly?
>>
>
> Exactly.  That's how I interpreted the comments.  And it fit what I
> needed it to do if the comments were correct.
>
> Of course, it could be the code was correct and the comments were
> wrong :).  I preferred not to think that.

I guess whoever implemented this function thought that the overhead
for counting stuff was not useful in case max_entries had been
reached.  I'm not really sure I disagree with that. :-)

I agree that printing "ignored 0" in that case is misleading, but the
fix might be to simply avoid printing how many entries have been
ignored then.  Maybe it will suffice to print how many entries have
been found and what the limit was?

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

#1435672 — Re: [PATCH 3/3] ACPI: fix acpi_parse_entries_array() so it reports overflow correctly

On 07/01/2016 03:54 PM, Rafael J. Wysocki wrote:
> On Fri, Jul 1, 2016 at 11:44 PM, Al Stone <ahs3@redhat.com> wrote:
>> On 07/01/2016 03:40 PM, Rafael J. Wysocki wrote:
>>> On Fri, Jul 1, 2016 at 11:21 PM, Al Stone <ahs3@redhat.com> wrote:
>>>> The function acpi_parse_entries_array() has a limiting parameter,
>>>> max_entries, which tells the function to stop looking at subtables
>>>> once that limit has been reached.  Further, if the limit is reached,
>>>> it is reported.  However, the logic is incorrect in that the loop
>>>> to examine all subtables will always stop when exactly max_entries
>>>> have been found, regardless of whether or not there are still subtables
>>>> to examine, and it will always report that zero subtables have been
>>>> ignored.  This change allows the loop to continue to look at all
>>>> subtables and count all the ones of interest; if we have already
>>>> reached the number of max_entries, though, we will not invoke the
>>>> callback functions.  If the max_entries limit has been exceeded,
>>>> report on that, as before, but more accurately, listing how many
>>>> subtables of interest there are in total (as was meant), and how
>>>> many entries each subtable type occupied.
>>>
>>> The problem appears to be that, if max_entries has been reached, it
>>> prints "ignored 0", although it should count all of the entries in
>>> that case too in principle.  Do I think correctly?
>>>
>>
>> Exactly.  That's how I interpreted the comments.  And it fit what I
>> needed it to do if the comments were correct.
>>
>> Of course, it could be the code was correct and the comments were
>> wrong :).  I preferred not to think that.
> 
> I guess whoever implemented this function thought that the overhead
> for counting stuff was not useful in case max_entries had been
> reached.  I'm not really sure I disagree with that. :-)

I'm not sure I disagree, either :).

> I agree that printing "ignored 0" in that case is misleading, but the
> fix might be to simply avoid printing how many entries have been
> ignored then.  Maybe it will suffice to print how many entries have
> been found and what the limit was?

That could work.

Unless I've misunderstood the code, though, the situation that seemed likely
to me is, for example, to suppose that the first five subtables out of 20 are
of a single type and cause my max_entries limit to be reached.  If I have three
callbacks, I'd end up with two other callback functions that would never get
called, even if some of the remaining 15 subtables are pertinent and could help
get the boot process further along.

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

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

#1435674 — Re: [PATCH 3/3] ACPI: fix acpi_parse_entries_array() so it reports overflow correctly

On Sat, Jul 2, 2016 at 12:38 AM, Al Stone <ahs3@redhat.com> wrote:
> On 07/01/2016 03:54 PM, Rafael J. Wysocki wrote:
>> On Fri, Jul 1, 2016 at 11:44 PM, Al Stone <ahs3@redhat.com> wrote:
>>> On 07/01/2016 03:40 PM, Rafael J. Wysocki wrote:
>>>> On Fri, Jul 1, 2016 at 11:21 PM, Al Stone <ahs3@redhat.com> wrote:
>>>>> The function acpi_parse_entries_array() has a limiting parameter,
>>>>> max_entries, which tells the function to stop looking at subtables
>>>>> once that limit has been reached.  Further, if the limit is reached,
>>>>> it is reported.  However, the logic is incorrect in that the loop
>>>>> to examine all subtables will always stop when exactly max_entries
>>>>> have been found, regardless of whether or not there are still subtables
>>>>> to examine, and it will always report that zero subtables have been
>>>>> ignored.  This change allows the loop to continue to look at all
>>>>> subtables and count all the ones of interest; if we have already
>>>>> reached the number of max_entries, though, we will not invoke the
>>>>> callback functions.  If the max_entries limit has been exceeded,
>>>>> report on that, as before, but more accurately, listing how many
>>>>> subtables of interest there are in total (as was meant), and how
>>>>> many entries each subtable type occupied.
>>>>
>>>> The problem appears to be that, if max_entries has been reached, it
>>>> prints "ignored 0", although it should count all of the entries in
>>>> that case too in principle.  Do I think correctly?
>>>>
>>>
>>> Exactly.  That's how I interpreted the comments.  And it fit what I
>>> needed it to do if the comments were correct.
>>>
>>> Of course, it could be the code was correct and the comments were
>>> wrong :).  I preferred not to think that.
>>
>> I guess whoever implemented this function thought that the overhead
>> for counting stuff was not useful in case max_entries had been
>> reached.  I'm not really sure I disagree with that. :-)
>
> I'm not sure I disagree, either :).
>
>> I agree that printing "ignored 0" in that case is misleading, but the
>> fix might be to simply avoid printing how many entries have been
>> ignored then.  Maybe it will suffice to print how many entries have
>> been found and what the limit was?
>
> That could work.
>
> Unless I've misunderstood the code, though, the situation that seemed likely
> to me is, for example, to suppose that the first five subtables out of 20 are
> of a single type and cause my max_entries limit to be reached.  If I have three
> callbacks, I'd end up with two other callback functions that would never get
> called, even if some of the remaining 15 subtables are pertinent and could help
> get the boot process further along.

That depends on what max_entries is used for which I don't recall ATM.

So before making changes here, I'd recommend looking for code that
uses max_entries in non-trivial ways and finding the reasons why it is
used.

Maybe it is just not really needed or maybe it should just be replaced
with something else.  In any case, without any research in that
direction, I'd rather do the simplest fix possible.

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

#1435654 — Re: [PATCH 3/3] ACPI: fix acpi_parse_entries_array() so it reports overflow correctly

On Fri, Jul 1, 2016 at 11:21 PM, Al Stone <ahs3@redhat.com> wrote:
> The function acpi_parse_entries_array() has a limiting parameter,
> max_entries, which tells the function to stop looking at subtables
> once that limit has been reached.  Further, if the limit is reached,
> it is reported.  However, the logic is incorrect in that the loop
> to examine all subtables will always stop when exactly max_entries
> have been found, regardless of whether or not there are still subtables
> to examine, and it will always report that zero subtables have been
> ignored.  This change allows the loop to continue to look at all
> subtables and count all the ones of interest; if we have already
> reached the number of max_entries, though, we will not invoke the
> callback functions.  If the max_entries limit has been exceeded,
> report on that, as before, but more accurately, listing how many
> subtables of interest there are in total (as was meant), and how
> many entries each subtable type occupied.

The problem appears to be that, if max_entries has been reached, it
prints "ignored 0", although it should count all of the entries in
that case too in principle.  Do I think correctly?

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web