[PATCH 0/4] mremap fix/cleanups

[PATCH 0/4] mremap fix/cleanups

[Oleg Nesterov]

On 06/18, Oleg Nesterov wrote:
>
> I'll send the fixes, but also I'll try to cleanup this code. Not
> sure I will succeed ;)

Yes, I think this code needs more cleanups, but this is not simple ;)

Let me first send the changes which look "obviously correct" to me.
Perhaps I'll send more patches on top of this later.

Please review.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 1/4] mremap: don't leak new_vma if f_op->mremap() fails

[Oleg Nesterov]

move_vma() can't just return if f_op->mremap() fails, we should
unmap the new vma like we do if move_page_tables() fails. To avoid
the code duplication this patch moves the "move entries back" under
the new "if (err)" branch.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 mm/mremap.c |   15 +++++++--------
 1 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/mm/mremap.c b/mm/mremap.c
index 034e2d3..a6306bc 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -275,6 +275,12 @@ static unsigned long move_vma(struct vm_area_struct *vma,
 	moved_len = move_page_tables(vma, old_addr, new_vma, new_addr, old_len,
 				     need_rmap_locks);
 	if (moved_len < old_len) {
+		err = -ENOMEM;
+	} else if (vma->vm_file && vma->vm_file->f_op->mremap) {
+		err = vma->vm_file->f_op->mremap(vma->vm_file, new_vma);
+	}
+
+	if (unlikely(err)) {
 		/*
 		 * On error, move entries back from new area to old,
 		 * which will succeed since page tables still there,
@@ -285,14 +291,7 @@ static unsigned long move_vma(struct vm_area_struct *vma,
 		vma = new_vma;
 		old_len = new_len;
 		old_addr = new_addr;
-		new_addr = -ENOMEM;
-	} else if (vma->vm_file && vma->vm_file->f_op->mremap) {
-		err = vma->vm_file->f_op->mremap(vma->vm_file, new_vma);
-		if (err < 0) {
-			move_page_tables(new_vma, new_addr, vma, old_addr,
-					 moved_len, true);
-			return err;
-		}
+		new_addr = err;
 	}
 
 	/* Conceal VM_ACCOUNT so old reservation is not undone */
-- 
1.5.5.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] mremap: don't leak new_vma if f_op->mremap() fails

[David Rientjes]

On Sat, 20 Jun 2015, Oleg Nesterov wrote:

> move_vma() can't just return if f_op->mremap() fails, we should
> unmap the new vma like we do if move_page_tables() fails. To avoid
> the code duplication this patch moves the "move entries back" under
> the new "if (err)" branch.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Looks good!  I think it was sniped by commit 4abad2ca4a4d ("mm: new 
arch_remap() hook") so it needs to be rebased, but after that feel free to 
add my

	Acked-by: David Rientjes <rientjes@google.com>

Re: [PATCH 1/4] mremap: don't leak new_vma if f_op->mremap() fails

[Oleg Nesterov]

On 06/30, David Rientjes wrote:
>
> On Sat, 20 Jun 2015, Oleg Nesterov wrote:
>
> > move_vma() can't just return if f_op->mremap() fails, we should
> > unmap the new vma like we do if move_page_tables() fails. To avoid
> > the code duplication this patch moves the "move entries back" under
> > the new "if (err)" branch.
> >
> > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
>
> Looks good!  I think it was sniped by commit 4abad2ca4a4d ("mm: new
> arch_remap() hook")

No, it was b2edffdd912b "fix mremap() vs. ioctx_kill() race"

> so it needs to be rebased,

Yes, thanks, I'll rebase...

But you know, I think 4abad2ca4a4d ("mm: new arch_remap() hook") should
be reverted later. Please see

	[PATCH v2 1/1] mm: move ->mremap() from file_operations to vm_operations_struct
	http://marc.info/?l=linux-kernel&m=143526519407521

I think arch_remap() should be turned into special_mapping_vmops->nremap().
But this is off-topic right now.

> but after that feel free to add my
>
 	Acked-by: David Rientjes <rientjes@google.com>

Thanks!

Oleg.

Re: [PATCH 1/4] mremap: don't leak new_vma if f_op->mremap() fails

[David Rientjes]

On Wed, 1 Jul 2015, Oleg Nesterov wrote:

> Yes, thanks, I'll rebase...
> 
> But you know, I think 4abad2ca4a4d ("mm: new arch_remap() hook") should
> be reverted later. Please see
> 
> 	[PATCH v2 1/1] mm: move ->mremap() from file_operations to vm_operations_struct
> 	http://marc.info/?l=linux-kernel&m=143526519407521
> 
> I think arch_remap() should be turned into special_mapping_vmops->nremap().
> But this is off-topic right now.
> 
> 

I like that approach with an update to Documentation/filesystems/vfs.txt.

[PATCH 2/4] mremap: don't do mm_populate(new_addr) on failure

[Oleg Nesterov]

move_vma() sets *locked even if move_page_tables() or ->mremap()
fails, change sys_mremap() to check "ret & ~PAGE_MASK".

I think we should simply remove the VM_LOCKED code in move_vma(),
that is why this patch doesn't change move_vma(). But this needs
more cleanups.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 mm/mremap.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/mm/mremap.c b/mm/mremap.c
index a6306bc..492721c 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -574,8 +574,10 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len,
 		ret = move_vma(vma, addr, old_len, new_len, new_addr, &locked);
 	}
 out:
-	if (ret & ~PAGE_MASK)
+	if (ret & ~PAGE_MASK) {
 		vm_unacct_memory(charged);
+		locked = 0;
+	}
 	up_write(&current->mm->mmap_sem);
 	if (locked && new_len > old_len)
 		mm_populate(new_addr + old_len, new_len - old_len);
-- 
1.5.5.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/4] mremap: don't do mm_populate(new_addr) on failure

[David Rientjes]

On Sat, 20 Jun 2015, Oleg Nesterov wrote:

> move_vma() sets *locked even if move_page_tables() or ->mremap()
> fails, change sys_mremap() to check "ret & ~PAGE_MASK".
> 
> I think we should simply remove the VM_LOCKED code in move_vma(),
> that is why this patch doesn't change move_vma(). But this needs
> more cleanups.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  mm/mremap.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/mm/mremap.c b/mm/mremap.c
> index a6306bc..492721c 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -574,8 +574,10 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len,
>  		ret = move_vma(vma, addr, old_len, new_len, new_addr, &locked);
>  	}
>  out:
> -	if (ret & ~PAGE_MASK)
> +	if (ret & ~PAGE_MASK) {
>  		vm_unacct_memory(charged);
> +		locked = 0;
> +	}
>  	up_write(&current->mm->mmap_sem);
>  	if (locked && new_len > old_len)
>  		mm_populate(new_addr + old_len, new_len - old_len);

