Re: [PATCH v4 1/2] alloc_tag: move release_module_tags() above reserve_module_tags()

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] reserve_module_tags() silently succeeds and returns a valid-looking pointer when mas_store() fails, corrupting the module tags allocation space.
- [High] release_module_tags() ignores failures from mas_store(), permanently leaking the module's maple tree range.
--

--- Patch [1]: [PATCH v4 1/2] alloc_tag: move release_module_tags() above reserve_module_tags() ---
commit 96b4c3fa8241c380a13e2a15009ac553cfc71daa
Author: Hao Ge <[email protected]>

alloc_tag: move release_module_tags() above reserve_module_tags()

release_module_tags() is a cleanup helper. reserve_module_tags() can
also fail after storing the reservation in the maple tree, in which
case it should call release_module_tags() to undo it. Move the helper
above reserve_module_tags() so no forward declaration is needed.

No functional change.

> diff --git a/mm/alloc_tag.c b/mm/alloc_tag.c
> index e93e7fec1f066..96a9196635fea 100644
> --- a/mm/alloc_tag.c
> +++ b/mm/alloc_tag.c
> @@ -493,6 +493,52 @@ static int vm_module_tags_populate(void)
>  	return 0;
>  }
>  
> +static void release_module_tags(struct module *mod, bool used)
> +{

[ ... ]

> +release_area:
> +	mas_store(&mas, used ? &unloaded_mod : NULL);
> +	val = mas_prev_range(&mas, 0);
> +	if (val == &prepend_mod)
> +		mas_store(&mas, NULL);

[Severity: High]
This isn't a bug introduced by this patch, but does this ignore failures from
mas_store()?

Since mas_store() is called with the maple tree lock held, it forces an
implicit GFP_NOWAIT allocation. If this allocation fails under memory
pressure, mas_store() sets mas into an error state without modifying the tree.

Because the return value is ignored, the next call mas_prev_range(&mas, 0)
would immediately return NULL upon seeing the error state, causing the padding
cleanup mas_store(&mas, NULL) to be skipped.

Could this result in the module's maple tree range being permanently leaked
because the original module pointer remains in the tree?

> +out:
> +	mas_unlock(&mas);
> +}
> +
> +static void *reserve_module_tags(struct module *mod, unsigned long size,
> +				 unsigned int prepend, unsigned long align)
> +{

[Severity: High]
This isn't a bug introduced by this patch, but does reserve_module_tags()
silently succeed when mas_store() fails?

If mas_store(&mas, mod) fails due to memory pressure:

	if (mas_is_err(&mas)) {
		mas.index = pad_start;
		mas_erase(&mas);
		ret = ERR_PTR(xa_err(mas.node));
	}

Does mas_erase(&mas) destroy the error state in mas.node before it can be
read?

mas_erase() resets the maple state and walks the tree, overwriting the error
pointer in mas.node with a valid tree node or NULL. The subsequent
ret = ERR_PTR(xa_err(mas.node)) then reads this non-error node, resulting in
ret = NULL.

Since ret is not an error pointer, would IS_ERR(ret) evaluate to false,
causing the function to incorrectly return success?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1