[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/3] io: Support shutdown of TLS channel
From: |
Daniel P . Berrangé |
Subject: |
Re: [PATCH 2/3] io: Support shutdown of TLS channel |
Date: |
Fri, 27 Mar 2020 16:40:40 +0000 |
User-agent: |
Mutt/1.13.3 (2020-01-12) |
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 <address@hidden>
> ---
> 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 :|
[PATCH 1/3] crypto: Add qcrypto_tls_shutdown(), Eric Blake, 2020/03/27
[PATCH 3/3] nbd: Use shutdown(SHUT_WR) after last item sent, Eric Blake, 2020/03/27