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

[PATCH] mm/mempolicy: fix potential 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.

Fixes: 42288fe366c4 ("mm: mempolicy: Convert shared_policy mutex to spinlock")
Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/mempolicy.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 34d2b29c96ad..f19f19d3558b 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] mm/mempolicy: fix potential mpol_new leak in shared_policy_replace

[Michal Hocko]

On Fri 11-03-22 17:36:24, 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.

The code is really hideous but is there really any bug there? AFAICS the
new policy is only allocated in if (n->end > end) branch and that one
will set the reference count on the retry. Or am I missing something?

> Fixes: 42288fe366c4 ("mm: mempolicy: Convert shared_policy mutex to spinlock")
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>  mm/mempolicy.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index 34d2b29c96ad..f19f19d3558b 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] mm/mempolicy: fix potential mpol_new leak in shared_policy_replace

[Miaohe Lin]

On 2022/3/15 0:44, Michal Hocko wrote:
> On Fri 11-03-22 17:36:24, 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.
> 
> The code is really hideous but is there really any bug there? AFAICS the
> new policy is only allocated in if (n->end > end) branch and that one
> will set the reference count on the retry. Or am I missing something?
> 

Many thanks for your comment.
IIUC, new policy is allocated via the below code:

shared_policy_replace:
	alloc_new:
		write_unlock(&sp->lock);
		ret = -ENOMEM;
		n_new = kmem_cache_alloc(sn_cache, GFP_KERNEL);
		if (!n_new)
			goto err_out;
		mpol_new = kmem_cache_alloc(policy_cache, GFP_KERNEL);
		if (!mpol_new)
			goto err_out;
		goto restart;

And mpol_new' reference count will be set before used in n->end > end case. But
if that is "not" the case, i.e. mpol_new is not inserted into the rb_tree, mpol_new
will be freed via mpol_put before return:

shared_policy_replace:
	err_out:
		if (mpol_new)
			mpol_put(mpol_new);
		if (n_new)
			kmem_cache_free(sn_cache, n_new);

But mpol_new' reference count is not set yet, we have trouble now.
Does this makes sense for you? Or am I miss something?

Thanks.

>> Fixes: 42288fe366c4 ("mm: mempolicy: Convert shared_policy mutex to spinlock")
>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>> ---
>>  mm/mempolicy.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
>> index 34d2b29c96ad..f19f19d3558b 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] mm/mempolicy: fix potential mpol_new leak in shared_policy_replace

[Michal Hocko]

On Tue 15-03-22 21:42:29, Miaohe Lin wrote:
> On 2022/3/15 0:44, Michal Hocko wrote:
> > On Fri 11-03-22 17:36:24, 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.
> > 
> > The code is really hideous but is there really any bug there? AFAICS the
> > new policy is only allocated in if (n->end > end) branch and that one
> > will set the reference count on the retry. Or am I missing something?
> > 
> 
> Many thanks for your comment.
> IIUC, new policy is allocated via the below code:
> 
> shared_policy_replace:
> 	alloc_new:
> 		write_unlock(&sp->lock);
> 		ret = -ENOMEM;
> 		n_new = kmem_cache_alloc(sn_cache, GFP_KERNEL);
> 		if (!n_new)
> 			goto err_out;
> 		mpol_new = kmem_cache_alloc(policy_cache, GFP_KERNEL);
> 		if (!mpol_new)
> 			goto err_out;
> 		goto restart;
> 
> And mpol_new' reference count will be set before used in n->end > end case. But
> if that is "not" the case, i.e. mpol_new is not inserted into the rb_tree, mpol_new
> will be freed via mpol_put before return:

One thing I have missed previously is that the lock is dropped during
the allocation so I guess the memory policy could have been changed
during that time. Is this possible? Have you explored this possibility?
Is this a theoretical problem or it can be triggered intentionally.

These details would be really interesting for the changelog so that we
can judge how important this would be.
-- 
Michal Hocko
SUSE Labs

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

[Miaohe Lin]

