Stefano Garzarella
2022-Nov-09 15:42 UTC
[PATCH] vhost-vdpa: fix potential memory leak during the release
Before commit 3d5698793897 ("vhost-vdpa: introduce asid based IOTLB")
we call vhost_vdpa_iotlb_unmap(v, iotlb, 0ULL, 0ULL - 1) during the
release to free all the resources allocated when processing user IOTLB
messages through vhost_vdpa_process_iotlb_update().
That commit changed the handling of IOTLB a bit, and we accidentally
removed some code called during the release.
We partially fixed with commit 037d4305569a ("vhost-vdpa: call
vhost_vdpa_cleanup during the release") but a potential memory leak is
still there as showed by kmemleak if the application does not send
VHOST_IOTLB_INVALIDATE or crashes:
unreferenced object 0xffff888007fbaa30 (size 16):
comm "blkio-bench", pid 914, jiffies 4294993521 (age 885.500s)
hex dump (first 16 bytes):
40 73 41 07 80 88 ff ff 00 00 00 00 00 00 00 00 @sA.............
backtrace:
[<0000000087736d2a>] kmem_cache_alloc_trace+0x142/0x1c0
[<0000000060740f50>] vhost_vdpa_process_iotlb_msg+0x68c/0x901
[vhost_vdpa]
[<0000000083e8e205>] vhost_chr_write_iter+0xc0/0x4a0 [vhost]
[<000000008f2f414a>] vhost_vdpa_chr_write_iter+0x18/0x20
[vhost_vdpa]
[<00000000de1cd4a0>] vfs_write+0x216/0x4b0
[<00000000a2850200>] ksys_write+0x71/0xf0
[<00000000de8e720b>] __x64_sys_write+0x19/0x20
[<0000000018b12cbb>] do_syscall_64+0x3f/0x90
[<00000000986ec465>] entry_SYSCALL_64_after_hwframe+0x63/0xcd
Let's fix calling vhost_vdpa_iotlb_unmap() on the whole range in
vhost_vdpa_remove_as(). We move that call before vhost_dev_cleanup()
since we need a valid v->vdev.mm in vhost_vdpa_pa_unmap().
vhost_iotlb_reset() call can be removed, since vhost_vdpa_iotlb_unmap()
on the whole range removes all the entries.
The kmemleak log reported was observed with a vDPA device that has `use_va`
set to true (e.g. VDUSE). This patch has been tested with both types of
devices.
Fixes: 037d4305569a ("vhost-vdpa: call vhost_vdpa_cleanup during the
release")
Fixes: 3d5698793897 ("vhost-vdpa: introduce asid based IOTLB")
Signed-off-by: Stefano Garzarella <sgarzare at redhat.com>
---
drivers/vhost/vdpa.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index 166044642fd5..b08e07fc7d1f 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -65,6 +65,10 @@ static DEFINE_IDA(vhost_vdpa_ida);
static dev_t vhost_vdpa_major;
+static void vhost_vdpa_iotlb_unmap(struct vhost_vdpa *v,
+ struct vhost_iotlb *iotlb,
+ u64 start, u64 last);
+
static inline u32 iotlb_to_asid(struct vhost_iotlb *iotlb)
{
struct vhost_vdpa_as *as = container_of(iotlb, struct
@@ -135,7 +139,7 @@ static int vhost_vdpa_remove_as(struct vhost_vdpa *v, u32
asid)
return -EINVAL;
hlist_del(&as->hash_link);
- vhost_iotlb_reset(&as->iotlb);
+ vhost_vdpa_iotlb_unmap(v, &as->iotlb, 0ULL, 0ULL - 1);
kfree(as);
return 0;
@@ -1162,14 +1166,14 @@ static void vhost_vdpa_cleanup(struct vhost_vdpa *v)
struct vhost_vdpa_as *as;
u32 asid;
- vhost_dev_cleanup(&v->vdev);
- kfree(v->vdev.vqs);
-
for (asid = 0; asid < v->vdpa->nas; asid++) {
as = asid_to_as(v, asid);
if (as)
vhost_vdpa_remove_as(v, asid);
}
+
+ vhost_dev_cleanup(&v->vdev);
+ kfree(v->vdev.vqs);
}
static int vhost_vdpa_open(struct inode *inode, struct file *filep)
--
2.38.1
Michael S. Tsirkin
2022-Nov-09 17:47 UTC
[PATCH] vhost-vdpa: fix potential memory leak during the release
On Wed, Nov 09, 2022 at 04:42:13PM +0100, Stefano Garzarella wrote:> Before commit 3d5698793897 ("vhost-vdpa: introduce asid based IOTLB") > we call vhost_vdpa_iotlb_unmap(v, iotlb, 0ULL, 0ULL - 1) during the > release to free all the resources allocated when processing user IOTLB > messages through vhost_vdpa_process_iotlb_update(). > That commit changed the handling of IOTLB a bit, and we accidentally > removed some code called during the release. > > We partially fixed with commit 037d4305569a ("vhost-vdpa: call > vhost_vdpa_cleanup during the release") but a potential memory leak is > still there as showed by kmemleak if the application does not send > VHOST_IOTLB_INVALIDATE or crashes: > > unreferenced object 0xffff888007fbaa30 (size 16): > comm "blkio-bench", pid 914, jiffies 4294993521 (age 885.500s) > hex dump (first 16 bytes): > 40 73 41 07 80 88 ff ff 00 00 00 00 00 00 00 00 @sA............. > backtrace: > [<0000000087736d2a>] kmem_cache_alloc_trace+0x142/0x1c0 > [<0000000060740f50>] vhost_vdpa_process_iotlb_msg+0x68c/0x901 [vhost_vdpa] > [<0000000083e8e205>] vhost_chr_write_iter+0xc0/0x4a0 [vhost] > [<000000008f2f414a>] vhost_vdpa_chr_write_iter+0x18/0x20 [vhost_vdpa] > [<00000000de1cd4a0>] vfs_write+0x216/0x4b0 > [<00000000a2850200>] ksys_write+0x71/0xf0 > [<00000000de8e720b>] __x64_sys_write+0x19/0x20 > [<0000000018b12cbb>] do_syscall_64+0x3f/0x90 > [<00000000986ec465>] entry_SYSCALL_64_after_hwframe+0x63/0xcd > > Let's fix calling vhost_vdpa_iotlb_unmap() on the whole range in > vhost_vdpa_remove_as(). We move that call before vhost_dev_cleanup() > since we need a valid v->vdev.mm in vhost_vdpa_pa_unmap(). > vhost_iotlb_reset() call can be removed, since vhost_vdpa_iotlb_unmap() > on the whole range removes all the entries. > > The kmemleak log reported was observed with a vDPA device that has `use_va` > set to true (e.g. VDUSE). This patch has been tested with both types of > devices. > > Fixes: 037d4305569a ("vhost-vdpa: call vhost_vdpa_cleanup during the release") > Fixes: 3d5698793897 ("vhost-vdpa: introduce asid based IOTLB") > Signed-off-by: Stefano Garzarella <sgarzare at redhat.com>It's fine, just pls don't say "potential" here in the subject, let's avoid pleonasms - it's a memory leak, yes it triggers under some coditions but little is unconditional in this world :) No need to repost.> --- > drivers/vhost/vdpa.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c > index 166044642fd5..b08e07fc7d1f 100644 > --- a/drivers/vhost/vdpa.c > +++ b/drivers/vhost/vdpa.c > @@ -65,6 +65,10 @@ static DEFINE_IDA(vhost_vdpa_ida); > > static dev_t vhost_vdpa_major; > > +static void vhost_vdpa_iotlb_unmap(struct vhost_vdpa *v, > + struct vhost_iotlb *iotlb, > + u64 start, u64 last); > + > static inline u32 iotlb_to_asid(struct vhost_iotlb *iotlb) > { > struct vhost_vdpa_as *as = container_of(iotlb, struct > @@ -135,7 +139,7 @@ static int vhost_vdpa_remove_as(struct vhost_vdpa *v, u32 asid) > return -EINVAL; > > hlist_del(&as->hash_link); > - vhost_iotlb_reset(&as->iotlb); > + vhost_vdpa_iotlb_unmap(v, &as->iotlb, 0ULL, 0ULL - 1); > kfree(as); > > return 0; > @@ -1162,14 +1166,14 @@ static void vhost_vdpa_cleanup(struct vhost_vdpa *v) > struct vhost_vdpa_as *as; > u32 asid; > > - vhost_dev_cleanup(&v->vdev); > - kfree(v->vdev.vqs); > - > for (asid = 0; asid < v->vdpa->nas; asid++) { > as = asid_to_as(v, asid); > if (as) > vhost_vdpa_remove_as(v, asid); > } > + > + vhost_dev_cleanup(&v->vdev); > + kfree(v->vdev.vqs); > } > > static int vhost_vdpa_open(struct inode *inode, struct file *filep) > -- > 2.38.1
Jason Wang
2022-Nov-10 02:03 UTC
[PATCH] vhost-vdpa: fix potential memory leak during the release
On Wed, Nov 9, 2022 at 11:42 PM Stefano Garzarella <sgarzare at redhat.com> wrote:> > Before commit 3d5698793897 ("vhost-vdpa: introduce asid based IOTLB") > we call vhost_vdpa_iotlb_unmap(v, iotlb, 0ULL, 0ULL - 1) during the > release to free all the resources allocated when processing user IOTLB > messages through vhost_vdpa_process_iotlb_update(). > That commit changed the handling of IOTLB a bit, and we accidentally > removed some code called during the release. > > We partially fixed with commit 037d4305569a ("vhost-vdpa: call > vhost_vdpa_cleanup during the release") but a potential memory leak is > still there as showed by kmemleak if the application does not send > VHOST_IOTLB_INVALIDATE or crashes: > > unreferenced object 0xffff888007fbaa30 (size 16): > comm "blkio-bench", pid 914, jiffies 4294993521 (age 885.500s) > hex dump (first 16 bytes): > 40 73 41 07 80 88 ff ff 00 00 00 00 00 00 00 00 @sA............. > backtrace: > [<0000000087736d2a>] kmem_cache_alloc_trace+0x142/0x1c0 > [<0000000060740f50>] vhost_vdpa_process_iotlb_msg+0x68c/0x901 [vhost_vdpa] > [<0000000083e8e205>] vhost_chr_write_iter+0xc0/0x4a0 [vhost] > [<000000008f2f414a>] vhost_vdpa_chr_write_iter+0x18/0x20 [vhost_vdpa] > [<00000000de1cd4a0>] vfs_write+0x216/0x4b0 > [<00000000a2850200>] ksys_write+0x71/0xf0 > [<00000000de8e720b>] __x64_sys_write+0x19/0x20 > [<0000000018b12cbb>] do_syscall_64+0x3f/0x90 > [<00000000986ec465>] entry_SYSCALL_64_after_hwframe+0x63/0xcd > > Let's fix calling vhost_vdpa_iotlb_unmap() on the whole range in > vhost_vdpa_remove_as(). We move that call before vhost_dev_cleanup() > since we need a valid v->vdev.mm in vhost_vdpa_pa_unmap(). > vhost_iotlb_reset() call can be removed, since vhost_vdpa_iotlb_unmap() > on the whole range removes all the entries. > > The kmemleak log reported was observed with a vDPA device that has `use_va` > set to true (e.g. VDUSE). This patch has been tested with both types of > devices. > > Fixes: 037d4305569a ("vhost-vdpa: call vhost_vdpa_cleanup during the release") > Fixes: 3d5698793897 ("vhost-vdpa: introduce asid based IOTLB") > Signed-off-by: Stefano Garzarella <sgarzare at redhat.com>Acked-by: Jason Wang <jasowang at redhat.com>> --- > drivers/vhost/vdpa.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c > index 166044642fd5..b08e07fc7d1f 100644 > --- a/drivers/vhost/vdpa.c > +++ b/drivers/vhost/vdpa.c > @@ -65,6 +65,10 @@ static DEFINE_IDA(vhost_vdpa_ida); > > static dev_t vhost_vdpa_major; > > +static void vhost_vdpa_iotlb_unmap(struct vhost_vdpa *v, > + struct vhost_iotlb *iotlb, > + u64 start, u64 last); > + > static inline u32 iotlb_to_asid(struct vhost_iotlb *iotlb) > { > struct vhost_vdpa_as *as = container_of(iotlb, struct > @@ -135,7 +139,7 @@ static int vhost_vdpa_remove_as(struct vhost_vdpa *v, u32 asid) > return -EINVAL; > > hlist_del(&as->hash_link); > - vhost_iotlb_reset(&as->iotlb); > + vhost_vdpa_iotlb_unmap(v, &as->iotlb, 0ULL, 0ULL - 1); > kfree(as); > > return 0; > @@ -1162,14 +1166,14 @@ static void vhost_vdpa_cleanup(struct vhost_vdpa *v) > struct vhost_vdpa_as *as; > u32 asid; > > - vhost_dev_cleanup(&v->vdev); > - kfree(v->vdev.vqs); > - > for (asid = 0; asid < v->vdpa->nas; asid++) { > as = asid_to_as(v, asid); > if (as) > vhost_vdpa_remove_as(v, asid); > } > + > + vhost_dev_cleanup(&v->vdev); > + kfree(v->vdev.vqs); > } > > static int vhost_vdpa_open(struct inode *inode, struct file *filep) > -- > 2.38.1 >