Perhaps I'm looking at the wrong tree (next-20150630), but why does 
setting locked to 0 here matter if it's unreferenced?

Re: [PATCH 2/4] mremap: don't do mm_populate(new_addr) on failure

[Oleg Nesterov]

On 06/30, David Rientjes wrote:
>
> On Sat, 20 Jun 2015, Oleg Nesterov wrote:
>
> > --- a/mm/mremap.c
> > +++ b/mm/mremap.c
> > @@ -574,8 +574,10 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len,
> >  		ret = move_vma(vma, addr, old_len, new_len, new_addr, &locked);
> >  	}
> >  out:
> > -	if (ret & ~PAGE_MASK)
> > +	if (ret & ~PAGE_MASK) {
> >  		vm_unacct_memory(charged);
> > +		locked = 0;
> > +	}
> >  	up_write(&current->mm->mmap_sem);
> >  	if (locked && new_len > old_len)
> >  		mm_populate(new_addr + old_len, new_len - old_len);
>
> Perhaps I'm looking at the wrong tree (next-20150630), but why does
> setting locked to 0 here matter if it's unreferenced?

See the "if (locked && ...)" check before mm_populate(). We should
not do this if move_vma() fails.

Oleg.

Re: [PATCH 2/4] mremap: don't do mm_populate(new_addr) on failure

