[PATCH] powerpc/pseries: fix of_read_drc_info_cell() to point at next record

[PATCH] powerpc/pseries: fix of_read_drc_info_cell() to point at next record

[Tyrel Datwyler]

The expectation is that when calling of_read_drc_info_cell()
repeatedly to parse multiple drc-info records that the in/out curval
parameter points at the start of the next record on return. However,
the current behavior has curval still pointing at the final value of
the record just parsed. The result of which is that if the
ibm,drc-info property contains multiple properties the parsed value
of the drc_type for any record after the first has the power_domain
value of the previous record appended to the type string.

Ex: observed the following 0xffffffff prepended to PHB

[   69.485037] drc-info: type: \xff\xff\xff\xffPHB, prefix: PHB , index_start: 0x20000001
[   69.485038] drc-info: suffix_start: 1, sequential_elems: 3072, sequential_inc: 1
[   69.485038] drc-info: power-domain: 0xffffffff, last_index: 0x20000c00

Fix by incrementing curval past the power_domain value to point at
drc_type string of next record.

Fixes: a29396653b8bf ("pseries/drc-info: Search DRC properties for CPU indexes")
Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
---
 arch/powerpc/platforms/pseries/of_helpers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/pseries/of_helpers.c b/arch/powerpc/platforms/pseries/of_helpers.c
