[PATCH] drm/syncobj: reset file ptr to NULL when released.

[PATCH] drm/syncobj: reset file ptr to NULL when released.

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

The vk cts test:
dEQP-VK.api.external.semaphore.opaque_fd.export_multiple_times_temporary

triggers a lot of
VFS: Close: file count is 0

This patch fixes it, but I'm guessing it's racy and someone will
smell rcu, but I just wanted to send out the proof of fixing it
so I remember.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_syncobj.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index f776fc1cc543..ffa5bbd75852 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -361,6 +361,7 @@ static int drm_syncobj_file_release(struct inode *inode, struct file *file)
 {
 	struct drm_syncobj *syncobj = file->private_data;
 
+	syncobj->file = NULL;
 	drm_syncobj_put(syncobj);
 	return 0;
 }
-- 
2.14.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/syncobj: reset file ptr to NULL when released.

[Chris Wilson]

Quoting Dave Airlie (2017-12-19 11:30:12)
> From: Dave Airlie <airlied@redhat.com>
> 
> The vk cts test:
> dEQP-VK.api.external.semaphore.opaque_fd.export_multiple_times_temporary
> 
> triggers a lot of
> VFS: Close: file count is 0
> 
> This patch fixes it, but I'm guessing it's racy and someone will
> smell rcu, but I just wanted to send out the proof of fixing it
> so I remember.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>  drivers/gpu/drm/drm_syncobj.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index f776fc1cc543..ffa5bbd75852 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -361,6 +361,7 @@ static int drm_syncobj_file_release(struct inode *inode, struct file *file)
>  {
>         struct drm_syncobj *syncobj = file->private_data;
>  
> +       syncobj->file = NULL;

Oh. That's scary as the opposite side assumes that the syncobj->file is
assigned for the lifetime of the syncobj - once allocated is never
unset.

If we stop trying to reuse the struct file and just allocated one for
each fd, the complications just vanish?

diff --cc include/drm/drm_syncobj.h
index 3980602472c0,ba54e0e58bbc..000000000000
--- a/include/drm/drm_syncobj.h
+++ b/include/drm/drm_syncobj.h
@@@ -56,10 -57,8 +56,6 @@@ struct drm_syncobj 
  	 * @lock: Protects &cb_list and write-locks &fence.
  	 */
  	spinlock_t lock;
- 	/**
- 	 * @file: A file backing for this syncobj.
- 	 */
- 	struct file *file;
 -
 -	struct rcu_head rcu;
  };
  
  typedef void (*drm_syncobj_func_t)(struct drm_syncobj *syncobj,
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index 131695915acd..0cca2e792719 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -399,23 +399,6 @@ static const struct file_operations drm_syncobj_file_fops = {
 	.release = drm_syncobj_file_release,
 };
 
-static int drm_syncobj_alloc_file(struct drm_syncobj *syncobj)
-{
-	struct file *file = anon_inode_getfile("syncobj_file",
-					       &drm_syncobj_file_fops,
-					       syncobj, 0);
-	if (IS_ERR(file))
-		return PTR_ERR(file);
-
-	drm_syncobj_get(syncobj);
-	if (cmpxchg(&syncobj->file, NULL, file)) {
-		/* lost the race */
-		fput(file);
-	}
-
-	return 0;
-}
-
 /**
  * drm_syncobj_get_fd - get a file descriptor from a syncobj
  * @syncobj: Sync object to export
@@ -427,21 +410,24 @@ static int drm_syncobj_alloc_file(struct drm_syncobj *syncobj)
  */
 int drm_syncobj_get_fd(struct drm_syncobj *syncobj, int *p_fd)
 {
-	int ret;
+	struct file *file;
 	int fd;
 
 	fd = get_unused_fd_flags(O_CLOEXEC);
 	if (fd < 0)
 		return fd;
 
-	if (!syncobj->file) {
-		ret = drm_syncobj_alloc_file(syncobj);
-		if (ret) {
-			put_unused_fd(fd);
-			return ret;
-		}
+	file = anon_inode_getfile("syncobj_file",
+				  &drm_syncobj_file_fops,
+				  syncobj, 0);
+	if (IS_ERR(file)) {
+		put_unused_fd(fd);
+		return PTR_ERR(file);
 	}
-	fd_install(fd, syncobj->file);
+
+	drm_syncobj_get(syncobj);
+	fd_install(fd, file);
+
 	*p_fd = fd;
 	return 0;
 }
@@ -461,31 +447,24 @@ static int drm_syncobj_handle_to_fd(struct drm_file *file_private,
 	return ret;
 }
 
-static struct drm_syncobj *drm_syncobj_fdget(int fd)
-{
-	struct file *file = fget(fd);
-
-	if (!file)
-		return NULL;
-	if (file->f_op != &drm_syncobj_file_fops)
-		goto err;
-
-	return file->private_data;
-err:
-	fput(file);
-	return NULL;
-};
-
 static int drm_syncobj_fd_to_handle(struct drm_file *file_private,
 				    int fd, u32 *handle)
 {
-	struct drm_syncobj *syncobj = drm_syncobj_fdget(fd);
+	struct drm_syncobj *syncobj;
+	struct file *file;
 	int ret;
 
-	if (!syncobj)
+	file = fget(fd);
+	if (!file)
 		return -EINVAL;
 
+	if (file->f_op != &drm_syncobj_file_fops) {
+		fput(file);
+		return -EINVAL;
+	}
+
 	/* take a reference to put in the idr */
+	syncobj = file->private_data;
 	drm_syncobj_get(syncobj);
 
 	idr_preload(GFP_KERNEL);
@@ -494,12 +473,11 @@ static int drm_syncobj_fd_to_handle(struct drm_file *file_private,
 	spin_unlock(&file_private->syncobj_table_lock);
 	idr_preload_end();
 
-	if (ret < 0) {
-		fput(syncobj->file);
-		return ret;
-	}
-	*handle = ret;
-	return 0;
+	if (ret > 0)
+		*handle = ret;
+
+	fput(file);
+	return ret;
 }
 
 static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private,
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/syncobj: reset file ptr to NULL when released.

[Daniel Vetter]

On Tue, Dec 19, 2017 at 09:30:12PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> The vk cts test:
> dEQP-VK.api.external.semaphore.opaque_fd.export_multiple_times_temporary
> 
> triggers a lot of
> VFS: Close: file count is 0
> 
> This patch fixes it, but I'm guessing it's racy and someone will
> smell rcu, but I just wanted to send out the proof of fixing it
> so I remember.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>  drivers/gpu/drm/drm_syncobj.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index f776fc1cc543..ffa5bbd75852 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -361,6 +361,7 @@ static int drm_syncobj_file_release(struct inode *inode, struct file *file)
>  {
>  	struct drm_syncobj *syncobj = file->private_data;
>  
> +	syncobj->file = NULL;

Yeah that won't work :-)

The problem is that you have a pointer to the file without holding a
reference. But if you'd hold a full reference then there's a loop and it
never frees.

We need the same trick as for dma_buf and gem objects where we track the
number of idr entries in ->handle_count and drop the ->file cache when
that reaches 0.

Or you just allocate a new file every time userspace asks for one. The
only reason we cache them for dma-buf is to allow userspace to figure out
uniqueness of reimported stuff (since some drivers reject execbuf if you
give them the underlying object twice). This option is definitely less
typing if you can get away with it :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/syncobj: Stop reusing the same struct file for all syncobj -> fd

[Chris Wilson]

The vk cts test:
dEQP-VK.api.external.semaphore.opaque_fd.export_multiple_times_temporary

triggers a lot of
VFS: Close: file count is 0

Dave pointed out that clearing the syncobj->file from
drm_syncobj_file_release() was sufficient to silence the test, but that
opens a can of worm since we assumed that the syncobj->file was never
unset. Stop trying to reuse the same struct file for every fd pointing
to the drm_syncobj, and allocate one file for each fd instead.

Reported-by: Dave Airlie <airlied@redhat.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_syncobj.c | 74 +++++++++++++++----------------------------
 include/drm/drm_syncobj.h     |  4 ---
 2 files changed, 26 insertions(+), 52 deletions(-)

diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index 131695915acd..0cca2e792719 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -399,23 +399,6 @@ static const struct file_operations drm_syncobj_file_fops = {
 	.release = drm_syncobj_file_release,
 };
 
-static int drm_syncobj_alloc_file(struct drm_syncobj *syncobj)
-{
-	struct file *file = anon_inode_getfile("syncobj_file",
-					       &drm_syncobj_file_fops,
-					       syncobj, 0);
-	if (IS_ERR(file))
-		return PTR_ERR(file);
-
-	drm_syncobj_get(syncobj);
-	if (cmpxchg(&syncobj->file, NULL, file)) {
-		/* lost the race */
-		fput(file);
-	}
-
-	return 0;
-}
-
 /**
  * drm_syncobj_get_fd - get a file descriptor from a syncobj
  * @syncobj: Sync object to export
@@ -427,21 +410,24 @@ static int drm_syncobj_alloc_file(struct drm_syncobj *syncobj)
  */
 int drm_syncobj_get_fd(struct drm_syncobj *syncobj, int *p_fd)
 {
-	int ret;
+	struct file *file;
 	int fd;
 
 	fd = get_unused_fd_flags(O_CLOEXEC);
 	if (fd < 0)
 		return fd;
 
-	if (!syncobj->file) {
-		ret = drm_syncobj_alloc_file(syncobj);
-		if (ret) {
-			put_unused_fd(fd);
-			return ret;
-		}
+	file = anon_inode_getfile("syncobj_file",
+				  &drm_syncobj_file_fops,
+				  syncobj, 0);
+	if (IS_ERR(file)) {
+		put_unused_fd(fd);
+		return PTR_ERR(file);
 	}
-	fd_install(fd, syncobj->file);
+
+	drm_syncobj_get(syncobj);
+	fd_install(fd, file);
+
 	*p_fd = fd;
 	return 0;
 }
@@ -461,31 +447,24 @@ static int drm_syncobj_handle_to_fd(struct drm_file *file_private,
 	return ret;
 }
 
-static struct drm_syncobj *drm_syncobj_fdget(int fd)
-{
-	struct file *file = fget(fd);
-
-	if (!file)
-		return NULL;
-	if (file->f_op != &drm_syncobj_file_fops)
-		goto err;
-
-	return file->private_data;
-err:
-	fput(file);
-	return NULL;
-};
-
 static int drm_syncobj_fd_to_handle(struct drm_file *file_private,
 				    int fd, u32 *handle)
 {
-	struct drm_syncobj *syncobj = drm_syncobj_fdget(fd);
+	struct drm_syncobj *syncobj;
+	struct file *file;
 	int ret;
 
-	if (!syncobj)
+	file = fget(fd);
+	if (!file)
 		return -EINVAL;
 
+	if (file->f_op != &drm_syncobj_file_fops) {
+		fput(file);
+		return -EINVAL;
+	}
+
 	/* take a reference to put in the idr */
+	syncobj = file->private_data;
 	drm_syncobj_get(syncobj);
 
 	idr_preload(GFP_KERNEL);
@@ -494,12 +473,11 @@ static int drm_syncobj_fd_to_handle(struct drm_file *file_private,
 	spin_unlock(&file_private->syncobj_table_lock);
 	idr_preload_end();
 
-	if (ret < 0) {
-		fput(syncobj->file);
-		return ret;
-	}
-	*handle = ret;
-	return 0;
+	if (ret > 0)
+		*handle = ret;
+
+	fput(file);
+	return ret;
 }
 
 static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private,
diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h
index 3980602472c0..ca5bf7d12d0b 100644
--- a/include/drm/drm_syncobj.h
+++ b/include/drm/drm_syncobj.h
@@ -56,10 +56,6 @@ struct drm_syncobj {
 	 * @lock: Protects &cb_list and write-locks &fence.
 	 */
 	spinlock_t lock;
-	/**
-	 * @file: A file backing for this syncobj.
-	 */
-	struct file *file;
 };
 
 typedef void (*drm_syncobj_func_t)(struct drm_syncobj *syncobj,
-- 
2.15.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/syncobj: Stop reusing the same struct file for all syncobj -> fd

[Daniel Vetter]

On Tue, Dec 19, 2017 at 12:07:00PM +0000, Chris Wilson wrote:
> The vk cts test:
> dEQP-VK.api.external.semaphore.opaque_fd.export_multiple_times_temporary
> 
> triggers a lot of
> VFS: Close: file count is 0
> 
> Dave pointed out that clearing the syncobj->file from
> drm_syncobj_file_release() was sufficient to silence the test, but that
> opens a can of worm since we assumed that the syncobj->file was never
> unset. Stop trying to reuse the same struct file for every fd pointing
> to the drm_syncobj, and allocate one file for each fd instead.

It's worse: syncobj->file points to a refcounted thing, and we never did
grab a reference for it. This is a classic use-after-free thing :-)

> Reported-by: Dave Airlie <airlied@redhat.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Dave Airlie <airlied@redhat.com>

Assuming it doesn't break the vk testsuite:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Also an igt for this would be nice:
1. create syncobj
2. export to fd
3. close fd, note that now syncobj->file points to a freed struct file
4. reexport -> BOOM

Cheers, Daniel

> ---
>  drivers/gpu/drm/drm_syncobj.c | 74 +++++++++++++++----------------------------
>  include/drm/drm_syncobj.h     |  4 ---
>  2 files changed, 26 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index 131695915acd..0cca2e792719 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -399,23 +399,6 @@ static const struct file_operations drm_syncobj_file_fops = {
>  	.release = drm_syncobj_file_release,
>  };
>  
> -static int drm_syncobj_alloc_file(struct drm_syncobj *syncobj)
> -{
> -	struct file *file = anon_inode_getfile("syncobj_file",
> -					       &drm_syncobj_file_fops,
> -					       syncobj, 0);
> -	if (IS_ERR(file))
> -		return PTR_ERR(file);
> -
> -	drm_syncobj_get(syncobj);
> -	if (cmpxchg(&syncobj->file, NULL, file)) {
> -		/* lost the race */
> -		fput(file);
> -	}
> -
> -	return 0;
> -}
> -
>  /**
>   * drm_syncobj_get_fd - get a file descriptor from a syncobj
>   * @syncobj: Sync object to export
> @@ -427,21 +410,24 @@ static int drm_syncobj_alloc_file(struct drm_syncobj *syncobj)
>   */
>  int drm_syncobj_get_fd(struct drm_syncobj *syncobj, int *p_fd)
>  {
> -	int ret;
> +	struct file *file;
>  	int fd;
>  
>  	fd = get_unused_fd_flags(O_CLOEXEC);
>  	if (fd < 0)
>  		return fd;
>  
> -	if (!syncobj->file) {
> -		ret = drm_syncobj_alloc_file(syncobj);
> -		if (ret) {
> -			put_unused_fd(fd);
> -			return ret;
> -		}
> +	file = anon_inode_getfile("syncobj_file",
> +				  &drm_syncobj_file_fops,
> +				  syncobj, 0);
> +	if (IS_ERR(file)) {
> +		put_unused_fd(fd);
> +		return PTR_ERR(file);
>  	}
> -	fd_install(fd, syncobj->file);
> +
> +	drm_syncobj_get(syncobj);
> +	fd_install(fd, file);
> +
>  	*p_fd = fd;
>  	return 0;
>  }
> @@ -461,31 +447,24 @@ static int drm_syncobj_handle_to_fd(struct drm_file *file_private,
>  	return ret;
>  }
>  
> -static struct drm_syncobj *drm_syncobj_fdget(int fd)
> -{
> -	struct file *file = fget(fd);
> -
> -	if (!file)
> -		return NULL;
> -	if (file->f_op != &drm_syncobj_file_fops)
> -		goto err;
> -
> -	return file->private_data;
> -err:
> -	fput(file);
> -	return NULL;
> -};
> -
>  static int drm_syncobj_fd_to_handle(struct drm_file *file_private,
>  				    int fd, u32 *handle)
>  {
> -	struct drm_syncobj *syncobj = drm_syncobj_fdget(fd);
> +	struct drm_syncobj *syncobj;
> +	struct file *file;
>  	int ret;
>  
> -	if (!syncobj)
> +	file = fget(fd);
> +	if (!file)
>  		return -EINVAL;
>  
> +	if (file->f_op != &drm_syncobj_file_fops) {
> +		fput(file);
> +		return -EINVAL;
> +	}
> +
>  	/* take a reference to put in the idr */
> +	syncobj = file->private_data;
>  	drm_syncobj_get(syncobj);
>  
>  	idr_preload(GFP_KERNEL);
> @@ -494,12 +473,11 @@ static int drm_syncobj_fd_to_handle(struct drm_file *file_private,
>  	spin_unlock(&file_private->syncobj_table_lock);
>  	idr_preload_end();
>  
> -	if (ret < 0) {
> -		fput(syncobj->file);
> -		return ret;
> -	}
> -	*handle = ret;
> -	return 0;
> +	if (ret > 0)
> +		*handle = ret;
> +
> +	fput(file);
> +	return ret;
>  }
>  
>  static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private,
> diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h
> index 3980602472c0..ca5bf7d12d0b 100644
> --- a/include/drm/drm_syncobj.h
> +++ b/include/drm/drm_syncobj.h
> @@ -56,10 +56,6 @@ struct drm_syncobj {
>  	 * @lock: Protects &cb_list and write-locks &fence.
>  	 */
>  	spinlock_t lock;
> -	/**
> -	 * @file: A file backing for this syncobj.
> -	 */
> -	struct file *file;
>  };
>  
>  typedef void (*drm_syncobj_func_t)(struct drm_syncobj *syncobj,
> -- 
> 2.15.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/syncobj: Stop reusing the same struct file for all syncobj -> fd

[Dave Airlie]

> @@ -494,12 +473,11 @@ static int drm_syncobj_fd_to_handle(struct drm_file *file_private,
>         spin_unlock(&file_private->syncobj_table_lock);
>         idr_preload_end();
>
> -       if (ret < 0) {
> -               fput(syncobj->file);
> -               return ret;
> -       }
> -       *handle = ret;
> -       return 0;
> +       if (ret > 0)
> +               *handle = ret;
> +
> +       fput(file);
> +       return ret;
>  }

This chunk breaks stuff, since it now returns the handle in ret if >
0, whereas before
it returned 0.

Otherwise the vulkan tests all pass on it.

Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/syncobj: Stop reusing the same struct file for all syncobj -> fd

[Chris Wilson]

Quoting Dave Airlie (2017-12-21 02:42:56)
> > @@ -494,12 +473,11 @@ static int drm_syncobj_fd_to_handle(struct drm_file *file_private,
> >         spin_unlock(&file_private->syncobj_table_lock);
> >         idr_preload_end();
> >
> > -       if (ret < 0) {
> > -               fput(syncobj->file);
> > -               return ret;
> > -       }
> > -       *handle = ret;
> > -       return 0;
> > +       if (ret > 0)
> > +               *handle = ret;
> > +
> > +       fput(file);
> > +       return ret;
> >  }
> 
> This chunk breaks stuff, since it now returns the handle in ret if >
> 0, whereas before
> it returned 0.

So much for trying to squeeze it into single point of return.
Do we prefer return ret; or ret = 0 ?
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel