Skip to contentSkip to conversation

Set device tree bootargs failed when DTB does not contain a chosen node.

BUG Applied Vicente Bergas 4 participants 19 messages Oct 26, 2018 – Nov 30, 2018

Subsystems (2)
Live update kexec handover (KHO)
lore
Share & tools
lore

Vicente Bergas

author
#0 lore raw

Hello,
when executing
kexec -d --dtb dtb_without_chosen_node.dtb --append 'cmdline' --load Image
it reports
dtb_set_property: fdt_add_subnode failed: <valid offset/length>
kexec: Set device tree bootargs failed.

It has been tested on the arm64 architecture with version v2.0.17 and v2.0.18-rc1

Regards,
Vicenç.

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

#1 lore raw

Hi Vicenç,

Quoted 10 lines · Vicente Bergas

Hello,
when executing
kexec -d --dtb dtb_without_chosen_node.dtb --append 'cmdline' --load Image
it reports
dtb_set_property: fdt_add_subnode failed: <valid offset/length>
kexec: Set device tree bootargs failed.

It has been tested on the arm64 architecture with version v2.0.17 and v2.0.18-rc1

Can you share some details on the underlying platform and kernel version you are using?

Looking at the logs, I am assuming you are running 'kexec' on a arm64 platform (as it depends on creating a dtb to be passed to the 2nd kernel).

My general advice is to avoid the --dtb option as it is known to cause issues with kexec in the past (see [0] for details)
Ideally, if you don't use the --dtb option, then kexec will read the existing kernel's dtb from /proc/device-tree, and that device tree will include all the changes that the boot loader has done, including adding details of the available memory in the system.

[0]. https://www.spinics.net/lists/arm-kernel/msg618236.html

Hope this helps.

Regards,
Bhupesh

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

author
#2 lore raw
Quoted 29 lines · Bhupesh Sharma

Hi Vicenç,

Quoted 10 lines · Vicente Bergas

Hello,
when executing
kexec -d --dtb dtb_without_chosen_node.dtb --append 'cmdline' --load Image
it reports
dtb_set_property: fdt_add_subnode failed: <valid offset/length>
kexec: Set device tree bootargs failed.

It has been tested on the arm64 architecture with version v2.0.17 and v2.0.18-rc1

Can you share some details on the underlying platform and kernel version you are using?

Looking at the logs, I am assuming you are running 'kexec' on a arm64 platform (as it depends on creating a dtb to be passed to the 2nd kernel).

My general advice is to avoid the --dtb option as it is known to cause issues with kexec in the past (see [0] for details)
Ideally, if you don't use the --dtb option, then kexec will read the existing kernel's dtb from /proc/device-tree, and that device tree will include all the changes that the boot loader has done, including adding details of the available memory in the system.

Quoted 6 lines

[0]. https://www.spinics.net/lists/arm-kernel/msg618236.html

Hope this helps.

Regards,
Bhupesh

Hi Bhupesh, thanks for answering and yes, this helps!

As you say, when not using the --dtb option it works fine. But this way is not applicable in my use case.

I am trying to use Linux as a bootloader. This bootloader will reside in a difficult-to-write memory (SPI-NOR Flash) and, because of that, will rarely be updated.
Over time, new kernel versions will provide updated DTB files, so, the DT from both kernels will diverge over time.
This is the reason to use the --dtb option.

Thanks for the information you provided, now I see a bug and also a missing feature.

Focusing on the bug, I have traced it down to here:
https://git.kernel.org/pub/scm/utils/kernel/kexec/kexec-tools.git/tree/kexec/dt-ops.c#n87
the result is tested for non-zero as error condition, but, as seen here
https://git.kernel.org/pub/scm/utils/kernel/kexec/kexec-tools.git/tree/kexec/libfdt/fdt_rw.c#n345
it looks like positive values are not errors.

Now, focusing on the missing feature.
kexec-tools already has the (buggy) ability to add the "/chosen/bootargs" node. I assume that it also adds or updates the "/chosen/linux,initrd-start" and "/chosen/linux,initrd-end" nodes because it has the --initrd option. Again, I assume it also adds/updates all "/memreserve" nodes with the location where the DTB and initrd reside in memory.

So, just adding the "/memory" node will make it feature complete. This node can be populated with data provided from the command line as parameters or with data extracted from /proc/device-tree.

Adding "/ethernetX/local-mac-address" would be a bonus.

As a side node, can I suggest updating libfdt with latest upstream? from:
https://git.kernel.org/pub/scm/utils/kernel/kexec/kexec-tools.git/tree/kexec/libfdt
to:
https://git.kernel.org/pub/scm/utils/dtc/dtc.git/tree/libfdt

Regards,
Vicenç

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

#3 lore raw

Hi Vicenç,

Sorry I was on holidays and came back to office just yesterday.

Quoted 14 lines · Vicente Bergas
Quoted 12 lines · Bhupesh Sharma

Hi Vicenç,

Quoted 8 lines · Vicente Bergas

Hello,
when executing
kexec -d --dtb dtb_without_chosen_node.dtb --append 'cmdline' --load Image
it reports
dtb_set_property: fdt_add_subnode failed: <valid offset/length>
kexec: Set device tree bootargs failed.

Quoted 5 lines
Quoted 5 lines
Quoted 2 lines

It has been tested on the arm64 architecture with version v2.0.17 and v2.0.18-rc1

Can you share some details on the underlying platform and kernel version you are using?

Again, would request you to share the above details, which would help me answer your queries in a better way.

Quoted 34 lines
Quoted 11 lines

Looking at the logs, I am assuming you are running 'kexec' on a arm64 platform (as it depends on creating a dtb to be passed to the 2nd kernel).

My general advice is to avoid the --dtb option as it is known to cause issues with kexec in the past (see [0] for details)
Ideally, if you don't use the --dtb option, then kexec will read the existing kernel's dtb from /proc/device-tree, and that device tree will include all the changes that the boot loader has done, including adding details of the available memory in the system.

Quoted 6 lines

[0]. https://www.spinics.net/lists/arm-kernel/msg618236.html

Hope this helps.

Regards,
Bhupesh

Hi Bhupesh, thanks for answering and yes, this helps!

As you say, when not using the --dtb option it works fine. But this way is not applicable in my use case.

I am trying to use Linux as a bootloader. This bootloader will reside in a difficult-to-write memory (SPI-NOR Flash) and, because of that, will rarely be updated.
Over time, new kernel versions will provide updated DTB files, so, the DT from both kernels will diverge over time.
This is the reason to use the --dtb option.

Thanks for the information you provided, now I see a bug and also a missing feature.

Quoted 5 lines

Focusing on the bug, I have traced it down to here:
https://git.kernel.org/pub/scm/utils/kernel/kexec/kexec-tools.git/tree/kexec/dt-ops.c#n87
the result is tested for non-zero as error condition, but, as seen here
https://git.kernel.org/pub/scm/utils/kernel/kexec/kexec-tools.git/tree/kexec/libfdt/fdt_rw.c#n345
it looks like positive values are not errors.

Hmmm.. looking hard at it, I am not sure if its indeed a bug, but more on the same below ...

Quoted 7 lines

Now, focusing on the missing feature.
kexec-tools already has the (buggy) ability to add the "/chosen/bootargs" node. I assume that it also adds or updates the "/chosen/linux,initrd-start" and "/chosen/linux,initrd-end" nodes because it has the --initrd option. Again, I assume it also adds/updates all "/memreserve" nodes with the location where the DTB and initrd reside in memory.

Quoted 5 lines

So, just adding the "/memory" node will make it feature complete. This node can be populated with data provided from the command line as parameters or with data extracted from /proc/device-tree.

Adding "/ethernetX/local-mac-address" would be a bonus.

... Not really, as again this is very board specific. For e.g. I can have a 'usb' (or even multiple Ethernet
cards on the board) as the transport media, which means that the 'kexec-tools' must be able to handle all of those
via the --dtb option and prepare the dtb to be sent to the 2nd kernel accordingly.

I hope you understand my point here that we have a minimalistic implementation intentionally as catering to all
dtb nodes in kexec-tools is probably equivalent to adding functionality similar to how a bootloader (like u-boot) allows dtb properties to be changed. And even if we add some basic skeleton for the same, we cannot be sure that it will be suitable for all arm64 hardware available.

Quoted 5 lines

As a side node, can I suggest updating libfdt with latest upstream? from:
https://git.kernel.org/pub/scm/utils/kernel/kexec/kexec-tools.git/tree/kexec/libfdt
to:
https://git.kernel.org/pub/scm/utils/dtc/dtc.git/tree/libfdt

This is a good suggestion. I have this already on my to-do queue. I will find some time to complete it over the weekend and share a patch on the same probably by next week.

Thanks,
Bhupesh

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

author
#4 lore raw

Hi Bhupesh,

Quoted 20 lines · Bhupesh Sharma

Hi Vicenç,

Sorry I was on holidays and came back to office just yesterday.

Quoted 14 lines · Vicente Bergas
Quoted 12 lines · Bhupesh Sharma

Hi Vicenç,

Quoted 8 lines · Vicente Bergas

Hello,
when executing
kexec -d --dtb dtb_without_chosen_node.dtb --append 'cmdline' --load Image
it reports
dtb_set_property: fdt_add_subnode failed: <valid offset/length>
kexec: Set device tree bootargs failed.

Quoted 8 lines
Quoted 5 lines
Quoted 5 lines
Quoted 2 lines

It has been tested on the arm64 architecture with version v2.0.17 and v2.0.18-rc1

Can you share some details on the underlying platform and kernel version you are using?

Again, would request you to share the above details, which would help me answer your queries in a better way.

It has been tested on the arm64 architecture and the kernel version was 4.19, now moved to 4.20-rc.

Quoted 63 lines
Quoted 34 lines
Quoted 11 lines

Looking at the logs, I am assuming you are running 'kexec' on a arm64 platform (as it depends on creating a dtb to be passed to the 2nd kernel).

My general advice is to avoid the --dtb option as it is known to cause issues with kexec in the past (see [0] for details)
Ideally, if you don't use the --dtb option, then kexec will read the existing kernel's dtb from /proc/device-tree, and that device tree will include all the changes that the boot loader has done, including adding details of the available memory in the system.

Quoted 6 lines

[0]. https://www.spinics.net/lists/arm-kernel/msg618236.html

Hope this helps.

Regards,
Bhupesh

Hi Bhupesh, thanks for answering and yes, this helps!

As you say, when not using the --dtb option it works fine. But this way is not applicable in my use case.

I am trying to use Linux as a bootloader. This bootloader will reside in a difficult-to-write memory (SPI-NOR Flash) and, because of that, will rarely be updated.
Over time, new kernel versions will provide updated DTB files, so, the DT from both kernels will diverge over time.
This is the reason to use the --dtb option.

Thanks for the information you provided, now I see a bug and also a missing feature.

Quoted 5 lines

Focusing on the bug, I have traced it down to here:
https://git.kernel.org/pub/scm/utils/kernel/kexec/kexec-tools.git/tree/kexec/dt-ops.c#n87
the result is tested for non-zero as error condition, but, as seen here
https://git.kernel.org/pub/scm/utils/kernel/kexec/kexec-tools.git/tree/kexec/libfdt/fdt_rw.c#n345
it looks like positive values are not errors.

Hmmm.. looking hard at it, I am not sure if its indeed a bug, but more on the same below ...

Quoted 7 lines

Now, focusing on the missing feature.
kexec-tools already has the (buggy) ability to add the "/chosen/bootargs" node. I assume that it also adds or updates the "/chosen/linux,initrd-start" and "/chosen/linux,initrd-end" nodes because it has the --initrd option. Again, I assume it also adds/updates all "/memreserve" nodes with the location where the DTB and initrd reside in memory.

Quoted 5 lines

So, just adding the "/memory" node will make it feature complete. This node can be populated with data provided from the command line as parameters or with data extracted from /proc/device-tree.

Adding "/ethernetX/local-mac-address" would be a bonus.

... Not really, as again this is very board specific. For e.g. I can have a 'usb' (or even multiple Ethernet
cards on the board) as the transport media, which means that the 'kexec-tools' must be able to handle all of those
via the --dtb option and prepare the dtb to be sent to the 2nd kernel accordingly.

Quoted 7 lines

I hope you understand my point here that we have a minimalistic implementation intentionally as catering to all
dtb nodes in kexec-tools is probably equivalent to adding functionality similar to how a bootloader (like u-boot) allows dtb properties to be changed. And even if we add some basic skeleton for the same, we cannot be sure that it will be suitable for all arm64 hardware available.

Ok, I see the point, so, forget about new features. But I still think the situation is improvable in one or other way for DTBs without a chosen node:
a.- In dt-ops.c:85 add the chosen node and proceed without error.
The current code already adds it, but then it aborts.
b.- Report an error message saying that this use case is not supported.
Otherwise the current message looks like the program is broken.

Quoted 6 lines
Quoted 5 lines

As a side node, can I suggest updating libfdt with latest upstream? from:
https://git.kernel.org/pub/scm/utils/kernel/kexec/kexec-tools.git/tree/kexec/libfdt
to:
https://git.kernel.org/pub/scm/utils/dtc/dtc.git/tree/libfdt

Quoted 6 lines

This is a good suggestion. I have this already on my to-do queue. I will find some time to complete it over the weekend and share a patch on the same probably by next week.

Thanks,
Bhupesh

Regards,
Vicenç.

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

#5 lore raw

Hi Vicenç,

Quoted 107 lines · Vicente Bergas

Hi Bhupesh,

Quoted 20 lines · Bhupesh Sharma

Hi Vicenç,

Sorry I was on holidays and came back to office just yesterday.

Quoted 14 lines · Vicente Bergas
Quoted 12 lines · Bhupesh Sharma

Hi Vicenç,

Quoted 8 lines · Vicente Bergas

Hello,
when executing
kexec -d --dtb dtb_without_chosen_node.dtb --append 'cmdline' --load Image
it reports
dtb_set_property: fdt_add_subnode failed: <valid offset/length>
kexec: Set device tree bootargs failed.

Quoted 8 lines
Quoted 5 lines
Quoted 5 lines
Quoted 2 lines

It has been tested on the arm64 architecture with version v2.0.17 and v2.0.18-rc1

Can you share some details on the underlying platform and kernel version you are using?

Again, would request you to share the above details, which would help me answer your queries in a better way.

It has been tested on the arm64 architecture and the kernel version was 4.19, now moved to 4.20-rc.

Quoted 63 lines
Quoted 34 lines
Quoted 11 lines

Looking at the logs, I am assuming you are running 'kexec' on a arm64 platform (as it depends on creating a dtb to be passed to the 2nd kernel).

My general advice is to avoid the --dtb option as it is known to cause issues with kexec in the past (see [0] for details)
Ideally, if you don't use the --dtb option, then kexec will read the existing kernel's dtb from /proc/device-tree, and that device tree will include all the changes that the boot loader has done, including adding details of the available memory in the system.

Quoted 6 lines

[0]. https://www.spinics.net/lists/arm-kernel/msg618236.html

Hope this helps.

Regards,
Bhupesh

Hi Bhupesh, thanks for answering and yes, this helps!

As you say, when not using the --dtb option it works fine. But this way is not applicable in my use case.

I am trying to use Linux as a bootloader. This bootloader will reside in a difficult-to-write memory (SPI-NOR Flash) and, because of that, will rarely be updated.
Over time, new kernel versions will provide updated DTB files, so, the DT from both kernels will diverge over time.
This is the reason to use the --dtb option.

Thanks for the information you provided, now I see a bug and also a missing feature.

Quoted 5 lines

Focusing on the bug, I have traced it down to here:
https://git.kernel.org/pub/scm/utils/kernel/kexec/kexec-tools.git/tree/kexec/dt-ops.c#n87
the result is tested for non-zero as error condition, but, as seen here
https://git.kernel.org/pub/scm/utils/kernel/kexec/kexec-tools.git/tree/kexec/libfdt/fdt_rw.c#n345
it looks like positive values are not errors.

Hmmm.. looking hard at it, I am not sure if its indeed a bug, but more on the same below ...

Quoted 7 lines

Now, focusing on the missing feature.
kexec-tools already has the (buggy) ability to add the "/chosen/bootargs" node. I assume that it also adds or updates the "/chosen/linux,initrd-start" and "/chosen/linux,initrd-end" nodes because it has the --initrd option. Again, I assume it also adds/updates all "/memreserve" nodes with the location where the DTB and initrd reside in memory.

