Re: [RFC PATCH net-next 06/13] drm/amdkfd: prepare kfd core for the knod provider

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] kfd_wait_on_events_kernel mixes user and kernel memory semantics, causing persistent failures and security risks.
- [High] kfd_ioctl_alloc_memory_of_gpu crashes if called from a kernel thread due to current->mm dereference.
- [High] kfd_ioctl_map_memory_to_gpu fails unconditionally for kernel callers due to memdup_array_user.
- [Medium] The return value of knod_init() is silently ignored during probe.
- [Medium] kfd_ioctl_export_dmabuf leaks a file descriptor when called from a kernel thread.
--

--- Patch [6]: [RFC PATCH net-next 06/13] drm/amdkfd: prepare kfd core for the knod provider ---
commit c9a02222fa92c5ac2eb02dcfd71c15c6d2016bd7
Author: Taehee Yoo <ap420073@gmail.com>
Subject: drm/amdkfd: prepare kfd core for the knod provider

Expose the kfd internals the knod provider needs to build and run GPU
queues on behalf of the kernel: kernel-side event waiting, GPUVM
allocation for VRAM/GTT, doorbell access, and the HSA queue/packet
layout (kfd_hsa.h). No functional change for existing user-mode
queue users; the provider itself is added in a following patch.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 4c0c77eafbd1c..aa974929d22a1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -2480,6 +2480,8 @@ static int amdgpu_pci_probe(struct pci_dev *pdev,
>  		drm_client_setup(adev_to_drm(adev), format);
>  	}
>  
> +	knod_init(adev);
> +

[Severity: Medium]
Is it intentional to discard the return value of knod_init()?

If this initialization fails or cannot allocate its resources, the error
is not propagated. This might leave the system in an inconsistent state and
lead to further issues when knod_fini() operates on uninitialized structures.

>  	ret = amdgpu_debugfs_init(adev);
>  	if (ret)
>  		DRM_ERROR("Creating debugfs files failed (%d).\n", ret);

[ ... ]

> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index c7edebd2fd8a4..7879a6ffaa762 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> @@ -1078,8 +1081,8 @@ static int kfd_ioctl_get_available_memory(struct file *filep,
>  	return 0;
>  }
>  
> -static int kfd_ioctl_alloc_memory_of_gpu(struct file *filep,
> -					struct kfd_process *p, void *data)
> +int kfd_ioctl_alloc_memory_of_gpu(struct file *filep,
> +				  struct kfd_process *p, void *data)
>  {
>  	struct kfd_ioctl_alloc_memory_of_gpu_args *args = data;
>  	struct kfd_process_device *pdd;

[Severity: High]
Could this cause a NULL pointer dereference when called from a kernel thread?

This function is now exported for kernel callers, but internally it has blocks
that dereference current->mm:

svm_range_list_lock_and_flush_work(&p->svms, current->mm);
mutex_lock(&p->svms.lock);
mmap_write_unlock(current->mm);

Since kernel threads execute without a user address space, current->mm is
NULL, which would cause a panic when accessed.

[ ... ]

> @@ -1279,8 +1282,8 @@ static int kfd_ioctl_free_memory_of_gpu(struct file *filep,
>  	return ret;
>  }
>  
> -static int kfd_ioctl_map_memory_to_gpu(struct file *filep,
> -					struct kfd_process *p, void *data)
> +int kfd_ioctl_map_memory_to_gpu(struct file *filep,
> +				struct kfd_process *p, void *data)
>  {
>  	struct kfd_ioctl_map_memory_to_gpu_args *args = data;
>  	struct kfd_process_device *pdd, *peer_pdd;

[Severity: High]
Will memdup_array_user() fail unconditionally when this API is invoked by a
kernel caller?

kfd_ioctl_map_memory_to_gpu() copies the device_ids_array_ptr parameter using
memdup_array_user(). Because memdup_array_user() internally relies on
copy_from_user(), it will fail the access_ok() check and return -EFAULT if
provided a kernel-allocated array of device IDs.

[ ... ]

> @@ -1624,8 +1627,8 @@ static int kfd_ioctl_import_dmabuf(struct file *filep,
>  	return r;
>  }
>  
> -static int kfd_ioctl_export_dmabuf(struct file *filep,
> -				   struct kfd_process *p, void *data)
> +int kfd_ioctl_export_dmabuf(struct file *filep,
> +			    struct kfd_process *p, void *data)
>  {
>  	struct kfd_ioctl_export_dmabuf_args *args = data;
>  	struct kfd_process_device *pdd;

[Severity: Medium]
Does this code permanently leak the file descriptor when called from a kernel
thread?

kfd_ioctl_export_dmabuf() calls dma_buf_fd(), which uses fd_install() to
allocate an integer file descriptor in the current process's FD table.
Because kernel threads execute with isolated FD tables and have no mechanism
to pass integer FDs back to userspace or automatically clean them up, both
the file descriptor and its underlying dmabuf reference might leak.

> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> index 81900b49d9d5b..91ef4c61a8bc3 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c
> @@ -1068,6 +1068,116 @@ int kfd_wait_on_events(struct kfd_process *p,
>  	return ret;
>  }
>  
> +int kfd_wait_on_events_kernel(struct kfd_process *p,
> +			      uint32_t num_events, void __user *data,
> +			      bool all, uint32_t *user_timeout_ms,
> +			      uint32_t *wait_result)
> +{
> +	struct kfd_event_data *events = (struct kfd_event_data *) data;
> +	uint32_t i;
> +	int ret = 0;
> +
> +	struct kfd_event_waiter *event_waiters = NULL;
> +	long timeout = user_timeout_to_jiffies(*user_timeout_ms);
> +
> +	event_waiters = alloc_event_waiters(num_events);
> +	if (!event_waiters) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	/* Use p->event_mutex here to protect against concurrent creation and
> +	 * destruction of events while we initialize event_waiters.
> +	 */
> +	mutex_lock(&p->event_mutex);
> +
> +	for (i = 0; i < num_events; i++) {
> +		struct kfd_event_data event_data;
> +
> +		memcpy(&event_data, &events[i], sizeof(struct kfd_event_data));
> +		ret = init_event_waiter(p, &event_waiters[i], &event_data);
> +		if (ret)
> +			goto out_unlock;
> +	}
> +
> +	/* Check condition once. */
> +	*wait_result = test_event_condition(all, num_events, event_waiters);
> +	if (*wait_result == KFD_IOC_WAIT_RESULT_COMPLETE) {
> +		ret = copy_signaled_event_data(num_events,
> +					       event_waiters, events);
> +		goto out_unlock;
> +	}

[Severity: High]
Can this mix of memory handling semantics cause failures for both user and
kernel callers?

When evaluating the loop in kfd_wait_on_events_kernel(), there is a direct
memcpy() from the data pointer. If a user pointer is inadvertently passed,
this bypasses copy_from_user(), which can trigger a SMAP violation.

However, when waking up on the fast or slow path, it relies on
copy_signaled_event_data() which internally uses copy_to_user(). If a kernel
caller correctly provides a kernel pointer, copy_to_user() fails the
access_ok() check and returns -EFAULT, causing the event data to be dropped.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260719175857.4071636-1-ap420073@gmail.com?part=6