[David Rientjes]

On Sat, 20 Jun 2015, Oleg Nesterov wrote:

> move_vma() sets *locked even if move_page_tables() or ->mremap()
> fails, change sys_mremap() to check "ret & ~PAGE_MASK".
> 
> I think we should simply remove the VM_LOCKED code in move_vma(),
> that is why this patch doesn't change move_vma(). But this needs
> more cleanups.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  mm/mremap.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/mm/mremap.c b/mm/mremap.c
> index a6306bc..492721c 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -574,8 +574,10 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len,
>  		ret = move_vma(vma, addr, old_len, new_len, new_addr, &locked);
>  	}
>  out:
> -	if (ret & ~PAGE_MASK)
> +	if (ret & ~PAGE_MASK) {
>  		vm_unacct_memory(charged);
> +		locked = 0;
> +	}
>  	up_write(&current->mm->mmap_sem);
>  	if (locked && new_len > old_len)
>  		mm_populate(new_addr + old_len, new_len - old_len);

Acked-by: David Rientjes <rientjes@google.com>

Might be cleaner to rename this to "populate" in sys_mremap() and set it 
to false in the previous old_len >= new_len conditional and remove the 
check here for new_len > old_len.

Re: [PATCH 2/4] mremap: don't do mm_populate(new_addr) on failure

[Oleg Nesterov]

On 07/01, David Rientjes wrote:
>
> On Sat, 20 Jun 2015, Oleg Nesterov wrote:
>
> > move_vma() sets *locked even if move_page_tables() or ->mremap()
> > fails, change sys_mremap() to check "ret & ~PAGE_MASK".
> >
> > I think we should simply remove the VM_LOCKED code in move_vma(),
> > that is why this patch doesn't change move_vma(). But this needs
> > more cleanups.
> >
> > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> > ---
> >  mm/mremap.c |    4 +++-
> >  1 files changed, 3 insertions(+), 1 deletions(-)
> >
> > diff --git a/mm/mremap.c b/mm/mremap.c
> > index a6306bc..492721c 100644
> > --- a/mm/mremap.c
> > +++ b/mm/mremap.c
> > @@ -574,8 +574,10 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len,
> >  		ret = move_vma(vma, addr, old_len, new_len, new_addr, &locked);
> >  	}
> >  out:
> > -	if (ret & ~PAGE_MASK)
> > +	if (ret & ~PAGE_MASK) {
> >  		vm_unacct_memory(charged);
> > +		locked = 0;
> > +	}
> >  	up_write(&current->mm->mmap_sem);
> >  	if (locked && new_len > old_len)
> >  		mm_populate(new_addr + old_len, new_len - old_len);
>
> Acked-by: David Rientjes <rientjes@google.com>

Thanks! I am resending this series with your acks applies.

Plus another patche which was already acked but ignored anyway ;)

> Might be cleaner to rename this to "populate" in sys_mremap() and set it
> to false in the previous old_len >= new_len conditional and remove the
> check here for new_len > old_len.

Oh, this needs much more cleanups. Imo the whole VM_LOCKED logic in these
paths is ugly and confusing. move_vma() shouldn't look at VM_LOCKED at all,
mremap_to() should not duplicate the vma_to_resize/move_vma code, etc.

Oleg.

[PATCH 3/4] mremap: don't do uneccesary checks if new_len == old_len

[Oleg Nesterov]

The "new_len > old_len" branch in vma_to_resize() looks very confusing.
It only covers the VM_DONTEXPAND/pgoff checks but everything below is
equally unneeded if new_len == old_len.

