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

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

[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;
   }

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

[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

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

[AKASHI Takahiro]

On Mon, Nov 12, 2018 at 02:00:16AM +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'.

Looks good.
Thank you.
-Takahiro Akashi

> [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

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

[Simon Horman]

On Mon, Nov 12, 2018 at 11:24:07AM +0900, AKASHI Takahiro wrote:
> On Mon, Nov 12, 2018 at 02:00:16AM +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'.
> 
> Looks good.
> Thank you.
> -Takahiro Akashi

Thanks, applied.

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

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

[Simon Horman]

On Thu, Nov 15, 2018 at 06:10:36AM -0800, Simon Horman wrote:
> On Mon, Nov 12, 2018 at 11:24:07AM +0900, AKASHI Takahiro wrote:
> > On Mon, Nov 12, 2018 at 02:00:16AM +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'.
> > 
> > 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
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

[Vicente Bergas]

On Thu, Nov 15, 2018 at 3:14 PM Simon Horman <horms@verge.net.au> wrote:
>
> On Thu, Nov 15, 2018 at 06:10:36AM -0800, Simon Horman wrote:
> > On Mon, Nov 12, 2018 at 11:24:07AM +0900, AKASHI Takahiro wrote:
> > > On Mon, Nov 12, 2018 at 02:00:16AM +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'.
> > >
> > > 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?

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ç.

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

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

[Bhupesh Sharma]

Hi Vicenç, Simon,

On Fri, Nov 16, 2018 at 12:08 AM Vicente Bergas <vicencb@gmail.com> wrote:
>
> On Thu, Nov 15, 2018 at 3:14 PM Simon Horman <horms@verge.net.au> wrote:
> >
> > On Thu, Nov 15, 2018 at 06:10:36AM -0800, Simon Horman wrote:
> > > On Mon, Nov 12, 2018 at 11:24:07AM +0900, AKASHI Takahiro wrote:
> > > > On Mon, Nov 12, 2018 at 02:00:16AM +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'.
> > > >
> > > > 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?
>
> 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));
>

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
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

[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:

On Fri, Nov 16, 2018 at 12:08 AM Vicente Bergas <vicencb@gmail.com> wrote:
>
> On Thu, Nov 15, 2018 at 3:14 PM Simon Horman <horms@verge.net.au> wrote:
> >
> > On Thu, Nov 15, 2018 at 06:10:36AM -0800, Simon Horman wrote:
> > > On Mon, Nov 12, 2018 at 11:24:07AM +0900, AKASHI Takahiro wrote:
> > > > On Mon, Nov 12, 2018 at 02:00:16AM +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'.
> > > >
> > > > 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?
>
> 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

>   ...
>   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ç.

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

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

[Vicente Bergas]

On Fri, Nov 16, 2018 at 1:04 PM Bhupesh Sharma <bhsharma@redhat.com> wrote:
>
> 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:
>
> On Fri, Nov 16, 2018 at 12:08 AM Vicente Bergas <vicencb@gmail.com> wrote:
> >
> > On Thu, Nov 15, 2018 at 3:14 PM Simon Horman <horms@verge.net.au> wrote:
> > >
> > > On Thu, Nov 15, 2018 at 06:10:36AM -0800, Simon Horman wrote:
> > > > On Mon, Nov 12, 2018 at 11:24:07AM +0900, AKASHI Takahiro wrote:
> > > > > On Mon, Nov 12, 2018 at 02:00:16AM +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'.
> > > > >
> > > > > 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?
> >
> > 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

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ç.

> >   ...
> >   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ç.

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

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

[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 <bhsharma@redhat.com>
Date: Thu, 29 Nov 2018 00:41:06 +0530
Subject: [PATCH] Add some debug logs

Signed-off-by: Bhupesh Sharma <bhsharma@redhat.com>
---
 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(-)

diff --git a/kexec/arch/arm64/kexec-arm64.c b/kexec/arch/arm64/kexec-arm64.c
index 6c7d2512b1d4..e29afc30db43 100644
--- a/kexec/arch/arm64/kexec-arm64.c
+++ b/kexec/arch/arm64/kexec-arm64.c
@@ -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);
diff --git a/kexec/dt-ops.c b/kexec/dt-ops.c
index a0276f1e48b3..79b0404bafc4 100644
--- a/kexec/dt-ops.c
+++ b/kexec/dt-ops.c
@@ -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));
diff --git a/kexec/libfdt/fdt_ro.c b/kexec/libfdt/fdt_ro.c
index 129b532bcc1a..0dce53083c96 100644
--- a/kexec/libfdt/fdt_ro.c
+++ b/kexec/libfdt/fdt_ro.c
@@ -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 */
 }
diff --git a/kexec/libfdt/fdt_rw.c b/kexec/libfdt/fdt_rw.c
index 8e7ec4cb7bcd..2fac7c0731c0 100644
--- a/kexec/libfdt/fdt_rw.c
+++ b/kexec/libfdt/fdt_rw.c
@@ -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
On Fri, Nov 16, 2018 at 8:59 PM Vicente Bergas <vicencb@gmail.com> wrote:
>
> On Fri, Nov 16, 2018 at 1:04 PM Bhupesh Sharma <bhsharma@redhat.com> wrote:
> >
> > 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:
> >
> > On Fri, Nov 16, 2018 at 12:08 AM Vicente Bergas <vicencb@gmail.com> wrote:
> > >
> > > On Thu, Nov 15, 2018 at 3:14 PM Simon Horman <horms@verge.net.au> wrote:
> > > >
> > > > On Thu, Nov 15, 2018 at 06:10:36AM -0800, Simon Horman wrote:
> > > > > On Mon, Nov 12, 2018 at 11:24:07AM +0900, AKASHI Takahiro wrote:
> > > > > > On Mon, Nov 12, 2018 at 02:00:16AM +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'.
> > > > > >
> > > > > > 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?
> > >
> > > 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
>
> 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ç.
>
> > >   ...
> > >   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ç.

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

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

[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

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


On Thu, Nov 29, 2018 at 3:30 AM Bhupesh Sharma <bhsharma@redhat.com> wrote:
>
> 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 <bhsharma@redhat.com>
> Date: Thu, 29 Nov 2018 00:41:06 +0530
> Subject: [PATCH] Add some debug logs
>
> Signed-off-by: Bhupesh Sharma <bhsharma@redhat.com>
> ---
>  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(-)
>
> diff --git a/kexec/arch/arm64/kexec-arm64.c b/kexec/arch/arm64/kexec-arm64.c
> index 6c7d2512b1d4..e29afc30db43 100644
> --- a/kexec/arch/arm64/kexec-arm64.c
> +++ b/kexec/arch/arm64/kexec-arm64.c
> @@ -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);
> diff --git a/kexec/dt-ops.c b/kexec/dt-ops.c
> index a0276f1e48b3..79b0404bafc4 100644
> --- a/kexec/dt-ops.c
> +++ b/kexec/dt-ops.c
> @@ -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));
> diff --git a/kexec/libfdt/fdt_ro.c b/kexec/libfdt/fdt_ro.c
> index 129b532bcc1a..0dce53083c96 100644
> --- a/kexec/libfdt/fdt_ro.c
> +++ b/kexec/libfdt/fdt_ro.c
> @@ -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 */
>  }
> diff --git a/kexec/libfdt/fdt_rw.c b/kexec/libfdt/fdt_rw.c
> index 8e7ec4cb7bcd..2fac7c0731c0 100644
> --- a/kexec/libfdt/fdt_rw.c
> +++ b/kexec/libfdt/fdt_rw.c
> @@ -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
> On Fri, Nov 16, 2018 at 8:59 PM Vicente Bergas <vicencb@gmail.com> wrote:
> >
> > On Fri, Nov 16, 2018 at 1:04 PM Bhupesh Sharma <bhsharma@redhat.com> wrote:
> > >
> > > 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:
> > >
> > > On Fri, Nov 16, 2018 at 12:08 AM Vicente Bergas <vicencb@gmail.com> wrote:
> > > >
> > > > On Thu, Nov 15, 2018 at 3:14 PM Simon Horman <horms@verge.net.au> wrote:
> > > > >
> > > > > On Thu, Nov 15, 2018 at 06:10:36AM -0800, Simon Horman wrote:
> > > > > > On Mon, Nov 12, 2018 at 11:24:07AM +0900, AKASHI Takahiro wrote:
> > > > > > > On Mon, Nov 12, 2018 at 02:00:16AM +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'.
> > > > > > >
> > > > > > > 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?
> > > >
> > > > 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
> >
> > 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ç.
> >
> > > >   ...
> > > >   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ç.

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

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

[Vicente Bergas]

On Fri, Nov 30, 2018 at 6:41 AM Bhupesh Sharma <bhsharma@redhat.com> wrote:
>
> 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?

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ç.

