[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

✓ Fi.CI.BAT: success for drm/syncobj: Stop reusing the same struct file for all syncobj -> fd

[Patchwork]

== Series Details ==

Series: drm/syncobj: Stop reusing the same struct file for all syncobj -> fd
URL   : https://patchwork.freedesktop.org/series/35576/
State : success

== Summary ==

Series 35576v1 drm/syncobj: Stop reusing the same struct file for all syncobj -> fd
https://patchwork.freedesktop.org/api/1.0/series/35576/revisions/1/mbox/

Test gem_exec_nop:
        Subgroup basic-series:
                incomplete -> PASS       (fi-snb-2600)
Test gem_mmap_gtt:
        Subgroup basic-small-bo-tiledx:
                pass       -> FAIL       (fi-gdg-551) fdo#102575
Test kms_psr_sink_crc:
        Subgroup psr_basic:
                dmesg-warn -> PASS       (fi-skl-6700hq) fdo#101144

fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
fdo#101144 https://bugs.freedesktop.org/show_bug.cgi?id=101144

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:439s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:437s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:381s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:495s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:275s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:492s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:491s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:478s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:474s
fi-elk-e7500     total:224  pass:163  dwarn:15  dfail:0   fail:0   skip:45 
fi-gdg-551       total:288  pass:178  dwarn:1   dfail:0   fail:1   skip:108 time:262s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:531s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:403s
fi-hsw-4770r     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:414s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:385s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:477s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:432s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:477s
fi-kbl-7560u     total:288  pass:268  dwarn:1   dfail:0   fail:0   skip:19  time:523s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:467s
fi-kbl-r         total:288  pass:260  dwarn:1   dfail:0   fail:0   skip:27  time:523s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:585s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:443s
fi-skl-6600u     total:288  pass:260  dwarn:1   dfail:0   fail:0   skip:27  time:527s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:558s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:505s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:502s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:444s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:543s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:406s
Blacklisted hosts:
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:593s

e044c9e03182a12636d79c909d3f272436862be6 drm-tip: 2017y-12m-19d-12h-46m-29s UTC integration manifest
3ed6b4d63f32 drm/syncobj: Stop reusing the same struct file for all syncobj -> fd

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7537/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/syncobj: Stop reusing the same struct file for all syncobj -> fd

[Patchwork]

== Series Details ==

Series: drm/syncobj: Stop reusing the same struct file for all syncobj -> fd
URL   : https://patchwork.freedesktop.org/series/35576/
State : success

== Summary ==

Test kms_cursor_legacy:
        Subgroup cursor-vs-flip-atomic:
                skip       -> PASS       (shard-hsw)
Test kms_flip:
        Subgroup flip-vs-panning-vs-hang-interruptible:
                pass       -> DMESG-WARN (shard-snb) fdo#103821
Test gem_eio:
        Subgroup in-flight:
                dmesg-warn -> PASS       (shard-snb) fdo#104058
Test kms_frontbuffer_tracking:
        Subgroup fbc-1p-offscren-pri-shrfb-draw-render:
                pass       -> FAIL       (shard-snb) fdo#101623

fdo#103821 https://bugs.freedesktop.org/show_bug.cgi?id=103821
fdo#104058 https://bugs.freedesktop.org/show_bug.cgi?id=104058
fdo#101623 https://bugs.freedesktop.org/show_bug.cgi?id=101623

shard-hsw        total:2712 pass:1537 dwarn:1   dfail:0   fail:10  skip:1164 time:9432s
shard-snb        total:2712 pass:1306 dwarn:2   dfail:0   fail:13  skip:1391 time:8018s
Blacklisted hosts:
shard-apl        total:2712 pass:1687 dwarn:1   dfail:1   fail:22  skip:1001 time:13769s
shard-kbl        total:2712 pass:1804 dwarn:1   dfail:0   fail:26  skip:881 time:11151s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7537/shards.html
_______________________________________________
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

[PATCH v2] 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. So stop trying to reuse the same struct file for every fd pointing
to the drm_syncobj, and allocate one file for each fd instead.

v2: Fixup return handling of drm_syncobj_fd_to_handle

Reported-by: Dave Airlie <airlied@redhat.com>
Testcase: igt/syncobj_base/test-create-close-twice
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_syncobj.c | 78 ++++++++++++++++---------------------------
 include/drm/drm_syncobj.h     |  4 ---
 2 files changed, 29 insertions(+), 53 deletions(-)

diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index 131695915acd..c235046c9ac2 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,32 +447,22 @@ 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;
 
-	/* take a reference to put in the idr */
-	drm_syncobj_get(syncobj);
+	if (file->f_op != &drm_syncobj_file_fops) {
+		ret = -EINVAL;
+		goto out_file;
+	}
+	syncobj = file->private_data;
 
 	idr_preload(GFP_KERNEL);
 	spin_lock(&file_private->syncobj_table_lock);
@@ -494,12 +470,16 @@ 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;
+	if (ret > 0) {
+		/* take a reference for the idr */
+		drm_syncobj_get(syncobj);
+		*handle = ret;
+		ret = 0;
 	}
-	*handle = ret;
-	return 0;
+
+out_file:
+	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

✓ Fi.CI.BAT: success for drm/syncobj: Stop reusing the same struct file for all syncobj -> fd (rev2)

[Patchwork]

== Series Details ==

Series: drm/syncobj: Stop reusing the same struct file for all syncobj -> fd (rev2)
URL   : https://patchwork.freedesktop.org/series/35576/
State : success

== Summary ==

Series 35576v2 drm/syncobj: Stop reusing the same struct file for all syncobj -> fd
https://patchwork.freedesktop.org/api/1.0/series/35576/revisions/2/mbox/

Test debugfs_test:
        Subgroup read_all_entries:
                dmesg-warn -> PASS       (fi-bdw-gvtdvm) fdo#103938 +1
Test gem_mmap_gtt:
        Subgroup basic-small-bo-tiledx:
                pass       -> FAIL       (fi-gdg-551) fdo#102575
Test kms_psr_sink_crc:
        Subgroup psr_basic:
                pass       -> DMESG-WARN (fi-skl-6700hq) fdo#101144

fdo#103938 https://bugs.freedesktop.org/show_bug.cgi?id=103938
fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
fdo#101144 https://bugs.freedesktop.org/show_bug.cgi?id=101144

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:435s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:441s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:380s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:504s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:279s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:488s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:495s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:483s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:471s
fi-elk-e7500     total:224  pass:163  dwarn:15  dfail:0   fail:0   skip:45 
fi-gdg-551       total:288  pass:178  dwarn:1   dfail:0   fail:1   skip:108 time:265s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:539s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:409s
fi-hsw-4770r     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:415s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:430s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:476s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:426s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:476s
fi-kbl-7560u     total:288  pass:268  dwarn:1   dfail:0   fail:0   skip:19  time:520s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:468s
fi-kbl-r         total:288  pass:260  dwarn:1   dfail:0   fail:0   skip:27  time:528s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:576s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:446s
fi-skl-6600u     total:288  pass:260  dwarn:1   dfail:0   fail:0   skip:27  time:540s
fi-skl-6700hq    total:288  pass:261  dwarn:1   dfail:0   fail:0   skip:26  time:558s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:508s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:499s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:448s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:555s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:413s
Blacklisted hosts:
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:592s

e421f7f2b48c47438cd22d673a2c025562d1f728 drm-tip: 2017y-12m-21d-08h-52m-50s UTC integration manifest
8307012eade3 drm/syncobj: Stop reusing the same struct file for all syncobj -> fd

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7556/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

Quoting Chris Wilson (2017-12-21 09:28:42)
> 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. So stop trying to reuse the same struct file for every fd pointing
> to the drm_syncobj, and allocate one file for each fd instead.
> 
> v2: Fixup return handling of drm_syncobj_fd_to_handle
> 
> Reported-by: Dave Airlie <airlied@redhat.com>
Fixes: e9083420bbac ("drm: introduce sync objects (v4)")

> Testcase: igt/syncobj_base/test-create-close-twice
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: <stable@vger.kernel.org> # v4.13+
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: warning for drm/syncobj: Stop reusing the same struct file for all syncobj -> fd (rev2)

[Patchwork]

== Series Details ==

Series: drm/syncobj: Stop reusing the same struct file for all syncobj -> fd (rev2)
URL   : https://patchwork.freedesktop.org/series/35576/
State : warning

== Summary ==

Test kms_frontbuffer_tracking:
        Subgroup fbc-1p-offscren-pri-indfb-draw-render:
                skip       -> PASS       (shard-snb) fdo#103167 +1
        Subgroup fbc-1p-primscrn-shrfb-msflip-blt:
                skip       -> PASS       (shard-hsw) fdo#101623
Test pm_rpm:
        Subgroup system-suspend-modeset:
                skip       -> PASS       (shard-hsw)
Test kms_fbcon_fbt:
        Subgroup fbc-suspend:
                pass       -> SKIP       (shard-snb)
Test kms_plane:
        Subgroup plane-panning-bottom-right-suspend-pipe-b-planes:
                pass       -> SKIP       (shard-hsw)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                pass       -> DMESG-WARN (shard-snb) fdo#103375
Test kms_draw_crc:
        Subgroup draw-method-xrgb2101010-mmap-gtt-xtiled:
                pass       -> SKIP       (shard-hsw)
Test kms_busy:
        Subgroup extended-pageflip-modeset-hang-oldfb-render-b:
                pass       -> SKIP       (shard-hsw)
Test kms_mmio_vs_cs_flip:
        Subgroup setcrtc_vs_cs_flip:
                pass       -> SKIP       (shard-hsw)

fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
fdo#101623 https://bugs.freedesktop.org/show_bug.cgi?id=101623
fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375

shard-hsw        total:2712 pass:1533 dwarn:1   dfail:0   fail:10  skip:1168 time:9296s
shard-snb        total:2664 pass:1285 dwarn:2   dfail:0   fail:11  skip:1365 time:7711s
Blacklisted hosts:
shard-apl        total:2690 pass:1663 dwarn:1   dfail:0   fail:24  skip:1001 time:13385s
shard-kbl        total:2664 pass:1769 dwarn:2   dfail:0   fail:24  skip:867 time:10494s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7556/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Dave Airlie]