Change this code to return if "new_len == old_len", new_len < old_len
is not possible, otherwise the code below is wrong anyway.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 mm/mremap.c |   21 +++++++++++----------
 1 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/mm/mremap.c b/mm/mremap.c
index 492721c..2416721 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -342,6 +342,7 @@ static struct vm_area_struct *vma_to_resize(unsigned long addr,
 {
 	struct mm_struct *mm = current->mm;
 	struct vm_area_struct *vma = find_vma(mm, addr);
+	unsigned long pgoff;
 
 	if (!vma || vma->vm_start > addr)
 		return ERR_PTR(-EFAULT);
@@ -353,17 +354,17 @@ static struct vm_area_struct *vma_to_resize(unsigned long addr,
 	if (old_len > vma->vm_end - addr)
 		return ERR_PTR(-EFAULT);
 
+	if (new_len == old_len)
+		return vma;
+
 	/* Need to be careful about a growing mapping */
-	if (new_len > old_len) {
-		unsigned long pgoff;
-
-		if (vma->vm_flags & (VM_DONTEXPAND | VM_PFNMAP))
-			return ERR_PTR(-EFAULT);
-		pgoff = (addr - vma->vm_start) >> PAGE_SHIFT;
-		pgoff += vma->vm_pgoff;
-		if (pgoff + (new_len >> PAGE_SHIFT) < pgoff)
-			return ERR_PTR(-EINVAL);
-	}
+	pgoff = (addr - vma->vm_start) >> PAGE_SHIFT;
+	pgoff += vma->vm_pgoff;
+	if (pgoff + (new_len >> PAGE_SHIFT) < pgoff)
+		return ERR_PTR(-EINVAL);
+
+	if (vma->vm_flags & (VM_DONTEXPAND | VM_PFNMAP))
+		return ERR_PTR(-EFAULT);
 
 	if (vma->vm_flags & VM_LOCKED) {
 		unsigned long locked, lock_limit;
-- 
1.5.5.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/4] mremap: don't do uneccesary checks if new_len == old_len

[David Rientjes]

On Sat, 20 Jun 2015, Oleg Nesterov wrote:

> The "new_len > old_len" branch in vma_to_resize() looks very confusing.
> It only covers the VM_DONTEXPAND/pgoff checks but everything below is
> equally unneeded if new_len == old_len.
> 
> Change this code to return if "new_len == old_len", new_len < old_len
> is not possible, otherwise the code below is wrong anyway.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Acked-by: David Rientjes <rientjes@google.com>

[PATCH 4/4] mremap: simplify the "overlap" check in mremap_to()

[Oleg Nesterov]

Minor, but this check is overcomplicated. Two half-intervals do NOT
overlap if END1 <= START2 || END2 <= START1, mremap_to() just needs
to negate this check.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 mm/mremap.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/mm/mremap.c b/mm/mremap.c
index 2416721..ed1b13a 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -406,10 +406,7 @@ static unsigned long mremap_to(unsigned long addr, unsigned long old_len,
 	/* Check if the location we're moving into overlaps the
 	 * old location at all, and fail if it does.
 	 */
-	if ((new_addr <= addr) && (new_addr+new_len) > addr)
-		goto out;
-
-	if ((addr <= new_addr) && (addr+old_len) > new_addr)
+	if (addr + old_len > new_addr && new_addr + new_len > addr)
 		goto out;
 
 	ret = do_munmap(mm, new_addr, new_len);
-- 
1.5.5.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 4/4] mremap: simplify the "overlap" check in mremap_to()

[David Rientjes]

On Sat, 20 Jun 2015, Oleg Nesterov wrote:

> Minor, but this check is overcomplicated. Two half-intervals do NOT
> overlap if END1 <= START2 || END2 <= START1, mremap_to() just needs
> to negate this check.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Acked-by: David Rientjes <rientjes@google.com>

If you rebase this patchset, it would be good to fixup the comment style 
while you're there.