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