Re: [PATCH 1/3] mm/vma: fix incorrectly disallowed anonymous VMA merges

[Wei Yang <richard.weiyang@gmail.com>]

On Fri, Apr 04, 2025 at 02:04:10PM +0100, Lorenzo Stoakes wrote:
>On Fri, Apr 04, 2025 at 12:53:15PM +0000, Wei Yang wrote:
>> On Mon, Mar 17, 2025 at 09:15:03PM +0000, Lorenzo Stoakes wrote:
>> [...]
>> >However, we have a problem here - typically the vma passed here is the
>> >destination VMA.
>> >
>> >For instance in vma_merge_existing_range() we invoke:
>> >
>> >can_vma_merge_left()
>> >-> [ check that there is an immediately adjacent prior VMA ]
>> >-> can_vma_merge_after()
>> >  -> is_mergeable_vma() for general attribute check
>> >-> is_mergeable_anon_vma([ proposed anon_vma ], prev->anon_vma, prev)
>> >
>> >So if we were considering a target unfaulted 'prev':
>> >
>> >	  unfaulted    faulted
>> >	|-----------|-----------|
>> >	|    prev   |    vma    |
>> >	|-----------|-----------|
>> >
>> >This would call is_mergeable_anon_vma(NULL, vma->anon_vma, prev).
>> >
>> >The list_is_singular() check for vma->anon_vma_chain, an empty list on
>> >fault, would cause this merge to _fail_ even though all else indicates a
>> >merge.
>> >
>>
>> Great spot. It is hiding there for 15 years.
>
>Thanks!
>
>>
>> >Equally a simple merge into a next VMA would hit the same problem:
>> >
>> >	   faulted    unfaulted
>> >	|-----------|-----------|
>> >	|    vma    |    next   |
>> >	|-----------|-----------|
>> >
>> [...]
>> >---
>> > mm/vma.c                |  81 +++++++++++++++++++++++---------
>> > tools/testing/vma/vma.c | 100 +++++++++++++++++++++-------------------
>> > 2 files changed, 111 insertions(+), 70 deletions(-)
>> >
>> >diff --git a/mm/vma.c b/mm/vma.c
>> >index 5cdc5612bfc1..5418eef3a852 100644
>> >--- a/mm/vma.c
>> >+++ b/mm/vma.c
>> >@@ -57,6 +57,22 @@ struct mmap_state {
>> > 		.state = VMA_MERGE_START,				\
>> > 	}
>> >
>> >+/*
>> >+ * If, at any point, the VMA had unCoW'd mappings from parents, it will maintain
>> >+ * more than one anon_vma_chain connecting it to more than one anon_vma. A merge
>> >+ * would mean a wider range of folios sharing the root anon_vma lock, and thus
>> >+ * potential lock contention, we do not wish to encourage merging such that this
>> >+ * scales to a problem.
>> >+ */
>>
>> I don't follow here. Take a look into do_wp_page(), where CoW happens. But I
>> don't find where it will unlink parent anon_vma from vma->anon_vma_chain.
>
>Look at anon_vma_clone() in fork case. It's not the point of CoW that's the
>issue, it's propagation of AVC's upon fork.
>
>>
>> Per my understanding, the unlink behavior happens in unlink_anon_vma() which
>> unlink all anon_vma on vma->anon_vma_chain. And the normal caller of
>> unlink_anon_vma() is free_pgtables(). Other callers are on error path to
>> release prepared data. From this perspective, I don't see the chance to unlink
>> parent anon_vma from vma->anon_vma_chain either.
>>
>> But maybe I missed something. If it is not too bother, would you mind giving
>> me a hint?
>
>What you're saying is 'we never go back and fix this up once unCoW'd' which is
>true, but I don't want to write several page-length essays in comments, and this
>is a sensible way of looking at things for the purposes of this check.
>
>In future, we may want to actually do something like this, if it makes sense.
>

Ok, this is the future plan instead of current behavior.

My personal feeling is it would misleading to readers. I would think if all
pages mapping in VMA is Cow'd, the vma->anon_vma_chain becomes singular in
current kernel. 

A page-length comment is not we want, how about "maybe_root_anon_vma"? When
vma->anon_vma_chain is empty or singular, it means the (future) vma->anon_vma
is the root anon_vma.