linux-kernel - Re: [PATCH] mm/mempolicy: fix lock contention on mems

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 9 Aug 2022 14:11:18 +0200
From:   Michal Hocko <mhocko@...e.com>
To:     Abel Wu <wuyun.abel@...edance.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Vlastimil Babka <vbabka@...e.cz>,
        Mel Gorman <mgorman@...hsingularity.net>,
        Muchun Song <songmuchun@...edance.com>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm/mempolicy: fix lock contention on mems_allowed

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@...edance.com>
> ---
>  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