Message ID | 20220322104345.36379-1-linmiaohe@huawei.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | [v3] mm/mempolicy: fix mpol_new leak in shared_policy_replace | expand |
On Tue, 22 Mar 2022 18:43:45 +0800 Miaohe Lin <linmiaohe@huawei.com> 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. 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 > are 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); > } > > ... > > --- 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; > } Two other sites in this file do atomic_set(&policy->refcnt, 1); Could we please instead have a little helper function which does the kmem_cache_alloc()+refcount_set()?
On 2022/3/26 8:29, Andrew Morton wrote: > On Tue, 22 Mar 2022 18:43:45 +0800 Miaohe Lin <linmiaohe@huawei.com> 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. 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 >> are 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); >> } >> >> ... >> >> --- 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; >> } > > Two other sites in this file do > > atomic_set(&policy->refcnt, 1); > > > Could we please instead have a little helper function which does the > kmem_cache_alloc()+refcount_set()?> . There are usecases like below: struct mempolicy *new = kmem_cache_alloc(policy_cache, GFP_KERNEL); *new = *old; ^^^^^^^^^^^^ refcount_set(&new->refcnt, 1); If we use helper function to do kmem_cache_alloc()+refcount_set() above, separate refcount_set(&new->refcnt, 1) is still needed as old is copied to new and overwrites the refcnt field. So that little helper function might not work. Or am I miss something? Many thanks. >
On Sat, 26 Mar 2022 14:46:28 +0800 Miaohe Lin <linmiaohe@huawei.com> wrote: > On 2022/3/26 8:29, Andrew Morton wrote: > > On Tue, 22 Mar 2022 18:43:45 +0800 Miaohe Lin <linmiaohe@huawei.com> 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. 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 > >> are 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); > >> } > >> > >> ... > >> > >> --- 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; > >> } > > > > Two other sites in this file do > > > > atomic_set(&policy->refcnt, 1); > > > > > > Could we please instead have a little helper function which does the > > kmem_cache_alloc()+refcount_set()?> . > > There are usecases like below: > > struct mempolicy *new = kmem_cache_alloc(policy_cache, GFP_KERNEL); > *new = *old; > ^^^^^^^^^^^^ > refcount_set(&new->refcnt, 1); > > If we use helper function to do kmem_cache_alloc()+refcount_set() above, separate > refcount_set(&new->refcnt, 1) is still needed as old is copied to new and overwrites > the refcnt field. So that little helper function might not work. Or am I miss something? > Hm, spose so. I guess the helper doesn't add much in that case. Can we please redo this on mainline? I'm not happy with the bloat which refcnt_t adds and I think I'll drop mm-mempolicy-convert-from-atomic_t-to-refcount_t-on-mempolicy-refcnt.patch.
On 2022/3/29 5:26, Andrew Morton wrote: > On Sat, 26 Mar 2022 14:46:28 +0800 Miaohe Lin <linmiaohe@huawei.com> wrote: > >> On 2022/3/26 8:29, Andrew Morton wrote: >>> On Tue, 22 Mar 2022 18:43:45 +0800 Miaohe Lin <linmiaohe@huawei.com> 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. 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 >>>> are 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); >>>> } >>>> >>>> ... >>>> >>>> --- 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; >>>> } >>> >>> Two other sites in this file do >>> >>> atomic_set(&policy->refcnt, 1); >>> >>> >>> Could we please instead have a little helper function which does the >>> kmem_cache_alloc()+refcount_set()?> . >> >> There are usecases like below: >> >> struct mempolicy *new = kmem_cache_alloc(policy_cache, GFP_KERNEL); >> *new = *old; >> ^^^^^^^^^^^^ >> refcount_set(&new->refcnt, 1); >> >> If we use helper function to do kmem_cache_alloc()+refcount_set() above, separate >> refcount_set(&new->refcnt, 1) is still needed as old is copied to new and overwrites >> the refcnt field. So that little helper function might not work. Or am I miss something? >> > > Hm, spose so. I guess the helper doesn't add much in that case. > > Can we please redo this on mainline? I'm not happy with the bloat > which refcnt_t adds and I think I'll drop > mm-mempolicy-convert-from-atomic_t-to-refcount_t-on-mempolicy-refcnt.patch. Will do this soon. Many 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; }