From: Jason Wang <jasowang@redhat.com> To: jasowang@redhat.com, mst@redhat.com, virtualization@lists.linux-foundation.org, linux-kernel@vger.kernel.org Cc: tglx@linutronix.de, peterz@infradead.org, paulmck@kernel.org, maz@kernel.org, pasic@linux.ibm.com, cohuck@redhat.com, eperezma@redhat.com, lulu@redhat.com, sgarzare@redhat.com, xuanzhuo@linux.alibaba.com Subject: [PATCH V4 8/9] virtio: harden vring IRQ Date: Sat, 7 May 2022 15:19:53 +0800 [thread overview] Message-ID: <20220507071954.14455-9-jasowang@redhat.com> (raw) In-Reply-To: <20220507071954.14455-1-jasowang@redhat.com> This is a rework on the previous IRQ hardening that is done for virtio-pci where several drawbacks were found and were reverted: 1) try to use IRQF_NO_AUTOEN which is not friendly to affinity managed IRQ that is used by some device such as virtio-blk 2) done only for PCI transport The vq->broken is re-used in this patch for implementing the IRQ hardening. The vq->broken is set to true during both initialization and reset. And the vq->broken is set to false in virtio_device_ready(). Then vring_interrupt can check and return when vq->broken is true. And in this case, switch to return IRQ_NONE to let the interrupt core aware of such invalid interrupt to prevent IRQ storm. The reason of using a per queue variable instead of a per device one is that we may need it for per queue reset hardening in the future. Note that the hardening is only done for vring interrupt since the config interrupt hardening is already done in commit 22b7050a024d7 ("virtio: defer config changed notifications"). But the method that is used by config interrupt can't be reused by the vring interrupt handler because it uses spinlock to do the synchronization which is expensive. Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Peter Zijlstra <peterz@infradead.org> Cc: "Paul E. McKenney" <paulmck@kernel.org> Cc: Marc Zyngier <maz@kernel.org> Cc: Halil Pasic <pasic@linux.ibm.com> Cc: Cornelia Huck <cohuck@redhat.com> Signed-off-by: Jason Wang <jasowang@redhat.com> --- drivers/virtio/virtio.c | 15 ++++++++++++--- drivers/virtio/virtio_ring.c | 11 +++++++---- include/linux/virtio_config.h | 12 ++++++++++++ 3 files changed, 31 insertions(+), 7 deletions(-) diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c index 8dde44ea044a..696f5ba4f38e 100644 --- a/drivers/virtio/virtio.c +++ b/drivers/virtio/virtio.c @@ -220,6 +220,15 @@ static int virtio_features_ok(struct virtio_device *dev) * */ void virtio_reset_device(struct virtio_device *dev) { + /* + * The below virtio_synchronize_cbs() guarantees that any + * interrupt for this line arriving after + * virtio_synchronize_vqs() has completed is guaranteed to see + * driver_ready == false. + */ + virtio_break_device(dev); + virtio_synchronize_cbs(dev); + dev->config->reset(dev); } EXPORT_SYMBOL_GPL(virtio_reset_device); @@ -428,6 +437,9 @@ int register_virtio_device(struct virtio_device *dev) dev->config_enabled = false; dev->config_change_pending = false; + INIT_LIST_HEAD(&dev->vqs); + spin_lock_init(&dev->vqs_list_lock); + /* We always start by resetting the device, in case a previous * driver messed it up. This also tests that code path a little. */ virtio_reset_device(dev); @@ -435,9 +447,6 @@ int register_virtio_device(struct virtio_device *dev) /* Acknowledge that we've seen the device. */ virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE); - INIT_LIST_HEAD(&dev->vqs); - spin_lock_init(&dev->vqs_list_lock); - /* * device_add() causes the bus infrastructure to look for a matching * driver. diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index 5b7df7c455f0..9dfad2890d7a 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -1690,7 +1690,7 @@ static struct virtqueue *vring_create_virtqueue_packed( vq->we_own_ring = true; vq->notify = notify; vq->weak_barriers = weak_barriers; - vq->broken = false; + vq->broken = true; vq->last_used_idx = 0; vq->event_triggered = false; vq->num_added = 0; @@ -2136,8 +2136,11 @@ irqreturn_t vring_interrupt(int irq, void *_vq) return IRQ_NONE; } - if (unlikely(vq->broken)) - return IRQ_HANDLED; + if (unlikely(vq->broken)) { + dev_warn_once(&vq->vq.vdev->dev, + "virtio vring IRQ raised before DRIVER_OK"); + return IRQ_NONE; + } /* Just a hint for performance: so it's ok that this can be racy! */ if (vq->event) @@ -2179,7 +2182,7 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index, vq->we_own_ring = false; vq->notify = notify; vq->weak_barriers = weak_barriers; - vq->broken = false; + vq->broken = true; vq->last_used_idx = 0; vq->event_triggered = false; vq->num_added = 0; diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h index d8a2340f928e..23f1694cdbd5 100644 --- a/include/linux/virtio_config.h +++ b/include/linux/virtio_config.h @@ -256,6 +256,18 @@ void virtio_device_ready(struct virtio_device *dev) unsigned status = dev->config->get_status(dev); BUG_ON(status & VIRTIO_CONFIG_S_DRIVER_OK); + + /* + * The virtio_synchronize_cbs() makes sure vring_interrupt() + * will see the driver specific setup if it sees vq->broken + * as false. + */ + virtio_synchronize_cbs(dev); + __virtio_unbreak_device(dev); + /* + * The transport is expected ensure the visibility of + * vq->broken before setting VIRTIO_CONFIG_S_DRIVER_OK. + */ dev->config->set_status(dev, status | VIRTIO_CONFIG_S_DRIVER_OK); } -- 2.25.1
WARNING: multiple messages have this Message-ID (diff)
From: Jason Wang <jasowang@redhat.com> To: jasowang@redhat.com, mst@redhat.com, virtualization@lists.linux-foundation.org, linux-kernel@vger.kernel.org Cc: lulu@redhat.com, paulmck@kernel.org, peterz@infradead.org, maz@kernel.org, cohuck@redhat.com, pasic@linux.ibm.com, eperezma@redhat.com, tglx@linutronix.de Subject: [PATCH V4 8/9] virtio: harden vring IRQ Date: Sat, 7 May 2022 15:19:53 +0800 [thread overview] Message-ID: <20220507071954.14455-9-jasowang@redhat.com> (raw) In-Reply-To: <20220507071954.14455-1-jasowang@redhat.com> This is a rework on the previous IRQ hardening that is done for virtio-pci where several drawbacks were found and were reverted: 1) try to use IRQF_NO_AUTOEN which is not friendly to affinity managed IRQ that is used by some device such as virtio-blk 2) done only for PCI transport The vq->broken is re-used in this patch for implementing the IRQ hardening. The vq->broken is set to true during both initialization and reset. And the vq->broken is set to false in virtio_device_ready(). Then vring_interrupt can check and return when vq->broken is true. And in this case, switch to return IRQ_NONE to let the interrupt core aware of such invalid interrupt to prevent IRQ storm. The reason of using a per queue variable instead of a per device one is that we may need it for per queue reset hardening in the future. Note that the hardening is only done for vring interrupt since the config interrupt hardening is already done in commit 22b7050a024d7 ("virtio: defer config changed notifications"). But the method that is used by config interrupt can't be reused by the vring interrupt handler because it uses spinlock to do the synchronization which is expensive. Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Peter Zijlstra <peterz@infradead.org> Cc: "Paul E. McKenney" <paulmck@kernel.org> Cc: Marc Zyngier <maz@kernel.org> Cc: Halil Pasic <pasic@linux.ibm.com> Cc: Cornelia Huck <cohuck@redhat.com> Signed-off-by: Jason Wang <jasowang@redhat.com> --- drivers/virtio/virtio.c | 15 ++++++++++++--- drivers/virtio/virtio_ring.c | 11 +++++++---- include/linux/virtio_config.h | 12 ++++++++++++ 3 files changed, 31 insertions(+), 7 deletions(-) diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c index 8dde44ea044a..696f5ba4f38e 100644 --- a/drivers/virtio/virtio.c +++ b/drivers/virtio/virtio.c @@ -220,6 +220,15 @@ static int virtio_features_ok(struct virtio_device *dev) * */ void virtio_reset_device(struct virtio_device *dev) { + /* + * The below virtio_synchronize_cbs() guarantees that any + * interrupt for this line arriving after + * virtio_synchronize_vqs() has completed is guaranteed to see + * driver_ready == false. + */ + virtio_break_device(dev); + virtio_synchronize_cbs(dev); + dev->config->reset(dev); } EXPORT_SYMBOL_GPL(virtio_reset_device); @@ -428,6 +437,9 @@ int register_virtio_device(struct virtio_device *dev) dev->config_enabled = false; dev->config_change_pending = false; + INIT_LIST_HEAD(&dev->vqs); + spin_lock_init(&dev->vqs_list_lock); + /* We always start by resetting the device, in case a previous * driver messed it up. This also tests that code path a little. */ virtio_reset_device(dev); @@ -435,9 +447,6 @@ int register_virtio_device(struct virtio_device *dev) /* Acknowledge that we've seen the device. */ virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE); - INIT_LIST_HEAD(&dev->vqs); - spin_lock_init(&dev->vqs_list_lock); - /* * device_add() causes the bus infrastructure to look for a matching * driver. diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index 5b7df7c455f0..9dfad2890d7a 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -1690,7 +1690,7 @@ static struct virtqueue *vring_create_virtqueue_packed( vq->we_own_ring = true; vq->notify = notify; vq->weak_barriers = weak_barriers; - vq->broken = false; + vq->broken = true; vq->last_used_idx = 0; vq->event_triggered = false; vq->num_added = 0; @@ -2136,8 +2136,11 @@ irqreturn_t vring_interrupt(int irq, void *_vq) return IRQ_NONE; } - if (unlikely(vq->broken)) - return IRQ_HANDLED; + if (unlikely(vq->broken)) { + dev_warn_once(&vq->vq.vdev->dev, + "virtio vring IRQ raised before DRIVER_OK"); + return IRQ_NONE; + } /* Just a hint for performance: so it's ok that this can be racy! */ if (vq->event) @@ -2179,7 +2182,7 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index, vq->we_own_ring = false; vq->notify = notify; vq->weak_barriers = weak_barriers; - vq->broken = false; + vq->broken = true; vq->last_used_idx = 0; vq->event_triggered = false; vq->num_added = 0; diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h index d8a2340f928e..23f1694cdbd5 100644 --- a/include/linux/virtio_config.h +++ b/include/linux/virtio_config.h @@ -256,6 +256,18 @@ void virtio_device_ready(struct virtio_device *dev) unsigned status = dev->config->get_status(dev); BUG_ON(status & VIRTIO_CONFIG_S_DRIVER_OK); + + /* + * The virtio_synchronize_cbs() makes sure vring_interrupt() + * will see the driver specific setup if it sees vq->broken + * as false. + */ + virtio_synchronize_cbs(dev); + __virtio_unbreak_device(dev); + /* + * The transport is expected ensure the visibility of + * vq->broken before setting VIRTIO_CONFIG_S_DRIVER_OK. + */ dev->config->set_status(dev, status | VIRTIO_CONFIG_S_DRIVER_OK); } -- 2.25.1 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
next prev parent reply other threads:[~2022-05-07 7:21 UTC|newest] Thread overview: 71+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-05-07 7:19 [PATCH V4 0/9] rework on the IRQ hardening of virtio Jason Wang 2022-05-07 7:19 ` Jason Wang 2022-05-07 7:19 ` [PATCH V4 1/9] virtio: use virtio_device_ready() in virtio_device_restore() Jason Wang 2022-05-07 7:19 ` Jason Wang 2022-05-09 15:22 ` Cornelia Huck 2022-05-09 15:22 ` Cornelia Huck 2022-05-10 1:50 ` Jason Wang 2022-05-10 1:50 ` Jason Wang 2022-05-07 7:19 ` [PATCH V4 2/9] virtio: use virtio_reset_device() when possible Jason Wang 2022-05-07 7:19 ` Jason Wang 2022-05-07 7:19 ` [PATCH V4 3/9] virtio: introduce config op to synchronize vring callbacks Jason Wang 2022-05-07 7:19 ` Jason Wang 2022-05-09 15:24 ` Cornelia Huck 2022-05-09 15:24 ` Cornelia Huck 2022-05-07 7:19 ` [PATCH V4 4/9] virtio-pci: implement synchronize_cbs() Jason Wang 2022-05-07 7:19 ` Jason Wang 2022-05-09 15:26 ` Cornelia Huck 2022-05-09 15:26 ` Cornelia Huck 2022-05-07 7:19 ` [PATCH V4 5/9] virtio-mmio: " Jason Wang 2022-05-07 7:19 ` Jason Wang 2022-05-09 15:34 ` Cornelia Huck 2022-05-09 15:34 ` Cornelia Huck 2022-05-07 7:19 ` [PATCH V4 6/9] virtio-ccw: " Jason Wang 2022-05-07 7:19 ` Jason Wang 2022-05-10 11:27 ` Michael S. Tsirkin 2022-05-10 11:27 ` Michael S. Tsirkin 2022-05-11 2:41 ` Jason Wang 2022-05-11 2:41 ` Jason Wang 2022-05-11 8:17 ` Cornelia Huck 2022-05-11 8:17 ` Cornelia Huck 2022-05-11 8:58 ` Jason Wang 2022-05-11 8:58 ` Jason Wang 2022-05-11 9:13 ` Cornelia Huck 2022-05-11 9:13 ` Cornelia Huck 2022-05-11 9:28 ` Jason Wang 2022-05-11 9:28 ` Jason Wang 2022-05-11 14:52 ` Vineeth Vijayan 2022-05-12 3:29 ` Jason Wang 2022-05-12 3:29 ` Jason Wang 2022-05-07 7:19 ` [PATCH V4 7/9] virtio: allow to unbreak virtqueue Jason Wang 2022-05-07 7:19 ` Jason Wang 2022-05-07 7:19 ` Jason Wang [this message] 2022-05-07 7:19 ` [PATCH V4 8/9] virtio: harden vring IRQ Jason Wang 2022-05-10 11:32 ` Michael S. Tsirkin 2022-05-10 11:32 ` Michael S. Tsirkin 2022-05-11 2:40 ` Jason Wang 2022-05-11 2:40 ` Jason Wang 2022-05-11 8:44 ` Cornelia Huck 2022-05-11 8:44 ` Cornelia Huck 2022-05-11 9:27 ` Jason Wang 2022-05-11 9:27 ` Jason Wang 2022-05-11 12:49 ` Halil Pasic 2022-05-11 12:49 ` Halil Pasic 2022-05-12 3:27 ` Jason Wang 2022-05-12 3:27 ` Jason Wang 2022-05-07 7:19 ` [PATCH V4 9/9] virtio: use WARN_ON() to warning illegal status value Jason Wang 2022-05-07 7:19 ` Jason Wang 2022-05-10 9:29 ` [PATCH V4 0/9] rework on the IRQ hardening of virtio Cornelia Huck 2022-05-10 9:29 ` Cornelia Huck 2022-05-11 2:22 ` Jason Wang 2022-05-11 2:22 ` Jason Wang 2022-05-11 14:01 ` Halil Pasic 2022-05-11 14:01 ` Halil Pasic 2022-05-12 3:31 ` Jason Wang 2022-05-12 3:31 ` Jason Wang 2022-05-16 11:20 ` Halil Pasic 2022-05-16 11:20 ` Halil Pasic 2022-05-16 14:25 ` Michael S. Tsirkin 2022-05-16 14:25 ` Michael S. Tsirkin 2022-05-17 1:00 ` Jason Wang 2022-05-17 1:00 ` Jason Wang
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20220507071954.14455-9-jasowang@redhat.com \ --to=jasowang@redhat.com \ --cc=cohuck@redhat.com \ --cc=eperezma@redhat.com \ --cc=linux-kernel@vger.kernel.org \ --cc=lulu@redhat.com \ --cc=maz@kernel.org \ --cc=mst@redhat.com \ --cc=pasic@linux.ibm.com \ --cc=paulmck@kernel.org \ --cc=peterz@infradead.org \ --cc=sgarzare@redhat.com \ --cc=tglx@linutronix.de \ --cc=virtualization@lists.linux-foundation.org \ --cc=xuanzhuo@linux.alibaba.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.