Quoted 5 lines

So, just adding the "/memory" node will make it feature complete. This node can be populated with data provided from the command line as parameters or with data extracted from /proc/device-tree.

Adding "/ethernetX/local-mac-address" would be a bonus.

... Not really, as again this is very board specific. For e.g. I can have a 'usb' (or even multiple Ethernet
cards on the board) as the transport media, which means that the 'kexec-tools' must be able to handle all of those
via the --dtb option and prepare the dtb to be sent to the 2nd kernel accordingly.

Quoted 7 lines

I hope you understand my point here that we have a minimalistic implementation intentionally as catering to all
dtb nodes in kexec-tools is probably equivalent to adding functionality similar to how a bootloader (like u-boot) allows dtb properties to be changed. And even if we add some basic skeleton for the same, we cannot be sure that it will be suitable for all arm64 hardware available.

Quoted 5 lines

Ok, I see the point, so, forget about new features. But I still think the situation is improvable in one or other way for DTBs without a chosen node:
a.- In dt-ops.c:85 add the chosen node and proceed without error.
The current code already adds it, but then it aborts.

Fair enough. I have submitted a patch (see here:
<http://lists.infradead.org/pipermail/kexec/2018-November/021828.html>),
taking the approach in point (a). Can you please test the same on your platform and share your test results?

Thanks,
Bhupesh

Quoted 13 lines

b.- Report an error message saying that this use case is not supported.
Otherwise the current message looks like the program is broken.

Quoted 10 lines
Quoted 5 lines

As a side node, can I suggest updating libfdt with latest upstream? from:
https://git.kernel.org/pub/scm/utils/kernel/kexec/kexec-tools.git/tree/kexec/libfdt
to:
https://git.kernel.org/pub/scm/utils/dtc/dtc.git/tree/libfdt

This is a good suggestion. I have this already on my to-do queue. I will find some time to complete it over the weekend and share a patch on the same probably by next week.

Quoted 5 lines
Quoted 2 lines

Thanks,
Bhupesh

Regards,
Vicenç.

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

#6 lore raw

PATCH 1/1 kexec/dt-ops.c: Fix check against 'fdt_add_subnode' return value

Vicenç reported (via [1]) that currently executing kexec with '--dtb' option and passing a .dtb which doesn't have a '/chosen' node leads to the following error:

  # kexec -d --dtb dtb_without_chosen_node.dtb --append 'cmdline' --load Image

dtb_set_property: fdt_add_subnode failed: <valid offset/length>
kexec: Set device tree bootargs failed.

This happens because currently we check the return value of 'fdt_add_subnode()' function call in 'dt-ops.c' incorrectly:

   result = fdt_add_subnode(new_dtb, nodeoffset, node);
   if (result) {
   	dbgprintf("%s: fdt_add_subnode failed: %s\n", _func__,
   		fdt_strerror(result));
   	goto on_error;
   }

As we can see in 'fdt_rw.c', a positive return value from 'fdt_add_subnode()' function doesn't indicate an error.

We can see that the Linux kernel (see 'drivers/firmware/efi/libstub/fdt.c' for example) also checks the 'fdt_add_subnode()' function against negative return values for errors. See an example below from 'update_fdt()' function in 'drivers/firmware/efi/libstub/fdt.c':

   node = fdt_add_subnode(fdt, 0, "chosen");
   if (node < 0) {
   	status = node;
   	<..snip..>
   	goto fdt_set_fail;
   }

This patch fixes the same in 'kexec-tools'.

[1]. http://lists.infradead.org/pipermail/kexec/2018-October/021746.html

Cc Simon Horman <[email protected]>
Cc AKASHI Takahiro <[email protected]>
Reported-by Vicente Bergas <[email protected]>
Signed-off-by Bhupesh Sharma <[email protected]>
---
 kexec/dt-ops.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
kexec/dt-ops.c +1 -1
index 915dbf55afd2..f15174c3c74e 100644
@@ -84,7 +84,7 @@ int dtb_set_property(char **dtb, off_t *dtb_size, const char *node,
 	if (nodeoffset == -FDT_ERR_NOTFOUND) {
 		result = fdt_add_subnode(new_dtb, nodeoffset, node);
 
-		if (result) {
+		if (result < 0) {
 			dbgprintf("%s: fdt_add_subnode failed: %s\n", __func__,
 				fdt_strerror(result));
 			goto on_error;
Signature · 7 lines
2.7.4


_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec
#7 lore raw

Re: [PATCH] kexec/dt-ops.c: Fix check against 'fdt_add_subnode' return value

Quoted 27 lines · Bhupesh Sharma

Vicenç reported (via [1]) that currently executing kexec with '--dtb' option and passing a .dtb which doesn't have a '/chosen' node leads to the following error:

  # kexec -d --dtb dtb_without_chosen_node.dtb --append 'cmdline' --load Image

dtb_set_property: fdt_add_subnode failed: <valid offset/length>
kexec: Set device tree bootargs failed.

This happens because currently we check the return value of 'fdt_add_subnode()' function call in 'dt-ops.c' incorrectly:

   result = fdt_add_subnode(new_dtb, nodeoffset, node);
   if (result) {
   	dbgprintf("%s: fdt_add_subnode failed: %s\n", _func__,
   		fdt_strerror(result));
   	goto on_error;
   }

As we can see in 'fdt_rw.c', a positive return value from 'fdt_add_subnode()' function doesn't indicate an error.

We can see that the Linux kernel (see 'drivers/firmware/efi/libstub/fdt.c' for example) also checks the 'fdt_add_subnode()' function against negative return values for errors. See an example below from 'update_fdt()' function in 'drivers/firmware/efi/libstub/fdt.c':

Quoted 8 lines
   node = fdt_add_subnode(fdt, 0, "chosen");
   if (node < 0) {
   	status = node;
   	<..snip..>
   	goto fdt_set_fail;
   }

This patch fixes the same in 'kexec-tools'.

Looks good.
Thank you.
-Takahiro Akashi

Quoted 26 lines

[1]. http://lists.infradead.org/pipermail/kexec/2018-October/021746.html

Cc Simon Horman <[email protected]>
Cc AKASHI Takahiro <[email protected]>
Reported-by Vicente Bergas <[email protected]>
Signed-off-by Bhupesh Sharma <[email protected]>
---
 kexec/dt-ops.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
kexec/dt-ops.c +1 -1
index 915dbf55afd2..f15174c3c74e 100644
@@ -84,7 +84,7 @@ int dtb_set_property(char **dtb, off_t *dtb_size, const char *node,
 	if (nodeoffset == -FDT_ERR_NOTFOUND) {
 		result = fdt_add_subnode(new_dtb, nodeoffset, node);
 
-		if (result) {
+		if (result < 0) {
 			dbgprintf("%s: fdt_add_subnode failed: %s\n", __func__,
 				fdt_strerror(result));
 			goto on_error;
Signature · 1 line
2.7.4

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

Applied
#8 lore raw

Re: [PATCH] kexec/dt-ops.c: Fix check against 'fdt_add_subnode' return value

Quoted 35 lines · AKASHI Takahiro
Quoted 34 lines · Bhupesh Sharma

Vicenç reported (via [1]) that currently executing kexec with '--dtb' option and passing a .dtb which doesn't have a '/chosen' node leads to the following error:

  # kexec -d --dtb dtb_without_chosen_node.dtb --append 'cmdline' --load Image

dtb_set_property: fdt_add_subnode failed: <valid offset/length>
kexec: Set device tree bootargs failed.

This happens because currently we check the return value of 'fdt_add_subnode()' function call in 'dt-ops.c' incorrectly:

   result = fdt_add_subnode(new_dtb, nodeoffset, node);
   if (result) {
   	dbgprintf("%s: fdt_add_subnode failed: %s\n", _func__,
   		fdt_strerror(result));
   	goto on_error;
   }

As we can see in 'fdt_rw.c', a positive return value from 'fdt_add_subnode()' function doesn't indicate an error.

We can see that the Linux kernel (see 'drivers/firmware/efi/libstub/fdt.c' for example) also checks the 'fdt_add_subnode()' function against negative return values for errors. See an example below from 'update_fdt()' function in 'drivers/firmware/efi/libstub/fdt.c':

   node = fdt_add_subnode(fdt, 0, "chosen");
   if (node < 0) {
   	status = node;
   	<..snip..>
   	goto fdt_set_fail;
   }
Quoted 5 lines
Quoted 1 line

This patch fixes the same in 'kexec-tools'.

Looks good.
Thank you.
-Takahiro Akashi

Thanks, applied.

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

#9 lore raw

Re: [PATCH] kexec/dt-ops.c: Fix check against 'fdt_add_subnode' return value

Quoted 36 lines · Simon Horman
Quoted 35 lines · AKASHI Takahiro
Quoted 34 lines · Bhupesh Sharma

Vicenç reported (via [1]) that currently executing kexec with '--dtb' option and passing a .dtb which doesn't have a '/chosen' node leads to the following error:

  # kexec -d --dtb dtb_without_chosen_node.dtb --append 'cmdline' --load Image

dtb_set_property: fdt_add_subnode failed: <valid offset/length>
kexec: Set device tree bootargs failed.

This happens because currently we check the return value of 'fdt_add_subnode()' function call in 'dt-ops.c' incorrectly:

   result = fdt_add_subnode(new_dtb, nodeoffset, node);
   if (result) {
   	dbgprintf("%s: fdt_add_subnode failed: %s\n", _func__,
   		fdt_strerror(result));
   	goto on_error;
   }

As we can see in 'fdt_rw.c', a positive return value from 'fdt_add_subnode()' function doesn't indicate an error.

We can see that the Linux kernel (see 'drivers/firmware/efi/libstub/fdt.c' for example) also checks the 'fdt_add_subnode()' function against negative return values for errors. See an example below from 'update_fdt()' function in 'drivers/firmware/efi/libstub/fdt.c':

   node = fdt_add_subnode(fdt, 0, "chosen");
   if (node < 0) {
   	status = node;
   	<..snip..>
   	goto fdt_set_fail;
   }
Quoted 7 lines
Quoted 5 lines
Quoted 1 line

This patch fixes the same in 'kexec-tools'.

Looks good.
Thank you.
-Takahiro Akashi

Thanks, applied.

Perhaps I was a little too hasty there, I have dropped the patch for now.

Vicente, does this patch work for you?

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

author
#10 lore raw

Re: [PATCH] kexec/dt-ops.c: Fix check against 'fdt_add_subnode' return value

Quoted 44 lines · Simon Horman
Quoted 42 lines · Simon Horman
Quoted 40 lines · AKASHI Takahiro
Quoted 27 lines · Bhupesh Sharma

Vicenç reported (via [1]) that currently executing kexec with '--dtb' option and passing a .dtb which doesn't have a '/chosen' node leads to the following error:

  # kexec -d --dtb dtb_without_chosen_node.dtb --append 'cmdline' --load Image

dtb_set_property: fdt_add_subnode failed: <valid offset/length>
kexec: Set device tree bootargs failed.

This happens because currently we check the return value of 'fdt_add_subnode()' function call in 'dt-ops.c' incorrectly:

   result = fdt_add_subnode(new_dtb, nodeoffset, node);
   if (result) {
          dbgprintf("%s: fdt_add_subnode failed: %s\n", _func__,
                  fdt_strerror(result));
          goto on_error;
   }

As we can see in 'fdt_rw.c', a positive return value from 'fdt_add_subnode()' function doesn't indicate an error.

We can see that the Linux kernel (see 'drivers/firmware/efi/libstub/fdt.c' for example) also checks the 'fdt_add_subnode()' function against negative return values for errors. See an example below from 'update_fdt()' function in 'drivers/firmware/efi/libstub/fdt.c':

Quoted 8 lines
   node = fdt_add_subnode(fdt, 0, "chosen");
   if (node < 0) {
          status = node;
          <..snip..>
          goto fdt_set_fail;
   }

This patch fixes the same in 'kexec-tools'.

Looks good.
Thank you.
-Takahiro Akashi

Quoted 5 lines
Quoted 1 line

Thanks, applied.

Perhaps I was a little too hasty there, I have dropped the patch for now.

Vicente, does this patch work for you?

The patch looks fine, but the end result is still non-working:

  # kexec -d --load Image --append 'debug' --dtb no_chosen.dtb
  ...
  dtb_set_property: fdt_setprop failed: FDT_ERR_BADOFFSET
  kexec: Set device tree bootargs failed.

Also tested this with the same result:

kexec/dt-ops.c +3 -1
@@ -84,11 +84,13 @@
        if (nodeoffset == -FDT_ERR_NOTFOUND) {
                result = fdt_add_subnode(new_dtb, nodeoffset, node);
 
-               if (result) {
+               if (result < 0) {
                        dbgprintf("%s: fdt_add_subnode failed: %s\n", __func__,
                                fdt_strerror(result));
                        goto on_error;
                }
+
+               nodeoffset = result;
        } else if (nodeoffset < 0) {
                dbgprintf("%s: fdt_path_offset failed: %s\n", __func__,
                        fdt_strerror(nodeoffset));

Regards,
Vicenç.

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

#11 lore raw

Re: [PATCH] kexec/dt-ops.c: Fix check against 'fdt_add_subnode' return value

Hi Vicenç, Simon,

Quoted 67 lines · Vicente Bergas
Quoted 44 lines · Simon Horman
Quoted 42 lines · Simon Horman
Quoted 40 lines · AKASHI Takahiro
Quoted 27 lines · Bhupesh Sharma

Vicenç reported (via [1]) that currently executing kexec with '--dtb' option and passing a .dtb which doesn't have a '/chosen' node leads to the following error:

  # kexec -d --dtb dtb_without_chosen_node.dtb --append 'cmdline' --load Image

dtb_set_property: fdt_add_subnode failed: <valid offset/length>
kexec: Set device tree bootargs failed.

This happens because currently we check the return value of 'fdt_add_subnode()' function call in 'dt-ops.c' incorrectly:

   result = fdt_add_subnode(new_dtb, nodeoffset, node);
   if (result) {
          dbgprintf("%s: fdt_add_subnode failed: %s\n", _func__,
                  fdt_strerror(result));
          goto on_error;
   }

As we can see in 'fdt_rw.c', a positive return value from 'fdt_add_subnode()' function doesn't indicate an error.

We can see that the Linux kernel (see 'drivers/firmware/efi/libstub/fdt.c' for example) also checks the 'fdt_add_subnode()' function against negative return values for errors. See an example below from 'update_fdt()' function in 'drivers/firmware/efi/libstub/fdt.c':

Quoted 8 lines
   node = fdt_add_subnode(fdt, 0, "chosen");
   if (node < 0) {
          status = node;
          <..snip..>
          goto fdt_set_fail;
   }

This patch fixes the same in 'kexec-tools'.

Looks good.
Thank you.
-Takahiro Akashi

Quoted 5 lines
Quoted 1 line

Thanks, applied.

Perhaps I was a little too hasty there, I have dropped the patch for now.

Vicente, does this patch work for you?

The patch looks fine, but the end result is still non-working:

  # kexec -d --load Image --append 'debug' --dtb no_chosen.dtb
  ...
  dtb_set_property: fdt_setprop failed: FDT_ERR_BADOFFSET
  kexec: Set device tree bootargs failed.

Also tested this with the same result:

kexec/dt-ops.c +0 -1
@@ -84,11 +84,13 @@
        if (nodeoffset == -FDT_ERR_NOTFOUND) {
                result = fdt_add_subnode(new_dtb, nodeoffset, node);
 
-               if (result) {

kexec/dt-ops.c line 96

Quoted 10 lines
kexec/dt-ops.c +3 -0
+               if (result < 0) {
                        dbgprintf("%s: fdt_add_subnode failed: %s\n", __func__,
                                fdt_strerror(result));
                        goto on_error;
                }
+
+               nodeoffset = result;
        } else if (nodeoffset < 0) {
                dbgprintf("%s: fdt_path_offset failed: %s\n", __func__,
                        fdt_strerror(nodeoffset));

I am checking this at my end today. I will come back with either an updated patch or some queries (in case I run into some issues) probably by tomorrow.

Thanks,
Bhupesh

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

#12 lore raw

Re: [PATCH] kexec/dt-ops.c: Fix check against 'fdt_add_subnode' return value

In reply to Vicente Bergas

Hi Vicenç,

I tried the patch at my end and it works fine as per my checks. So, can I request you to provide some additional data which will help me understand your environment better. See inline:

Quoted 46 lines · Vicente Bergas
Quoted 44 lines · Simon Horman
Quoted 42 lines · Simon Horman
Quoted 40 lines · AKASHI Takahiro
Quoted 27 lines · Bhupesh Sharma

Vicenç reported (via [1]) that currently executing kexec with '--dtb' option and passing a .dtb which doesn't have a '/chosen' node leads to the following error:

  # kexec -d --dtb dtb_without_chosen_node.dtb --append 'cmdline' --load Image

dtb_set_property: fdt_add_subnode failed: <valid offset/length>
kexec: Set device tree bootargs failed.

This happens because currently we check the return value of 'fdt_add_subnode()' function call in 'dt-ops.c' incorrectly:

   result = fdt_add_subnode(new_dtb, nodeoffset, node);
   if (result) {
          dbgprintf("%s: fdt_add_subnode failed: %s\n", _func__,
                  fdt_strerror(result));
          goto on_error;
   }

As we can see in 'fdt_rw.c', a positive return value from 'fdt_add_subnode()' function doesn't indicate an error.

We can see that the Linux kernel (see 'drivers/firmware/efi/libstub/fdt.c' for example) also checks the 'fdt_add_subnode()' function against negative return values for errors. See an example below from 'update_fdt()' function in 'drivers/firmware/efi/libstub/fdt.c':

Quoted 8 lines
   node = fdt_add_subnode(fdt, 0, "chosen");
   if (node < 0) {
          status = node;
          <..snip..>
          goto fdt_set_fail;
   }

This patch fixes the same in 'kexec-tools'.

Looks good.
Thank you.
-Takahiro Akashi

Quoted 8 lines
Quoted 5 lines
Quoted 1 line

Thanks, applied.

Perhaps I was a little too hasty there, I have dropped the patch for now.

Vicente, does this patch work for you?

The patch looks fine, but the end result is still non-working:

  # kexec -d --load Image --append 'debug' --dtb no_chosen.dtb

Can you share the .dts with which you see this issue and steps you use to create the .dtb file at your end?

I normally use at my end the following command:
dtc -o no-chosen.dtb -I dts -O dtb no-chosen.dts

Thanks,
Bhupesh

Quoted 23 lines

...
dtb_set_property: fdt_setprop failed: FDT_ERR_BADOFFSET
kexec: Set device tree bootargs failed.

Also tested this with the same result:

kexec/dt-ops.c +3 -1
@@ -84,11 +84,13 @@
        if (nodeoffset == -FDT_ERR_NOTFOUND) {
                result = fdt_add_subnode(new_dtb, nodeoffset, node);
 
-               if (result) {
+               if (result < 0) {
                        dbgprintf("%s: fdt_add_subnode failed: %s\n", __func__,
                                fdt_strerror(result));
                        goto on_error;
                }
+
+               nodeoffset = result;
        } else if (nodeoffset < 0) {
                dbgprintf("%s: fdt_path_offset failed: %s\n", __func__,
                        fdt_strerror(nodeoffset));
Quoted 2 lines

Regards,
Vicenç.

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

author
#13 lore raw

Re: [PATCH] kexec/dt-ops.c: Fix check against 'fdt_add_subnode' return value

Quoted 63 lines · Bhupesh Sharma

Hi Vicenç,

I tried the patch at my end and it works fine as per my checks. So, can I request you to provide some additional data which will help me understand your environment better. See inline:

Quoted 46 lines · Vicente Bergas
Quoted 44 lines · Simon Horman
Quoted 42 lines · Simon Horman
Quoted 40 lines · AKASHI Takahiro
Quoted 27 lines · Bhupesh Sharma

Vicenç reported (via [1]) that currently executing kexec with '--dtb' option and passing a .dtb which doesn't have a '/chosen' node leads to the following error:

  # kexec -d --dtb dtb_without_chosen_node.dtb --append 'cmdline' --load Image

dtb_set_property: fdt_add_subnode failed: <valid offset/length>
kexec: Set device tree bootargs failed.

This happens because currently we check the return value of 'fdt_add_subnode()' function call in 'dt-ops.c' incorrectly:

   result = fdt_add_subnode(new_dtb, nodeoffset, node);
   if (result) {
          dbgprintf("%s: fdt_add_subnode failed: %s\n", _func__,
                  fdt_strerror(result));
          goto on_error;
   }

As we can see in 'fdt_rw.c', a positive return value from 'fdt_add_subnode()' function doesn't indicate an error.

We can see that the Linux kernel (see 'drivers/firmware/efi/libstub/fdt.c' for example) also checks the 'fdt_add_subnode()' function against negative return values for errors. See an example below from 'update_fdt()' function in 'drivers/firmware/efi/libstub/fdt.c':

Quoted 8 lines
   node = fdt_add_subnode(fdt, 0, "chosen");
   if (node < 0) {
          status = node;
          <..snip..>
          goto fdt_set_fail;
   }

This patch fixes the same in 'kexec-tools'.

Looks good.
Thank you.
-Takahiro Akashi

Quoted 8 lines
Quoted 5 lines
Quoted 1 line

Thanks, applied.

Perhaps I was a little too hasty there, I have dropped the patch for now.

Vicente, does this patch work for you?

The patch looks fine, but the end result is still non-working:

  # kexec -d --load Image --append 'debug' --dtb no_chosen.dtb
Quoted 8 lines

Can you share the .dts with which you see this issue and steps you use to create the .dtb file at your end?

I normally use at my end the following command:
dtc -o no-chosen.dtb -I dts -O dtb no-chosen.dts

Thanks,
Bhupesh

Hi Bhupesh,
the issue is not with dtc, it is with kexec. The following script reproduces the issue:
#!/bin/sh -ve

uname -m

# aarch64

git clone 'https://git.kernel.org/pub/scm/utils/kernel/kexec/kexec-tools.git' kexec-tools-orig
cp -a kexec-tools-orig kexec-tools-test
git -C kexec-tools-orig checkout -b orig 'v2.0.18'
git -C kexec-tools-test checkout -b test 'v2.0.18'

cat patch.diff
#--- a/kexec/dt-ops.c
#+++ b/kexec/dt-ops.c
#@@ -84,7 +84,7 @@ int dtb_set_property(char **dtb, off_t *dtb_size, const char *node,
# if (nodeoffset == -FDT_ERR_NOTFOUND) {
# result = fdt_add_subnode(new_dtb, nodeoffset, node);
#
#- if (result) {
#+ if (result < 0) {
# dbgprintf("%s: fdt_add_subnode failed: %s\n", __func__,
# fdt_strerror(result));
# goto on_error;
patch -d kexec-tools-test -p1 < patch.diff

( cd kexec-tools-orig ; ./bootstrap )
( cd kexec-tools-test ; ./bootstrap )
( cd kexec-tools-orig ; ./configure )
( cd kexec-tools-test ; ./configure )
make -C kexec-tools-orig
make -C kexec-tools-test

wget 'http://mirror.archlinuxarm.org/aarch64/core/linux-aarch64-4.19.2-1-aarch64.pkg.tar.xz' bsdtar xf 'linux-aarch64-4.19.2-1-aarch64.pkg.tar.xz'

sudo kexec-tools-orig/build/sbin/kexec -d --load boot/Image --append 'debug' --dtb boot/dtbs/rockchip/rk3399-sapphire.dtb
# dtb_set_property: fdt_add_subnode failed: <valid offset/length> # kexec: Set device tree bootargs failed.

sudo kexec-tools-test/build/sbin/kexec -d --load boot/Image --append 'debug' --dtb boot/dtbs/rockchip/rk3399-sapphire.dtb
# dtb_set_property: fdt_setprop failed: FDT_ERR_BADOFFSET # kexec: Set device tree bootargs failed.

Regards,
Vicenç.

Quoted 23 lines
Quoted 23 lines

...
dtb_set_property: fdt_setprop failed: FDT_ERR_BADOFFSET
kexec: Set device tree bootargs failed.

Also tested this with the same result:

kexec/dt-ops.c +3 -1
@@ -84,11 +84,13 @@
        if (nodeoffset == -FDT_ERR_NOTFOUND) {
                result = fdt_add_subnode(new_dtb, nodeoffset, node);
 
-               if (result) {
+               if (result < 0) {
                        dbgprintf("%s: fdt_add_subnode failed: %s\n", __func__,
                                fdt_strerror(result));
                        goto on_error;
                }
+
+               nodeoffset = result;
        } else if (nodeoffset < 0) {
                dbgprintf("%s: fdt_path_offset failed: %s\n", __func__,
                        fdt_strerror(nodeoffset));
Quoted 2 lines
Quoted 2 lines

Regards,
Vicenç.

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

#14 lore raw

Re: [PATCH] kexec/dt-ops.c: Fix check against 'fdt_add_subnode' return value

Hi Vicenç,

Sorry I got busy and just got some spare time to look at this issue. I am able to fix it partially, but I am still not completely happy with it, but mainly here is the issue for the 'kexec load' option - we are passing wrong 'nodeoffset' while calling 'fdt_add_subnode()', which should be 0 instead of FDT_ERR_NOTFOUND when the chosen node is not seen in the passed dtb :

From 2433daf279744ae30fa9c74ec5b39507740d7163 Mon Sep 17 00:00:00 2001
From: Bhupesh Sharma <[email protected]>
Date: Thu, 29 Nov 2018 00:41:06 +0530
Subject: [PATCH] Add some debug logs

Signed-off-by Bhupesh Sharma <[email protected]>
---
 kexec/arch/arm64/kexec-arm64.c |  8 ++++++++
 kexec/dt-ops.c                 |  3 ++-
 kexec/libfdt/fdt_ro.c          | 14 +++++++-------
 kexec/libfdt/fdt_rw.c          |  2 ++
 4 files changed, 19 insertions(+), 8 deletions(-)
kexec/arch/arm64/kexec-arm64.c +0 -0
index 6c7d2512b1d4..e29afc30db43 100644
@@ -207,6 +207,9 @@ static int set_bootargs(struct dtb *dtb, const

char *command_line)

     if (!command_line || !command_line[0])
         return 0;

+ dbgprintf("%s: found %s\n", __func__, dtb->path);
+ dump_fdt(dtb->buf);
+

     result = dtb_set_bootargs(&dtb->buf, &dtb->size, command_line);

     if (result) {
@@ -413,6 +416,11 @@ static int setup_2nd_dtb(struct dtb *dtb, char
*command_line, int on_crash)
     }

     result = set_bootargs(dtb, command_line);
+    if (result) {
+        fprintf(stderr, "kexec: cannot set bootargs.\n");
+        result = -EINVAL;
+        goto on_error;

+ }

     /* determine #address-cells and #size-cells */
     result = get_cells_size(dtb->buf, &address_cells, &size_cells);
kexec/dt-ops.c +0 -0
index a0276f1e48b3..79b0404bafc4 100644
@@ -86,13 +86,14 @@ int dtb_set_property(char **dtb, off_t *dtb_size,

const char *node,

     nodeoffset = fdt_path_offset(new_dtb, node);

     if (nodeoffset == -FDT_ERR_NOTFOUND) {

- result = fdt_add_subnode(new_dtb, nodeoffset, node);
+ result = fdt_add_subnode(new_dtb, 0, node);

         if (result < 0) {
             dbgprintf("%s: fdt_add_subnode failed: %s\n", __func__,
                 fdt_strerror(result));
             goto on_error;
         }
+        nodeoffset = result;
     } else if (nodeoffset < 0) {
         dbgprintf("%s: fdt_path_offset failed: %s\n", __func__,
             fdt_strerror(nodeoffset));
kexec/libfdt/fdt_ro.c +0 -0
index 129b532bcc1a..0dce53083c96 100644
@@ -105,14 +105,14 @@ int fdt_subnode_offset_namelen(const void *fdt,

int offset,

     FDT_CHECK_HEADER(fdt);

     for (depth = 0;

- offset >= 0;
- offset = fdt_next_node(fdt, offset, &depth)) {
- if (depth < 0)
- return -FDT_ERR_NOTFOUND;
- else if ((depth == 1)
- && _fdt_nodename_eq(fdt, offset, name, namelen))
+ (offset >= 0) && (depth >= 0);
+ offset = fdt_next_node(fdt, offset, &depth))
+ if ((depth == 1)
+ && _fdt_nodename_eq(fdt, offset, name, namelen))

             return offset;

- }
+
+ if (depth < 0)
+ return -FDT_ERR_NOTFOUND;

return offset; /* error */
}

kexec/libfdt/fdt_rw.c +0 -0
index 8e7ec4cb7bcd..2fac7c0731c0 100644
@@ -101,6 +101,8 @@ static int _fdt_splice(void *fdt, void

*splicepoint, int oldlen, int newlen)

     if (((p + oldlen) < p) || ((p + oldlen) > end))
         return -FDT_ERR_BADOFFSET;
+    if ((p < (char *)fdt) || ((end - oldlen + newlen) < (char *)fdt))
+        return -FDT_ERR_BADOFFSET;
     if ((end - oldlen + newlen) > ((char *)fdt + fdt_totalsize(fdt)))
         return -FDT_ERR_NOSPACE;
     memmove(p + newlen, p + oldlen, end - p - oldlen);
Signature · 159 lines
2.7.4

Here are some logs at my end after this fix (I used some earlier work
I did for dump'ing dtb on arm64 boards to get the dtb logs:
<https://www.spinics.net/lists/kexec/msg20951.html>):

# kexec -d -l /root/Image --append 'debug' --dtb /root/rk3399-sapphire.dtb
<..snip..>

image_arm64_load: PE format:      yes
 / {
    compatible = "rockchip,rk3399-sapphire";
    interrupt-parent = <0x00000001>;
    #address-cells = <0x00000002>;
    #size-cells = <0x00000002>;
    model = "Sapphire-RK3399 Board";
    /chosen {
        bootargs = "debug";
    };
    aliases {

 <..snip..>
 };
get_cells_size: #address-cells:2 #size-cells:2
cells_size_fitted: 0-0
cells_size_fitted: 0-0
setup_2nd_dtb: no kaslr-seed found
dtb:    base 81ef0000, size d0a1h (53409)
sym: sha256_starts info: 12 other: 00 shndx: 1 value: ec0 size: 6c
sym: sha256_starts value: 81f00ec0 addr: 81f00018
machine_apply_elf_rel: CALL26 580006b394000000->580006b3940003aa
sym: sha256_update info: 12 other: 00 shndx: 1 value: 5168 size: c
sym: sha256_update value: 81f05168 addr: 81f00034
machine_apply_elf_rel: CALL26 9100427394000000->910042739400144d
sym: sha256_finish info: 12 other: 00 shndx: 1 value: 5174 size: 1cc
sym: sha256_finish value: 81f05174 addr: 81f00050
machine_apply_elf_rel: CALL26 aa1403e094000000->aa1403e094001449
sym:     memcmp info: 12 other: 00 shndx: 1 value: 64c size: 34
sym: memcmp value: 81f0064c addr: 81f00060
machine_apply_elf_rel: CALL26 340003c094000000->340003c09400017b
sym:     printf info: 12 other: 00 shndx: 1 value: 55c size: 90
sym: printf value: 81f0055c addr: 81f00070
machine_apply_elf_rel: CALL26 5800046094000000->580004609400013b
sym:     printf info: 12 other: 00 shndx: 1 value: 55c size: 90
sym: printf value: 81f0055c addr: 81f00078
machine_apply_elf_rel: CALL26 5800047594000000->5800047594000139
sym:     printf info: 12 other: 00 shndx: 1 value: 55c size: 90
sym: printf value: 81f0055c addr: 81f00088
machine_apply_elf_rel: CALL26 9100067394000000->9100067394000135
sym:     printf info: 12 other: 00 shndx: 1 value: 55c size: 90
sym: printf value: 81f0055c addr: 81f000a8
machine_apply_elf_rel: CALL26 5800036094000000->580003609400012d
sym:     printf info: 12 other: 00 shndx: 1 value: 55c size: 90
sym: printf value: 81f0055c addr: 81f000b0
machine_apply_elf_rel: CALL26 910402e194000000->910402e19400012b
sym:     printf info: 12 other: 00 shndx: 1 value: 55c size: 90
sym: printf value: 81f0055c addr: 81f000c0
machine_apply_elf_rel: CALL26 9100067394000000->9100067394000127
sym:     printf info: 12 other: 00 shndx: 1 value: 55c size: 90
sym: printf value: 81f0055c addr: 81f000d4
machine_apply_elf_rel: CALL26 5280002094000000->5280002094000122
sym:      .data info: 03 other: 00 shndx: 4 value: 0 size: 0
sym: .data value: 81f053b8 addr: 81f000f0
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f053b8
sym: .rodata.str1.1 info: 03 other: 00 shndx: 3 value: 0 size: 0
sym: .rodata.str1.1 value: 81f05348 addr: 81f000f8
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f05348
sym: .rodata.str1.1 info: 03 other: 00 shndx: 3 value: 0 size: 0
sym: .rodata.str1.1 value: 81f05368 addr: 81f00100
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f05368
sym: .rodata.str1.1 info: 03 other: 00 shndx: 3 value: 0 size: 0
sym: .rodata.str1.1 value: 81f05378 addr: 81f00108
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f05378
sym: .rodata.str1.1 info: 03 other: 00 shndx: 3 value: 0 size: 0
sym: .rodata.str1.1 value: 81f0537e addr: 81f00110
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f0537e
sym: .rodata.str1.1 info: 03 other: 00 shndx: 3 value: 0 size: 0
sym: .rodata.str1.1 value: 81f05380 addr: 81f00118
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f05380
sym:     printf info: 12 other: 00 shndx: 1 value: 55c size: 90
sym: printf value: 81f0055c addr: 81f0012c
machine_apply_elf_rel: CALL26 9400000094000000->940000009400010c
sym: setup_arch info: 12 other: 00 shndx: 1 value: eb8 size: 4
sym: setup_arch value: 81f00eb8 addr: 81f00130
machine_apply_elf_rel: CALL26 5800016094000000->5800016094000362
sym: verify_sha256_digest info: 12 other: 00 shndx: 1 value: 0 size: f0
sym: verify_sha256_digest value: 81f00000 addr: 81f00140
machine_apply_elf_rel: CALL26 3400004094000000->3400004097ffffb0
sym: post_verification_setup_arch info: 12 other: 00 shndx: 1 value: eb4 size: 4
sym: post_verification_setup_arch value: 81f00eb4 addr: 81f00150
machine_apply_elf_rel: JUMP26 d503201f14000000->d503201f14000359
sym: .rodata.str1.1 info: 03 other: 00 shndx: 3 value: 0 size: 0
sym: .rodata.str1.1 value: 81f05390 addr: 81f00158
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f05390
sym:      .data info: 03 other: 00 shndx: 4 value: 0 size: 0
sym: .data value: 81f053b8 addr: 81f00160
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f053b8
sym:    putchar info: 12 other: 00 shndx: 1 value: eb0 size: 4
sym: putchar value: 81f00eb0 addr: 81f001c4
machine_apply_elf_rel: CALL26 f94037a194000000->f94037a19400033b
sym:    putchar info: 12 other: 00 shndx: 1 value: eb0 size: 4
sym: putchar value: 81f00eb0 addr: 81f00238
machine_apply_elf_rel: CALL26 910006f794000000->910006f79400031e
sym:    putchar info: 12 other: 00 shndx: 1 value: eb0 size: 4
sym: putchar value: 81f00eb0 addr: 81f00490
machine_apply_elf_rel: CALL26 9100073994000000->9100073994000288
sym: .rodata.str1.1 info: 03 other: 00 shndx: 3 value: 0 size: 0
sym: .rodata.str1.1 value: 81f053a2 addr: 81f004d0
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f053a2
sym:   vsprintf info: 12 other: 00 shndx: 1 value: 168 size: 364
sym: vsprintf value: 81f00168 addr: 81f00550
machine_apply_elf_rel: CALL26 a8d07bfd94000000->a8d07bfd97ffff06
sym:   vsprintf info: 12 other: 00 shndx: 1 value: 168 size: 364
sym: vsprintf value: 81f00168 addr: 81f005e0
machine_apply_elf_rel: CALL26 a8d17bfd94000000->a8d17bfd97fffee2
sym:  purgatory info: 12 other: 00 shndx: 1 value: 120 size: 34
sym: purgatory value: 81f00120 addr: 81f00688
machine_apply_elf_rel: CALL26 5800001194000000->5800001197fffea6
sym: arm64_kernel_entry info: 10 other: 00 shndx: 4 value: 128 size: 8
sym: arm64_kernel_entry value: 81f054e0 addr: 81f0068c
machine_apply_elf_rel: LD_PREL_LO19 5800000058000011->58000000580272b1
sym: arm64_dtb_addr info: 10 other: 00 shndx: 4 value: 130 size: 8
sym: arm64_dtb_addr value: 81f054e8 addr: 81f00690
machine_apply_elf_rel: LD_PREL_LO19 aa1f03e158000000->aa1f03e1580272c0
sym: sha256_process info: 12 other: 00 shndx: 1 value: f2c size: 4134
sym: sha256_process value: 81f00f2c addr: 81f050cc
machine_apply_elf_rel: CALL26 d101029494000000->d101029497ffef98
sym:     memcpy info: 12 other: 00 shndx: 1 value: 62c size: 20
sym: memcpy value: 81f0062c addr: 81f05128
machine_apply_elf_rel: JUMP26 b4fffc5814000000->b4fffc5817ffed41
sym:     memcpy info: 12 other: 00 shndx: 1 value: 62c size: 20
sym: memcpy value: 81f0062c addr: 81f05140
machine_apply_elf_rel: CALL26 aa1503e094000000->aa1503e097ffed3b
sym: sha256_process info: 12 other: 00 shndx: 1 value: f2c size: 4134
sym: sha256_process value: 81f00f2c addr: 81f0514c
machine_apply_elf_rel: CALL26 cb1302d694000000->cb1302d697ffef78
sym:      .data info: 03 other: 00 shndx: 4 value: 0 size: 0
sym: .data value: 81f054f0 addr: 81f05340
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f054f0
kexec_load: entry = 0x81f00680 flags = 0xb70000
nr_segments = 3
segment[0].buf   = 0xffff8bab0010
segment[0].bufsz = 0x1999a00
segment[0].mem   = 0x80480000
segment[0].memsz = 0x1a70000
segment[1].buf   = 0x16984e10
segment[1].bufsz = 0xd0a1
segment[1].mem   = 0x81ef0000
segment[1].memsz = 0x10000
segment[2].buf   = 0x16992230
segment[2].bufsz = 0x5530
segment[2].mem   = 0x81f00000
segment[2].memsz = 0x10000

I will try to stabilize this more both for 'kexec load' and 'kexec
dump' use-cases and try to send a patch soon to fix this issue.

Regards,
Bhupesh
Quoted 149 lines · Vicente Bergas
Quoted 63 lines · Bhupesh Sharma

Hi Vicenç,

I tried the patch at my end and it works fine as per my checks. So, can I request you to provide some additional data which will help me understand your environment better. See inline:

Quoted 46 lines · Vicente Bergas
Quoted 44 lines · Simon Horman
Quoted 42 lines · Simon Horman
Quoted 40 lines · AKASHI Takahiro
Quoted 27 lines · Bhupesh Sharma

Vicenç reported (via [1]) that currently executing kexec with '--dtb' option and passing a .dtb which doesn't have a '/chosen' node leads to the following error:

  # kexec -d --dtb dtb_without_chosen_node.dtb --append 'cmdline' --load Image

dtb_set_property: fdt_add_subnode failed: <valid offset/length>
kexec: Set device tree bootargs failed.

This happens because currently we check the return value of 'fdt_add_subnode()' function call in 'dt-ops.c' incorrectly:

   result = fdt_add_subnode(new_dtb, nodeoffset, node);
   if (result) {
          dbgprintf("%s: fdt_add_subnode failed: %s\n", _func__,
                  fdt_strerror(result));
          goto on_error;
   }

As we can see in 'fdt_rw.c', a positive return value from 'fdt_add_subnode()' function doesn't indicate an error.

We can see that the Linux kernel (see 'drivers/firmware/efi/libstub/fdt.c' for example) also checks the 'fdt_add_subnode()' function against negative return values for errors. See an example below from 'update_fdt()' function in 'drivers/firmware/efi/libstub/fdt.c':

Quoted 8 lines
   node = fdt_add_subnode(fdt, 0, "chosen");
   if (node < 0) {
          status = node;
          <..snip..>
          goto fdt_set_fail;
   }

This patch fixes the same in 'kexec-tools'.

Looks good.
Thank you.
-Takahiro Akashi

Quoted 8 lines
Quoted 5 lines
Quoted 1 line

Thanks, applied.

Perhaps I was a little too hasty there, I have dropped the patch for now.

Vicente, does this patch work for you?

The patch looks fine, but the end result is still non-working:

  # kexec -d --load Image --append 'debug' --dtb no_chosen.dtb
Quoted 8 lines

Can you share the .dts with which you see this issue and steps you use to create the .dtb file at your end?

I normally use at my end the following command:
dtc -o no-chosen.dtb -I dts -O dtb no-chosen.dts

Thanks,
Bhupesh

Hi Bhupesh,
the issue is not with dtc, it is with kexec. The following script reproduces the issue:
#!/bin/sh -ve

uname -m

# aarch64

git clone 'https://git.kernel.org/pub/scm/utils/kernel/kexec/kexec-tools.git' kexec-tools-orig
cp -a kexec-tools-orig kexec-tools-test
git -C kexec-tools-orig checkout -b orig 'v2.0.18'
git -C kexec-tools-test checkout -b test 'v2.0.18'

cat patch.diff
#--- a/kexec/dt-ops.c
#+++ b/kexec/dt-ops.c
#@@ -84,7 +84,7 @@ int dtb_set_property(char **dtb, off_t *dtb_size, const char *node,
# if (nodeoffset == -FDT_ERR_NOTFOUND) {
# result = fdt_add_subnode(new_dtb, nodeoffset, node);
#
#- if (result) {
#+ if (result < 0) {
# dbgprintf("%s: fdt_add_subnode failed: %s\n", __func__,
# fdt_strerror(result));
# goto on_error;
patch -d kexec-tools-test -p1 < patch.diff

( cd kexec-tools-orig ; ./bootstrap )
( cd kexec-tools-test ; ./bootstrap )
( cd kexec-tools-orig ; ./configure )
( cd kexec-tools-test ; ./configure )
make -C kexec-tools-orig
make -C kexec-tools-test

wget 'http://mirror.archlinuxarm.org/aarch64/core/linux-aarch64-4.19.2-1-aarch64.pkg.tar.xz' bsdtar xf 'linux-aarch64-4.19.2-1-aarch64.pkg.tar.xz'

sudo kexec-tools-orig/build/sbin/kexec -d --load boot/Image --append 'debug' --dtb boot/dtbs/rockchip/rk3399-sapphire.dtb
# dtb_set_property: fdt_add_subnode failed: <valid offset/length> # kexec: Set device tree bootargs failed.

sudo kexec-tools-test/build/sbin/kexec -d --load boot/Image --append 'debug' --dtb boot/dtbs/rockchip/rk3399-sapphire.dtb
# dtb_set_property: fdt_setprop failed: FDT_ERR_BADOFFSET # kexec: Set device tree bootargs failed.

Regards,
Vicenç.

Quoted 23 lines
Quoted 23 lines

...
dtb_set_property: fdt_setprop failed: FDT_ERR_BADOFFSET
kexec: Set device tree bootargs failed.

Also tested this with the same result:

kexec/dt-ops.c +3 -1
@@ -84,11 +84,13 @@
        if (nodeoffset == -FDT_ERR_NOTFOUND) {
                result = fdt_add_subnode(new_dtb, nodeoffset, node);
 
-               if (result) {
+               if (result < 0) {
                        dbgprintf("%s: fdt_add_subnode failed: %s\n", __func__,
                                fdt_strerror(result));
                        goto on_error;
                }
+
+               nodeoffset = result;
        } else if (nodeoffset < 0) {
                dbgprintf("%s: fdt_path_offset failed: %s\n", __func__,
                        fdt_strerror(nodeoffset));
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

Regards,
Vicenç.

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

#15 lore raw

Re: [PATCH] kexec/dt-ops.c: Fix check against 'fdt_add_subnode' return value

Hi Vicenç,

I have posted a revised and more comprehensive patchset for this issue today (please see the same here:
<http://lists.infradead.org/pipermail/kexec/2018-November/021999.html>)

I was able to exercise all the basic 'kexe load' and 'kdump' use cases on arm64 after these changes and I can also get the use-cases where we don't have a '/chosen' node in the .dtb to either PASS or FAIL with correct error reporting. Here is a listing of the basic use cases and their results:

# kexec -d -l Image --append=debug --dtb rk3399-sapphire.dtb
ok
# kexec -d -l Image --reuse-cmdline --initrd=/boot/initramfs-`uname -r`.img
ok
# kexec -d -p Image --reuse-cmdline --initrd=/boot/initramfs-`uname -r`.img
ok
# kexec -d -p Image --append=debug --dtb rk3399-sapphire.dtb
<..snip..>
setup_2nd_dtb: /chosen node not found - can't create dtb for dump
kernel: FDT_ERR_NOTFOUND
kexec: setup_2nd_dtb failed.
kexec: load failed.
Cannot load Image

Can you please try and test this patchset on your setup and let me know in case you land in any further issues?

Thanks,
Bhupesh

Quoted 423 lines · Bhupesh Sharma

Hi Vicenç,

Sorry I got busy and just got some spare time to look at this issue. I am able to fix it partially, but I am still not completely happy with it, but mainly here is the issue for the 'kexec load' option - we are passing wrong 'nodeoffset' while calling 'fdt_add_subnode()', which should be 0 instead of FDT_ERR_NOTFOUND when the chosen node is not seen in the passed dtb :

From 2433daf279744ae30fa9c74ec5b39507740d7163 Mon Sep 17 00:00:00 2001
From: Bhupesh Sharma <[email protected]>
Date: Thu, 29 Nov 2018 00:41:06 +0530
Subject: [PATCH] Add some debug logs

Signed-off-by Bhupesh Sharma <[email protected]>
---
 kexec/arch/arm64/kexec-arm64.c |  8 ++++++++
 kexec/dt-ops.c                 |  3 ++-
 kexec/libfdt/fdt_ro.c          | 14 +++++++-------
 kexec/libfdt/fdt_rw.c          |  2 ++
 4 files changed, 19 insertions(+), 8 deletions(-)
kexec/arch/arm64/kexec-arm64.c +3 -0
index 6c7d2512b1d4..e29afc30db43 100644
@@ -207,6 +207,9 @@ static int set_bootargs(struct dtb *dtb, const
 char *command_line)
     if (!command_line || !command_line[0])
         return 0;
 
+    dbgprintf("%s: found %s\n", __func__, dtb->path);
+    dump_fdt(dtb->buf);
+
     result = dtb_set_bootargs(&dtb->buf, &dtb->size, command_line);
 

     if (result) {
kexec/arch/arm64/kexec-arm64.c +5 -0
@@ -413,6 +416,11 @@ static int setup_2nd_dtb(struct dtb *dtb, char
 *command_line, int on_crash)
     }
 
     result = set_bootargs(dtb, command_line);
+    if (result) {
+        fprintf(stderr, "kexec: cannot set bootargs.\n");
+        result = -EINVAL;
+        goto on_error;
+    }
 
     /* determine #address-cells and #size-cells */

     result = get_cells_size(dtb->buf, &address_cells, &size_cells);
kexec/dt-ops.c +2 -1
index a0276f1e48b3..79b0404bafc4 100644
@@ -86,13 +86,14 @@ int dtb_set_property(char **dtb, off_t *dtb_size,
 const char *node,
     nodeoffset = fdt_path_offset(new_dtb, node);
 
     if (nodeoffset == -FDT_ERR_NOTFOUND) {
-        result = fdt_add_subnode(new_dtb, nodeoffset, node);
+        result = fdt_add_subnode(new_dtb, 0, node);
 
         if (result < 0) {
             dbgprintf("%s: fdt_add_subnode failed: %s\n", __func__,
                 fdt_strerror(result));
             goto on_error;
         }
+        nodeoffset = result;
     } else if (nodeoffset < 0) {
         dbgprintf("%s: fdt_path_offset failed: %s\n", __func__,

             fdt_strerror(nodeoffset));
kexec/libfdt/fdt_ro.c +7 -7
index 129b532bcc1a..0dce53083c96 100644
@@ -105,14 +105,14 @@ int fdt_subnode_offset_namelen(const void *fdt,
 int offset,
     FDT_CHECK_HEADER(fdt);
 
     for (depth = 0;
-         offset >= 0;
-         offset = fdt_next_node(fdt, offset, &depth)) {
-        if (depth < 0)
-            return -FDT_ERR_NOTFOUND;
-        else if ((depth == 1)
-             && _fdt_nodename_eq(fdt, offset, name, namelen))
+         (offset >= 0) && (depth >= 0);
+         offset = fdt_next_node(fdt, offset, &depth))
+        if ((depth == 1)
+            && _fdt_nodename_eq(fdt, offset, name, namelen))
             return offset;
-    }
+
+    if (depth < 0)
+        return -FDT_ERR_NOTFOUND;
 
     return offset; /* error */

}

kexec/libfdt/fdt_rw.c +2 -0
index 8e7ec4cb7bcd..2fac7c0731c0 100644
@@ -101,6 +101,8 @@ static int _fdt_splice(void *fdt, void
 *splicepoint, int oldlen, int newlen)
 
     if (((p + oldlen) < p) || ((p + oldlen) > end))
         return -FDT_ERR_BADOFFSET;
+    if ((p < (char *)fdt) || ((end - oldlen + newlen) < (char *)fdt))
+        return -FDT_ERR_BADOFFSET;
     if ((end - oldlen + newlen) > ((char *)fdt + fdt_totalsize(fdt)))
         return -FDT_ERR_NOSPACE;

     memmove(p + newlen, p + oldlen, end - p - oldlen);

--
2.7.4

Here are some logs at my end after this fix (I used some earlier work I did for dump'ing dtb on arm64 boards to get the dtb logs:
<https://www.spinics.net/lists/kexec/msg20951.html>):

# kexec -d -l /root/Image --append 'debug' --dtb /root/rk3399-sapphire.dtb
<..snip..>

image_arm64_load: PE format: yes

 / {
    compatible = "rockchip,rk3399-sapphire";
    interrupt-parent = <0x00000001>;
    #address-cells = <0x00000002>;
    #size-cells = <0x00000002>;
    model = "Sapphire-RK3399 Board";
    /chosen {
        bootargs = "debug";
    };
    aliases {

<..snip..>
};

get_cells_size: #address-cells:2 #size-cells:2
cells_size_fitted: 0-0
cells_size_fitted: 0-0
setup_2nd_dtb: no kaslr-seed found
dtb:    base 81ef0000, size d0a1h (53409)
sym: sha256_starts info: 12 other: 00 shndx: 1 value: ec0 size: 6c
sym: sha256_starts value: 81f00ec0 addr: 81f00018
machine_apply_elf_rel: CALL26 580006b394000000->580006b3940003aa
sym: sha256_update info: 12 other: 00 shndx: 1 value: 5168 size: c
sym: sha256_update value: 81f05168 addr: 81f00034
machine_apply_elf_rel: CALL26 9100427394000000->910042739400144d
sym: sha256_finish info: 12 other: 00 shndx: 1 value: 5174 size: 1cc
sym: sha256_finish value: 81f05174 addr: 81f00050
machine_apply_elf_rel: CALL26 aa1403e094000000->aa1403e094001449
sym:     memcmp info: 12 other: 00 shndx: 1 value: 64c size: 34
sym: memcmp value: 81f0064c addr: 81f00060
machine_apply_elf_rel: CALL26 340003c094000000->340003c09400017b
sym:     printf info: 12 other: 00 shndx: 1 value: 55c size: 90
sym: printf value: 81f0055c addr: 81f00070
machine_apply_elf_rel: CALL26 5800046094000000->580004609400013b
sym:     printf info: 12 other: 00 shndx: 1 value: 55c size: 90
sym: printf value: 81f0055c addr: 81f00078
machine_apply_elf_rel: CALL26 5800047594000000->5800047594000139
sym:     printf info: 12 other: 00 shndx: 1 value: 55c size: 90
sym: printf value: 81f0055c addr: 81f00088
machine_apply_elf_rel: CALL26 9100067394000000->9100067394000135
sym:     printf info: 12 other: 00 shndx: 1 value: 55c size: 90
sym: printf value: 81f0055c addr: 81f000a8
machine_apply_elf_rel: CALL26 5800036094000000->580003609400012d
sym:     printf info: 12 other: 00 shndx: 1 value: 55c size: 90
sym: printf value: 81f0055c addr: 81f000b0
machine_apply_elf_rel: CALL26 910402e194000000->910402e19400012b
sym:     printf info: 12 other: 00 shndx: 1 value: 55c size: 90
sym: printf value: 81f0055c addr: 81f000c0
machine_apply_elf_rel: CALL26 9100067394000000->9100067394000127
sym:     printf info: 12 other: 00 shndx: 1 value: 55c size: 90
sym: printf value: 81f0055c addr: 81f000d4
machine_apply_elf_rel: CALL26 5280002094000000->5280002094000122
sym:      .data info: 03 other: 00 shndx: 4 value: 0 size: 0
sym: .data value: 81f053b8 addr: 81f000f0
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f053b8
sym: .rodata.str1.1 info: 03 other: 00 shndx: 3 value: 0 size: 0
sym: .rodata.str1.1 value: 81f05348 addr: 81f000f8
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f05348
sym: .rodata.str1.1 info: 03 other: 00 shndx: 3 value: 0 size: 0
sym: .rodata.str1.1 value: 81f05368 addr: 81f00100
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f05368
sym: .rodata.str1.1 info: 03 other: 00 shndx: 3 value: 0 size: 0
sym: .rodata.str1.1 value: 81f05378 addr: 81f00108
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f05378
sym: .rodata.str1.1 info: 03 other: 00 shndx: 3 value: 0 size: 0
sym: .rodata.str1.1 value: 81f0537e addr: 81f00110
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f0537e
sym: .rodata.str1.1 info: 03 other: 00 shndx: 3 value: 0 size: 0
sym: .rodata.str1.1 value: 81f05380 addr: 81f00118
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f05380
sym:     printf info: 12 other: 00 shndx: 1 value: 55c size: 90
sym: printf value: 81f0055c addr: 81f0012c
machine_apply_elf_rel: CALL26 9400000094000000->940000009400010c
sym: setup_arch info: 12 other: 00 shndx: 1 value: eb8 size: 4
sym: setup_arch value: 81f00eb8 addr: 81f00130
machine_apply_elf_rel: CALL26 5800016094000000->5800016094000362
sym: verify_sha256_digest info: 12 other: 00 shndx: 1 value: 0 size: f0
sym: verify_sha256_digest value: 81f00000 addr: 81f00140
machine_apply_elf_rel: CALL26 3400004094000000->3400004097ffffb0
sym: post_verification_setup_arch info: 12 other: 00 shndx: 1 value: eb4 size: 4
sym: post_verification_setup_arch value: 81f00eb4 addr: 81f00150
machine_apply_elf_rel: JUMP26 d503201f14000000->d503201f14000359
sym: .rodata.str1.1 info: 03 other: 00 shndx: 3 value: 0 size: 0
sym: .rodata.str1.1 value: 81f05390 addr: 81f00158
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f05390
sym:      .data info: 03 other: 00 shndx: 4 value: 0 size: 0
sym: .data value: 81f053b8 addr: 81f00160
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f053b8
sym:    putchar info: 12 other: 00 shndx: 1 value: eb0 size: 4
sym: putchar value: 81f00eb0 addr: 81f001c4
machine_apply_elf_rel: CALL26 f94037a194000000->f94037a19400033b
sym:    putchar info: 12 other: 00 shndx: 1 value: eb0 size: 4
sym: putchar value: 81f00eb0 addr: 81f00238
machine_apply_elf_rel: CALL26 910006f794000000->910006f79400031e
sym:    putchar info: 12 other: 00 shndx: 1 value: eb0 size: 4
sym: putchar value: 81f00eb0 addr: 81f00490
machine_apply_elf_rel: CALL26 9100073994000000->9100073994000288
sym: .rodata.str1.1 info: 03 other: 00 shndx: 3 value: 0 size: 0
sym: .rodata.str1.1 value: 81f053a2 addr: 81f004d0
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f053a2
sym:   vsprintf info: 12 other: 00 shndx: 1 value: 168 size: 364
sym: vsprintf value: 81f00168 addr: 81f00550
machine_apply_elf_rel: CALL26 a8d07bfd94000000->a8d07bfd97ffff06
sym:   vsprintf info: 12 other: 00 shndx: 1 value: 168 size: 364
sym: vsprintf value: 81f00168 addr: 81f005e0
machine_apply_elf_rel: CALL26 a8d17bfd94000000->a8d17bfd97fffee2
sym:  purgatory info: 12 other: 00 shndx: 1 value: 120 size: 34
sym: purgatory value: 81f00120 addr: 81f00688
machine_apply_elf_rel: CALL26 5800001194000000->5800001197fffea6
sym: arm64_kernel_entry info: 10 other: 00 shndx: 4 value: 128 size: 8
sym: arm64_kernel_entry value: 81f054e0 addr: 81f0068c
machine_apply_elf_rel: LD_PREL_LO19 5800000058000011->58000000580272b1
sym: arm64_dtb_addr info: 10 other: 00 shndx: 4 value: 130 size: 8
sym: arm64_dtb_addr value: 81f054e8 addr: 81f00690
machine_apply_elf_rel: LD_PREL_LO19 aa1f03e158000000->aa1f03e1580272c0
sym: sha256_process info: 12 other: 00 shndx: 1 value: f2c size: 4134
sym: sha256_process value: 81f00f2c addr: 81f050cc
machine_apply_elf_rel: CALL26 d101029494000000->d101029497ffef98
sym:     memcpy info: 12 other: 00 shndx: 1 value: 62c size: 20
sym: memcpy value: 81f0062c addr: 81f05128
machine_apply_elf_rel: JUMP26 b4fffc5814000000->b4fffc5817ffed41
sym:     memcpy info: 12 other: 00 shndx: 1 value: 62c size: 20
sym: memcpy value: 81f0062c addr: 81f05140
machine_apply_elf_rel: CALL26 aa1503e094000000->aa1503e097ffed3b
sym: sha256_process info: 12 other: 00 shndx: 1 value: f2c size: 4134
sym: sha256_process value: 81f00f2c addr: 81f0514c
machine_apply_elf_rel: CALL26 cb1302d694000000->cb1302d697ffef78
sym:      .data info: 03 other: 00 shndx: 4 value: 0 size: 0
sym: .data value: 81f054f0 addr: 81f05340
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f054f0
kexec_load: entry = 0x81f00680 flags = 0xb70000

nr_segments = 3
segment[0].buf = 0xffff8bab0010
segment[0].bufsz = 0x1999a00
segment[0].mem = 0x80480000
segment[0].memsz = 0x1a70000
segment[1].buf = 0x16984e10
segment[1].bufsz = 0xd0a1
segment[1].mem = 0x81ef0000
segment[1].memsz = 0x10000
segment[2].buf = 0x16992230
segment[2].bufsz = 0x5530
segment[2].mem = 0x81f00000
segment[2].memsz = 0x10000

I will try to stabilize this more both for 'kexec load' and 'kexec dump' use-cases and try to send a patch soon to fix this issue.

Regards,
Bhupesh

Quoted 149 lines · Vicente Bergas
Quoted 63 lines · Bhupesh Sharma

Hi Vicenç,

I tried the patch at my end and it works fine as per my checks. So, can I request you to provide some additional data which will help me understand your environment better. See inline:

Quoted 46 lines · Vicente Bergas
Quoted 44 lines · Simon Horman
Quoted 42 lines · Simon Horman
Quoted 40 lines · AKASHI Takahiro
Quoted 27 lines · Bhupesh Sharma

Vicenç reported (via [1]) that currently executing kexec with '--dtb' option and passing a .dtb which doesn't have a '/chosen' node leads to the following error:

  # kexec -d --dtb dtb_without_chosen_node.dtb --append 'cmdline' --load Image

dtb_set_property: fdt_add_subnode failed: <valid offset/length>
kexec: Set device tree bootargs failed.

This happens because currently we check the return value of 'fdt_add_subnode()' function call in 'dt-ops.c' incorrectly:

   result = fdt_add_subnode(new_dtb, nodeoffset, node);
   if (result) {
          dbgprintf("%s: fdt_add_subnode failed: %s\n", _func__,
                  fdt_strerror(result));
          goto on_error;
   }

As we can see in 'fdt_rw.c', a positive return value from 'fdt_add_subnode()' function doesn't indicate an error.

We can see that the Linux kernel (see 'drivers/firmware/efi/libstub/fdt.c' for example) also checks the 'fdt_add_subnode()' function against negative return values for errors. See an example below from 'update_fdt()' function in 'drivers/firmware/efi/libstub/fdt.c':

Quoted 8 lines
   node = fdt_add_subnode(fdt, 0, "chosen");
   if (node < 0) {
          status = node;
          <..snip..>
          goto fdt_set_fail;
   }

This patch fixes the same in 'kexec-tools'.

Looks good.
Thank you.
-Takahiro Akashi

Quoted 8 lines
Quoted 5 lines
Quoted 1 line

Thanks, applied.

Perhaps I was a little too hasty there, I have dropped the patch for now.

Vicente, does this patch work for you?

The patch looks fine, but the end result is still non-working:

  # kexec -d --load Image --append 'debug' --dtb no_chosen.dtb
Quoted 8 lines

Can you share the .dts with which you see this issue and steps you use to create the .dtb file at your end?

I normally use at my end the following command:
dtc -o no-chosen.dtb -I dts -O dtb no-chosen.dts

Thanks,
Bhupesh

Hi Bhupesh,
the issue is not with dtc, it is with kexec. The following script reproduces the issue:
#!/bin/sh -ve

uname -m

# aarch64

git clone 'https://git.kernel.org/pub/scm/utils/kernel/kexec/kexec-tools.git' kexec-tools-orig
cp -a kexec-tools-orig kexec-tools-test
git -C kexec-tools-orig checkout -b orig 'v2.0.18'
git -C kexec-tools-test checkout -b test 'v2.0.18'

cat patch.diff
#--- a/kexec/dt-ops.c
#+++ b/kexec/dt-ops.c
#@@ -84,7 +84,7 @@ int dtb_set_property(char **dtb, off_t *dtb_size, const char *node,
# if (nodeoffset == -FDT_ERR_NOTFOUND) {
# result = fdt_add_subnode(new_dtb, nodeoffset, node);
#
#- if (result) {
#+ if (result < 0) {
# dbgprintf("%s: fdt_add_subnode failed: %s\n", __func__,
# fdt_strerror(result));
# goto on_error;
patch -d kexec-tools-test -p1 < patch.diff

( cd kexec-tools-orig ; ./bootstrap )
( cd kexec-tools-test ; ./bootstrap )
( cd kexec-tools-orig ; ./configure )
( cd kexec-tools-test ; ./configure )
make -C kexec-tools-orig
make -C kexec-tools-test

wget 'http://mirror.archlinuxarm.org/aarch64/core/linux-aarch64-4.19.2-1-aarch64.pkg.tar.xz' bsdtar xf 'linux-aarch64-4.19.2-1-aarch64.pkg.tar.xz'

sudo kexec-tools-orig/build/sbin/kexec -d --load boot/Image --append 'debug' --dtb boot/dtbs/rockchip/rk3399-sapphire.dtb
# dtb_set_property: fdt_add_subnode failed: <valid offset/length> # kexec: Set device tree bootargs failed.

sudo kexec-tools-test/build/sbin/kexec -d --load boot/Image --append 'debug' --dtb boot/dtbs/rockchip/rk3399-sapphire.dtb
# dtb_set_property: fdt_setprop failed: FDT_ERR_BADOFFSET # kexec: Set device tree bootargs failed.

Regards,
Vicenç.

Quoted 23 lines
Quoted 23 lines

...
dtb_set_property: fdt_setprop failed: FDT_ERR_BADOFFSET
kexec: Set device tree bootargs failed.

Also tested this with the same result:

kexec/dt-ops.c +3 -1
@@ -84,11 +84,13 @@
        if (nodeoffset == -FDT_ERR_NOTFOUND) {
                result = fdt_add_subnode(new_dtb, nodeoffset, node);
 
-               if (result) {
+               if (result < 0) {
                        dbgprintf("%s: fdt_add_subnode failed: %s\n", __func__,
                                fdt_strerror(result));
                        goto on_error;
                }
+
+               nodeoffset = result;
        } else if (nodeoffset < 0) {
                dbgprintf("%s: fdt_path_offset failed: %s\n", __func__,
                        fdt_strerror(nodeoffset));
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

Regards,
Vicenç.

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

author
#16 lore raw

Re: [PATCH] kexec/dt-ops.c: Fix check against 'fdt_add_subnode' return value

Quoted 30 lines · Bhupesh Sharma

Hi Vicenç,

I have posted a revised and more comprehensive patchset for this issue today (please see the same here:
<http://lists.infradead.org/pipermail/kexec/2018-November/021999.html>)

I was able to exercise all the basic 'kexe load' and 'kdump' use cases on arm64 after these changes and I can also get the use-cases where we don't have a '/chosen' node in the .dtb to either PASS or FAIL with correct error reporting. Here is a listing of the basic use cases and their results:

# kexec -d -l Image --append=debug --dtb rk3399-sapphire.dtb
ok
# kexec -d -l Image --reuse-cmdline --initrd=/boot/initramfs-`uname -r`.img
ok
# kexec -d -p Image --reuse-cmdline --initrd=/boot/initramfs-`uname -r`.img
ok
# kexec -d -p Image --append=debug --dtb rk3399-sapphire.dtb
<..snip..>
setup_2nd_dtb: /chosen node not found - can't create dtb for dump
kernel: FDT_ERR_NOTFOUND
kexec: setup_2nd_dtb failed.
kexec: load failed.
Cannot load Image
Quoted 2 lines

Can you please try and test this patchset on your setup and let me know in case you land in any further issues?

Hi Bhupesh,
thank you, this is an improvement.
Unfortunately there is a remaining issue with the initrd option and without the reuse-cmdline option:
kexec -d -l /boot/Image --dtb /boot/dtbs/rockchip/rk3399-sapphire.dtb --initrd /boot/initramfs-linux.img
...
get_cells_size: #address-cells:2 #size-cells:2
cells_size_fitted: 0-0
cells_size_fitted: 0-0
setup_2nd_dtb: no kaslr-seed found
initrd: base 1d36000, size 5b3d3dh (5979453)
dtb_set_initrd: start 30629888, end 36609341, size 5979453 (5839 KiB)
dtb_set_property: fdt_add_subnode failed: FDT_ERR_EXISTS
dtb_delete_property: fdt_path_offset failed: FDT_ERR_NOTFOUND
kexec: load failed.
Cannot load /boot/Image

In any case, do not worry and take your time, this is not a blocker issue. Regards,
Vicenç.

Quoted 428 lines

Thanks,
Bhupesh

Quoted 423 lines · Bhupesh Sharma

Hi Vicenç,

Sorry I got busy and just got some spare time to look at this issue. I am able to fix it partially, but I am still not completely happy with it, but mainly here is the issue for the 'kexec load' option - we are passing wrong 'nodeoffset' while calling 'fdt_add_subnode()', which should be 0 instead of FDT_ERR_NOTFOUND when the chosen node is not seen in the passed dtb :

From 2433daf279744ae30fa9c74ec5b39507740d7163 Mon Sep 17 00:00:00 2001
From: Bhupesh Sharma <[email protected]>
Date: Thu, 29 Nov 2018 00:41:06 +0530
Subject: [PATCH] Add some debug logs

Signed-off-by Bhupesh Sharma <[email protected]>
---
 kexec/arch/arm64/kexec-arm64.c |  8 ++++++++
 kexec/dt-ops.c                 |  3 ++-
 kexec/libfdt/fdt_ro.c          | 14 +++++++-------
 kexec/libfdt/fdt_rw.c          |  2 ++
 4 files changed, 19 insertions(+), 8 deletions(-)
kexec/arch/arm64/kexec-arm64.c +3 -0
index 6c7d2512b1d4..e29afc30db43 100644
@@ -207,6 +207,9 @@ static int set_bootargs(struct dtb *dtb, const
 char *command_line)
     if (!command_line || !command_line[0])
         return 0;
 
+    dbgprintf("%s: found %s\n", __func__, dtb->path);
+    dump_fdt(dtb->buf);
+
     result = dtb_set_bootargs(&dtb->buf, &dtb->size, command_line);
 

     if (result) {
kexec/arch/arm64/kexec-arm64.c +5 -0
@@ -413,6 +416,11 @@ static int setup_2nd_dtb(struct dtb *dtb, char
 *command_line, int on_crash)
     }
 
     result = set_bootargs(dtb, command_line);
+    if (result) {
+        fprintf(stderr, "kexec: cannot set bootargs.\n");
+        result = -EINVAL;
+        goto on_error;
+    }
 
     /* determine #address-cells and #size-cells */

     result = get_cells_size(dtb->buf, &address_cells, &size_cells);
kexec/dt-ops.c +2 -1
index a0276f1e48b3..79b0404bafc4 100644
@@ -86,13 +86,14 @@ int dtb_set_property(char **dtb, off_t *dtb_size,
 const char *node,
     nodeoffset = fdt_path_offset(new_dtb, node);
 
     if (nodeoffset == -FDT_ERR_NOTFOUND) {
-        result = fdt_add_subnode(new_dtb, nodeoffset, node);
+        result = fdt_add_subnode(new_dtb, 0, node);
 
         if (result < 0) {
             dbgprintf("%s: fdt_add_subnode failed: %s\n", __func__,
                 fdt_strerror(result));
             goto on_error;
         }
+        nodeoffset = result;
     } else if (nodeoffset < 0) {
         dbgprintf("%s: fdt_path_offset failed: %s\n", __func__,

             fdt_strerror(nodeoffset));
kexec/libfdt/fdt_ro.c +7 -7
index 129b532bcc1a..0dce53083c96 100644
@@ -105,14 +105,14 @@ int fdt_subnode_offset_namelen(const void *fdt,
 int offset,
     FDT_CHECK_HEADER(fdt);
 
     for (depth = 0;
-         offset >= 0;
-         offset = fdt_next_node(fdt, offset, &depth)) {
-        if (depth < 0)
-            return -FDT_ERR_NOTFOUND;
-        else if ((depth == 1)
-             && _fdt_nodename_eq(fdt, offset, name, namelen))
+         (offset >= 0) && (depth >= 0);
+         offset = fdt_next_node(fdt, offset, &depth))
+        if ((depth == 1)
+            && _fdt_nodename_eq(fdt, offset, name, namelen))
             return offset;
-    }
+
+    if (depth < 0)
+        return -FDT_ERR_NOTFOUND;
 
     return offset; /* error */

}

kexec/libfdt/fdt_rw.c +2 -0
index 8e7ec4cb7bcd..2fac7c0731c0 100644
@@ -101,6 +101,8 @@ static int _fdt_splice(void *fdt, void
 *splicepoint, int oldlen, int newlen)
 
     if (((p + oldlen) < p) || ((p + oldlen) > end))
         return -FDT_ERR_BADOFFSET;
+    if ((p < (char *)fdt) || ((end - oldlen + newlen) < (char *)fdt))
+        return -FDT_ERR_BADOFFSET;
     if ((end - oldlen + newlen) > ((char *)fdt + fdt_totalsize(fdt)))
         return -FDT_ERR_NOSPACE;

     memmove(p + newlen, p + oldlen, end - p - oldlen);

--
2.7.4

Here are some logs at my end after this fix (I used some earlier work I did for dump'ing dtb on arm64 boards to get the dtb logs:
<https://www.spinics.net/lists/kexec/msg20951.html>):

# kexec -d -l /root/Image --append 'debug' --dtb /root/rk3399-sapphire.dtb
<..snip..>

image_arm64_load: PE format: yes

 / {
    compatible = "rockchip,rk3399-sapphire";
    interrupt-parent = <0x00000001>;
    #address-cells = <0x00000002>;
    #size-cells = <0x00000002>;
    model = "Sapphire-RK3399 Board";
    /chosen {
        bootargs = "debug";
    };
    aliases {

<..snip..>
};

get_cells_size: #address-cells:2 #size-cells:2
cells_size_fitted: 0-0
cells_size_fitted: 0-0
setup_2nd_dtb: no kaslr-seed found
dtb:    base 81ef0000, size d0a1h (53409)
sym: sha256_starts info: 12 other: 00 shndx: 1 value: ec0 size: 6c
sym: sha256_starts value: 81f00ec0 addr: 81f00018
machine_apply_elf_rel: CALL26 580006b394000000->580006b3940003aa
sym: sha256_update info: 12 other: 00 shndx: 1 value: 5168 size: c
sym: sha256_update value: 81f05168 addr: 81f00034
machine_apply_elf_rel: CALL26 9100427394000000->910042739400144d
sym: sha256_finish info: 12 other: 00 shndx: 1 value: 5174 size: 1cc
sym: sha256_finish value: 81f05174 addr: 81f00050
machine_apply_elf_rel: CALL26 aa1403e094000000->aa1403e094001449
sym:     memcmp info: 12 other: 00 shndx: 1 value: 64c size: 34
sym: memcmp value: 81f0064c addr: 81f00060
machine_apply_elf_rel: CALL26 340003c094000000->340003c09400017b
sym:     printf info: 12 other: 00 shndx: 1 value: 55c size: 90
sym: printf value: 81f0055c addr: 81f00070
machine_apply_elf_rel: CALL26 5800046094000000->580004609400013b
sym:     printf info: 12 other: 00 shndx: 1 value: 55c size: 90
sym: printf value: 81f0055c addr: 81f00078
machine_apply_elf_rel: CALL26 5800047594000000->5800047594000139
sym:     printf info: 12 other: 00 shndx: 1 value: 55c size: 90
sym: printf value: 81f0055c addr: 81f00088
machine_apply_elf_rel: CALL26 9100067394000000->9100067394000135
sym:     printf info: 12 other: 00 shndx: 1 value: 55c size: 90
sym: printf value: 81f0055c addr: 81f000a8
machine_apply_elf_rel: CALL26 5800036094000000->580003609400012d
sym:     printf info: 12 other: 00 shndx: 1 value: 55c size: 90
sym: printf value: 81f0055c addr: 81f000b0
machine_apply_elf_rel: CALL26 910402e194000000->910402e19400012b
sym:     printf info: 12 other: 00 shndx: 1 value: 55c size: 90
sym: printf value: 81f0055c addr: 81f000c0
machine_apply_elf_rel: CALL26 9100067394000000->9100067394000127
sym:     printf info: 12 other: 00 shndx: 1 value: 55c size: 90
sym: printf value: 81f0055c addr: 81f000d4
machine_apply_elf_rel: CALL26 5280002094000000->5280002094000122
sym:      .data info: 03 other: 00 shndx: 4 value: 0 size: 0
sym: .data value: 81f053b8 addr: 81f000f0
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f053b8
sym: .rodata.str1.1 info: 03 other: 00 shndx: 3 value: 0 size: 0
sym: .rodata.str1.1 value: 81f05348 addr: 81f000f8
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f05348
sym: .rodata.str1.1 info: 03 other: 00 shndx: 3 value: 0 size: 0
sym: .rodata.str1.1 value: 81f05368 addr: 81f00100
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f05368
sym: .rodata.str1.1 info: 03 other: 00 shndx: 3 value: 0 size: 0
sym: .rodata.str1.1 value: 81f05378 addr: 81f00108
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f05378
sym: .rodata.str1.1 info: 03 other: 00 shndx: 3 value: 0 size: 0
sym: .rodata.str1.1 value: 81f0537e addr: 81f00110
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f0537e
sym: .rodata.str1.1 info: 03 other: 00 shndx: 3 value: 0 size: 0
sym: .rodata.str1.1 value: 81f05380 addr: 81f00118
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f05380
sym:     printf info: 12 other: 00 shndx: 1 value: 55c size: 90
sym: printf value: 81f0055c addr: 81f0012c
machine_apply_elf_rel: CALL26 9400000094000000->940000009400010c
sym: setup_arch info: 12 other: 00 shndx: 1 value: eb8 size: 4
sym: setup_arch value: 81f00eb8 addr: 81f00130
machine_apply_elf_rel: CALL26 5800016094000000->5800016094000362
sym: verify_sha256_digest info: 12 other: 00 shndx: 1 value: 0 size: f0
sym: verify_sha256_digest value: 81f00000 addr: 81f00140
machine_apply_elf_rel: CALL26 3400004094000000->3400004097ffffb0
sym: post_verification_setup_arch info: 12 other: 00 shndx: 1 value: eb4 size: 4
sym: post_verification_setup_arch value: 81f00eb4 addr: 81f00150
machine_apply_elf_rel: JUMP26 d503201f14000000->d503201f14000359
sym: .rodata.str1.1 info: 03 other: 00 shndx: 3 value: 0 size: 0
sym: .rodata.str1.1 value: 81f05390 addr: 81f00158
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f05390
sym:      .data info: 03 other: 00 shndx: 4 value: 0 size: 0
sym: .data value: 81f053b8 addr: 81f00160
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f053b8
sym:    putchar info: 12 other: 00 shndx: 1 value: eb0 size: 4
sym: putchar value: 81f00eb0 addr: 81f001c4
machine_apply_elf_rel: CALL26 f94037a194000000->f94037a19400033b
sym:    putchar info: 12 other: 00 shndx: 1 value: eb0 size: 4
sym: putchar value: 81f00eb0 addr: 81f00238
machine_apply_elf_rel: CALL26 910006f794000000->910006f79400031e
sym:    putchar info: 12 other: 00 shndx: 1 value: eb0 size: 4
sym: putchar value: 81f00eb0 addr: 81f00490
machine_apply_elf_rel: CALL26 9100073994000000->9100073994000288
sym: .rodata.str1.1 info: 03 other: 00 shndx: 3 value: 0 size: 0
sym: .rodata.str1.1 value: 81f053a2 addr: 81f004d0
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f053a2
sym:   vsprintf info: 12 other: 00 shndx: 1 value: 168 size: 364
sym: vsprintf value: 81f00168 addr: 81f00550
machine_apply_elf_rel: CALL26 a8d07bfd94000000->a8d07bfd97ffff06
sym:   vsprintf info: 12 other: 00 shndx: 1 value: 168 size: 364
sym: vsprintf value: 81f00168 addr: 81f005e0
machine_apply_elf_rel: CALL26 a8d17bfd94000000->a8d17bfd97fffee2
sym:  purgatory info: 12 other: 00 shndx: 1 value: 120 size: 34
sym: purgatory value: 81f00120 addr: 81f00688
machine_apply_elf_rel: CALL26 5800001194000000->5800001197fffea6
sym: arm64_kernel_entry info: 10 other: 00 shndx: 4 value: 128 size: 8
sym: arm64_kernel_entry value: 81f054e0 addr: 81f0068c
machine_apply_elf_rel: LD_PREL_LO19 5800000058000011->58000000580272b1
sym: arm64_dtb_addr info: 10 other: 00 shndx: 4 value: 130 size: 8
sym: arm64_dtb_addr value: 81f054e8 addr: 81f00690
machine_apply_elf_rel: LD_PREL_LO19 aa1f03e158000000->aa1f03e1580272c0
sym: sha256_process info: 12 other: 00 shndx: 1 value: f2c size: 4134
sym: sha256_process value: 81f00f2c addr: 81f050cc
machine_apply_elf_rel: CALL26 d101029494000000->d101029497ffef98
sym:     memcpy info: 12 other: 00 shndx: 1 value: 62c size: 20
sym: memcpy value: 81f0062c addr: 81f05128
machine_apply_elf_rel: JUMP26 b4fffc5814000000->b4fffc5817ffed41
sym:     memcpy info: 12 other: 00 shndx: 1 value: 62c size: 20
sym: memcpy value: 81f0062c addr: 81f05140
machine_apply_elf_rel: CALL26 aa1503e094000000->aa1503e097ffed3b
sym: sha256_process info: 12 other: 00 shndx: 1 value: f2c size: 4134
sym: sha256_process value: 81f00f2c addr: 81f0514c
machine_apply_elf_rel: CALL26 cb1302d694000000->cb1302d697ffef78
sym:      .data info: 03 other: 00 shndx: 4 value: 0 size: 0
sym: .data value: 81f054f0 addr: 81f05340
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f054f0
kexec_load: entry = 0x81f00680 flags = 0xb70000

nr_segments = 3
segment[0].buf = 0xffff8bab0010
segment[0].bufsz = 0x1999a00
segment[0].mem = 0x80480000
segment[0].memsz = 0x1a70000
segment[1].buf = 0x16984e10
segment[1].bufsz = 0xd0a1
segment[1].mem = 0x81ef0000
segment[1].memsz = 0x10000
segment[2].buf = 0x16992230
segment[2].bufsz = 0x5530
segment[2].mem = 0x81f00000
segment[2].memsz = 0x10000

I will try to stabilize this more both for 'kexec load' and 'kexec dump' use-cases and try to send a patch soon to fix this issue.

Regards,
Bhupesh

Quoted 149 lines · Vicente Bergas
Quoted 63 lines · Bhupesh Sharma

Hi Vicenç,

I tried the patch at my end and it works fine as per my checks. So, can I request you to provide some additional data which will help me understand your environment better. See inline:

Quoted 46 lines · Vicente Bergas
Quoted 44 lines · Simon Horman
Quoted 42 lines · Simon Horman
Quoted 40 lines · AKASHI Takahiro
Quoted 27 lines · Bhupesh Sharma

Vicenç reported (via [1]) that currently executing kexec with '--dtb' option and passing a .dtb which doesn't have a '/chosen' node leads to the following error:

  # kexec -d --dtb dtb_without_chosen_node.dtb --append 'cmdline' --load Image

dtb_set_property: fdt_add_subnode failed: <valid offset/length>
kexec: Set device tree bootargs failed.

This happens because currently we check the return value of 'fdt_add_subnode()' function call in 'dt-ops.c' incorrectly:

   result = fdt_add_subnode(new_dtb, nodeoffset, node);
   if (result) {
          dbgprintf("%s: fdt_add_subnode failed: %s\n", _func__,
                  fdt_strerror(result));
          goto on_error;
   }

As we can see in 'fdt_rw.c', a positive return value from 'fdt_add_subnode()' function doesn't indicate an error.

We can see that the Linux kernel (see 'drivers/firmware/efi/libstub/fdt.c' for example) also checks the 'fdt_add_subnode()' function against negative return values for errors. See an example below from 'update_fdt()' function in 'drivers/firmware/efi/libstub/fdt.c':

Quoted 8 lines
   node = fdt_add_subnode(fdt, 0, "chosen");
   if (node < 0) {
          status = node;
          <..snip..>
          goto fdt_set_fail;
   }

This patch fixes the same in 'kexec-tools'.

Looks good.
Thank you.
-Takahiro Akashi

Quoted 8 lines
Quoted 5 lines
Quoted 1 line

Thanks, applied.

Perhaps I was a little too hasty there, I have dropped the patch for now.

Vicente, does this patch work for you?

The patch looks fine, but the end result is still non-working:

  # kexec -d --load Image --append 'debug' --dtb no_chosen.dtb
Quoted 8 lines

Can you share the .dts with which you see this issue and steps you use to create the .dtb file at your end?

I normally use at my end the following command:
dtc -o no-chosen.dtb -I dts -O dtb no-chosen.dts

Thanks,
Bhupesh

Hi Bhupesh,
the issue is not with dtc, it is with kexec. The following script reproduces the issue:
#!/bin/sh -ve

uname -m

# aarch64

git clone 'https://git.kernel.org/pub/scm/utils/kernel/kexec/kexec-tools.git' kexec-tools-orig
cp -a kexec-tools-orig kexec-tools-test
git -C kexec-tools-orig checkout -b orig 'v2.0.18'
git -C kexec-tools-test checkout -b test 'v2.0.18'

cat patch.diff
#--- a/kexec/dt-ops.c
#+++ b/kexec/dt-ops.c
#@@ -84,7 +84,7 @@ int dtb_set_property(char **dtb, off_t *dtb_size, const char *node,
# if (nodeoffset == -FDT_ERR_NOTFOUND) {
# result = fdt_add_subnode(new_dtb, nodeoffset, node);
#
#- if (result) {
#+ if (result < 0) {
# dbgprintf("%s: fdt_add_subnode failed: %s\n", __func__,
# fdt_strerror(result));
# goto on_error;
patch -d kexec-tools-test -p1 < patch.diff

( cd kexec-tools-orig ; ./bootstrap )
( cd kexec-tools-test ; ./bootstrap )
( cd kexec-tools-orig ; ./configure )
( cd kexec-tools-test ; ./configure )
make -C kexec-tools-orig
make -C kexec-tools-test

wget 'http://mirror.archlinuxarm.org/aarch64/core/linux-aarch64-4.19.2-1-aarch64.pkg.tar.xz' bsdtar xf 'linux-aarch64-4.19.2-1-aarch64.pkg.tar.xz'

sudo kexec-tools-orig/build/sbin/kexec -d --load boot/Image --append 'debug' --dtb boot/dtbs/rockchip/rk3399-sapphire.dtb
# dtb_set_property: fdt_add_subnode failed: <valid offset/length> # kexec: Set device tree bootargs failed.

sudo kexec-tools-test/build/sbin/kexec -d --load boot/Image --append 'debug' --dtb boot/dtbs/rockchip/rk3399-sapphire.dtb
# dtb_set_property: fdt_setprop failed: FDT_ERR_BADOFFSET # kexec: Set device tree bootargs failed.

Regards,
Vicenç.

Quoted 23 lines
Quoted 23 lines

...
dtb_set_property: fdt_setprop failed: FDT_ERR_BADOFFSET
kexec: Set device tree bootargs failed.

Also tested this with the same result:

kexec/dt-ops.c +3 -1
@@ -84,11 +84,13 @@
        if (nodeoffset == -FDT_ERR_NOTFOUND) {
                result = fdt_add_subnode(new_dtb, nodeoffset, node);
 
-               if (result) {
+               if (result < 0) {
                        dbgprintf("%s: fdt_add_subnode failed: %s\n", __func__,
                                fdt_strerror(result));
                        goto on_error;
                }
+
+               nodeoffset = result;
        } else if (nodeoffset < 0) {
                dbgprintf("%s: fdt_path_offset failed: %s\n", __func__,
                        fdt_strerror(nodeoffset));
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

Regards,
Vicenç.

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

#17 lore raw

Re: [PATCH] kexec/dt-ops.c: Fix check against 'fdt_add_subnode' return value

Thanks for the feedback.

Quoted 53 lines · Vicente Bergas
Quoted 30 lines · Bhupesh Sharma

Hi Vicenç,

I have posted a revised and more comprehensive patchset for this issue today (please see the same here:
<http://lists.infradead.org/pipermail/kexec/2018-November/021999.html>)

I was able to exercise all the basic 'kexe load' and 'kdump' use cases on arm64 after these changes and I can also get the use-cases where we don't have a '/chosen' node in the .dtb to either PASS or FAIL with correct error reporting. Here is a listing of the basic use cases and their results:

# kexec -d -l Image --append=debug --dtb rk3399-sapphire.dtb
ok
# kexec -d -l Image --reuse-cmdline --initrd=/boot/initramfs-`uname -r`.img
ok
# kexec -d -p Image --reuse-cmdline --initrd=/boot/initramfs-`uname -r`.img
ok
# kexec -d -p Image --append=debug --dtb rk3399-sapphire.dtb
<..snip..>
setup_2nd_dtb: /chosen node not found - can't create dtb for dump
kernel: FDT_ERR_NOTFOUND
kexec: setup_2nd_dtb failed.
kexec: load failed.
Cannot load Image
Quoted 2 lines

Can you please try and test this patchset on your setup and let me know in case you land in any further issues?

Hi Bhupesh,
thank you, this is an improvement.
Unfortunately there is a remaining issue with the initrd option and without the reuse-cmdline option:
kexec -d -l /boot/Image --dtb /boot/dtbs/rockchip/rk3399-sapphire.dtb --initrd /boot/initramfs-linux.img
...
get_cells_size: #address-cells:2 #size-cells:2
cells_size_fitted: 0-0
cells_size_fitted: 0-0
setup_2nd_dtb: no kaslr-seed found
initrd: base 1d36000, size 5b3d3dh (5979453)
dtb_set_initrd: start 30629888, end 36609341, size 5979453 (5839 KiB)
dtb_set_property: fdt_add_subnode failed: FDT_ERR_EXISTS
dtb_delete_property: fdt_path_offset failed: FDT_ERR_NOTFOUND
kexec: load failed.
Cannot load /boot/Image

Quoted 1 line

In any case, do not worry and take your time, this is not a blocker issue.

I remember testing this use case as well (but perhaps not with the rockchip dtb) and did not meet this issue.
Let me recheck with the rockchip dtb as well and will send a v2 patchset if a fix is needed.

But its good to know that the rest of the issues are fixed.

Thanks,
Bhupesh

Quoted 429 lines
Quoted 429 lines

Thanks,
Bhupesh

Quoted 423 lines · Bhupesh Sharma

Hi Vicenç,

Sorry I got busy and just got some spare time to look at this issue. I am able to fix it partially, but I am still not completely happy with it, but mainly here is the issue for the 'kexec load' option - we are passing wrong 'nodeoffset' while calling 'fdt_add_subnode()', which should be 0 instead of FDT_ERR_NOTFOUND when the chosen node is not seen in the passed dtb :

From 2433daf279744ae30fa9c74ec5b39507740d7163 Mon Sep 17 00:00:00 2001
From: Bhupesh Sharma <[email protected]>
Date: Thu, 29 Nov 2018 00:41:06 +0530
Subject: [PATCH] Add some debug logs

Signed-off-by Bhupesh Sharma <[email protected]>
---
 kexec/arch/arm64/kexec-arm64.c |  8 ++++++++
 kexec/dt-ops.c                 |  3 ++-
 kexec/libfdt/fdt_ro.c          | 14 +++++++-------
 kexec/libfdt/fdt_rw.c          |  2 ++
 4 files changed, 19 insertions(+), 8 deletions(-)
kexec/arch/arm64/kexec-arm64.c +3 -0
index 6c7d2512b1d4..e29afc30db43 100644
@@ -207,6 +207,9 @@ static int set_bootargs(struct dtb *dtb, const
 char *command_line)
     if (!command_line || !command_line[0])
         return 0;
 
+    dbgprintf("%s: found %s\n", __func__, dtb->path);
+    dump_fdt(dtb->buf);
+
     result = dtb_set_bootargs(&dtb->buf, &dtb->size, command_line);
 

     if (result) {
kexec/arch/arm64/kexec-arm64.c +5 -0
@@ -413,6 +416,11 @@ static int setup_2nd_dtb(struct dtb *dtb, char
 *command_line, int on_crash)
     }
 
     result = set_bootargs(dtb, command_line);
+    if (result) {
+        fprintf(stderr, "kexec: cannot set bootargs.\n");
+        result = -EINVAL;
+        goto on_error;
+    }
 
     /* determine #address-cells and #size-cells */

     result = get_cells_size(dtb->buf, &address_cells, &size_cells);
kexec/dt-ops.c +2 -1
index a0276f1e48b3..79b0404bafc4 100644
@@ -86,13 +86,14 @@ int dtb_set_property(char **dtb, off_t *dtb_size,
 const char *node,
     nodeoffset = fdt_path_offset(new_dtb, node);
 
     if (nodeoffset == -FDT_ERR_NOTFOUND) {
-        result = fdt_add_subnode(new_dtb, nodeoffset, node);
+        result = fdt_add_subnode(new_dtb, 0, node);
 
         if (result < 0) {
             dbgprintf("%s: fdt_add_subnode failed: %s\n", __func__,
                 fdt_strerror(result));
             goto on_error;
         }
+        nodeoffset = result;
     } else if (nodeoffset < 0) {
         dbgprintf("%s: fdt_path_offset failed: %s\n", __func__,

             fdt_strerror(nodeoffset));
kexec/libfdt/fdt_ro.c +7 -7
index 129b532bcc1a..0dce53083c96 100644
@@ -105,14 +105,14 @@ int fdt_subnode_offset_namelen(const void *fdt,
 int offset,
     FDT_CHECK_HEADER(fdt);
 
     for (depth = 0;
-         offset >= 0;
-         offset = fdt_next_node(fdt, offset, &depth)) {
-        if (depth < 0)
-            return -FDT_ERR_NOTFOUND;
-        else if ((depth == 1)
-             && _fdt_nodename_eq(fdt, offset, name, namelen))
+         (offset >= 0) && (depth >= 0);
+         offset = fdt_next_node(fdt, offset, &depth))
+        if ((depth == 1)
+            && _fdt_nodename_eq(fdt, offset, name, namelen))
             return offset;
-    }
+
+    if (depth < 0)
+        return -FDT_ERR_NOTFOUND;
 
     return offset; /* error */

}

kexec/libfdt/fdt_rw.c +2 -0
index 8e7ec4cb7bcd..2fac7c0731c0 100644
@@ -101,6 +101,8 @@ static int _fdt_splice(void *fdt, void
 *splicepoint, int oldlen, int newlen)
 
     if (((p + oldlen) < p) || ((p + oldlen) > end))
         return -FDT_ERR_BADOFFSET;
+    if ((p < (char *)fdt) || ((end - oldlen + newlen) < (char *)fdt))
+        return -FDT_ERR_BADOFFSET;
     if ((end - oldlen + newlen) > ((char *)fdt + fdt_totalsize(fdt)))
         return -FDT_ERR_NOSPACE;

     memmove(p + newlen, p + oldlen, end - p - oldlen);

--
2.7.4

Here are some logs at my end after this fix (I used some earlier work I did for dump'ing dtb on arm64 boards to get the dtb logs:
<https://www.spinics.net/lists/kexec/msg20951.html>):

# kexec -d -l /root/Image --append 'debug' --dtb /root/rk3399-sapphire.dtb
<..snip..>

image_arm64_load: PE format: yes

 / {
    compatible = "rockchip,rk3399-sapphire";
    interrupt-parent = <0x00000001>;
    #address-cells = <0x00000002>;
    #size-cells = <0x00000002>;
    model = "Sapphire-RK3399 Board";
    /chosen {
        bootargs = "debug";
    };
    aliases {

<..snip..>
};

get_cells_size: #address-cells:2 #size-cells:2
cells_size_fitted: 0-0
cells_size_fitted: 0-0
setup_2nd_dtb: no kaslr-seed found
dtb:    base 81ef0000, size d0a1h (53409)
sym: sha256_starts info: 12 other: 00 shndx: 1 value: ec0 size: 6c
sym: sha256_starts value: 81f00ec0 addr: 81f00018
machine_apply_elf_rel: CALL26 580006b394000000->580006b3940003aa
sym: sha256_update info: 12 other: 00 shndx: 1 value: 5168 size: c
sym: sha256_update value: 81f05168 addr: 81f00034
machine_apply_elf_rel: CALL26 9100427394000000->910042739400144d
sym: sha256_finish info: 12 other: 00 shndx: 1 value: 5174 size: 1cc
sym: sha256_finish value: 81f05174 addr: 81f00050
machine_apply_elf_rel: CALL26 aa1403e094000000->aa1403e094001449
sym:     memcmp info: 12 other: 00 shndx: 1 value: 64c size: 34
sym: memcmp value: 81f0064c addr: 81f00060
machine_apply_elf_rel: CALL26 340003c094000000->340003c09400017b
sym:     printf info: 12 other: 00 shndx: 1 value: 55c size: 90
sym: printf value: 81f0055c addr: 81f00070
machine_apply_elf_rel: CALL26 5800046094000000->580004609400013b
sym:     printf info: 12 other: 00 shndx: 1 value: 55c size: 90
sym: printf value: 81f0055c addr: 81f00078
machine_apply_elf_rel: CALL26 5800047594000000->5800047594000139
sym:     printf info: 12 other: 00 shndx: 1 value: 55c size: 90
sym: printf value: 81f0055c addr: 81f00088
machine_apply_elf_rel: CALL26 9100067394000000->9100067394000135
sym:     printf info: 12 other: 00 shndx: 1 value: 55c size: 90
sym: printf value: 81f0055c addr: 81f000a8
machine_apply_elf_rel: CALL26 5800036094000000->580003609400012d
sym:     printf info: 12 other: 00 shndx: 1 value: 55c size: 90
sym: printf value: 81f0055c addr: 81f000b0
machine_apply_elf_rel: CALL26 910402e194000000->910402e19400012b
sym:     printf info: 12 other: 00 shndx: 1 value: 55c size: 90
sym: printf value: 81f0055c addr: 81f000c0
machine_apply_elf_rel: CALL26 9100067394000000->9100067394000127
sym:     printf info: 12 other: 00 shndx: 1 value: 55c size: 90
sym: printf value: 81f0055c addr: 81f000d4
machine_apply_elf_rel: CALL26 5280002094000000->5280002094000122
sym:      .data info: 03 other: 00 shndx: 4 value: 0 size: 0
sym: .data value: 81f053b8 addr: 81f000f0
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f053b8
sym: .rodata.str1.1 info: 03 other: 00 shndx: 3 value: 0 size: 0
sym: .rodata.str1.1 value: 81f05348 addr: 81f000f8
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f05348
sym: .rodata.str1.1 info: 03 other: 00 shndx: 3 value: 0 size: 0
sym: .rodata.str1.1 value: 81f05368 addr: 81f00100
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f05368
sym: .rodata.str1.1 info: 03 other: 00 shndx: 3 value: 0 size: 0
sym: .rodata.str1.1 value: 81f05378 addr: 81f00108
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f05378
sym: .rodata.str1.1 info: 03 other: 00 shndx: 3 value: 0 size: 0
sym: .rodata.str1.1 value: 81f0537e addr: 81f00110
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f0537e
sym: .rodata.str1.1 info: 03 other: 00 shndx: 3 value: 0 size: 0
sym: .rodata.str1.1 value: 81f05380 addr: 81f00118
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f05380
sym:     printf info: 12 other: 00 shndx: 1 value: 55c size: 90
sym: printf value: 81f0055c addr: 81f0012c
machine_apply_elf_rel: CALL26 9400000094000000->940000009400010c
sym: setup_arch info: 12 other: 00 shndx: 1 value: eb8 size: 4
sym: setup_arch value: 81f00eb8 addr: 81f00130
machine_apply_elf_rel: CALL26 5800016094000000->5800016094000362
sym: verify_sha256_digest info: 12 other: 00 shndx: 1 value: 0 size: f0
sym: verify_sha256_digest value: 81f00000 addr: 81f00140
machine_apply_elf_rel: CALL26 3400004094000000->3400004097ffffb0
sym: post_verification_setup_arch info: 12 other: 00 shndx: 1 value: eb4 size: 4
sym: post_verification_setup_arch value: 81f00eb4 addr: 81f00150
machine_apply_elf_rel: JUMP26 d503201f14000000->d503201f14000359
sym: .rodata.str1.1 info: 03 other: 00 shndx: 3 value: 0 size: 0
sym: .rodata.str1.1 value: 81f05390 addr: 81f00158
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f05390
sym:      .data info: 03 other: 00 shndx: 4 value: 0 size: 0
sym: .data value: 81f053b8 addr: 81f00160
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f053b8
sym:    putchar info: 12 other: 00 shndx: 1 value: eb0 size: 4
sym: putchar value: 81f00eb0 addr: 81f001c4
machine_apply_elf_rel: CALL26 f94037a194000000->f94037a19400033b
sym:    putchar info: 12 other: 00 shndx: 1 value: eb0 size: 4
sym: putchar value: 81f00eb0 addr: 81f00238
machine_apply_elf_rel: CALL26 910006f794000000->910006f79400031e
sym:    putchar info: 12 other: 00 shndx: 1 value: eb0 size: 4
sym: putchar value: 81f00eb0 addr: 81f00490
machine_apply_elf_rel: CALL26 9100073994000000->9100073994000288
sym: .rodata.str1.1 info: 03 other: 00 shndx: 3 value: 0 size: 0
sym: .rodata.str1.1 value: 81f053a2 addr: 81f004d0
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f053a2
sym:   vsprintf info: 12 other: 00 shndx: 1 value: 168 size: 364
sym: vsprintf value: 81f00168 addr: 81f00550
machine_apply_elf_rel: CALL26 a8d07bfd94000000->a8d07bfd97ffff06
sym:   vsprintf info: 12 other: 00 shndx: 1 value: 168 size: 364
sym: vsprintf value: 81f00168 addr: 81f005e0
machine_apply_elf_rel: CALL26 a8d17bfd94000000->a8d17bfd97fffee2
sym:  purgatory info: 12 other: 00 shndx: 1 value: 120 size: 34
sym: purgatory value: 81f00120 addr: 81f00688
machine_apply_elf_rel: CALL26 5800001194000000->5800001197fffea6
sym: arm64_kernel_entry info: 10 other: 00 shndx: 4 value: 128 size: 8
sym: arm64_kernel_entry value: 81f054e0 addr: 81f0068c
machine_apply_elf_rel: LD_PREL_LO19 5800000058000011->58000000580272b1
sym: arm64_dtb_addr info: 10 other: 00 shndx: 4 value: 130 size: 8
sym: arm64_dtb_addr value: 81f054e8 addr: 81f00690
machine_apply_elf_rel: LD_PREL_LO19 aa1f03e158000000->aa1f03e1580272c0
sym: sha256_process info: 12 other: 00 shndx: 1 value: f2c size: 4134
sym: sha256_process value: 81f00f2c addr: 81f050cc
machine_apply_elf_rel: CALL26 d101029494000000->d101029497ffef98
sym:     memcpy info: 12 other: 00 shndx: 1 value: 62c size: 20
sym: memcpy value: 81f0062c addr: 81f05128
machine_apply_elf_rel: JUMP26 b4fffc5814000000->b4fffc5817ffed41
sym:     memcpy info: 12 other: 00 shndx: 1 value: 62c size: 20
sym: memcpy value: 81f0062c addr: 81f05140
machine_apply_elf_rel: CALL26 aa1503e094000000->aa1503e097ffed3b
sym: sha256_process info: 12 other: 00 shndx: 1 value: f2c size: 4134
sym: sha256_process value: 81f00f2c addr: 81f0514c
machine_apply_elf_rel: CALL26 cb1302d694000000->cb1302d697ffef78
sym:      .data info: 03 other: 00 shndx: 4 value: 0 size: 0
sym: .data value: 81f054f0 addr: 81f05340
machine_apply_elf_rel: ABS64 0000000000000000->0000000081f054f0
kexec_load: entry = 0x81f00680 flags = 0xb70000

nr_segments = 3
segment[0].buf = 0xffff8bab0010
segment[0].bufsz = 0x1999a00
segment[0].mem = 0x80480000
segment[0].memsz = 0x1a70000
segment[1].buf = 0x16984e10
segment[1].bufsz = 0xd0a1
segment[1].mem = 0x81ef0000
segment[1].memsz = 0x10000
segment[2].buf = 0x16992230
segment[2].bufsz = 0x5530
segment[2].mem = 0x81f00000
segment[2].memsz = 0x10000

I will try to stabilize this more both for 'kexec load' and 'kexec dump' use-cases and try to send a patch soon to fix this issue.

Regards,
Bhupesh

Quoted 149 lines · Vicente Bergas
Quoted 63 lines · Bhupesh Sharma

Hi Vicenç,

I tried the patch at my end and it works fine as per my checks. So, can I request you to provide some additional data which will help me understand your environment better. See inline:

Quoted 46 lines · Vicente Bergas
Quoted 44 lines · Simon Horman
Quoted 42 lines · Simon Horman
Quoted 40 lines · AKASHI Takahiro
Quoted 27 lines · Bhupesh Sharma

Vicenç reported (via [1]) that currently executing kexec with '--dtb' option and passing a .dtb which doesn't have a '/chosen' node leads to the following error:

  # kexec -d --dtb dtb_without_chosen_node.dtb --append 'cmdline' --load Image

dtb_set_property: fdt_add_subnode failed: <valid offset/length>
kexec: Set device tree bootargs failed.

This happens because currently we check the return value of 'fdt_add_subnode()' function call in 'dt-ops.c' incorrectly:

   result = fdt_add_subnode(new_dtb, nodeoffset, node);
   if (result) {
          dbgprintf("%s: fdt_add_subnode failed: %s\n", _func__,
                  fdt_strerror(result));
          goto on_error;
   }

As we can see in 'fdt_rw.c', a positive return value from 'fdt_add_subnode()' function doesn't indicate an error.

We can see that the Linux kernel (see 'drivers/firmware/efi/libstub/fdt.c' for example) also checks the 'fdt_add_subnode()' function against negative return values for errors. See an example below from 'update_fdt()' function in 'drivers/firmware/efi/libstub/fdt.c':

Quoted 8 lines
   node = fdt_add_subnode(fdt, 0, "chosen");
   if (node < 0) {
          status = node;
          <..snip..>
          goto fdt_set_fail;
   }

This patch fixes the same in 'kexec-tools'.

Looks good.
Thank you.
-Takahiro Akashi

Quoted 8 lines
Quoted 5 lines
Quoted 1 line

Thanks, applied.

Perhaps I was a little too hasty there, I have dropped the patch for now.

Vicente, does this patch work for you?

The patch looks fine, but the end result is still non-working:

  # kexec -d --load Image --append 'debug' --dtb no_chosen.dtb
Quoted 8 lines

Can you share the .dts with which you see this issue and steps you use to create the .dtb file at your end?

I normally use at my end the following command:
dtc -o no-chosen.dtb -I dts -O dtb no-chosen.dts

Thanks,
Bhupesh

Hi Bhupesh,
the issue is not with dtc, it is with kexec. The following script reproduces the issue:
#!/bin/sh -ve

uname -m

# aarch64

git clone 'https://git.kernel.org/pub/scm/utils/kernel/kexec/kexec-tools.git' kexec-tools-orig
cp -a kexec-tools-orig kexec-tools-test
git -C kexec-tools-orig checkout -b orig 'v2.0.18'
git -C kexec-tools-test checkout -b test 'v2.0.18'

cat patch.diff
#--- a/kexec/dt-ops.c
#+++ b/kexec/dt-ops.c
#@@ -84,7 +84,7 @@ int dtb_set_property(char **dtb, off_t *dtb_size, const char *node,
# if (nodeoffset == -FDT_ERR_NOTFOUND) {
# result = fdt_add_subnode(new_dtb, nodeoffset, node);
#
#- if (result) {
#+ if (result < 0) {
# dbgprintf("%s: fdt_add_subnode failed: %s\n", __func__,
# fdt_strerror(result));
# goto on_error;
patch -d kexec-tools-test -p1 < patch.diff

( cd kexec-tools-orig ; ./bootstrap )
( cd kexec-tools-test ; ./bootstrap )
( cd kexec-tools-orig ; ./configure )
( cd kexec-tools-test ; ./configure )
make -C kexec-tools-orig
make -C kexec-tools-test

wget 'http://mirror.archlinuxarm.org/aarch64/core/linux-aarch64-4.19.2-1-aarch64.pkg.tar.xz' bsdtar xf 'linux-aarch64-4.19.2-1-aarch64.pkg.tar.xz'

sudo kexec-tools-orig/build/sbin/kexec -d --load boot/Image --append 'debug' --dtb boot/dtbs/rockchip/rk3399-sapphire.dtb
# dtb_set_property: fdt_add_subnode failed: <valid offset/length> # kexec: Set device tree bootargs failed.

sudo kexec-tools-test/build/sbin/kexec -d --load boot/Image --append 'debug' --dtb boot/dtbs/rockchip/rk3399-sapphire.dtb
# dtb_set_property: fdt_setprop failed: FDT_ERR_BADOFFSET # kexec: Set device tree bootargs failed.

Regards,
Vicenç.

Quoted 23 lines
Quoted 23 lines

...
dtb_set_property: fdt_setprop failed: FDT_ERR_BADOFFSET
kexec: Set device tree bootargs failed.

Also tested this with the same result:

kexec/dt-ops.c +3 -1
@@ -84,11 +84,13 @@
        if (nodeoffset == -FDT_ERR_NOTFOUND) {
                result = fdt_add_subnode(new_dtb, nodeoffset, node);
 
-               if (result) {
+               if (result < 0) {
                        dbgprintf("%s: fdt_add_subnode failed: %s\n", __func__,
                                fdt_strerror(result));
                        goto on_error;
                }
+
+               nodeoffset = result;
        } else if (nodeoffset < 0) {
                dbgprintf("%s: fdt_path_offset failed: %s\n", __func__,
                        fdt_strerror(nodeoffset));
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

Regards,
Vicenç.

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

author
#18 lore raw

PATCH 1/1 kexec/dt-ops.c: Fix check against 'fdt_add_subnode' return value

In reply to Bhupesh Sharma
Quoted 27 lines · Bhupesh Sharma

Vicenç reported (via [1]) that currently executing kexec with '--dtb' option and passing a .dtb which doesn't have a '/chosen' node leads to the following error:

  # kexec -d --dtb dtb_without_chosen_node.dtb --append 'cmdline' --load Image

dtb_set_property: fdt_add_subnode failed: <valid offset/length>
kexec: Set device tree bootargs failed.

This happens because currently we check the return value of 'fdt_add_subnode()' function call in 'dt-ops.c' incorrectly:

   result = fdt_add_subnode(new_dtb, nodeoffset, node);
   if (result) {
   	dbgprintf("%s: fdt_add_subnode failed: %s\n", _func__,
   		fdt_strerror(result));
   	goto on_error;
   }

As we can see in 'fdt_rw.c', a positive return value from 'fdt_add_subnode()' function doesn't indicate an error.

We can see that the Linux kernel (see 'drivers/firmware/efi/libstub/fdt.c' for example) also checks the 'fdt_add_subnode()' function against negative return values for errors. See an example below from 'update_fdt()' function in 'drivers/firmware/efi/libstub/fdt.c':

Quoted 8 lines
   node = fdt_add_subnode(fdt, 0, "chosen");
   if (node < 0) {
   	status = node;
   	<..snip..>
   	goto fdt_set_fail;
   }

This patch fixes the same in 'kexec-tools'.

The patch looks fine, but the end result is still non-working:

  # kexec -d --load Image --append 'debug' --dtb no_chosen.dtb 
  ...
  dtb_set_property: fdt_setprop failed: FDT_ERR_BADOFFSET
  kexec: Set device tree bootargs failed.

Also tested this with the same result:

kexec/dt-ops.c +3 -1
@@ -84,11 +84,13 @@
 	if (nodeoffset == -FDT_ERR_NOTFOUND) {
 		result = fdt_add_subnode(new_dtb, nodeoffset, node);
 
-		if (result) {
+		if (result < 0) {
 			dbgprintf("%s: fdt_add_subnode failed: %s\n", __func__,
 				fdt_strerror(result));
 			goto on_error;
 		}
+
+		nodeoffset = result;
 	} else if (nodeoffset < 0) {
 		dbgprintf("%s: fdt_path_offset failed: %s\n", __func__,
 			fdt_strerror(nodeoffset));

Regards,
Vicenç.

Quoted 25 lines

[1]. http://lists.infradead.org/pipermail/kexec/2018-October/021746.html

Cc Simon Horman <[email protected]>
Cc AKASHI Takahiro <[email protected]>
Reported-by Vicente Bergas <[email protected]>
Signed-off-by Bhupesh Sharma <[email protected]>
---
 kexec/dt-ops.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
kexec/dt-ops.c +1 -1
index 915dbf55afd2..f15174c3c74e 100644
@@ -84,7 +84,7 @@ int dtb_set_property(char **dtb, off_t *dtb_size, const char *node,
 	if (nodeoffset == -FDT_ERR_NOTFOUND) {
 		result = fdt_add_subnode(new_dtb, nodeoffset, node);
 
-		if (result) {
+		if (result < 0) {
 			dbgprintf("%s: fdt_add_subnode failed: %s\n", __func__,
 				fdt_strerror(result));
 			goto on_error;
Signature · 1 line
2.7.4

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec