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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Vicenç, Simon,

On Fri, Nov 16, 2018 at 12:08 AM Vicente Bergas <vicencb@xxxxxxxxx> wrote:
>
> On Thu, Nov 15, 2018 at 3:14 PM Simon Horman <horms@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/kexec




[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux