* Re: [PATCH] drm/syncobj: Stop reusing the same struct file for all syncobj -> fd
2017-12-19 12:07 ` [PATCH] drm/syncobj: Stop reusing the same struct file for all syncobj -> fd Chris Wilson
@ 2017-12-19 12:28 ` Daniel Vetter
2017-12-21 2:42 ` Dave Airlie
2017-12-21 9:28 ` [PATCH v2] " Chris Wilson
2 siblings, 0 replies; 12+ messages in thread
From: Daniel Vetter @ 2017-12-19 12:28 UTC (permalink / raw)
To: Chris Wilson; +Cc: Dave Airlie, intel-gfx, dri-devel
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] drm/syncobj: Stop reusing the same struct file for all syncobj -> fd
2017-12-19 12:07 ` [PATCH] drm/syncobj: Stop reusing the same struct file for all syncobj -> fd Chris Wilson
2017-12-19 12:28 ` Daniel Vetter
@ 2017-12-21 2:42 ` Dave Airlie
2017-12-21 7:51 ` [Intel-gfx] " Chris Wilson
2017-12-21 9:28 ` [PATCH v2] " Chris Wilson
2 siblings, 1 reply; 12+ messages in thread
From: Dave Airlie @ 2017-12-21 2:42 UTC (permalink / raw)
To: Chris Wilson; +Cc: Dave Airlie, intel-gfx, dri-devel
> @@ -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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Intel-gfx] [PATCH] drm/syncobj: Stop reusing the same struct file for all syncobj -> fd
2017-12-21 2:42 ` Dave Airlie
@ 2017-12-21 7:51 ` Chris Wilson
0 siblings, 0 replies; 12+ messages in thread
From: Chris Wilson @ 2017-12-21 7:51 UTC (permalink / raw)
To: Dave Airlie; +Cc: Dave Airlie, intel-gfx, dri-devel
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2] drm/syncobj: Stop reusing the same struct file for all syncobj -> fd
2017-12-19 12:07 ` [PATCH] drm/syncobj: Stop reusing the same struct file for all syncobj -> fd Chris Wilson
2017-12-19 12:28 ` Daniel Vetter
2017-12-21 2:42 ` Dave Airlie
@ 2017-12-21 9:28 ` Chris Wilson
2017-12-21 10:50 ` Chris Wilson
2017-12-22 3:05 ` Dave Airlie
2 siblings, 2 replies; 12+ messages in thread
From: Chris Wilson @ 2017-12-21 9:28 UTC (permalink / raw)
To: intel-gfx; +Cc: Daniel Vetter, Dave Airlie
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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v2] drm/syncobj: Stop reusing the same struct file for all syncobj -> fd
2017-12-21 9:28 ` [PATCH v2] " Chris Wilson
@ 2017-12-21 10:50 ` Chris Wilson
2017-12-22 3:05 ` Dave Airlie
1 sibling, 0 replies; 12+ messages in thread
From: Chris Wilson @ 2017-12-21 10:50 UTC (permalink / raw)
To: intel-gfx; +Cc: Dave Airlie, Daniel Vetter
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2] drm/syncobj: Stop reusing the same struct file for all syncobj -> fd
2017-12-21 9:28 ` [PATCH v2] " Chris Wilson
2017-12-21 10:50 ` Chris Wilson
@ 2017-12-22 3:05 ` Dave Airlie
2017-12-22 9:28 ` Chris Wilson
1 sibling, 1 reply; 12+ messages in thread
From: Dave Airlie @ 2017-12-22 3:05 UTC (permalink / raw)
To: Chris Wilson; +Cc: Daniel Vetter, intel-gfx, 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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2] drm/syncobj: Stop reusing the same struct file for all syncobj -> fd
2017-12-22 3:05 ` Dave Airlie
@ 2017-12-22 9:28 ` Chris Wilson
0 siblings, 0 replies; 12+ messages in thread
From: Chris Wilson @ 2017-12-22 9:28 UTC (permalink / raw)
To: Dave Airlie; +Cc: Daniel Vetter, intel-gfx, Dave Airlie
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
^ permalink raw reply [flat|nested] 12+ messages in thread