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

From: Mike Rapoport
Date: Mon Oct 09 2023 - 06:47:42 EST


On Sat, Oct 07, 2023 at 05:47:48PM +0100, 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);
>
> 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 are
> conservative and account all pages or none at all.
>
> Acked-by: Vlastimil Babka <vbabka@xxxxxxx>
> Signed-off-by: Lorenzo Stoakes <lstoakes@xxxxxxxxx>

Acked-by: Mike Rapoport (IBM) <rppt@xxxxxxxxxx>

> ---
>
> v2:
> - Minor spelling correction.
>
> v1:
> https://lore.kernel.org/all/20230626204612.106165-1-lstoakes@xxxxxxxxx/
>
>
> mm/mprotect.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/mm/mprotect.c b/mm/mprotect.c
> index b94fbb45d5c7..10685ec35c5e 100644
> --- a/mm/mprotect.c
> +++ b/mm/mprotect.c
> @@ -608,8 +608,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 to be 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. */
> @@ -623,6 +626,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;
> }
>
> /*
> @@ -653,6 +659,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.42.0
>

--
Sincerely yours,
Mike.