[Patch v2] drm/amdgpu: Don't inherit GEM object VMAs in child process

[Christian König]

Am 11.12.21 um 01:55 schrieb Felix Kuehling:
> On 2021-12-10 4:48 p.m., Rajneesh Bhardwaj wrote:
>> 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.
>>
>> To limit the impact of the change to user space consumers such as OpenGL
>> etc, limit it to KFD BOs only.
>>
>> Cc: Felix Kuehling <Felix.Kuehling at amd.com>
>>
>> Signed-off-by: David Yat Sin <david.yatsin at amd.com>
>> Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj at amd.com>
>
> Acked-by: Felix Kuehling <Felix.Kuehling at amd.com>

At some point we probably want to extend that to all AMDGPU BOs, but for 
now the patch is Reviewed-by: Christian König <christian.koenig at amd.com>

Thanks,
Christian.

>
>
>> ---
>>
>> Changes in v2:
>>   * Addressed Christian's concerns for user space impact
>>   * Further reduced the scope to KFD BOs only
>>
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> index a224b5295edd..64a7931eda8c 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> @@ -263,6 +263,9 @@ static int amdgpu_gem_object_mmap(struct 
>> drm_gem_object *obj, struct vm_area_str
>>           !(vma->vm_flags & (VM_READ | VM_WRITE | VM_EXEC)))
>>           vma->vm_flags &= ~VM_MAYWRITE;
>>   +    if (bo->kfd_bo)
>> +        vma->vm_flags |= VM_DONTCOPY;
>> +
>>       return drm_gem_ttm_mmap(obj, vma);
>>   }