Re: [PATCH v8 3/6] kho: persist blob size in KHO FDT

Pratyush Yadav
2026-04-03T11:37:54Z

On Mon, Mar 16 2026, Breno Leitao wrote:

> On Fri, Mar 13, 2026 at 09:21:50AM +0000, Pratyush Yadav wrote:
>> On Mon, Mar 09 2026, Breno Leitao wrote:
[...]
>> I noticed that the error handling here is a bit broken. We open the
>> subnode for the subtree, but then if we fail to add the "preserved-data"
>> property, we don't remove the subnode. So the next kernel gets an
>> invalid FDT (per KHO ABI) and might as well refuse to parse it.
>>
>> Similarly here, the FDT might also be missing the size and then the next
>> kernel might reject the FDT.
>>
>> Also, we directly return the FDT error code to the caller, which
>> wouldn't make sense since it probably expects -errno.
>>
>> Not something this patchset has to fix, but I am pointing this out in
>> case someone (possibly also future me) is interested in fixing this up.
>
> That is a good point, do you mean a fix like the following?
>
> commit 633d0cb01ed959676b60de8b1851dad1757d8fe5
> Author: Breno Leitao <leitao@debian.org>
> Date: Mon Mar 16 04:03:51 2026 -0700
>
> kho: fix error handling in kho_add_subtree()
>
> Fix two error handling issues in kho_add_subtree():
>
> 1. If fdt_setprop() fails after the subnode has been created, the
> subnode is not removed. This leaves an incomplete node in the FDT
> (missing "preserved-data" or "blob-size" properties), which violates
> the KHO ABI and may cause the next kernel to reject the FDT.
>
> 2. The fdt_setprop() return value (an FDT error code) is stored
> directly in err and returned to the caller, which expects -errno.
>
> Fix both by storing fdt_setprop() results in fdt_err, jumping to a new
> out_del_node label that removes the subnode on failure, and only setting
> err = 0 on the success path.
>
> Signed-off-by: Breno Leitao <leitao@debian.org>
> Suggested-by: Pratyush Yadav <pratyush@kernel.org>
>
> diff --git a/kernel/liveupdate/kexec_handover.c b/kernel/liveupdate/kexec_handover.c
> index 62b1b8a9aa337..8d2d30119f6d4 100644
> --- a/kernel/liveupdate/kexec_handover.c
> +++ b/kernel/liveupdate/kexec_handover.c
> @@ -787,19 +787,24 @@ int kho_add_subtree(const char *name, void *blob, size_t size)
> goto out_pack;
> }
>
> - err = fdt_setprop(root_fdt, off, KHO_SUB_TREE_PROP_NAME,
> - &phys, sizeof(phys));
> - if (err < 0)
> - goto out_pack;
> + fdt_err = fdt_setprop(root_fdt, off, KHO_SUB_TREE_PROP_NAME,
> + &phys, sizeof(phys));
> + if (fdt_err < 0)
> + goto out_del_node;
>
> - err = fdt_setprop(root_fdt, off, KHO_SUB_TREE_SIZE_PROP_NAME,
> - &size_u64, sizeof(size_u64));
> - if (err < 0)
> - goto out_pack;
> + fdt_err = fdt_setprop(root_fdt, off, KHO_SUB_TREE_SIZE_PROP_NAME,
> + &size_u64, sizeof(size_u64));
> + if (fdt_err < 0)
> + goto out_del_node;
>
> WARN_ON_ONCE(kho_debugfs_blob_add(&kho_out.dbg, name, blob,
> size, false));
>
> + err = 0;
> + goto out_pack;
> +
> +out_del_node:
> + fdt_del_node(root_fdt, off);
> out_pack:
> fdt_pack(root_fdt);
>
>
> Given this is not strictly related to this patchset, I am planning to
> send this fix separately.

Yep, looks good. Please send it out as an independent patch.

[...]

--
Regards,
Pratyush Yadav