[PATCH] dma-buf: WARN on dmabuf release with pending attachments

[PATCH] dma-buf: WARN on dmabuf release with pending attachments

[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@codeaurora.org>
---
 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);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of the Code Aurora Forum, hosted by The Linux Foundation

[PATCH] dma-buf: WARN on dmabuf release with pending attachments

[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@codeaurora.org>
---
 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);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of the Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH] dma-buf: WARN on dmabuf release with pending attachments

[Christian König]

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@codeaurora.org>

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@amd.com> and I'm 
going to push this to drm-misc-next on Monday if nobody objects.

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);

Re: [PATCH] dma-buf: WARN on dmabuf release with pending attachments

[Christian König]

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@codeaurora.org>

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@amd.com> and I'm 
going to push this to drm-misc-next on Monday if nobody objects.

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);

Re: [PATCH] dma-buf: WARN on dmabuf release with pending attachments

[Daniel Vetter]

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@codeaurora.org>
> 
> 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@amd.com> 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@ffwll.ch>
> 
> 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

Re: [PATCH] dma-buf: WARN on dmabuf release with pending attachments

[Daniel Vetter]

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@codeaurora.org>
> 
> 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@amd.com> 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@ffwll.ch>
> 
> 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