linux-api.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH] mm/mremap: Fail map duplication attempts for private mappings
       [not found] ` <1499961495-8063-1-git-send-email-mike.kravetz-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
@ 2017-07-13 19:11   ` Vlastimil Babka
  2017-07-13 22:33     ` Mike Kravetz
  2017-07-19 16:39     ` Mike Kravetz
  0 siblings, 2 replies; 12+ messages in thread
From: Vlastimil Babka @ 2017-07-13 19:11 UTC (permalink / raw)
  To: Mike Kravetz, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA
  Cc: Andrew Morton, Andrea Arcangeli, Michal Hocko, Aaron Lu,
	Kirill A . Shutemov, Anshuman Khandual, Linux API

[+CC linux-api]

On 07/13/2017 05:58 PM, Mike Kravetz wrote:
> mremap will create a 'duplicate' mapping if old_size == 0 is
> specified.  Such duplicate mappings make no sense for private
> mappings.  If duplication is attempted for a private mapping,
> mremap creates a separate private mapping unrelated to the
> original mapping and makes no modifications to the original.
> This is contrary to the purpose of mremap which should return
> a mapping which is in some way related to the original.
> 
> Therefore, return EINVAL in the case where if an attempt is
> made to duplicate a private mapping.
> 
> Signed-off-by: Mike Kravetz <mike.kravetz-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>

Acked-by: Vlastimil Babka <vbabka-AlSwsSmVLrQ@public.gmane.org>

> ---
>  mm/mremap.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/mm/mremap.c b/mm/mremap.c
> index cd8a1b1..076f506 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -383,6 +383,13 @@ static struct vm_area_struct *vma_to_resize(unsigned long addr,
>  	if (!vma || vma->vm_start > addr)
>  		return ERR_PTR(-EFAULT);
>  
> +	/*
> +	 * !old_len  is a special case where a mapping is 'duplicated'.
> +	 * Do not allow this for private mappings.
> +	 */
> +	if (!old_len && !(vma->vm_flags & (VM_SHARED | VM_MAYSHARE)))
> +		return ERR_PTR(-EINVAL);
> +
>  	if (is_vm_hugetlb_page(vma))
>  		return ERR_PTR(-EINVAL);
>  
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] mm/mremap: Fail map duplication attempts for private mappings
  2017-07-13 19:11   ` [PATCH] mm/mremap: Fail map duplication attempts for private mappings Vlastimil Babka
@ 2017-07-13 22:33     ` Mike Kravetz
  2017-07-14  4:51       ` Anshuman Khandual
       [not found]       ` <415625d2-1be9-71f0-ca11-a014cef98a3f-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
  2017-07-19 16:39     ` Mike Kravetz
  1 sibling, 2 replies; 12+ messages in thread
From: Mike Kravetz @ 2017-07-13 22:33 UTC (permalink / raw)
  To: Vlastimil Babka, linux-mm, linux-kernel
  Cc: Andrew Morton, Andrea Arcangeli, Michal Hocko, Aaron Lu,
	Kirill A . Shutemov, Anshuman Khandual, Linux API

On 07/13/2017 12:11 PM, Vlastimil Babka wrote:
> [+CC linux-api]
> 
> On 07/13/2017 05:58 PM, Mike Kravetz wrote:
>> mremap will create a 'duplicate' mapping if old_size == 0 is
>> specified.  Such duplicate mappings make no sense for private
>> mappings.  If duplication is attempted for a private mapping,
>> mremap creates a separate private mapping unrelated to the
>> original mapping and makes no modifications to the original.
>> This is contrary to the purpose of mremap which should return
>> a mapping which is in some way related to the original.
>>
>> Therefore, return EINVAL in the case where if an attempt is
>> made to duplicate a private mapping.
>>
>> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
> 
> Acked-by: Vlastimil Babka <vbabka@suse.cz>
> 

In another e-mail thread, Andrea makes the case that mremap(old_size == 0)
of private file backed mappings could possibly be used for something useful.
For example to create a private COW mapping.  Of course, a better way to do
this would be simply using the fd to create a private mapping.

If returning EINVAL for all private mappings is too general, the following
patch adds a check to only return EINVAL for private anon mappings.

mm/mremap: Fail map duplication attempts for private anon mappings

mremap will create a 'duplicate' mapping if old_size == 0 is
specified.  Such duplicate mappings make no sense for private
anonymous mappings.  If duplication is attempted for a private
anon mapping, mremap creates a separate private mapping unrelated
to the original mapping and makes no modifications to the original.
This is contrary to the purpose of mremap which should return a
mapping which is in some way related to the original.

Therefore, return EINVAL in the case where an attempt is made to
duplicate a private anon mapping.

Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
---
 mm/mremap.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/mm/mremap.c b/mm/mremap.c
index cd8a1b1..586ea3d 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -383,6 +383,14 @@ static struct vm_area_struct *vma_to_resize(unsigned long addr,
 	if (!vma || vma->vm_start > addr)
 		return ERR_PTR(-EFAULT);
 
+	/*
+	 * !old_len  is a special case where a mapping is 'duplicated'.
+	 * Do not allow this for private anon mappings.
+	 */
+	if (!old_len && vma_is_anonymous(vma) &&
+	    !(vma->vm_flags & (VM_SHARED | VM_MAYSHARE)))
+		return ERR_PTR(-EINVAL);
+
 	if (is_vm_hugetlb_page(vma))
 		return ERR_PTR(-EINVAL);
 
-- 
2.7.5

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] mm/mremap: Fail map duplication attempts for private mappings
  2017-07-13 22:33     ` Mike Kravetz
@ 2017-07-14  4:51       ` Anshuman Khandual
       [not found]       ` <415625d2-1be9-71f0-ca11-a014cef98a3f-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
  1 sibling, 0 replies; 12+ messages in thread
From: Anshuman Khandual @ 2017-07-14  4:51 UTC (permalink / raw)
  To: Mike Kravetz, Vlastimil Babka, linux-mm, linux-kernel
  Cc: Andrew Morton, Andrea Arcangeli, Michal Hocko, Aaron Lu,
	Kirill A . Shutemov, Anshuman Khandual, Linux API

On 07/14/2017 04:03 AM, Mike Kravetz wrote:
> On 07/13/2017 12:11 PM, Vlastimil Babka wrote:
>> [+CC linux-api]
>>
>> On 07/13/2017 05:58 PM, Mike Kravetz wrote:
>>> mremap will create a 'duplicate' mapping if old_size == 0 is
>>> specified.  Such duplicate mappings make no sense for private
>>> mappings.  If duplication is attempted for a private mapping,
>>> mremap creates a separate private mapping unrelated to the
>>> original mapping and makes no modifications to the original.
>>> This is contrary to the purpose of mremap which should return
>>> a mapping which is in some way related to the original.
>>>
>>> Therefore, return EINVAL in the case where if an attempt is
>>> made to duplicate a private mapping.
>>>
>>> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
>> Acked-by: Vlastimil Babka <vbabka@suse.cz>
>>
> In another e-mail thread, Andrea makes the case that mremap(old_size == 0)
> of private file backed mappings could possibly be used for something useful.
> For example to create a private COW mapping.  Of course, a better way to do
> this would be simply using the fd to create a private mapping.
> 
> If returning EINVAL for all private mappings is too general, the following
> patch adds a check to only return EINVAL for private anon mappings.
> 
> mm/mremap: Fail map duplication attempts for private anon mappings
> 
> mremap will create a 'duplicate' mapping if old_size == 0 is
> specified.  Such duplicate mappings make no sense for private
> anonymous mappings.  If duplication is attempted for a private
> anon mapping, mremap creates a separate private mapping unrelated
> to the original mapping and makes no modifications to the original.
> This is contrary to the purpose of mremap which should return a
> mapping which is in some way related to the original.
> 
> Therefore, return EINVAL in the case where an attempt is made to
> duplicate a private anon mapping.
> 
> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
> ---
>  mm/mremap.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/mm/mremap.c b/mm/mremap.c
> index cd8a1b1..586ea3d 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -383,6 +383,14 @@ static struct vm_area_struct *vma_to_resize(unsigned long addr,
>  	if (!vma || vma->vm_start > addr)
>  		return ERR_PTR(-EFAULT);
>  
> +	/*
> +	 * !old_len  is a special case where a mapping is 'duplicated'.
> +	 * Do not allow this for private anon mappings.
> +	 */
> +	if (!old_len && vma_is_anonymous(vma) &&
> +	    !(vma->vm_flags & (VM_SHARED | VM_MAYSHARE)))
> +		return ERR_PTR(-EINVAL);

Sounds better compared to rejecting everything private.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] mm/mremap: Fail map duplication attempts for private mappings
       [not found]       ` <415625d2-1be9-71f0-ca11-a014cef98a3f-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
@ 2017-07-14  8:26         ` Michal Hocko
  2017-07-14 17:29           ` Mike Kravetz
  0 siblings, 1 reply; 12+ messages in thread
From: Michal Hocko @ 2017-07-14  8:26 UTC (permalink / raw)
  To: Mike Kravetz
  Cc: Vlastimil Babka, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Andrew Morton,
	Andrea Arcangeli, Aaron Lu, Kirill A . Shutemov,
	Anshuman Khandual, Linux API

On Thu 13-07-17 15:33:47, Mike Kravetz wrote:
> On 07/13/2017 12:11 PM, Vlastimil Babka wrote:
> > [+CC linux-api]
> > 
> > On 07/13/2017 05:58 PM, Mike Kravetz wrote:
> >> mremap will create a 'duplicate' mapping if old_size == 0 is
> >> specified.  Such duplicate mappings make no sense for private
> >> mappings.  If duplication is attempted for a private mapping,
> >> mremap creates a separate private mapping unrelated to the
> >> original mapping and makes no modifications to the original.
> >> This is contrary to the purpose of mremap which should return
> >> a mapping which is in some way related to the original.
> >>
> >> Therefore, return EINVAL in the case where if an attempt is
> >> made to duplicate a private mapping.
> >>
> >> Signed-off-by: Mike Kravetz <mike.kravetz-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> > 
> > Acked-by: Vlastimil Babka <vbabka-AlSwsSmVLrQ@public.gmane.org>
> > 
> 
> In another e-mail thread, Andrea makes the case that mremap(old_size == 0)
> of private file backed mappings could possibly be used for something useful.
> For example to create a private COW mapping.

What does this mean exactly? I do not see it would force CoW so again
the new mapping could fail with the basic invariant that the content
of the new mapping should match the old one (e.g. old mapping already
CoWed some pages the new mapping would still contain the origin content
unless I am missing something).

[...]
> +	/*
> +	 * !old_len  is a special case where a mapping is 'duplicated'.
> +	 * Do not allow this for private anon mappings.
> +	 */
> +	if (!old_len && vma_is_anonymous(vma) &&
> +	    !(vma->vm_flags & (VM_SHARED | VM_MAYSHARE)))
> +		return ERR_PTR(-EINVAL);

Why is vma_is_anonymous() without VM_*SHARE* check insufficient?
-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] mm/mremap: Fail map duplication attempts for private mappings
  2017-07-14  8:26         ` Michal Hocko
@ 2017-07-14 17:29           ` Mike Kravetz
  2017-07-17  6:44             ` Michal Hocko
  0 siblings, 1 reply; 12+ messages in thread
From: Mike Kravetz @ 2017-07-14 17:29 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Vlastimil Babka, linux-mm, linux-kernel, Andrew Morton,
	Andrea Arcangeli, Aaron Lu, Kirill A . Shutemov,
	Anshuman Khandual, Linux API

On 07/14/2017 01:26 AM, Michal Hocko wrote:
> On Thu 13-07-17 15:33:47, Mike Kravetz wrote:
>> On 07/13/2017 12:11 PM, Vlastimil Babka wrote:
>>> [+CC linux-api]
>>>
>>> On 07/13/2017 05:58 PM, Mike Kravetz wrote:
>>>> mremap will create a 'duplicate' mapping if old_size == 0 is
>>>> specified.  Such duplicate mappings make no sense for private
>>>> mappings.  If duplication is attempted for a private mapping,
>>>> mremap creates a separate private mapping unrelated to the
>>>> original mapping and makes no modifications to the original.
>>>> This is contrary to the purpose of mremap which should return
>>>> a mapping which is in some way related to the original.
>>>>
>>>> Therefore, return EINVAL in the case where if an attempt is
>>>> made to duplicate a private mapping.
>>>>
>>>> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
>>>
>>> Acked-by: Vlastimil Babka <vbabka@suse.cz>
>>>
>>
>> In another e-mail thread, Andrea makes the case that mremap(old_size == 0)
>> of private file backed mappings could possibly be used for something useful.
>> For example to create a private COW mapping.
> 
> What does this mean exactly? I do not see it would force CoW so again
> the new mapping could fail with the basic invariant that the content
> of the new mapping should match the old one (e.g. old mapping already
> CoWed some pages the new mapping would still contain the origin content
> unless I am missing something).

I do not think you are missing anything.  You are correct in saying that
the new mapping would be COW of the original file contents.  It is NOT
based on any private pages of the old private mapping.  Sorry, my wording
above was not quite clear.

As previously discussed, the more straight forward to way to accomplish
the same thing would be a simple call to mmap with the fd.

After thinking about this some more, perhaps the original patch to return
EINVAL for all private mappings makes more sense.  Even in the case of a
file backed private mapping, the new mapping will be based on the file and
not the old mapping.  The purpose of mremap is to create a new mapping
based on the old mapping.  So, this is not strictly in line with the purpose
of mremap.

Actually, the more I think about this, the more I wish there was some way
to deprecate and eventually eliminate the old_size == 0 behavior.

> [...]
>> +	/*
>> +	 * !old_len  is a special case where a mapping is 'duplicated'.
>> +	 * Do not allow this for private anon mappings.
>> +	 */
>> +	if (!old_len && vma_is_anonymous(vma) &&
>> +	    !(vma->vm_flags & (VM_SHARED | VM_MAYSHARE)))
>> +		return ERR_PTR(-EINVAL);
> 
> Why is vma_is_anonymous() without VM_*SHARE* check insufficient?

Are you asking,
why is if (!old_len && vma_is_anonymous(vma)) insufficient?

If so, you are correct that the additional check for VM_*SHARE* is not
necessary.  Shared mappings are technically not anonymous as they must
contain a common backing object.

The !(vma->vm_flags & (VM_SHARED | VM_MAYSHARE) check was there in the first
patch to catch all private mappings.  When adding vma_is_anonymous(vma), I
missed the fact that it was redundant.  But, based on your comments above
I think the first patch is more correct.

-- 
Mike Kravetz

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] mm/mremap: Fail map duplication attempts for private mappings
  2017-07-14 17:29           ` Mike Kravetz
@ 2017-07-17  6:44             ` Michal Hocko
  0 siblings, 0 replies; 12+ messages in thread
From: Michal Hocko @ 2017-07-17  6:44 UTC (permalink / raw)
  To: Mike Kravetz
  Cc: Vlastimil Babka, linux-mm, linux-kernel, Andrew Morton,
	Andrea Arcangeli, Aaron Lu, Kirill A . Shutemov,
	Anshuman Khandual, Linux API

On Fri 14-07-17 10:29:01, Mike Kravetz wrote:
> On 07/14/2017 01:26 AM, Michal Hocko wrote:
> > On Thu 13-07-17 15:33:47, Mike Kravetz wrote:
> >> On 07/13/2017 12:11 PM, Vlastimil Babka wrote:
> >>> [+CC linux-api]
> >>>
> >>> On 07/13/2017 05:58 PM, Mike Kravetz wrote:
> >>>> mremap will create a 'duplicate' mapping if old_size == 0 is
> >>>> specified.  Such duplicate mappings make no sense for private
> >>>> mappings.  If duplication is attempted for a private mapping,
> >>>> mremap creates a separate private mapping unrelated to the
> >>>> original mapping and makes no modifications to the original.
> >>>> This is contrary to the purpose of mremap which should return
> >>>> a mapping which is in some way related to the original.
> >>>>
> >>>> Therefore, return EINVAL in the case where if an attempt is
> >>>> made to duplicate a private mapping.
> >>>>
> >>>> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
> >>>
> >>> Acked-by: Vlastimil Babka <vbabka@suse.cz>
> >>>
> >>
> >> In another e-mail thread, Andrea makes the case that mremap(old_size == 0)
> >> of private file backed mappings could possibly be used for something useful.
> >> For example to create a private COW mapping.
> > 
> > What does this mean exactly? I do not see it would force CoW so again
> > the new mapping could fail with the basic invariant that the content
> > of the new mapping should match the old one (e.g. old mapping already
> > CoWed some pages the new mapping would still contain the origin content
> > unless I am missing something).
> 
> I do not think you are missing anything.  You are correct in saying that
> the new mapping would be COW of the original file contents.  It is NOT
> based on any private pages of the old private mapping.  Sorry, my wording
> above was not quite clear.
> 
> As previously discussed, the more straight forward to way to accomplish
> the same thing would be a simple call to mmap with the fd.
> 
> After thinking about this some more, perhaps the original patch to return
> EINVAL for all private mappings makes more sense.  Even in the case of a
> file backed private mapping, the new mapping will be based on the file and
> not the old mapping.  The purpose of mremap is to create a new mapping
> based on the old mapping.  So, this is not strictly in line with the purpose
> of mremap.

Yes that is exactly my point. One would expect that the new mapping has
the same content as the previous mapping at the time when it was created
and the copy will be "atomic" (wrt. page faults). Otherwise you could
simply implement it in the userspace.

That being said, I do not think we should try to pretend this is a
correct behavior and the !old_len should be supported only for the
shared mappings which have at least reasonable semantic.

> Actually, the more I think about this, the more I wish there was some way
> to deprecate and eventually eliminate the old_size == 0 behavior.
> 
> > [...]
> >> +	/*
> >> +	 * !old_len  is a special case where a mapping is 'duplicated'.
> >> +	 * Do not allow this for private anon mappings.
> >> +	 */
> >> +	if (!old_len && vma_is_anonymous(vma) &&
> >> +	    !(vma->vm_flags & (VM_SHARED | VM_MAYSHARE)))
> >> +		return ERR_PTR(-EINVAL);
> > 
> > Why is vma_is_anonymous() without VM_*SHARE* check insufficient?
> 
> Are you asking,
> why is if (!old_len && vma_is_anonymous(vma)) insufficient?

yes

> If so, you are correct that the additional check for VM_*SHARE* is not
> necessary.  Shared mappings are technically not anonymous as they must
> contain a common backing object.

that is my understanding as well. But maybe there are some weird
mappings which do not have vm_ops and populate the whole range inside
the mmap callback. I remember we had a CVE for those but forgot all
details of course. Failing on those doesn't seem like a tragedy to me
and maybe it is even correct.
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] mm/mremap: Fail map duplication attempts for private mappings
  2017-07-13 19:11   ` [PATCH] mm/mremap: Fail map duplication attempts for private mappings Vlastimil Babka
  2017-07-13 22:33     ` Mike Kravetz
@ 2017-07-19 16:39     ` Mike Kravetz
  2017-07-20  8:20       ` Michal Hocko
  1 sibling, 1 reply; 12+ messages in thread
From: Mike Kravetz @ 2017-07-19 16:39 UTC (permalink / raw)
  To: Vlastimil Babka, linux-mm, linux-kernel
  Cc: Andrew Morton, Andrea Arcangeli, Michal Hocko, Aaron Lu,
	Kirill A . Shutemov, Anshuman Khandual, Linux API

On 07/13/2017 12:11 PM, Vlastimil Babka wrote:
> [+CC linux-api]
> 
> On 07/13/2017 05:58 PM, Mike Kravetz wrote:
>> mremap will create a 'duplicate' mapping if old_size == 0 is
>> specified.  Such duplicate mappings make no sense for private
>> mappings.  If duplication is attempted for a private mapping,
>> mremap creates a separate private mapping unrelated to the
>> original mapping and makes no modifications to the original.
>> This is contrary to the purpose of mremap which should return
>> a mapping which is in some way related to the original.
>>
>> Therefore, return EINVAL in the case where if an attempt is
>> made to duplicate a private mapping.
>>
>> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
> 
> Acked-by: Vlastimil Babka <vbabka@suse.cz>

After considering Michal's concerns with follow on patch, it appears
this patch provides the most desired behavior.  Any other concerns
or issues with this patch?

If this moves forward, I will create man page updates to describe the
mremap(old_size == 0) behavior.

-- 
Mike Kravetz

> 
>> ---
>>  mm/mremap.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/mm/mremap.c b/mm/mremap.c
>> index cd8a1b1..076f506 100644
>> --- a/mm/mremap.c
>> +++ b/mm/mremap.c
>> @@ -383,6 +383,13 @@ static struct vm_area_struct *vma_to_resize(unsigned long addr,
>>  	if (!vma || vma->vm_start > addr)
>>  		return ERR_PTR(-EFAULT);
>>  
>> +	/*
>> +	 * !old_len  is a special case where a mapping is 'duplicated'.
>> +	 * Do not allow this for private mappings.
>> +	 */
>> +	if (!old_len && !(vma->vm_flags & (VM_SHARED | VM_MAYSHARE)))
>> +		return ERR_PTR(-EINVAL);
>> +
>>  	if (is_vm_hugetlb_page(vma))
>>  		return ERR_PTR(-EINVAL);
>>  
>>
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] mm/mremap: Fail map duplication attempts for private mappings
  2017-07-19 16:39     ` Mike Kravetz
@ 2017-07-20  8:20       ` Michal Hocko
  2017-07-20 20:37         ` [PATCH v2] " Mike Kravetz
  0 siblings, 1 reply; 12+ messages in thread
From: Michal Hocko @ 2017-07-20  8:20 UTC (permalink / raw)
  To: Mike Kravetz
  Cc: Vlastimil Babka, linux-mm, linux-kernel, Andrew Morton,
	Andrea Arcangeli, Aaron Lu, Kirill A . Shutemov,
	Anshuman Khandual, Linux API

On Wed 19-07-17 09:39:50, Mike Kravetz wrote:
> On 07/13/2017 12:11 PM, Vlastimil Babka wrote:
> > [+CC linux-api]
> > 
> > On 07/13/2017 05:58 PM, Mike Kravetz wrote:
> >> mremap will create a 'duplicate' mapping if old_size == 0 is
> >> specified.  Such duplicate mappings make no sense for private
> >> mappings.  If duplication is attempted for a private mapping,
> >> mremap creates a separate private mapping unrelated to the
> >> original mapping and makes no modifications to the original.
> >> This is contrary to the purpose of mremap which should return
> >> a mapping which is in some way related to the original.
> >>
> >> Therefore, return EINVAL in the case where if an attempt is
> >> made to duplicate a private mapping.
> >>
> >> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
> > 
> > Acked-by: Vlastimil Babka <vbabka@suse.cz>
> 
> After considering Michal's concerns with follow on patch, it appears
> this patch provides the most desired behavior.  Any other concerns
> or issues with this patch?

Maybe we should add a pr_warn_once to make users aware that this is no
longer supported.

> If this moves forward, I will create man page updates to describe the
> mremap(old_size == 0) behavior.
> 
> -- 
> Mike Kravetz
> 
> > 
> >> ---
> >>  mm/mremap.c | 7 +++++++
> >>  1 file changed, 7 insertions(+)
> >>
> >> diff --git a/mm/mremap.c b/mm/mremap.c
> >> index cd8a1b1..076f506 100644
> >> --- a/mm/mremap.c
> >> +++ b/mm/mremap.c
> >> @@ -383,6 +383,13 @@ static struct vm_area_struct *vma_to_resize(unsigned long addr,
> >>  	if (!vma || vma->vm_start > addr)
> >>  		return ERR_PTR(-EFAULT);
> >>  
> >> +	/*
> >> +	 * !old_len  is a special case where a mapping is 'duplicated'.
> >> +	 * Do not allow this for private mappings.

Do not allow this for private mappings because we have never really
duplicated the range for those so the new VMA is a fresh one unrelated
to the original one which breaks mremap semantic. While we can do that
there doesn't seem to be any existing usecase currently.

> >> +	 */
> >> +	if (!old_len && !(vma->vm_flags & (VM_SHARED | VM_MAYSHARE)))
> >> +		return ERR_PTR(-EINVAL);
> >> +
> >>  	if (is_vm_hugetlb_page(vma))
> >>  		return ERR_PTR(-EINVAL);
> >>  
> >>
> > 

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2] mm/mremap: Fail map duplication attempts for private mappings
  2017-07-20  8:20       ` Michal Hocko
@ 2017-07-20 20:37         ` Mike Kravetz
  2017-07-21 14:36           ` Michal Hocko
  0 siblings, 1 reply; 12+ messages in thread
From: Mike Kravetz @ 2017-07-20 20:37 UTC (permalink / raw)
  To: linux-mm, Linux API, linux-kernel
  Cc: Andrew Morton, Andrea Arcangeli, Michal Hocko, Aaron Lu,
	Kirill A . Shutemov, Vlastimil Babka, Anshuman Khandual,
	Mike Kravetz

mremap will create a 'duplicate' mapping if old_size == 0 is
specified.  Such duplicate mappings make no sense for private
mappings.  If duplication is attempted for a private mapping,
mremap creates a separate private mapping unrelated to the
original mapping and makes no modifications to the original.
This is contrary to the purpose of mremap which should return
a mapping which is in some way related to the original.

Therefore, return EINVAL in the case where if an attempt is
made to duplicate a private mapping.  Also, print a warning
message (once) if such an attempt is made.

Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
---
 mm/mremap.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/mm/mremap.c b/mm/mremap.c
index cd8a1b1..949f6a7 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -383,6 +383,15 @@ static struct vm_area_struct *vma_to_resize(unsigned long addr,
 	if (!vma || vma->vm_start > addr)
 		return ERR_PTR(-EFAULT);
 
+	/*
+	 * !old_len  is a special case where a mapping is 'duplicated'.
+	 * Do not allow this for private mappings.
+	 */
+	if (!old_len && !(vma->vm_flags & (VM_SHARED | VM_MAYSHARE))) {
+		pr_warn_once("%s (%d): attempted to duplicate a private mapping with mremap.  This is not supported.\n", current->comm, current->pid);
+		return ERR_PTR(-EINVAL);
+	}
+
 	if (is_vm_hugetlb_page(vma))
 		return ERR_PTR(-EINVAL);
 
-- 
2.7.5

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] mm/mremap: Fail map duplication attempts for private mappings
  2017-07-20 20:37         ` [PATCH v2] " Mike Kravetz
@ 2017-07-21 14:36           ` Michal Hocko
  2017-07-21 21:18             ` Mike Kravetz
  0 siblings, 1 reply; 12+ messages in thread
From: Michal Hocko @ 2017-07-21 14:36 UTC (permalink / raw)
  To: Mike Kravetz
  Cc: linux-mm, Linux API, linux-kernel, Andrew Morton,
	Andrea Arcangeli, Aaron Lu, Kirill A . Shutemov, Vlastimil Babka,
	Anshuman Khandual

On Thu 20-07-17 13:37:59, Mike Kravetz wrote:
> mremap will create a 'duplicate' mapping if old_size == 0 is
> specified.  Such duplicate mappings make no sense for private
> mappings.

sorry for the nit picking but this is not true strictly speaking.
It makes some sense, arguably (e.g. take an atomic snapshot of the
mapping). It doesn't make any sense with the _current_ implementation.

> If duplication is attempted for a private mapping,
> mremap creates a separate private mapping unrelated to the
> original mapping and makes no modifications to the original.
> This is contrary to the purpose of mremap which should return
> a mapping which is in some way related to the original.
> 
> Therefore, return EINVAL in the case where if an attempt is
> made to duplicate a private mapping.  Also, print a warning
> message (once) if such an attempt is made.
> 
> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>

I do not insist on the comment update suggested
http://lkml.kernel.org/r/20170720082058.GF9058@dhcp22.suse.cz
but I would appreciate it...

Other than that looks reasonably to me

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

> ---
>  mm/mremap.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/mm/mremap.c b/mm/mremap.c
> index cd8a1b1..949f6a7 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -383,6 +383,15 @@ static struct vm_area_struct *vma_to_resize(unsigned long addr,
>  	if (!vma || vma->vm_start > addr)
>  		return ERR_PTR(-EFAULT);
>  
> +	/*
> +	 * !old_len  is a special case where a mapping is 'duplicated'.
> +	 * Do not allow this for private mappings.
> +	 */
> +	if (!old_len && !(vma->vm_flags & (VM_SHARED | VM_MAYSHARE))) {
> +		pr_warn_once("%s (%d): attempted to duplicate a private mapping with mremap.  This is not supported.\n", current->comm, current->pid);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
>  	if (is_vm_hugetlb_page(vma))
>  		return ERR_PTR(-EINVAL);
>  
> -- 
> 2.7.5
> 

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] mm/mremap: Fail map duplication attempts for private mappings
  2017-07-21 14:36           ` Michal Hocko
@ 2017-07-21 21:18             ` Mike Kravetz
       [not found]               ` <cb9d9f6a-7095-582f-15a5-62643d65c736-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
  0 siblings, 1 reply; 12+ messages in thread
From: Mike Kravetz @ 2017-07-21 21:18 UTC (permalink / raw)
  To: Michal Hocko
  Cc: linux-mm, Linux API, linux-kernel, Andrew Morton,
	Andrea Arcangeli, Aaron Lu, Kirill A . Shutemov, Vlastimil Babka,
	Anshuman Khandual

On 07/21/2017 07:36 AM, Michal Hocko wrote:
> On Thu 20-07-17 13:37:59, Mike Kravetz wrote:
>> mremap will create a 'duplicate' mapping if old_size == 0 is
>> specified.  Such duplicate mappings make no sense for private
>> mappings.
> 
> sorry for the nit picking but this is not true strictly speaking.
> It makes some sense, arguably (e.g. take an atomic snapshot of the
> mapping). It doesn't make any sense with the _current_ implementation.
> 
>> If duplication is attempted for a private mapping,
>> mremap creates a separate private mapping unrelated to the
>> original mapping and makes no modifications to the original.
>> This is contrary to the purpose of mremap which should return
>> a mapping which is in some way related to the original.
>>
>> Therefore, return EINVAL in the case where if an attempt is
>> made to duplicate a private mapping.  Also, print a warning
>> message (once) if such an attempt is made.
>>
>> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
> 
> I do not insist on the comment update suggested
> http://lkml.kernel.org/r/20170720082058.GF9058@dhcp22.suse.cz
> but I would appreciate it...
> 
> Other than that looks reasonably to me
> 
> Acked-by: Michal Hocko <mhocko@suse.com>

My apologies.  I overlooked your comment about the comment when
creating the patch.  Below is the patch with commit message and
comment updated.

>From 5c4a1602bd6a942544ed011dc0a72fd258e874b2 Mon Sep 17 00:00:00 2001
From: Mike Kravetz <mike.kravetz@oracle.com>
Date: Wed, 12 Jul 2017 13:52:47 -0700
Subject: [PATCH] mm/mremap: Fail map duplication attempts for private mappings

mremap will attempt to create a 'duplicate' mapping if old_size
== 0 is specified.  In the case of private mappings, mremap
will actually create a fresh separate private mapping unrelated
to the original.  This does not fit with the design semantics of
mremap as the intention is to create a new mapping based on the
original.

Therefore, return EINVAL in the case where an attempt is made
to duplicate a private mapping.  Also, print a warning message
(once) if such an attempt is made.

Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
---
 mm/mremap.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/mm/mremap.c b/mm/mremap.c
index cd8a1b1..75b167d 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -383,6 +383,19 @@ static struct vm_area_struct *vma_to_resize(unsigned long addr,
 	if (!vma || vma->vm_start > addr)
 		return ERR_PTR(-EFAULT);
 
+	/*
+	 * !old_len is a special case where an attempt is made to 'duplicate'
+	 * a mapping.  This makes no sense for private mappings as it will
+	 * instead create a fresh/new mapping unrelated to the original.  This
+	 * is contrary to the basic idea of mremap which creates new mappings
+	 * based on the original.  There are no known use cases for this
+	 * behavior.  As a result, fail such attempts.
+	 */
+	if (!old_len && !(vma->vm_flags & (VM_SHARED | VM_MAYSHARE))) {
+		pr_warn_once("%s (%d): attempted to duplicate a private mapping with mremap.  This is not supported.\n", current->comm, current->pid);
+		return ERR_PTR(-EINVAL);
+	}
+
 	if (is_vm_hugetlb_page(vma))
 		return ERR_PTR(-EINVAL);
 
-- 
2.7.5

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] mm/mremap: Fail map duplication attempts for private mappings
       [not found]               ` <cb9d9f6a-7095-582f-15a5-62643d65c736-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
@ 2017-07-24  8:50                 ` Michal Hocko
  0 siblings, 0 replies; 12+ messages in thread
From: Michal Hocko @ 2017-07-24  8:50 UTC (permalink / raw)
  To: Mike Kravetz
  Cc: linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Linux API,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Andrew Morton,
	Andrea Arcangeli, Aaron Lu, Kirill A . Shutemov, Vlastimil Babka,
	Anshuman Khandual

On Fri 21-07-17 14:18:31, Mike Kravetz wrote:
[...]
> >From 5c4a1602bd6a942544ed011dc0a72fd258e874b2 Mon Sep 17 00:00:00 2001
> From: Mike Kravetz <mike.kravetz-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> Date: Wed, 12 Jul 2017 13:52:47 -0700
> Subject: [PATCH] mm/mremap: Fail map duplication attempts for private mappings
> 
> mremap will attempt to create a 'duplicate' mapping if old_size
> == 0 is specified.  In the case of private mappings, mremap
> will actually create a fresh separate private mapping unrelated
> to the original.  This does not fit with the design semantics of
> mremap as the intention is to create a new mapping based on the
> original.
> 
> Therefore, return EINVAL in the case where an attempt is made
> to duplicate a private mapping.  Also, print a warning message
> (once) if such an attempt is made.
> 
> Signed-off-by: Mike Kravetz <mike.kravetz-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>

Acked-by: Michal Hocko <mhocko-IBi9RG/b67k@public.gmane.org>

Thanks!

> ---
>  mm/mremap.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/mm/mremap.c b/mm/mremap.c
> index cd8a1b1..75b167d 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -383,6 +383,19 @@ static struct vm_area_struct *vma_to_resize(unsigned long addr,
>  	if (!vma || vma->vm_start > addr)
>  		return ERR_PTR(-EFAULT);
>  
> +	/*
> +	 * !old_len is a special case where an attempt is made to 'duplicate'
> +	 * a mapping.  This makes no sense for private mappings as it will
> +	 * instead create a fresh/new mapping unrelated to the original.  This
> +	 * is contrary to the basic idea of mremap which creates new mappings
> +	 * based on the original.  There are no known use cases for this
> +	 * behavior.  As a result, fail such attempts.
> +	 */
> +	if (!old_len && !(vma->vm_flags & (VM_SHARED | VM_MAYSHARE))) {
> +		pr_warn_once("%s (%d): attempted to duplicate a private mapping with mremap.  This is not supported.\n", current->comm, current->pid);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
>  	if (is_vm_hugetlb_page(vma))
>  		return ERR_PTR(-EINVAL);
>  
> -- 
> 2.7.5

-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2017-07-24  8:50 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <1499961495-8063-1-git-send-email-mike.kravetz@oracle.com>
     [not found] ` <1499961495-8063-1-git-send-email-mike.kravetz-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
2017-07-13 19:11   ` [PATCH] mm/mremap: Fail map duplication attempts for private mappings Vlastimil Babka
2017-07-13 22:33     ` Mike Kravetz
2017-07-14  4:51       ` Anshuman Khandual
     [not found]       ` <415625d2-1be9-71f0-ca11-a014cef98a3f-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
2017-07-14  8:26         ` Michal Hocko
2017-07-14 17:29           ` Mike Kravetz
2017-07-17  6:44             ` Michal Hocko
2017-07-19 16:39     ` Mike Kravetz
2017-07-20  8:20       ` Michal Hocko
2017-07-20 20:37         ` [PATCH v2] " Mike Kravetz
2017-07-21 14:36           ` Michal Hocko
2017-07-21 21:18             ` Mike Kravetz
     [not found]               ` <cb9d9f6a-7095-582f-15a5-62643d65c736-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
2017-07-24  8:50                 ` Michal Hocko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).