Re: [PATCH v2] mm/mempolicy: fix mpol_new leak in shared_policy_replace

Re: [PATCH v2] mm/mempolicy: fix mpol_new leak in shared_policy_replace

[Michal Hocko]

On Tue 22-03-22 16:34:56, Miaohe Lin wrote:
> If mpol_new is allocated but not used in restart loop, mpol_new will be
> freed via mpol_put before returning to the caller.  But refcnt is not
> initialized yet, so mpol_put could not do the right things and might leak
> the unused mpol_new.

I would just add:

This would happen if mempolicy was updated on the shared shmem file
while the sp->lock has been dropped during the memory allocation.

> This issue could be triggered easily with the below
> code snippet if there're many processes doing the below work at the same
> time:
> 
>   shmid = shmget((key_t)5566, 1024 * PAGE_SIZE, 0666|IPC_CREAT);
>   shm = shmat(shmid, 0, 0);
>   loop many times {
>     mbind(shm, 1024 * PAGE_SIZE, MPOL_LOCAL, mask, maxnode, 0);
>     mbind(shm + 128 * PAGE_SIZE, 128 * PAGE_SIZE, MPOL_DEFAULT, mask,
>           maxnode, 0);
>   }
> 
> Fixes: 42288fe366c4 ("mm: mempolicy: Convert shared_policy mutex to spinlock")
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> Cc: <stable@vger.kernel.org> # 3.8

Acked-by: Michal Hocko <mhocko@suse.com>

Thanks a lot!
> ---
> v1->v2:
>   Add reproducer snippet and Cc stable.
>   Thanks Michal Hocko for review and comment!
> ---
>  mm/mempolicy.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index a2516d31db6c..4cdd425b2752 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -2733,6 +2733,7 @@ static int shared_policy_replace(struct shared_policy *sp, unsigned long start,
>  	mpol_new = kmem_cache_alloc(policy_cache, GFP_KERNEL);
>  	if (!mpol_new)
>  		goto err_out;
> +	refcount_set(&mpol_new->refcnt, 1);
>  	goto restart;
>  }
>  
> -- 
> 2.23.0

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2] mm/mempolicy: fix mpol_new leak in shared_policy_replace

[kernel test robot]

Hi Miaohe,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linux/master]
[also build test ERROR on linus/master v5.17]
[cannot apply to hnaz-mm/master next-20220321]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Miaohe-Lin/mm-mempolicy-fix-mpol_new-leak-in-shared_policy_replace/20220321-200100
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 2c271fe77d52a0555161926c232cd5bc07178b39
config: ia64-defconfig (https://download.01.org/0day-ci/archive/20220322/202203220201.toJrpBkF-lkp@intel.com/config)
compiler: ia64-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/9a91a8a7964a3af0b60f08dc38b7815e5118206a
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Miaohe-Lin/mm-mempolicy-fix-mpol_new-leak-in-shared_policy_replace/20220321-200100
        git checkout 9a91a8a7964a3af0b60f08dc38b7815e5118206a
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=ia64 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   mm/mempolicy.c: In function 'shared_policy_replace':
>> mm/mempolicy.c:2745:22: error: passing argument 1 of 'refcount_set' from incompatible pointer type [-Werror=incompatible-pointer-types]
    2745 |         refcount_set(&mpol_new->refcnt, 1);
         |                      ^~~~~~~~~~~~~~~~~
         |                      |
         |                      atomic_t *
   In file included from include/linux/pid.h:7,
                    from include/linux/sched.h:14,
                    from include/linux/mempolicy.h:9,
                    from mm/mempolicy.c:73:
   include/linux/refcount.h:134:45: note: expected 'refcount_t *' {aka 'struct refcount_struct *'} but argument is of type 'atomic_t *'
     134 | static inline void refcount_set(refcount_t *r, int n)
         |                                 ~~~~~~~~~~~~^
   cc1: some warnings being treated as errors


vim +/refcount_set +2745 mm/mempolicy.c

  2681	
  2682	/* Replace a policy range. */
  2683	static int shared_policy_replace(struct shared_policy *sp, unsigned long start,
  2684					 unsigned long end, struct sp_node *new)
  2685	{
  2686		struct sp_node *n;
  2687		struct sp_node *n_new = NULL;
  2688		struct mempolicy *mpol_new = NULL;
  2689		int ret = 0;
  2690	
  2691	restart:
  2692		write_lock(&sp->lock);
  2693		n = sp_lookup(sp, start, end);
  2694		/* Take care of old policies in the same range. */
  2695		while (n && n->start < end) {
  2696			struct rb_node *next = rb_next(&n->nd);
  2697			if (n->start >= start) {
  2698				if (n->end <= end)
  2699					sp_delete(sp, n);
  2700				else
  2701					n->start = end;
  2702			} else {
  2703				/* Old policy spanning whole new range. */
  2704				if (n->end > end) {
  2705					if (!n_new)
  2706						goto alloc_new;
  2707	
  2708					*mpol_new = *n->policy;
  2709					atomic_set(&mpol_new->refcnt, 1);
  2710					sp_node_init(n_new, end, n->end, mpol_new);
  2711					n->end = start;
  2712					sp_insert(sp, n_new);
  2713					n_new = NULL;
  2714					mpol_new = NULL;
  2715					break;
  2716				} else
  2717					n->end = start;
  2718			}
  2719			if (!next)
  2720				break;
  2721			n = rb_entry(next, struct sp_node, nd);
  2722		}
  2723		if (new)
  2724			sp_insert(sp, new);
  2725		write_unlock(&sp->lock);
  2726		ret = 0;
  2727	
  2728	err_out:
  2729		if (mpol_new)
  2730			mpol_put(mpol_new);
  2731		if (n_new)
  2732			kmem_cache_free(sn_cache, n_new);
  2733	
  2734		return ret;
  2735	
  2736	alloc_new:
  2737		write_unlock(&sp->lock);
  2738		ret = -ENOMEM;
  2739		n_new = kmem_cache_alloc(sn_cache, GFP_KERNEL);
  2740		if (!n_new)
  2741			goto err_out;
  2742		mpol_new = kmem_cache_alloc(policy_cache, GFP_KERNEL);
  2743		if (!mpol_new)
  2744			goto err_out;
> 2745		refcount_set(&mpol_new->refcnt, 1);
  2746		goto restart;
  2747	}
  2748	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v2] mm/mempolicy: fix mpol_new leak in shared_policy_replace

