| Message ID | 20221115142329.92524-3-antoine.damhet@shadow.tech (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | TLS: fix read stall with large buffers | expand |
On Tue, Nov 15, 2022 at 03:23:29PM +0100, antoine.damhet@shadow.tech wrote: > From: Antoine Damhet <antoine.damhet@shadow.tech> > > 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> > --- > io/channel-tls.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 65 insertions(+), 1 deletion(-) Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> With regards, Daniel
On Wed, Nov 16, 2022 at 10:52:20AM +0000, Daniel P. Berrangé wrote: > On Tue, Nov 15, 2022 at 03:23:29PM +0100, antoine.damhet@shadow.tech wrote: > > From: Antoine Damhet <antoine.damhet@shadow.tech> > > > > 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> > > --- > > io/channel-tls.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 65 insertions(+), 1 deletion(-) > > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Thanks, Now that the 7.2.0 is released, can we hope to get this queued ? If not what should I do ? Best regards,
On Fri, Dec 16, 2022 at 11:38:03AM +0100, Antoine Damhet wrote: > On Wed, Nov 16, 2022 at 10:52:20AM +0000, Daniel P. Berrangé wrote: > > On Tue, Nov 15, 2022 at 03:23:29PM +0100, antoine.damhet@shadow.tech wrote: > > > From: Antoine Damhet <antoine.damhet@shadow.tech> > > > > > > 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> > > > --- > > > io/channel-tls.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++- > > > 1 file changed, 65 insertions(+), 1 deletion(-) > > > > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> > > Thanks, > > Now that the 7.2.0 is released, can we hope to get this queued ? If not > what should I do ? Yes, I have this queued already for my next pull req. With regards, Daniel
On 15/11/2022 15.23, antoine.damhet@shadow.tech wrote: > From: Antoine Damhet <antoine.damhet@shadow.tech> > > 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> > --- > io/channel-tls.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 65 insertions(+), 1 deletion(-) Hi Antoine, Charles and Daniel! This patch can cause a crash with character devices in QEMU that check for a valid amount of data that should be written to it, like e.g. the sclpconsole of qemu-system-s390x. And I guess other character devices are affected, too, in the sense that they are dropping characters silently. The problem can be reproduced like this: 1) In one terminal, do this: mkdir qemu-pki cd qemu-pki openssl genrsa 2048 > ca-key.pem openssl req -new -x509 -nodes -days 365000 -key ca-key.pem -out ca-cert.pem # enter some dummy value for the cert openssl genrsa 2048 > server-key.pem openssl req -new -x509 -nodes -days 365000 -key server-key.pem \ -out server-cert.pem # enter some other dummy values for the cert gnutls-serv --echo --x509cafile ca-cert.pem --x509keyfile server-key.pem \ --x509certfile server-cert.pem -p 8338 2) In another terminal, do this: wget https://download.fedoraproject.org/pub/fedora-secondary/releases/39/Cloud/s390x/images/Fedora-Cloud-Base-39-1.5.s390x.qcow2 qemu-system-s390x -nographic -nodefaults \ -hda Fedora-Cloud-Base-39-1.5.s390x.qcow2 \ -object tls-creds-x509,id=tls0,endpoint=client,verify-peer=false,dir=$PWD/qemu-pki \ -chardev socket,id=tls_chardev,host=localhost,port=8338,tls-creds=tls0 \ -device sclpconsole,chardev=tls_chardev,id=tls_serial QEMU then aborts after a second or two with: qemu-system-s390x: ../../devel/qemu/hw/char/sclpconsole.c:73: chr_read: Assertion `size <= SIZE_BUFFER_VT220 - scon->iov_data_len' failed. Aborted (core dumped) It looks like the second read does not trigger the chr_can_read() function to be called before the read, which should normally always be done before sending bytes to a character device to see how much it can handle. ... do you maybe have any ideas how to fix this? Thanks, Thomas > diff --git a/io/channel-tls.c b/io/channel-tls.c > index 4ce890a538..4f2b8828f9 100644 > --- a/io/channel-tls.c > +++ b/io/channel-tls.c > @@ -388,12 +388,76 @@ static void qio_channel_tls_set_aio_fd_handler(QIOChannel *ioc, > qio_channel_set_aio_fd_handler(tioc->master, ctx, io_read, io_write, opaque); > } > > +typedef struct QIOChannelTLSSource QIOChannelTLSSource; > +struct QIOChannelTLSSource { > + GSource parent; > + QIOChannelTLS *tioc; > +}; > + > +static gboolean > +qio_channel_tls_source_check(GSource *source) > +{ > + QIOChannelTLSSource *tsource = (QIOChannelTLSSource *)source; > + > + return qcrypto_tls_session_check_pending(tsource->tioc->session) > 0; > +} > + > +static gboolean > +qio_channel_tls_source_prepare(GSource *source, gint *timeout) > +{ > + *timeout = -1; > + return qio_channel_tls_source_check(source); > +} > + > +static gboolean > +qio_channel_tls_source_dispatch(GSource *source, GSourceFunc callback, > + gpointer user_data) > +{ > + return G_SOURCE_CONTINUE; > +} > + > +static void > +qio_channel_tls_source_finalize(GSource *source) > +{ > + QIOChannelTLSSource *tsource = (QIOChannelTLSSource *)source; > + > + object_unref(OBJECT(tsource->tioc)); > +} > + > +static GSourceFuncs qio_channel_tls_source_funcs = { > + qio_channel_tls_source_prepare, > + qio_channel_tls_source_check, > + qio_channel_tls_source_dispatch, > + qio_channel_tls_source_finalize > +}; > + > +static void > +qio_channel_tls_read_watch(QIOChannelTLS *tioc, GSource *source) > +{ > + GSource *child; > + QIOChannelTLSSource *tlssource; > + > + child = g_source_new(&qio_channel_tls_source_funcs, > + sizeof(QIOChannelTLSSource)); > + tlssource = (QIOChannelTLSSource *)child; > + > + tlssource->tioc = tioc; > + object_ref(OBJECT(tioc)); > + > + g_source_add_child_source(source, child); > +} > + > static GSource *qio_channel_tls_create_watch(QIOChannel *ioc, > GIOCondition condition) > { > QIOChannelTLS *tioc = QIO_CHANNEL_TLS(ioc); > + GSource *source = qio_channel_create_watch(tioc->master, condition); > + > + if (condition & G_IO_IN) { > + qio_channel_tls_read_watch(tioc, source); > + } > > - return qio_channel_create_watch(tioc->master, condition); > + return source; > } > > QCryptoTLSSession *
diff --git a/io/channel-tls.c b/io/channel-tls.c index 4ce890a538..4f2b8828f9 100644 --- a/io/channel-tls.c +++ b/io/channel-tls.c @@ -388,12 +388,76 @@ static void qio_channel_tls_set_aio_fd_handler(QIOChannel *ioc, qio_channel_set_aio_fd_handler(tioc->master, ctx, io_read, io_write, opaque); } +typedef struct QIOChannelTLSSource QIOChannelTLSSource; +struct QIOChannelTLSSource { + GSource parent; + QIOChannelTLS *tioc; +}; + +static gboolean +qio_channel_tls_source_check(GSource *source) +{ + QIOChannelTLSSource *tsource = (QIOChannelTLSSource *)source; + + return qcrypto_tls_session_check_pending(tsource->tioc->session) > 0; +} + +static gboolean +qio_channel_tls_source_prepare(GSource *source, gint *timeout) +{ + *timeout = -1; + return qio_channel_tls_source_check(source); +} + +static gboolean +qio_channel_tls_source_dispatch(GSource *source, GSourceFunc callback, + gpointer user_data) +{ + return G_SOURCE_CONTINUE; +} + +static void +qio_channel_tls_source_finalize(GSource *source) +{ + QIOChannelTLSSource *tsource = (QIOChannelTLSSource *)source; + + object_unref(OBJECT(tsource->tioc)); +} + +static GSourceFuncs qio_channel_tls_source_funcs = { + qio_channel_tls_source_prepare, + qio_channel_tls_source_check, + qio_channel_tls_source_dispatch, + qio_channel_tls_source_finalize +}; + +static void +qio_channel_tls_read_watch(QIOChannelTLS *tioc, GSource *source) +{ + GSource *child; + QIOChannelTLSSource *tlssource; + + child = g_source_new(&qio_channel_tls_source_funcs, + sizeof(QIOChannelTLSSource)); + tlssource = (QIOChannelTLSSource *)child; + + tlssource->tioc = tioc; + object_ref(OBJECT(tioc)); + + g_source_add_child_source(source, child); +} + static GSource *qio_channel_tls_create_watch(QIOChannel *ioc, GIOCondition condition) { QIOChannelTLS *tioc = QIO_CHANNEL_TLS(ioc); + GSource *source = qio_channel_create_watch(tioc->master, condition); + + if (condition & G_IO_IN) { + qio_channel_tls_read_watch(tioc, source); + } - return qio_channel_create_watch(tioc->master, condition); + return source; } QCryptoTLSSession *