io: return 0 for EOF in TLS session read after shutdown
PATCH 1/1 io: return 0 for EOF in TLS session read after shutdown
GNUTLS takes a paranoid approach when seeing 0 bytes returned by the underlying OS read() function. It will consider this an error and return GNUTLS_E_PREMATURE_TERMINATION instead of propagating the 0 return value. It expects apps to arrange for clean termination at the protocol level and not rely on seeing EOF from a read call to detect shutdown. This is to harden apps against a malicious 3rd party causing termination of the sockets layer.
This is unhelpful for the QEMU NBD code which does have a clean protocol level shutdown, but still relies on seeing 0 from the I/O channel read in the coroutine handling incoming replies.
The upshot is that when using a plain NBD connection shutdown is silent, but when using TLS, the client spams the console with
Cannot read from TLS channel: Broken pipe
The NBD connection has, however, called qio_channel_shutdown() at this point to indicate that it is done with I/O. This gives the opportunity to optimize the code such that when the channel has been shutdown in the read direction, the error code GNUTLS_E_PREMATURE_TERMINATION gets turned into a '0' return instead of an error.
--- crypto/tlssession.c | 3 +++ include/io/channel-tls.h | 1 + include/io/channel.h | 6 +++--- io/channel-tls.c | 5 +++++ 4 files changed, 12 insertions(+), 3 deletions(-)
crypto/tlssession.c +3 -0
@@ -473,6 +473,9 @@ qcrypto_tls_session_read(QCryptoTLSSession *session,case GNUTLS_E_INTERRUPTED : errno =EINTR ;break ; +case GNUTLS_E_PREMATURE_TERMINATION : + errno =ECONNABORTED ; +break ;default : errno =EIO ;break ;
include/io/channel-tls.h +1 -0
@@ -48,6 +48,7 @@ struct QIOChannelTLS {QIOChannel parent;QIOChannel *master;QCryptoTLSSession *session; +QIOChannelShutdown shutdown; };/**
include/io/channel.h +3 -3
@@ -51,9 +51,9 @@ enum QIOChannelFeature {typedef enum QIOChannelShutdown QIOChannelShutdown ;enum QIOChannelShutdown { -QIO_CHANNEL_SHUTDOWN_BOTH , -QIO_CHANNEL_SHUTDOWN_READ , -QIO_CHANNEL_SHUTDOWN_WRITE , +QIO_CHANNEL_SHUTDOWN_READ =1 , +QIO_CHANNEL_SHUTDOWN_WRITE =2 , +QIO_CHANNEL_SHUTDOWN_BOTH =3 , }; typedef gboolean (*QIOChannelFunc)(QIOChannel *ioc,
io/channel-tls.c +5 -0
@@ -275,6 +275,9 @@ static ssize_t qio_channel_tls_readv(QIOChannel *ioc, }else {return QIO_CHANNEL_ERR_BLOCK ; } + }else if (errno ==ECONNABORTED && + (tioc->shutdown &QIO_CHANNEL_SHUTDOWN_READ )) { +return 0 ; } error_setg_errno(errp, errno,
@@ -357,6 +360,8 @@ static int qio_channel_tls_shutdown(QIOChannel *ioc, {QIOChannelTLS *tioc =QIO_CHANNEL_TLS (ioc); + tioc->shutdown |= how; +return qio_channel_shutdown (tioc->master, how, errp); }
Signature · 1 line
2.19.1
Quoted 15 lines · Daniel P. Berrangé
GNUTLS takes a paranoid approach when seeing 0 bytes returned by the underlying OS
read()function. It will consider this an error and returnGNUTLS_E_PREMATURE_TERMINATIONinstead of propagating the 0 return value. It expects apps to arrange for clean termination at the protocol level and not rely on seeing EOF from a read call to detect shutdown. This is to harden apps against a malicious 3rd party causing termination of the sockets layer.This is unhelpful for the QEMU NBD code which does have a clean protocol level shutdown, but still relies on seeing 0 from the I/O channel read in the coroutine handling incoming replies.
The upshot is that when using a plain NBD connection shutdown is silent, but when using TLS, the client spams the console with
Quoted 8 lines
Cannot read from TLS channel: Broken pipe
The NBD connection has, however, called
qio_channel_shutdown()at this point to indicate that it is done with I/O. This gives the opportunity to optimize the code such that when the channel has been shutdown in the read direction, the error codeGNUTLS_E_PREMATURE_TERMINATIONgets turned into a '0' return instead of an error.
Detecting premature termination when the client has NOT requested orderly shutdown is still important, and this patch preserves that aspect. You are only changing the case where the client has informed the qio code "yes, an early termination is now okay".
Quoted 6 lines
Signed-off-by Daniel P. Berrangé <[email protected]>---
+++
b/include/io/channel.h
include/io/channel.h enum QIOChannelFeature { · line 56
Quoted 9 lines
include/io/channel.h +3
-3@@ -51,9 +51,9 @@ enum QIOChannelFeature {typedef enum QIOChannelShutdown QIOChannelShutdown ;enum QIOChannelShutdown { -QIO_CHANNEL_SHUTDOWN_BOTH , -QIO_CHANNEL_SHUTDOWN_READ , -QIO_CHANNEL_SHUTDOWN_WRITE , +QIO_CHANNEL_SHUTDOWN_READ =1 , +QIO_CHANNEL_SHUTDOWN_WRITE =2 , +QIO_CHANNEL_SHUTDOWN_BOTH =3 ,
Nice use of bit operations :)
io/channel-tls.c static ssize_t qio_channel_tls_readv(QIOChannel *ioc, · line 281
Quoted 9 lines
io/channel-tls.c +4
-0+++ b/io/channel-tls.c@@ -275,6 +275,9 @@ static ssize_t qio_channel_tls_readv(QIOChannel *ioc, }else {return QIO_CHANNEL_ERR_BLOCK ; } + }else if (errno ==ECONNABORTED && + (tioc->shutdown &QIO_CHANNEL_SHUTDOWN_READ )) { +return 0 ; }
I like this patch better than my proposed hack to ignore read errors after requesting quit; testing it now.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org