Re: [PATCH] mm/mprotect: allow unfaulted VMAs to be unaccounted on mprotect()

[Lorenzo Stoakes]

On Tue, Jun 27, 2023 at 08:59:33AM +0200, David Hildenbrand wrote:
> Hi all,
>
> On 27.06.23 08:28, Vlastimil Babka wrote:
> > On 6/26/23 22:46, Lorenzo Stoakes wrote:
> > > When mprotect() is used to make unwritable VMAs writable, they have the
> > > VM_ACCOUNT flag applied and memory accounted accordingly.
> > >
> > > If the VMA has had no pages faulted in and is then made unwritable once
> > > again, it will remain accounted for, despite not being capable of extending
> > > memory usage.
> > >
> > > Consider:-
> > >
> > > ptr = mmap(NULL, page_size * 3, PROT_READ, MAP_ANON | MAP_PRIVATE, -1, 0);
> > > mprotect(ptr + page_size, page_size, PROT_READ | PROT_WRITE);
> > > mprotect(ptr + page_size, page_size, PROT_READ);
> >
> > In the original Mike's example there were actual pages populated, in that
> > case we still won't merge the vma's, right? Guess that can't be helped.
> >
>
> I am clearly missing the motivation for this patch: above is a artificial
> reproducer that I cannot really imagine being relevant in practice.

I cc'd you on this patch exactly because I knew you'd scrutinise it
greatly :)

Well the motivator for the initial investigation was rppt playing with
R[WO]X (this came from an #mm irc conversation), however in his case he
will be mapping pages between the two.

(apologies to rppt, I forgot to add the Reported-By...)

>
> So is there any sane workload that does random mprotect() without even
> touching memory once? Sure, fuzzing, ... artificial reproducers ... but is
> there any real-life problem we're solving here?
>
> IOW, why did you (Lorenzo) invest time optimizing for this andcrafting this
> patch and why should reviewer invest time to understand if it's correct? :)
>

So why I (that Stoakes guy) invested time here was, well I had chased down
the issue for rppt out of curiosity, and 'proved' the point by making
essentially this patch.

I dug into it further and (as the patch message aludes to) have convinced
myself that this is safe, so essentially why NOT submit it :)

In real-use scenarios, yes fuzzers are a thing, but what comes to mind more
immediately is a process that maps a big chunk of virtual memory PROT_NONE
and uses that as part of an internal allocator.

If the process then allocates memory from this chunk (mprotect() ->
PROT_READ | PROT_WRITE), which then gets freed without being used
(mprotect() -> PROT_NONE) we hit the issue. For OVERCOMMIT_NEVER this could
become quite an issue more so than the VMA fragmentation.

In addition, I think a user simply doing the artificial test above would
find the split remaining quite confusing, and somebody debugging some code
like this would equally wonder why it happened, so there is benefit in
clarity too (they of course observing the VMA fragmentation from the
perspective of /proc/$pid/[s]maps).

I believe given we hold a very strong lock (write on mm->mmap_lock) and
that vma->anon_vma being NULL really does seem to imply no pages have been
allocated that this is therefore a safe thing to do and worthwhile.

>
> > > The first mprotect() splits the range into 3 VMAs and the second fails to
> > > merge the three as the middle VMA has VM_ACCOUNT set and the others do not,
> > > rendering them unmergeable.
> > >
> > > This is unnecessary, since no pages have actually been allocated and the
> > > middle VMA is not capable of utilising more memory, thereby introducing
> > > unnecessary VMA fragmentation (and accounting for more memory than is
> > > necessary).
> > >
> > > Since we cannot efficiently determine which pages map to an anonymous VMA,
> > > we have to be very conservative - determining whether any pages at all have
> > > been faulted in, by checking whether vma->anon_vma is NULL.
> > >
> > > We can see that the lack of anon_vma implies that no anonymous pages are
> > > present as evidenced by vma_needs_copy() utilising this on fork to
> > > determine whether page tables need to be copied.
> > >
> > > The only place where anon_vma is set NULL explicitly is on fork with
> > > VM_WIPEONFORK set, however since this flag is intended to cause the child
> > > process to not CoW on a given memory range, it is right to interpret this
> > > as indicating the VMA has no faulted-in anonymous memory mapped.
> > >
> > > If the VMA was forked without VM_WIPEONFORK set, then anon_vma_fork() will
> > > have ensured that a new anon_vma is assigned (and correctly related to its
> > > parent anon_vma) should any pages be CoW-mapped.
> > >
> > > The overall operation is safe against races as we hold a write lock against
> > > mm->mmap_lock.
> > >
> > > If we could efficiently look up the VMA's faulted-in pages then we would
> > > unaccount all those pages not yet faulted in. However as the original
> > > comment alludes this simply isn't currently possible, so we remain
> > > conservative and account all pages or none at all.
> > >
> > > Signed-off-by: Lorenzo Stoakes <lstoakes@xxxxxxxxx>
> >
> > So in practice programs will likely do the PROT_WRITE in order to actually
> > populate the area, so this won't trigger as I commented above. But it can
> > still help in some cases and is cheap to do, so:
>
> IMHO we should much rather look into getting hugetlb ranges merged. Mt
> recollection is that we'll never end up merging hugetlb VMAs once split.

I'm not sure how that's relevant to fragmented non-hugetlb VMAs though?

>
> This patch adds code without a clear motivation. Maybe there is a good
> motivation?

See above for motivational thoughts :)

>
> --
> Cheers,
>
> David / dhildenb
>