Jason Wang
2021-Oct-21 08:24 UTC
[PATCH] vduse: Don't call interrupt callback without DRIVER_OK bit set
On Thu, Oct 21, 2021 at 11:29 AM Xie Yongji <xieyongji at bytedance.com> wrote:> > Commit 1018722ef0b7 ("vduse: Fix race condition between resetting and > irq injecting") tries to fix a race condition like: > > CPU0 CPU1 > ---- ---- > vduse_dev_ioctl() > check DRIVER_OK > vduse_dev_reset() > flush_work(&vq->inject) > queue_work(vduse_irq_wq, &vq->inject) > virtio_vdpa_probe() > virtio_vdpa_find_vqs() > vduse_vq_irq_inject() > vq->cb.callback(vq->cb.private); > set DRIVER_OK > > The irq callback will be triggered before DRIVER_OK is set in this case. > > To fix it, commit 1018722ef0b7 introduces a rwsem to add synchronization > between resetting and irq injecting. But it missed the synchronization > with set_status() and increased overhead on irq injection a bit. This > patch tries a simpler way to fix it. We just check DRIVER_OK again in > vduse_vq_irq_inject() and make sure each time setting status is done > under the irq lock.Acked-by: Jason Wang <jasowang at redhat.com> I wonder if we can use RCU to synchronize them in the future. Thanks> > Signed-off-by: Xie Yongji <xieyongji at bytedance.com> > --- > drivers/vdpa/vdpa_user/vduse_dev.c | 48 ++++++++++++++++++++++++-------------- > 1 file changed, 30 insertions(+), 18 deletions(-) > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c > index 841667a896dd..bf3c71ee597f 100644 > --- a/drivers/vdpa/vdpa_user/vduse_dev.c > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c > @@ -39,6 +39,8 @@ > #define VDUSE_IOVA_SIZE (128 * 1024 * 1024) > #define VDUSE_MSG_DEFAULT_TIMEOUT 30 > > +struct vduse_dev; > + > struct vduse_virtqueue { > u16 index; > u16 num_max; > @@ -55,10 +57,9 @@ struct vduse_virtqueue { > struct vdpa_callback cb; > struct work_struct inject; > struct work_struct kick; > + struct vduse_dev *dev; > }; > > -struct vduse_dev; > - > struct vduse_vdpa { > struct vdpa_device vdpa; > struct vduse_dev *dev; > @@ -80,7 +81,6 @@ 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; > @@ -402,6 +402,22 @@ static __poll_t vduse_dev_poll(struct file *file, poll_table *wait) > return mask; > } > > +static inline void vduse_dev_lock_vqs(struct vduse_dev *dev) > +{ > + int i; > + > + for (i = 0; i < dev->vq_num; i++) > + spin_lock(&dev->vqs[i].irq_lock); > +} > + > +static inline void vduse_dev_unlock_vqs(struct vduse_dev *dev) > +{ > + int i; > + > + for (i = 0; i < dev->vq_num; i++) > + spin_unlock(&dev->vqs[i].irq_lock); > +} > + > static void vduse_dev_reset(struct vduse_dev *dev) > { > int i; > @@ -411,8 +427,6 @@ 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++; > @@ -446,8 +460,6 @@ 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, > @@ -640,7 +652,11 @@ static void vduse_vdpa_set_status(struct vdpa_device *vdpa, u8 status) > if (vduse_dev_set_status(dev, status)) > return; > > + spin_lock(&dev->irq_lock); > + vduse_dev_lock_vqs(dev); > dev->status = status; > + vduse_dev_unlock_vqs(dev); > + spin_unlock(&dev->irq_lock); > } > > static size_t vduse_vdpa_get_config_size(struct vdpa_device *vdpa) > @@ -874,7 +890,8 @@ static void vduse_dev_irq_inject(struct work_struct *work) > struct vduse_dev *dev = container_of(work, struct vduse_dev, inject); > > spin_lock_irq(&dev->irq_lock); > - if (dev->config_cb.callback) > + if (dev->status & VIRTIO_CONFIG_S_DRIVER_OK && > + dev->config_cb.callback) > dev->config_cb.callback(dev->config_cb.private); > spin_unlock_irq(&dev->irq_lock); > } > @@ -883,9 +900,10 @@ static void vduse_vq_irq_inject(struct work_struct *work) > { > struct vduse_virtqueue *vq = container_of(work, > struct vduse_virtqueue, inject); > + struct vduse_dev *dev = vq->dev; > > spin_lock_irq(&vq->irq_lock); > - if (vq->ready && vq->cb.callback) > + if (dev->status & VIRTIO_CONFIG_S_DRIVER_OK && vq->cb.callback) > vq->cb.callback(vq->cb.private); > spin_unlock_irq(&vq->irq_lock); > } > @@ -893,18 +911,12 @@ static void vduse_vq_irq_inject(struct work_struct *work) > static int vduse_dev_queue_irq_work(struct vduse_dev *dev, > struct work_struct *irq_work) > { > - int ret = -EINVAL; > - > - down_read(&dev->rwsem); > if (!(dev->status & VIRTIO_CONFIG_S_DRIVER_OK)) > - goto unlock; > + return -EINVAL; > > - ret = 0; > queue_work(vduse_irq_wq, irq_work); > -unlock: > - up_read(&dev->rwsem); > > - return ret; > + return 0; > } > > static long vduse_dev_ioctl(struct file *file, unsigned int cmd, > @@ -1156,7 +1168,6 @@ 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); > @@ -1322,6 +1333,7 @@ static int vduse_create_dev(struct vduse_dev_config *config, > > for (i = 0; i < dev->vq_num; i++) { > dev->vqs[i].index = i; > + dev->vqs[i].dev = dev; > INIT_WORK(&dev->vqs[i].inject, vduse_vq_irq_inject); > INIT_WORK(&dev->vqs[i].kick, vduse_vq_kick_work); > spin_lock_init(&dev->vqs[i].kick_lock); > -- > 2.11.0 >