[kernel test robot]

Hi Miaohe,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linux/master]
[also build test ERROR on linus/master v5.17]
[cannot apply to hnaz-mm/master next-20220321]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Miaohe-Lin/mm-mempolicy-fix-mpol_new-leak-in-shared_policy_replace/20220321-200100
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 2c271fe77d52a0555161926c232cd5bc07178b39
config: x86_64-randconfig-a002-20220321 (https://download.01.org/0day-ci/archive/20220322/202203220336.VpfVL4ng-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 85e9b2687a13d1908aa86d1b89c5ce398a06cd39)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/9a91a8a7964a3af0b60f08dc38b7815e5118206a
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Miaohe-Lin/mm-mempolicy-fix-mpol_new-leak-in-shared_policy_replace/20220321-200100
        git checkout 9a91a8a7964a3af0b60f08dc38b7815e5118206a
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> mm/mempolicy.c:2745:15: error: incompatible pointer types passing 'atomic_t *' to parameter of type 'refcount_t *' (aka 'struct refcount_struct *') [-Werror,-Wincompatible-pointer-types]
           refcount_set(&mpol_new->refcnt, 1);
                        ^~~~~~~~~~~~~~~~~
   include/linux/refcount.h:134:45: note: passing argument to parameter 'r' here
   static inline void refcount_set(refcount_t *r, int n)
                                               ^
   1 error generated.


vim +2745 mm/mempolicy.c

  2681	
  2682	/* Replace a policy range. */
  2683	static int shared_policy_replace(struct shared_policy *sp, unsigned long start,
  2684					 unsigned long end, struct sp_node *new)
  2685	{
  2686		struct sp_node *n;
  2687		struct sp_node *n_new = NULL;
  2688		struct mempolicy *mpol_new = NULL;
  2689		int ret = 0;
  2690	
  2691	restart:
  2692		write_lock(&sp->lock);
  2693		n = sp_lookup(sp, start, end);
  2694		/* Take care of old policies in the same range. */
  2695		while (n && n->start < end) {
  2696			struct rb_node *next = rb_next(&n->nd);
  2697			if (n->start >= start) {
  2698				if (n->end <= end)
  2699					sp_delete(sp, n);
  2700				else
  2701					n->start = end;
  2702			} else {
  2703				/* Old policy spanning whole new range. */
  2704				if (n->end > end) {
  2705					if (!n_new)
  2706						goto alloc_new;
  2707	
  2708					*mpol_new = *n->policy;
  2709					atomic_set(&mpol_new->refcnt, 1);
  2710					sp_node_init(n_new, end, n->end, mpol_new);
  2711					n->end = start;
  2712					sp_insert(sp, n_new);
  2713					n_new = NULL;
  2714					mpol_new = NULL;
  2715					break;
  2716				} else
  2717					n->end = start;
  2718			}
  2719			if (!next)
  2720				break;
  2721			n = rb_entry(next, struct sp_node, nd);
  2722		}
  2723		if (new)
  2724			sp_insert(sp, new);
  2725		write_unlock(&sp->lock);
  2726		ret = 0;
  2727	
  2728	err_out:
  2729		if (mpol_new)
  2730			mpol_put(mpol_new);
  2731		if (n_new)
  2732			kmem_cache_free(sn_cache, n_new);
  2733	
  2734		return ret;
  2735	
  2736	alloc_new:
  2737		write_unlock(&sp->lock);
  2738		ret = -ENOMEM;
  2739		n_new = kmem_cache_alloc(sn_cache, GFP_KERNEL);
  2740		if (!n_new)
  2741			goto err_out;
  2742		mpol_new = kmem_cache_alloc(policy_cache, GFP_KERNEL);
  2743		if (!mpol_new)
  2744			goto err_out;
> 2745		refcount_set(&mpol_new->refcnt, 1);
  2746		goto restart;
  2747	}
  2748	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v2] mm/mempolicy: fix mpol_new leak in shared_policy_replace

