Re: [PATCH -v2] rmap: make anon_vma_prepare link in all the anon_vmasof a mergeable VMA
From: Rik van Riel
Date: Sat Apr 10 2010 - 16:25:43 EST
On 04/10/2010 04:05 PM, Linus Torvalds wrote:
And vma_adjust is the one place that does that anon_vma_merge(), which is
apart from the actual unmapping sequence the only other place that
actually free's anon_vmas. So there are reasons to be very suspicious of
that code.
It frees anon_vma_chain structures, but not actual anon_vmas.
Walking the anon_vma (from rmap) requires the anon_vma->lock,
which is taken in anon_vma_merge whenever a chain is unlinked.
And I think that code can actually lose an anon_vma chain. It's totally
screwing up the "import anonvma" case: when it does
if (anon_vma_clone(importer, vma)) {
return -ENOMEM;
}
importer->anon_vma = anon_vma;
we can actually have "importer == vma", but "anon_vma = next->anon_vma".
A few lines up from that code, we have:
if (vma->anon_vma && (insert || importer || start !=
vma->vm_start))
anon_vma = vma->anon_vma;
So anon_vma should always be vma->anon_vma.
If we have already imported an anon_vma, we will not
do so twice, because of the !importer->anon_vma check.
What am I overlooking?
In which case we actually end up with an _empty_ chain (because importer
didn't have a chain to begin with!) but "importer->anon_vma" points to an
anon_vma.
If we import a chain, from vma to importer, importer->anon_vma
will be equal to vma->anon_vma.
I do not see how 'importer' could get a state different from 'vma'.
Also, the conditional nesting makes no sense (the whole anon_vma_clone()
only makes sense if importer is set, and it is only ever set _inside_ the
earlier if-statement, so the whole code should be moved inside there), nor
does some of the comments.
No argument there, vma_adjust is very hard to read and it took
me a few days to convince myself that my changes kept things
equivalent to how they were before.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/