[PATCH] drm/amdkfd: Check cgroup when returning DMABuf info

[Wu, David]

On 3/20/2024 4:21 PM, Felix Kuehling wrote:
> On 2024-03-18 16:12, Felix Kuehling wrote:
>>
>> On 2024-03-15 14:17, Mukul Joshi wrote:
>>> Check cgroup permissions when returning DMA-buf info and
>>> based on cgroup check return the id of the GPU that has
>>> access to the BO.
>>>
>>> Signed-off-by: Mukul Joshi <mukul.joshi at amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c 
>>> b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>>> index dfa8c69532d4..f9631f4b1a02 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>>> @@ -1523,7 +1523,7 @@ static int kfd_ioctl_get_dmabuf_info(struct 
>>> file *filep,
>>>         /* Find a KFD GPU device that supports the get_dmabuf_info 
>>> query */
>>>       for (i = 0; kfd_topology_enum_kfd_devices(i, &dev) == 0; i++)
>>> -        if (dev)
>>> +        if (dev && !kfd_devcgroup_check_permission(dev))
>>>               break;
>>>       if (!dev)
>>>           return -EINVAL;
>>> @@ -1545,7 +1545,7 @@ static int kfd_ioctl_get_dmabuf_info(struct 
>>> file *filep,
>>>       if (xcp_id >= 0)
>>>           args->gpu_id = dmabuf_adev->kfd.dev->nodes[xcp_id]->id;
>>>       else
>>> -        args->gpu_id = dmabuf_adev->kfd.dev->nodes[0]->id;
>>> +        args->gpu_id = dev->id;
>>
>> If I remember correctly, this was meant as a fallback in case for GTT 
>> BOs where the exporting partition wasn't known and the application 
>> didn't have access to the first partition. I think the way you wrote 
>> this, it could also change the behaviour (report the wrong GPU ID) on 
>> single-partition GPUs, which is probably not intended.
>
> Never mind. I double checked: On single-partition GPUs, bo->xcp_id 
> always seems to be 0. So your code won't change the behaviour here. 
> The patch is
should bo->xcp_id be >= 0 for all cases? if yes then I think all tests 
can be moved. (like below)
args->gpu_id = dmabuf_adev->kfd.dev->nodes[xcp_id]->id;

David
>
> Reviewed-by: Felix Kuehling <felix.kuehling at amd.com>
>
>
>
>> Maybe this would preserve the behaviour for that case:
>>
>>     ...
>> -    else
>> +    else if 
>> (!kfd_devcgroup_check_permission(dmabuf_adev->kfd.dev->nodes[0]))
>>          args->gpu_id = dmabuf_adev->kfd.dev->nodes[0]->id;
>> +    else
>> +        args->gpu_id = dev->id;
>>
>> Or maybe a more general solution would make DMABuf import work when 
>> the exporter is really unknown or not even a GPU. This came up not so 
>> long ago in the context of interop with 3rd-party devices. This may 
>> require user mode changes as well.
>>
>> Regards,
>>   Felix
>>
>>
>>>       args->flags = flags;
>>>         /* Copy metadata buffer to user mode */