[Miaohe Lin]

On 2022/3/21 20:12, Michal Hocko wrote:
> On Tue 22-03-22 16:34:56, Miaohe Lin wrote:
>> If mpol_new is allocated but not used in restart loop, mpol_new will be
>> freed via mpol_put before returning to the caller.  But refcnt is not
>> initialized yet, so mpol_put could not do the right things and might leak
>> the unused mpol_new.
> 
> I would just add:
> 
> This would happen if mempolicy was updated on the shared shmem file
> while the sp->lock has been dropped during the memory allocation.
> 

Do you mean the below commit log?

"""
If mpol_new is allocated but not used in restart loop, mpol_new will be
freed via mpol_put before returning to the caller.  But refcnt is not
initialized yet, so mpol_put could not do the right things and might leak
the unused mpol_new. This would happen if mempolicy was updated on the
shared shmem file while the sp->lock has been dropped during the memory
allocation.

This issue could be triggered easily with the below code snippet if
there're many processes doing the below work at the same time:

  shmid = shmget((key_t)5566, 1024 * PAGE_SIZE, 0666|IPC_CREAT);
  shm = shmat(shmid, 0, 0);
  loop many times {
    mbind(shm, 1024 * PAGE_SIZE, MPOL_LOCAL, mask, maxnode, 0);
    mbind(shm + 128 * PAGE_SIZE, 128 * PAGE_SIZE, MPOL_DEFAULT, mask,
          maxnode, 0);
  }
"""

>> This issue could be triggered easily with the below
>> code snippet if there're many processes doing the below work at the same
>> time:
>>
>>   shmid = shmget((key_t)5566, 1024 * PAGE_SIZE, 0666|IPC_CREAT);
>>   shm = shmat(shmid, 0, 0);
>>   loop many times {
>>     mbind(shm, 1024 * PAGE_SIZE, MPOL_LOCAL, mask, maxnode, 0);
>>     mbind(shm + 128 * PAGE_SIZE, 128 * PAGE_SIZE, MPOL_DEFAULT, mask,
>>           maxnode, 0);
>>   }
>>
>> Fixes: 42288fe366c4 ("mm: mempolicy: Convert shared_policy mutex to spinlock")
>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>> Cc: <stable@vger.kernel.org> # 3.8
> 
> Acked-by: Michal Hocko <mhocko@suse.com>
> 
> Thanks a lot!

Many thanks for comment and Acked-by tag! :)

>> ---
>> v1->v2:
>>   Add reproducer snippet and Cc stable.
>>   Thanks Michal Hocko for review and comment!
>> ---
>>  mm/mempolicy.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
>> index a2516d31db6c..4cdd425b2752 100644
>> --- a/mm/mempolicy.c
>> +++ b/mm/mempolicy.c
>> @@ -2733,6 +2733,7 @@ static int shared_policy_replace(struct shared_policy *sp, unsigned long start,
>>  	mpol_new = kmem_cache_alloc(policy_cache, GFP_KERNEL);
>>  	if (!mpol_new)
>>  		goto err_out;
>> +	refcount_set(&mpol_new->refcnt, 1);
>>  	goto restart;
>>  }
>>  
>> -- 
>> 2.23.0
> 

