On 2024-03-20 15:09, Joshi, Mukul wrote:
[AMD Official Use Only - General]
-----Original Message-----
From: Kuehling, Felix <Felix.Kuehling@xxxxxxx>
Sent: Monday, March 18, 2024 4:13 PM
To: Joshi, Mukul <Mukul.Joshi@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
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@xxxxxxx>
---
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 */
