[PATCH 0/3 for 9.0] Fix TLS support for chardevs and incoming data loss on EOF

[PATCH 0/3 for 9.0] Fix TLS support for chardevs and incoming data loss on EOF

[Daniel P. Berrangé]

This fixes a problem with TLS support on chardevs that Thomas has
previously attempted to deal with:

  https://lists.nongnu.org/archive/html/qemu-devel/2024-02/msg06915.html

Unfortunately that fix caused unexpected side effects that resulted
in premature termination of the TLS connection. See patch 2 for
details.

I've since identified the root cause of the problem that Thomas was
trying to fix - bad assumptions about GSource 'prepare' functions
always being run. See patch 3 for details.

Patch 3 re-exposed a bug we've know about for a while whereby incoming
data on chardevs is sometimes discarded when POLLHUP is reported at the
same time. This required patch 1 to be applied before doing the revert
in patch 3, otherwise test-char would now very frequently fail.

So we get 2 bug fixes for the price of one :-)

Daniel P. Berrangé (3):
  chardev: lower priority of the HUP GSource in socket chardev
  Revert "chardev/char-socket: Fix TLS io channels sending too much data
    to the backend"
  Revert "chardev: use a child source for qio input source"

 chardev/char-io.c     | 55 +++++++++++++++++++++++++++++++++++++++----
 chardev/char-socket.c | 22 ++++++++++++++---
 2 files changed, 69 insertions(+), 8 deletions(-)

-- 
2.43.0

[PATCH 1/3 for 9.0] chardev: lower priority of the HUP GSource in socket chardev

[Daniel P. Berrangé]

The socket chardev often has 2 GSource object registered against the
same FD. One is registered all the time and is just intended to handle
POLLHUP events, while the other gets registered & unregistered on the
fly as the frontend is ready to receive more data or not.

It is very common for poll() to signal a POLLHUP event at the same time
as there is pending incoming data from the disconnected client. It is
therefore essential to process incoming data prior to processing HUP.
The problem with having 2 GSource on the same FD is that there is no
guaranteed ordering of execution between them, so the chardev code may
process HUP first and thus discard data.

This failure scenario is non-deterministic but can be seen fairly
reliably by reverting a7077b8e354d90fec26c2921aa2dea85b90dff90, and
then running 'tests/unit/test-char', which will sometimes fail with
missing data.

Ideally QEMU would only have 1 GSource, but that's a complex code
refactoring job. The next best solution is to try to ensure ordering
between the 2 GSource objects. This can be achieved by lowering the
priority of the HUP GSource, so that it is never dispatched if the
main GSource is also ready to dispatch. Counter-intuitively, lowering
the priority of a GSource is done by raising its priority number.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 chardev/char-socket.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 8a0406cc1e..2c4dffc0e6 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -601,6 +601,22 @@ static void update_ioc_handlers(SocketChardev *s)
 
     remove_hup_source(s);
     s->hup_source = qio_channel_create_watch(s->ioc, G_IO_HUP);
+    /*
+     * poll() is liable to return POLLHUP even when there is
+     * still incoming data available to read on the FD. If
+     * we have the hup_source at the same priority as the
+     * main io_add_watch_poll GSource, then we might end up
+     * processing the POLLHUP event first, closing the FD,
+     * and as a result silently discard data we should have
+     * read.
+     *
+     * By setting the hup_source to G_PRIORITY_DEFAULT + 1,
+     * we ensure that io_add_watch_poll GSource will always
+     * be dispatched first, thus guaranteeing we will be
+     * able to process all incoming data before closing the
+     * FD
+     */
+    g_source_set_priority(s->hup_source, G_PRIORITY_DEFAULT + 1);
     g_source_set_callback(s->hup_source, (GSourceFunc)tcp_chr_hup,
                           chr, NULL);
     g_source_attach(s->hup_source, chr->gcontext);
-- 
2.43.0

[PATCH 2/3 for 9.0] Revert "chardev/char-socket: Fix TLS io channels sending too much data to the backend"

[Daniel P. Berrangé]

This commit results in unexpected termination of the TLS connection.
When 'fd_can_read' returns 0, the code goes on to pass a zero length
buffer to qio_channel_read. The TLS impl calls into gnutls_recv()
with this zero length buffer, at which point GNUTLS returns an error
GNUTLS_E_INVALID_REQUEST. This is treated as fatal by QEMU's TLS code
resulting in the connection being torn down by the chardev.

Simply skipping the qio_channel_read when the buffer length is zero
is also not satisfactory, as it results in a high CPU burn busy loop
massively slowing QEMU's functionality.