On 2022/3/15 23:27, Michal Hocko wrote:
> On Tue 15-03-22 21:42:29, Miaohe Lin wrote:
>> On 2022/3/15 0:44, Michal Hocko wrote:
>>> On Fri 11-03-22 17:36:24, 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.
>>>
>>> The code is really hideous but is there really any bug there? AFAICS the
>>> new policy is only allocated in if (n->end > end) branch and that one
>>> will set the reference count on the retry. Or am I missing something?
>>>
>>
>> Many thanks for your comment.
>> IIUC, new policy is allocated via the below code:
>>
>> shared_policy_replace:
>> 	alloc_new:
>> 		write_unlock(&sp->lock);
>> 		ret = -ENOMEM;
>> 		n_new = kmem_cache_alloc(sn_cache, GFP_KERNEL);
>> 		if (!n_new)
>> 			goto err_out;
>> 		mpol_new = kmem_cache_alloc(policy_cache, GFP_KERNEL);
>> 		if (!mpol_new)
>> 			goto err_out;
>> 		goto restart;
>>
>> And mpol_new' reference count will be set before used in n->end > end case. But
>> if that is "not" the case, i.e. mpol_new is not inserted into the rb_tree, mpol_new
>> will be freed via mpol_put before return:
> 
> One thing I have missed previously is that the lock is dropped during
> the allocation so I guess the memory policy could have been changed
> during that time. Is this possible? Have you explored this possibility?
> Is this a theoretical problem or it can be triggered intentionally.
> 

This is found via code investigation. I think this could be triggered if there
are many concurrent mpol_set_shared_policy in place. But the user-visible effect
might be obscure as only sizeof(struct mempolicy) bytes leaks possiblely every time.

> These details would be really interesting for the changelog so that we
> can judge how important this would be.

This might not be that important as this issue should have been well-concealed for
almost ten years (since commit 42288fe366c4 ("mm: mempolicy: Convert shared_policy mutex to spinlock")).

> 

Thanks.

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

[Michal Hocko]

On Wed 16-03-22 14:39:37, Miaohe Lin wrote:
> On 2022/3/15 23:27, Michal Hocko wrote:
> > On Tue 15-03-22 21:42:29, Miaohe Lin wrote:
> >> On 2022/3/15 0:44, Michal Hocko wrote:
> >>> On Fri 11-03-22 17:36:24, 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.
> >>>
> >>> The code is really hideous but is there really any bug there? AFAICS the
> >>> new policy is only allocated in if (n->end > end) branch and that one
> >>> will set the reference count on the retry. Or am I missing something?
> >>>
> >>
> >> Many thanks for your comment.
> >> IIUC, new policy is allocated via the below code:
> >>
> >> shared_policy_replace:
> >> 	alloc_new:
> >> 		write_unlock(&sp->lock);
> >> 		ret = -ENOMEM;
> >> 		n_new = kmem_cache_alloc(sn_cache, GFP_KERNEL);
> >> 		if (!n_new)
> >> 			goto err_out;
> >> 		mpol_new = kmem_cache_alloc(policy_cache, GFP_KERNEL);
> >> 		if (!mpol_new)
> >> 			goto err_out;
> >> 		goto restart;
> >>
> >> And mpol_new' reference count will be set before used in n->end > end case. But
> >> if that is "not" the case, i.e. mpol_new is not inserted into the rb_tree, mpol_new
> >> will be freed via mpol_put before return:
> > 
> > One thing I have missed previously is that the lock is dropped during
> > the allocation so I guess the memory policy could have been changed
> > during that time. Is this possible? Have you explored this possibility?
> > Is this a theoretical problem or it can be triggered intentionally.
> > 
> 
> This is found via code investigation. I think this could be triggered if there
> are many concurrent mpol_set_shared_policy in place. But the user-visible effect
> might be obscure as only sizeof(struct mempolicy) bytes leaks possiblely every time.
> 
> > These details would be really interesting for the changelog so that we
> > can judge how important this would be.
> 
> This might not be that important as this issue should have been well-concealed for
> almost ten years (since commit 42288fe366c4 ("mm: mempolicy: Convert shared_policy mutex to spinlock")).

