Re: [PATCH -v2] rmap: make anon_vma_prepare link in all the anon_vmas of a mergeable VMA

[Linus Torvalds <torvalds@linux-foundation.org>]

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