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

[Eric Blake <eblake@redhat.com>]

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 <eblake@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.

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