I think it is really worth to drill down to the bottom of the issue.
While theoretically possible can be a good enough to justify the change
it is usually preferred to describe the underlying problem for future
maintainability. 
-- 
Michal Hocko
SUSE Labs

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

[Miaohe Lin]

On 2022/3/16 17:56, Michal Hocko wrote:
> On Wed 16-03-22 14:39:37, Miaohe Lin wrote:
>> On 2022/3/15 23:27, Michal Hocko wrote:
>>> On Tue 15-03-22 21:42:29, Miaohe Lin wrote:
>>>> On 2022/3/15 0:44, Michal Hocko wrote:
>>>>> On Fri 11-03-22 17:36:24, 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.
>>>>>
>>>>> The code is really hideous but is there really any bug there? AFAICS the
>>>>> new policy is only allocated in if (n->end > end) branch and that one
>>>>> will set the reference count on the retry. Or am I missing something?
>>>>>
>>>>
>>>> Many thanks for your comment.
>>>> IIUC, new policy is allocated via the below code:
>>>>
>>>> shared_policy_replace:
>>>> 	alloc_new:
>>>> 		write_unlock(&sp->lock);
>>>> 		ret = -ENOMEM;
>>>> 		n_new = kmem_cache_alloc(sn_cache, GFP_KERNEL);
>>>> 		if (!n_new)
>>>> 			goto err_out;
>>>> 		mpol_new = kmem_cache_alloc(policy_cache, GFP_KERNEL);
>>>> 		if (!mpol_new)
>>>> 			goto err_out;
>>>> 		goto restart;
>>>>
>>>> And mpol_new' reference count will be set before used in n->end > end case. But
>>>> if that is "not" the case, i.e. mpol_new is not inserted into the rb_tree, mpol_new
>>>> will be freed via mpol_put before return:
>>>
>>> One thing I have missed previously is that the lock is dropped during
>>> the allocation so I guess the memory policy could have been changed
>>> during that time. Is this possible? Have you explored this possibility?
>>> Is this a theoretical problem or it can be triggered intentionally.
>>>
>>
>> This is found via code investigation. I think this could be triggered if there
>> are many concurrent mpol_set_shared_policy in place. But the user-visible effect
>> might be obscure as only sizeof(struct mempolicy) bytes leaks possiblely every time.
>>
>>> These details would be really interesting for the changelog so that we
>>> can judge how important this would be.
>>
>> This might not be that important as this issue should have been well-concealed for
>> almost ten years (since commit 42288fe366c4 ("mm: mempolicy: Convert shared_policy mutex to spinlock")).
> 
> I think it is really worth to drill down to the bottom of the issue.
> While theoretically possible can be a good enough to justify the change
> it is usually preferred to describe the underlying problem for future
> maintainability. 

This issue mainly causes mpol_new memory leaks and this is pointed out in the commit log.
Am I supposed to do something more to move forward this patch ? Could you point that out
for me?

Many thanks!

> 

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

[Michal Hocko]

On Thu 17-03-22 10:05:08, Miaohe Lin wrote:
> On 2022/3/16 17:56, Michal Hocko wrote:
> > On Wed 16-03-22 14:39:37, Miaohe Lin wrote:
> >> On 2022/3/15 23:27, Michal Hocko wrote:
> >>> On Tue 15-03-22 21:42:29, Miaohe Lin wrote:
> >>>> On 2022/3/15 0:44, Michal Hocko wrote:
> >>>>> On Fri 11-03-22 17:36:24, 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.
> >>>>>
> >>>>> The code is really hideous but is there really any bug there? AFAICS the
> >>>>> new policy is only allocated in if (n->end > end) branch and that one
> >>>>> will set the reference count on the retry. Or am I missing something?
> >>>>>
> >>>>
> >>>> Many thanks for your comment.
> >>>> IIUC, new policy is allocated via the below code:
> >>>>
> >>>> shared_policy_replace:
> >>>> 	alloc_new:
> >>>> 		write_unlock(&sp->lock);
> >>>> 		ret = -ENOMEM;
> >>>> 		n_new = kmem_cache_alloc(sn_cache, GFP_KERNEL);
> >>>> 		if (!n_new)
> >>>> 			goto err_out;
> >>>> 		mpol_new = kmem_cache_alloc(policy_cache, GFP_KERNEL);
> >>>> 		if (!mpol_new)
> >>>> 			goto err_out;
> >>>> 		goto restart;
> >>>>
> >>>> And mpol_new' reference count will be set before used in n->end > end case. But
> >>>> if that is "not" the case, i.e. mpol_new is not inserted into the rb_tree, mpol_new
> >>>> will be freed via mpol_put before return:
> >>>
> >>> One thing I have missed previously is that the lock is dropped during
> >>> the allocation so I guess the memory policy could have been changed
> >>> during that time. Is this possible? Have you explored this possibility?
> >>> Is this a theoretical problem or it can be triggered intentionally.
> >>>
> >>
> >> This is found via code investigation. I think this could be triggered if there
> >> are many concurrent mpol_set_shared_policy in place. But the user-visible effect
> >> might be obscure as only sizeof(struct mempolicy) bytes leaks possiblely every time.
> >>
> >>> These details would be really interesting for the changelog so that we
> >>> can judge how important this would be.
> >>
> >> This might not be that important as this issue should have been well-concealed for
> >> almost ten years (since commit 42288fe366c4 ("mm: mempolicy: Convert shared_policy mutex to spinlock")).
> > 
> > I think it is really worth to drill down to the bottom of the issue.
> > While theoretically possible can be a good enough to justify the change
> > it is usually preferred to describe the underlying problem for future
> > maintainability. 
> 
> This issue mainly causes mpol_new memory leaks and this is pointed out in the commit log.
> Am I supposed to do something more to move forward this patch ? Could you point that out
> for me?

Sorry if I was not really clear. My main request is to have a clear
insight whether this is a theretical issue or the leak could be really
triggered. If the later we need to mark it properly and backport to
older kernels because memory leaks can lead to DoS when they are
reasonably easy to trigger.

Is this more clear now?
-- 
Michal Hocko
SUSE Labs

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

[Miaohe Lin]

On 2022/3/17 17:03, Michal Hocko wrote:
> On Thu 17-03-22 10:05:08, Miaohe Lin wrote:
>> On 2022/3/16 17:56, Michal Hocko wrote:
>>> On Wed 16-03-22 14:39:37, Miaohe Lin wrote:
>>>> On 2022/3/15 23:27, Michal Hocko wrote:
>>>>> On Tue 15-03-22 21:42:29, Miaohe Lin wrote:
>>>>>> On 2022/3/15 0:44, Michal Hocko wrote:
>>>>>>> On Fri 11-03-22 17:36:24, 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.
>>>>>>>
>>>>>>> The code is really hideous but is there really any bug there? AFAICS the
>>>>>>> new policy is only allocated in if (n->end > end) branch and that one
>>>>>>> will set the reference count on the retry. Or am I missing something?
>>>>>>>
>>>>>>
>>>>>> Many thanks for your comment.
>>>>>> IIUC, new policy is allocated via the below code:
>>>>>>
>>>>>> shared_policy_replace:
>>>>>> 	alloc_new:
>>>>>> 		write_unlock(&sp->lock);
>>>>>> 		ret = -ENOMEM;
>>>>>> 		n_new = kmem_cache_alloc(sn_cache, GFP_KERNEL);
>>>>>> 		if (!n_new)
>>>>>> 			goto err_out;
>>>>>> 		mpol_new = kmem_cache_alloc(policy_cache, GFP_KERNEL);
>>>>>> 		if (!mpol_new)
>>>>>> 			goto err_out;
>>>>>> 		goto restart;
>>>>>>
>>>>>> And mpol_new' reference count will be set before used in n->end > end case. But
>>>>>> if that is "not" the case, i.e. mpol_new is not inserted into the rb_tree, mpol_new
>>>>>> will be freed via mpol_put before return:
>>>>>
>>>>> One thing I have missed previously is that the lock is dropped during
>>>>> the allocation so I guess the memory policy could have been changed
>>>>> during that time. Is this possible? Have you explored this possibility?
>>>>> Is this a theoretical problem or it can be triggered intentionally.
>>>>>
>>>>
>>>> This is found via code investigation. I think this could be triggered if there
>>>> are many concurrent mpol_set_shared_policy in place. But the user-visible effect
>>>> might be obscure as only sizeof(struct mempolicy) bytes leaks possiblely every time.
>>>>
>>>>> These details would be really interesting for the changelog so that we
>>>>> can judge how important this would be.
>>>>
>>>> This might not be that important as this issue should have been well-concealed for
>>>> almost ten years (since commit 42288fe366c4 ("mm: mempolicy: Convert shared_policy mutex to spinlock")).
>>>
>>> I think it is really worth to drill down to the bottom of the issue.
>>> While theoretically possible can be a good enough to justify the change
>>> it is usually preferred to describe the underlying problem for future
>>> maintainability. 
>>
>> This issue mainly causes mpol_new memory leaks and this is pointed out in the commit log.
>> Am I supposed to do something more to move forward this patch ? Could you point that out
>> for me?
> 
> Sorry if I was not really clear. My main request is to have a clear
> insight whether this is a theretical issue or the leak could be really
> triggered. If the later we need to mark it properly and backport to
> older kernels because memory leaks can lead to DoS when they are
> reasonably easy to trigger.
> 
> Is this more clear now?

I see. Many thanks. I would have a try to trigger this. :)

> 

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

[Miaohe Lin]

On 2022/3/17 17:34, Miaohe Lin wrote:
> On 2022/3/17 17:03, Michal Hocko wrote:
>> On Thu 17-03-22 10:05:08, Miaohe Lin wrote:
>>> On 2022/3/16 17:56, Michal Hocko wrote:
>>>> On Wed 16-03-22 14:39:37, Miaohe Lin wrote:
>>>>> On 2022/3/15 23:27, Michal Hocko wrote:
>>>>>> On Tue 15-03-22 21:42:29, Miaohe Lin wrote:
>>>>>>> On 2022/3/15 0:44, Michal Hocko wrote:
>>>>>>>> On Fri 11-03-22 17:36:24, 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.
>>>>>>>>
>>>>>>>> The code is really hideous but is there really any bug there? AFAICS the
>>>>>>>> new policy is only allocated in if (n->end > end) branch and that one
>>>>>>>> will set the reference count on the retry. Or am I missing something?
>>>>>>>>
>>>>>>>
>>>>>>> Many thanks for your comment.
>>>>>>> IIUC, new policy is allocated via the below code:
>>>>>>>
>>>>>>> shared_policy_replace:
>>>>>>> 	alloc_new:
>>>>>>> 		write_unlock(&sp->lock);
>>>>>>> 		ret = -ENOMEM;
>>>>>>> 		n_new = kmem_cache_alloc(sn_cache, GFP_KERNEL);
>>>>>>> 		if (!n_new)
>>>>>>> 			goto err_out;
>>>>>>> 		mpol_new = kmem_cache_alloc(policy_cache, GFP_KERNEL);
>>>>>>> 		if (!mpol_new)
>>>>>>> 			goto err_out;
>>>>>>> 		goto restart;
>>>>>>>
>>>>>>> And mpol_new' reference count will be set before used in n->end > end case. But
>>>>>>> if that is "not" the case, i.e. mpol_new is not inserted into the rb_tree, mpol_new
>>>>>>> will be freed via mpol_put before return:
>>>>>>
>>>>>> One thing I have missed previously is that the lock is dropped during
>>>>>> the allocation so I guess the memory policy could have been changed
>>>>>> during that time. Is this possible? Have you explored this possibility?
>>>>>> Is this a theoretical problem or it can be triggered intentionally.
>>>>>>
>>>>>
>>>>> This is found via code investigation. I think this could be triggered if there
>>>>> are many concurrent mpol_set_shared_policy in place. But the user-visible effect
>>>>> might be obscure as only sizeof(struct mempolicy) bytes leaks possiblely every time.
>>>>>
>>>>>> These details would be really interesting for the changelog so that we
>>>>>> can judge how important this would be.
>>>>>
>>>>> This might not be that important as this issue should have been well-concealed for
>>>>> almost ten years (since commit 42288fe366c4 ("mm: mempolicy: Convert shared_policy mutex to spinlock")).
>>>>
>>>> I think it is really worth to drill down to the bottom of the issue.
>>>> While theoretically possible can be a good enough to justify the change
>>>> it is usually preferred to describe the underlying problem for future
>>>> maintainability. 
>>>
>>> This issue mainly causes mpol_new memory leaks and this is pointed out in the commit log.
>>> Am I supposed to do something more to move forward this patch ? Could you point that out
>>> for me?
>>
>> Sorry if I was not really clear. My main request is to have a clear
>> insight whether this is a theretical issue or the leak could be really
>> triggered. If the later we need to mark it properly and backport to
>> older kernels because memory leaks can lead to DoS when they are
>> reasonably easy to trigger.
>>
>> Is this more clear now?
> 
> I see. Many thanks. I would have a try to trigger this. :)
> 

This would be triggered easily with below code snippet in my virtual machine:

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

If there're many process doing the above work, mpol_new will be leaked easily.
So should I resend this patch with Cc stable? But it seems I'am not supposed
to make this decision and the maintainer will take care of this?

Many thanks. :)

>>
> 

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

[Michal Hocko]

On Sat 19-03-22 18:42:33, Miaohe Lin wrote:
[...]
> This would be triggered easily with below code snippet in my virtual machine:
> 
> 	shmid = shmget((key_t)5566, 1024 * PAGE_SIZE, 0666|IPC_CREAT);
> 	shm = shmat(shmid, 0, 0);
> 	loop {
> 		mbind(shm, 1024 * PAGE_SIZE, MPOL_LOCAL, mask, maxnode, 0);
> 		mbind(shm + 128 * PAGE_SIZE, 128 * PAGE_SIZE, MPOL_DEFAULT, mask, maxnode, 0);
> 	}
> 
> If there're many process doing the above work, mpol_new will be leaked easily.
> So should I resend this patch with Cc stable? But it seems I'am not supposed
> to make this decision and the maintainer will take care of this?

I would just add
Fixes: 42288fe366c4 ("mm: mempolicy: Convert shared_policy mutex to spinlock")
Cc: stable # 3.8

And also add your above reproducer snippet added to the original changelog.
This would be more then enough to conclude the importance.

Thank you for working hard on this!
-- 
Michal Hocko
SUSE Labs

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

[Miaohe Lin]

On 2022/3/21 16:59, Michal Hocko wrote:
> On Sat 19-03-22 18:42:33, Miaohe Lin wrote:
> [...]
>> This would be triggered easily with below code snippet in my virtual machine:
>>
>> 	shmid = shmget((key_t)5566, 1024 * PAGE_SIZE, 0666|IPC_CREAT);
>> 	shm = shmat(shmid, 0, 0);
>> 	loop {
>> 		mbind(shm, 1024 * PAGE_SIZE, MPOL_LOCAL, mask, maxnode, 0);
>> 		mbind(shm + 128 * PAGE_SIZE, 128 * PAGE_SIZE, MPOL_DEFAULT, mask, maxnode, 0);
>> 	}
>>
>> If there're many process doing the above work, mpol_new will be leaked easily.
>> So should I resend this patch with Cc stable? But it seems I'am not supposed
>> to make this decision and the maintainer will take care of this?
> 
> I would just add
> Fixes: 42288fe366c4 ("mm: mempolicy: Convert shared_policy mutex to spinlock")
> Cc: stable # 3.8
> 
> And also add your above reproducer snippet added to the original changelog.
> This would be more then enough to conclude the importance.

Will do. Many thanks for your patience and suggestion!

> 
> Thank you for working hard on this!

Thanks. :)

>