Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] `kho_radix_tree_freeze()` locks the tree mutex without verifying if the tree has been initialized. - [Medium] `kho_radix_del_key()` does not accurately report `-ENOENT` if the key is missing but its leaf node exists. - [Low] The docstring for `kho_radix_tree` instructs developers to pass a physical address to initialize the tree, which will lead to a crash. - [Low] The error path in `kho_init()` frees the radix tree root using `free_page()`, breaking the allocator abstraction. --
commit da7ab3d5519dc40884c4213f1650e1238149beb0 Author: Jork Loeser <[email protected]> kho: add radix tree freeze and del_key() error reporting This commit adds kho_radix_tree_freeze() to prevent modifications to a KHO radix tree. It also updates kho_radix_del_key() to return an integer so it can report errors like -EBUSY and -ENOENT. > diff --git a/include/linux/kho_radix_tree.h b/include/linux/kho_radix_tree.h > index c0840ecb230c8..4fe2238e1e30a 100644 > --- a/include/linux/kho_radix_tree.h > +++ b/include/linux/kho_radix_tree.h > @@ -21,10 +21,10 @@ > * scheme. Each key is an unsigned long that combines a page's physical > * address and its order. > * > - * Client code is responsible for allocating the root node of the tree, > - * initializing the mutex lock, and managing its lifecycle. It must use the > - * tree data structures defined in the KHO ABI, > - * `include/linux/kho/abi/kexec_handover.h`. > + * Client code must initialize the tree using kho_radix_tree_init(). Pass > + * a physical address to restore a tree preserved across kexec, or 0 to [Severity: Low] Is this documentation accurate? The tree traversal functions treat tree->root as a virtual address pointer. If a caller passes a raw physical address without using phys_to_virt(), would it cause a page fault upon the first dereference? > + * allocate a fresh empty tree. The tree uses data structures defined in > + * the KHO ABI, `include/linux/kho/abi/kexec_handover.h`. > */ [ ... ] > diff --git a/kernel/liveupdate/kexec_handover.c > b/kernel/liveupdate/kexec_handover.c > index 797ec285b698c..2e2b4e73f00d3 100644 > --- a/kernel/liveupdate/kexec_handover.c > +++ b/kernel/liveupdate/kexec_handover.c > @@ -79,9 +79,6 @@ struct kho_out { > > static struct kho_out kho_out = { > .lock = __MUTEX_INITIALIZER(kho_out.lock), > - .radix_tree = { > - .lock = __MUTEX_INITIALIZER(kho_out.radix_tree.lock), > - }, > }; [ ... ] > @@ -180,6 +177,28 @@ static void __ref kho_radix_free_node(struct > kho_radix_node *node) > memblock_free(node, PAGE_SIZE); > } > > +/** > + * kho_radix_tree_freeze - Freeze the tree, preventing further modifications. > + * @tree: The KHO radix tree to freeze. > + * > + * After freezing, kho_radix_add_key() and kho_radix_del_key() will return > + * -EBUSY. The check is performed under the tree's mutex, so there is no > + * race between a concurrent add/del and the freeze. > + * > + * Return: 0 on success, -EBUSY if the tree is already frozen. > + */ > +int kho_radix_tree_freeze(struct kho_radix_tree *tree) > +{ > + guard(mutex)(&tree->lock); [Severity: High] Could this cause a crash if called on an uninitialized tree? Because the static __MUTEX_INITIALIZER was removed from kho_out.radix_tree.lock in this patch, the mutex is zero-initialized before kho_radix_init_tree() is called. Unlike kho_radix_add_key() and kho_radix_del_key(), which safely check if (WARN_ON_ONCE(!tree->root)) before taking the mutex, this function directly attempts to lock it. > + > + if (tree->frozen) > + return -EBUSY; > + > + tree->frozen = true; > + return 0; > +} > +EXPORT_SYMBOL_GPL(kho_radix_tree_freeze); [ ... ] > @@ -300,6 +328,8 @@ void kho_radix_del_key(struct kho_radix_tree *tree, > unsigned long key) > leaf = (struct kho_radix_leaf *)node; > idx = kho_radix_get_bitmap_index(key); > __clear_bit(idx, leaf->bitmap); > + > + return 0; > } [Severity: Medium] Will this correctly report -ENOENT if the key is missing but the leaf node exists? The commit message states kho_radix_del_key() was updated to report -ENOENT when a key is not present. However, if other keys share the same leaf node and the target key's bit is already 0, __clear_bit() does nothing and this silently returns 0. Should this use test_and_clear_bit() and return -ENOENT if the bit was not previously set? > EXPORT_SYMBOL_GPL(kho_radix_del_key); [ ... ] > @@ -1807,7 +1836,7 @@ static __init int kho_init(void) > err_free_fdt: > kho_unpreserve_free(kho_out.fdt); > err_free_kho_radix_tree_root: > - kfree(tree->root); > + free_page((unsigned long)tree->root); [Severity: Low] Is it safe to use free_page() directly here instead of calling kho_radix_free_node()? Since kho_radix_init_tree() allocates the node internally via kho_radix_alloc_node(), calling free_page() directly bypasses the fallback safety checks in kho_radix_free_node(), which handles memblock_free() when the slab is unavailable. > tree->root = NULL; > err_free_scratch: > kho_out.fdt = NULL; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=13
