Re: [PATCH] vduse: Disallow injecting interrupt before DRIVER_OK is set

Re: [PATCH] vduse: Disallow injecting interrupt before DRIVER_OK is set

[Jason Wang]

On Thu, Sep 23, 2021 at 3:57 PM Xie Yongji <xieyongji@bytedance.com> wrote:
>
> The interrupt callback should not be triggered before DRIVER_OK
> is set. Otherwise, it might break the virtio device driver.
> So let's add a check to avoid the unexpected behavior.
>
> Fixes: c8a6153b6c59 ("vduse: Introduce VDUSE - vDPA Device in Userspace")
> Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> ---
>  drivers/vdpa/vdpa_user/vduse_dev.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index 29a38ecba19e..972c13a7e5da 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -968,6 +968,10 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
>                 break;
>         }
>         case VDUSE_DEV_INJECT_CONFIG_IRQ:
> +               ret = -EINVAL;
> +               if (!(dev->status & VIRTIO_CONFIG_S_DRIVER_OK))
> +                       break;
> +

I wonder if we need any synchronization with set_status()?

Thanks

>                 ret = 0;
>                 queue_work(vduse_irq_wq, &dev->inject);
>                 break;
> @@ -1047,6 +1051,10 @@ 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;
> --
> 2.11.0
>

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

Re: [PATCH] vduse: Disallow injecting interrupt before DRIVER_OK is set

[Michael S. Tsirkin]

On Thu, Sep 23, 2021 at 03:57:22PM +0800, Xie Yongji wrote:
> The interrupt callback should not be triggered before DRIVER_OK
> is set. Otherwise, it might break the virtio device driver.
> So let's add a check to avoid the unexpected behavior.
> 
> Fixes: c8a6153b6c59 ("vduse: Introduce VDUSE - vDPA Device in Userspace")
> Signed-off-by: Xie Yongji <xieyongji@bytedance.com>

OK I guess, except - what about locking/ordering in all this?

> ---
>  drivers/vdpa/vdpa_user/vduse_dev.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index 29a38ecba19e..972c13a7e5da 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -968,6 +968,10 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
>  		break;
>  	}
>  	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);
>  		break;
> @@ -1047,6 +1051,10 @@ 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;
> -- 
> 2.11.0

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