* [PATCH] crypto: propagate Error object on premature termination
@ 2025-10-06 19:01 Daniel P. Berrangé
2025-10-06 20:00 ` Peter Xu
2025-10-21 18:55 ` Eric Blake
0 siblings, 2 replies; 4+ messages in thread
From: Daniel P. Berrangé @ 2025-10-06 19:01 UTC (permalink / raw)
To: qemu-devel; +Cc: Daniel P. Berrangé, Peter Xu
The way that premature termination was handled in TLS connections was
changed to handle an ordering problem during graceful shutdown in the
migration code.
Unfortunately one of the codepaths returned -1 to indicate an error
condition, but failed to set the 'errp' parameter.
This broke error handling in the qio_channel_tls_handshake function,
as the QTask callback would no longer see that an error was raised.
As a result, the client will go on to try to use the already closed
TLS connection, resulting in misleading errors.
This was evidenced in the I/O test 233 which showed changes such as
-qemu-nbd: Certificate does not match the hostname localhost
+qemu-nbd: Failed to read initial magic: Unable to read from socket: Connection reset by peer
Fixes: 7e0c22d585581b8083ffdeb332ea497218665daf
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
crypto/tlssession.c | 8 +++++---
io/channel-tls.c | 13 +++++++------
2 files changed, 12 insertions(+), 9 deletions(-)
diff --git a/crypto/tlssession.c b/crypto/tlssession.c
index ac38c2121d..8c0bf457ad 100644
--- a/crypto/tlssession.c
+++ b/crypto/tlssession.c
@@ -569,8 +569,6 @@ qcrypto_tls_session_read(QCryptoTLSSession *session,
if (ret < 0) {
if (ret == GNUTLS_E_AGAIN) {
return QCRYPTO_TLS_SESSION_ERR_BLOCK;
- } else if (ret == GNUTLS_E_PREMATURE_TERMINATION) {
- return QCRYPTO_TLS_SESSION_PREMATURE_TERMINATION;
} else {
if (session->rerr) {
error_propagate(errp, session->rerr);
@@ -580,7 +578,11 @@ qcrypto_tls_session_read(QCryptoTLSSession *session,
"Cannot read from TLS channel: %s",
gnutls_strerror(ret));
}
- return -1;
+ if (ret == GNUTLS_E_PREMATURE_TERMINATION) {
+ return QCRYPTO_TLS_SESSION_PREMATURE_TERMINATION;
+ } else {
+ return -1;
+ }
}
}
diff --git a/io/channel-tls.c b/io/channel-tls.c
index 1fbed4be0c..70fad38d18 100644
--- a/io/channel-tls.c
+++ b/io/channel-tls.c
@@ -368,6 +368,7 @@ static ssize_t qio_channel_tls_readv(QIOChannel *ioc,
int flags,
Error **errp)
{
+ ERRP_GUARD();
QIOChannelTLS *tioc = QIO_CHANNEL_TLS(ioc);
size_t i;
ssize_t got = 0;
@@ -384,13 +385,13 @@ static ssize_t qio_channel_tls_readv(QIOChannel *ioc,
} else {
return QIO_CHANNEL_ERR_BLOCK;
}
- } else if (ret == QCRYPTO_TLS_SESSION_PREMATURE_TERMINATION) {
- if (qio_channel_tls_allow_premature_termination(tioc, flags)) {
- ret = 0;
- } else {
- return -1;
- }
} else if (ret < 0) {
+ if (ret == QCRYPTO_TLS_SESSION_PREMATURE_TERMINATION &&
+ qio_channel_tls_allow_premature_termination(tioc, flags)) {
+ error_free(*errp);
+ *errp = NULL;
+ return got;
+ }
return -1;
}
got += ret;
--
2.50.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] crypto: propagate Error object on premature termination
2025-10-06 19:01 [PATCH] crypto: propagate Error object on premature termination Daniel P. Berrangé
@ 2025-10-06 20:00 ` Peter Xu
2025-10-06 20:04 ` Daniel P. Berrangé
2025-10-21 18:55 ` Eric Blake
1 sibling, 1 reply; 4+ messages in thread
From: Peter Xu @ 2025-10-06 20:00 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: qemu-devel
On Mon, Oct 06, 2025 at 08:01:26PM +0100, Daniel P. Berrangé wrote:
> The way that premature termination was handled in TLS connections was
> changed to handle an ordering problem during graceful shutdown in the
> migration code.
>
> Unfortunately one of the codepaths returned -1 to indicate an error
> condition, but failed to set the 'errp' parameter.
>
> This broke error handling in the qio_channel_tls_handshake function,
> as the QTask callback would no longer see that an error was raised.
> As a result, the client will go on to try to use the already closed
> TLS connection, resulting in misleading errors.
>
> This was evidenced in the I/O test 233 which showed changes such as
Oops..
>
> -qemu-nbd: Certificate does not match the hostname localhost
> +qemu-nbd: Failed to read initial magic: Unable to read from socket: Connection reset by peer
>
> Fixes: 7e0c22d585581b8083ffdeb332ea497218665daf
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> crypto/tlssession.c | 8 +++++---
> io/channel-tls.c | 13 +++++++------
> 2 files changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/crypto/tlssession.c b/crypto/tlssession.c
> index ac38c2121d..8c0bf457ad 100644
> --- a/crypto/tlssession.c
> +++ b/crypto/tlssession.c
> @@ -569,8 +569,6 @@ qcrypto_tls_session_read(QCryptoTLSSession *session,
> if (ret < 0) {
> if (ret == GNUTLS_E_AGAIN) {
> return QCRYPTO_TLS_SESSION_ERR_BLOCK;
> - } else if (ret == GNUTLS_E_PREMATURE_TERMINATION) {
> - return QCRYPTO_TLS_SESSION_PREMATURE_TERMINATION;
> } else {
> if (session->rerr) {
> error_propagate(errp, session->rerr);
> @@ -580,7 +578,11 @@ qcrypto_tls_session_read(QCryptoTLSSession *session,
> "Cannot read from TLS channel: %s",
> gnutls_strerror(ret));
> }
> - return -1;
> + if (ret == GNUTLS_E_PREMATURE_TERMINATION) {
> + return QCRYPTO_TLS_SESSION_PREMATURE_TERMINATION;
> + } else {
> + return -1;
> + }
> }
> }
>
> diff --git a/io/channel-tls.c b/io/channel-tls.c
> index 1fbed4be0c..70fad38d18 100644
> --- a/io/channel-tls.c
> +++ b/io/channel-tls.c
> @@ -368,6 +368,7 @@ static ssize_t qio_channel_tls_readv(QIOChannel *ioc,
> int flags,
> Error **errp)
> {
> + ERRP_GUARD();
> QIOChannelTLS *tioc = QIO_CHANNEL_TLS(ioc);
> size_t i;
> ssize_t got = 0;
> @@ -384,13 +385,13 @@ static ssize_t qio_channel_tls_readv(QIOChannel *ioc,
> } else {
> return QIO_CHANNEL_ERR_BLOCK;
> }
> - } else if (ret == QCRYPTO_TLS_SESSION_PREMATURE_TERMINATION) {
> - if (qio_channel_tls_allow_premature_termination(tioc, flags)) {
> - ret = 0;
> - } else {
Would it be simpler to set error only here (and also we can have an
explicit error message for the premature termination)?
No strong opinions.. feel free to use whatever version:
Acked-by: Peter Xu <peterx@redhat.com>
Thanks!
> - return -1;
> - }
> } else if (ret < 0) {
> + if (ret == QCRYPTO_TLS_SESSION_PREMATURE_TERMINATION &&
> + qio_channel_tls_allow_premature_termination(tioc, flags)) {
> + error_free(*errp);
> + *errp = NULL;
> + return got;
> + }
> return -1;
> }
> got += ret;
> --
> 2.50.1
>
--
Peter Xu
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] crypto: propagate Error object on premature termination
2025-10-06 20:00 ` Peter Xu
@ 2025-10-06 20:04 ` Daniel P. Berrangé
0 siblings, 0 replies; 4+ messages in thread
From: Daniel P. Berrangé @ 2025-10-06 20:04 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel
On Mon, Oct 06, 2025 at 04:00:41PM -0400, Peter Xu wrote:
> On Mon, Oct 06, 2025 at 08:01:26PM +0100, Daniel P. Berrangé wrote:
> > The way that premature termination was handled in TLS connections was
> > changed to handle an ordering problem during graceful shutdown in the
> > migration code.
> >
> > Unfortunately one of the codepaths returned -1 to indicate an error
> > condition, but failed to set the 'errp' parameter.
> >
> > This broke error handling in the qio_channel_tls_handshake function,
> > as the QTask callback would no longer see that an error was raised.
> > As a result, the client will go on to try to use the already closed
> > TLS connection, resulting in misleading errors.
> >
> > This was evidenced in the I/O test 233 which showed changes such as
>
> Oops..
>
> >
> > -qemu-nbd: Certificate does not match the hostname localhost
> > +qemu-nbd: Failed to read initial magic: Unable to read from socket: Connection reset by peer
> >
> > Fixes: 7e0c22d585581b8083ffdeb332ea497218665daf
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> > crypto/tlssession.c | 8 +++++---
> > io/channel-tls.c | 13 +++++++------
> > 2 files changed, 12 insertions(+), 9 deletions(-)
> >
> > diff --git a/crypto/tlssession.c b/crypto/tlssession.c
> > index ac38c2121d..8c0bf457ad 100644
> > --- a/crypto/tlssession.c
> > +++ b/crypto/tlssession.c
> > @@ -569,8 +569,6 @@ qcrypto_tls_session_read(QCryptoTLSSession *session,
> > if (ret < 0) {
> > if (ret == GNUTLS_E_AGAIN) {
> > return QCRYPTO_TLS_SESSION_ERR_BLOCK;
> > - } else if (ret == GNUTLS_E_PREMATURE_TERMINATION) {
> > - return QCRYPTO_TLS_SESSION_PREMATURE_TERMINATION;
> > } else {
> > if (session->rerr) {
> > error_propagate(errp, session->rerr);
> > @@ -580,7 +578,11 @@ qcrypto_tls_session_read(QCryptoTLSSession *session,
> > "Cannot read from TLS channel: %s",
> > gnutls_strerror(ret));
> > }
> > - return -1;
> > + if (ret == GNUTLS_E_PREMATURE_TERMINATION) {
> > + return QCRYPTO_TLS_SESSION_PREMATURE_TERMINATION;
> > + } else {
> > + return -1;
> > + }
> > }
> > }
> >
> > diff --git a/io/channel-tls.c b/io/channel-tls.c
> > index 1fbed4be0c..70fad38d18 100644
> > --- a/io/channel-tls.c
> > +++ b/io/channel-tls.c
> > @@ -368,6 +368,7 @@ static ssize_t qio_channel_tls_readv(QIOChannel *ioc,
> > int flags,
> > Error **errp)
> > {
> > + ERRP_GUARD();
> > QIOChannelTLS *tioc = QIO_CHANNEL_TLS(ioc);
> > size_t i;
> > ssize_t got = 0;
> > @@ -384,13 +385,13 @@ static ssize_t qio_channel_tls_readv(QIOChannel *ioc,
> > } else {
> > return QIO_CHANNEL_ERR_BLOCK;
> > }
> > - } else if (ret == QCRYPTO_TLS_SESSION_PREMATURE_TERMINATION) {
> > - if (qio_channel_tls_allow_premature_termination(tioc, flags)) {
> > - ret = 0;
> > - } else {
>
> Would it be simpler to set error only here (and also we can have an
> explicit error message for the premature termination)?
I wanted to retain the original GNUTLS provided error message, which
is not available here, so must set it in the tlssession.c file.
> No strong opinions.. feel free to use whatever version:
>
> Acked-by: Peter Xu <peterx@redhat.com>
>
> Thanks!
>
> > - return -1;
> > - }
> > } else if (ret < 0) {
> > + if (ret == QCRYPTO_TLS_SESSION_PREMATURE_TERMINATION &&
> > + qio_channel_tls_allow_premature_termination(tioc, flags)) {
> > + error_free(*errp);
> > + *errp = NULL;
> > + return got;
> > + }
> > return -1;
> > }
> > got += ret;
> > --
> > 2.50.1
> >
>
> --
> Peter Xu
>
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 :|
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] crypto: propagate Error object on premature termination
2025-10-06 19:01 [PATCH] crypto: propagate Error object on premature termination Daniel P. Berrangé
2025-10-06 20:00 ` Peter Xu
@ 2025-10-21 18:55 ` Eric Blake
1 sibling, 0 replies; 4+ messages in thread
From: Eric Blake @ 2025-10-21 18:55 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: qemu-devel, Peter Xu
On Mon, Oct 06, 2025 at 08:01:26PM +0100, Daniel P. Berrangé wrote:
> The way that premature termination was handled in TLS connections was
> changed to handle an ordering problem during graceful shutdown in the
> migration code.
>
> Unfortunately one of the codepaths returned -1 to indicate an error
> condition, but failed to set the 'errp' parameter.
>
> This broke error handling in the qio_channel_tls_handshake function,
> as the QTask callback would no longer see that an error was raised.
> As a result, the client will go on to try to use the already closed
> TLS connection, resulting in misleading errors.
>
> This was evidenced in the I/O test 233 which showed changes such as
>
> -qemu-nbd: Certificate does not match the hostname localhost
> +qemu-nbd: Failed to read initial magic: Unable to read from socket: Connection reset by peer
>
> Fixes: 7e0c22d585581b8083ffdeb332ea497218665daf
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> crypto/tlssession.c | 8 +++++---
> io/channel-tls.c | 13 +++++++------
> 2 files changed, 12 insertions(+), 9 deletions(-)
Thanks; queued in my NBD tree, since I have further patches that will
further alter the output of 233 to give more details about NBD
devices.
Tested-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-10-21 18:55 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-10-06 19:01 [PATCH] crypto: propagate Error object on premature termination Daniel P. Berrangé
2025-10-06 20:00 ` Peter Xu
2025-10-06 20:04 ` Daniel P. Berrangé
2025-10-21 18:55 ` Eric Blake
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.