Re: [PATCH] powerpc/iommu: Memory leak in TCE table userspace view

[Ritesh Harjani (IBM) <ritesh.list@gmail.com>]

Gaurav Batra <gbatra@linux.ibm.com> writes:

> When a device is opened by a userspace driver, via VFIO interface, DMA
> window is created. This DMA window has TCE Table and a corresponding
> data for userview of
> TCE table.
>
> When the userspace driver closes the device, all the above infrastructure
> is free'ed and the device control given back to kernel. Both DMA window
> and TCE table is getting free'ed. But due to a code bug, userview of the
> TCE table is not getting free'ed. This is resulting in a memory leak.
>
> Befow is the information from KMEMLEAK
>
> unreferenced object 0xc008000022af0000 (size 16777216):
>   comm "senlib_unit_tes", pid 9346, jiffies 4294983174
>   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 (crc 0):
>     kmemleak_vmalloc+0xc8/0x1a0
>     __vmalloc_node_range+0x284/0x340
>     vzalloc+0x58/0x70
>     spapr_tce_create_table+0x4b0/0x8d0
>     tce_iommu_create_table+0xcc/0x170 [vfio_iommu_spapr_tce]
>     tce_iommu_create_window+0x144/0x2f0 [vfio_iommu_spapr_tce]
>     tce_iommu_ioctl.part.0+0x59c/0xc90 [vfio_iommu_spapr_tce]
>     vfio_fops_unl_ioctl+0x88/0x280 [vfio]
>     sys_ioctl+0xf4/0x160
>     system_call_exception+0x164/0x310
>     system_call_vectored_common+0xe8/0x278
> unreferenced object 0xc008000023b00000 (size 4194304):
>   comm "senlib_unit_tes", pid 9351, jiffies 4294984116
>   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 (crc 0):
>     kmemleak_vmalloc+0xc8/0x1a0
>     __vmalloc_node_range+0x284/0x340
>     vzalloc+0x58/0x70
>     spapr_tce_create_table+0x4b0/0x8d0
>     tce_iommu_create_table+0xcc/0x170 [vfio_iommu_spapr_tce]
>     tce_iommu_create_window+0x144/0x2f0 [vfio_iommu_spapr_tce]
>     tce_iommu_create_default_window+0x88/0x120 [vfio_iommu_spapr_tce]
>     tce_iommu_ioctl.part.0+0x57c/0xc90 [vfio_iommu_spapr_tce]
>     vfio_fops_unl_ioctl+0x88/0x280 [vfio]
>     sys_ioctl+0xf4/0x160
>     system_call_exception+0x164/0x310
>     system_call_vectored_common+0xe8/0x278
>
> Fixes: f431a8cde7f1 ("powerpc/iommu: Reimplement the iommu_table_group_ops for pSeries")
> Signed-off-by: Gaurav Batra <gbatra@linux.ibm.com>
> ---
>  arch/powerpc/platforms/pseries/iommu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
> index d6ebc19fb99c..eec333dd2e59 100644
> --- a/arch/powerpc/platforms/pseries/iommu.c
> +++ b/arch/powerpc/platforms/pseries/iommu.c
> @@ -197,7 +197,7 @@ static void tce_iommu_userspace_view_free(struct iommu_table *tbl)
>
>  static void tce_free_pSeries(struct iommu_table *tbl)
>  {
> -	if (!tbl->it_userspace)
> +	if (tbl->it_userspace)
>  		tce_iommu_userspace_view_free(tbl);
>  }

Gr8 catch. That clearly looks like a miss in the original code.

vfree() can be called even directly and it says no operation is
performed if addr passed to vfree is NULL. However I don't really see
any value add in doing that except maybe we can kill tce_free_pSeries()
function. But vfree() still does few checks in there. So we may as well
check for a non-null address before calling vfree().

nitpick: I might have re-pharsed the commit msg as:
     powerpc/pseries/iommu: Fix kmemleak in TCE table userspace view

The patch looks good to me purely from the kmemleak bug perspective.
So feel free to take: 
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>


-ritesh