Gaurav Batra <gba...@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 <gba...@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.l...@gmail.com>
-ritesh