The proper solution is to avoid tcp_chr_read being called at all
unless the frontend is able to accept more data. This will be done
in a followup commit.

This reverts commit 1907f4d149c3589ade641423c6a33fd7598fa4d3.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 chardev/char-socket.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 2c4dffc0e6..812d7aa38a 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -496,9 +496,9 @@ static gboolean tcp_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
         s->max_size <= 0) {
         return TRUE;
     }
-    len = tcp_chr_read_poll(opaque);
-    if (len > sizeof(buf)) {
-        len = sizeof(buf);
+    len = sizeof(buf);
+    if (len > s->max_size) {
+        len = s->max_size;
     }
     size = tcp_chr_recv(chr, (void *)buf, len);
     if (size == 0 || (size == -1 && errno != EAGAIN)) {
-- 
2.43.0

[PATCH 3/3 for 9.0] Revert "chardev: use a child source for qio input source"

[Daniel P. Berrangé]

This reverts commit a7077b8e354d90fec26c2921aa2dea85b90dff90,
and add comments to explain why child sources cannot be used.

When a GSource is added as a child of another GSource, if its
'prepare' function indicates readiness, then the parent's
'prepare' function will never be run. The io_watch_poll_prepare
absolutely *must* be run on every iteration of the main loop,
to ensure that the chardev backend doesn't feed data to the
frontend that it is unable to consume.

At the time a7077b8e354d90fec26c2921aa2dea85b90dff90 was made,
all the child GSource impls were relying on poll'ing an FD,
so their 'prepare' functions would never indicate readiness
ahead of poll() being invoked. So the buggy behaviour was
not noticed and lay dormant.

Relatively recently the QIOChannelTLS impl introduced a
level 2 child GSource, which checks with GNUTLS whether it
has cached any data that was decoded but not yet consumed:

  commit ffda5db65aef42266a5053a4be34515106c4c7ee
  Author: Antoine Damhet <antoine.damhet@shadow.tech>
  Date:   Tue Nov 15 15:23:29 2022 +0100

    io/channel-tls: fix handling of bigger read buffers

    Since the TLS backend can read more data from the underlying QIOChannel
    we introduce a minimal child GSource to notify if we still have more
    data available to be read.

    Signed-off-by: Antoine Damhet <antoine.damhet@shadow.tech>
    Signed-off-by: Charles Frey <charles.frey@shadow.tech>
    Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

With this, it is now quite common for the 'prepare' function
on a QIOChannelTLS GSource to indicate immediate readiness,
bypassing the parent GSource 'prepare' function. IOW, the
critical 'io_watch_poll_prepare' is being skipped on some
iterations of the main loop. As a result chardev frontend
asserts are now being triggered as they are fed data they
are not ready to consume.

A reproducer is as follows:

 * In terminal 1 run a GNUTLS *echo* server

   $ gnutls-serv --echo \
                 --x509cafile ca-cert.pem \
                 --x509keyfile server-key.pem \
		 --x509certfile server-cert.pem \
		 -p 9000

 * In terminal 2 run a QEMU guest

   $ qemu-system-s390x \
       -nodefaults \
       -display none \
       -object tls-creds-x509,id=tls0,dir=$PWD,endpoint=client \
       -chardev socket,id=con0,host=localhost,port=9000,tls-creds=tls0 \
       -device sclpconsole,chardev=con0 \
       -hda Fedora-Cloud-Base-39-1.5.s390x.qcow2

After the previous patch revert, but before this patch revert,
this scenario will crash:

  qemu-system-s390x: ../hw/char/sclpconsole.c:73: chr_read: Assertion
  `size <= SIZE_BUFFER_VT220 - scon->iov_data_len' failed.

This assert indicates that 'tcp_chr_read' was called without
'tcp_chr_read_poll' having first been checked for ability to
receive more data

QEMU's use of a 'prepare' function to create/delete another
GSource is rather a hack and not normally the kind of thing that
is expected to be done by a GSource. There is no mechanism to
force GLib to always run the 'prepare' function of a parent
GSource. The best option is to simply not use the child source
concept, and go back to the functional approach previously
relied on.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 chardev/char-io.c | 55 ++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 50 insertions(+), 5 deletions(-)

diff --git a/chardev/char-io.c b/chardev/char-io.c
index 4451128cba..3c725f530b 100644
--- a/chardev/char-io.c
+++ b/chardev/char-io.c
@@ -33,6 +33,7 @@ typedef struct IOWatchPoll {
     IOCanReadHandler *fd_can_read;
     GSourceFunc fd_read;
     void *opaque;
+    GMainContext *context;
 } IOWatchPoll;
 
 static IOWatchPoll *io_watch_poll_from_source(GSource *source)
@@ -50,28 +51,58 @@ static gboolean io_watch_poll_prepare(GSource *source,
         return FALSE;
     }
 
+    /*
+     * We do not register the QIOChannel watch as a child GSource.
+     * The 'prepare' function on the parent GSource will be
+     * skipped if a child GSource's 'prepare' function indicates
+     * readiness. We need this prepare function be guaranteed
+     * to run on *every* iteration of the main loop, because
+     * it is critical to ensure we remove the QIOChannel watch
+     * if 'fd_can_read' indicates the frontend cannot receive
+     * more data.
+     */
     if (now_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);
+        g_source_attach(iwp->src, iwp->context);
     } else {
-        g_source_remove_child_source(source, iwp->src);
+        g_source_destroy(iwp->src);
+        g_source_unref(iwp->src);
         iwp->src = NULL;
     }
     return FALSE;
 }
 
+static gboolean io_watch_poll_check(GSource *source)
+{
+    return FALSE;
+}
+
 static gboolean io_watch_poll_dispatch(GSource *source, GSourceFunc callback,
                                        gpointer user_data)
 {
-    return G_SOURCE_CONTINUE;
+    abort();
+}
+
+static void io_watch_poll_finalize(GSource *source)
+{
+    /* Due to a glib bug, removing the last reference to a source
+     * inside a finalize callback causes recursive locking (and a
+     * deadlock).  This is not a problem inside other callbacks,
+     * including dispatch callbacks, so we call io_remove_watch_poll
+     * to remove this source.  At this point, iwp->src must
+     * be NULL, or we would leak it.
+     */
+    IOWatchPoll *iwp = io_watch_poll_from_source(source);
+    assert(iwp->src == NULL);
 }
 
 static GSourceFuncs io_watch_poll_funcs = {
     .prepare = io_watch_poll_prepare,
+    .check = io_watch_poll_check,
     .dispatch = io_watch_poll_dispatch,
+    .finalize = io_watch_poll_finalize,
 };
 
 GSource *io_add_watch_poll(Chardev *chr,
@@ -91,6 +122,7 @@ GSource *io_add_watch_poll(Chardev *chr,
     iwp->ioc = ioc;
     iwp->fd_read = (GSourceFunc) fd_read;
     iwp->src = NULL;
+    iwp->context = context;
 
     name = g_strdup_printf("chardev-iowatch-%s", chr->label);
     g_source_set_name((GSource *)iwp, name);
@@ -101,10 +133,23 @@ GSource *io_add_watch_poll(Chardev *chr,
     return (GSource *)iwp;
 }
 
+static void io_remove_watch_poll(GSource *source)
+{
+    IOWatchPoll *iwp;
+
+    iwp = io_watch_poll_from_source(source);
+    if (iwp->src) {
+        g_source_destroy(iwp->src);
+        g_source_unref(iwp->src);
+        iwp->src = NULL;
+    }
+    g_source_destroy(&iwp->parent);
+}
+
 void remove_fd_in_watch(Chardev *chr)
 {
     if (chr->gsource) {
-        g_source_destroy(chr->gsource);
+        io_remove_watch_poll(chr->gsource);
         chr->gsource = NULL;
     }
 }
-- 
2.43.0

Re: [PATCH 2/3 for 9.0] Revert "chardev/char-socket: Fix TLS io channels sending too much data to the backend"

[Marc-André Lureau]

Hi

On Mon, Mar 18, 2024 at 10:23 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> This commit results in unexpected termination of the TLS connection.
> When 'fd_can_read' returns 0, the code goes on to pass a zero length
> buffer to qio_channel_read. The TLS impl calls into gnutls_recv()
> with this zero length buffer, at which point GNUTLS returns an error
> GNUTLS_E_INVALID_REQUEST. This is treated as fatal by QEMU's TLS code
> resulting in the connection being torn down by the chardev.
>
> Simply skipping the qio_channel_read when the buffer length is zero
> is also not satisfactory, as it results in a high CPU burn busy loop
> massively slowing QEMU's functionality.
>
> The proper solution is to avoid tcp_chr_read being called at all
> unless the frontend is able to accept more data. This will be done
> in a followup commit.
>
> This reverts commit 1907f4d149c3589ade641423c6a33fd7598fa4d3.

Actually 462945cd22d2bcd233401ed3aa167d83a8e35b05 upstream.

>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  chardev/char-socket.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 2c4dffc0e6..812d7aa38a 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -496,9 +496,9 @@ static gboolean tcp_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
>          s->max_size <= 0) {
>          return TRUE;
>      }
> -    len = tcp_chr_read_poll(opaque);
> -    if (len > sizeof(buf)) {
> -        len = sizeof(buf);
> +    len = sizeof(buf);
> +    if (len > s->max_size) {
> +        len = s->max_size;
>      }
>      size = tcp_chr_recv(chr, (void *)buf, len);
>      if (size == 0 || (size == -1 && errno != EAGAIN)) {
> --
> 2.43.0
>

Re: [PATCH 1/3 for 9.0] chardev: lower priority of the HUP GSource in socket chardev

[Marc-André Lureau]

On Mon, Mar 18, 2024 at 10:25 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> The socket chardev often has 2 GSource object registered against the
> same FD. One is registered all the time and is just intended to handle
> POLLHUP events, while the other gets registered & unregistered on the
> fly as the frontend is ready to receive more data or not.
>
> It is very common for poll() to signal a POLLHUP event at the same time
> as there is pending incoming data from the disconnected client. It is
> therefore essential to process incoming data prior to processing HUP.
> The problem with having 2 GSource on the same FD is that there is no
> guaranteed ordering of execution between them, so the chardev code may
> process HUP first and thus discard data.
>
> This failure scenario is non-deterministic but can be seen fairly
> reliably by reverting a7077b8e354d90fec26c2921aa2dea85b90dff90, and
> then running 'tests/unit/test-char', which will sometimes fail with
> missing data.
>
> Ideally QEMU would only have 1 GSource, but that's a complex code
> refactoring job. The next best solution is to try to ensure ordering
> between the 2 GSource objects. This can be achieved by lowering the
> priority of the HUP GSource, so that it is never dispatched if the
> main GSource is also ready to dispatch. Counter-intuitively, lowering
> the priority of a GSource is done by raising its priority number.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  chardev/char-socket.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 8a0406cc1e..2c4dffc0e6 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -601,6 +601,22 @@ static void update_ioc_handlers(SocketChardev *s)
>
>      remove_hup_source(s);
>      s->hup_source = qio_channel_create_watch(s->ioc, G_IO_HUP);
> +    /*
> +     * poll() is liable to return POLLHUP even when there is
> +     * still incoming data available to read on the FD. If
> +     * we have the hup_source at the same priority as the
> +     * main io_add_watch_poll GSource, then we might end up
> +     * processing the POLLHUP event first, closing the FD,
> +     * and as a result silently discard data we should have
> +     * read.
> +     *
> +     * By setting the hup_source to G_PRIORITY_DEFAULT + 1,
> +     * we ensure that io_add_watch_poll GSource will always
> +     * be dispatched first, thus guaranteeing we will be
> +     * able to process all incoming data before closing the
> +     * FD
> +     */
> +    g_source_set_priority(s->hup_source, G_PRIORITY_DEFAULT + 1);
>      g_source_set_callback(s->hup_source, (GSourceFunc)tcp_chr_hup,
>                            chr, NULL);
>      g_source_attach(s->hup_source, chr->gcontext);
> --
> 2.43.0
>
>


-- 
Marc-André Lureau

Re: [PATCH 3/3 for 9.0] Revert "chardev: use a child source for qio input source"

[Marc-André Lureau]

Hi

On Mon, Mar 18, 2024 at 10:25 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> This reverts commit a7077b8e354d90fec26c2921aa2dea85b90dff90,
> and add comments to explain why child sources cannot be used.
>
> When a GSource is added as a child of another GSource, if its
> 'prepare' function indicates readiness, then the parent's
> 'prepare' function will never be run. The io_watch_poll_prepare
> absolutely *must* be run on every iteration of the main loop,
> to ensure that the chardev backend doesn't feed data to the
> frontend that it is unable to consume.
>
> At the time a7077b8e354d90fec26c2921aa2dea85b90dff90 was made,
> all the child GSource impls were relying on poll'ing an FD,
> so their 'prepare' functions would never indicate readiness
> ahead of poll() being invoked. So the buggy behaviour was
> not noticed and lay dormant.
>
> Relatively recently the QIOChannelTLS impl introduced a
> level 2 child GSource, which checks with GNUTLS whether it
> has cached any data that was decoded but not yet consumed:
>
>   commit ffda5db65aef42266a5053a4be34515106c4c7ee
>   Author: Antoine Damhet <antoine.damhet@shadow.tech>
>   Date:   Tue Nov 15 15:23:29 2022 +0100
>
>     io/channel-tls: fix handling of bigger read buffers
>
>     Since the TLS backend can read more data from the underlying QIOChannel
>     we introduce a minimal child GSource to notify if we still have more
>     data available to be read.
>
>     Signed-off-by: Antoine Damhet <antoine.damhet@shadow.tech>
>     Signed-off-by: Charles Frey <charles.frey@shadow.tech>
>     Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>
> With this, it is now quite common for the 'prepare' function
> on a QIOChannelTLS GSource to indicate immediate readiness,
> bypassing the parent GSource 'prepare' function. IOW, the
> critical 'io_watch_poll_prepare' is being skipped on some
> iterations of the main loop. As a result chardev frontend
> asserts are now being triggered as they are fed data they
> are not ready to consume.
>
> A reproducer is as follows:
>
>  * In terminal 1 run a GNUTLS *echo* server
>
>    $ gnutls-serv --echo \
>                  --x509cafile ca-cert.pem \
>                  --x509keyfile server-key.pem \
>                  --x509certfile server-cert.pem \
>                  -p 9000
>
>  * In terminal 2 run a QEMU guest
>
>    $ qemu-system-s390x \
>        -nodefaults \
>        -display none \
>        -object tls-creds-x509,id=tls0,dir=$PWD,endpoint=client \
>        -chardev socket,id=con0,host=localhost,port=9000,tls-creds=tls0 \
>        -device sclpconsole,chardev=con0 \
>        -hda Fedora-Cloud-Base-39-1.5.s390x.qcow2
>
> After the previous patch revert, but before this patch revert,
> this scenario will crash:
>
>   qemu-system-s390x: ../hw/char/sclpconsole.c:73: chr_read: Assertion
>   `size <= SIZE_BUFFER_VT220 - scon->iov_data_len' failed.
>
> This assert indicates that 'tcp_chr_read' was called without
> 'tcp_chr_read_poll' having first been checked for ability to
> receive more data
>
> QEMU's use of a 'prepare' function to create/delete another
> GSource is rather a hack and not normally the kind of thing that
> is expected to be done by a GSource. There is no mechanism to
> force GLib to always run the 'prepare' function of a parent
> GSource. The best option is to simply not use the child source
> concept, and go back to the functional approach previously
> relied on.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  chardev/char-io.c | 55 ++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 50 insertions(+), 5 deletions(-)
>
> diff --git a/chardev/char-io.c b/chardev/char-io.c
> index 4451128cba..3c725f530b 100644
> --- a/chardev/char-io.c
> +++ b/chardev/char-io.c
> @@ -33,6 +33,7 @@ typedef struct IOWatchPoll {
>      IOCanReadHandler *fd_can_read;
>      GSourceFunc fd_read;
>      void *opaque;
> +    GMainContext *context;
>  } IOWatchPoll;
>
>  static IOWatchPoll *io_watch_poll_from_source(GSource *source)
> @@ -50,28 +51,58 @@ static gboolean io_watch_poll_prepare(GSource *source,
>          return FALSE;
>      }
>
> +    /*
> +     * We do not register the QIOChannel watch as a child GSource.
> +     * The 'prepare' function on the parent GSource will be
> +     * skipped if a child GSource's 'prepare' function indicates
> +     * readiness. We need this prepare function be guaranteed

argh, indeed

I suppose the child 'prepare' could be changed to always return FALSE,
but that would be hackish too

> +     * to run on *every* iteration of the main loop, because
> +     * it is critical to ensure we remove the QIOChannel watch
> +     * if 'fd_can_read' indicates the frontend cannot receive
> +     * more data.
> +     */
>      if (now_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);
> +        g_source_attach(iwp->src, iwp->context);
>      } else {
> -        g_source_remove_child_source(source, iwp->src);
> +        g_source_destroy(iwp->src);
> +        g_source_unref(iwp->src);
>          iwp->src = NULL;
>      }
>      return FALSE;
>  }
>
> +static gboolean io_watch_poll_check(GSource *source)
> +{
> +    return FALSE;
> +}
> +
>  static gboolean io_watch_poll_dispatch(GSource *source, GSourceFunc callback,
>                                         gpointer user_data)
>  {
> -    return G_SOURCE_CONTINUE;
> +    abort();
> +}
> +
> +static void io_watch_poll_finalize(GSource *source)
> +{
> +    /* Due to a glib bug, removing the last reference to a source
> +     * inside a finalize callback causes recursive locking (and a
> +     * deadlock).  This is not a problem inside other callbacks,
> +     * including dispatch callbacks, so we call io_remove_watch_poll
> +     * to remove this source.  At this point, iwp->src must
> +     * be NULL, or we would leak it.
> +     */
> +    IOWatchPoll *iwp = io_watch_poll_from_source(source);
> +    assert(iwp->src == NULL);
>  }
>
>  static GSourceFuncs io_watch_poll_funcs = {
>      .prepare = io_watch_poll_prepare,
> +    .check = io_watch_poll_check,
>      .dispatch = io_watch_poll_dispatch,
> +    .finalize = io_watch_poll_finalize,
>  };
>
>  GSource *io_add_watch_poll(Chardev *chr,
> @@ -91,6 +122,7 @@ GSource *io_add_watch_poll(Chardev *chr,
>      iwp->ioc = ioc;
>      iwp->fd_read = (GSourceFunc) fd_read;
>      iwp->src = NULL;
> +    iwp->context = context;
>
>      name = g_strdup_printf("chardev-iowatch-%s", chr->label);
>      g_source_set_name((GSource *)iwp, name);
> @@ -101,10 +133,23 @@ GSource *io_add_watch_poll(Chardev *chr,
>      return (GSource *)iwp;
>  }
>
> +static void io_remove_watch_poll(GSource *source)
> +{
> +    IOWatchPoll *iwp;
> +
> +    iwp = io_watch_poll_from_source(source);
> +    if (iwp->src) {
> +        g_source_destroy(iwp->src);
> +        g_source_unref(iwp->src);
> +        iwp->src = NULL;
> +    }
> +    g_source_destroy(&iwp->parent);
> +}
> +
>  void remove_fd_in_watch(Chardev *chr)
>  {
>      if (chr->gsource) {
> -        g_source_destroy(chr->gsource);
> +        io_remove_watch_poll(chr->gsource);
>          chr->gsource = NULL;
>      }
>  }
> --
> 2.43.0
>
>



Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>



-- 
Marc-André Lureau

Re: [PATCH 2/3 for 9.0] Revert "chardev/char-socket: Fix TLS io channels sending too much data to the backend"

[Thomas Huth]

On 18/03/2024 19.23, Daniel P. Berrangé wrote:
> This commit results in unexpected termination of the TLS connection.
> When 'fd_can_read' returns 0, the code goes on to pass a zero length
> buffer to qio_channel_read. The TLS impl calls into gnutls_recv()
> with this zero length buffer, at which point GNUTLS returns an error
> GNUTLS_E_INVALID_REQUEST. This is treated as fatal by QEMU's TLS code
> resulting in the connection being torn down by the chardev.
> 
> Simply skipping the qio_channel_read when the buffer length is zero
> is also not satisfactory, as it results in a high CPU burn busy loop
> massively slowing QEMU's functionality.
> 
> The proper solution is to avoid tcp_chr_read being called at all
> unless the frontend is able to accept more data. This will be done
> in a followup commit.
> 
> This reverts commit 1907f4d149c3589ade641423c6a33fd7598fa4d3.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   chardev/char-socket.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 1/3 for 9.0] chardev: lower priority of the HUP GSource in socket chardev

[Thomas Huth]

On 18/03/2024 19.23, Daniel P. Berrangé wrote:
> The socket chardev often has 2 GSource object registered against the
> same FD. One is registered all the time and is just intended to handle
> POLLHUP events, while the other gets registered & unregistered on the
> fly as the frontend is ready to receive more data or not.
> 
> It is very common for poll() to signal a POLLHUP event at the same time
> as there is pending incoming data from the disconnected client. It is
> therefore essential to process incoming data prior to processing HUP.
> The problem with having 2 GSource on the same FD is that there is no
> guaranteed ordering of execution between them, so the chardev code may
> process HUP first and thus discard data.
> 
> This failure scenario is non-deterministic but can be seen fairly
> reliably by reverting a7077b8e354d90fec26c2921aa2dea85b90dff90, and
> then running 'tests/unit/test-char', which will sometimes fail with
> missing data.
> 
> Ideally QEMU would only have 1 GSource, but that's a complex code
> refactoring job. The next best solution is to try to ensure ordering
> between the 2 GSource objects. This can be achieved by lowering the
> priority of the HUP GSource, so that it is never dispatched if the
> main GSource is also ready to dispatch. Counter-intuitively, lowering
> the priority of a GSource is done by raising its priority number.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   chardev/char-socket.c | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 3/3 for 9.0] Revert "chardev: use a child source for qio input source"

