Re: [PATCH v2] mm/mempolicy: fix memory leaks in weighted_interleave_auto_store()

[Donet Tom <donettom@linux.ibm.com>]

[-- Attachment #1: Type: text/plain, Size: 2752 bytes --]


On 4/1/26 6:27 AM, Jackie Liu wrote:
> From: Jackie Liu<liuyun01@kylinos.cn>
>
> weighted_interleave_auto_store() fetches old_wi_state inside the
> if (!input) block only. This causes two memory leaks:
>
> 1. When a user writes "false" and the current mode is already manual,
>     the function returns early without freeing the freshly allocated
>     new_wi_state.
>
> 2. When a user writes "true", old_wi_state stays NULL because the
>     fetch is skipped entirely. The old state is then overwritten by
>     rcu_assign_pointer() but never freed, since the cleanup path is
>     gated on old_wi_state being non-NULL. A user can trigger this
>     repeatedly by writing "1" in a loop.
>
> Fix both leaks by moving the old_wi_state fetch before the input
> check, making it unconditional. This also allows a unified early
> return for both "true" and "false" when the requested mode matches the current mode. Cc: 
> stable@vger.kernel.org # v6.16+ Link: 
> https://sashiko.dev/#/patchset/20260331100740.84906-1-liu.yun@linux.dev 
> Fixes: e341f9c3c841 ("mm/mempolicy: Weighted Interleave Auto-tuning")
> Signed-off-by: Jackie Liu<liuyun01@kylinos.cn>
> ---
> Changes in v2:
> - Move old_wi_state fetch unconditionally before the input check,
>    instead of just adding kfree() to the early return path
> - Also fix an additional memory leak when writing "true" where the
>    previous wi_state was never freed (Sashiko)
>
>   mm/mempolicy.c | 23 ++++++++++++-----------
>   1 file changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index cf92bd6a8226..ebe4bc8220b1 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -3706,18 +3706,19 @@ static ssize_t weighted_interleave_auto_store(struct kobject *kobj,
>   		new_wi_state->iw_table[i] = 1;
>   
>   	mutex_lock(&wi_state_lock);
> -	if (!input) {
> -		old_wi_state = rcu_dereference_protected(wi_state,
> -					lockdep_is_held(&wi_state_lock));
> -		if (!old_wi_state)
> -			goto update_wi_state;
> -		if (input == old_wi_state->mode_auto) {
> -			mutex_unlock(&wi_state_lock);
> -			return count;
> -		}
> +	old_wi_state = rcu_dereference_protected(wi_state,
> +				lockdep_is_held(&wi_state_lock));
>   
> -		memcpy(new_wi_state->iw_table, old_wi_state->iw_table,
> -					       nr_node_ids * sizeof(u8));
> +	if (old_wi_state && input == old_wi_state->mode_auto) {
> +		mutex_unlock(&wi_state_lock);
> +		kfree(new_wi_state);
> +		return count;
> +	}
> +
> +	if (!input) {
> +		if (old_wi_state)
> +			memcpy(new_wi_state->iw_table, old_wi_state->iw_table,
> +						       nr_node_ids * sizeof(u8));

Yes, these are valid issues. This looks good to me.

Reviewed by: Donet Tom <donettom@linux.ibm.com>



>   		goto update_wi_state;
>   	}
>   

[-- Attachment #2: Type: text/html, Size: 4262 bytes --]