Thanks Christian for this nice cleanup!!
On 12/6/2022 8:42 PM, Christian König wrote:
> @@ -638,10 +630,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 +656,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);
This fput() still endup in calling the dma_buf_file_release() where
there are no checks before accessing the file->private_data(=dmabuf)
which can result in null pointer access. Am I missing something trivial?
> err_module:
> module_put(exp_info->owner);
> return ERR_PTR(ret);
> -- 2.34.1
