Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Stefan Hajnoczi <stefanha@redhat.com>
To:     virtualization@lists.linux-foundation.org
Cc:     Gerd Hoffmann <kraxel@redhat.com>, linux-kernel@vger.kernel.org,
        David Airlie <airlied@linux.ie>, vgoyal@redhat.com,
        jasowang@redhat.com, "Michael S. Tsirkin" <mst@redhat.com>,
        Stefan Hajnoczi <stefanha@redhat.com>
Subject: [RFC PATCH 0/1] virtio: false unhandled irqs from vring_interrupt()
Date:   Tue, 24 Aug 2021 11:59:43 +0100
Message-Id: <20210824105944.172659-1-stefanha@redhat.com>
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

While investigating an unhandled irq from vring_interrupt() with virtiofs I=
=0D
stumbled onto a possible race that also affects virtio_gpu. This theory is=
=0D
based on code inspection and hopefully you can point out something that mak=
es=0D
this a non-issue in practice :).=0D
=0D
The vring_interrupt() function returns IRQ_NONE when an MSI-X interrupt is=
=0D
taken with no used (completed) buffers in the virtqueue. The kernel disable=
s=0D
the irq and the driver is no longer receives irqs when this happens:=0D
=0D
  irq 77: nobody cared (try booting with the "irqpoll" option)=0D
  ...=0D
  handlers:=0D
  [<00000000a40a49bb>] vring_interrupt=0D
  Disabling IRQ #77=0D
=0D
Consider the following:=0D
=0D
1. An virtiofs irq is handled and the virtio_fs_requests_done_work() work=0D
   function is scheduled to dequeue used buffers:=0D
   vring_interrupt() -> virtio_fs_vq_done() -> schedule_work()=0D
=0D
2. The device adds more used requests and just before...=0D
=0D
3. ...virtio_fs_requests_done_work() empties the virtqueue with=0D
   virtqueue_get_buf().=0D
=0D
4. The device raises the irq and vring_interrupt() is called after=0D
   virtio_fs_requests_done_work emptied the virtqueue:=0D
=0D
   irqreturn_t vring_interrupt(int irq, void *_vq)=0D
   {=0D
       struct vring_virtqueue *vq =3D to_vvq(_vq);=0D
=0D
       if (!more_used(vq)) {=0D
           pr_debug("virtqueue interrupt with no work for %p\n", vq);=0D
           return IRQ_NONE;=0D
           ^^^^^^^^^^^^^^^^=0D
=0D
I have included a patch that switches virtiofs from spin_lock() to=0D
spin_lock_irqsave() to prevent vring_interrupt() from running while the=0D
virtqueue is processed from a work function.=0D
=0D
virtio_gpu has a similar case where virtio_gpu_dequeue_ctrl_func() and=0D
virtio_gpu_dequeue_cursor_func() work functions only use spin_lock().=0D
I think this can result in the same false unhandled irq problem as virtiofs=
.=0D
=0D
This race condition could in theory affect all drivers. The VIRTIO=0D
specification says:=0D
=0D
  Neither of these notification suppression methods are reliable, as they a=
re=0D
  not synchronized with the device, but they serve as useful optimizations.=
=0D
=0D
If virtqueue_disable_cb() is just a hint and might not disable virtqueue ir=
qs=0D
then virtio_net and other drivers have a problem because because an irq cou=
ld=0D
be raised while the driver is dequeuing used buffers. I think we haven't se=
en=0D
this because software VIRTIO devices honor virtqueue_disable_cb(). Hardware=
=0D
devices might cache the value and not disable notifications for some time..=
.=0D
=0D
Have I missed something?=0D
=0D
The virtiofs patch I attached is being stress tested to see if the unhandle=
d=0D
irqs still occur.=0D
=0D
Stefan Hajnoczi (1):=0D
  fuse: disable local irqs when processing vq completions=0D
=0D
 fs/fuse/virtio_fs.c | 15 ++++++++++-----=0D
 1 file changed, 10 insertions(+), 5 deletions(-)=0D
=0D
-- =0D
2.31.1=0D
=0D