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