lkml.org 
[lkml]   [2023]   [Jun]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2023-06-27 12:24    [W:0.152 / U:0.216 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site