Re: [PATCH] drm/ttm: Don't inherit GEM object VMAs in child process

["Bhardwaj, Rajneesh" <rajneesh.bhardwaj@amd.com>]

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@amd.com>
>>>>
>>>> Signed-off-by: David Yat Sin <david.yatsin@amd.com>
>>>> Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@amd.com>
>>>> ---
>>>>   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;
>>>>   }
>>>
>