Re: [PATCH v2] mm/mempolicy: fix mpol_new leak in shared_policy_replace

[Miaohe Lin]

On 2022/3/22 4:01, kernel test robot wrote:
> Hi Miaohe,
> 
> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on linux/master]
> [also build test ERROR on linus/master v5.17]
> [cannot apply to hnaz-mm/master next-20220321]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:    https://github.com/0day-ci/linux/commits/Miaohe-Lin/mm-mempolicy-fix-mpol_new-leak-in-shared_policy_replace/20220321-200100
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 2c271fe77d52a0555161926c232cd5bc07178b39
> config: x86_64-randconfig-a002-20220321 (https://download.01.org/0day-ci/archive/20220322/202203220336.VpfVL4ng-lkp@intel.com/config)
> compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 85e9b2687a13d1908aa86d1b89c5ce398a06cd39)
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # https://github.com/0day-ci/linux/commit/9a91a8a7964a3af0b60f08dc38b7815e5118206a
>         git remote add linux-review https://github.com/0day-ci/linux
>         git fetch --no-tags linux-review Miaohe-Lin/mm-mempolicy-fix-mpol_new-leak-in-shared_policy_replace/20220321-200100
>         git checkout 9a91a8a7964a3af0b60f08dc38b7815e5118206a
>         # save the config file to linux build tree
>         mkdir build_dir
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 
>>> mm/mempolicy.c:2745:15: error: incompatible pointer types passing 'atomic_t *' to parameter of type 'refcount_t *' (aka 'struct refcount_struct *') [-Werror,-Wincompatible-pointer-types]
>            refcount_set(&mpol_new->refcnt, 1);
>                         ^~~~~~~~~~~~~~~~~
>    include/linux/refcount.h:134:45: note: passing argument to parameter 'r' here
>    static inline void refcount_set(refcount_t *r, int n)
>                                                ^
>    1 error generated.
> 

Many thanks for report. Commit 4fbd79de2889 ("mm/mempolicy: convert from atomic_t to refcount_t on mempolicy->refcnt") has
changed mpol_new->refcnt from atomic_t to refcount_t. So we should use refcount_set instead of atomic_set here.

Thanks.

> 
> vim +2745 mm/mempolicy.c
> 
>   2681	
>   2682	/* Replace a policy range. */
>   2683	static int shared_policy_replace(struct shared_policy *sp, unsigned long start,
>   2684					 unsigned long end, struct sp_node *new)
>   2685	{
>   2686		struct sp_node *n;
>   2687		struct sp_node *n_new = NULL;
>   2688		struct mempolicy *mpol_new = NULL;
>   2689		int ret = 0;
>   2690	
>   2691	restart:
>   2692		write_lock(&sp->lock);
>   2693		n = sp_lookup(sp, start, end);
>   2694		/* Take care of old policies in the same range. */
>   2695		while (n && n->start < end) {
>   2696			struct rb_node *next = rb_next(&n->nd);
>   2697			if (n->start >= start) {
>   2698				if (n->end <= end)
>   2699					sp_delete(sp, n);
>   2700				else
>   2701					n->start = end;
>   2702			} else {
>   2703				/* Old policy spanning whole new range. */
>   2704				if (n->end > end) {
>   2705					if (!n_new)
>   2706						goto alloc_new;
>   2707	
>   2708					*mpol_new = *n->policy;
>   2709					atomic_set(&mpol_new->refcnt, 1);
>   2710					sp_node_init(n_new, end, n->end, mpol_new);
>   2711					n->end = start;
>   2712					sp_insert(sp, n_new);
>   2713					n_new = NULL;
>   2714					mpol_new = NULL;
>   2715					break;
>   2716				} else
>   2717					n->end = start;
>   2718			}
>   2719			if (!next)
>   2720				break;
>   2721			n = rb_entry(next, struct sp_node, nd);
>   2722		}
>   2723		if (new)
>   2724			sp_insert(sp, new);
>   2725		write_unlock(&sp->lock);
>   2726		ret = 0;
>   2727	
>   2728	err_out:
>   2729		if (mpol_new)
>   2730			mpol_put(mpol_new);
>   2731		if (n_new)
>   2732			kmem_cache_free(sn_cache, n_new);
>   2733	
>   2734		return ret;
>   2735	
>   2736	alloc_new:
>   2737		write_unlock(&sp->lock);
>   2738		ret = -ENOMEM;
>   2739		n_new = kmem_cache_alloc(sn_cache, GFP_KERNEL);
>   2740		if (!n_new)
>   2741			goto err_out;
>   2742		mpol_new = kmem_cache_alloc(policy_cache, GFP_KERNEL);
>   2743		if (!mpol_new)
>   2744			goto err_out;
>> 2745		refcount_set(&mpol_new->refcnt, 1);
>   2746		goto restart;
>   2747	}
>   2748	
> 

