Date:   Thu, 17 Aug 2023 09:42:31 -0400
From:   "Liam R. Howlett" <Liam.Howlett@...cle.com>
To:     Jann Horn <jannh@...gle.com>
Cc:     Linux-MM <linux-mm@...ck.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Shaohua Li <shaohua.li@...el.com>,
        kernel list <linux-kernel@...r.kernel.org>,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: [mm] VMA merging behavior wrt anon_vma has been slightly broken
 since Linux 3.0 (in a non-dangerous way)

* Jann Horn <jannh@...gle.com> [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

Powered by blists - more mailing lists