[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 2/3] io: Support shutdown of TLS channel

From: Eric Blake
Subject: Re: [PATCH 2/3] io: Support shutdown of TLS channel
Date: Fri, 27 Mar 2020 12:29:39 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.6.0

On 3/27/20 11:40 AM, Daniel P. Berrangé wrote:
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) {
+            /* No TLS counterpart */
+            break;
+            ret = qcrypto_tls_session_shutdown(tioc->session, QCRYPTO_SHUT_WR);
+            break;
+            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.

Hmm, good point. Should we give qio_channel_tls_shutdown a bool parameter that says whether it should wait (good for use when we are being run in a coroutine and can deal with the additional I/O) or just fail with -EAGAIN (which the caller can ignore if it is not worried)?

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().

We _are_ using gnutls_bye(GNUTLS_SHUT_RDWR) on the close() path (which is indeed a cleanup path where not blocking is worthwhile) even without this patch, but the question is whether using gnutls_bye(GNUTLS_SHUT_WR) in the normal data path, where we are already using coroutines to manage callbacks, can benefit the remote endpoint, giving them a chance to see clean shutdown instead of abrupt termination.

Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

reply via email to

[Prev in Thread] Current Thread [Next in Thread]