Messages in this thread | | | Date | Sat, 10 Apr 2010 16:24:01 -0400 | From | Rik van Riel <> | Subject | Re: [PATCH -v2] rmap: make anon_vma_prepare link in all the anon_vmas of a mergeable VMA |
| |
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.
| |