[PATCH v2 0/5] mremap fix/cleanups

[PATCH v2 0/5] mremap fix/cleanups

[Oleg Nesterov]

Actually this is resend, not v2. Added the acks I got from David
(thanks!).

2/5 (acked by Pavel and Kirill) was sent separately and ignored
too, I think it fits this series.

(and I think that the recently added arch_remap() hook is not
 the right thing, we can remove it with these changes).

Andrew, should I resend the

	[PATCH 0/3] special_mapping_fault() is broken
	http://marc.info/?l=linux-kernel&m=143492093907585

series too?

Oleg.

[PATCH v2 1/5] 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>
Acked-by: David Rientjes <rientjes@google.com>
---
 mm/mremap.c |   16 +++++++---------
 1 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/mm/mremap.c b/mm/mremap.c
index a7c93ec..f54a43f 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -276,6 +276,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,
@@ -286,16 +292,8 @@ static unsigned long move_vma(struct vm_area_struct *vma,
 		vma = new_vma;
 		old_len = new_len;
 		old_addr = new_addr;
-		new_addr = -ENOMEM;
+		new_addr = err;
 	} 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;
-			}
-		}
 		arch_remap(mm, old_addr, old_addr + old_len,
 			   new_addr, new_addr + new_len);
 	}
-- 
1.5.5.1

[PATCH v2 2/5] mm: move ->mremap() from file_operations to vm_operations_struct

[Oleg Nesterov]

vma->vm_ops->mremap() looks more natural and clean in move_vma(), and
this way ->mremap() can have more users. Say, vdso.

While at it, s/aio_ring_remap/aio_ring_mremap/.

Note: this is the minimal change before ->mremap() finds another user
in file_operations; this method should have more arguments, and it can
be used to kill arch_remap().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Pavel Emelyanov <xemul@parallels.com>
Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 fs/aio.c           |   25 ++++++++++++++++---------
 include/linux/fs.h |    1 -
 include/linux/mm.h |    1 +
 mm/mremap.c        |    4 ++--
 4 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/fs/aio.c b/fs/aio.c
index 480440f..c122624 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -308,15 +308,9 @@ static void aio_free_ring(struct kioctx *ctx)
 	}
 }
 
-static int aio_ring_mmap(struct file *file, struct vm_area_struct *vma)
-{
-	vma->vm_flags |= VM_DONTEXPAND;
-	vma->vm_ops = &generic_file_vm_ops;
-	return 0;
-}
-
-static int aio_ring_remap(struct file *file, struct vm_area_struct *vma)
+static int aio_ring_mremap(struct vm_area_struct *vma)
 {
+	struct file *file = vma->vm_file;
 	struct mm_struct *mm = vma->vm_mm;
 	struct kioctx_table *table;
 	int i, res = -EINVAL;
@@ -342,9 +336,22 @@ static int aio_ring_remap(struct file *file, struct vm_area_struct *vma)
 	return res;
 }
 
+static const struct vm_operations_struct aio_ring_vm_ops = {
+	.mremap		= aio_ring_mremap,
+	.fault		= filemap_fault,
+	.map_pages	= filemap_map_pages,
+	.page_mkwrite	= filemap_page_mkwrite,
+};
+
+static int aio_ring_mmap(struct file *file, struct vm_area_struct *vma)
+{
+	vma->vm_flags |= VM_DONTEXPAND;
+	vma->vm_ops = &aio_ring_vm_ops;
+	return 0;
+}
+
 static const struct file_operations aio_ring_fops = {
 	.mmap = aio_ring_mmap,
-	.mremap = aio_ring_remap,
 };
 
 #if IS_ENABLED(CONFIG_MIGRATION)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 35ec87e..42aac09 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1582,7 +1582,6 @@ struct file_operations {
 	long (*unlocked_ioctl) (struct file *, unsigned int, unsigned long);
 	long (*compat_ioctl) (struct file *, unsigned int, unsigned long);
 	int (*mmap) (struct file *, struct vm_area_struct *);
