diff mbox series

dt: Set new property length in dt_resize_property()

Message ID 20200619025752.200662-1-bauerman@linux.ibm.com
State Accepted
Headers show
Series dt: Set new property length in dt_resize_property() | expand

Checks

Context Check Description
snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot-dco success Signed-off-by present
snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot success Test snowpatch/job/snowpatch-skiboot on branch master
snowpatch_ozlabs/apply_patch success Successfully applied on branch master (fe70fbb78d33abea788a3221bc409a7c50c019c3)

Commit Message

Thiago Jung Bauermann June 19, 2020, 2:57 a.m. UTC
All callers of dt_resize_property() need to set the new property length
after calling it. append_chip_id() wasn't doing it, which caused this
assert when booting my machine:

[  136.387213258,3] Unable to use memory range 0 from MSAREA 0
[  136.387356677,3] Unable to use memory range 0 from MSAREA 2
[  136.387408390,3] ***********************************************
[  136.387454272,3] < assert failed at core/device.c:605 >
[  136.387493225,3]     .
[  136.387512799,3]      .
[  136.387534056,3]       .
[  136.387550294,3]         OO__)
[  136.387579530,3]        <"__/
[  136.387605086,3]         ^ ^
[  136.387719329,3] Fatal TRAP at 0000000030028a18   .dt_property_set_cell+0x34  MSR 9000000000021002
[  136.387801707,3] CFAR : 00000000300bfd3c MSR  : 9000000000001000
[  136.387847032,3] SRR0 : 0000000030028a18 SRR1 : 9000000000021002
[  136.387893119,3] HSRR0: 0000000030012524 HSRR1: 9000000000001000
[  136.387936830,3] DSISR: 40000000         DAR  : 00000002019df000
[  136.387983570,3] LR   : 00000000300bfd40 CTR  : 0000000000000000
[  136.388046031,3] CR   : 20004202         XER  : 00000000
[  136.388094553,3] GPR00: 00000000300bfd40 GPR16: 0000000000000001
[  136.388139862,3] GPR01: 0000000031e536e0 GPR17: 00000000300ca3c9
[  136.388181131,3] GPR02: 0000000030121200 GPR18: 0000000030103e1c
[  136.388224105,3] GPR03: 000000003053fc60 GPR19: 0000000000000008
[  136.388270356,3] GPR04: 0000000000000001 GPR20: 000000003053fba0
[  136.388313950,3] GPR05: 0000000000000008 GPR21: 0000000000000001
[  136.388363021,3] GPR06: 0000000031e50060 GPR22: 0000000000000001
[  136.388416754,3] GPR07: 0000000000000000 GPR23: 0000000000000000
[  136.388465729,3] GPR08: 0000000000000000 GPR24: 0000000000000000
[  136.388508156,3] GPR09: 0000000000000004 GPR25: 0000000031204060
[  136.388556203,3] GPR10: 0000000000000008 GPR26: 000000003120402c
[  136.388599076,3] GPR11: 0000000000000000 GPR27: 0000000030010000
[  136.388642108,3] GPR12: 0000000040004204 GPR28: 0000000000000002
[  136.388694064,3] GPR13: 0000000031e50000 GPR29: 0000000031203ee0
[  136.388743298,3] GPR14: 00000000300cbf03 GPR30: 0000000031202e80
[  136.388797131,3] GPR15: 00000000300cc01c GPR31: 0000000030103a33
CPU 0048 Backtrace:
 S: 0000000031e539e0 R: 0000000030028874   .dt_resize_property+0x28
 S: 0000000031e53a60 R: 00000000300bfd40   .memory_parse+0xd84
 S: 0000000031e53c40 R: 00000000300bc4d8   .parse_hdat+0xed0
 S: 0000000031e53e30 R: 000000003001504c   .main_cpu_entry+0x1ac
 S: 0000000031e53f00 R: 0000000030002760   boot_entry+0x1b0

Avoid further appearances of the unidentified animal of doom by making
dt_resize_property() do the length updating itself, freeing its callers
from that need.

Suggested-by: Oliver O'Halloran <oohall@gmail.com>
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
---
 core/cpufeatures.c   | 1 -
 core/device.c        | 1 +
 hdata/hostservices.c | 1 -
 hw/npu.c             | 1 -
 hw/npu2.c            | 1 -
 hw/npu3-nvlink.c     | 1 -
 6 files changed, 1 insertion(+), 5 deletions(-)

