Re: [PATCH] drm/ttm: nuke VM_MIXEDMAP on BO mappings

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

On 6/2/21 2:04 PM, Christian König wrote:
>
>
> Am 02.06.21 um 13:24 schrieb Thomas Hellström (Intel):
>> [SNIP]
>>>>> @@ -576,14 +565,10 @@ static void ttm_bo_mmap_vma_setup(struct 
>>>>> ttm_buffer_object *bo, struct vm_area_s
>>>>>         vma->vm_private_data = bo;
>>>>>   -    /*
>>>>> -     * We'd like to use VM_PFNMAP on shared mappings, where
>>>>> -     * (vma->vm_flags & VM_SHARED) != 0, for performance reasons,
>>>>> -     * but for some reason VM_PFNMAP + x86 PAT + write-combine is 
>>>>> very
>>>>> -     * bad for performance. Until that has been sorted out, use
>>>>> -     * VM_MIXEDMAP on all mappings. See freedesktop.org bug #75719
>>>>> +    /* Enforce VM_SHARED here since no driver backend actually 
>>>>> supports COW
>>>>> +     * on TTM buffer object mappings.
>>>>
>>>> I think by default all TTM drivers support COW mappings in the 
>>>> sense that written data never makes it to the bo but stays in 
>>>> anonymous pages, although I can't find a single usecase. So comment 
>>>> should be changed to state that they are useless for us and that we 
>>>> can't support COW mappings with VM_PFNMAP.
>>>
>>> Well the problem I see with that is that it only works as long as 
>>> the BO is in system memory. When it then suddenly migrates to VRAM 
>>> everybody sees the same content again and the COW pages are dropped. 
>>> That is really inconsistent and I can't see why we would want to do 
>>> that.
>> Hmm, yes, that's actually a bug in drm_vma_manager().
>
> Hui? How is that related to drm_vma_manager() ?
>
Last argument of "unmap_mapping_range()" is "even_cows".
>>>
>>> Additionally to that when you allow COW mappings you need to make 
>>> sure your COWed pages have the right caching attribute and that the 
>>> reference count is initialized and taken into account properly. Not 
>>> driver actually gets that right at the moment.
>>
>> I was under the impression that COW'ed pages were handled 
>> transparently by the vm, you'd always get cached properly refcounted 
>> COW'ed pages but anyway since we're going to ditch support for them, 
>> doesn't really matter.
>
> Yeah, but I would have expected that the new COWed page should have 
> the same caching attributes as the old one and that is not really the 
> case.
>
>>
>>>
>>>>
>>>>>        */
>>>>> -    vma->vm_flags |= VM_MIXEDMAP;
>>>>> +    vma->vm_flags |= VM_PFNMAP | VM_SHARED;
>>>>
>>>> Hmm, shouldn't we refuse COW mappings instead, like my old patch on 
>>>> this subject did? In theory someone could be setting up what she 
>>>> thinks is a private mapping to a shared buffer object, and write 
>>>> sensitive data to it, which will immediately leak. It's a simple 
>>>> check, could open-code if necessary.
>>>
>>> Yeah, though about that as well. Rejecting things would mean we 
>>> potentially break userspace which just happened to work by 
>>> coincident previously. Not totally evil, but not nice either.
>>>
>>> How about we do a WARN_ON_ONCE(!(vma->vm_flags & VM_SHARED)); instead?
>>
>> Umm, yes but that wouldn't notify the user, and would be triggerable 
>> from user-space. But you can also set up legal non-COW mappings 
>> without the VM_SHARED flag, IIRC, see is_cow_mapping(). I think when 
>> this was up for discussion last time we arrived in a 
>> vma_is_cow_mapping() utility...
>
> Well userspace could trigger that only once, so no spamming of the log 
> can be expected here. And extra warnings in the logs are usually 
> reported by people rather quickly.

OK, I'm mostly worried about adding a security flaw that we know about 
from the start.

/Thomas


>
> Christian.
>
>>
>> /Thomas
>>
>>
>