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

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Jun 02, 2021 at 02:21:17PM +0200, Thomas Hellström wrote:
> 
> 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.

VM_SHARED is already cleared in vma_set_page_prot() due to the VM_PFNMAP
check in vma_wants_writenotify.

I'm honestly not sure whether userspace then even can notice this or
anything, so might be worth a quick testcase.

Even if I'm wrong here we shouldn't allow cow mappings of gem_bo, that
just seems too nasty with all the side-effects.
-Daniel

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch