Re: [PATCH] vduse: Fix race condition between resetting and irq injecting
From: Jason Wang <hidden>
Date: 2021-09-28 03:14:18
On Mon, Sep 27, 2021 at 9:18 PM Xie Yongji [off-list ref] wrote:
quoted hunk
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 <redacted> --- 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
quoted hunk
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