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

[Eric Blake]

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) {
> > +        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