[PATCH 1/1] drm/amdkfd: Improve kfd_process lookup in kfd_ioctl

[Philip Yang]

One comment in line.

With it is fixed, this is reviewed by Philip Yang <Philip.Yang at amd.com>

Philip

On 2019-12-04 11:13 p.m., Felix Kuehling wrote:
> Use filep->private_data to store a pointer to the kfd_process data
> structure. Take an extra reference for that, which gets released in
> the kfd_release callback. Check that the process calling kfd_ioctl
> is the same that opened the file descriptor. Return -EBADF if it's
> not, so that this error can be distinguished in user mode.
> 
> Signed-off-by: Felix Kuehling <Felix.Kuehling at amd.com>
> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 30 ++++++++++++++++++++----
>   drivers/gpu/drm/amd/amdkfd/kfd_process.c |  2 ++
>   2 files changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index 2cb51d1e857c..1aebda4bbbe7 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> @@ -42,6 +42,7 @@
>   
>   static long kfd_ioctl(struct file *, unsigned int, unsigned long);
>   static int kfd_open(struct inode *, struct file *);
> +static int kfd_release(struct inode *, struct file *);
>   static int kfd_mmap(struct file *, struct vm_area_struct *);
>   
>   static const char kfd_dev_name[] = "kfd";
> @@ -51,6 +52,7 @@ static const struct file_operations kfd_fops = {
>   	.unlocked_ioctl = kfd_ioctl,
>   	.compat_ioctl = kfd_ioctl,
>   	.open = kfd_open,
> +	.release = kfd_release,
>   	.mmap = kfd_mmap,
>   };
>   
> @@ -124,8 +126,13 @@ static int kfd_open(struct inode *inode, struct file *filep)
>   	if (IS_ERR(process))
>   		return PTR_ERR(process);
>   
> -	if (kfd_is_locked())
> +	if (kfd_is_locked()) {
> +		kfd_unref_process(process);
>   		return -EAGAIN;
> +	}
> +
> +	/* filep now owns the reference returned by kfd_create_process */
> +	filep->private_data = process;
>   
>   	dev_dbg(kfd_device, "process %d opened, compat mode (32 bit) - %d\n",
>   		process->pasid, process->is_32bit_user_mode);
> @@ -133,6 +140,16 @@ static int kfd_open(struct inode *inode, struct file *filep)
>   	return 0;
>   }
>   
> +static int kfd_release(struct inode *inode, struct file *filep)
> +{
> +	struct kfd_process *process = filep->private_data;
> +
> +	if (process)
> +		kfd_unref_process(process);
> +
> +	return 0;
> +}
> +
>   static int kfd_ioctl_get_version(struct file *filep, struct kfd_process *p,
>   					void *data)
>   {
> @@ -1840,9 +1857,14 @@ static long kfd_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
>   
>   	dev_dbg(kfd_device, "ioctl cmd 0x%x (#0x%x), arg 0x%lx\n", cmd, nr, arg);
>   
> -	process = kfd_get_process(current);
> -	if (IS_ERR(process)) {
> -		dev_dbg(kfd_device, "no process\n");
> +	/* Get the process struct from the filep. Only the process
> +	 * that opened /dev/kfd can use the file descriptor. Child
> +	 * processes need to create their own KFD device context.
> +	 */
> +	process = filep->private_data;
> +	if (process->lead_thread != current->group_leader) {
> +		dev_dbg(kfd_device, "Using KFD FD in wrong process\n");
> +		retcode = -EBADF;
>   		goto err_i1;
>   	}
>   
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> index 8276601a122f..bb2f26532feb 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> @@ -324,6 +324,8 @@ struct kfd_process *kfd_create_process(struct file *filep)
>   					(int)process->lead_thread->pid);
>   	}
>   out:
> +	if (process)
if (!IS_ERR_OR_NULL(process))

> +		kref_get(&process->ref);
>   	mutex_unlock(&kfd_processes_mutex);
>   
>   	return process;
>