[Linaro-mm-sig] Re: [PATCH] dma-buf: fix dma_buf_export init order

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