Re: [PATCH] RDMA/uverbs: Fix radix tree node leak in uapi_add_elm()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] RDMA/uverbs: incomplete radix_tree_node leak fix in uapi_add_elm()
--

commit 2393d302cac044b0b4ac5a4d059afc74697b1fb2
Author: Deepanshu Kartikey <kartikey406@gmail.com>

RDMA/uverbs: Fix radix tree node leak in uapi_add_elm()

This commit attempts to fix a memory leak of intermediate radix_tree_node
objects when radix_tree_insert() fails with -ENOMEM in uapi_add_elm() by
inserting a dummy entry at the same key and immediately deleting it.

> diff --git a/drivers/infiniband/core/uverbs_uapi.c b/drivers/infiniband/core/uverbs_uapi.c
> index b67db3790f03c..91350d3479a80 100644
> --- a/drivers/infiniband/core/uverbs_uapi.c
> +++ b/drivers/infiniband/core/uverbs_uapi.c
> @@ -15,6 +15,7 @@ static int ib_uverbs_notsupp(struct uverbs_attr_bundle *attrs)
>  
>  static void *uapi_add_elm(struct uverbs_api *uapi, u32 key, size_t alloc_size)
>  {
> +	static void *dummy;
>  	void *elm;
>  	int rc;
>  
> @@ -27,6 +28,16 @@ static void *uapi_add_elm(struct uverbs_api *uapi, u32 key, size_t alloc_size)
>  	rc = radix_tree_insert(&uapi->radix, key, elm);
>  	if (rc) {
>  		kfree(elm);
> +
> +		/*
> +		 * A failed radix_tree_insert() can leave empty intermediate
> +		 * nodes linked in the tree. They are only freed when an entry
> +		 * under them is deleted, so populate and delete the slot to
> +		 * reclaim them.
> +		 */
> +		if (rc == -ENOMEM &&
> +		    !radix_tree_insert(&uapi->radix, key, &dummy))
> +		    radix_tree_delete(&uapi->radix, key);

[Severity: Medium]
Does this recovery path still leak radix_tree_node objects when the second
radix_tree_insert() in uapi_add_elm() also fails with -ENOMEM?

In lib/radix-tree.c, radix_tree_insert() -> __radix_tree_create() links
intermediate nodes at each level as it descends down to shift == 0:

lib/radix-tree.c:__radix_tree_create() {
	...
	while (shift > 0) {
		shift -= RADIX_TREE_MAP_SHIFT;
		if (child == NULL) {
			/* Have to add a child node.  */
			child = radix_tree_node_alloc(gfp, node, root, shift,
							offset, 0, 0);
			if (!child)
				return -ENOMEM;
			rcu_assign_pointer(*slot, node_to_entry(child));
			if (node)
				node->count++;
		} else if (!radix_tree_is_internal_node(child))
			break;
	...
}

When the initial radix_tree_insert(&uapi->radix, key, elm) returns -ENOMEM
after linking an upper-level node, the lower-level child node(s) down to
shift == 0 were not allocated. Freeing elm via kfree(elm) only releases a
small kmalloc object rather than a 576-byte node from
radix_tree_node_cachep.

Calling radix_tree_insert(&uapi->radix, key, &dummy) immediately afterward
must still call radix_tree_node_alloc() in __radix_tree_create() to allocate
those missing lower-level node(s) before &dummy can be inserted. Under
continued memory pressure, memcg limits, or multi-allocation fault injection,
that retry allocation can also fail with -ENOMEM (potentially after
allocating and linking another intermediate level in a 3-level tree). When
that happens, radix_tree_delete() is skipped and uverbs_destroy_api() ->
uapi_remove_range() still leaks the empty intermediate radix_tree_node
objects.

Would calling xa_destroy(&uapi->radix) in uverbs_destroy_api() after
uapi_remove_range(), or unwinding newly allocated empty nodes directly
on failure, avoid depending on a new radix_tree_node allocation to succeed
during -ENOMEM cleanup?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106013908.21789-1-kartikey406@gmail.com?part=1