* Jann Horn <jannh@xxxxxxxxxx> [230815 15:44]:
> Hi!
>
> I think VMA merging was accidentally nerfed a bit by commit
> 965f55dea0e3 ("mmap: avoid merging cloned VMAs"), which landed in
> Linux 3.0 - essentially, that commit makes it impossible to merge a
> VMA with an anon_vma into an adjacent VMA that does not have an
> anon_vma. (But the other direction works.)
>
>
> is_mergeable_anon_vma() is defined as:
>
> ```
> static inline bool is_mergeable_anon_vma(struct anon_vma *anon_vma1,
> struct anon_vma *anon_vma2, struct vm_area_struct *vma)
> {
> /*
> * The list_is_singular() test is to avoid merging VMA cloned from
> * parents. This can improve scalability caused by anon_vma lock.
> */
> if ((!anon_vma1 || !anon_vma2) && (!vma ||
> list_is_singular(&vma->anon_vma_chain)))
> return true;
> return anon_vma1 == anon_vma2;
> }
> ```
>
> If this function is called with a non-NULL vma pointer (which is
> almost always the case, except when checking for whether it's possible
> to merge in both directions at the same time),
You are talking about case 1 & 6 here? To get here merge_prev and
merge_next must be set.. which means can_vma_merge_after() and
can_vma_merge_before() must succeed.. which means
is_mergeable_anon_vma() returned true with both prev and next being
passed through as "vma". So, I think, even that case suffers the same
issue?
That is, we won't have merge_prev == true if prev has an empty
anon_vma_chain. The same is true for merge_next.
>and one of the two
> anon_vmas is non-NULL, this returns
> list_is_singular(&vma->anon_vma_chain). I believe that
> list_is_singular() call is supposed to check whether the
> anon_vma_chain contains *more than one* element, but it actually also
> fails if the anon_vma_chain contains zero elements.
>
> This means that the dup_anon_vma() calls in vma_merge() are
> effectively all no-ops because they are never called with a target
> that does not have an anon_vma and a source that has an anon_vma.
>
> I think this is unintentional - though I guess this unintentional
> refusal to merge VMAs this way also lowers the complexity of what can
> happen in the VMA merging logic. So I think the right fix here is to
> make this kind of merging possible again by changing
> "list_is_singular(&vma->anon_vma_chain)" to
> "list_empty(&vma->anon_vma_chain) ||
> list_is_singular(&vma->anon_vma_chain)", but my security hat makes me
> say that I'd also be happy if the unintentional breakage stayed this
> way it is now.
The commit message for the offending change says
find_mergeable_anon_vma() already considers merging these, so it may not
be as nerfed as it looks?
>From what I understand the merging is an optimisation and, from the
commit message, the change was to increase scalability, so this shifts
to using more memory to gain scalability on the anon_vma lock. Making
this change will shift back to (maybe?) less memory for more pressure on
that lock then? I am hesitant to suggest un-nerfing it, but it
shouldn't be left as it is since the code is unclear on what is
happening.
Thanks,
Liam
