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

[Felix Kuehling]

On 2024-03-20 15:09, Joshi, Mukul wrote:
> [AMD Official Use Only - General]
>
>> -----Original Message-----
>> From: Kuehling, Felix <Felix.Kuehling at amd.com>
>> Sent: Monday, March 18, 2024 4:13 PM
>> To: Joshi, Mukul <Mukul.Joshi at amd.com>; amd-gfx at lists.freedesktop.org
>> Subject: Re: [PATCH] drm/amdkfd: Check cgroup when returning DMABuf info
>>
>>
>> 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. Maybe this would preserve the behaviour for that
>> case:
>>
> Can you please explain why this could be a issue on a single partition GPU?

What would xcp_id be on a single-partition GPU? If it's < 0, then your 
patch changes the behaviour. Instead or returning the GPU ID from the 
GPU where the memory was allocated, it returns some arbitrary GPU that 
the application has access to.

Regards,
   Felix


>
> Regards,
> Mukul
>
>>        ...
>> -     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 */