[Thomas Huth]

On 18/03/2024 19.23, Daniel P. Berrangé wrote:
> This reverts commit a7077b8e354d90fec26c2921aa2dea85b90dff90,
> and add comments to explain why child sources cannot be used.

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 0/3 for 9.0] Fix TLS support for chardevs and incoming data loss on EOF

[Thomas Huth]

On 18/03/2024 19.23, Daniel P. Berrangé wrote:
> This fixes a problem with TLS support on chardevs that Thomas has
> previously attempted to deal with:
> 
>    https://lists.nongnu.org/archive/html/qemu-devel/2024-02/msg06915.html
> 
> Unfortunately that fix caused unexpected side effects that resulted
> in premature termination of the TLS connection. See patch 2 for
> details.
> 
> I've since identified the root cause of the problem that Thomas was
> trying to fix - bad assumptions about GSource 'prepare' functions
> always being run. See patch 3 for details.
> 
> Patch 3 re-exposed a bug we've know about for a while whereby incoming
> data on chardevs is sometimes discarded when POLLHUP is reported at the
> same time. This required patch 1 to be applied before doing the revert
> in patch 3, otherwise test-char would now very frequently fail.
> 
> So we get 2 bug fixes for the price of one :-)
> 
> Daniel P. Berrangé (3):
>    chardev: lower priority of the HUP GSource in socket chardev
>    Revert "chardev/char-socket: Fix TLS io channels sending too much data
>      to the backend"
>    Revert "chardev: use a child source for qio input source"

Thank you very much for fixing this! I've now also checked that it fixes the 
test scenario for me:

Tested-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 3/3 for 9.0] Revert "chardev: use a child source for qio input source"

[Daniel P. Berrangé]

On Tue, Mar 19, 2024 at 12:20:18AM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Mar 18, 2024 at 10:25 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > This reverts commit a7077b8e354d90fec26c2921aa2dea85b90dff90,
> > and add comments to explain why child sources cannot be used.
> >
> > When a GSource is added as a child of another GSource, if its
> > 'prepare' function indicates readiness, then the parent's
> > 'prepare' function will never be run. The io_watch_poll_prepare
> > absolutely *must* be run on every iteration of the main loop,
> > to ensure that the chardev backend doesn't feed data to the
> > frontend that it is unable to consume.
> >
> > At the time a7077b8e354d90fec26c2921aa2dea85b90dff90 was made,
> > all the child GSource impls were relying on poll'ing an FD,
> > so their 'prepare' functions would never indicate readiness
> > ahead of poll() being invoked. So the buggy behaviour was
> > not noticed and lay dormant.
> >
> > Relatively recently the QIOChannelTLS impl introduced a
> > level 2 child GSource, which checks with GNUTLS whether it
> > has cached any data that was decoded but not yet consumed:
> >
> >   commit ffda5db65aef42266a5053a4be34515106c4c7ee
> >   Author: Antoine Damhet <antoine.damhet@shadow.tech>
> >   Date:   Tue Nov 15 15:23:29 2022 +0100
> >
> >     io/channel-tls: fix handling of bigger read buffers
> >
> >     Since the TLS backend can read more data from the underlying QIOChannel
> >     we introduce a minimal child GSource to notify if we still have more
> >     data available to be read.
> >
> >     Signed-off-by: Antoine Damhet <antoine.damhet@shadow.tech>
> >     Signed-off-by: Charles Frey <charles.frey@shadow.tech>
> >     Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> >
> > With this, it is now quite common for the 'prepare' function
> > on a QIOChannelTLS GSource to indicate immediate readiness,
> > bypassing the parent GSource 'prepare' function. IOW, the
> > critical 'io_watch_poll_prepare' is being skipped on some
> > iterations of the main loop. As a result chardev frontend
> > asserts are now being triggered as they are fed data they
> > are not ready to consume.
> >
> > A reproducer is as follows:
> >
> >  * In terminal 1 run a GNUTLS *echo* server
> >
> >    $ gnutls-serv --echo \
> >                  --x509cafile ca-cert.pem \
> >                  --x509keyfile server-key.pem \
> >                  --x509certfile server-cert.pem \
> >                  -p 9000
> >
> >  * In terminal 2 run a QEMU guest
> >
> >    $ qemu-system-s390x \
> >        -nodefaults \
> >        -display none \
> >        -object tls-creds-x509,id=tls0,dir=$PWD,endpoint=client \
> >        -chardev socket,id=con0,host=localhost,port=9000,tls-creds=tls0 \
> >        -device sclpconsole,chardev=con0 \
> >        -hda Fedora-Cloud-Base-39-1.5.s390x.qcow2
> >
> > After the previous patch revert, but before this patch revert,
> > this scenario will crash:
> >
> >   qemu-system-s390x: ../hw/char/sclpconsole.c:73: chr_read: Assertion
> >   `size <= SIZE_BUFFER_VT220 - scon->iov_data_len' failed.
> >
> > This assert indicates that 'tcp_chr_read' was called without
> > 'tcp_chr_read_poll' having first been checked for ability to
> > receive more data
> >
> > QEMU's use of a 'prepare' function to create/delete another
> > GSource is rather a hack and not normally the kind of thing that
> > is expected to be done by a GSource. There is no mechanism to
> > force GLib to always run the 'prepare' function of a parent
> > GSource. The best option is to simply not use the child source
> > concept, and go back to the functional approach previously
> > relied on.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  chardev/char-io.c | 55 ++++++++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 50 insertions(+), 5 deletions(-)
> >
> > diff --git a/chardev/char-io.c b/chardev/char-io.c
> > index 4451128cba..3c725f530b 100644
> > --- a/chardev/char-io.c
> > +++ b/chardev/char-io.c
> > @@ -33,6 +33,7 @@ typedef struct IOWatchPoll {
> >      IOCanReadHandler *fd_can_read;
> >      GSourceFunc fd_read;
> >      void *opaque;
> > +    GMainContext *context;
> >  } IOWatchPoll;
> >
> >  static IOWatchPoll *io_watch_poll_from_source(GSource *source)
> > @@ -50,28 +51,58 @@ static gboolean io_watch_poll_prepare(GSource *source,
> >          return FALSE;
> >      }
> >
> > +    /*
> > +     * We do not register the QIOChannel watch as a child GSource.
> > +     * The 'prepare' function on the parent GSource will be
> > +     * skipped if a child GSource's 'prepare' function indicates
> > +     * readiness. We need this prepare function be guaranteed
> 
> argh, indeed
> 
> I suppose the child 'prepare' could be changed to always return FALSE,
> but that would be hackish too

If the child returned 'FALSE', then that would defeat the point
of the TLS GSource. It has to return 'TRUE' in order to avoid
sleeping in poll() when there is cached data inside GNUTLS.

> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Thank you for the review.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 2/3 for 9.0] Revert "chardev/char-socket: Fix TLS io channels sending too much data to the backend"

[Daniel P. Berrangé]

On Mon, Mar 18, 2024 at 11:09:23PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Mar 18, 2024 at 10:23 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > This commit results in unexpected termination of the TLS connection.
> > When 'fd_can_read' returns 0, the code goes on to pass a zero length
> > buffer to qio_channel_read. The TLS impl calls into gnutls_recv()
> > with this zero length buffer, at which point GNUTLS returns an error
> > GNUTLS_E_INVALID_REQUEST. This is treated as fatal by QEMU's TLS code
> > resulting in the connection being torn down by the chardev.
> >
> > Simply skipping the qio_channel_read when the buffer length is zero
> > is also not satisfactory, as it results in a high CPU burn busy loop
> > massively slowing QEMU's functionality.
> >
> > The proper solution is to avoid tcp_chr_read being called at all
> > unless the frontend is able to accept more data. This will be done
> > in a followup commit.
> >
> > This reverts commit 1907f4d149c3589ade641423c6a33fd7598fa4d3.
> 
> Actually 462945cd22d2bcd233401ed3aa167d83a8e35b05 upstream.

Opps, yes, will fix this before I send a pull.

> 
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  chardev/char-socket.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > index 2c4dffc0e6..812d7aa38a 100644
> > --- a/chardev/char-socket.c
> > +++ b/chardev/char-socket.c
> > @@ -496,9 +496,9 @@ static gboolean tcp_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
> >          s->max_size <= 0) {
> >          return TRUE;
> >      }
> > -    len = tcp_chr_read_poll(opaque);
> > -    if (len > sizeof(buf)) {
> > -        len = sizeof(buf);
> > +    len = sizeof(buf);
> > +    if (len > s->max_size) {
> > +        len = s->max_size;
> >      }
> >      size = tcp_chr_recv(chr, (void *)buf, len);
> >      if (size == 0 || (size == -1 && errno != EAGAIN)) {
> > --
> > 2.43.0
> >
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|