[PATCH] chardev/char-io: Fix polling by not removing polls when buffers are full

[PATCH] chardev/char-io: Fix polling by not removing polls when buffers are full

[Iris Johnson]

Currently, the chardev backend code will prepare for IO polling to occur
by potentially adding or removing a watch of the backing channel for the
chardev. The chardev poll is added if the fd_can_read() function reports
more than 0 byte of buffer space, if a poll handler is already setup and
the bufer is now empty, the poll handler is removed.

This causes a bug where the device buffer becomes ready, but the poll is
blocking on a sleep (potentially forever), because the buffer is small
and fills up immediately, while the backend channel has more data. This
leads to a stall condition or potentially a deadlock in the guest.

The guest is looping, waiting for data to be reported as ready to read,
the host sees that the buffer is ready for reading and adds the poll,
the poll returns since data is available and data is made available to
the guest. Before the guest code is able to retrieve the data and clear
the full buffer, the poll code runs again, sees that the buffer is now
full, and removes the poll. At this point only a timeout from another
polled source, or another source having it's poll complete will result
in the loop running again to see that the buffer is now ready and to
add the poll again.

We solve this issue by removing the logic that removes the poll, keeping
the existing logic to only create the poll once there's space for the
first read.

Buglink: https://bugs.launchpad.net/qemu/+bug/1913341
Signed-off-by: Iris Johnson <iris@modwiz.com>
---
 chardev/char-io.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/chardev/char-io.c b/chardev/char-io.c
index 8ced184160..fa9e222f78 100644
--- a/chardev/char-io.c
+++ b/chardev/char-io.c
@@ -50,16 +50,14 @@ static gboolean io_watch_poll_prepare(GSource *source,
         return FALSE;
     }
 
-    if (now_active) {
+    if (now_active && !was_active) {
         iwp->src = qio_channel_create_watch(
             iwp->ioc, G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL);
         g_source_set_callback(iwp->src, iwp->fd_read, iwp->opaque, NULL);
         g_source_add_child_source(source, iwp->src);
         g_source_unref(iwp->src);
-    } else {
-        g_source_remove_child_source(source, iwp->src);
-        iwp->src = NULL;
     }
+
     return FALSE;
 }
 
-- 
2.25.1

Re: [PATCH] chardev/char-io: Fix polling by not removing polls when buffers are full

[Marc-André Lureau]

