Messages in this thread | | | Date | Sat, 10 Apr 2010 13:34:55 -0700 (PDT) | From | Linus Torvalds <> | Subject | Re: [PATCH -v2] rmap: make anon_vma_prepare link in all the anon_vmas of a mergeable VMA |
| |
On Sat, 10 Apr 2010, Rik van Riel wrote:
> 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.
Rik, I think you're ignoring the fact that the anon_vma_chain is also the implicit refcount.
So when you don't create the chains, you implicitly end up freeing the anon_vma too early. In fact, it might well happen at that 'anon_vma_merge()': when it does the unlink_anon_vmas(), it may be unlinking the last remaining anon_vma ref, and then anon_vma_unlink _will_ in fact free the anon_vma.
Even though we have a 'vma->anon_vma' pointer that points to it - because the chains weren't set up correctly.
> Walking the anon_vma (from rmap) requires the anon_vma->lock, > which is taken in anon_vma_merge whenever a chain is unlinked.
None of that matters. If the dang thing got free'd, the lock isn't reliable any more.
> 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.
No. vma->anon_vma is NULL, so the above lines are total no-ops. We're trying to _fill_ it. But we're doing it wrong.
So we end up with:
anon_vma = next->anon-vma importer = vma
and we do:
if (anon_vma_clone(importer, vma)) { return -ENOMEM; } importer->anon_vma = anon_vma;
do you see?
The "anon_vma_clone(importer, vma)" does NOTHING, because it is cloning from the wrong source (from 'vma', rather than from 'next', so it leaves the vma chains empty.
And then, despite having empty chains, we do that
importer->anon_vma = anon_vma;
which sets the anon_vma to the (non-NULL) next->anon_vma.
And then, a bit later, we'll do
anon_vma_merge(vma, next);
which will happily notice that the anon_vma's of both vma and next match (because we just _set_ them to match), and then frees the ONLY REMAINING CHAIN - the one in next. The one we DID NOT CORRECTLY COPY, because we got our sources completely screwed up. > What am I overlooking?
Can you see it now?
> If we import a chain, from vma to importer, importer->anon_vma > will be equal to vma->anon_vma.
The thing you seem to miss is that we aren't supposed to import the chain from 'vma' AT ALL. The anon_vma came from _next_, not from 'vma'!
> I do not see how 'importer' could get a state different from 'vma'.
Stop worrying about 'vma'. Start worrying about 'next'.
Linus
| |