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

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

[Lorenzo Stoakes]

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);

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@gmail.com>
---
 mm/mprotect.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/mm/mprotect.c b/mm/mprotect.c
index 6f658d483704..9461c936082b 100644
--- a/mm/mprotect.c
+++ b/mm/mprotect.c
@@ -607,8 +607,11 @@ mprotect_fixup(struct vma_iterator *vmi, struct mmu_gather *tlb,
 	/*
 	 * If we make a private mapping writable we increase our commit;
 	 * but (without finer accounting) cannot reduce our commit if we
-	 * make it unwritable again. hugetlb mapping were accounted for
-	 * even if read-only so there is no need to account for them here
+	 * make it unwritable again except in the anonymous case where no
+	 * anon_vma has yet been assigned.
+	 *
+	 * hugetlb mapping were accounted for even if read-only so there is
+	 * no need to account for them here.
 	 */
 	if (newflags & VM_WRITE) {
 		/* Check space limits when area turns into data. */
@@ -622,6 +625,9 @@ mprotect_fixup(struct vma_iterator *vmi, struct mmu_gather *tlb,
 				return -ENOMEM;
 			newflags |= VM_ACCOUNT;
 		}
+	} else if ((oldflags & VM_ACCOUNT) && vma_is_anonymous(vma) &&
+		   !vma->anon_vma) {
+		newflags &= ~VM_ACCOUNT;
 	}
 
 	/*
@@ -652,6 +658,9 @@ mprotect_fixup(struct vma_iterator *vmi, struct mmu_gather *tlb,
 	}
 
 success:
+	if ((oldflags & VM_ACCOUNT) && !(newflags & VM_ACCOUNT))
+		vm_unacct_memory(nrpages);
+
 	/*
 	 * vm_flags and vm_page_prot are protected by the mmap_lock
 	 * held in write mode.
-- 
2.41.0

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

[Vlastimil Babka]

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.

> 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@gmail.com>

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:

Acked-by: Vlastimil Babka <vbabka@suse.cz>

> ---
>  mm/mprotect.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/mprotect.c b/mm/mprotect.c
> index 6f658d483704..9461c936082b 100644
> --- a/mm/mprotect.c
> +++ b/mm/mprotect.c
> @@ -607,8 +607,11 @@ mprotect_fixup(struct vma_iterator *vmi, struct mmu_gather *tlb,
>  	/*
>  	 * If we make a private mapping writable we increase our commit;
>  	 * but (without finer accounting) cannot reduce our commit if we
> -	 * make it unwritable again. hugetlb mapping were accounted for
> -	 * even if read-only so there is no need to account for them here
> +	 * make it unwritable again except in the anonymous case where no
> +	 * anon_vma has yet been assigned.
> +	 *
> +	 * hugetlb mapping were accounted for even if read-only so there is
> +	 * no need to account for them here.
>  	 */
>  	if (newflags & VM_WRITE) {
>  		/* Check space limits when area turns into data. */
> @@ -622,6 +625,9 @@ mprotect_fixup(struct vma_iterator *vmi, struct mmu_gather *tlb,
>  				return -ENOMEM;
>  			newflags |= VM_ACCOUNT;
>  		}
> +	} else if ((oldflags & VM_ACCOUNT) && vma_is_anonymous(vma) &&
> +		   !vma->anon_vma) {
> +		newflags &= ~VM_ACCOUNT;
>  	}
>  
>  	/*
> @@ -652,6 +658,9 @@ mprotect_fixup(struct vma_iterator *vmi, struct mmu_gather *tlb,
>  	}
>  
>  success:
> +	if ((oldflags & VM_ACCOUNT) && !(newflags & VM_ACCOUNT))
> +		vm_unacct_memory(nrpages);
> +
>  	/*
>  	 * vm_flags and vm_page_prot are protected by the mmap_lock
>  	 * held in write mode.

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

[David Hildenbrand]

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.

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? :)


>> 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@gmail.com>
> 
> 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.

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

-- 
Cheers,

David / dhildenb

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@gmail.com>
> >
> > 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
>

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

[David Hildenbrand]

On 27.06.23 10:49, Lorenzo Stoakes wrote:
> 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 :)
> 

Yeah, and that needs time and you have to motivate me :)

> 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.

And that's the scenario I think we care about in practice (actually 
accessing memory).

> 
> (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.

Using mprotect() when allocating/freeing memory in an allocator is 
already horribly harmful for performance (well, and the #VMAs), so I 
don't think that scenario is relevant in practice.

What some allocators (iirc even glibc) do is reserve a bigger area with 
PROT_NONE and grow the accessible part slowly on demand, discarding 
freed memory using MADV_DONTNEED. So you essentially end up with two 
VMAs -- one completely accessible, one completely inaccessible.

They don't use mprotect() because:
(a) It's bad for performance
(b) It might increase the #VMAs

There is efence, but I remember it simply does mmap()+munmap() and runs 
into VMA limits easily just by relying on a lot of mappings.


> 
> 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).

My answer would have been "memory gets commited the first time we allow 
write access, and that wasn't the case for all memory in that range".


Now, take your example above and touch the memory.


ptr = mmap(NULL, page_size * 3, PROT_READ, MAP_ANON | MAP_PRIVATE, -1, 0);
mprotect(ptr + page_size, page_size, PROT_READ | PROT_WRITE);
*(ptr + page_size) = 1;
mprotect(ptr + page_size, page_size, PROT_READ);


And we'll not merge the VMAs.

Which, at least to me, makes existing handling more consistent.

And users could rightfully wonder "why isn't it getting merged". And the 
answer would be the same: "memory gets commited the first time we allow 
write access, and that wasn't the case for all memory in that range".

> 
> 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.

Do we have to care about the VMA locks now that pagefaults can be served 
without the mmap_lock in write mode?

[...]

>>> 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?

It's a VMA merging issue that can be hit in practice, so I raised it.


No strong opinion from my side, just my 2 cents reading the patch 
description and wondering "why do we even invest time thinking about 
this case" -- and eventually make handling less consistent IMHO (see above).

-- 
Cheers,

David / dhildenb

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

[Lorenzo Stoakes]

On Tue, Jun 27, 2023 at 11:13:59AM +0200, David Hildenbrand wrote:
> On 27.06.23 10:49, Lorenzo Stoakes wrote:
> > 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 :)
> >
>
> Yeah, and that needs time and you have to motivate me :)
>

Beer? ;)

> > 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.
>
> And that's the scenario I think we care about in practice (actually
> accessing memory).

Yes indeed, I mean I am not denying this patch is edge case stuff, in reality
you'd allocate pages, and correctly that would be accountable, the unallocated
R/O bit not and it would remain accountable.

>
> >
> > (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.
>
> Using mprotect() when allocating/freeing memory in an allocator is already
> horribly harmful for performance (well, and the #VMAs), so I don't think
> that scenario is relevant in practice.

Chrome for instance maintains vast memory ranges as PROT_NONE. I've not dug
into what they're doing, but surely to make use of them they'd need to
mprotect() or mmap()/mremap() (which maybe is what the intent is)

But fair point. However I can't imagine m[re]map'ing like this would be
cheap either, as you're doing the same kind of expensive operations, so the
general _approach_ seems like it's used in some way in practice.

>
> What some allocators (iirc even glibc) do is reserve a bigger area with
> PROT_NONE and grow the accessible part slowly on demand, discarding freed
> memory using MADV_DONTNEED. So you essentially end up with two VMAs -- one
> completely accessible, one completely inaccessible.
>
> They don't use mprotect() because:
> (a) It's bad for performance
> (b) It might increase the #VMAs
>
> There is efence, but I remember it simply does mmap()+munmap() and runs into
> VMA limits easily just by relying on a lot of mappings.
>
>
> >
> > 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).
>
> My answer would have been "memory gets commited the first time we allow
> write access, and that wasn't the case for all memory in that range".
>
>
> Now, take your example above and touch the memory.
>
>
> ptr = mmap(NULL, page_size * 3, PROT_READ, MAP_ANON | MAP_PRIVATE, -1, 0);
> mprotect(ptr + page_size, page_size, PROT_READ | PROT_WRITE);
> *(ptr + page_size) = 1;
> mprotect(ptr + page_size, page_size, PROT_READ);
>
>
> And we'll not merge the VMAs.
>
> Which, at least to me, makes existing handling more consistent.

Indeed, but I don't think it's currently consistent at all.

The 'correct' solution would be to:-

1. account for the block when it becomes writable
2. unaccount for any pages not used when it becomes unwritable

However since we can't go from vma -> folios for anon pages without some
extreme effort this is not feasible.

Therefore the existing code hacks it and just keep things accountable.

The patch reduces the hacking so we get halfway to the correct approach.

So before: "if you ever make this read/write, we account it forever"
After: "if you ever make this read/write and USE IT, we account it forever"

To me it is more consistent. Of course this is subjective...

>
> And users could rightfully wonder "why isn't it getting merged". And the
> answer would be the same: "memory gets commited the first time we allow
> write access, and that wasn't the case for all memory in that range".
>

Yes indeed, a bigger answer is that we don't have fine-grained accounting
for pages for anon_vma.

> >
> > 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.
>
> Do we have to care about the VMA locks now that pagefaults can be served
> without the mmap_lock in write mode?

Any switch to VMA locking would need careful attention applied to mprotect
and require equally strong assurances given we are fiddling with entries in
the maple tree (and more broadly the mmap_lock implies something stronger).

>
> [...]
>
> > > > 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?
>
> It's a VMA merging issue that can be hit in practice, so I raised it.
>
>
> No strong opinion from my side, just my 2 cents reading the patch
> description and wondering "why do we even invest time thinking about this
> case" -- and eventually make handling less consistent IMHO (see above).

Hmm it seems ilke you have quite a strong opinion :P but this is why I cc-d
you, as you are a great scrutiniser.

Yeah, the time investment was just by accident, the patch was originally a
throwaway thing to prove the point :]

I very much appreciate your time though! And I owe you at least one beer now.

I would ask that while you might question the value, whether you think it
so harmful as not to go in, so Andrew can know whether this debate = don't
take?

An Ack-with-meh would be fine. But also if you want to nak, it's also
fine. I will buy you the beer either way ;)

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

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

[David Hildenbrand]

[...]

>>
>> Yeah, and that needs time and you have to motivate me :)
>>
> 
> Beer? ;)

Oh, that always works :)

> 
>>> 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.
>>
>> And that's the scenario I think we care about in practice (actually
>> accessing memory).

[...]

>>> 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.
>>
>> Using mprotect() when allocating/freeing memory in an allocator is already
>> horribly harmful for performance (well, and the #VMAs), so I don't think
>> that scenario is relevant in practice.
> 
> Chrome for instance maintains vast memory ranges as PROT_NONE. I've not dug
> into what they're doing, but surely to make use of them they'd need to
> mprotect() or mmap()/mremap() (which maybe is what the intent is)

I suspect they are doing something similar than glibc (and some other 
allocators like jemalloc IIRC), because they want to minimze the #VMAs.

> 
> But fair point. However I can't imagine m[re]map'ing like this would be
> cheap either, as you're doing the same kind of expensive operations, so the
> general _approach_ seems like it's used in some way in practice.

Usually people access memory and not play mprotect() games for fun :)

> 
>>
>> What some allocators (iirc even glibc) do is reserve a bigger area with
>> PROT_NONE and grow the accessible part slowly on demand, discarding freed
>> memory using MADV_DONTNEED. So you essentially end up with two VMAs -- one
>> completely accessible, one completely inaccessible.
>>
>> They don't use mprotect() because:
>> (a) It's bad for performance
>> (b) It might increase the #VMAs
>>
>> There is efence, but I remember it simply does mmap()+munmap() and runs into
>> VMA limits easily just by relying on a lot of mappings.
>>
>>
>>>
>>> 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).
>>
>> My answer would have been "memory gets commited the first time we allow
>> write access, and that wasn't the case for all memory in that range".
>>
>>
>> Now, take your example above and touch the memory.
>>
>>
>> ptr = mmap(NULL, page_size * 3, PROT_READ, MAP_ANON | MAP_PRIVATE, -1, 0);
>> mprotect(ptr + page_size, page_size, PROT_READ | PROT_WRITE);
>> *(ptr + page_size) = 1;
>> mprotect(ptr + page_size, page_size, PROT_READ);
>>
>>
>> And we'll not merge the VMAs.
>>
>> Which, at least to me, makes existing handling more consistent.
> 
> Indeed, but I don't think it's currently consistent at all.
> 
> The 'correct' solution would be to:-
> 
> 1. account for the block when it becomes writable
> 2. unaccount for any pages not used when it becomes unwritable
> 

I've been messing with something related (but slightly different) for a 
while now in my mind, and I'm not at the point where I can talk about my 
work/idea yet.

But because I've been messing with it, I can comment on some existing 
oddities. Just imagine:

* userfaultfd() can place anon pages even in PROT_NONE areas
* ptrace can place anon pages in PROT_READ areas
* "fun" like the forbidden shared zeropage on s390x in some VMAs can
   place anon pages into PROT_READ areas.

It's all far from "correct" when talking about memory accounting. But it 
seems to get the job done for the most case for now.

> However since we can't go from vma -> folios for anon pages without some
> extreme effort this is not feasible.
> 
> Therefore the existing code hacks it and just keep things accountable.
> 
> The patch reduces the hacking so we get halfway to the correct approach.
> 
> So before: "if you ever make this read/write, we account it forever"
> After: "if you ever make this read/write and USE IT, we account it forever"
> 

"USE" is probably the wrong word. Maybe "MODIFIED", but there are other 
cases (MADV_POPULATE_WRITE)

> To me it is more consistent. Of course this is subjective...
> 
You made the conditional more complicated to make it consistent, won't 
argue with that :)

>>
>> And users could rightfully wonder "why isn't it getting merged". And the
>> answer would be the same: "memory gets commited the first time we allow
>> write access, and that wasn't the case for all memory in that range".
>>
> 
> Yes indeed, a bigger answer is that we don't have fine-grained accounting
> for pages for anon_vma.

Yes, VM_ACCOUNT is all-or nothing, which makes a lot of sense in many 
cases (not in all, though).

[...]

>>
>>>>> 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?
>>
>> It's a VMA merging issue that can be hit in practice, so I raised it.
>>
>>
>> No strong opinion from my side, just my 2 cents reading the patch
>> description and wondering "why do we even invest time thinking about this
>> case" -- and eventually make handling less consistent IMHO (see above).
> 
> Hmm it seems ilke you have quite a strong opinion :P but this is why I cc-d
> you, as you are a great scrutiniser.

I might make it sound like a strong opinion (because I am challenging 
the motivation), but there is no nak :)

> 
> Yeah, the time investment was just by accident, the patch was originally a
> throwaway thing to prove the point :]
> 
> I very much appreciate your time though! And I owe you at least one beer now.
> 
> I would ask that while you might question the value, whether you think it
> so harmful as not to go in, so Andrew can know whether this debate = don't
> take?
> 
> An Ack-with-meh would be fine. But also if you want to nak, it's also
> fine. I will buy you the beer either way ;)

It's more a "no nak" --  I don't see the real benefit but I also don't 
see the harm (as long as VMA locking is not an issue). If others see the 
benefit, great, so I'll let these decide.

-- 
Cheers,

David / dhildenb