linux-kernel - Re: [PATCH -v2] rmap: make anon_vma_prepare link in all the anon

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.LFD.2.00.1004091638290.3558@i5.linux-foundation.org>
Date:	Fri, 9 Apr 2010 16:56:13 -0700 (PDT)
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Johannes Weiner <hannes@...xchg.org>
cc:	Borislav Petkov <bp@...en8.de>,
	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>,
	Rik van Riel <riel@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Minchan Kim <minchan.kim@...il.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Lee Schermerhorn <Lee.Schermerhorn@...com>,
	Nick Piggin <npiggin@...e.de>,
	Andrea Arcangeli <aarcange@...hat.com>,
	Hugh Dickins <hugh.dickins@...cali.co.uk>,
	sgunderson@...foot.com
Subject: Re: [PATCH -v2] rmap: make anon_vma_prepare link in all the anon_vmas
 of a mergeable VMA



On Fri, 9 Apr 2010, Linus Torvalds wrote:
> 
> Can we have some better high-level documentation on what happens for all 
> the cases.
> 
>  - split (mprotect, or munmap in the middle):
> 
> 	anon_vma_clone: the two vma's will have the same anon_vma, and the 
> 	anon_vma chains will be equivalent. 
> 
>  - merge (mprotect that creates a mergeable state):
> 
> 	anon_vma_merge: we're supposed to have a anon_vma_chain that is 
> 	a superset of the two chains of the merged entries.
> 
>  - fork:
> 
> 	anon_vma_fork: each new vma will have a _new_ anon_vma as it's 
> 	primary one, and will link to the old primary trough the 
> 	anon_vma_chain. It's doing this with a anon_vma_clone() followed 
> 	by adding an entra entry to the new anon_vma, and setting 
> 	vma->anon_vma to the new one.
> 
>  - create/mmap:
> 
> 	anon_vma_prepare: find a mergeable anon_vma and use that as a 
> 	singleton, because the other entries on the anon_vma chain won't 
> 	matter, since they cannot be associated with any pages associated 
> 	with the newly created vma..
> 
> Correct?

Ok, so I don't know if the above is correct, but if it is, let's ignore 
the "merge" case as being complex, and look at the other cases.

With fork, the main anon_vma becomes different, so let's ignore that. That 
always means that the resulting list is not comparable or compatible, and 
we'll never mix them up.

If we make one very _simple_ rule for the create/mmap case, namely that we 
only re-use another _singleton_ anon_vma, then split and create case will 
look exactly the same. And in particular, we get a very simple and 
powerful rule: if the anon_vma matches, then the _list_ will also always 
match.

And that, in turn, would make 'merge' trivial too: you really can always 
drop the side that goes away. There's never any question about how to 
merge the lists, or which to pick, because every single operation that 
leaves the anon_vma the same will guarantee that the list will be 
identical too.

So now the simple rule is that if the anon_vma is the same, then the list 
of associated anon_vma's will always be the same - across all of merge, 
split and create.

Isn't that a _much_ simpler model to think about?

So _instead_ of all the patches that have floated about, I would suggest 
this simple change to "find_mergeable_anon_vma()" instead..

Oh, and maybe it's the meds talking again. I'm feeling better than 
yesterday, but am still a bit lightheaded. 

		Linus

---
 mm/mmap.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index 75557c6..462a8ca 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -850,7 +850,8 @@ struct anon_vma *find_mergeable_anon_vma(struct vm_area_struct *vma)
 	vm_flags = vma->vm_flags & ~(VM_READ|VM_WRITE|VM_EXEC);
 	vm_flags |= near->vm_flags & (VM_READ|VM_WRITE|VM_EXEC);
 
-	if (near->anon_vma && vma->vm_end == near->vm_start &&
+	if (near->anon_vma && list_is_singular(&near->anon_vma_chain) &&
+			vma->vm_end == near->vm_start &&
  			mpol_equal(vma_policy(vma), vma_policy(near)) &&
 			can_vma_merge_before(near, vm_flags,
 				NULL, vma->vm_file, vma->vm_pgoff +
@@ -871,7 +872,8 @@ try_prev:
 	vm_flags = vma->vm_flags & ~(VM_READ|VM_WRITE|VM_EXEC);
 	vm_flags |= near->vm_flags & (VM_READ|VM_WRITE|VM_EXEC);
 
-	if (near->anon_vma && near->vm_end == vma->vm_start &&
+	if (near->anon_vma && list_is_singular(&near->anon_vma_chain) &&
+			near->vm_end == vma->vm_start &&
   			mpol_equal(vma_policy(near), vma_policy(vma)) &&
 			can_vma_merge_after(near, vm_flags,
 				NULL, vma->vm_file, vma->vm_pgoff))
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/