On 6/2/21 8:36 PM, Daniel Vetter wrote:
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.
Yes, but that's only on a local variable to get a write-protected prot.
vmwgfx does the same for its dirty-tracking. Here we're debating setting
VM_SHARED on a private mapping.
I'm honestly not sure whether userspace then even can notice this or
anything, so might be worth a quick testcase.
The net result is that in the very unlikely case the user requested a
private GPU mapping to write secret data into, That secret data is no
longer secret. And, for example in the case of AMD's SEV encryption that
data would have been encrypted in an anonymous page with COW mappings,
but not so if we add VM_SHARED, then it will be unencrypted in in GPU
pages. Then I think it's better to refuse COW mappings in mmap:
if (is_cow_mapping(vma->vm_flags))
return -EINVAL;
This will still allow private read-only mappings which is OK. And if
someone was actually relying on private COW'd GPU mappings, we'd only
break the code slightly more...
/Thomas
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.
Completely agree.
-Daniel
/Thomas
Christian.
/Thomas
