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

[Breno Leitao <leitao@debian.org>]

On Fri, Mar 13, 2026 at 09:21:50AM +0000, Pratyush Yadav wrote:
> On Mon, Mar 09 2026, Breno Leitao wrote:
> 
> > kho_add_subtree() accepts a size parameter but only forwards it to
> > debugfs. The size is not persisted in the KHO FDT, so it is lost across
> > kexec. This makes it impossible for the incoming kernel to determine the
> > blob size without understanding the blob format.
> >
> > Store the blob size as a "blob-size" property in the KHO FDT alongside
> > the "preserved-data" physical address. This allows the receiving kernel
> > to recover the size for any blob regardless of format.
> >
> > Also extend kho_retrieve_subtree() with an optional size output
> > parameter so callers can learn the blob size without needing to
> > understand the blob format. Update all callers to pass NULL for the
> > new parameter.
> >
> > Signed-off-by: Breno Leitao <leitao@debian.org>
> > ---
> [...]
> > diff --git a/kernel/liveupdate/kexec_handover.c b/kernel/liveupdate/kexec_handover.c
> > index 54fe59fe43acd..1f22705d5d246 100644
> > --- a/kernel/liveupdate/kexec_handover.c
> > +++ b/kernel/liveupdate/kexec_handover.c
> > @@ -768,6 +768,7 @@ int kho_add_subtree(const char *name, void *blob, size_t size)
> >  {
> >  	phys_addr_t phys = virt_to_phys(blob);
> >  	void *root_fdt = kho_out.fdt;
> > +	u64 size_u64 = size;
> >  	int err = -ENOMEM;
> >  	int off, fdt_err;
> >  
> > @@ -784,11 +785,16 @@ int kho_add_subtree(const char *name, void *blob, size_t size)
> >  		goto out_pack;
> >  	}
> >  
> > -	err = fdt_setprop(root_fdt, off, KHO_FDT_SUB_TREE_PROP_NAME,
> > +	err = fdt_setprop(root_fdt, off, KHO_SUB_TREE_PROP_NAME,
> >  			  &phys, sizeof(phys));
> >  	if (err < 0)
> >  		goto out_pack;
> >  
> > +	err = fdt_setprop(root_fdt, off, KHO_SUB_TREE_SIZE_PROP_NAME,
> > +			  &size_u64, sizeof(size_u64));
> > +	if (err < 0)
> > +		goto out_pack;
> > +
> 
> 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.

> >  	WARN_ON_ONCE(kho_debugfs_blob_add(&kho_out.dbg, name, blob,
> >  					  size, false));
> >  
> > @@ -817,7 +823,7 @@ void kho_remove_subtree(void *blob)
> >  		const u64 *val;
> >  		int len;
> >  
> > -		val = fdt_getprop(root_fdt, off, KHO_FDT_SUB_TREE_PROP_NAME, &len);
> > +		val = fdt_getprop(root_fdt, off, KHO_SUB_TREE_PROP_NAME, &len);
> >  		if (!val || len != sizeof(phys_addr_t))
> >  			continue;
> >  
> > @@ -1314,13 +1320,14 @@ EXPORT_SYMBOL_GPL(is_kho_boot);
> >   * kho_retrieve_subtree - retrieve a preserved sub blob by its name.
> >   * @name: the name of the sub blob passed to kho_add_subtree().
> >   * @phys: if found, the physical address of the sub blob is stored in @phys.
> > + * @size: if not NULL and found, the size of the sub blob is stored in @size.
> >   *
> >   * Retrieve a preserved sub blob named @name and store its physical
> > - * address in @phys.
> > + * address in @phys and optionally its size in @size.
> >   *
> >   * Return: 0 on success, error code on failure
> >   */
> > -int kho_retrieve_subtree(const char *name, phys_addr_t *phys)
> > +int kho_retrieve_subtree(const char *name, phys_addr_t *phys, size_t *size)
> >  {
> >  	const void *fdt = kho_get_fdt();
> >  	const u64 *val;
> > @@ -1336,12 +1343,21 @@ int kho_retrieve_subtree(const char *name, phys_addr_t *phys)
> >  	if (offset < 0)
> >  		return -ENOENT;
> >  
> > -	val = fdt_getprop(fdt, offset, KHO_FDT_SUB_TREE_PROP_NAME, &len);
> > +	val = fdt_getprop(fdt, offset, KHO_SUB_TREE_PROP_NAME, &len);
> >  	if (!val || len != sizeof(*val))
> >  		return -EINVAL;
> >  
> >  	*phys = (phys_addr_t)*val;
> >  
> > +	if (size) {
> > +		val = fdt_getprop(fdt, offset, KHO_SUB_TREE_SIZE_PROP_NAME,
> > +				  &len);
> > +		if (val && len == sizeof(*val))
> > +			*size = (size_t)*val;
> > +		else
> > +			*size = 0;
> 
> If the size property is invalid, is it a good idea to ignore it? Should
> we instead consider the subnode to be broken and reject it entirely with
> an error message? Because if a caller expects a blob of 16 bytes but
> gets one with 0 bytes, it will likely error out anyway.

Ack, let me update this, then.

Thanks for the review,
--breno