| Message ID | 20210704205205.6132-1-gdawar@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] vhost-vdpa: mark vhost device invalid to reflect vdpa device unregistration | expand |
On Mon, Jul 05, 2021 at 02:22:04AM +0530, gautam.dawar@xilinx.com wrote: > From: Gautam Dawar <gdawar@xilinx.com> > > As mentioned in Bug 213179, any malicious user-space application can render > a module registering a vDPA device to hang forever. This will typically > surface when vdpa_device_unregister() is called from the function > responsible for module unload, leading rmmod commands to not return. > > This patch unblocks the caller module by continuing with the clean-up > but after marking the vhost device as unavailable. For future requests > from user-space application, the vhost device availability is checked > first and if it has gone unavailable, such requests are denied. > > Signed-off-by: Gautam Dawar <gdawar@xilinx.com> I don't seem mappings handled below. Did I miss it? > --- > drivers/vhost/vdpa.c | 45 ++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 39 insertions(+), 6 deletions(-) > > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c > index e4b7d26649d8..623bc7f0c0ca 100644 > --- a/drivers/vhost/vdpa.c > +++ b/drivers/vhost/vdpa.c > @@ -47,6 +47,7 @@ struct vhost_vdpa { > int minor; > struct eventfd_ctx *config_ctx; > int in_batch; > + int dev_invalid; > struct vdpa_iova_range range; > }; > > @@ -61,6 +62,11 @@ static void handle_vq_kick(struct vhost_work *work) > struct vhost_vdpa *v = container_of(vq->dev, struct vhost_vdpa, vdev); > const struct vdpa_config_ops *ops = v->vdpa->config; > > + if (v->dev_invalid) { > + dev_info(&v->dev, > + "%s: vhost_vdpa device unavailable\n", __func__); > + return; > + } > ops->kick_vq(v->vdpa, vq - v->vqs); > } > > @@ -120,6 +126,11 @@ static void vhost_vdpa_reset(struct vhost_vdpa *v) > { > struct vdpa_device *vdpa = v->vdpa; > > + if (v->dev_invalid) { > + dev_info(&v->dev, > + "%s: vhost_vdpa device unavailable\n", __func__); > + return; > + } > vdpa_reset(vdpa); > v->in_batch = 0; > } > @@ -367,6 +378,11 @@ static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd, > u32 idx; > long r; > > + if (v->dev_invalid) { > + dev_info(&v->dev, > + "%s: vhost_vdpa device unavailable\n", __func__); > + return -ENODEV; > + } > r = get_user(idx, (u32 __user *)argp); > if (r < 0) > return r; > @@ -450,6 +466,11 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep, > return 0; > } > > + if (v->dev_invalid) { > + dev_info(&v->dev, > + "%s: vhost_vdpa device unavailable\n", __func__); > + return -ENODEV; > + } > mutex_lock(&d->mutex); > > switch (cmd) { > @@ -745,8 +766,13 @@ static int vhost_vdpa_process_iotlb_msg(struct vhost_dev *dev, > const struct vdpa_config_ops *ops = vdpa->config; > int r = 0; > > - mutex_lock(&dev->mutex); > + if (v->dev_invalid) { > + dev_info(&v->dev, > + "%s: vhost_vdpa device unavailable\n", __func__); > + return -ENODEV; > + } > > + mutex_lock(&dev->mutex); > r = vhost_dev_check_owner(dev); > if (r) > goto unlock; > @@ -949,6 +975,11 @@ static vm_fault_t vhost_vdpa_fault(struct vm_fault *vmf) > struct vm_area_struct *vma = vmf->vma; > u16 index = vma->vm_pgoff; > > + if (v->dev_invalid) { > + dev_info(&v->dev, > + "%s: vhost_vdpa device unavailable\n", __func__); > + return VM_FAULT_NOPAGE; > + } > notify = ops->get_vq_notification(vdpa, index); > > vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); > @@ -1091,11 +1122,13 @@ static void vhost_vdpa_remove(struct vdpa_device *vdpa) > opened = atomic_cmpxchg(&v->opened, 0, 1); > if (!opened) > break; > - wait_for_completion_timeout(&v->completion, > - msecs_to_jiffies(1000)); > - dev_warn_once(&v->dev, > - "%s waiting for /dev/%s to be closed\n", > - __func__, dev_name(&v->dev)); > + if (!wait_for_completion_timeout(&v->completion, > + msecs_to_jiffies(1000))) { > + dev_warn(&v->dev, > + "%s /dev/%s in use, continue..\n", > + __func__, dev_name(&v->dev)); > + break; > + } When you have an arbitrary timeout you know something's not entirely robust ... > } while (1); > > put_device(&v->dev); > + v->dev_invalid = true; > -- > 2.30.1
在 2021/7/5 上午4:52, gautam.dawar@xilinx.com 写道: > vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); > @@ -1091,11 +1122,13 @@ static void vhost_vdpa_remove(struct vdpa_device *vdpa) > opened = atomic_cmpxchg(&v->opened, 0, 1); > if (!opened) > break; > - wait_for_completion_timeout(&v->completion, > - msecs_to_jiffies(1000)); > - dev_warn_once(&v->dev, > - "%s waiting for/dev/%s to be closed\n", > - __func__, dev_name(&v->dev)); > + if (!wait_for_completion_timeout(&v->completion, > + msecs_to_jiffies(1000))) { > + dev_warn(&v->dev, > + "%s/dev/%s in use, continue..\n", > + __func__, dev_name(&v->dev)); > + break; > + } > } while (1); > > put_device(&v->dev); > + v->dev_invalid = true; Besides the mapping handling mentioned by Michael. I think this can lead use-after-free. put_device may release the memory. Another fundamental issue, vDPA is the parent of vhost-vDPA device. I'm not sure the device core can allow the parent to go away first. Thanks
在 2021/7/5 上午11:48, Jason Wang 写道: > > 在 2021/7/5 上午4:52, gautam.dawar@xilinx.com 写道: >> vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); >> @@ -1091,11 +1122,13 @@ static void vhost_vdpa_remove(struct >> vdpa_device *vdpa) >> opened = atomic_cmpxchg(&v->opened, 0, 1); >> if (!opened) >> break; >> - wait_for_completion_timeout(&v->completion, >> - msecs_to_jiffies(1000)); >> - dev_warn_once(&v->dev, >> - "%s waiting for/dev/%s to be closed\n", >> - __func__, dev_name(&v->dev)); >> + if (!wait_for_completion_timeout(&v->completion, >> + msecs_to_jiffies(1000))) { >> + dev_warn(&v->dev, >> + "%s/dev/%s in use, continue..\n", >> + __func__, dev_name(&v->dev)); >> + break; >> + } >> } while (1); >> put_device(&v->dev); >> + v->dev_invalid = true; > > > Besides the mapping handling mentioned by Michael. I think this can > lead use-after-free. put_device may release the memory. > > Another fundamental issue, vDPA is the parent of vhost-vDPA device. > I'm not sure the device core can allow the parent to go away first. Or this probably means you need couple the fd loosely with the vhost-vDPA device. Thanks > > Thanks > >
diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c index e4b7d26649d8..623bc7f0c0ca 100644 --- a/drivers/vhost/vdpa.c +++ b/drivers/vhost/vdpa.c @@ -47,6 +47,7 @@ struct vhost_vdpa { int minor; struct eventfd_ctx *config_ctx; int in_batch; + int dev_invalid; struct vdpa_iova_range range; }; @@ -61,6 +62,11 @@ static void handle_vq_kick(struct vhost_work *work) struct vhost_vdpa *v = container_of(vq->dev, struct vhost_vdpa, vdev); const struct vdpa_config_ops *ops = v->vdpa->config; + if (v->dev_invalid) { + dev_info(&v->dev, + "%s: vhost_vdpa device unavailable\n", __func__); + return; + } ops->kick_vq(v->vdpa, vq - v->vqs); } @@ -120,6 +126,11 @@ static void vhost_vdpa_reset(struct vhost_vdpa *v) { struct vdpa_device *vdpa = v->vdpa; + if (v->dev_invalid) { + dev_info(&v->dev, + "%s: vhost_vdpa device unavailable\n", __func__); + return; + } vdpa_reset(vdpa); v->in_batch = 0; } @@ -367,6 +378,11 @@ static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd, u32 idx; long r; + if (v->dev_invalid) { + dev_info(&v->dev, + "%s: vhost_vdpa device unavailable\n", __func__); + return -ENODEV; + } r = get_user(idx, (u32 __user *)argp); if (r < 0) return r; @@ -450,6 +466,11 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep, return 0; } + if (v->dev_invalid) { + dev_info(&v->dev, + "%s: vhost_vdpa device unavailable\n", __func__); + return -ENODEV; + } mutex_lock(&d->mutex); switch (cmd) { @@ -745,8 +766,13 @@ static int vhost_vdpa_process_iotlb_msg(struct vhost_dev *dev, const struct vdpa_config_ops *ops = vdpa->config; int r = 0; - mutex_lock(&dev->mutex); + if (v->dev_invalid) { + dev_info(&v->dev, + "%s: vhost_vdpa device unavailable\n", __func__); + return -ENODEV; + } + mutex_lock(&dev->mutex); r = vhost_dev_check_owner(dev); if (r) goto unlock; @@ -949,6 +975,11 @@ static vm_fault_t vhost_vdpa_fault(struct vm_fault *vmf) struct vm_area_struct *vma = vmf->vma; u16 index = vma->vm_pgoff; + if (v->dev_invalid) { + dev_info(&v->dev, + "%s: vhost_vdpa device unavailable\n", __func__); + return VM_FAULT_NOPAGE; + } notify = ops->get_vq_notification(vdpa, index); vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); @@ -1091,11 +1122,13 @@ static void vhost_vdpa_remove(struct vdpa_device *vdpa) opened = atomic_cmpxchg(&v->opened, 0, 1); if (!opened) break; - wait_for_completion_timeout(&v->completion, - msecs_to_jiffies(1000)); - dev_warn_once(&v->dev, - "%s waiting for /dev/%s to be closed\n", - __func__, dev_name(&v->dev)); + if (!wait_for_completion_timeout(&v->completion, + msecs_to_jiffies(1000))) { + dev_warn(&v->dev, + "%s /dev/%s in use, continue..\n", + __func__, dev_name(&v->dev)); + break; + } } while (1); put_device(&v->dev);