[lkml]   [2010]   [Apr]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [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'.


 \ /
  Last update: 2010-04-10 22:41    [W:0.236 / U:0.076 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site