* [PATCH v3] mm/mempolicy: fix mpol_new leak in shared_policy_replace
@ 2022-03-22 10:43 Miaohe Lin
2022-03-26 0:29 ` Andrew Morton
0 siblings, 1 reply; 5+ messages in thread
From: Miaohe Lin @ 2022-03-22 10:43 UTC (permalink / raw)
To: akpm, mhocko; +Cc: kosaki.motohiro, mgorman, linux-mm, linux-kernel, linmiaohe
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);
}
Fixes: 42288fe366c4 ("mm: mempolicy: Convert shared_policy mutex to spinlock")
Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
Acked-by: Michal Hocko <mhocko@suse.com>
Cc: <stable@vger.kernel.org> # 3.8
---
v2->v3:
enhance commit log and collect Acked-by tag per Michal Hocko. Thanks!
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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH v3] mm/mempolicy: fix mpol_new leak in shared_policy_replace
2022-03-22 10:43 [PATCH v3] mm/mempolicy: fix mpol_new leak in shared_policy_replace Miaohe Lin
@ 2022-03-26 0:29 ` Andrew Morton
2022-03-26 6:46 ` Miaohe Lin
0 siblings, 1 reply; 5+ messages in thread
From: Andrew Morton @ 2022-03-26 0:29 UTC (permalink / raw)
To: Miaohe Lin; +Cc: mhocko, kosaki.motohiro, mgorman, linux-mm, linux-kernel
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()?
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v3] mm/mempolicy: fix mpol_new leak in shared_policy_replace
2022-03-26 0:29 ` Andrew Morton
@ 2022-03-26 6:46 ` Miaohe Lin
2022-03-28 21:26 ` Andrew Morton
0 siblings, 1 reply; 5+ messages in thread
From: Miaohe Lin @ 2022-03-26 6:46 UTC (permalink / raw)
To: Andrew Morton; +Cc: mhocko, kosaki.motohiro, mgorman, linux-mm, linux-kernel
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.
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v3] mm/mempolicy: fix mpol_new leak in shared_policy_replace
2022-03-26 6:46 ` Miaohe Lin
@ 2022-03-28 21:26 ` Andrew Morton
2022-03-29 2:13 ` Miaohe Lin
0 siblings, 1 reply; 5+ messages in thread
From: Andrew Morton @ 2022-03-28 21:26 UTC (permalink / raw)
To: Miaohe Lin; +Cc: mhocko, kosaki.motohiro, mgorman, linux-mm, linux-kernel
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.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v3] mm/mempolicy: fix mpol_new leak in shared_policy_replace
2022-03-28 21:26 ` Andrew Morton
@ 2022-03-29 2:13 ` Miaohe Lin
0 siblings, 0 replies; 5+ messages in thread
From: Miaohe Lin @ 2022-03-29 2:13 UTC (permalink / raw)
To: Andrew Morton; +Cc: mhocko, kosaki.motohiro, mgorman, linux-mm, linux-kernel
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.
> .
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-03-29 2:13 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-22 10:43 [PATCH v3] mm/mempolicy: fix mpol_new leak in shared_policy_replace Miaohe Lin
2022-03-26 0:29 ` Andrew Morton
2022-03-26 6:46 ` Miaohe Lin
2022-03-28 21:26 ` Andrew Morton
2022-03-29 2:13 ` Miaohe Lin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.