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

Groups > linux.kernel > #1692023 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] mm/mremap: Fail map duplication attempts for private  mappings Mike Kravetz <mike.kravetz@oracle.com> - 2017-07-19 18:50 +0200
    Re: [PATCH] mm/mremap: Fail map duplication attempts for private  mappings Michal Hocko <mhocko@kernel.org> - 2017-07-20 10:30 +0200
      [PATCH v2] mm/mremap: Fail map duplication attempts for private mappings Mike Kravetz <mike.kravetz@oracle.com> - 2017-07-20 22:40 +0200
        Re: [PATCH v2] mm/mremap: Fail map duplication attempts for private  mappings Michal Hocko <mhocko@kernel.org> - 2017-07-21 16:40 +0200
          Re: [PATCH v2] mm/mremap: Fail map duplication attempts for private  mappings Mike Kravetz <mike.kravetz@oracle.com> - 2017-07-21 23:20 +0200
            Re: [PATCH v2] mm/mremap: Fail map duplication attempts for private  mappings Michal Hocko <mhocko@kernel.org> - 2017-07-24 11:00 +0200

#1692023 — 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>

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

[toc] | [next] | [standalone]

#1692553

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

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

#1693221 — [PATCH v2] 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.  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

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

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

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

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

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

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

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

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

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@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>

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web