Comments

Vasant Hegde June 19, 2020, 4:59 a.m. UTC | #1
On 6/19/20 8:27 AM, Thiago Jung Bauermann wrote:
> All callers of dt_resize_property() need to set the new property length
> after calling it. append_chip_id() wasn't doing it, which caused this
> assert when booting my machine:
> 
> [  136.387213258,3] Unable to use memory range 0 from MSAREA 0
> [  136.387356677,3] Unable to use memory range 0 from MSAREA 2
> [  136.387408390,3] ***********************************************
> [  136.387454272,3] < assert failed at core/device.c:605 >
> [  136.387493225,3]     .
> [  136.387512799,3]      .
> [  136.387534056,3]       .
> [  136.387550294,3]         OO__)
> [  136.387579530,3]        <"__/
> [  136.387605086,3]         ^ ^
> [  136.387719329,3] Fatal TRAP at 0000000030028a18   .dt_property_set_cell+0x34  MSR 9000000000021002
> [  136.387801707,3] CFAR : 00000000300bfd3c MSR  : 9000000000001000
> [  136.387847032,3] SRR0 : 0000000030028a18 SRR1 : 9000000000021002
> [  136.387893119,3] HSRR0: 0000000030012524 HSRR1: 9000000000001000
> [  136.387936830,3] DSISR: 40000000         DAR  : 00000002019df000
> [  136.387983570,3] LR   : 00000000300bfd40 CTR  : 0000000000000000
> [  136.388046031,3] CR   : 20004202         XER  : 00000000
> [  136.388094553,3] GPR00: 00000000300bfd40 GPR16: 0000000000000001
> [  136.388139862,3] GPR01: 0000000031e536e0 GPR17: 00000000300ca3c9
> [  136.388181131,3] GPR02: 0000000030121200 GPR18: 0000000030103e1c
> [  136.388224105,3] GPR03: 000000003053fc60 GPR19: 0000000000000008
> [  136.388270356,3] GPR04: 0000000000000001 GPR20: 000000003053fba0
> [  136.388313950,3] GPR05: 0000000000000008 GPR21: 0000000000000001
> [  136.388363021,3] GPR06: 0000000031e50060 GPR22: 0000000000000001
> [  136.388416754,3] GPR07: 0000000000000000 GPR23: 0000000000000000
> [  136.388465729,3] GPR08: 0000000000000000 GPR24: 0000000000000000
> [  136.388508156,3] GPR09: 0000000000000004 GPR25: 0000000031204060
> [  136.388556203,3] GPR10: 0000000000000008 GPR26: 000000003120402c
> [  136.388599076,3] GPR11: 0000000000000000 GPR27: 0000000030010000
> [  136.388642108,3] GPR12: 0000000040004204 GPR28: 0000000000000002
> [  136.388694064,3] GPR13: 0000000031e50000 GPR29: 0000000031203ee0
> [  136.388743298,3] GPR14: 00000000300cbf03 GPR30: 0000000031202e80
> [  136.388797131,3] GPR15: 00000000300cc01c GPR31: 0000000030103a33
> CPU 0048 Backtrace:
>   S: 0000000031e539e0 R: 0000000030028874   .dt_resize_property+0x28
>   S: 0000000031e53a60 R: 00000000300bfd40   .memory_parse+0xd84
>   S: 0000000031e53c40 R: 00000000300bc4d8   .parse_hdat+0xed0
>   S: 0000000031e53e30 R: 000000003001504c   .main_cpu_entry+0x1ac
>   S: 0000000031e53f00 R: 0000000030002760   boot_entry+0x1b0
> 
> Avoid further appearances of the unidentified animal of doom by making
> dt_resize_property() do the length updating itself, freeing its callers
> from that need.
> 
> Suggested-by: Oliver O'Halloran <oohall@gmail.com>
> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>

Looks good.
Reviewed-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>

-Vasant
Thiago Jung Bauermann June 19, 2020, 3:59 p.m. UTC | #2
Vasant Hegde <hegdevasant@linux.vnet.ibm.com> writes:

> On 6/19/20 8:27 AM, Thiago Jung Bauermann wrote:
>> Suggested-by: Oliver O'Halloran <oohall@gmail.com>
>> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
>
> Looks good.
> Reviewed-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>

Thanks for the review!
Oliver O'Halloran July 3, 2020, 5:43 a.m. UTC | #3
On Fri, Jun 19, 2020 at 12:58 PM Thiago Jung Bauermann
<bauerman@linux.ibm.com> wrote:
>
> All callers of dt_resize_property() need to set the new property length
> after calling it. append_chip_id() wasn't doing it, which caused this
> assert when booting my machine:
>
> *snip*
>
> Avoid further appearances of the unidentified animal of doom by making
> dt_resize_property() do the length updating itself, freeing its callers
> from that need.
>
> Suggested-by: Oliver O'Halloran <oohall@gmail.com>
> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>

Thanks, I'm still surprised we haven't hit this bug until now. Merged
as 5021a037a790.
diff mbox series

Patch

diff --git a/core/cpufeatures.c b/core/cpufeatures.c
index c6754abbe..f31850ab6 100644
--- a/core/cpufeatures.c
+++ b/core/cpufeatures.c
@@ -827,7 +827,6 @@  static void add_cpufeatures_dependencies(struct dt_node *features)
 		deps_names = f->dependencies_names;
 		nr_deps = strcount(deps_names, " ") + 1;
 		dt_resize_property(&deps, nr_deps * sizeof(u32));
-		deps->len = nr_deps * sizeof(u32);
 
 		DBG("feature %s has %d dependencies (%s)\n", f->name, nr_deps, deps_names);
 		/*
diff --git a/core/device.c b/core/device.c
index 4fd44597b..b102dd973 100644
--- a/core/device.c
+++ b/core/device.c
@@ -476,6 +476,7 @@  void dt_resize_property(struct dt_property **prop, size_t len)
 	size_t new_len = sizeof(**prop) + len;
 
 	*prop = realloc(*prop, new_len);
+	(*prop)->len = len;
 
 	/* Fix up linked lists in case we moved. (note: not an empty list). */
 	(*prop)->list.next->prev = &(*prop)->list;
diff --git a/hdata/hostservices.c b/hdata/hostservices.c
index 9e53a865e..2181ecd8b 100644
--- a/hdata/hostservices.c
+++ b/hdata/hostservices.c
@@ -36,7 +36,6 @@  static void merge_property(const struct dt_node *src_root,
 	/* Append src to dst. */
 	dt_resize_property(&dst, dst->len + src->len);
 	memcpy(dst->prop + dst->len, src->prop, src->len);
-	dst->len += src->len;
 }
 
 static void hservice_parse_dt_tree(const struct dt_node *src)
diff --git a/hw/npu.c b/hw/npu.c
index 9266f3674..dba7ee50f 100644
--- a/hw/npu.c
+++ b/hw/npu.c
@@ -416,7 +416,6 @@  static void npu_append_pci_phandle(struct dt_node *dn, u32 phandle)
 	prop_len = pci_npu_phandle_prop->len;
 	prop_len += sizeof(*npu_phandles);
 	dt_resize_property(&pci_npu_phandle_prop, prop_len);
-	pci_npu_phandle_prop->len = prop_len;
 
 	npu_phandles = (uint32_t *) pci_npu_phandle_prop->prop;
 	npu_phandles[prop_len/sizeof(*npu_phandles) - 1] = phandle;
diff --git a/hw/npu2.c b/hw/npu2.c
index 1624d52cf..cf57eeb0c 100644
--- a/hw/npu2.c
+++ b/hw/npu2.c
@@ -510,7 +510,6 @@  static void npu2_append_phandle(struct dt_node *dn,
 	/* Need to append to the properties */
 	len = prop->len + sizeof(*npu_phandles);
 	dt_resize_property(&prop, len);
-	prop->len = len;
 
 	npu_phandles = (uint32_t *)prop->prop;
 	npu_phandles[len / sizeof(*npu_phandles) - 1] = phandle;
diff --git a/hw/npu3-nvlink.c b/hw/npu3-nvlink.c
index 42054cfca..920864b32 100644
--- a/hw/npu3-nvlink.c
+++ b/hw/npu3-nvlink.c
@@ -350,7 +350,6 @@  static void npu3_append_phandle(struct dt_node *dn, const char *name,
 	/* Need to append to the property */
 	len = prop->len + sizeof(*phandles);
 	dt_resize_property(&prop, len);
-	prop->len = len;
 
 	phandles = (uint32_t *)prop->prop;
 	phandles[len / sizeof(*phandles) - 1] = phandle;