Re: [PATCH 1/2] liveupdate: kho: warn and fail on metadata or preserved memory in scratch area

[Mike Rapoport <rppt@kernel.org>]

On Wed, Oct 15, 2025 at 08:36:25AM -0400, Pasha Tatashin wrote:
> > > Signed-off-by: Pasha Tatashin <pasha.tatashin@soleen.com>
> > > ---
> > >  kernel/liveupdate/Kconfig                   | 15 ++++++++++
> >
> > Feels like kernel/liveupdate/Makefile change is missing
> 
> It's not, we already have KEXEC_HANDOVER_DEBUGFS that pulls in
> kexec_handover_debug.c
> 
> That debug file contains KHO debugfs and debug code. The debug code
> adds KEXEC_HANDOVER_DEBUGFS as a dependency, which I think is
> appropriate for a debug build.
> 
> However, I do not like ugly ifdefs in .c, so perhaps, we should have two files:
> kexec_handover_debugfs.c for debugfs and kexec_handover_debug.c ? What
> do you think?
> 
> > >  kernel/liveupdate/kexec_handover.c          | 32 ++++++++++++++++++---
> > >  kernel/liveupdate/kexec_handover_debug.c    | 18 ++++++++++++
> > >  kernel/liveupdate/kexec_handover_internal.h |  9 ++++++
> > >  4 files changed, 70 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/kernel/liveupdate/Kconfig b/kernel/liveupdate/Kconfig
> > > index 522b9f74d605..d119f4f3f4b1 100644
> > > --- a/kernel/liveupdate/Kconfig
> > > +++ b/kernel/liveupdate/Kconfig
> > > @@ -27,4 +27,19 @@ config KEXEC_HANDOVER_DEBUGFS
> > >         Also, enables inspecting the KHO fdt trees with the debugfs binary
> > >         blobs.
> > >
> > > +config KEXEC_HANDOVER_DEBUG
> > > +     bool "Enable Kexec Handover debug checks"
> > > +     depends on KEXEC_HANDOVER_DEBUGFS
> > > +     help
> > > +       This option enables extra sanity checks for the Kexec Handover
> > > +       subsystem.
> > > +
> > > +       These checks verify that neither preserved memory regions nor KHO's
> > > +       internal metadata are allocated from within a KHO scratch area.
> > > +       An overlap can lead to memory corruption during a subsequent kexec
> > > +       operation.
> > > +
> > > +       If an overlap is detected, the kernel will print a warning and the
> > > +       offending operation will fail. This should only be enabled for
> > > +       debugging purposes due to runtime overhead.
> > >  endmenu
> > > diff --git a/kernel/liveupdate/kexec_handover.c b/kernel/liveupdate/kexec_handover.c
> > > index 5da21f1510cc..ef1e6f7a234b 100644
> > > --- a/kernel/liveupdate/kexec_handover.c
> > > +++ b/kernel/liveupdate/kexec_handover.c
> > > @@ -141,6 +141,11 @@ static void *xa_load_or_alloc(struct xarray *xa, unsigned long index, size_t sz)
> > >       if (!elm)
> > >               return ERR_PTR(-ENOMEM);
> > >
> > > +     if (WARN_ON(kho_scratch_overlap(virt_to_phys(elm), sz))) {
> > > +             kfree(elm);
> >
> > I think __free() cleanup would be better than this.
> 
> Sorry, not sure what do you mean. kfree() is already is in this
> function in case of failure.

There's __free(kfree) cleanup function defined in include/linux/cleanup.h
that ensures that on return from a function resources are not leaked.
With kfree we could do something like

	void *elm __free(kfree) = NULL;

	if (error)
		return ERR_PTR(errno);

	return no_free_ptr(elm);

There's no __free() definition for free_page() though :(

The second best IMHO is to use goto for error handling rather than free()
inside if (error).

> > > +             return ERR_PTR(-EINVAL);
> > > +     }
> > > +
> > >       res = xa_cmpxchg(xa, index, NULL, elm, GFP_KERNEL);
> > >       if (xa_is_err(res))
> > >               res = ERR_PTR(xa_err(res));
> > > @@ -354,7 +359,13 @@ static struct khoser_mem_chunk *new_chunk(struct khoser_mem_chunk *cur_chunk,
> > >
> > >       chunk = kzalloc(PAGE_SIZE, GFP_KERNEL);
> > >       if (!chunk)
> > > -             return NULL;
> > > +             return ERR_PTR(-ENOMEM);
> >
> > I don't think it's important to return -errno here, it's not that it's
> > called from a syscall and we need to set errno for the userspace.
> > BTW, the same applies to xa_load_or_alloc() IMO.
> 
> HM, but they are very different errors: ENOMEM, the KHO user can try
> again after more memory is available, but the new -EINVAL return from
> this function tells the caller that there is something broken in the
> system, and using KHO is futile until this bug is fixed.

Do you really see the callers handling this differently? 
And we already have WARN_ON() because something is broken in the system.
 
> > > +
> > > +     if (WARN_ON(kho_scratch_overlap(virt_to_phys(chunk), PAGE_SIZE))) {
> > > +             kfree(chunk);
> > > +             return ERR_PTR(-EINVAL);
> > > +     }
> > > +

-- 
Sincerely yours,
Mike.