Messages in this thread | | | Date | Tue, 27 Jun 2023 12:18:58 +0200 | From | David Hildenbrand <> | Subject | Re: [PATCH] mm/mprotect: allow unfaulted VMAs to be unaccounted on mprotect() |
| |
[...]
>> >> 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
| |