> Thanks,
> Bhupesh
>
>
> On Thu, Nov 29, 2018 at 3:30 AM Bhupesh Sharma <bhsharma@redhat.com> wrote:
> >
> > 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 <bhsharma@redhat.com>
> > Date: Thu, 29 Nov 2018 00:41:06 +0530
> > Subject: [PATCH] Add some debug logs
> >
> > Signed-off-by: Bhupesh Sharma <bhsharma@redhat.com>
> > ---
> >  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(-)
> >
> > diff --git a/kexec/arch/arm64/kexec-arm64.c b/kexec/arch/arm64/kexec-arm64.c
> > index 6c7d2512b1d4..e29afc30db43 100644
> > --- a/kexec/arch/arm64/kexec-arm64.c
> > +++ b/kexec/arch/arm64/kexec-arm64.c
> > @@ -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);
> > diff --git a/kexec/dt-ops.c b/kexec/dt-ops.c
> > index a0276f1e48b3..79b0404bafc4 100644
> > --- a/kexec/dt-ops.c
> > +++ b/kexec/dt-ops.c
> > @@ -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));
> > diff --git a/kexec/libfdt/fdt_ro.c b/kexec/libfdt/fdt_ro.c
> > index 129b532bcc1a..0dce53083c96 100644
> > --- a/kexec/libfdt/fdt_ro.c
> > +++ b/kexec/libfdt/fdt_ro.c
> > @@ -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 */
> >  }
> > diff --git a/kexec/libfdt/fdt_rw.c b/kexec/libfdt/fdt_rw.c
> > index 8e7ec4cb7bcd..2fac7c0731c0 100644
> > --- a/kexec/libfdt/fdt_rw.c
> > +++ b/kexec/libfdt/fdt_rw.c
> > @@ -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
> > On Fri, Nov 16, 2018 at 8:59 PM Vicente Bergas <vicencb@gmail.com> wrote:
> > >
> > > On Fri, Nov 16, 2018 at 1:04 PM Bhupesh Sharma <bhsharma@redhat.com> wrote:
> > > >
> > > > 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:
> > > >
> > > > On Fri, Nov 16, 2018 at 12:08 AM Vicente Bergas <vicencb@gmail.com> wrote:
> > > > >
> > > > > On Thu, Nov 15, 2018 at 3:14 PM Simon Horman <horms@verge.net.au> wrote:
> > > > > >
> > > > > > On Thu, Nov 15, 2018 at 06:10:36AM -0800, Simon Horman wrote:
> > > > > > > On Mon, Nov 12, 2018 at 11:24:07AM +0900, AKASHI Takahiro wrote:
> > > > > > > > On Mon, Nov 12, 2018 at 02:00:16AM +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'.
> > > > > > > >
> > > > > > > > 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?
> > > > >
> > > > > 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
> > >
> > > 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ç.
> > >
> > > > >   ...
> > > > >   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ç.

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

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

[Bhupesh Sharma]

Thanks for the feedback.

On Fri, Nov 30, 2018 at 8:29 PM Vicente Bergas <vicencb@gmail.com> wrote:
>
> On Fri, Nov 30, 2018 at 6:41 AM Bhupesh Sharma <bhsharma@redhat.com> wrote:
> >
> > 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?
>
> 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.

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

>
> > Thanks,
> > Bhupesh
> >
> >
> > On Thu, Nov 29, 2018 at 3:30 AM Bhupesh Sharma <bhsharma@redhat.com> wrote:
> > >
> > > 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 <bhsharma@redhat.com>
> > > Date: Thu, 29 Nov 2018 00:41:06 +0530
> > > Subject: [PATCH] Add some debug logs
> > >
> > > Signed-off-by: Bhupesh Sharma <bhsharma@redhat.com>
> > > ---
> > >  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(-)
> > >
> > > diff --git a/kexec/arch/arm64/kexec-arm64.c b/kexec/arch/arm64/kexec-arm64.c
> > > index 6c7d2512b1d4..e29afc30db43 100644
> > > --- a/kexec/arch/arm64/kexec-arm64.c
> > > +++ b/kexec/arch/arm64/kexec-arm64.c
> > > @@ -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);
> > > diff --git a/kexec/dt-ops.c b/kexec/dt-ops.c
> > > index a0276f1e48b3..79b0404bafc4 100644
> > > --- a/kexec/dt-ops.c
> > > +++ b/kexec/dt-ops.c
> > > @@ -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));
> > > diff --git a/kexec/libfdt/fdt_ro.c b/kexec/libfdt/fdt_ro.c
> > > index 129b532bcc1a..0dce53083c96 100644
> > > --- a/kexec/libfdt/fdt_ro.c
> > > +++ b/kexec/libfdt/fdt_ro.c
> > > @@ -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 */
> > >  }
> > > diff --git a/kexec/libfdt/fdt_rw.c b/kexec/libfdt/fdt_rw.c
> > > index 8e7ec4cb7bcd..2fac7c0731c0 100644
> > > --- a/kexec/libfdt/fdt_rw.c
> > > +++ b/kexec/libfdt/fdt_rw.c
> > > @@ -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
> > > On Fri, Nov 16, 2018 at 8:59 PM Vicente Bergas <vicencb@gmail.com> wrote:
> > > >
> > > > On Fri, Nov 16, 2018 at 1:04 PM Bhupesh Sharma <bhsharma@redhat.com> wrote:
> > > > >
> > > > > 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:
> > > > >
> > > > > On Fri, Nov 16, 2018 at 12:08 AM Vicente Bergas <vicencb@gmail.com> wrote:
> > > > > >
> > > > > > On Thu, Nov 15, 2018 at 3:14 PM Simon Horman <horms@verge.net.au> wrote:
> > > > > > >
> > > > > > > On Thu, Nov 15, 2018 at 06:10:36AM -0800, Simon Horman wrote:
> > > > > > > > On Mon, Nov 12, 2018 at 11:24:07AM +0900, AKASHI Takahiro wrote:
> > > > > > > > > On Mon, Nov 12, 2018 at 02:00:16AM +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'.
> > > > > > > > >
> > > > > > > > > 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?
> > > > > >
> > > > > > 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
> > > >
> > > > 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ç.
> > > >
> > > > > >   ...
> > > > > >   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ç.

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

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

[Vicente Bergas]

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