[Linaro-mm-sig] Re: [PATCH 1/3] udmabuf: Keep track current device mappings

On 1/24/24 4:36 PM, Kasireddy, Vivek wrote:

Hi Andrew,

...

When a device attaches to and maps our buffer we need to keep track of this mapping/device. This is needed for synchronization with these devices when beginning and ending CPU access for instance. Add a list that tracks device mappings as part of {map,unmap}_udmabuf().

Signed-off-by: Andrew Davis afd@ti.com

drivers/dma-buf/udmabuf.c | 43 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 41 insertions(+), 2 deletions(-)

diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c index c406459996489..3a23f0a7d112a 100644 --- a/drivers/dma-buf/udmabuf.c +++ b/drivers/dma-buf/udmabuf.c @@ -28,6 +28,14 @@ struct udmabuf { struct page **pages; struct sg_table *sg; struct miscdevice *device;

• struct list_head attachments;
• struct mutex lock;

+};

+struct udmabuf_attachment {

• struct device *dev;

• struct sg_table *table;

• struct list_head list; };

  static vm_fault_t udmabuf_vm_fault(struct vm_fault *vmf)

@@ -120,14 +128,42 @@ static void put_sg_table(struct device *dev, struct sg_table *sg, static struct sg_table *map_udmabuf(struct dma_buf_attachment *at, enum dma_data_direction direction) {

• return get_sg_table(at->dev, at->dmabuf, direction);

• struct udmabuf *ubuf = at->dmabuf->priv;
• struct udmabuf_attachment *a;
• a = kzalloc(sizeof(*a), GFP_KERNEL);
• if (!a)

• return ERR_PTR(-ENOMEM);
  

• a->table = get_sg_table(at->dev, at->dmabuf, direction);
• if (IS_ERR(a->table)) {

• kfree(a);
  

• return a->table;
  

Isn't that a use-after-free bug?

Indeed it is, will fix.

Seems coccicheck also caught this but I missed it when reviewing its output, my bad :(

Andrew

Rest of the patch lgtm.

Thanks, Vivek

...

• }

• a->dev = at->dev;

• mutex_lock(&ubuf->lock);

• list_add(&a->list, &ubuf->attachments);

• mutex_unlock(&ubuf->lock);

• return a->table; }

  static void unmap_udmabuf(struct dma_buf_attachment *at, struct sg_table *sg, enum dma_data_direction direction) {

• return put_sg_table(at->dev, sg, direction);

• struct udmabuf_attachment *a = at->priv;

• struct udmabuf *ubuf = at->dmabuf->priv;

• mutex_lock(&ubuf->lock);

• list_del(&a->list);

• mutex_unlock(&ubuf->lock);

• put_sg_table(at->dev, sg, direction);

• kfree(a); }

  static void release_udmabuf(struct dma_buf *buf)

@@ -263,6 +299,9 @@ static long udmabuf_create(struct miscdevice *device, memfd = NULL; }

• INIT_LIST_HEAD(&ubuf->attachments);
• mutex_init(&ubuf->lock);
• exp_info.ops = &udmabuf_ops; exp_info.size = ubuf->pagecount << PAGE_SHIFT; exp_info.priv = ubuf;

-- 2.39.2