Re: [PATCH v2] mm/mempolicy: fix mpol_new leak in shared_policy_replace

[Miaohe Lin]

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

On 2022/3/22 4:01, kernel test robot wrote:
> Hi Miaohe,
> 
> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on linux/master]
> [also build test ERROR on linus/master v5.17]
> [cannot apply to hnaz-mm/master next-20220321]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:    https://github.com/0day-ci/linux/commits/Miaohe-Lin/mm-mempolicy-fix-mpol_new-leak-in-shared_policy_replace/20220321-200100
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 2c271fe77d52a0555161926c232cd5bc07178b39
> config: x86_64-randconfig-a002-20220321 (https://download.01.org/0day-ci/archive/20220322/202203220336.VpfVL4ng-lkp(a)intel.com/config)
> compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 85e9b2687a13d1908aa86d1b89c5ce398a06cd39)
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # https://github.com/0day-ci/linux/commit/9a91a8a7964a3af0b60f08dc38b7815e5118206a
>         git remote add linux-review https://github.com/0day-ci/linux
>         git fetch --no-tags linux-review Miaohe-Lin/mm-mempolicy-fix-mpol_new-leak-in-shared_policy_replace/20220321-200100
>         git checkout 9a91a8a7964a3af0b60f08dc38b7815e5118206a
>         # save the config file to linux build tree
>         mkdir build_dir
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 
>>> mm/mempolicy.c:2745:15: error: incompatible pointer types passing 'atomic_t *' to parameter of type 'refcount_t *' (aka 'struct refcount_struct *') [-Werror,-Wincompatible-pointer-types]
>            refcount_set(&mpol_new->refcnt, 1);
>                         ^~~~~~~~~~~~~~~~~
>    include/linux/refcount.h:134:45: note: passing argument to parameter 'r' here
>    static inline void refcount_set(refcount_t *r, int n)
>                                                ^
>    1 error generated.
> 

Many thanks for report. Commit 4fbd79de2889 ("mm/mempolicy: convert from atomic_t to refcount_t on mempolicy->refcnt") has
changed mpol_new->refcnt from atomic_t to refcount_t. So we should use refcount_set instead of atomic_set here.

Thanks.

> 
> vim +2745 mm/mempolicy.c
> 
>   2681	
>   2682	/* Replace a policy range. */
>   2683	static int shared_policy_replace(struct shared_policy *sp, unsigned long start,
>   2684					 unsigned long end, struct sp_node *new)
>   2685	{
>   2686		struct sp_node *n;
>   2687		struct sp_node *n_new = NULL;
>   2688		struct mempolicy *mpol_new = NULL;
>   2689		int ret = 0;
>   2690	
>   2691	restart:
>   2692		write_lock(&sp->lock);
>   2693		n = sp_lookup(sp, start, end);
>   2694		/* Take care of old policies in the same range. */
>   2695		while (n && n->start < end) {
>   2696			struct rb_node *next = rb_next(&n->nd);
>   2697			if (n->start >= start) {
>   2698				if (n->end <= end)
>   2699					sp_delete(sp, n);
>   2700				else
>   2701					n->start = end;
>   2702			} else {
>   2703				/* Old policy spanning whole new range. */
>   2704				if (n->end > end) {
>   2705					if (!n_new)
>   2706						goto alloc_new;
>   2707	
>   2708					*mpol_new = *n->policy;
>   2709					atomic_set(&mpol_new->refcnt, 1);
>   2710					sp_node_init(n_new, end, n->end, mpol_new);
>   2711					n->end = start;
>   2712					sp_insert(sp, n_new);
>   2713					n_new = NULL;
>   2714					mpol_new = NULL;
>   2715					break;
>   2716				} else
>   2717					n->end = start;
>   2718			}
>   2719			if (!next)
>   2720				break;
>   2721			n = rb_entry(next, struct sp_node, nd);
>   2722		}
>   2723		if (new)
>   2724			sp_insert(sp, new);
>   2725		write_unlock(&sp->lock);
>   2726		ret = 0;
>   2727	
>   2728	err_out:
>   2729		if (mpol_new)
>   2730			mpol_put(mpol_new);
>   2731		if (n_new)
>   2732			kmem_cache_free(sn_cache, n_new);
>   2733	
>   2734		return ret;
>   2735	
>   2736	alloc_new:
>   2737		write_unlock(&sp->lock);
>   2738		ret = -ENOMEM;
>   2739		n_new = kmem_cache_alloc(sn_cache, GFP_KERNEL);
>   2740		if (!n_new)
>   2741			goto err_out;
>   2742		mpol_new = kmem_cache_alloc(policy_cache, GFP_KERNEL);
>   2743		if (!mpol_new)
>   2744			goto err_out;
>> 2745		refcount_set(&mpol_new->refcnt, 1);
>   2746		goto restart;
>   2747	}
>   2748	
> 

