Sounds good. I will send a v2 with only ttm_bo_mmap_obj change. Thank you!
On 12/9/2021 10:27 AM, Christian König wrote:
Hi Rajneesh,
yes, separating this from the drm_gem_mmap_obj() change is certainly a
good idea.
The child cannot access the BOs mapped by the parent anyway with
access restrictions applied
exactly that is not correct. That behavior is actively used by some
userspace stacks as far as I know.
Regards,
Christian.
Am 09.12.21 um 16:23 schrieb Bhardwaj, Rajneesh:
Thanks Christian. Would it make it less intrusive if I just use the
flag for ttm bo mmap and remove the drm_gem_mmap_obj change from this
patch? For our use case, just the ttm_bo_mmap_obj change should
suffice and we don't want to put any more work arounds in the user
space (thunk, in our case).
The child cannot access the BOs mapped by the parent anyway with
access restrictions applied so I wonder why even inherit the vma?
On 12/9/2021 2:54 AM, Christian König wrote:
Am 08.12.21 um 21:53 schrieb Rajneesh Bhardwaj:
When an application having open file access to a node forks, its
shared
mappings also get reflected in the address space of child process even
though it cannot access them with the object permissions applied.
With the
existing permission checks on the gem objects, it might be
reasonable to
also create the VMAs with VM_DONTCOPY flag so a user space application
doesn't need to explicitly call the madvise(addr, len, MADV_DONTFORK)
system call to prevent the pages in the mapped range to appear in the
address space of the child process. It also prevents the memory leaks
due to additional reference counts on the mapped BOs in the child
process that prevented freeing the memory in the parent for which
we had
worked around earlier in the user space inside the thunk library.
Additionally, we faced this issue when using CRIU to checkpoint
restore
an application that had such inherited mappings in the child which
confuse CRIU when it mmaps on restore. Having this flag set for the
render node VMAs helps. VMAs mapped via KFD already take care of
this so
this is needed only for the render nodes.
Unfortunately that is most likely a NAK. We already tried something
similar.
While it is illegal by the OpenGL specification and doesn't work for
most userspace stacks, we do have some implementations which call
fork() with a GL context open and expect it to work.
Regards,
Christian.
Cc: Felix Kuehling <Felix.Kuehling@xxxxxxx>
Signed-off-by: David Yat Sin <david.yatsin@xxxxxxx>
Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@xxxxxxx>
---
drivers/gpu/drm/drm_gem.c | 3 ++-
drivers/gpu/drm/ttm/ttm_bo_vm.c | 2 +-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 09c820045859..d9c4149f36dd 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -1058,7 +1058,8 @@ int drm_gem_mmap_obj(struct drm_gem_object
*obj, unsigned long obj_size,
goto err_drm_gem_object_put;
}
- vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND |
VM_DONTDUMP;
+ vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND
+ | VM_DONTDUMP | VM_DONTCOPY;
vma->vm_page_prot =
pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
}
diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c
b/drivers/gpu/drm/ttm/ttm_bo_vm.c
index 33680c94127c..420a4898fdd2 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
@@ -566,7 +566,7 @@ int ttm_bo_mmap_obj(struct vm_area_struct *vma,
struct ttm_buffer_object *bo)
vma->vm_private_data = bo;
- vma->vm_flags |= VM_PFNMAP;
+ vma->vm_flags |= VM_PFNMAP | VM_DONTCOPY;
vma->vm_flags |= VM_IO | VM_DONTEXPAND | VM_DONTDUMP;
return 0;
}
