On Fri, Mar 27, 2020 at 11:19:35AM -0500, Eric Blake wrote: > Gnutls documents that while many apps simply yank out the underlying > transport at the end of communication in the name of efficiency, this > is indistinguishable from a malicious actor terminating the connection > prematurely. Since our channel I/O code already supports the notion of > a graceful shutdown request, it is time to plumb that through to the > TLS layer, and wait for TLS to give the all clear before then > terminating traffic on the underlying channel. > > Note that channel-tls now always advertises shutdown support, > regardless of whether the underlying channel also has that support. > > Signed-off-by: Eric Blake <ebl...@redhat.com> > --- > io/channel-tls.c | 27 ++++++++++++++++++++++++++- > 1 file changed, 26 insertions(+), 1 deletion(-) > > diff --git a/io/channel-tls.c b/io/channel-tls.c > index 7ec8ceff2f01..f90905823e1d 100644 > --- a/io/channel-tls.c > +++ b/io/channel-tls.c > @@ -360,10 +360,35 @@ static int qio_channel_tls_shutdown(QIOChannel *ioc, > Error **errp) > { > QIOChannelTLS *tioc = QIO_CHANNEL_TLS(ioc); > + int ret = 0; > > tioc->shutdown |= how; > > - return qio_channel_shutdown(tioc->master, how, errp); > + do { > + switch (how) { > + case QIO_CHANNEL_SHUTDOWN_READ: > + /* No TLS counterpart */ > + break; > + case QIO_CHANNEL_SHUTDOWN_WRITE: > + ret = qcrypto_tls_session_shutdown(tioc->session, > QCRYPTO_SHUT_WR); > + break; > + case QIO_CHANNEL_SHUTDOWN_BOTH: > + ret = qcrypto_tls_session_shutdown(tioc->session, > + QCRYPTO_SHUT_RDWR); > + break; > + default: > + abort(); > + } > + } while (ret == -EAGAIN);
I don't think it is acceptable to do this loop here. The gnutls_bye() function triggers several I/O operations which could block. Looping like this means we busy-wait, blocking this thread for as long as I/O is blocking on the socket. If we must call gnutls_bye(), then it needs to be done in a way that can integrate with the main loop so it poll()'s / unblocks the current coroutine/thread. This makes the whole thing significantly more complex to deal with, especially if the shutdown is being done in cleanup paths which ordinarily are expected to execute without blocking on I/O. This is the big reason why i never made any attempt to use gnutls_bye(). 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 :|