Re: [PATCH] vhost-vdpa: cleanup memory maps when closing vdpa fds

[Longpeng (Mike, Cloud Infrastructure Service Product Dept.)]

在 2023/2/15 10:00, Jason Wang 写道:
> On Tue, Feb 14, 2023 at 2:28 PM Longpeng (Mike, Cloud Infrastructure
> Service Product Dept.) <longpeng2@xxxxxxxxxx> wrote:
> > 在 2023/2/14 14:16, Jason Wang 写道:
> > > 在 2023/1/31 22:53, Longpeng(Mike) 写道:
> > > > From: Longpeng <longpeng2@xxxxxxxxxx>
> > > >
> > > > We must cleanup all memory maps when closing the vdpa fds, otherwise
> > > > some critical resources (e.g. memory, iommu map) will leaked if the
> > > > userspace exits unexpectedly (e.g. kill -9).
> > >
> > >
> > > Sounds like a bug of the kernel, should we fix there?
> >
> > For example, the iommu map is setup when QEMU calls VHOST_IOTLB_UPDATE
> > ioctl and it'll be freed if QEMU calls VHOST_IOTLB_INVALIDATE ioctl.
> >
> > So maybe we release these resources in vdpa framework in kernel is a
> > suitable choice?
>
> I think I need understand what does "resources" mean here:
>
> For iommu mapping, it should be freed by vhost_vdpa_free_domain() in
> vhost_vdpa_release()?

Please consider the following lifecycle of the vdpa device:

1. vhost_vdpa_open
    vhost_vdpa_alloc_domain

2. vhost_vdpa_pa_map
    pin_user_pages
    vhost_vdpa_map
      iommu_map

3. kill QEMU

4. vhost_vdpa_release
    vhost_vdpa_free_domain

In this case, we have no opportunity to invoke unpin_user_pages or 
iommu_unmap to free the memory.

> static int vhost_vdpa_release(struct inode *inode, struct file *filep)
> {
>          struct vhost_vdpa *v = filep->private_data;
>          struct vhost_dev *d = &v->vdev;
>
>          mutex_lock(&d->mutex);
>          filep->private_data = NULL;
>          vhost_vdpa_clean_irq(v);
>          vhost_vdpa_reset(v);
>          vhost_dev_stop(&v->vdev);
>          vhost_vdpa_free_domain(v);
>          vhost_vdpa_config_put(v);
>          vhost_vdpa_cleanup(v);
>          mutex_unlock(&d->mutex);
>
>          atomic_dec(&v->opened);
>          complete(&v->completion);
>
>          return 0;
> }
>
> > By the way, Jason, can you reproduce the problem in your machine?
>
> Haven't got time in doing this but it should be the responsibility of
> the author to validate this anyhow.
>
> Thanks
>
> > > Thanks
> > >
> > >
> > > > Signed-off-by: Longpeng <longpeng2@xxxxxxxxxx>
> > > > ---
> > > >    drivers/vhost/vdpa.c | 13 +++++++++++++
> > > >    1 file changed, 13 insertions(+)
> > > >
> > > > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> > > > index a527eeeac637..37477cffa5aa 100644
> > > > --- a/drivers/vhost/vdpa.c
> > > > +++ b/drivers/vhost/vdpa.c
> > > > @@ -823,6 +823,18 @@ static void vhost_vdpa_unmap(struct vhost_vdpa *v,
> > > >            vhost_vdpa_remove_as(v, asid);
> > > >    }
> > > > +static void vhost_vdpa_clean_map(struct vhost_vdpa *v)
> > > > +{
> > > > +    struct vhost_vdpa_as *as;
> > > > +    u32 asid;
> > > > +
> > > > +    for (asid = 0; asid < v->vdpa->nas; asid++) {
> > > > +        as = asid_to_as(v, asid);
> > > > +        if (as)
> > > > +            vhost_vdpa_unmap(v, &as->iotlb, 0ULL, 0ULL - 1);
> > > > +    }
> > > > +}
> > > > +
> > > >    static int vhost_vdpa_va_map(struct vhost_vdpa *v,
> > > >                     struct vhost_iotlb *iotlb,
> > > >                     u64 iova, u64 size, u64 uaddr, u32 perm)
> > > > @@ -1247,6 +1259,7 @@ static int vhost_vdpa_release(struct inode
> > > > *inode, struct file *filep)
> > > >        vhost_vdpa_clean_irq(v);
> > > >        vhost_vdpa_reset(v);
> > > >        vhost_dev_stop(&v->vdev);
> > > > +    vhost_vdpa_clean_map(v);
> > > >        vhost_vdpa_free_domain(v);
> > > >        vhost_vdpa_config_put(v);
> > > >        vhost_vdpa_cleanup(v);
> > >
> > > .
>
> .