Re: [PATCH v2] mm/mempolicy: fix mpol_new leak in shared_policy_replace

[Michal Hocko]

On Tue 22-03-22 09:50:35, Miaohe Lin wrote:
> On 2022/3/21 20:12, Michal Hocko wrote:
> > On Tue 22-03-22 16:34:56, Miaohe Lin wrote:
> >> If mpol_new is allocated but not used in restart loop, mpol_new will be
> >> freed via mpol_put before returning to the caller.  But refcnt is not
> >> initialized yet, so mpol_put could not do the right things and might leak
> >> the unused mpol_new.
> > 
> > I would just add:
> > 
> > This would happen if mempolicy was updated on the shared shmem file
> > while the sp->lock has been dropped during the memory allocation.
> > 
> 
> Do you mean the below commit log?
> 
> """
> If mpol_new is allocated but not used in restart loop, mpol_new will be
> freed via mpol_put before returning to the caller.  But refcnt is not
> initialized yet, so mpol_put could not do the right things and might leak
> the unused mpol_new. This would happen if mempolicy was updated on the
> shared shmem file while the sp->lock has been dropped during the memory
> allocation.
> 
> This issue could be triggered easily with the below code snippet if
> there're many processes doing the below work at the same time:
> 
>   shmid = shmget((key_t)5566, 1024 * PAGE_SIZE, 0666|IPC_CREAT);
>   shm = shmat(shmid, 0, 0);
>   loop many times {
>     mbind(shm, 1024 * PAGE_SIZE, MPOL_LOCAL, mask, maxnode, 0);
>     mbind(shm + 128 * PAGE_SIZE, 128 * PAGE_SIZE, MPOL_DEFAULT, mask,
>           maxnode, 0);
>   }
> """

Yes, LGTM.
Thanks!
-- 
Michal Hocko
SUSE Labs

[PATCH v2] mm/mempolicy: fix mpol_new leak in shared_policy_replace

[Miaohe Lin]

If mpol_new is allocated but not used in restart loop, mpol_new will be
freed via mpol_put before returning to the caller.  But refcnt is not
initialized yet, so mpol_put could not do the right things and might leak
the unused mpol_new. This issue could be triggered easily with the below
code snippet if there're many processes doing the below work at the same
time:

  shmid = shmget((key_t)5566, 1024 * PAGE_SIZE, 0666|IPC_CREAT);
  shm = shmat(shmid, 0, 0);
  loop many times {
    mbind(shm, 1024 * PAGE_SIZE, MPOL_LOCAL, mask, maxnode, 0);
    mbind(shm + 128 * PAGE_SIZE, 128 * PAGE_SIZE, MPOL_DEFAULT, mask,
          maxnode, 0);
  }

Fixes: 42288fe366c4 ("mm: mempolicy: Convert shared_policy mutex to spinlock")
Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
Cc: <stable@vger.kernel.org> # 3.8
---
v1->v2:
  Add reproducer snippet and Cc stable.
  Thanks Michal Hocko for review and comment!
---
 mm/mempolicy.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index a2516d31db6c..4cdd425b2752 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -2733,6 +2733,7 @@ static int shared_policy_replace(struct shared_policy *sp, unsigned long start,
 	mpol_new = kmem_cache_alloc(policy_cache, GFP_KERNEL);
 	if (!mpol_new)
 		goto err_out;
+	refcount_set(&mpol_new->refcnt, 1);
 	goto restart;
 }
 
-- 
2.23.0