On Fri, Jul 23, 2021 at 02:34:13PM +0200, Christian König wrote: > Am 23.07.21 um 14:31 schrieb Charan Teja Reddy: > > It is expected from the clients to follow the below steps on an imported > > dmabuf fd: > > a) dmabuf = dma_buf_get(fd) // Get the dmabuf from fd > > b) dma_buf_attach(dmabuf); // Clients attach to the dmabuf > > o Here the kernel does some slab allocations, say for > > dma_buf_attachment and may be some other slab allocation in the > > dmabuf->ops->attach(). > > c) Client may need to do dma_buf_map_attachment(). > > d) Accordingly dma_buf_unmap_attachment() should be called. > > e) dma_buf_detach () // Clients detach to the dmabuf. > > o Here the slab allocations made in b) are freed. > > f) dma_buf_put(dmabuf) // Can free the dmabuf if it is the last > > reference. > > > > Now say an erroneous client failed at step c) above thus it directly > > called dma_buf_put(), step f) above. Considering that it may be the last > > reference to the dmabuf, buffer will be freed with pending attachments > > left to the dmabuf which can show up as the 'memory leak'. This should > > at least be reported as the WARN(). > > > > Signed-off-by: Charan Teja Reddy <charante@xxxxxxxxxxxxxx> > > Good idea. I would expect a crash immediately, but from such a backtrace it > is quite hard to tell what the problem is. > > Patch is Reviewed-by: Christian König <christian.koenig@xxxxxxx> and I'm > going to push this to drm-misc-next on Monday if nobody objects. The boom only happens a lot later when the offending import uses the attachment again. This here has a good chance to catch that early drm_buf_put(), so I think it's a good improvement. We'll still Oops later on ofc, but meh. Acked-by: Daniel Vetter <daniel.vetter@xxxxxxxx> > > Thanks, > Christian. > > > --- > > drivers/dma-buf/dma-buf.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > > index 511fe0d..733c8b1 100644 > > --- a/drivers/dma-buf/dma-buf.c > > +++ b/drivers/dma-buf/dma-buf.c > > @@ -79,6 +79,7 @@ static void dma_buf_release(struct dentry *dentry) > > if (dmabuf->resv == (struct dma_resv *)&dmabuf[1]) > > dma_resv_fini(dmabuf->resv); > > + WARN_ON(!list_empty(&dmabuf->attachments)); > > module_put(dmabuf->owner); > > kfree(dmabuf->name); > > kfree(dmabuf); > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
