Re: [PATCH] mm/mempolicy: fix lock contention on mems_allowed

[Michal Hocko]

On Tue 09-08-22 18:49:27, Abel Wu wrote:
> The mems_allowed field can be modified by other tasks, so it
> isn't safe to access it with alloc_lock unlocked even in the
> current process context.

It would be useful to describe the racing scenario and the effect it
would have. 78b132e9bae9 hasn't really explained thinking behind and why
it was considered safe to drop the lock. I assume it was based on the
fact that the operation happens on the current task but this is hard to
tell.

> Fixes: 78b132e9bae9 ("mm/mempolicy: remove or narrow the lock on current")
> Signed-off-by: Abel Wu <wuyun.abel@xxxxxxxxxxxxx>
> ---
>  mm/mempolicy.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index d39b01fd52fe..ae422e44affb 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -855,12 +855,14 @@ static long do_set_mempolicy(unsigned short mode, unsigned short flags,
>  		goto out;
>  	}
>  
> +	task_lock(current);
>  	ret = mpol_set_nodemask(new, nodes, scratch);
>  	if (ret) {
> +		task_unlock(current);
>  		mpol_put(new);
>  		goto out;
>  	}
> -	task_lock(current);
> +
>  	old = current->mempolicy;
>  	current->mempolicy = new;
>  	if (new && new->mode == MPOL_INTERLEAVE)
> @@ -1295,7 +1297,9 @@ static long do_mbind(unsigned long start, unsigned long len,
>  		NODEMASK_SCRATCH(scratch);
>  		if (scratch) {
>  			mmap_write_lock(mm);
> +			task_lock(current);
>  			err = mpol_set_nodemask(new, nmask, scratch);
> +			task_unlock(current);
>  			if (err)
>  				mmap_write_unlock(mm);
>  		} else
> -- 
> 2.31.1

-- 
Michal Hocko
SUSE Labs