On Fri, Jan 29, 2021 at 11:57 PM Iris Johnson <iris@modwiz.com> wrote:
>
> Currently, the chardev backend code will prepare for IO polling to occur
> by potentially adding or removing a watch of the backing channel for the
> chardev. The chardev poll is added if the fd_can_read() function reports
> more than 0 byte of buffer space, if a poll handler is already setup and
> the bufer is now empty, the poll handler is removed.
>
> This causes a bug where the device buffer becomes ready, but the poll is
> blocking on a sleep (potentially forever), because the buffer is small
> and fills up immediately, while the backend channel has more data. This
> leads to a stall condition or potentially a deadlock in the guest.
>
> The guest is looping, waiting for data to be reported as ready to read,
> the host sees that the buffer is ready for reading and adds the poll,
> the poll returns since data is available and data is made available to
> the guest. Before the guest code is able to retrieve the data and clear
> the full buffer, the poll code runs again, sees that the buffer is now
> full, and removes the poll. At this point only a timeout from another
> polled source, or another source having it's poll complete will result
> in the loop running again to see that the buffer is now ready and to
> add the poll again.
>
> We solve this issue by removing the logic that removes the poll, keeping
> the existing logic to only create the poll once there's space for the
> first read.
>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1913341
> Signed-off-by: Iris Johnson <iris@modwiz.com>
> ---
>  chardev/char-io.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/chardev/char-io.c b/chardev/char-io.c
> index 8ced184160..fa9e222f78 100644
> --- a/chardev/char-io.c
> +++ b/chardev/char-io.c
> @@ -50,16 +50,14 @@ static gboolean io_watch_poll_prepare(GSource *source,
>          return FALSE;
>      }
>
> -    if (now_active) {
> +    if (now_active && !was_active) {
>          iwp->src = qio_channel_create_watch(
>              iwp->ioc, G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL);
>          g_source_set_callback(iwp->src, iwp->fd_read, iwp->opaque, NULL);
>          g_source_add_child_source(source, iwp->src);
>          g_source_unref(iwp->src);
> -    } else {
> -        g_source_remove_child_source(source, iwp->src);
> -        iwp->src = NULL;
>      }

I don't think this works well enough: if the source isn't removed, but
fd_can_read() returns 0, there is a potential busy loop on the next
fd_read().

My understanding is that if data is read from the frontend, the loop
will be re-entered and io_watch_poll_prepare will set the callback
again.

Could you provide a simple use-case or reproducer where we can
evaluate how your patch improves the situation?

thanks

-- 
Marc-André Lureau

Re: [PATCH] chardev/char-io: Fix polling by not removing polls when buffers are full

[Iris Johnson]

[-- Attachment #1: Type: text/plain, Size: 2041 bytes --]

>
> > I don't think this works well enough: if the source isn't removed, but

> fd_can_read() returns 0, there is a potential busy loop on the next
> > fd_read().
>

This shouldn't happen. The sources all get set to non-blocking mode, the
only
blocking code is the poll itself. If fd_can_read() returns 0, then the next
time
fd_read() is called, it will attempt to read zero bytes. The backend logic
checks the
results of the same method that fd_can_read() calls and sets its read size
to that amount, in the case of a full buffer it will read 0 bytes and
return.

> My understanding is that if data is read from the frontend, the loop
> > will be re-entered and io_watch_poll_prepare will set the callback
> > again.
>

This just doesn't happen. The issue is that between the poll being added
(and
some but not all data being read) and the frontend code getting triggered by
the guest, the IO loop runs again and the poll is removed, it then runs
again
with the poll removed (since the poll is removed during setup) and it's now
just going to block because the input fd in question has been "temporarily
removed". Except that nothing in the fd set it polls on is now connected to
the guest clearing the buffer.

Meanwhile the guest reads the data during what can be a potentially
infinite block (if nothing else sets the timeout, in my case something
in the uart peripheral sets a 1000ms timeout so I could read a byte
every second or so in the guest). The guest will now be spinning until
the poll is re-added, meanwhile the poll is blocking on a timeout or another
fd becoming ready because the buffers are small, the fd in question has
already been removed from the set by the time the guest has a chance
to clear the buffer.


> > Could you provide a simple use-case or reproducer where we can
> > evaluate how your patch improves the situation?


I can do this, but I don't have anything ready immediately, my test case
isn't
ideal for others to reproduce. But I can attach one later today when I have
that done.

Thanks,
Iris Johnson

[-- Attachment #2: Type: text/html, Size: 3027 bytes --]

Re: [PATCH] chardev/char-io: Fix polling by not removing polls when buffers are full

[Iris Johnson]

[-- Attachment #1: Type: text/plain, Size: 2958 bytes --]

I was working on preparing this test case for the virt machine (I've been
using a test machine based around the exynos UART).

While looking at the uart code for the virt to find which serial peripheral
it uses (and the register layout), I noticed that the pl011
uart code calls qemu_chr_fe_accept_input, in fact it looks like every uart
except the exynos UART that I was using has this call.

Somehow while I was debugging this issue I missed that call. (In fact
adding a call like that was my backup patch to this one),
In light of the discovery I'll send a follow up patch that adds this
functionality into the exynos4210_uart code instead of modifying
the chardev handling itself.

You can safely disregard this patch. Sorry about the confusion.

Iris

On Sat, Jan 30, 2021 at 9:54 AM Iris Johnson <iris@modwiz.com> wrote:

> > I don't think this works well enough: if the source isn't removed, but
>
> > fd_can_read() returns 0, there is a potential busy loop on the next
>> > fd_read().
>>
>
> This shouldn't happen. The sources all get set to non-blocking mode, the
> only
> blocking code is the poll itself. If fd_can_read() returns 0, then the
> next time
> fd_read() is called, it will attempt to read zero bytes. The backend logic
> checks the
> results of the same method that fd_can_read() calls and sets its read size
> to that amount, in the case of a full buffer it will read 0 bytes and
> return.
>
> > My understanding is that if data is read from the frontend, the loop
>> > will be re-entered and io_watch_poll_prepare will set the callback
>> > again.
>>
>
> This just doesn't happen. The issue is that between the poll being added
> (and
> some but not all data being read) and the frontend code getting triggered
> by
> the guest, the IO loop runs again and the poll is removed, it then runs
> again
> with the poll removed (since the poll is removed during setup) and it's now
> just going to block because the input fd in question has been "temporarily
> removed". Except that nothing in the fd set it polls on is now connected to
> the guest clearing the buffer.
>
> Meanwhile the guest reads the data during what can be a potentially
> infinite block (if nothing else sets the timeout, in my case something
> in the uart peripheral sets a 1000ms timeout so I could read a byte
> every second or so in the guest). The guest will now be spinning until
> the poll is re-added, meanwhile the poll is blocking on a timeout or
> another
> fd becoming ready because the buffers are small, the fd in question has
> already been removed from the set by the time the guest has a chance
> to clear the buffer.
>
>
>> > Could you provide a simple use-case or reproducer where we can
>> > evaluate how your patch improves the situation?
>
>
> I can do this, but I don't have anything ready immediately, my test case
> isn't
> ideal for others to reproduce. But I can attach one later today when I
> have that done.
>
> Thanks,
> Iris Johnson
>

[-- Attachment #2: Type: text/html, Size: 4265 bytes --]