mremap fix/cleanups
Subsystems (2)
cover mremap fix/cleanups
Quoted 3 lines · Oleg Nesterov
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
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.
--- mm/mremap.c | 15 +++++++-------- 1 files changed, 7 insertions(+), 8 deletions(-)
mm/mremap.c +7 -8
@@ -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 */
Signature · 5 lines
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
Quoted 6 lines · Oleg Nesterov
move_vma()can't just return iff_op->mremap()fails, we should unmap the new vma like we do ifmove_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 <[email protected]>
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
Oleg Nesterov
Acked-by David RientjesRe: [PATCH 1/4] mremap: don't leak new_vma if f_op->mremap() fails
Quoted 8 lines · David Rientjes
Quoted 5 lines · Oleg Nesterov
move_vma()can't just return iff_op->mremap()fails, we should unmap the new vma like we do ifmove_page_tables()fails. To avoid the code duplication this patch moves the "move entries back" under the new "if (err)" branch.
Quoted 4 lines
Quoted 1 line
Signed-off-by Oleg Nesterov <[email protected]>Looks good! I think it was sniped by commit
4abad2ca4a4d("mm: newarch_remap()hook")
No, it was b2edffdd912b "fix mremap() vs. ioctx_kill() race"
Quoted 1 line
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.
Quoted 2 lines
but after that feel free to add my
Thanks!
Oleg.
Re: [PATCH 1/4] mremap: don't leak new_vma if f_op->mremap() fails
Quoted 12 lines · Oleg Nesterov
Yes, thanks, I'll rebase...
But you know, I think
4abad2ca4a4d("mm: newarch_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=143526519407521I think
arch_remap()should be turned intospecial_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
In reply to Oleg Nesterovmove_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.
--- mm/mremap.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)
mm/mremap.c +3 -1
@@ -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 (¤t->mm->mmap_sem);if (locked && new_len > old_len)mm_populate (new_addr + old_len, new_len - old_len);
Signature · 5 lines
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
Quoted 18 lines · Oleg Nesterov
move_vma()sets *locked even ifmove_page_tables()or ->mremap() fails, changesys_mremap()to check "ret & ~PAGE_MASK".I think we should simply remove the
VM_LOCKEDcode inmove_vma(), that is why this patch doesn't changemove_vma(). But this needs more cleanups.Signed-off-by Oleg Nesterov <[email protected]>--- mm/mremap.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)mm/mremap.c +0
-0@@ -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);
mm/mremap.c SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len, · line 583
Quoted 10 lines
mm/mremap.c +3
-1} out: -if (ret & ~PAGE_MASK ) +if (ret & ~PAGE_MASK ) {vm_unacct_memory (charged); + locked =0 ; + }up_write (¤t->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
Quoted 12 lines · David Rientjes
Quoted 9 lines · Oleg Nesterov
mm/mremap.c +1
-1@@ -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);
Quoted 8 lines
mm/mremap.c SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len, · line 583
Quoted 5 lines
mm/mremap.c +2
-0+ locked =0 ; + }up_write (¤t->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
In reply to Oleg NesterovQuoted 18 lines · Oleg Nesterov
move_vma()sets *locked even ifmove_page_tables()or ->mremap() fails, changesys_mremap()to check "ret & ~PAGE_MASK".I think we should simply remove the
VM_LOCKEDcode inmove_vma(), that is why this patch doesn't changemove_vma(). But this needs more cleanups.Signed-off-by Oleg Nesterov <[email protected]>--- mm/mremap.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)mm/mremap.c +0
-0@@ -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);
mm/mremap.c SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len, · line 583
Quoted 10 lines
mm/mremap.c +3
-1} out: -if (ret & ~PAGE_MASK ) +if (ret & ~PAGE_MASK ) {vm_unacct_memory (charged); + locked =0 ; + }up_write (¤t->mm->mmap_sem);if (locked && new_len > old_len)mm_populate (new_addr + old_len, new_len - old_len);
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
Quoted 25 lines · David Rientjes
Quoted 22 lines · Oleg Nesterov
move_vma()sets *locked even ifmove_page_tables()or ->mremap() fails, changesys_mremap()to check "ret & ~PAGE_MASK".I think we should simply remove the
VM_LOCKEDcode inmove_vma(), that is why this patch doesn't changemove_vma(). But this needs more cleanups.Signed-off-by Oleg Nesterov <[email protected]>--- mm/mremap.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)mm/mremap.c +1
-1@@ -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 ) {
Quoted 8 lines
mm/mremap.c SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len, · line 583
Quoted 6 lines
mm/mremap.c +2
-0vm_unacct_memory (charged); + locked =0 ; + }up_write (¤t->mm->mmap_sem);if (locked && new_len > old_len)mm_populate (new_addr + old_len, new_len - old_len);Acked-by David Rientjes <[email protected]>
Thanks! I am resending this series with your acks applies.
Plus another patche which was already acked but ignored anyway ;)
Quoted 3 lines
Might be cleaner to rename this to "populate" in
sys_mremap()and set it to false in the previousold_len>=new_lenconditional and remove the check here fornew_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
In reply to Oleg NesterovThe "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.
--- mm/mremap.c | 21 +++++++++++---------- 1 files changed, 11 insertions(+), 10 deletions(-)
mm/mremap.c +11 -10
@@ -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;
Signature · 5 lines
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
Quoted 8 lines · Oleg Nesterov
The "
new_len>old_len" branch invma_to_resize()looks very confusing. It only covers the VM_DONTEXPAND/pgoff checks but everything below is equally unneeded ifnew_len==old_len.Change this code to return if "
new_len==old_len",new_len<old_lenis not possible, otherwise the code below is wrong anyway.Signed-off-by Oleg Nesterov <[email protected]>
PATCH 4/4 mremap: simplify the "overlap" check in mremap_to()
In reply to Oleg NesterovMinor, but this check is overcomplicated. Two half-intervals do NOT overlap if END1 <= START2 || END2 <= START1, mremap_to() just needs to negate this check.
--- mm/mremap.c | 5 +---- 1 files changed, 1 insertions(+), 4 deletions(-)
mm/mremap.c +1 -4
@@ -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);
Signature · 5 lines
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()
Quoted 5 lines · 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 <[email protected]>
If you rebase this patchset, it would be good to fixup the comment style while you're there.