On Thu, Sep 11, 2025 at 05:23:53PM -0400, Peter Xu wrote: > QCryptoTLSSession allows TLS premature termination in two cases, one of the > case is when the channel shutdown() is invoked on READ side. > > It's possible the shutdown() happened after the read thread blocked at > gnutls_record_recv(). In this case, we should allow the premature > termination to happen. > > The problem is by the time qcrypto_tls_session_read() was invoked, > tioc->shutdown may not have been set, so this may instead be treated as an > error if there is concurrent shutdown() calls. > > To allow the flag to reflect the latest status of tioc->shutdown, move the > check upper into the QIOChannel level, so as to read the flag only after > QEMU gets an GNUTLS_E_PREMATURE_TERMINATION. > > When at it, introduce qio_channel_tls_allow_premature_termination() helper > to make the condition checks easier to read. > > This patch will fix a qemu qtest warning when running the preempt tls test, > reporting premature termination: > > QTEST_QEMU_BINARY=./qemu-system-x86_64 ./tests/qtest/migration-test --full -r > /x86_64/migration/postcopy/preempt/tls/psk > ... > qemu-kvm: Cannot read from TLS channel: The TLS connection was non-properly > terminated. > ... > > In this specific case, the error was set by postcopy_preempt_thread, which > normally will be concurrently shutdown()ed by the main thread. > > Signed-off-by: Peter Xu <[email protected]> > --- > include/crypto/tlssession.h | 7 +------ > crypto/tlssession.c | 7 ++----- > io/channel-tls.c | 21 +++++++++++++++++++-- > 3 files changed, 22 insertions(+), 13 deletions(-)
Reviewed-by: Daniel P. Berrangé <[email protected]> > diff --git a/include/crypto/tlssession.h b/include/crypto/tlssession.h > index 2f62ce2d67..6b4fcadee7 100644 > --- a/include/crypto/tlssession.h > +++ b/include/crypto/tlssession.h > @@ -110,6 +110,7 @@ > typedef struct QCryptoTLSSession QCryptoTLSSession; > > #define QCRYPTO_TLS_SESSION_ERR_BLOCK -2 > +#define QCRYPTO_TLS_SESSION_PREMATURE_TERMINATION -3 > > /** > * qcrypto_tls_session_new: > @@ -259,7 +260,6 @@ ssize_t qcrypto_tls_session_write(QCryptoTLSSession *sess, > * @sess: the TLS session object > * @buf: to fill with plain text received > * @len: the length of @buf > - * @gracefulTermination: treat premature termination as graceful EOF > * @errp: pointer to hold returned error object > * > * Receive up to @len bytes of data from the remote peer > @@ -267,10 +267,6 @@ ssize_t qcrypto_tls_session_write(QCryptoTLSSession > *sess, > * qcrypto_tls_session_set_callbacks(), decrypt it and > * store it in @buf. > * > - * If @gracefulTermination is true, then a premature termination > - * of the TLS session will be treated as indicating EOF, as > - * opposed to an error. > - * Could you say something about QCRYPTO_TLS_SESSION_PREMATURE_TERMINATION being a possible return code here (no need to repost just for that). > * It is an error to call this before > * qcrypto_tls_session_handshake() returns > * QCRYPTO_TLS_HANDSHAKE_COMPLETE > @@ -282,7 +278,6 @@ ssize_t qcrypto_tls_session_write(QCryptoTLSSession *sess, > ssize_t qcrypto_tls_session_read(QCryptoTLSSession *sess, > char *buf, > size_t len, > - bool gracefulTermination, > Error **errp); > > /** > +static bool > +qio_channel_tls_allow_premature_termination(QIOChannelTLS *tioc, int flags) > +{ > + if (flags & QIO_CHANNEL_READ_FLAG_RELAXED_EOF) { > + return true; > + } > + > + if (qatomic_read(&tioc->shutdown) & QIO_CHANNEL_SHUTDOWN_READ) { > + return true; > + } > + > + return false; > +} > > static ssize_t qio_channel_tls_readv(QIOChannel *ioc, > const struct iovec *iov, > @@ -364,8 +377,6 @@ static ssize_t qio_channel_tls_readv(QIOChannel *ioc, > tioc->session, > iov[i].iov_base, > iov[i].iov_len, > - flags & QIO_CHANNEL_READ_FLAG_RELAXED_EOF || > - qatomic_load_acquire(&tioc->shutdown) & > QIO_CHANNEL_SHUTDOWN_READ, > errp); The original code uses qatomic_load_acquire() while the new code uses qatomic_read() which imposes weaker ordering constraints. Does this matter ? I'm not familiar enough with atomics to say which we need here ? > if (ret == QCRYPTO_TLS_SESSION_ERR_BLOCK) { > if (got) { > @@ -373,6 +384,12 @@ 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) { > return -1; > } > -- > 2.50.1 > 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 :|
