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

On Thu, Oct 16, 2025 at 1:23 PM Mike Rapoport rppt@kernel.org wrote:

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).

Makes sense, let me try to get it done. I actually like what cleanup.h provides.

...

...

...

•         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.

Maybe, also maybe for some self-tests. I think it is more consistent to return PTR_ERR() based on other code in this file (i.e. xa_load_or_alloc() already uses it). Let's keep it.

...

...

...

• if (WARN_ON(kho_scratch_overlap(virt_to_phys(chunk), PAGE_SIZE))) {
  

•         kfree(chunk);
  

•         return ERR_PTR(-EINVAL);
  

• }
  

-- Sincerely yours, Mike.