index 66dfd8256712..23241c71ef37 100644
--- a/arch/powerpc/platforms/pseries/of_helpers.c
+++ b/arch/powerpc/platforms/pseries/of_helpers.c
@@ -88,7 +88,7 @@ int of_read_drc_info_cell(struct property **prop, const __be32 **curval,
 		return -EINVAL;
 
 	/* Should now know end of current entry */
-	(*curval) = (void *)p2;
+	(*curval) = (void *)(++p2);
 	data->last_drc_index = data->drc_index_start +
 		((data->num_sequential_elems - 1) * data->sequential_inc);
 
-- 
2.16.4

Re: [PATCH] powerpc/pseries: fix of_read_drc_info_cell() to point at next record

[Nathan Lynch]

Tyrel Datwyler <tyreld@linux.ibm.com> writes:
> The expectation is that when calling of_read_drc_info_cell()
> repeatedly to parse multiple drc-info records that the in/out curval
> parameter points at the start of the next record on return. However,
> the current behavior has curval still pointing at the final value of
> the record just parsed. The result of which is that if the
> ibm,drc-info property contains multiple properties the parsed value
> of the drc_type for any record after the first has the power_domain
> value of the previous record appended to the type string.
>
> Ex: observed the following 0xffffffff prepended to PHB
>
> [   69.485037] drc-info: type: \xff\xff\xff\xffPHB, prefix: PHB , index_start: 0x20000001
> [   69.485038] drc-info: suffix_start: 1, sequential_elems: 3072, sequential_inc: 1
> [   69.485038] drc-info: power-domain: 0xffffffff, last_index: 0x20000c00
>
> Fix by incrementing curval past the power_domain value to point at
> drc_type string of next record.
>
> Fixes: a29396653b8bf ("pseries/drc-info: Search DRC properties for CPU indexes")

I have a different commit hash for that:
e83636ac3334 pseries/drc-info: Search DRC properties for CPU indexes

> Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>

Otherwise:
Acked-by: Nathan Lynch <nathanl@linux.ibm.com>

Re: [PATCH] powerpc/pseries: fix of_read_drc_info_cell() to point at next record

[Tyrel Datwyler]

On 3/10/20 10:25 AM, Nathan Lynch wrote:
> Tyrel Datwyler <tyreld@linux.ibm.com> writes:
>> The expectation is that when calling of_read_drc_info_cell()
>> repeatedly to parse multiple drc-info records that the in/out curval
>> parameter points at the start of the next record on return. However,
>> the current behavior has curval still pointing at the final value of
>> the record just parsed. The result of which is that if the
>> ibm,drc-info property contains multiple properties the parsed value
>> of the drc_type for any record after the first has the power_domain
>> value of the previous record appended to the type string.
>>
>> Ex: observed the following 0xffffffff prepended to PHB
>>
>> [   69.485037] drc-info: type: \xff\xff\xff\xffPHB, prefix: PHB , index_start: 0x20000001
>> [   69.485038] drc-info: suffix_start: 1, sequential_elems: 3072, sequential_inc: 1
>> [   69.485038] drc-info: power-domain: 0xffffffff, last_index: 0x20000c00
>>
>> Fix by incrementing curval past the power_domain value to point at
>> drc_type string of next record.
>>
>> Fixes: a29396653b8bf ("pseries/drc-info: Search DRC properties for CPU indexes")
> 
> I have a different commit hash for that:
> e83636ac3334 pseries/drc-info: Search DRC properties for CPU indexes

Oof, looks like I grabbed the commit hash from the SLES 15 SP1 branch. You have
the correct upstream commit.

Fixes: e83636ac3334 ("pseries/drc-info: Search DRC properties for CPU indexes")

Michael, let me know if you want me to resubmit, or if you will fixup the Fixes
tag on your end?

-Tyrel

> 
>> Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
> 
> Otherwise:
> Acked-by: Nathan Lynch <nathanl@linux.ibm.com>
> 

Re: [PATCH] powerpc/pseries: fix of_read_drc_info_cell() to point at next record

[Michael Ellerman]

Tyrel Datwyler <tyreld@linux.ibm.com> writes:
> On 3/10/20 10:25 AM, Nathan Lynch wrote:
>> Tyrel Datwyler <tyreld@linux.ibm.com> writes:
>>> The expectation is that when calling of_read_drc_info_cell()
>>> repeatedly to parse multiple drc-info records that the in/out curval
>>> parameter points at the start of the next record on return. However,
>>> the current behavior has curval still pointing at the final value of
>>> the record just parsed. The result of which is that if the
>>> ibm,drc-info property contains multiple properties the parsed value
>>> of the drc_type for any record after the first has the power_domain
>>> value of the previous record appended to the type string.
>>>
>>> Ex: observed the following 0xffffffff prepended to PHB
>>>
>>> [   69.485037] drc-info: type: \xff\xff\xff\xffPHB, prefix: PHB , index_start: 0x20000001
>>> [   69.485038] drc-info: suffix_start: 1, sequential_elems: 3072, sequential_inc: 1
>>> [   69.485038] drc-info: power-domain: 0xffffffff, last_index: 0x20000c00
>>>
>>> Fix by incrementing curval past the power_domain value to point at
>>> drc_type string of next record.
>>>
>>> Fixes: a29396653b8bf ("pseries/drc-info: Search DRC properties for CPU indexes")
>> 
>> I have a different commit hash for that:
>> e83636ac3334 pseries/drc-info: Search DRC properties for CPU indexes
>
> Oof, looks like I grabbed the commit hash from the SLES 15 SP1 branch. You have
> the correct upstream commit.
>
> Fixes: e83636ac3334 ("pseries/drc-info: Search DRC properties for CPU indexes")
>
> Michael, let me know if you want me to resubmit, or if you will fixup the Fixes
> tag on your end?

I can update the Fixes tag.

What's the practical effect of this bug? It seems like it should break
all the hotplug stuff, but presumably it doesn't in practice for some
reason?

It would also be *really* nice if we had some unit tests for this
parsing code, it's demonstrably very bug prone.

cheers

Re: [PATCH] powerpc/pseries: fix of_read_drc_info_cell() to point at next record

[Nathan Lynch]

Michael Ellerman <mpe@ellerman.id.au> writes:
>
> It would also be *really* nice if we had some unit tests for this
> parsing code, it's demonstrably very bug prone.

Can you say more about what form unit tests could take? Like some self
tests that run at boot time, or is there a way to run the code in a user
space test harness?

Re: [PATCH] powerpc/pseries: fix of_read_drc_info_cell() to point at next record

[Tyrel Datwyler]

On 3/11/20 10:43 PM, Michael Ellerman wrote:
> Tyrel Datwyler <tyreld@linux.ibm.com> writes:
>> On 3/10/20 10:25 AM, Nathan Lynch wrote:
>>> Tyrel Datwyler <tyreld@linux.ibm.com> writes:
>>>> The expectation is that when calling of_read_drc_info_cell()
>>>> repeatedly to parse multiple drc-info records that the in/out curval
>>>> parameter points at the start of the next record on return. However,
>>>> the current behavior has curval still pointing at the final value of
>>>> the record just parsed. The result of which is that if the
>>>> ibm,drc-info property contains multiple properties the parsed value
>>>> of the drc_type for any record after the first has the power_domain
>>>> value of the previous record appended to the type string.
>>>>
>>>> Ex: observed the following 0xffffffff prepended to PHB
>>>>
>>>> [   69.485037] drc-info: type: \xff\xff\xff\xffPHB, prefix: PHB , index_start: 0x20000001
>>>> [   69.485038] drc-info: suffix_start: 1, sequential_elems: 3072, sequential_inc: 1
>>>> [   69.485038] drc-info: power-domain: 0xffffffff, last_index: 0x20000c00
>>>>
>>>> Fix by incrementing curval past the power_domain value to point at
>>>> drc_type string of next record.
>>>>
>>>> Fixes: a29396653b8bf ("pseries/drc-info: Search DRC properties for CPU indexes")
>>>
>>> I have a different commit hash for that:
>>> e83636ac3334 pseries/drc-info: Search DRC properties for CPU indexes
>>
>> Oof, looks like I grabbed the commit hash from the SLES 15 SP1 branch. You have
>> the correct upstream commit.
>>
>> Fixes: e83636ac3334 ("pseries/drc-info: Search DRC properties for CPU indexes")
>>
>> Michael, let me know if you want me to resubmit, or if you will fixup the Fixes
>> tag on your end?
> 
> I can update the Fixes tag.
> 
> What's the practical effect of this bug? It seems like it should break
> all the hotplug stuff, but presumably it doesn't in practice for some
> reason?
> 
> It would also be *really* nice if we had some unit tests for this
> parsing code, it's demonstrably very bug prone.
> 
> cheers
> 

In practice PHBs are the only type of connector in the ibm,drc-info property
that has multiple records. So, it breaks PHB hotplug, but by chance not pci,
cpu, slot, or mem because they happen to only ever be a single record.

I have a follow up patch to at least add some pr_debug at the end of this
parsing function to dump each record as its read.

-Tyrel

Re: [PATCH] powerpc/pseries: fix of_read_drc_info_cell() to point at next record

[Michael Ellerman]

Tyrel Datwyler <tyreld@linux.ibm.com> writes:
> On 3/11/20 10:43 PM, Michael Ellerman wrote:
>> Tyrel Datwyler <tyreld@linux.ibm.com> writes:
>>> On 3/10/20 10:25 AM, Nathan Lynch wrote:
>>>> Tyrel Datwyler <tyreld@linux.ibm.com> writes:
>>>>> The expectation is that when calling of_read_drc_info_cell()
>>>>> repeatedly to parse multiple drc-info records that the in/out curval
>>>>> parameter points at the start of the next record on return. However,
>>>>> the current behavior has curval still pointing at the final value of
>>>>> the record just parsed. The result of which is that if the
>>>>> ibm,drc-info property contains multiple properties the parsed value
>>>>> of the drc_type for any record after the first has the power_domain
>>>>> value of the previous record appended to the type string.
>>>>>
>>>>> Ex: observed the following 0xffffffff prepended to PHB
>>>>>
>>>>> [   69.485037] drc-info: type: \xff\xff\xff\xffPHB, prefix: PHB , index_start: 0x20000001
>>>>> [   69.485038] drc-info: suffix_start: 1, sequential_elems: 3072, sequential_inc: 1
>>>>> [   69.485038] drc-info: power-domain: 0xffffffff, last_index: 0x20000c00
>>>>>
>>>>> Fix by incrementing curval past the power_domain value to point at
>>>>> drc_type string of next record.
>>>>>
>>>>> Fixes: a29396653b8bf ("pseries/drc-info: Search DRC properties for CPU indexes")
>>>>
>>>> I have a different commit hash for that:
>>>> e83636ac3334 pseries/drc-info: Search DRC properties for CPU indexes
>>>
>>> Oof, looks like I grabbed the commit hash from the SLES 15 SP1 branch. You have
>>> the correct upstream commit.
>>>
>>> Fixes: e83636ac3334 ("pseries/drc-info: Search DRC properties for CPU indexes")
>>>
>>> Michael, let me know if you want me to resubmit, or if you will fixup the Fixes
>>> tag on your end?
>> 
>> I can update the Fixes tag.
>> 
>> What's the practical effect of this bug? It seems like it should break
>> all the hotplug stuff, but presumably it doesn't in practice for some
>> reason?
>> 
>> It would also be *really* nice if we had some unit tests for this
>> parsing code, it's demonstrably very bug prone.
>
> In practice PHBs are the only type of connector in the ibm,drc-info property
> that has multiple records. So, it breaks PHB hotplug, but by chance not pci,
> cpu, slot, or mem because they happen to only ever be a single record.

Thanks. I folded that into the change log.

cheers

Re: [PATCH] powerpc/pseries: fix of_read_drc_info_cell() to point at next record

[Michael Ellerman]

Nathan Lynch <nathanl@linux.ibm.com> writes:
> Michael Ellerman <mpe@ellerman.id.au> writes:
>>
>> It would also be *really* nice if we had some unit tests for this
>> parsing code, it's demonstrably very bug prone.
>
> Can you say more about what form unit tests could take? Like some self
> tests that run at boot time, or is there a way to run the code in a user
> space test harness?

It depends.

A userspace selftest is ideal as it's the easiest option for people to
run, ie. they just have to build a user program. There's also CI systems
setup to run the kernel selftests automatically.

See eg. tools/testing/selftests/powerpc/vphn/vphn.c

We also have boot time tests, they are good for code that we want to
test in the kernel proper, or that are hard to extract into a userspace
program. Most of those are configurable, so testing them requires
someone to enable the appropriate CONFIG_FOO and build & boot that
kernel. That reduces coverage obviously.

See eg. arch/powerpc/lib/code-patching.c

These days there's also kunit, which is a "proper" way to do kernel unit
tests. They get built into the kernel but then there's some support for
running those like a user program using UML on x86. On powerpc we'd have
to boot the kunit enabled kernel on hardware or in qemu to exercise the
tests. So they're essentially just boot time tests but with some nicer
infrastructure vs doing it all by hand like we used to.

In this case the actual function we want to test could be trivially
built into a userspace program, but the underlying device tree helpers
probably can't be.

eg. drivers/of/property.c is quite large and contains quite a few
things, we'd need to shim quite a bit to get that building in userspace
I suspect, which then becomes a maintenance burden.

So this is probably a good candidate for a kunit test, though I haven't
personally written/converted any tests to kunit so I can't say exactly
how easy that is.

cheers

Re: [PATCH] powerpc/pseries: fix of_read_drc_info_cell() to point at next record

[Michael Ellerman]

On Sat, 2020-03-07 at 02:45:47 UTC, Tyrel Datwyler wrote:
> The expectation is that when calling of_read_drc_info_cell()
> repeatedly to parse multiple drc-info records that the in/out curval
> parameter points at the start of the next record on return. However,
> the current behavior has curval still pointing at the final value of
> the record just parsed. The result of which is that if the
> ibm,drc-info property contains multiple properties the parsed value
> of the drc_type for any record after the first has the power_domain
> value of the previous record appended to the type string.
> 
> Ex: observed the following 0xffffffff prepended to PHB
> 
> [   69.485037] drc-info: type: \xff\xff\xff\xffPHB, prefix: PHB , index_start: 0x20000001
> [   69.485038] drc-info: suffix_start: 1, sequential_elems: 3072, sequential_inc: 1
> [   69.485038] drc-info: power-domain: 0xffffffff, last_index: 0x20000c00
> 
> Fix by incrementing curval past the power_domain value to point at
> drc_type string of next record.
> 
> Fixes: a29396653b8bf ("pseries/drc-info: Search DRC properties for CPU indexes")
> Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/c5e76fa05b2df519b9f08571cc57e623c1569faa

cheers