Re: folio_mmapped

[David Hildenbrand <david@redhat.com>]

On 19.03.24 10:47, Quentin Perret wrote:
> On Monday 04 Mar 2024 at 22:58:49 (+0100), David Hildenbrand wrote:
>> On 04.03.24 22:43, Elliot Berman wrote:
>>> On Mon, Mar 04, 2024 at 09:17:05PM +0100, David Hildenbrand wrote:
>>>> On 04.03.24 20:04, Sean Christopherson wrote:
>>>>> On Mon, Mar 04, 2024, Quentin Perret wrote:
>>>>>>> As discussed in the sub-thread, that might still be required.
>>>>>>>
>>>>>>> One could think about completely forbidding GUP on these mmap'ed
>>>>>>> guest-memfds. But likely, there might be use cases in the future where you
>>>>>>> want to use GUP on shared memory inside a guest_memfd.
>>>>>>>
>>>>>>> (the iouring example I gave might currently not work because
>>>>>>> FOLL_PIN|FOLL_LONGTERM|FOLL_WRITE only works on shmem+hugetlb, and
>>>>>>> guest_memfd will likely not be detected as shmem; 8ac268436e6d contains some
>>>>>>> details)
>>>>>>
>>>>>> Perhaps it would be wise to start with GUP being forbidden if the
>>>>>> current users do not need it (not sure if that is the case in Android,
>>>>>> I'll check) ? We can always relax this constraint later when/if the
>>>>>> use-cases arise, which is obviously much harder to do the other way
>>>>>> around.
>>>>>
>>>>> +1000.  At least on the KVM side, I would like to be as conservative as possible
>>>>> when it comes to letting anything other than the guest access guest_memfd.
>>>>
>>>> So we'll have to do it similar to any occurrences of "secretmem" in gup.c.
>>>> We'll have to see how to marry KVM guest_memfd with core-mm code similar to
>>>> e.g., folio_is_secretmem().
>>>>
>>>> IIRC, we might not be able to de-reference the actual mapping because it
>>>> could get free concurrently ...
>>>>
>>>> That will then prohibit any kind of GUP access to these pages, including
>>>> reading/writing for ptrace/debugging purposes, for core dumping purposes
>>>> etc. But at least, you know that nobody was able to optain page references
>>>> using GUP that might be used for reading/writing later.
>>>
>>> Do you have any concerns to add to enum mapping_flags, AS_NOGUP, and
>>> replacing folio_is_secretmem() with a test of this bit instead of
>>> comparing the a_ops? I think it scales better.
>>
>> The only concern I have are races, but let's look into the details:
>>
>> In GUP-fast, we can essentially race with unmap of folios, munmap() of VMAs
>> etc.
>>
>> We had a similar discussion recently about possible races. It's documented
>> in folio_fast_pin_allowed() regarding disabled IRQs and RCU grace periods.
>>
>> "inodes and thus their mappings are freed under RCU, which means the mapping
>> cannot be freed beneath us and thus we can safely dereference it."
>>
>> So if we follow the same rules as folio_fast_pin_allowed(), we can
>> de-reference folio->mapping, for example comparing mapping->a_ops.
>>
>> [folio_is_secretmem should better follow the same approach]
> 
> Resurecting this discussion, I had discussions internally and as it
> turns out Android makes extensive use of vhost/vsock when communicating
> with guest VMs, which requires GUP. So, my bad, not supporting GUP for
> the pKVM variant of guest_memfd is a bit of a non-starter, we'll need to
> support it from the start. But again this should be a matter of 'simply'
> having a dedicated KVM exit reason so hopefully it's not too bad.

Likely you should look into ways to teach these interfaces that require 
GUP to consume fd+offset instead.

Yes, there is no such thing as free lunch :P

-- 
Cheers,

David / dhildenb