linux-kernel - Re: [RFC PATCH 1/3] mm/mempolicy: refactor do_set

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]

Message-ID: <20231002120317.000058ef@Huawei.com>
Date:   Mon, 2 Oct 2023 12:03:17 +0100
From:   Jonathan Cameron <Jonathan.Cameron@...wei.com>
To:     Gregory Price <gourry.memverge@...il.com>
CC:     <linux-mm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-arch@...r.kernel.org>, <linux-api@...r.kernel.org>,
        <linux-cxl@...r.kernel.org>, <luto@...nel.org>,
        <tglx@...utronix.de>, <mingo@...hat.com>, <bp@...en8.de>,
        <dave.hansen@...ux.intel.com>, <hpa@...or.com>, <arnd@...db.de>,
        <akpm@...ux-foundation.org>, <x86@...nel.org>,
        Gregory Price <gregory.price@...verge.com>
Subject: Re: [RFC PATCH 1/3] mm/mempolicy: refactor do_set_mempolicy for
 code re-use

On Thu, 14 Sep 2023 19:54:55 -0400
Gregory Price <gourry.memverge@...il.com> wrote:

> Refactors do_set_mempolicy into swap_mempolicy and do_set_mempolicy
> so that swap_mempolicy can be re-used with set_mempolicy2.
> 
> Signed-off-by: Gregory Price <gregory.price@...verge.com>

Obviously this is an RFC, so you probably didn't give it the polish
a finished patch might have.  Still I was curious and reading it and
I can't resist pointing out trivial stuff.. So....

> ---
>  mm/mempolicy.c | 44 +++++++++++++++++++++++++++++---------------
>  1 file changed, 29 insertions(+), 15 deletions(-)
> 
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index 42b5567e3773..f49337f6f300 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -855,28 +855,21 @@ static int mbind_range(struct vma_iterator *vmi, struct vm_area_struct *vma,
>  	return vma_replace_policy(vma, new_pol);
>  }
>  
> -/* Set the process memory policy */
> -static long do_set_mempolicy(unsigned short mode, unsigned short flags,
> -			     nodemask_t *nodes)
> +/* Swap in a new mempolicy, release the old one if successful */

Not really swapping. More replacing given we don't get the
old one back to do something else with it.

> +static long swap_mempolicy(struct mempolicy *new,
> +			   nodemask_t *nodes)

Excessive wrapping.

>  {
> -	struct mempolicy *new, *old;
> -	NODEMASK_SCRATCH(scratch);
> +	struct mempolicy *old = NULL;
>  	int ret;
> +	NODEMASK_SCRATCH(scratch);

I'd avoid the reordering as makes it look like slightly more is happening
in this change than is actually the case.

>  
>  	if (!scratch)
>  		return -ENOMEM;
>  
> -	new = mpol_new(mode, flags, nodes);
> -	if (IS_ERR(new)) {
> -		ret = PTR_ERR(new);
> -		goto out;
> -	}
> -
>  	task_lock(current);
>  	ret = mpol_set_nodemask(new, nodes, scratch);
>  	if (ret) {
>  		task_unlock(current);
> -		mpol_put(new);
>  		goto out;
>  	}
>  
> @@ -884,14 +877,35 @@ static long do_set_mempolicy(unsigned short mode, unsigned short flags,
>  	current->mempolicy = new;
>  	if (new && new->mode == MPOL_INTERLEAVE)
>  		current->il_prev = MAX_NUMNODES-1;
> -	task_unlock(current);
> -	mpol_put(old);
> -	ret = 0;
>  out:
> +	task_unlock(current);
> +	if (old)
> +		mpol_put(old);
It's protected against NULL parameter internally, so
	mpol_put(old);

which has advantage that a block of diff will hopefully disappear making
this patch easier to read.

> +
>  	NODEMASK_SCRATCH_FREE(scratch);
>  	return ret;
>  }
>  
> +/* Set the process memory policy */
> +static long do_set_mempolicy(unsigned short mode, unsigned short flags,
> +			     nodemask_t *nodes)
> +{
> +	struct mempolicy *new;
> +	int ret;
> +
> +	new = mpol_new(mode, flags, nodes);
> +	if (IS_ERR(new)) {
> +		ret = PTR_ERR(new);
> +		goto out;

Given nothing to do at out lable, in keeping with at least some local
style, you could do direct returns on errors.

	if (IS_ERR(new))
		return PTR_ERR(new)

	ret = swap_mempolicy(new, nodes);
	if (ret) {
		mpol_put(new);
		return ret;
	}

	return 0;

> +	}
> +
> +	ret = swap_mempolicy(new, nodes);
> +	if (ret)
> +		mpol_put(new);
> +out:
> +	return ret;
> +}
> +
>  /*
>   * Return nodemask for policy for get_mempolicy() query
>   *