mbox series

[0/2] dma-mapping, remoteproc: Fix dma_mem leak after rproc_shutdown

Message ID 20220422062436.14384-1-mark-pk.tsai@mediatek.com (mailing list archive)
Headers show
Series dma-mapping, remoteproc: Fix dma_mem leak after rproc_shutdown | expand

Message

Mark-PK Tsai (蔡沛剛) April 22, 2022, 6:24 a.m. UTC
Release dma coherent memory before rvdev is free in
rproc_rvdev_release().

Below is the kmemleak report:
unreferenced object 0xffffff8051c1a980 (size 128):
  comm "sh", pid 4895, jiffies 4295026604 (age 15481.896s)
  hex dump (first 32 bytes):
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<000000003a0f3ec0>] dma_declare_coherent_memory+0x44/0x11c
    [<00000000ad243164>] rproc_add_virtio_dev+0xb8/0x20c
    [<00000000d219c8e9>] rproc_vdev_do_start+0x18/0x24
    [<00000000e694b468>] rproc_start+0x22c/0x3e0
    [<000000000b938941>] rproc_boot+0x4a4/0x860
    [<000000003c4dc532>] state_store.52856+0x10c/0x1b8
    [<00000000df2297ac>] dev_attr_store+0x34/0x84
    [<0000000083a53bdb>] sysfs_kf_write+0x60/0xbc
    [<000000008ed830df>] kernfs_fop_write+0x198/0x458
    [<0000000072b9ad06>] __vfs_write+0x50/0x210
    [<00000000377d7469>] vfs_write+0xe4/0x1a8
    [<00000000c3fc594e>] ksys_write+0x78/0x144
    [<000000009aef6f4b>] __arm64_sys_write+0x1c/0x28
    [<0000000003496a98>] el0_svc_common+0xc8/0x22c
    [<00000000ea3fe7a3>] el0_svc_compat_handler+0x1c/0x28
    [<00000000d1a85a4e>] el0_svc_compat+0x8/0x24

Mark-PK Tsai (2):
  dma-mapping: Add dma_release_coherent_memory to DMA API
  remoteproc: Fix dma_mem leak after rproc_shutdown

 drivers/remoteproc/remoteproc_core.c |  1 +
 include/linux/dma-map-ops.h          |  3 +++
 kernel/dma/coherent.c                | 10 ++++++++--
 3 files changed, 12 insertions(+), 2 deletions(-)

Comments

Christoph Hellwig April 23, 2022, 5:46 p.m. UTC | #1
Sigh.  In theory drivers should never declare coherent memory like
this, and there has been some work to fix remoteproc in that regard.

But I guess until that is merged we'll need somthing like this fix.
Mark-PK Tsai (蔡沛剛) May 23, 2022, 10:15 a.m. UTC | #2
> Sigh.  In theory drivers should never declare coherent memory like
> this, and there has been some work to fix remoteproc in that regard.
>
> But I guess until that is merged we'll need somthing like this fix.

Hi,

Thanks for your comment.
As I didn't see other fix of this issue, should we use this patch
before the remoteproc work you mentioned is merged?
Robin Murphy May 23, 2022, 10:24 a.m. UTC | #3
On 2022-05-23 11:15, Mark-PK Tsai wrote:
>> Sigh.  In theory drivers should never declare coherent memory like
>> this, and there has been some work to fix remoteproc in that regard.
>>
>> But I guess until that is merged we'll need somthing like this fix.
> 
> Hi,
> 
> Thanks for your comment.
> As I didn't see other fix of this issue, should we use this patch
> before the remoteproc work you mentioned is merged?

TBH I think it would be better "fixed" with a kmemleak_ignore() and a 
big comment, rather than adding API cruft for something that isn't a 
real problem. I'm quite sure that no real-world user is unbinding 
remoteproc drivers frequently enough that leaking a 48-byte allocation 
each time has any practical significance.

Thanks,
Robin.
Mark-PK Tsai (蔡沛剛) May 23, 2022, 12:27 p.m. UTC | #4
> >> Sigh.  In theory drivers should never declare coherent memory like
> >> this, and there has been some work to fix remoteproc in that regard.
> >>
> >> But I guess until that is merged we'll need somthing like this fix.
> > 
> > Hi,
> > 
> > Thanks for your comment.
> > As I didn't see other fix of this issue, should we use this patch
> > before the remoteproc work you mentioned is merged?
> 
> TBH I think it would be better "fixed" with a kmemleak_ignore() and a 
> big comment, rather than adding API cruft for something that isn't a 
> real problem. I'm quite sure that no real-world user is unbinding 
> remoteproc drivers frequently enough that leaking a 48-byte allocation 
> each time has any practical significance.
> 

Actually we stop 2 remote processors using the same remoteproc driver
when system suspend or screen off on our arm64 platform.
And the 48-byte slab allocation will be aligned up to 128 bytes on arm64.
So the leak can be significant in our use case especially
in stress test...

We really don't want to ignore it. Maybe there're other way to fix
it without adding APIs?
Mathieu Poirier June 22, 2022, 4:25 p.m. UTC | #5
On Sat, Apr 23, 2022 at 07:46:50PM +0200, Christoph Hellwig wrote:
> Sigh.  In theory drivers should never declare coherent memory like
> this, and there has been some work to fix remoteproc in that regard.
> 
> But I guess until that is merged we'll need somthing like this fix.

Should I take this in the remoteproc tree?  If so, can I get an RB?

Thanks,
Mathieu
Christoph Hellwig June 23, 2022, 5:42 a.m. UTC | #6
On Wed, Jun 22, 2022 at 10:25:40AM -0600, Mathieu Poirier wrote:
> On Sat, Apr 23, 2022 at 07:46:50PM +0200, Christoph Hellwig wrote:
> > Sigh.  In theory drivers should never declare coherent memory like
> > this, and there has been some work to fix remoteproc in that regard.
> > 
> > But I guess until that is merged we'll need somthing like this fix.
> 
> Should I take this in the remoteproc tree?  If so, can I get an RB?

Reluctantly-Acked-by: Christoph Hellwig <hch@lst.de>
Mathieu Poirier June 24, 2022, 4:48 p.m. UTC | #7
On Fri, Apr 22, 2022 at 02:24:34PM +0800, Mark-PK Tsai wrote:
> Release dma coherent memory before rvdev is free in
> rproc_rvdev_release().
> 
> Below is the kmemleak report:
> unreferenced object 0xffffff8051c1a980 (size 128):
>   comm "sh", pid 4895, jiffies 4295026604 (age 15481.896s)
>   hex dump (first 32 bytes):
>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>   backtrace:
>     [<000000003a0f3ec0>] dma_declare_coherent_memory+0x44/0x11c
>     [<00000000ad243164>] rproc_add_virtio_dev+0xb8/0x20c
>     [<00000000d219c8e9>] rproc_vdev_do_start+0x18/0x24
>     [<00000000e694b468>] rproc_start+0x22c/0x3e0
>     [<000000000b938941>] rproc_boot+0x4a4/0x860
>     [<000000003c4dc532>] state_store.52856+0x10c/0x1b8
>     [<00000000df2297ac>] dev_attr_store+0x34/0x84
>     [<0000000083a53bdb>] sysfs_kf_write+0x60/0xbc
>     [<000000008ed830df>] kernfs_fop_write+0x198/0x458
>     [<0000000072b9ad06>] __vfs_write+0x50/0x210
>     [<00000000377d7469>] vfs_write+0xe4/0x1a8
>     [<00000000c3fc594e>] ksys_write+0x78/0x144
>     [<000000009aef6f4b>] __arm64_sys_write+0x1c/0x28
>     [<0000000003496a98>] el0_svc_common+0xc8/0x22c
>     [<00000000ea3fe7a3>] el0_svc_compat_handler+0x1c/0x28
>     [<00000000d1a85a4e>] el0_svc_compat+0x8/0x24
> 
> Mark-PK Tsai (2):
>   dma-mapping: Add dma_release_coherent_memory to DMA API
>   remoteproc: Fix dma_mem leak after rproc_shutdown
> 
>  drivers/remoteproc/remoteproc_core.c |  1 +
>  include/linux/dma-map-ops.h          |  3 +++
>  kernel/dma/coherent.c                | 10 ++++++++--
>  3 files changed, 12 insertions(+), 2 deletions(-)

Applied.

Thanks,
Mathieu

> 
> -- 
> 2.18.0
>