Re: [RFC PATCH 13/20] kho: add radix tree freeze and del_key() error reporting

[sashiko-bot@kernel.org]

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 <jloeser@linux.microsoft.com>

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/20260528004204.1484584-1-jloeser@linux.microsoft.com?part=13