Re: [PATCH v2 01/10] liveupdate: centralize state management into struct luo_ser

[Pasha Tatashin <pasha.tatashin@soleen.com>]

On 05-17 20:20, Mike Rapoport wrote:
> On Thu, May 14, 2026 at 10:26:19PM +0000, Pasha Tatashin wrote:
> > Transition the LUO to ABI v2, which centralizes state management into a
> > single struct luo_ser header.
> > 
> > Previously, LUO state was spread across multiple FDT properties and
> > subnodes. ABI v2 simplifies this by placing all core state, including
> > the liveupdate number and physical addresses for sessions and FLB
> > headers into a centralized struct luo_ser.
> > 
> > Signed-off-by: Pasha Tatashin <pasha.tatashin@soleen.com>
> > ---
> >  include/linux/kho/abi/luo.h      | 91 +++++++++++---------------------
> >  kernel/liveupdate/luo_core.c     | 59 ++++++++++++++-------
> >  kernel/liveupdate/luo_flb.c      | 65 ++++-------------------
> >  kernel/liveupdate/luo_internal.h |  8 +--
> >  kernel/liveupdate/luo_session.c  | 57 +++-----------------
> >  5 files changed, 93 insertions(+), 187 deletions(-)
> 
> This looks lovely :)
> 
> > @@ -115,27 +117,29 @@ static int __init luo_early_startup(void)
> >  		return -EINVAL;
> >  	}
> >  
> > -	ln_size = 0;
> > -	ptr = fdt_getprop(luo_global.fdt_in, 0, LUO_FDT_LIVEUPDATE_NUM,
> > -			  &ln_size);
> > -	if (!ptr || ln_size != sizeof(luo_global.liveupdate_num)) {
> > -		pr_err("Unable to get live update number '%s' [%d]\n",
> > -		       LUO_FDT_LIVEUPDATE_NUM, ln_size);
> > +	header_size = 0;
> > +	ptr = fdt_getprop(luo_global.fdt_in, 0, LUO_FDT_ABI_HEADER, &header_size);
> > +	if (!ptr || header_size != sizeof(u64)) {
> > +		pr_err("Unable to get ABI header '%s' [%d]\n",
> > +		       LUO_FDT_ABI_HEADER, header_size);
> >  
> >  		return -EINVAL;
> >  	}
> >  
> > -	luo_global.liveupdate_num = get_unaligned((u64 *)ptr);
> > +	luo_ser_pa = get_unaligned((u64 *)ptr);
> > +	luo_ser = phys_to_virt(luo_ser_pa);
> > +
> > +	luo_global.liveupdate_num = luo_ser->liveupdate_num;
> >  	pr_info("Retrieved live update data, liveupdate number: %lld\n",
> >  		luo_global.liveupdate_num);
> >  
> > -	err = luo_session_setup_incoming(luo_global.fdt_in);
> > +	err = luo_session_setup_incoming(luo_ser->sessions_pa);
> >  	if (err)
> >  		return err;
> >  
> > -	err = luo_flb_setup_incoming(luo_global.fdt_in);
> > +	luo_flb_setup_incoming(luo_ser->flbs_pa);
> 
> Sashiko asks: 
> 
>   Is there a leak of the preserved luo_ser memory here?
> 
>   The outgoing kernel allocates the header using kho_alloc_preserve(), but
>   luo_early_startup() returns without calling kho_restore_free() on the
>   luo_ser pointer once the fields have been processed. This results in an
>   unreleased memory reservation on every successful live update.
> 
> This seems preexisting, and we probably don't care enough, but still...

The freeing happen during finish, that was by design.

> 
> There were more comments by sashiko, didn't check them yet.

The other comment is stupid: yes, changing ABI prevents updating from 
older kernel.

>   
> > -	return err;
> > +	return 0;
> >  }
> >  
> >  static int __init liveupdate_early_init(void)
> > @@ -156,7 +160,8 @@ early_initcall(liveupdate_early_init);
> >  /* Called during boot to create outgoing LUO fdt tree */
> >  static int __init luo_fdt_setup(void)
> >  {
> > -	const u64 ln = luo_global.liveupdate_num + 1;
> > +	struct luo_ser *luo_ser;
> > +	u64 luo_ser_pa;
> >  	void *fdt_out;
> >  	int err;
> >  
> > @@ -166,27 +171,45 @@ static int __init luo_fdt_setup(void)
> >  		return PTR_ERR(fdt_out);
> >  	}
> >  
> > +	luo_ser = kho_alloc_preserve(sizeof(*luo_ser));
> > +	if (IS_ERR(luo_ser)) {
> > +		err = PTR_ERR(luo_ser);
> > +		goto exit_free_fdt;
> > +	}
> > +	luo_ser_pa = virt_to_phys(luo_ser);
> > +
> >  	err = fdt_create(fdt_out, LUO_FDT_SIZE);
> >  	err |= fdt_finish_reservemap(fdt_out);
> >  	err |= fdt_begin_node(fdt_out, "");
> >  	err |= fdt_property_string(fdt_out, "compatible", LUO_FDT_COMPATIBLE);
> > -	err |= fdt_property(fdt_out, LUO_FDT_LIVEUPDATE_NUM, &ln, sizeof(ln));
> > -	err |= luo_session_setup_outgoing(fdt_out);
> > -	err |= luo_flb_setup_outgoing(fdt_out);
> > +	err |= fdt_property(fdt_out, LUO_FDT_ABI_HEADER, &luo_ser_pa,
> > +			    sizeof(luo_ser_pa));
> >  	err |= fdt_end_node(fdt_out);
> >  	err |= fdt_finish(fdt_out);
> >  	if (err)
> > -		goto exit_free;
> > +		goto exit_free_luo_ser;
> > +
> > +	err = luo_session_setup_outgoing(&luo_ser->sessions_pa);
> > +	if (err)
> > +		goto exit_free_luo_ser;
> > +
> > +	err = luo_flb_setup_outgoing(&luo_ser->flbs_pa);
> > +	if (err)
> > +		goto exit_free_luo_ser;
> > +
> > +	luo_ser->liveupdate_num = luo_global.liveupdate_num + 1;
> >  
> >  	err = kho_add_subtree(LUO_FDT_KHO_ENTRY_NAME, fdt_out,
> >  			      fdt_totalsize(fdt_out));
> >  	if (err)
> > -		goto exit_free;
> > +		goto exit_free_luo_ser;
> 
> And here we also seem to leak memory allocated by sesttion/flb setup.
> 
> >  	luo_global.fdt_out = fdt_out;
> >  
> >  	return 0;
> >  
> > -exit_free:
> > +exit_free_luo_ser:
> > +	kho_unpreserve_free(luo_ser);
> > +exit_free_fdt:
> >  	kho_unpreserve_free(fdt_out);
> >  	pr_err("failed to prepare LUO FDT: %d\n", err);
> 
> -- 
> Sincerely yours,
> Mike.