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

[Pratyush Yadav <pratyush@kernel.org>]

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