-	int (*mremap)(struct file *, struct vm_area_struct *);
 	int (*open) (struct inode *, struct file *);
 	int (*flush) (struct file *, fl_owner_t id);
 	int (*release) (struct inode *, struct file *);
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 0755b9f..bb51ced 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -244,6 +244,7 @@ struct vm_fault {
 struct vm_operations_struct {
 	void (*open)(struct vm_area_struct * area);
 	void (*close)(struct vm_area_struct * area);
+	int (*mremap)(struct vm_area_struct * area);
 	int (*fault)(struct vm_area_struct *vma, struct vm_fault *vmf);
 	void (*map_pages)(struct vm_area_struct *vma, struct vm_fault *vmf);
 
diff --git a/mm/mremap.c b/mm/mremap.c
index f54a43f..3310378 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -277,8 +277,8 @@ static unsigned long move_vma(struct vm_area_struct *vma,
 				     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);
+	} else if (vma->vm_ops && vma->vm_ops->mremap) {
+		err = vma->vm_ops->mremap(new_vma);
 	}
 
 	if (unlikely(err)) {
-- 
1.5.5.1

[PATCH v2 3/5] 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>
Acked-by: David Rientjes <rientjes@google.com>
---
 mm/mremap.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/mm/mremap.c b/mm/mremap.c
index 3310378..7dcf7b4 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -578,8 +578,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

[PATCH v2 4/5] 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>
Acked-by: David Rientjes <rientjes@google.com>
---
 mm/mremap.c |   21 +++++++++++----------
 1 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/mm/mremap.c b/mm/mremap.c
index 7dcf7b4..d3f42be 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -346,6 +346,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);
@@ -357,17 +358,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

[PATCH v2 5/5] 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>
Acked-by: David Rientjes <rientjes@google.com>
---
 mm/mremap.c |    9 ++-------
 1 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/mm/mremap.c b/mm/mremap.c
index d3f42be..5a71cce 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -407,13 +407,8 @@ static unsigned long mremap_to(unsigned long addr, unsigned long old_len,
 	if (new_len > TASK_SIZE || new_addr > TASK_SIZE - new_len)
 		goto out;
 
-	/* 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)
+	/* Ensure the old/new locations do not overlap */
+	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

Re: [PATCH v2 0/5] mremap fix/cleanups

[David Rientjes]

On Thu, 2 Jul 2015, Oleg Nesterov wrote:

> Actually this is resend, not v2. Added the acks I got from David
> (thanks!).
> 
> 2/5 (acked by Pavel and Kirill) was sent separately and ignored
> too, I think it fits this series.
> 
> (and I think that the recently added arch_remap() hook is not
>  the right thing, we can remove it with these changes).
> 

This series applies cleanly to next-20150715 and has acks, was it deferred 
for some reason?

Re: [PATCH v2 0/5] mremap fix/cleanups

[Andrew Morton]

On Wed, 15 Jul 2015 16:34:35 -0700 (PDT) David Rientjes <rientjes@google.com> wrote:

> On Thu, 2 Jul 2015, Oleg Nesterov wrote:
> 
> > Actually this is resend, not v2. Added the acks I got from David
> > (thanks!).
> > 
> > 2/5 (acked by Pavel and Kirill) was sent separately and ignored
> > too, I think it fits this series.
> > 
> > (and I think that the recently added arch_remap() hook is not
> >  the right thing, we can remove it with these changes).
> > 
> 
> This series applies cleanly to next-20150715 and has acks, was it deferred 
> for some reason?

I applied

mremap-dont-leak-new_vma-if-f_op-mremap-fails.patch
mm-move-mremap-from-file_operations-to-vm_operations_struct.patch
mremap-dont-do-mm_populatenew_addr-on-failure.patch
mremap-dont-do-uneccesary-checks-if-new_len-==-old_len.patch
mremap-simplify-the-overlap-check-in-mremap_to.patch

Check spam folder ;)