On 21 December 2017 at 19:28, Chris Wilson <chris@chris-wilson.co.uk> 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. So stop trying to reuse the same struct file for every fd pointing
> to the drm_syncobj, and allocate one file for each fd instead.
>
> v2: Fixup return handling of drm_syncobj_fd_to_handle
>
> Reported-by: Dave Airlie <airlied@redhat.com>
> Testcase: igt/syncobj_base/test-create-close-twice
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_syncobj.c | 78 ++++++++++++++++---------------------------
>  include/drm/drm_syncobj.h     |  4 ---
>  2 files changed, 29 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index 131695915acd..c235046c9ac2 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,32 +447,22 @@ 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;
>
> -       /* take a reference to put in the idr */
> -       drm_syncobj_get(syncobj);
> +       if (file->f_op != &drm_syncobj_file_fops) {
> +               ret = -EINVAL;
> +               goto out_file;
> +       }
> +       syncobj = file->private_data;
>
>         idr_preload(GFP_KERNEL);
>         spin_lock(&file_private->syncobj_table_lock);
> @@ -494,12 +470,16 @@ 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;
> +       if (ret > 0) {
> +               /* take a reference for the idr */
> +               drm_syncobj_get(syncobj);
> +               *handle = ret;
> +               ret = 0;

I can't see getting the refcount after adding it to the idr is safe here.

Entering this function we could have just the fd reference to the syncobj,
We add it to the dir, and someone could call syncobj_destroy on the
handle randomly (i.e. without us having it), then we'd free syncobj.

I'm going to push this patch with the get in there, but with a put
in the error path I think.

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

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

[Chris Wilson]

Quoting Dave Airlie (2017-12-22 03:05:16)
> On 21 December 2017 at 19:28, Chris Wilson <chris@chris-wilson.co.uk> 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. So stop trying to reuse the same struct file for every fd pointing
> > to the drm_syncobj, and allocate one file for each fd instead.
> >
> > v2: Fixup return handling of drm_syncobj_fd_to_handle
> >
> > Reported-by: Dave Airlie <airlied@redhat.com>
> > Testcase: igt/syncobj_base/test-create-close-twice
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Dave Airlie <airlied@redhat.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/drm_syncobj.c | 78 ++++++++++++++++---------------------------
> >  include/drm/drm_syncobj.h     |  4 ---
> >  2 files changed, 29 insertions(+), 53 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> > index 131695915acd..c235046c9ac2 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,32 +447,22 @@ 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;
> >
> > -       /* take a reference to put in the idr */
> > -       drm_syncobj_get(syncobj);
> > +       if (file->f_op != &drm_syncobj_file_fops) {
> > +               ret = -EINVAL;
> > +               goto out_file;
> > +       }
> > +       syncobj = file->private_data;
> >
> >         idr_preload(GFP_KERNEL);
> >         spin_lock(&file_private->syncobj_table_lock);
> > @@ -494,12 +470,16 @@ 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;
> > +       if (ret > 0) {
> > +               /* take a reference for the idr */
> > +               drm_syncobj_get(syncobj);
> > +               *handle = ret;
> > +               ret = 0;
> 
> I can't see getting the refcount after adding it to the idr is safe here.

I was thinking we were protected by our local file ref, and that the
other end hasn't seen handle yet so can't manipulate the idr location.
I don't think the idr is allowed to be reaped (on drm_file release)
while in this function. So I think it is safe, but preinc + dec-on-error
is safer
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx