[PATCH v2] virtio: console: fix lost wakeup when device is written and polled

[PATCH v2] virtio: console: fix lost wakeup when device is written and polled

[Lorenz Bauer]

A process issuing blocking writes to a virtio console may get stuck
indefinitely if another thread polls the device. Here is how to trigger
the bug:

- Thread A writes to the port until the virtqueue is full.
- Thread A calls wait_port_writable() and goes to sleep, waiting on
  port->waitqueue.
- The host processes some of the write, marks buffers as used and raises
  an interrupt.
- Before the interrupt is serviced, thread B executes port_fops_poll().
  This calls reclaim_consumed_buffers() via will_write_block() and
  consumes all used buffers.
- The interrupt is serviced. vring_interrupt() finds no used buffers
  via more_used() and returns without waking port->waitqueue.
- Thread A is still in wait_event(port->waitqueue), waiting for a
  wakeup that never arrives.

The crux is that invoking reclaim_consumed_buffers() may cause
vring_interrupt() to omit wakeups.

Fix this by calling reclaim_consumed_buffers() in out_int() before
waking. This is similar to the call to discard_port_data() in
in_intr() which also frees buffer from a non-sleepable context.
This in turn guarantees that port->outvq_full is up to date when
handling polling. Since in_intr() already populates port->inbuf we
use that to avoid changing reader state.

Cc: stable@vger.kernel.org
Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
---
As far as I can tell all currently maintained stable series kernels need
this commit. Applies and builds cleanly on 5.10.247, verified to fix
the issue.
---
Changes in v2:
- Call reclaim_consumed_buffers() in out_intr instead of
  issuing another wake.
- Link to v1: https://lore.kernel.org/r/20251215-virtio-console-lost-wakeup-v1-1-79a5c57815e7@isovalent.com
---
 drivers/char/virtio_console.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 088182e54debd6029ea2c2a5542d7a28500e67b8..351e445da35e25910671615a8ecc79165dfc66b7 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -971,10 +971,17 @@ static __poll_t port_fops_poll(struct file *filp, poll_table *wait)
 		return EPOLLHUP;
 	}
 	ret = 0;
-	if (!will_read_block(port))
+
+	spin_lock(&port->inbuf_lock);
+	if (port->inbuf)
 		ret |= EPOLLIN | EPOLLRDNORM;
-	if (!will_write_block(port))
+	spin_unlock(&port->inbuf_lock);
+
+	spin_lock(&port->outvq_lock);
+	if (!port->outvq_full)
 		ret |= EPOLLOUT;
+	spin_unlock(&port->outvq_lock);
+
 	if (!port->host_connected)
 		ret |= EPOLLHUP;
 
@@ -1698,6 +1705,7 @@ static void flush_bufs(struct virtqueue *vq, bool can_sleep)
 static void out_intr(struct virtqueue *vq)
 {
 	struct port *port;
+	unsigned long flags;
 
 	port = find_port_by_vq(vq->vdev->priv, vq);
 	if (!port) {
@@ -1705,6 +1713,10 @@ static void out_intr(struct virtqueue *vq)
 		return;
 	}
 
+	spin_lock_irqsave(&port->outvq_lock, flags);
+	reclaim_consumed_buffers(port);
+	spin_unlock_irqrestore(&port->outvq_lock, flags);
+
 	wake_up_interruptible(&port->waitqueue);
 }
 

---
base-commit: d358e5254674b70f34c847715ca509e46eb81e6f
change-id: 20251215-virtio-console-lost-wakeup-0f566c5cd35f

Best regards,
-- 
Lorenz Bauer <lmb@isovalent.com>

Re: [PATCH v2] virtio: console: fix lost wakeup when device is written and polled

[Arnd Bergmann]

On Mon, Dec 22, 2025, at 17:04, Lorenz Bauer wrote:
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -971,10 +971,17 @@ static __poll_t port_fops_poll(struct file *filp, 
> poll_table *wait)
>  		return EPOLLHUP;
>  	}
>  	ret = 0;
> -	if (!will_read_block(port))
> +
> +	spin_lock(&port->inbuf_lock);
> +	if (port->inbuf)

As far as I can tell, you got the interrupt flag handling wrong
in both places: port_fops_poll() is called with interrupts
enabled, so you have to use spin_lock_irq() to block the
interrupt from hanging.

> @@ -1705,6 +1713,10 @@ static void out_intr(struct virtqueue *vq)
>  		return;
>  	}
> 
> +	spin_lock_irqsave(&port->outvq_lock, flags);
> +	reclaim_consumed_buffers(port);
> +	spin_unlock_irqrestore(&port->outvq_lock, flags);
> +
>  	wake_up_interruptible(&port->waitqueue);

The callback seems to always be called with interrupts
disabled(), so here it's safe to use spin_lock() instead
of spin_lock_irqsave().

     Arnd

Re: [PATCH v2] virtio: console: fix lost wakeup when device is written and polled

[Lorenz Bauer]

On Mon, Dec 22, 2025 at 5:13 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Mon, Dec 22, 2025, at 17:04, Lorenz Bauer wrote:
> > --- a/drivers/char/virtio_console.c
> > +++ b/drivers/char/virtio_console.c
> > @@ -971,10 +971,17 @@ static __poll_t port_fops_poll(struct file *filp,
> > poll_table *wait)
> >               return EPOLLHUP;
> >       }
> >       ret = 0;
> > -     if (!will_read_block(port))
> > +
> > +     spin_lock(&port->inbuf_lock);
> > +     if (port->inbuf)
>
> As far as I can tell, you got the interrupt flag handling wrong
> in both places: port_fops_poll() is called with interrupts
> enabled, so you have to use spin_lock_irq() to block the
> interrupt from hanging.

Ack.

> > @@ -1705,6 +1713,10 @@ static void out_intr(struct virtqueue *vq)
> >               return;
> >       }
> >
> > +     spin_lock_irqsave(&port->outvq_lock, flags);
> > +     reclaim_consumed_buffers(port);
> > +     spin_unlock_irqrestore(&port->outvq_lock, flags);
> > +
> >       wake_up_interruptible(&port->waitqueue);
>
> The callback seems to always be called with interrupts
> disabled(), so here it's safe to use spin_lock() instead
> of spin_lock_irqsave().

This is pretty much just copied from in_intr which also uses _irqsave.
I think it makes sense to stick to that for consistency's sake. What
do you think?

Re: [PATCH v2] virtio: console: fix lost wakeup when device is written and polled

[Arnd Bergmann]

On Mon, Dec 22, 2025, at 17:23, Lorenz Bauer wrote:
> On Mon, Dec 22, 2025 at 5:13 PM Arnd Bergmann <arnd@arndb.de> wrote:
>> On Mon, Dec 22, 2025, at 17:04, Lorenz Bauer wrote:
>> > @@ -1705,6 +1713,10 @@ static void out_intr(struct virtqueue *vq)
>> >               return;
>> >       }
>> >
>> > +     spin_lock_irqsave(&port->outvq_lock, flags);
>> > +     reclaim_consumed_buffers(port);
>> > +     spin_unlock_irqrestore(&port->outvq_lock, flags);
>> > +
>> >       wake_up_interruptible(&port->waitqueue);
>>
>> The callback seems to always be called with interrupts
>> disabled(), so here it's safe to use spin_lock() instead
>> of spin_lock_irqsave().
>
> This is pretty much just copied from in_intr which also uses _irqsave.
> I think it makes sense to stick to that for consistency's sake. What
> do you think?

The usual rule is that you must use spin_lock_irqsave() if
the function can be called from either interrupt or non-interrupt
context. It's also safe to be used if you are not sure.

However, in interrupt handlers you usually want to use the
plain spin_lock() both for efficiency reasons and to document
the calling conventions.

     Arnd