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

2019-12-05 Thread Felix Kuehling 

On 2019-12-05 11:10 a.m., Philip Yang wrote:

One comment in line.

With it is fixed, this is reviewed by Philip Yang 

Philip

On 2019-12-04 11:13 p.m., Felix Kuehling wrote:
  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))


Thanks for catching that. I think !IS_ERR should be fine. Process should 
never be NULL here. Also the caller of this function only checks IS_ERR.


Regards,
  Felix





+    kref_get(>ref);
  mutex_unlock(_processes_mutex);
    return process;


___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

2019-12-05 Thread Philip Yang 

One comment in line.

With it is fixed, this is reviewed by Philip Yang 

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 
---
  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(>ref);
mutex_unlock(_processes_mutex);
  
  	return process;



___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

2019-12-04 Thread Felix Kuehling 
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 
---
 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)
+   kref_get(>ref);
mutex_unlock(_processes_mutex);
 
return process;
-- 
2.24.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx