RE: [PATCH] dma-buf: fix dma_buf_export init order v2

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



>-----Original Message-----
>From: dri-devel <dri-devel-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of
>Christian König
>Sent: Friday, December 9, 2022 2:16 AM
>To: quic_charante@xxxxxxxxxxx; cuigaosheng1@xxxxxxxxxx;
>sumit.semwal@xxxxxxxxxx
>Cc: linaro-mm-sig@xxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; linux-
>media@xxxxxxxxxxxxxxx
>Subject: [PATCH] dma-buf: fix dma_buf_export init order v2
>
>The init order and resulting error handling in dma_buf_export
>was pretty messy.
>
>Subordinate objects like the file and the sysfs kernel objects
>were initializing and wiring itself up with the object in the
>wrong order resulting not only in complicating and partially
>incorrect error handling, but also in publishing only halve
>initialized DMA-buf objects.
>
>Clean this up thoughtfully by allocating the file independent
>of the DMA-buf object. Then allocate and initialize the DMA-buf
>object itself, before publishing it through sysfs. If everything
>works as expected the file is then connected with the DMA-buf
>object and publish it through debugfs.
>
>Also adds the missing dma_resv_fini() into the error handling.
>
>v2: add some missing changes to dma_bug_getfile() and a missing NULL
>    check in dma_buf_file_release()

Looks good.

Reviewed-by: Michael J. Ruhl <michael.j.ruhl@xxxxxxxxx>

Mike

>Signed-off-by: Christian König <christian.koenig@xxxxxxx>
>---
> drivers/dma-buf/dma-buf-sysfs-stats.c |  7 +--
> drivers/dma-buf/dma-buf-sysfs-stats.h |  4 +-
> drivers/dma-buf/dma-buf.c             | 84 +++++++++++++--------------
> 3 files changed, 43 insertions(+), 52 deletions(-)
>
>diff --git a/drivers/dma-buf/dma-buf-sysfs-stats.c b/drivers/dma-buf/dma-
>buf-sysfs-stats.c
>index 2bba0babcb62..4b680e10c15a 100644
>--- a/drivers/dma-buf/dma-buf-sysfs-stats.c
>+++ b/drivers/dma-buf/dma-buf-sysfs-stats.c
>@@ -168,14 +168,11 @@ void dma_buf_uninit_sysfs_statistics(void)
> 	kset_unregister(dma_buf_stats_kset);
> }
>
>-int dma_buf_stats_setup(struct dma_buf *dmabuf)
>+int dma_buf_stats_setup(struct dma_buf *dmabuf, struct file *file)
> {
> 	struct dma_buf_sysfs_entry *sysfs_entry;
> 	int ret;
>
>-	if (!dmabuf || !dmabuf->file)
>-		return -EINVAL;
>-
> 	if (!dmabuf->exp_name) {
> 		pr_err("exporter name must not be empty if stats
>needed\n");
> 		return -EINVAL;
>@@ -192,7 +189,7 @@ int dma_buf_stats_setup(struct dma_buf *dmabuf)
>
> 	/* create the directory for buffer stats */
> 	ret = kobject_init_and_add(&sysfs_entry->kobj, &dma_buf_ktype,
>NULL,
>-				   "%lu", file_inode(dmabuf->file)->i_ino);
>+				   "%lu", file_inode(file)->i_ino);
> 	if (ret)
> 		goto err_sysfs_dmabuf;
>
>diff --git a/drivers/dma-buf/dma-buf-sysfs-stats.h b/drivers/dma-buf/dma-
>buf-sysfs-stats.h
>index a49c6e2650cc..7a8a995b75ba 100644
>--- a/drivers/dma-buf/dma-buf-sysfs-stats.h
>+++ b/drivers/dma-buf/dma-buf-sysfs-stats.h
>@@ -13,7 +13,7 @@
> int dma_buf_init_sysfs_statistics(void);
> void dma_buf_uninit_sysfs_statistics(void);
>
>-int dma_buf_stats_setup(struct dma_buf *dmabuf);
>+int dma_buf_stats_setup(struct dma_buf *dmabuf, struct file *file);
>
> void dma_buf_stats_teardown(struct dma_buf *dmabuf);
> #else
>@@ -25,7 +25,7 @@ static inline int dma_buf_init_sysfs_statistics(void)
>
> static inline void dma_buf_uninit_sysfs_statistics(void) {}
>
>-static inline int dma_buf_stats_setup(struct dma_buf *dmabuf)
>+static inline int dma_buf_stats_setup(struct dma_buf *dmabuf, struct file
>*file)
> {
> 	return 0;
> }
>diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>index e6f36c014c4c..eb6b59363c4f 100644
>--- a/drivers/dma-buf/dma-buf.c
>+++ b/drivers/dma-buf/dma-buf.c
>@@ -95,10 +95,11 @@ static int dma_buf_file_release(struct inode *inode,
>struct file *file)
> 		return -EINVAL;
>
> 	dmabuf = file->private_data;
>-
>-	mutex_lock(&db_list.lock);
>-	list_del(&dmabuf->list_node);
>-	mutex_unlock(&db_list.lock);
>+	if (dmabuf) {
>+		mutex_lock(&db_list.lock);
>+		list_del(&dmabuf->list_node);
>+		mutex_unlock(&db_list.lock);
>+	}
>
> 	return 0;
> }
>@@ -523,17 +524,17 @@ static inline int is_dma_buf_file(struct file *file)
> 	return file->f_op == &dma_buf_fops;
> }
>
>-static struct file *dma_buf_getfile(struct dma_buf *dmabuf, int flags)
>+static struct file *dma_buf_getfile(size_t size, int flags)
> {
> 	static atomic64_t dmabuf_inode = ATOMIC64_INIT(0);
>-	struct file *file;
> 	struct inode *inode = alloc_anon_inode(dma_buf_mnt->mnt_sb);
>+	struct file *file;
>
> 	if (IS_ERR(inode))
> 		return ERR_CAST(inode);
>
>-	inode->i_size = dmabuf->size;
>-	inode_set_bytes(inode, dmabuf->size);
>+	inode->i_size = size;
>+	inode_set_bytes(inode, size);
>
> 	/*
> 	 * The ->i_ino acquired from get_next_ino() is not unique thus
>@@ -547,8 +548,6 @@ static struct file *dma_buf_getfile(struct dma_buf
>*dmabuf, int flags)
> 				 flags, &dma_buf_fops);
> 	if (IS_ERR(file))
> 		goto err_alloc_file;
>-	file->private_data = dmabuf;
>-	file->f_path.dentry->d_fsdata = dmabuf;
>
> 	return file;
>
>@@ -614,19 +613,11 @@ struct dma_buf *dma_buf_export(const struct
>dma_buf_export_info *exp_info)
> 	size_t alloc_size = sizeof(struct dma_buf);
> 	int ret;
>
>-	if (!exp_info->resv)
>-		alloc_size += sizeof(struct dma_resv);
>-	else
>-		/* prevent &dma_buf[1] == dma_buf->resv */
>-		alloc_size += 1;
>-
>-	if (WARN_ON(!exp_info->priv
>-			  || !exp_info->ops
>-			  || !exp_info->ops->map_dma_buf
>-			  || !exp_info->ops->unmap_dma_buf
>-			  || !exp_info->ops->release)) {
>+	if (WARN_ON(!exp_info->priv || !exp_info->ops
>+		    || !exp_info->ops->map_dma_buf
>+		    || !exp_info->ops->unmap_dma_buf
>+		    || !exp_info->ops->release))
> 		return ERR_PTR(-EINVAL);
>-	}
>
> 	if (WARN_ON(exp_info->ops->cache_sgt_mapping &&
> 		    (exp_info->ops->pin || exp_info->ops->unpin)))
>@@ -638,10 +629,21 @@ struct dma_buf *dma_buf_export(const struct
>dma_buf_export_info *exp_info)
> 	if (!try_module_get(exp_info->owner))
> 		return ERR_PTR(-ENOENT);
>
>+	file = dma_buf_getfile(exp_info->size, exp_info->flags);
>+	if (IS_ERR(file)) {
>+		ret = PTR_ERR(file);
>+		goto err_module;
>+	}
>+
>+	if (!exp_info->resv)
>+		alloc_size += sizeof(struct dma_resv);
>+	else
>+		/* prevent &dma_buf[1] == dma_buf->resv */
>+		alloc_size += 1;
> 	dmabuf = kzalloc(alloc_size, GFP_KERNEL);
> 	if (!dmabuf) {
> 		ret = -ENOMEM;
>-		goto err_module;
>+		goto err_file;
> 	}
>
> 	dmabuf->priv = exp_info->priv;
>@@ -653,44 +655,36 @@ struct dma_buf *dma_buf_export(const struct
>dma_buf_export_info *exp_info)
> 	init_waitqueue_head(&dmabuf->poll);
> 	dmabuf->cb_in.poll = dmabuf->cb_out.poll = &dmabuf->poll;
> 	dmabuf->cb_in.active = dmabuf->cb_out.active = 0;
>+	mutex_init(&dmabuf->lock);
>+	INIT_LIST_HEAD(&dmabuf->attachments);
>
> 	if (!resv) {
>-		resv = (struct dma_resv *)&dmabuf[1];
>-		dma_resv_init(resv);
>+		dmabuf->resv = (struct dma_resv *)&dmabuf[1];
>+		dma_resv_init(dmabuf->resv);
>+	} else {
>+		dmabuf->resv = resv;
> 	}
>-	dmabuf->resv = resv;
>
>-	file = dma_buf_getfile(dmabuf, exp_info->flags);
>-	if (IS_ERR(file)) {
>-		ret = PTR_ERR(file);
>+	ret = dma_buf_stats_setup(dmabuf, file);
>+	if (ret)
> 		goto err_dmabuf;
>-	}
>
>+	file->private_data = dmabuf;
>+	file->f_path.dentry->d_fsdata = dmabuf;
> 	dmabuf->file = file;
>
>-	mutex_init(&dmabuf->lock);
>-	INIT_LIST_HEAD(&dmabuf->attachments);
>-
> 	mutex_lock(&db_list.lock);
> 	list_add(&dmabuf->list_node, &db_list.head);
> 	mutex_unlock(&db_list.lock);
>
>-	ret = dma_buf_stats_setup(dmabuf);
>-	if (ret)
>-		goto err_sysfs;
>-
> 	return dmabuf;
>
>-err_sysfs:
>-	/*
>-	 * Set file->f_path.dentry->d_fsdata to NULL so that when
>-	 * dma_buf_release() gets invoked by dentry_ops, it exits
>-	 * early before calling the release() dma_buf op.
>-	 */
>-	file->f_path.dentry->d_fsdata = NULL;
>-	fput(file);
> err_dmabuf:
>+	if (!resv)
>+		dma_resv_fini(dmabuf->resv);
> 	kfree(dmabuf);
>+err_file:
>+	fput(file);
> err_module:
> 	module_put(exp_info->owner);
> 	return ERR_PTR(ret);
>--
>2.34.1





[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux