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

[Vicente Bergas <vicencb@gmail.com>]

On Mon, 12 Nov 2018 02:00:16 +0530, Bhupesh Sharma wrote:
> 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'.

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:
--- a/kexec/dt-ops.c
+++ b/kexec/dt-ops.c
@@ -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ç.

> [1]. http://lists.infradead.org/pipermail/kexec/2018-October/021746.html
> 
> Cc: Simon Horman <horms@verge.net.au>
> Cc: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Reported-by: Vicente Bergas <vicencb@gmail.com>
> Signed-off-by: Bhupesh Sharma <bhsharma@redhat.com>
> ---
>  kexec/dt-ops.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kexec/dt-ops.c b/kexec/dt-ops.c
> index 915dbf55afd2..f15174c3c74e 100644
> --- 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;
> -- 
> 2.7.4

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec