Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1686718 > unrolled thread

[PATCH] mm/mremap: Fail map duplication attempts for private mappings

Back to article view | Back to linux.kernel

Contents

  [PATCH] mm/mremap: Fail map duplication attempts for private mappings Mike Kravetz <mike.kravetz@oracle.com> - 2017-07-13 18:00 +0200
    Re: [PATCH] mm/mremap: Fail map duplication attempts for private  mappings Vlastimil Babka <vbabka@suse.cz> - 2017-07-13 21:20 +0200
      Re: [PATCH] mm/mremap: Fail map duplication attempts for private  mappings Mike Kravetz <mike.kravetz@oracle.com> - 2017-07-14 00:40 +0200
        Re: [PATCH] mm/mremap: Fail map duplication attempts for private  mappings Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-07-14 07:00 +0200
        Re: [PATCH] mm/mremap: Fail map duplication attempts for private  mappings Michal Hocko <mhocko@kernel.org> - 2017-07-14 10:30 +0200
          Re: [PATCH] mm/mremap: Fail map duplication attempts for private  mappings Mike Kravetz <mike.kravetz@oracle.com> - 2017-07-14 19:30 +0200
            Re: [PATCH] mm/mremap: Fail map duplication attempts for private  mappings Michal Hocko <mhocko@kernel.org> - 2017-07-17 08:50 +0200

#1686718 — [PATCH] mm/mremap: Fail map duplication attempts for private mappings

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>
---
 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);
 
-- 
2.7.5

[toc] | [next] | [standalone]

#1686852 — Re: [PATCH] mm/mremap: Fail map duplication attempts for private mappings

[+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>

> ---
>  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);
>  
> 

[toc] | [prev] | [next] | [standalone]

#1686969 — Re: [PATCH] mm/mremap: Fail map duplication attempts for private mappings

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

[toc] | [prev] | [next] | [standalone]

#1687063 — Re: [PATCH] mm/mremap: Fail map duplication attempts for private mappings

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.

[toc] | [prev] | [next] | [standalone]

#1687152 — Re: [PATCH] mm/mremap: Fail map duplication attempts for private mappings

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).

[...]
> +	/*
> +	 * !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

[toc] | [prev] | [next] | [standalone]

#1687579 — Re: [PATCH] mm/mremap: Fail map duplication attempts for private mappings

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

[toc] | [prev] | [next] | [standalone]

#1688699 — Re: [PATCH] mm/mremap: Fail map duplication attempts for private mappings

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web