| Message ID | 20250207142758.6936-2-farosas@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | crypto,io,migration: Add support to gnutls_bye() | expand |
On Fri, Feb 07, 2025 at 11:27:51AM -0300, Fabiano Rosas wrote: > QEMU's TLS session code provides no way to call gnutls_bye() to > terminate a TLS session. Callers of qcrypto_tls_session_read() can > choose to ignore a GNUTLS_E_PREMATURE_TERMINATION error by setting the > gracefulTermination argument. > > The QIOChannelTLS ignores the premature termination error whenever > shutdown() has already been issued. This is not enough anymore for the > migration code due to changes [1] in the synchronization between > migration source and destination. > > Add support for calling gnutls_bye() in the tlssession layer so users > of QIOChannelTLS can clearly identify the end of a TLS session. > > 1- 1d457daf86 ("migration/multifd: Further remove the SYNC on complete") Would be slightly clearer as "[1] 1d457daf86 ...." > > Signed-off-by: Fabiano Rosas <farosas@suse.de> > --- > crypto/tlssession.c | 41 +++++++++++++++++++++++++++++++++++++ > include/crypto/tlssession.h | 22 ++++++++++++++++++++ > 2 files changed, 63 insertions(+) Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Acked-by: Daniel P. Berrangé <berrange@redhat.com> With regards, Daniel
On Fri, Feb 07, 2025 at 11:27:51AM -0300, Fabiano Rosas wrote: > QEMU's TLS session code provides no way to call gnutls_bye() to > terminate a TLS session. Callers of qcrypto_tls_session_read() can > choose to ignore a GNUTLS_E_PREMATURE_TERMINATION error by setting the > gracefulTermination argument. > > The QIOChannelTLS ignores the premature termination error whenever > shutdown() has already been issued. This is not enough anymore for the > migration code due to changes [1] in the synchronization between > migration source and destination. This sentence seems to say commit [1] changed something on the tls condition, but IMHO fundamentally the issue is multifd recv thread model that relies on blocking readv() rather than request-based (like what src multifd does). Now src uses either shutdown() or close() to kick dest multifd recv threads out from readv(). That has nothing to do with what we do during complete() with those sync messages.. referencing it is ok, but we'll need to reference also the other commit to be clear pre-9.0 can also be prone to this. To me, it's more important to mention the root cause on the multifd recv thread model, which requires explicit tls terminations. > > Add support for calling gnutls_bye() in the tlssession layer so users > of QIOChannelTLS can clearly identify the end of a TLS session. > > 1- 1d457daf86 ("migration/multifd: Further remove the SYNC on complete") > > Signed-off-by: Fabiano Rosas <farosas@suse.de>
Peter Xu <peterx@redhat.com> writes: > On Fri, Feb 07, 2025 at 11:27:51AM -0300, Fabiano Rosas wrote: >> QEMU's TLS session code provides no way to call gnutls_bye() to >> terminate a TLS session. Callers of qcrypto_tls_session_read() can >> choose to ignore a GNUTLS_E_PREMATURE_TERMINATION error by setting the >> gracefulTermination argument. >> >> The QIOChannelTLS ignores the premature termination error whenever >> shutdown() has already been issued. This is not enough anymore for the >> migration code due to changes [1] in the synchronization between >> migration source and destination. > > This sentence seems to say commit [1] changed something on the tls > condition, but IMHO fundamentally the issue is multifd recv thread model > that relies on blocking readv() rather than request-based (like what src > multifd does). > > Now src uses either shutdown() or close() to kick dest multifd recv threads > out from readv(). That has nothing to do with what we do during complete() > with those sync messages.. referencing it is ok, but we'll need to > reference also the other commit to be clear pre-9.0 can also be prone to > this. To me, it's more important to mention the root cause on the multifd > recv thread model, which requires explicit tls terminations. > I didn't want to go into too much detail in a commit for crypto/. The motivation for *this* patch is just: migration needs it. What about: The QIOChannelTLS ignores the premature termination error whenever shutdown() has already been issued. This was found to be not enough for the migration code because shutdown() might not have been issued before the connection is terminated. >> >> Add support for calling gnutls_bye() in the tlssession layer so users >> of QIOChannelTLS can clearly identify the end of a TLS session. >> >> 1- 1d457daf86 ("migration/multifd: Further remove the SYNC on complete") >> >> Signed-off-by: Fabiano Rosas <farosas@suse.de>
On Fri, Feb 07, 2025 at 02:55:57PM -0300, Fabiano Rosas wrote: > Peter Xu <peterx@redhat.com> writes: > > > On Fri, Feb 07, 2025 at 11:27:51AM -0300, Fabiano Rosas wrote: > >> QEMU's TLS session code provides no way to call gnutls_bye() to > >> terminate a TLS session. Callers of qcrypto_tls_session_read() can > >> choose to ignore a GNUTLS_E_PREMATURE_TERMINATION error by setting the > >> gracefulTermination argument. > >> > >> The QIOChannelTLS ignores the premature termination error whenever > >> shutdown() has already been issued. This is not enough anymore for the > >> migration code due to changes [1] in the synchronization between > >> migration source and destination. > > > > This sentence seems to say commit [1] changed something on the tls > > condition, but IMHO fundamentally the issue is multifd recv thread model > > that relies on blocking readv() rather than request-based (like what src > > multifd does). > > > > Now src uses either shutdown() or close() to kick dest multifd recv threads > > out from readv(). That has nothing to do with what we do during complete() > > with those sync messages.. referencing it is ok, but we'll need to > > reference also the other commit to be clear pre-9.0 can also be prone to > > this. To me, it's more important to mention the root cause on the multifd > > recv thread model, which requires explicit tls terminations. > > > > I didn't want to go into too much detail in a commit for crypto/. The You already did so by referencing a multifd commit that changes how complete() works! > motivation for *this* patch is just: migration needs it. What about: > > The QIOChannelTLS ignores the premature termination error whenever > shutdown() has already been issued. This was found to be not enough for > the migration code because shutdown() might not have been issued before > the connection is terminated. Looks good to me, thanks.
diff --git a/crypto/tlssession.c b/crypto/tlssession.c index 77286e23f4..d769d7a304 100644 --- a/crypto/tlssession.c +++ b/crypto/tlssession.c @@ -585,6 +585,40 @@ qcrypto_tls_session_get_handshake_status(QCryptoTLSSession *session) } } +int +qcrypto_tls_session_bye(QCryptoTLSSession *session, Error **errp) +{ + int ret; + + if (!session->handshakeComplete) { + return 0; + } + + ret = gnutls_bye(session->handle, GNUTLS_SHUT_WR); + + if (!ret) { + return QCRYPTO_TLS_BYE_COMPLETE; + } + + if (ret == GNUTLS_E_INTERRUPTED || ret == GNUTLS_E_AGAIN) { + int direction = gnutls_record_get_direction(session->handle); + return direction ? QCRYPTO_TLS_BYE_SENDING : QCRYPTO_TLS_BYE_RECVING; + } + + if (session->rerr || session->werr) { + error_setg(errp, "TLS termination failed: %s: %s", gnutls_strerror(ret), + error_get_pretty(session->rerr ? + session->rerr : session->werr)); + } else { + error_setg(errp, "TLS termination failed: %s", gnutls_strerror(ret)); + } + + error_free(session->rerr); + error_free(session->werr); + session->rerr = session->werr = NULL; + + return -1; +} int qcrypto_tls_session_get_key_size(QCryptoTLSSession *session, @@ -699,6 +733,13 @@ qcrypto_tls_session_get_handshake_status(QCryptoTLSSession *sess) } +int +qcrypto_tls_session_bye(QCryptoTLSSession *session, Error **errp) +{ + return QCRYPTO_TLS_BYE_COMPLETE; +} + + int qcrypto_tls_session_get_key_size(QCryptoTLSSession *sess, Error **errp) diff --git a/include/crypto/tlssession.h b/include/crypto/tlssession.h index f694a5c3c5..c0f64ce989 100644 --- a/include/crypto/tlssession.h +++ b/include/crypto/tlssession.h @@ -323,6 +323,28 @@ typedef enum { QCryptoTLSSessionHandshakeStatus qcrypto_tls_session_get_handshake_status(QCryptoTLSSession *sess); +typedef enum { + QCRYPTO_TLS_BYE_COMPLETE, + QCRYPTO_TLS_BYE_SENDING, + QCRYPTO_TLS_BYE_RECVING, +} QCryptoTLSSessionByeStatus; + +/** + * qcrypto_tls_session_bye: + * @session: the TLS session object + * @errp: pointer to a NULL-initialized error object + * + * Start, or continue, a TLS termination sequence. If the underlying + * data channel is non-blocking, then this method may return control + * before the termination is complete. The return value will indicate + * whether the termination has completed, or is waiting to send or + * receive data. In the latter cases, the caller should setup an event + * loop watch and call this method again once the underlying data + * channel is ready to read or write again. + */ +int +qcrypto_tls_session_bye(QCryptoTLSSession *session, Error **errp); + /** * qcrypto_tls_session_get_key_size: * @sess: the TLS session object
QEMU's TLS session code provides no way to call gnutls_bye() to terminate a TLS session. Callers of qcrypto_tls_session_read() can choose to ignore a GNUTLS_E_PREMATURE_TERMINATION error by setting the gracefulTermination argument. The QIOChannelTLS ignores the premature termination error whenever shutdown() has already been issued. This is not enough anymore for the migration code due to changes [1] in the synchronization between migration source and destination. Add support for calling gnutls_bye() in the tlssession layer so users of QIOChannelTLS can clearly identify the end of a TLS session. 1- 1d457daf86 ("migration/multifd: Further remove the SYNC on complete") Signed-off-by: Fabiano Rosas <farosas@suse.de> --- crypto/tlssession.c | 41 +++++++++++++++++++++++++++++++++++++ include/crypto/tlssession.h | 22 ++++++++++++++++++++ 2 files changed, 63 insertions(+)