Re: [PATCH] vduse: Fix race condition between resetting and irq injecting

Re: [PATCH] vduse: Fix race condition between resetting and irq injecting

[Jason Wang]

On Mon, Sep 27, 2021 at 9:18 PM Xie Yongji <xieyongji@bytedance.com> wrote:
>
> The interrupt might be triggered after a reset since there is
> no synchronization between resetting and irq injecting. And it
> might break something if the interrupt is delayed until a new
> round of device initialization.
>
> Fixes: c8a6153b6c59 ("vduse: Introduce VDUSE - vDPA Device in Userspace")
> Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> ---
>  drivers/vdpa/vdpa_user/vduse_dev.c | 31 +++++++++++++++++++------------
>  1 file changed, 19 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index cefb301b2ee4..ea7d80ade559 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -80,6 +80,7 @@ struct vduse_dev {
>         struct vdpa_callback config_cb;
>         struct work_struct inject;
>         spinlock_t irq_lock;
> +       struct rw_semaphore rwsem;
>         int minor;
>         bool broken;
>         bool connected;
> @@ -410,6 +411,8 @@ static void vduse_dev_reset(struct vduse_dev *dev)
>         if (domain->bounce_map)
>                 vduse_domain_reset_bounce_map(domain);
>
> +       down_write(&dev->rwsem);
> +
>         dev->status = 0;
>         dev->driver_features = 0;
>         dev->generation++;
> @@ -443,6 +446,8 @@ static void vduse_dev_reset(struct vduse_dev *dev)
>                 flush_work(&vq->inject);
>                 flush_work(&vq->kick);
>         }
> +
> +       up_write(&dev->rwsem);
>  }
>
>  static int vduse_vdpa_set_vq_address(struct vdpa_device *vdpa, u16 idx,
> @@ -967,11 +972,12 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
>         }
>         case VDUSE_DEV_INJECT_CONFIG_IRQ:
>                 ret = -EINVAL;
> -               if (!(dev->status & VIRTIO_CONFIG_S_DRIVER_OK))
> -                       break;
> -
> -               ret = 0;
> -               queue_work(vduse_irq_wq, &dev->inject);
> +               down_read(&dev->rwsem);
> +               if (dev->status & VIRTIO_CONFIG_S_DRIVER_OK) {
> +                       ret = 0;
> +                       queue_work(vduse_irq_wq, &dev->inject);
> +               }
> +               up_read(&dev->rwsem);
>                 break;
>         case VDUSE_VQ_SETUP: {
>                 struct vduse_vq_config config;
> @@ -1049,10 +1055,6 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
>         case VDUSE_VQ_INJECT_IRQ: {
>                 u32 index;
>
> -               ret = -EINVAL;
> -               if (!(dev->status & VIRTIO_CONFIG_S_DRIVER_OK))
> -                       break;
> -
>                 ret = -EFAULT;
>                 if (get_user(index, (u32 __user *)argp))
>                         break;
> @@ -1061,9 +1063,13 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
>                 if (index >= dev->vq_num)
>                         break;
>
> -               ret = 0;
> -               index = array_index_nospec(index, dev->vq_num);
> -               queue_work(vduse_irq_wq, &dev->vqs[index].inject);
> +               down_read(&dev->rwsem);
> +               if (dev->status & VIRTIO_CONFIG_S_DRIVER_OK) {
> +                       ret = 0;
> +                       index = array_index_nospec(index, dev->vq_num);
> +                       queue_work(vduse_irq_wq, &dev->vqs[index].inject);
> +               }
> +               up_read(&dev->rwsem);

Nit: I think we can factor out the common logic to a helper.

Others look good.

Thanks

>                 break;
>         }
>         default:
> @@ -1144,6 +1150,7 @@ static struct vduse_dev *vduse_dev_create(void)
>         INIT_LIST_HEAD(&dev->send_list);
>         INIT_LIST_HEAD(&dev->recv_list);
>         spin_lock_init(&dev->irq_lock);
> +       init_rwsem(&dev->rwsem);
>
>         INIT_WORK(&dev->inject, vduse_dev_irq_inject);
>         init_waitqueue_head(&dev->waitq);
> --
> 2.11.0
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization