| Message ID | 20220322083456.16563-1-linmiaohe@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] mm/mempolicy: fix mpol_new leak in shared_policy_replace | expand |
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
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
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
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 >
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 >
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!
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; }
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(+)