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
