| Message ID | 20240812065906.241398-1-viro@zeniv.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] new helper: drm_gem_prime_handle_to_dmabuf() | expand |
Hi Am 12.08.24 um 08:59 schrieb Al Viro: > Once something had been put into descriptor table, the only thing you > can do with it is returning descriptor to userland - you can't withdraw > it on subsequent failure exit, etc. You certainly can't count upon > it staying in the same slot of descriptor table - another thread > could've played with close(2)/dup2(2)/whatnot. This paragraph appears to refer to the newly added call to fd_install(). Maybe spell that out. > > Add drm_gem_prime_handle_to_dmabuf() - the "set dmabuf up" parts of > drm_gem_prime_handle_to_fd() without the descriptor-related ones. > Instead of inserting into descriptor table and returning the file > descriptor it just returns the struct file. > > drm_gem_prime_handle_to_fd() becomes a wrapper for it. Other users > will be introduced in the next commit. > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > --- > drivers/gpu/drm/drm_prime.c | 84 +++++++++++++++++++------------------ > include/drm/drm_prime.h | 3 ++ > 2 files changed, 46 insertions(+), 41 deletions(-) > > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c > index 03bd3c7bd0dc..467c7a278ad3 100644 > --- a/drivers/gpu/drm/drm_prime.c > +++ b/drivers/gpu/drm/drm_prime.c > @@ -409,23 +409,9 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev, > return dmabuf; > } > > -/** > - * drm_gem_prime_handle_to_fd - PRIME export function for GEM drivers > - * @dev: dev to export the buffer from > - * @file_priv: drm file-private structure > - * @handle: buffer handle to export > - * @flags: flags like DRM_CLOEXEC > - * @prime_fd: pointer to storage for the fd id of the create dma-buf > - * > - * This is the PRIME export function which must be used mandatorily by GEM > - * drivers to ensure correct lifetime management of the underlying GEM object. > - * The actual exporting from GEM object to a dma-buf is done through the > - * &drm_gem_object_funcs.export callback. > - */ > -int drm_gem_prime_handle_to_fd(struct drm_device *dev, > +struct dma_buf *drm_gem_prime_handle_to_dmabuf(struct drm_device *dev, If it's exported it should have kernel docs. At least copy-paste the docs from drm_gem_prime_handle_to_fd() and reword a few bits. Best regards Thomas > struct drm_file *file_priv, uint32_t handle, > - uint32_t flags, > - int *prime_fd) > + uint32_t flags) > { > struct drm_gem_object *obj; > int ret = 0; > @@ -434,14 +420,14 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, > mutex_lock(&file_priv->prime.lock); > obj = drm_gem_object_lookup(file_priv, handle); > if (!obj) { > - ret = -ENOENT; > + dmabuf = ERR_PTR(-ENOENT); > goto out_unlock; > } > > dmabuf = drm_prime_lookup_buf_by_handle(&file_priv->prime, handle); > if (dmabuf) { > get_dma_buf(dmabuf); > - goto out_have_handle; > + goto out; > } > > mutex_lock(&dev->object_name_lock); > @@ -463,7 +449,6 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, > /* normally the created dma-buf takes ownership of the ref, > * but if that fails then drop the ref > */ > - ret = PTR_ERR(dmabuf); > mutex_unlock(&dev->object_name_lock); > goto out; > } > @@ -478,34 +463,51 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, > ret = drm_prime_add_buf_handle(&file_priv->prime, > dmabuf, handle); > mutex_unlock(&dev->object_name_lock); > - if (ret) > - goto fail_put_dmabuf; > - > -out_have_handle: > - ret = dma_buf_fd(dmabuf, flags); > - /* > - * We must _not_ remove the buffer from the handle cache since the newly > - * created dma buf is already linked in the global obj->dma_buf pointer, > - * and that is invariant as long as a userspace gem handle exists. > - * Closing the handle will clean out the cache anyway, so we don't leak. > - */ > - if (ret < 0) { > - goto fail_put_dmabuf; > - } else { > - *prime_fd = ret; > - ret = 0; > + if (ret) { > + dma_buf_put(dmabuf); > + dmabuf = ERR_PTR(ret); > } > - > - goto out; > - > -fail_put_dmabuf: > - dma_buf_put(dmabuf); > out: > drm_gem_object_put(obj); > out_unlock: > mutex_unlock(&file_priv->prime.lock); > + return dmabuf; > +} > +EXPORT_SYMBOL(drm_gem_prime_handle_to_dmabuf); > > - return ret; > +/** > + * drm_gem_prime_handle_to_fd - PRIME export function for GEM drivers > + * @dev: dev to export the buffer from > + * @file_priv: drm file-private structure > + * @handle: buffer handle to export > + * @flags: flags like DRM_CLOEXEC > + * @prime_fd: pointer to storage for the fd id of the create dma-buf > + * > + * This is the PRIME export function which must be used mandatorily by GEM > + * drivers to ensure correct lifetime management of the underlying GEM object. > + * The actual exporting from GEM object to a dma-buf is done through the > + * &drm_gem_object_funcs.export callback. > + */ > +int drm_gem_prime_handle_to_fd(struct drm_device *dev, > + struct drm_file *file_priv, uint32_t handle, > + uint32_t flags, > + int *prime_fd) > +{ > + struct dma_buf *dmabuf; > + int fd = get_unused_fd_flags(flags); > + > + if (fd < 0) > + return fd; > + > + dmabuf = drm_gem_prime_handle_to_dmabuf(dev, file_priv, handle, flags); > + if (IS_ERR(dmabuf)) { > + put_unused_fd(fd); > + return PTR_ERR(dmabuf); > + } > + > + fd_install(fd, dmabuf->file); > + *prime_fd = fd; > + return 0; > } > EXPORT_SYMBOL(drm_gem_prime_handle_to_fd); > > diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h > index 2a1d01e5b56b..fa085c44d4ca 100644 > --- a/include/drm/drm_prime.h > +++ b/include/drm/drm_prime.h > @@ -69,6 +69,9 @@ void drm_gem_dmabuf_release(struct dma_buf *dma_buf); > > int drm_gem_prime_fd_to_handle(struct drm_device *dev, > struct drm_file *file_priv, int prime_fd, uint32_t *handle); > +struct dma_buf *drm_gem_prime_handle_to_dmabuf(struct drm_device *dev, > + struct drm_file *file_priv, uint32_t handle, > + uint32_t flags); > int drm_gem_prime_handle_to_fd(struct drm_device *dev, > struct drm_file *file_priv, uint32_t handle, uint32_t flags, > int *prime_fd);
On Thu, Aug 22, 2024 at 04:41:59PM +0200, Thomas Zimmermann wrote: > Hi > > Am 12.08.24 um 08:59 schrieb Al Viro: > > Once something had been put into descriptor table, the only thing you > > can do with it is returning descriptor to userland - you can't withdraw > > it on subsequent failure exit, etc. You certainly can't count upon > > it staying in the same slot of descriptor table - another thread > > could've played with close(2)/dup2(2)/whatnot. > > This paragraph appears to refer to the newly added call to fd_install(). It refers to dma_buf_fd() call that had been there all along, actually. dma_buf_fd() is get_unused_fd_flags() + fd_install(). The reason for splitting it in new variant and calling get_unused_fd_flags() and fd_install() separately is that it makes for simpler cleanup; we could use dma_buf_fd() instead - it would be a bit more clumsy, but that's it. The real issue is that drm_gem_prime_handle_to_fd() forces us to make the thing reachable via descriptor table; it's just what we need when all we are going to do is returning descriptor to userland, but it's inherently racy for internal uses - anything put into descriptor table, be it by fd_install() or by dma_buf_fd(), is fair game for all syscalls by other threads. > > Add drm_gem_prime_handle_to_dmabuf() - the "set dmabuf up" parts of > > drm_gem_prime_handle_to_fd() without the descriptor-related ones. > > Instead of inserting into descriptor table and returning the file > > descriptor it just returns the struct file. > > > > drm_gem_prime_handle_to_fd() becomes a wrapper for it. Other users > > will be introduced in the next commit. > > -int drm_gem_prime_handle_to_fd(struct drm_device *dev, > > +struct dma_buf *drm_gem_prime_handle_to_dmabuf(struct drm_device *dev, > > If it's exported it should have kernel docs. At least copy-paste the docs > from drm_gem_prime_handle_to_fd() > and reword a few bits. Point...
Do you have any problems with the variant below? The only changes are
in commit message and added comment for new helper...
commit 8c291056e3e88153ef4b6316d5247547da200757
Author: Al Viro <viro@zeniv.linux.org.uk>
Date: Fri Aug 2 09:56:28 2024 -0400
new helper: drm_gem_prime_handle_to_dmabuf()
Once something had been put into descriptor table, the only thing you
can do with it is returning descriptor to userland - you can't withdraw
it on subsequent failure exit, etc. You certainly can't count upon
it staying in the same slot of descriptor table - another thread
could've played with close(2)/dup2(2)/whatnot.
drm_gem_prime_handle_to_fd() creates a dmabuf, allocates a descriptor
and attaches dmabuf's file to it (the last two steps are done
in dma_buf_fd()). That's nice when all you are going to do is
passing a descriptor to userland. If you just need to work with the
resulting object or have something else to be done that might fail,
drm_gem_prime_handle_to_fd() is racy.
The problem is analogous to one with anon_inode_getfd(), and solution
is similar to what anon_inode_getfile() provides.
Add drm_gem_prime_handle_to_dmabuf() - the "set dmabuf up" parts of
drm_gem_prime_handle_to_fd() without the descriptor-related ones.
Instead of inserting into descriptor table and returning the file
descriptor it just returns the struct file.
drm_gem_prime_handle_to_fd() becomes a wrapper for it. Other users
will be introduced in the next commit.
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 03bd3c7bd0dc..0e3f8adf162f 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -410,22 +410,30 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev,
}
/**
- * drm_gem_prime_handle_to_fd - PRIME export function for GEM drivers
+ * drm_gem_prime_handle_to_dmabuf - PRIME export function for GEM drivers
* @dev: dev to export the buffer from
* @file_priv: drm file-private structure
* @handle: buffer handle to export
* @flags: flags like DRM_CLOEXEC
- * @prime_fd: pointer to storage for the fd id of the create dma-buf
*
* This is the PRIME export function which must be used mandatorily by GEM
* drivers to ensure correct lifetime management of the underlying GEM object.
* The actual exporting from GEM object to a dma-buf is done through the
* &drm_gem_object_funcs.export callback.
+ *
+ * Unlike drm_gem_prime_handle_to_fd(), it returns the struct dma_buf it
+ * has created, without attaching it to any file descriptors. The difference
+ * between those two is similar to that between anon_inode_getfile() and
+ * anon_inode_getfd(); insertion into descriptor table is something you
+ * can not revert if any cleanup is needed, so the descriptor-returning
+ * variants should only be used when you are past the last failure exit
+ * and the only thing left is passing the new file descriptor to userland.
+ * When all you need is the object itself or when you need to do something
+ * else that might fail, use that one instead.
*/
-int drm_gem_prime_handle_to_fd(struct drm_device *dev,
+struct dma_buf *drm_gem_prime_handle_to_dmabuf(struct drm_device *dev,
struct drm_file *file_priv, uint32_t handle,
- uint32_t flags,
- int *prime_fd)
+ uint32_t flags)
{
struct drm_gem_object *obj;
int ret = 0;
@@ -434,14 +442,14 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
mutex_lock(&file_priv->prime.lock);
obj = drm_gem_object_lookup(file_priv, handle);
if (!obj) {
- ret = -ENOENT;
+ dmabuf = ERR_PTR(-ENOENT);
goto out_unlock;
}
dmabuf = drm_prime_lookup_buf_by_handle(&file_priv->prime, handle);
if (dmabuf) {
get_dma_buf(dmabuf);
- goto out_have_handle;
+ goto out;
}
mutex_lock(&dev->object_name_lock);
@@ -463,7 +471,6 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
/* normally the created dma-buf takes ownership of the ref,
* but if that fails then drop the ref
*/
- ret = PTR_ERR(dmabuf);
mutex_unlock(&dev->object_name_lock);
goto out;
}
@@ -478,34 +485,51 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
ret = drm_prime_add_buf_handle(&file_priv->prime,
dmabuf, handle);
mutex_unlock(&dev->object_name_lock);
- if (ret)
- goto fail_put_dmabuf;
-
-out_have_handle:
- ret = dma_buf_fd(dmabuf, flags);
- /*
- * We must _not_ remove the buffer from the handle cache since the newly
- * created dma buf is already linked in the global obj->dma_buf pointer,
- * and that is invariant as long as a userspace gem handle exists.
- * Closing the handle will clean out the cache anyway, so we don't leak.
- */
- if (ret < 0) {
- goto fail_put_dmabuf;
- } else {
- *prime_fd = ret;
- ret = 0;
+ if (ret) {
+ dma_buf_put(dmabuf);
+ dmabuf = ERR_PTR(ret);
}
-
- goto out;
-
-fail_put_dmabuf:
- dma_buf_put(dmabuf);
out:
drm_gem_object_put(obj);
out_unlock:
mutex_unlock(&file_priv->prime.lock);
+ return dmabuf;
+}
+EXPORT_SYMBOL(drm_gem_prime_handle_to_dmabuf);
- return ret;
+/**
+ * drm_gem_prime_handle_to_fd - PRIME export function for GEM drivers
+ * @dev: dev to export the buffer from
+ * @file_priv: drm file-private structure
+ * @handle: buffer handle to export
+ * @flags: flags like DRM_CLOEXEC
+ * @prime_fd: pointer to storage for the fd id of the create dma-buf
+ *
+ * This is the PRIME export function which must be used mandatorily by GEM
+ * drivers to ensure correct lifetime management of the underlying GEM object.
+ * The actual exporting from GEM object to a dma-buf is done through the
+ * &drm_gem_object_funcs.export callback.
+ */
+int drm_gem_prime_handle_to_fd(struct drm_device *dev,
+ struct drm_file *file_priv, uint32_t handle,
+ uint32_t flags,
+ int *prime_fd)
+{
+ struct dma_buf *dmabuf;
+ int fd = get_unused_fd_flags(flags);
+
+ if (fd < 0)
+ return fd;
+
+ dmabuf = drm_gem_prime_handle_to_dmabuf(dev, file_priv, handle, flags);
+ if (IS_ERR(dmabuf)) {
+ put_unused_fd(fd);
+ return PTR_ERR(dmabuf);
+ }
+
+ fd_install(fd, dmabuf->file);
+ *prime_fd = fd;
+ return 0;
}
EXPORT_SYMBOL(drm_gem_prime_handle_to_fd);
diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h
index 2a1d01e5b56b..fa085c44d4ca 100644
--- a/include/drm/drm_prime.h
+++ b/include/drm/drm_prime.h
@@ -69,6 +69,9 @@ void drm_gem_dmabuf_release(struct dma_buf *dma_buf);
int drm_gem_prime_fd_to_handle(struct drm_device *dev,
struct drm_file *file_priv, int prime_fd, uint32_t *handle);
+struct dma_buf *drm_gem_prime_handle_to_dmabuf(struct drm_device *dev,
+ struct drm_file *file_priv, uint32_t handle,
+ uint32_t flags);
int drm_gem_prime_handle_to_fd(struct drm_device *dev,
struct drm_file *file_priv, uint32_t handle, uint32_t flags,
int *prime_fd);
Hi Am 23.08.24 um 03:57 schrieb Al Viro: > Do you have any problems with the variant below? The only changes are > in commit message and added comment for new helper... > > commit 8c291056e3e88153ef4b6316d5247547da200757 > Author: Al Viro <viro@zeniv.linux.org.uk> > Date: Fri Aug 2 09:56:28 2024 -0400 > > new helper: drm_gem_prime_handle_to_dmabuf() > > Once something had been put into descriptor table, the only thing you > can do with it is returning descriptor to userland - you can't withdraw > it on subsequent failure exit, etc. You certainly can't count upon > it staying in the same slot of descriptor table - another thread > could've played with close(2)/dup2(2)/whatnot. > > drm_gem_prime_handle_to_fd() creates a dmabuf, allocates a descriptor > and attaches dmabuf's file to it (the last two steps are done > in dma_buf_fd()). That's nice when all you are going to do is > passing a descriptor to userland. If you just need to work with the > resulting object or have something else to be done that might fail, > drm_gem_prime_handle_to_fd() is racy. > > The problem is analogous to one with anon_inode_getfd(), and solution > is similar to what anon_inode_getfile() provides. > > Add drm_gem_prime_handle_to_dmabuf() - the "set dmabuf up" parts of > drm_gem_prime_handle_to_fd() without the descriptor-related ones. > Instead of inserting into descriptor table and returning the file > descriptor it just returns the struct file. > > drm_gem_prime_handle_to_fd() becomes a wrapper for it. Other users > will be introduced in the next commit. > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> Acked-by: Thomas Zimmermann <tzimmermann@suse.de> Thank you so much. Best regards Thomas > > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c > index 03bd3c7bd0dc..0e3f8adf162f 100644 > --- a/drivers/gpu/drm/drm_prime.c > +++ b/drivers/gpu/drm/drm_prime.c > @@ -410,22 +410,30 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev, > } > > /** > - * drm_gem_prime_handle_to_fd - PRIME export function for GEM drivers > + * drm_gem_prime_handle_to_dmabuf - PRIME export function for GEM drivers > * @dev: dev to export the buffer from > * @file_priv: drm file-private structure > * @handle: buffer handle to export > * @flags: flags like DRM_CLOEXEC > - * @prime_fd: pointer to storage for the fd id of the create dma-buf > * > * This is the PRIME export function which must be used mandatorily by GEM > * drivers to ensure correct lifetime management of the underlying GEM object. > * The actual exporting from GEM object to a dma-buf is done through the > * &drm_gem_object_funcs.export callback. > + * > + * Unlike drm_gem_prime_handle_to_fd(), it returns the struct dma_buf it > + * has created, without attaching it to any file descriptors. The difference > + * between those two is similar to that between anon_inode_getfile() and > + * anon_inode_getfd(); insertion into descriptor table is something you > + * can not revert if any cleanup is needed, so the descriptor-returning > + * variants should only be used when you are past the last failure exit > + * and the only thing left is passing the new file descriptor to userland. > + * When all you need is the object itself or when you need to do something > + * else that might fail, use that one instead. > */ > -int drm_gem_prime_handle_to_fd(struct drm_device *dev, > +struct dma_buf *drm_gem_prime_handle_to_dmabuf(struct drm_device *dev, > struct drm_file *file_priv, uint32_t handle, > - uint32_t flags, > - int *prime_fd) > + uint32_t flags) > { > struct drm_gem_object *obj; > int ret = 0; > @@ -434,14 +442,14 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, > mutex_lock(&file_priv->prime.lock); > obj = drm_gem_object_lookup(file_priv, handle); > if (!obj) { > - ret = -ENOENT; > + dmabuf = ERR_PTR(-ENOENT); > goto out_unlock; > } > > dmabuf = drm_prime_lookup_buf_by_handle(&file_priv->prime, handle); > if (dmabuf) { > get_dma_buf(dmabuf); > - goto out_have_handle; > + goto out; > } > > mutex_lock(&dev->object_name_lock); > @@ -463,7 +471,6 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, > /* normally the created dma-buf takes ownership of the ref, > * but if that fails then drop the ref > */ > - ret = PTR_ERR(dmabuf); > mutex_unlock(&dev->object_name_lock); > goto out; > } > @@ -478,34 +485,51 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, > ret = drm_prime_add_buf_handle(&file_priv->prime, > dmabuf, handle); > mutex_unlock(&dev->object_name_lock); > - if (ret) > - goto fail_put_dmabuf; > - > -out_have_handle: > - ret = dma_buf_fd(dmabuf, flags); > - /* > - * We must _not_ remove the buffer from the handle cache since the newly > - * created dma buf is already linked in the global obj->dma_buf pointer, > - * and that is invariant as long as a userspace gem handle exists. > - * Closing the handle will clean out the cache anyway, so we don't leak. > - */ > - if (ret < 0) { > - goto fail_put_dmabuf; > - } else { > - *prime_fd = ret; > - ret = 0; > + if (ret) { > + dma_buf_put(dmabuf); > + dmabuf = ERR_PTR(ret); > } > - > - goto out; > - > -fail_put_dmabuf: > - dma_buf_put(dmabuf); > out: > drm_gem_object_put(obj); > out_unlock: > mutex_unlock(&file_priv->prime.lock); > + return dmabuf; > +} > +EXPORT_SYMBOL(drm_gem_prime_handle_to_dmabuf); > > - return ret; > +/** > + * drm_gem_prime_handle_to_fd - PRIME export function for GEM drivers > + * @dev: dev to export the buffer from > + * @file_priv: drm file-private structure > + * @handle: buffer handle to export > + * @flags: flags like DRM_CLOEXEC > + * @prime_fd: pointer to storage for the fd id of the create dma-buf > + * > + * This is the PRIME export function which must be used mandatorily by GEM > + * drivers to ensure correct lifetime management of the underlying GEM object. > + * The actual exporting from GEM object to a dma-buf is done through the > + * &drm_gem_object_funcs.export callback. > + */ > +int drm_gem_prime_handle_to_fd(struct drm_device *dev, > + struct drm_file *file_priv, uint32_t handle, > + uint32_t flags, > + int *prime_fd) > +{ > + struct dma_buf *dmabuf; > + int fd = get_unused_fd_flags(flags); > + > + if (fd < 0) > + return fd; > + > + dmabuf = drm_gem_prime_handle_to_dmabuf(dev, file_priv, handle, flags); > + if (IS_ERR(dmabuf)) { > + put_unused_fd(fd); > + return PTR_ERR(dmabuf); > + } > + > + fd_install(fd, dmabuf->file); > + *prime_fd = fd; > + return 0; > } > EXPORT_SYMBOL(drm_gem_prime_handle_to_fd); > > diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h > index 2a1d01e5b56b..fa085c44d4ca 100644 > --- a/include/drm/drm_prime.h > +++ b/include/drm/drm_prime.h > @@ -69,6 +69,9 @@ void drm_gem_dmabuf_release(struct dma_buf *dma_buf); > > int drm_gem_prime_fd_to_handle(struct drm_device *dev, > struct drm_file *file_priv, int prime_fd, uint32_t *handle); > +struct dma_buf *drm_gem_prime_handle_to_dmabuf(struct drm_device *dev, > + struct drm_file *file_priv, uint32_t handle, > + uint32_t flags); > int drm_gem_prime_handle_to_fd(struct drm_device *dev, > struct drm_file *file_priv, uint32_t handle, uint32_t flags, > int *prime_fd);
On Fri, Aug 23, 2024 at 09:21:14AM +0200, Thomas Zimmermann wrote: > Acked-by: Thomas Zimmermann <tzimmermann@suse.de> > > Thank you so much. OK, Acked-by added, branch force-pushed to git://git.kernel.org:/pub/scm/linux/kernel/git/viro/vfs.git #for-drm In case if anybody wants a signed pull request, see below; or just cherry-pick, etc. - entirely up to drm and amd folks... The following changes since commit 8400291e289ee6b2bf9779ff1c83a291501f017b: Linux 6.11-rc1 (2024-07-28 14:19:55 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git tags/pull-for-drm for you to fetch changes up to 30581926c42d1886cce2a04dcf615f551d829814: amdgpu: get rid of bogus includes of fdtable.h (2024-08-23 03:46:46 -0400) ---------------------------------------------------------------- get rid of racy manipulations of descriptor table in amdgpu ---------------------------------------------------------------- Al Viro (4): new helper: drm_gem_prime_handle_to_dmabuf() amdgpu: fix a race in kfd_mem_export_dmabuf() amdkfd CRIU fixes amdgpu: get rid of bogus includes of fdtable.h .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c | 1 - drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 12 +--- drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c | 1 - drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 64 ++++++++++++----- drivers/gpu/drm/drm_prime.c | 84 ++++++++++++++-------- include/drm/drm_prime.h | 3 + 6 files changed, 106 insertions(+), 59 deletions(-)
Thanks. I cherry-picked these to my tree. Sorry for the delay. Alex On Fri, Aug 23, 2024 at 3:53 AM Al Viro <viro@zeniv.linux.org.uk> wrote: > > On Fri, Aug 23, 2024 at 09:21:14AM +0200, Thomas Zimmermann wrote: > > > Acked-by: Thomas Zimmermann <tzimmermann@suse.de> > > > > Thank you so much. > > OK, Acked-by added, branch force-pushed to > git://git.kernel.org:/pub/scm/linux/kernel/git/viro/vfs.git #for-drm > > In case if anybody wants a signed pull request, see below; or just > cherry-pick, etc. - entirely up to drm and amd folks... > > The following changes since commit 8400291e289ee6b2bf9779ff1c83a291501f017b: > > Linux 6.11-rc1 (2024-07-28 14:19:55 -0700) > > are available in the Git repository at: > > git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git tags/pull-for-drm > > for you to fetch changes up to 30581926c42d1886cce2a04dcf615f551d829814: > > amdgpu: get rid of bogus includes of fdtable.h (2024-08-23 03:46:46 -0400) > > ---------------------------------------------------------------- > get rid of racy manipulations of descriptor table in amdgpu > > ---------------------------------------------------------------- > Al Viro (4): > new helper: drm_gem_prime_handle_to_dmabuf() > amdgpu: fix a race in kfd_mem_export_dmabuf() > amdkfd CRIU fixes > amdgpu: get rid of bogus includes of fdtable.h > > .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c | 1 - > drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 12 +--- > drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c | 1 - > drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 64 ++++++++++++----- > drivers/gpu/drm/drm_prime.c | 84 ++++++++++++++-------- > include/drm/drm_prime.h | 3 + > 6 files changed, 106 insertions(+), 59 deletions(-)
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 03bd3c7bd0dc..467c7a278ad3 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -409,23 +409,9 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev, return dmabuf; } -/** - * drm_gem_prime_handle_to_fd - PRIME export function for GEM drivers - * @dev: dev to export the buffer from - * @file_priv: drm file-private structure - * @handle: buffer handle to export - * @flags: flags like DRM_CLOEXEC - * @prime_fd: pointer to storage for the fd id of the create dma-buf - * - * This is the PRIME export function which must be used mandatorily by GEM - * drivers to ensure correct lifetime management of the underlying GEM object. - * The actual exporting from GEM object to a dma-buf is done through the - * &drm_gem_object_funcs.export callback. - */ -int drm_gem_prime_handle_to_fd(struct drm_device *dev, +struct dma_buf *drm_gem_prime_handle_to_dmabuf(struct drm_device *dev, struct drm_file *file_priv, uint32_t handle, - uint32_t flags, - int *prime_fd) + uint32_t flags) { struct drm_gem_object *obj; int ret = 0; @@ -434,14 +420,14 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, mutex_lock(&file_priv->prime.lock); obj = drm_gem_object_lookup(file_priv, handle); if (!obj) { - ret = -ENOENT; + dmabuf = ERR_PTR(-ENOENT); goto out_unlock; } dmabuf = drm_prime_lookup_buf_by_handle(&file_priv->prime, handle); if (dmabuf) { get_dma_buf(dmabuf); - goto out_have_handle; + goto out; } mutex_lock(&dev->object_name_lock); @@ -463,7 +449,6 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, /* normally the created dma-buf takes ownership of the ref, * but if that fails then drop the ref */ - ret = PTR_ERR(dmabuf); mutex_unlock(&dev->object_name_lock); goto out; } @@ -478,34 +463,51 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, ret = drm_prime_add_buf_handle(&file_priv->prime, dmabuf, handle); mutex_unlock(&dev->object_name_lock); - if (ret) - goto fail_put_dmabuf; - -out_have_handle: - ret = dma_buf_fd(dmabuf, flags); - /* - * We must _not_ remove the buffer from the handle cache since the newly - * created dma buf is already linked in the global obj->dma_buf pointer, - * and that is invariant as long as a userspace gem handle exists. - * Closing the handle will clean out the cache anyway, so we don't leak. - */ - if (ret < 0) { - goto fail_put_dmabuf; - } else { - *prime_fd = ret; - ret = 0; + if (ret) { + dma_buf_put(dmabuf); + dmabuf = ERR_PTR(ret); } - - goto out; - -fail_put_dmabuf: - dma_buf_put(dmabuf); out: drm_gem_object_put(obj); out_unlock: mutex_unlock(&file_priv->prime.lock); + return dmabuf; +} +EXPORT_SYMBOL(drm_gem_prime_handle_to_dmabuf); - return ret; +/** + * drm_gem_prime_handle_to_fd - PRIME export function for GEM drivers + * @dev: dev to export the buffer from + * @file_priv: drm file-private structure + * @handle: buffer handle to export + * @flags: flags like DRM_CLOEXEC + * @prime_fd: pointer to storage for the fd id of the create dma-buf + * + * This is the PRIME export function which must be used mandatorily by GEM + * drivers to ensure correct lifetime management of the underlying GEM object. + * The actual exporting from GEM object to a dma-buf is done through the + * &drm_gem_object_funcs.export callback. + */ +int drm_gem_prime_handle_to_fd(struct drm_device *dev, + struct drm_file *file_priv, uint32_t handle, + uint32_t flags, + int *prime_fd) +{ + struct dma_buf *dmabuf; + int fd = get_unused_fd_flags(flags); + + if (fd < 0) + return fd; + + dmabuf = drm_gem_prime_handle_to_dmabuf(dev, file_priv, handle, flags); + if (IS_ERR(dmabuf)) { + put_unused_fd(fd); + return PTR_ERR(dmabuf); + } + + fd_install(fd, dmabuf->file); + *prime_fd = fd; + return 0; } EXPORT_SYMBOL(drm_gem_prime_handle_to_fd); diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h index 2a1d01e5b56b..fa085c44d4ca 100644 --- a/include/drm/drm_prime.h +++ b/include/drm/drm_prime.h @@ -69,6 +69,9 @@ void drm_gem_dmabuf_release(struct dma_buf *dma_buf); int drm_gem_prime_fd_to_handle(struct drm_device *dev, struct drm_file *file_priv, int prime_fd, uint32_t *handle); +struct dma_buf *drm_gem_prime_handle_to_dmabuf(struct drm_device *dev, + struct drm_file *file_priv, uint32_t handle, + uint32_t flags); int drm_gem_prime_handle_to_fd(struct drm_device *dev, struct drm_file *file_priv, uint32_t handle, uint32_t flags, int *prime_fd);
Once something had been put into descriptor table, the only thing you can do with it is returning descriptor to userland - you can't withdraw it on subsequent failure exit, etc. You certainly can't count upon it staying in the same slot of descriptor table - another thread could've played with close(2)/dup2(2)/whatnot. Add drm_gem_prime_handle_to_dmabuf() - the "set dmabuf up" parts of drm_gem_prime_handle_to_fd() without the descriptor-related ones. Instead of inserting into descriptor table and returning the file descriptor it just returns the struct file. drm_gem_prime_handle_to_fd() becomes a wrapper for it. Other users will be introduced in the next commit. Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- drivers/gpu/drm/drm_prime.c | 84 +++++++++++++++++++------------------ include/drm/drm_prime.h | 3 ++ 2 files changed, 46 insertions(+), 41 deletions(-)