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

...

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.

...

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

...

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

•         kfree(chunk);
  

•         return ERR_PTR(-EINVAL);
  

• }
  

• chunk->hdr.order = order;
  if (cur_chunk)
          KHOSER_STORE_PTR(cur_chunk->hdr.next, chunk);
  

@@ -379,14 +390,17 @@ static int kho_mem_serialize(struct kho_out *kho_out) struct khoser_mem_chunk *chunk = NULL; struct kho_mem_phys *physxa; unsigned long order;

• int ret = -ENOMEM;
  
  xa_for_each(&kho_out->track.orders, order, physxa) {
          struct kho_mem_phys_bits *bits;
          unsigned long phys;
  
          chunk = new_chunk(chunk, order);
  

•         if (!chunk)
  

•         if (IS_ERR(chunk)) {
  

•                 ret = PTR_ERR(chunk);
  

... and indeed, -errno from new_chunk() juts makes things more complex :(

...

                  goto err_free;

•         }
  
          if (!first_chunk)
                  first_chunk = chunk;
  

@@ -396,8 +410,10 @@ static int kho_mem_serialize(struct kho_out *kho_out)

                  if (chunk->hdr.num_elms == ARRAY_SIZE(chunk->bitmaps)) {
                          chunk = new_chunk(chunk, order);

•                         if (!chunk)
  

•                         if (IS_ERR(chunk)) {
  

•                                 ret = PTR_ERR(chunk);
                                  goto err_free;
  

•                         }
                  }
  
                  elm = &chunk->bitmaps[chunk->hdr.num_elms];
  

@@ -414,7 +430,7 @@ static int kho_mem_serialize(struct kho_out *kho_out)

err_free: kho_mem_ser_free(first_chunk);

• return -ENOMEM;
  

• return ret;
  

}

static void __init deserialize_bitmap(unsigned int order, @@ -737,6 +753,9 @@ int kho_preserve_folio(struct folio *folio) const unsigned int order = folio_order(folio); struct kho_mem_track *track = &kho_out.track;

• if (WARN_ON(kho_scratch_overlap(pfn << PAGE_SHIFT, PAGE_SIZE << order)))
  

•         return -EINVAL;
  

• return __kho_preserve_order(track, pfn, order);
  

} EXPORT_SYMBOL_GPL(kho_preserve_folio); @@ -784,6 +803,11 @@ int kho_preserve_pages(struct page *page, unsigned int nr_pages) unsigned long failed_pfn = 0; int err = 0;

• if (WARN_ON(kho_scratch_overlap(start_pfn << PAGE_SHIFT,
  

•                                 nr_pages << PAGE_SHIFT))) {
  

•         return -EINVAL;
  

• }
  

Can't we check this in __kho_preseve_order() and not duplicate the code?

Yes, that is possible, I will move it in the next version.

...

• while (pfn < end_pfn) {
          const unsigned int order =
                  min(count_trailing_zeros(pfn), ilog2(end_pfn - pfn));
  

-- Sincerely yours, Mike.