| Message ID | 20240123221227.868341-1-afd@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] udmabuf: Keep track current device mappings | expand |
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? 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
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 >
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; + } + + 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;
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(-)