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
