Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [High] In-place conversion destructively modifies the old kernel's memory, corrupting the crash dump. - [High] Unbounded per-node `memremap()` calls can quickly exhaust the crash kernel's limited memory resources. - [High] Lack of physical address validation before `memremap()` risks triggering Machine Check Exceptions (MCE). - [Medium] `kho_radix_crash_contains_page()` fails to identify subpages of higher-order folios. - [Medium] The exported API lacks a teardown function, resulting in permanent vmap memory leaks. --
commit c1bba78846e7698e1b8a124f8432407d41f69b0c Author: Jork Loeser <[email protected]> Subject: kho: Add crash-kernel-safe radix tree presence check In the crash kernel, the old kernel's memory is outside the direct map. This patch adds a read-only radix tree variant that memremaps nodes during init so that subsequent page presence checks can traverse the tree with plain pointer dereferencing. > diff --git a/kernel/liveupdate/kexec_handover.c > b/kernel/liveupdate/kexec_handover.c > index 2e2b4e73f00d3..0dfdf0f9781ef 100644 > --- a/kernel/liveupdate/kexec_handover.c > +++ b/kernel/liveupdate/kexec_handover.c [ ... ] > @@ -396,6 +397,129 @@ void kho_radix_destroy_tree(struct kho_radix_tree *tree) > } > EXPORT_SYMBOL_GPL(kho_radix_destroy_tree); > > +/* > + * Convert a crash tree node's children from PA to VA in-place via > memremap(). > + * On failure, already-remapped pages are not cleaned up -- the crash kernel > + * is short-lived and will reboot after dump collection, so the leak is > + * inconsequential. > + */ > +static int kho_radix_crash_convert_node(struct kho_radix_node *node, > + unsigned int level) > +{ > + struct kho_radix_node *child; > + unsigned int i; > + int err; > + > + for (i = 0; i < (1 << KHO_TABLE_SIZE_LOG2); i++) { > + if (!node->table[i]) > + continue; > + > + /* Validate: PA must have bit 63 clear and be page-aligned */ > + if ((node->table[i] & BIT_ULL(63)) || > + (node->table[i] & (PAGE_SIZE - 1))) > + return -EINVAL; > + > + child = memremap(node->table[i], PAGE_SIZE, MEMREMAP_WB); [Severity: High] Since this recursively calls memremap() for every populated node in the tree, could this exhaust the crash kernel's limited memory resources? A fully populated tree might create thousands of individual virtual mappings, consuming significant memory for tracking structures like vm_struct and page tables. Also, the untrusted physical addresses read from the crashed kernel's memory are passed blindly to memremap(). If the old kernel crashed due to memory corruption, could these pointers contain garbage values pointing to non-RAM regions like MMIO, potentially triggering a Machine Check Exception (MCE) when accessed? > + if (!child) > + return -ENOMEM; > + > + /* Overwrite PA with VA in-place */ > + node->table[i] = (u64)(uintptr_t)child; [Severity: High] Because memremap() creates a direct mapping to the underlying physical memory, does assigning the virtual address here destructively modify the old kernel's physical memory? This seems like it would corrupt the crash dump, causing offline analysis tools like crash or makedumpfile to encounter meaningless virtual addresses instead of the original physical addresses. > + > + /* Recurse for intermediate levels; level 1 children are leaves > */ > + if (level > 1) { > + err = kho_radix_crash_convert_node(child, level - 1); > + if (err) > + return err; > + } > + } > + > + return 0; > +} > + > +/** > + * kho_radix_crash_init - Initialize a crash-kernel view of a KHO radix tree. [ ... ] > + */ > +int kho_radix_crash_init(struct kho_radix_crash_tree *tree, phys_addr_t > root_pa) > +{ > + struct kho_radix_node *root; > + int err; > + > + tree->root = NULL; > + > + if (!root_pa || (root_pa & (PAGE_SIZE - 1))) > + return -EINVAL; > + > + root = memremap(root_pa, PAGE_SIZE, MEMREMAP_WB); > + if (!root) > + return -ENOMEM; > + > + err = kho_radix_crash_convert_node(root, KHO_TREE_MAX_DEPTH - 1); > + if (err) > + return err; > + > + tree->root = root; > + return 0; > +} > +EXPORT_SYMBOL_GPL(kho_radix_crash_init); [Severity: Medium] This API is exported to modules and allocates mappings via memremap(), but there is no corresponding kho_radix_crash_destroy() provided. Even if the crash kernel is short-lived, should kernel APIs exported to modules enforce symmetrical resource lifecycle management to avoid permanent vmap memory leaks if a module is unloaded or fails initialization? > + > +/** > + * kho_radix_crash_contains_page - Check if a page is in a crash-kernel > radix tree. [ ... ] > + * Note: This function checks specifically for the presence of the page at > the > + * given order. If a larger order page that encompasses this page is > preserved, > + * this function will return false. > + * > + * Return: true if the page is present in the tree, false otherwise. > + */ > +bool kho_radix_crash_contains_page(struct kho_radix_crash_tree *tree, > + unsigned long pfn, unsigned int order) > +{ > + unsigned long key = kho_encode_radix_key(PFN_PHYS(pfn), order); [Severity: Medium] Since this searches for an exact match of the provided pfn and order, doesn't this fail to identify subpages of higher-order folios? If a page was preserved as part of a higher-order folio, an order-0 query for a PFN within that folio will fail to find the key and incorrectly return false. Would callers like pfn_is_ram() be forced to manually iterate through all possible orders up to MAX_PAGE_ORDER to reliably determine if a page is preserved? > + struct kho_radix_node *node = tree->root; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=14
