| Message ID | 20221109154213.146789-1-sgarzare@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | vhost-vdpa: fix potential memory leak during the release | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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@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
On Wed, Nov 9, 2022 at 11:42 PM Stefano Garzarella <sgarzare@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@redhat.com> Acked-by: Jason Wang <jasowang@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 >
On Wed, Nov 09, 2022 at 12:47:19PM -0500, Michael S. Tsirkin wrote: >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@redhat.com> > >It's fine, just pls don't say "potential" here in the subject, let's >avoid pleonasms Got it, I'll avoid in the future. >- it's a memory leak, yes it triggers under some coditions >but little is unconditional in this world :) Nothing could be more true :-) > >No need to repost. > Thanks, Stefano
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)
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@redhat.com> --- drivers/vhost/vdpa.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)