[PATCH 0/5] crypto: improve error reporting detail

[PATCH 0/5] crypto: improve error reporting detail

[Daniel P. Berrangé]

This small series came about after struggling to diagnose some problems
with TLS, due to unhelpfully generic error messages.

Daniel P. Berrangé (5):
  qapi: allow for g_autoptr(Error) usage
  chardev: add tracing of socket error conditions
  crypto: drop gnutls debug logging support
  crypto: push error reporting into TLS session I/O APIs
  crypto: propagate errors from TLS session I/O callbacks

 chardev/char-socket.c               |  34 +++++----
 chardev/trace-events                |  10 +++
 crypto/init.c                       |  11 ---
 crypto/tlssession.c                 | 110 ++++++++++++++++++++--------
 include/crypto/tlssession.h         |  33 +++++++--
 include/qapi/error.h                |   2 +
 io/channel-tls.c                    |  62 +++++++---------
 tests/unit/test-crypto-tlssession.c |  28 ++++++-
 8 files changed, 189 insertions(+), 101 deletions(-)

-- 
2.45.2

[PATCH 1/5] qapi: allow for g_autoptr(Error) usage

[Daniel P. Berrangé]

While common error propagation practice does not require manually
free'ing of local 'Error' objects, there are some cases where this
is needed. One example is where the 'Error' object is only used
for providing info to a trace event probe. Supporting g_autoptr
avoids the need to manually call 'error_free'.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/qapi/error.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index 71f8fb2c50..6e429809d8 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -437,6 +437,8 @@ Error *error_copy(const Error *err);
  */
 void error_free(Error *err);
 
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(Error, error_free);
+
 /*
  * Convenience function to assert that *@errp is set, then silently free it.
  */
-- 
2.45.2

Re: [PATCH 1/5] qapi: allow for g_autoptr(Error) usage

[Philippe Mathieu-Daudé]

On 22/7/24 15:16, Daniel P. Berrangé wrote:
> While common error propagation practice does not require manually
> free'ing of local 'Error' objects, there are some cases where this
> is needed. One example is where the 'Error' object is only used
> for providing info to a trace event probe. Supporting g_autoptr
> avoids the need to manually call 'error_free'.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   include/qapi/error.h | 2 ++
>   1 file changed, 2 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 1/5] qapi: allow for g_autoptr(Error) usage

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> While common error propagation practice does not require manually
> free'ing of local 'Error' objects, there are some cases where this
> is needed. One example is where the 'Error' object is only used
> for providing info to a trace event probe. Supporting g_autoptr
> avoids the need to manually call 'error_free'.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  include/qapi/error.h | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index 71f8fb2c50..6e429809d8 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -437,6 +437,8 @@ Error *error_copy(const Error *err);
>   */
>  void error_free(Error *err);
>  
> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(Error, error_free);
> +
>  /*
>   * Convenience function to assert that *@errp is set, then silently free it.
>   */

The Error interface is designed for a certain way of using it: an Error
object flows from the spot detecting the error to a spot handling it.
Failure to handle the error is a memory leak.  Our tooling can help with
tracking these down.

The interface tries to make the intended use easy: functions that report
an error consume the Error object.  Explicit error_free() should only
needed when you handle an error in some other way.

When such an explicit error_free() is needed on all paths to return,
then replacing it with auto-freeing is nice.  But what if it isn't?

Say we add a new error path and use error_report_err(err) there.  This
has always been just fine.  No more: if @err is auto-freed, this is a
double-free.  We have to also add err = NULL.  Feels like a trap for
developers to me.

Your use of auto-freeing is in the next patch.  It's this pattern:

    g_autoptr(Error) err = NULL;

    if (!frobnicate(args, &err)) {
        trace_frobnicate_err(..., error_get_pretty(err));
    }

You want to report the error to a trace point.  That's perfectly
legitimate.  The problem is that this kind of error reporting function
does not free, unlike the ones provided by qapi/error.h.

We could extend tracing to accept Error values, so that

        trace_frobnicate_err(..., err);

does free.  Doesn't seem worthwhile unless we find quite a few more uses
for it.

If we conclude we want to provide auto-free as an option, we at least
need to point out the trap in a comment.  A bit of a pain to write, and
whether people will read, understand, and remember it is uncertain.

My gut feeling right now: stick to the design, and free manually.  If
you think my gut is wrong, tell me.

Re: [PATCH 1/5] qapi: allow for g_autoptr(Error) usage

[Daniel P. Berrangé]

On Tue, Jul 23, 2024 at 01:36:32PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > While common error propagation practice does not require manually
> > free'ing of local 'Error' objects, there are some cases where this
> > is needed. One example is where the 'Error' object is only used
> > for providing info to a trace event probe. Supporting g_autoptr
> > avoids the need to manually call 'error_free'.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  include/qapi/error.h | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/include/qapi/error.h b/include/qapi/error.h
> > index 71f8fb2c50..6e429809d8 100644
> > --- a/include/qapi/error.h
> > +++ b/include/qapi/error.h
> > @@ -437,6 +437,8 @@ Error *error_copy(const Error *err);
> >   */
> >  void error_free(Error *err);
> >  
> > +G_DEFINE_AUTOPTR_CLEANUP_FUNC(Error, error_free);
> > +
> >  /*
> >   * Convenience function to assert that *@errp is set, then silently free it.
> >   */
> 
> The Error interface is designed for a certain way of using it: an Error
> object flows from the spot detecting the error to a spot handling it.
> Failure to handle the error is a memory leak.  Our tooling can help with
> tracking these down.
> 
> The interface tries to make the intended use easy: functions that report
> an error consume the Error object.  Explicit error_free() should only
> needed when you handle an error in some other way.
> 
> When such an explicit error_free() is needed on all paths to return,
> then replacing it with auto-freeing is nice.  But what if it isn't?
> 
> Say we add a new error path and use error_report_err(err) there.  This
> has always been just fine.  No more: if @err is auto-freed, this is a
> double-free.  We have to also add err = NULL.  Feels like a trap for
> developers to me.
> 
> Your use of auto-freeing is in the next patch.  It's this pattern:
> 
>     g_autoptr(Error) err = NULL;
> 
>     if (!frobnicate(args, &err)) {
>         trace_frobnicate_err(..., error_get_pretty(err));
>     }
> 
> You want to report the error to a trace point.  That's perfectly
> legitimate.  The problem is that this kind of error reporting function
> does not free, unlike the ones provided by qapi/error.h.
> 
> We could extend tracing to accept Error values, so that
> 
>         trace_frobnicate_err(..., err);
> 
> does free.  Doesn't seem worthwhile unless we find quite a few more uses
> for it.

That is awkward because the trace calls expand to nothing at all
when tracing is disabled, so we can't rely on them to free any
args.


> If we conclude we want to provide auto-free as an option, we at least
> need to point out the trap in a comment.  A bit of a pain to write, and
> whether people will read, understand, and remember it is uncertain.
> 
> My gut feeling right now: stick to the design, and free manually.  If
> you think my gut is wrong, tell me.

I'll drop this since there's only one place benefitting right now.

With 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 :|

Re: [PATCH 1/5] qapi: allow for g_autoptr(Error) usage

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Tue, Jul 23, 2024 at 01:36:32PM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > While common error propagation practice does not require manually
>> > free'ing of local 'Error' objects, there are some cases where this
>> > is needed. One example is where the 'Error' object is only used
>> > for providing info to a trace event probe. Supporting g_autoptr
>> > avoids the need to manually call 'error_free'.
>> >
>> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>> > ---
>> >  include/qapi/error.h | 2 ++
>> >  1 file changed, 2 insertions(+)
>> >
>> > diff --git a/include/qapi/error.h b/include/qapi/error.h
>> > index 71f8fb2c50..6e429809d8 100644
>> > --- a/include/qapi/error.h
>> > +++ b/include/qapi/error.h
>> > @@ -437,6 +437,8 @@ Error *error_copy(const Error *err);
>> >   */
>> >  void error_free(Error *err);
>> >  
>> > +G_DEFINE_AUTOPTR_CLEANUP_FUNC(Error, error_free);
>> > +
>> >  /*
>> >   * Convenience function to assert that *@errp is set, then silently free it.
>> >   */
>> 
>> The Error interface is designed for a certain way of using it: an Error
>> object flows from the spot detecting the error to a spot handling it.
>> Failure to handle the error is a memory leak.  Our tooling can help with
>> tracking these down.
>> 
>> The interface tries to make the intended use easy: functions that report
>> an error consume the Error object.  Explicit error_free() should only
>> needed when you handle an error in some other way.
>> 
>> When such an explicit error_free() is needed on all paths to return,
>> then replacing it with auto-freeing is nice.  But what if it isn't?
>> 
>> Say we add a new error path and use error_report_err(err) there.  This
>> has always been just fine.  No more: if @err is auto-freed, this is a
>> double-free.  We have to also add err = NULL.  Feels like a trap for
>> developers to me.
>> 
>> Your use of auto-freeing is in the next patch.  It's this pattern:
>> 
>>     g_autoptr(Error) err = NULL;
>> 
>>     if (!frobnicate(args, &err)) {
>>         trace_frobnicate_err(..., error_get_pretty(err));
>>     }
>> 
>> You want to report the error to a trace point.  That's perfectly
>> legitimate.  The problem is that this kind of error reporting function
>> does not free, unlike the ones provided by qapi/error.h.
>> 
>> We could extend tracing to accept Error values, so that
>> 
>>         trace_frobnicate_err(..., err);
>> 
>> does free.  Doesn't seem worthwhile unless we find quite a few more uses
>> for it.
>
> That is awkward because the trace calls expand to nothing at all
> when tracing is disabled, so we can't rely on them to free any
> args.

True.

Another idea:

    g_autofree char *errmsg = error_to_pretty(err);
    trace_frobnicate_err(..., errmsg);

where error_to_pretty() frees the error except for err->msg, which it
returns.

>> If we conclude we want to provide auto-free as an option, we at least
>> need to point out the trap in a comment.  A bit of a pain to write, and
>> whether people will read, understand, and remember it is uncertain.
>> 
>> My gut feeling right now: stick to the design, and free manually.  If
>> you think my gut is wrong, tell me.
>
> I'll drop this since there's only one place benefitting right now.

Sensible.  Thanks!

[PATCH 2/5] chardev: add tracing of socket error conditions

[Daniel P. Berrangé]

This adds trace points to every error scenario in the chardev socket
backend that can lead to termination of the connection.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 chardev/char-socket.c | 34 +++++++++++++++++++++-------------
 chardev/trace-events  | 10 ++++++++++
 2 files changed, 31 insertions(+), 13 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 812d7aa38a..582b88e6d8 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -33,6 +33,7 @@
 #include "qapi/clone-visitor.h"
 #include "qapi/qapi-visit-sockets.h"
 #include "qemu/yank.h"
+#include "trace.h"
 
 #include "chardev/char-io.h"
 #include "chardev/char-socket.h"
@@ -126,6 +127,7 @@ static int tcp_chr_write(Chardev *chr, const uint8_t *buf, int len)
         if (ret < 0 && errno != EAGAIN) {
             if (tcp_chr_read_poll(chr) <= 0) {
                 /* Perform disconnect and return error. */
+                trace_chr_socket_poll_err(chr, chr->label);
                 tcp_chr_disconnect_locked(chr);
             } /* else let the read handler finish it properly */
         }
@@ -279,15 +281,16 @@ static ssize_t tcp_chr_recv(Chardev *chr, char *buf, size_t len)
     size_t i;
     int *msgfds = NULL;
     size_t msgfds_num = 0;
+    g_autoptr(Error) err = NULL;
 
     if (qio_channel_has_feature(s->ioc, QIO_CHANNEL_FEATURE_FD_PASS)) {
         ret = qio_channel_readv_full(s->ioc, &iov, 1,
                                      &msgfds, &msgfds_num,
-                                     0, NULL);
+                                     0, &err);
     } else {
         ret = qio_channel_readv_full(s->ioc, &iov, 1,
                                      NULL, NULL,
-                                     0, NULL);
+                                     0, &err);
     }
 
     if (msgfds_num) {
@@ -322,7 +325,10 @@ static ssize_t tcp_chr_recv(Chardev *chr, char *buf, size_t len)
         errno = EAGAIN;
         ret = -1;
     } else if (ret == -1) {
+        trace_chr_socket_recv_err(chr, chr->label, error_get_pretty(err));
         errno = EIO;
+    } else if (ret == 0) {
+        trace_chr_socket_recv_eof(chr, chr->label);
     }
 
     return ret;
@@ -463,6 +469,7 @@ static void tcp_chr_disconnect_locked(Chardev *chr)
     SocketChardev *s = SOCKET_CHARDEV(chr);
     bool emit_close = s->state == TCP_CHARDEV_STATE_CONNECTED;
 
+    trace_chr_socket_disconnect(chr, chr->label);
     tcp_chr_free_connection(chr);
 
     if (s->listener) {
@@ -521,6 +528,7 @@ static gboolean tcp_chr_hup(QIOChannel *channel,
                                void *opaque)
 {
     Chardev *chr = CHARDEV(opaque);
+    trace_chr_socket_hangup(chr, chr->label);
     tcp_chr_disconnect(chr);
     return G_SOURCE_REMOVE;
 }
@@ -672,15 +680,17 @@ static gboolean tcp_chr_telnet_init_io(QIOChannel *ioc,
     SocketChardev *s = user_data;
     Chardev *chr = CHARDEV(s);
     TCPChardevTelnetInit *init = s->telnet_init;
+    g_autoptr(Error) err = NULL;
     ssize_t ret;
 
     assert(init);
 
-    ret = qio_channel_write(ioc, init->buf, init->buflen, NULL);
+    ret = qio_channel_write(ioc, init->buf, init->buflen, &err);
     if (ret < 0) {
         if (ret == QIO_CHANNEL_ERR_BLOCK) {
             ret = 0;
         } else {
+            trace_chr_socket_write_err(chr, chr->label, error_get_pretty(err));
             tcp_chr_disconnect(chr);
             goto end;
         }
@@ -762,12 +772,10 @@ static void tcp_chr_websock_handshake(QIOTask *task, gpointer user_data)
 {
     Chardev *chr = user_data;
     SocketChardev *s = user_data;
-    Error *err = NULL;
+    g_autoptr(Error) err = NULL;
 
     if (qio_task_propagate_error(task, &err)) {
-        error_reportf_err(err,
-                          "websock handshake of character device %s failed: ",
-                          chr->label);
+        trace_chr_socket_ws_handshake_err(chr, chr->label, error_get_pretty(err));
         tcp_chr_disconnect(chr);
     } else {
         if (s->do_telnetopt) {
@@ -802,12 +810,10 @@ static void tcp_chr_tls_handshake(QIOTask *task,
 {
     Chardev *chr = user_data;
     SocketChardev *s = user_data;
-    Error *err = NULL;
+    g_autoptr(Error) err = NULL;
 
     if (qio_task_propagate_error(task, &err)) {
-        error_reportf_err(err,
-                          "TLS handshake of character device %s failed: ",
-                          chr->label);
+        trace_chr_socket_tls_handshake_err(chr, chr->label, error_get_pretty(err));
         tcp_chr_disconnect(chr);
     } else {
         if (s->is_websock) {
@@ -826,19 +832,21 @@ static void tcp_chr_tls_init(Chardev *chr)
     SocketChardev *s = SOCKET_CHARDEV(chr);
     QIOChannelTLS *tioc;
     gchar *name;
+    g_autoptr(Error) err = NULL;
 
     if (s->is_listen) {
         tioc = qio_channel_tls_new_server(
             s->ioc, s->tls_creds,
             s->tls_authz,
-            NULL);
+            &err);
     } else {
         tioc = qio_channel_tls_new_client(
             s->ioc, s->tls_creds,
             s->addr->u.inet.host,
-            NULL);
+            &err);
     }
     if (tioc == NULL) {
+        trace_chr_socket_tls_init_err(chr, chr->label, error_get_pretty(err));
         tcp_chr_disconnect(chr);
         return;
     }
diff --git a/chardev/trace-events b/chardev/trace-events
index 027107b0c1..7e97b8a988 100644
--- a/chardev/trace-events
+++ b/chardev/trace-events
@@ -17,3 +17,13 @@ spice_vmc_register_interface(void *scd) "spice vmc registered interface %p"
 spice_vmc_unregister_interface(void *scd) "spice vmc unregistered interface %p"
 spice_vmc_event(int event) "spice vmc event %d"
 
+# char-socket.c
+chr_socket_poll_err(void *chrdev, const char *label) "chardev socket poll error %p (%s)"
+chr_socket_recv_err(void *chrdev, const char *label, const char *err) "chardev socket recv error %p (%s): %s"
+chr_socket_recv_eof(void *chrdev, const char *label) "chardev socket recv end-of-file %p (%s)"
+chr_socket_write_err(void *chrdev, const char *label, const char *err) "chardev socket write error %p (%s): %s"
+chr_socket_disconnect(void *chrdev, const char *label) "chardev socket disconnect %p (%s)"
+chr_socket_hangup(void *chrdev, const char *label) "chardev socket hangup %p (%s)"
+chr_socket_ws_handshake_err(void *chrdev, const char *label, const char *err) "chardev socket websock handshake error %p (%s): %s"
+chr_socket_tls_handshake_err(void *chrdev, const char *label, const char *err) "chardev socket TLS handshake error %p (%s): %s"
+chr_socket_tls_init_err(void *chrdev, const char *label, const char *err) "chardev socket TLS init error %p (%s): %s"
-- 
2.45.2

Re: [PATCH 2/5] chardev: add tracing of socket error conditions

[Philippe Mathieu-Daudé]

On 22/7/24 15:16, Daniel P. Berrangé wrote:
> This adds trace points to every error scenario in the chardev socket
> backend that can lead to termination of the connection.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   chardev/char-socket.c | 34 +++++++++++++++++++++-------------
>   chardev/trace-events  | 10 ++++++++++
>   2 files changed, 31 insertions(+), 13 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 3/5] crypto: drop gnutls debug logging support

[Daniel P. Berrangé]

GNUTLS already supports dynamically enabling its logging at runtime by
setting the env var 'GNUTLS_DEBUG_LEVEL=10', so there is no need to
re-invent this logic in QEMU in a way that requires a re-compile.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 crypto/init.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/crypto/init.c b/crypto/init.c
index fb7f1bff10..2d6dfa3091 100644
--- a/crypto/init.c
+++ b/crypto/init.c
@@ -34,13 +34,6 @@
 
 #include "crypto/random.h"
 
-/* #define DEBUG_GNUTLS */
-#ifdef DEBUG_GNUTLS
-static void qcrypto_gnutls_log(int level, const char *str)
-{
-    fprintf(stderr, "%d: %s", level, str);
-}
-#endif
 
 int qcrypto_init(Error **errp)
 {
@@ -53,10 +46,6 @@ int qcrypto_init(Error **errp)
                    gnutls_strerror(ret));
         return -1;
     }
-#ifdef DEBUG_GNUTLS
-    gnutls_global_set_log_level(10);
-    gnutls_global_set_log_function(qcrypto_gnutls_log);
-#endif
 #endif
 
 #ifdef CONFIG_GCRYPT
-- 
2.45.2

Re: [PATCH 3/5] crypto: drop gnutls debug logging support

[Philippe Mathieu-Daudé]

On 22/7/24 15:16, Daniel P. Berrangé wrote:
> GNUTLS already supports dynamically enabling its logging at runtime by
> setting the env var 'GNUTLS_DEBUG_LEVEL=10', so there is no need to
> re-invent this logic in QEMU in a way that requires a re-compile.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   crypto/init.c | 11 -----------
>   1 file changed, 11 deletions(-)
> 
> diff --git a/crypto/init.c b/crypto/init.c
> index fb7f1bff10..2d6dfa3091 100644
> --- a/crypto/init.c
> +++ b/crypto/init.c
> @@ -34,13 +34,6 @@
>   
>   #include "crypto/random.h"
>   
> -/* #define DEBUG_GNUTLS */

Maybe mention GNUTLS_DEBUG_LEVEL=10 here or in header? Otherwise

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 3/5] crypto: drop gnutls debug logging support

[Daniel P. Berrangé]

On Mon, Jul 22, 2024 at 04:32:23PM +0200, Philippe Mathieu-Daudé wrote:
> On 22/7/24 15:16, Daniel P. Berrangé wrote:
> > GNUTLS already supports dynamically enabling its logging at runtime by
> > setting the env var 'GNUTLS_DEBUG_LEVEL=10', so there is no need to
> > re-invent this logic in QEMU in a way that requires a re-compile.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >   crypto/init.c | 11 -----------
> >   1 file changed, 11 deletions(-)
> > 
> > diff --git a/crypto/init.c b/crypto/init.c
> > index fb7f1bff10..2d6dfa3091 100644
> > --- a/crypto/init.c
> > +++ b/crypto/init.c
> > @@ -34,13 +34,6 @@
> >   #include "crypto/random.h"
> > -/* #define DEBUG_GNUTLS */
> 
> Maybe mention GNUTLS_DEBUG_LEVEL=10 here or in header? Otherwise

I'm adding

/*
 * To debug GNUTLS see env vars listed in
 * https://gnutls.org/manual/html_node/Debugging-and-auditing.html
 */



> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 

With 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 4/5] crypto: push error reporting into TLS session I/O APIs

[Daniel P. Berrangé]

The current TLS session I/O APIs just return a synthetic errno
value on error, which has been translated from a gnutls error
value. This looses a large amount of valuable information that
distinguishes different scenarios.

Pushing population of the "Error *errp" object into the TLS
session I/O APIs gives more detailed error information.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 crypto/tlssession.c         | 47 ++++++++++++++++---------------------
 include/crypto/tlssession.h | 23 ++++++++++++++----
 io/channel-tls.c            | 44 ++++++++++++++--------------------
 3 files changed, 57 insertions(+), 57 deletions(-)

diff --git a/crypto/tlssession.c b/crypto/tlssession.c
index 1e98f44e0d..f0076a4add 100644
--- a/crypto/tlssession.c
+++ b/crypto/tlssession.c
@@ -441,23 +441,19 @@ qcrypto_tls_session_set_callbacks(QCryptoTLSSession *session,
 ssize_t
 qcrypto_tls_session_write(QCryptoTLSSession *session,
                           const char *buf,
-                          size_t len)
+                          size_t len,
+                          Error **errp)
 {
     ssize_t ret = gnutls_record_send(session->handle, buf, len);
 
     if (ret < 0) {
-        switch (ret) {
-        case GNUTLS_E_AGAIN:
-            errno = EAGAIN;
-            break;
-        case GNUTLS_E_INTERRUPTED:
-            errno = EINTR;
-            break;
-        default:
-            errno = EIO;
-            break;
+        if (ret == GNUTLS_E_AGAIN) {
+            return QCRYPTO_TLS_SESSION_ERR_BLOCK;
+        } else {
+            error_setg(errp,
+                       "Cannot write to TLS channel: %s", gnutls_strerror(ret));
+            return -1;
         }
-        ret = -1;
     }
 
     return ret;
@@ -467,26 +463,23 @@ qcrypto_tls_session_write(QCryptoTLSSession *session,
 ssize_t
 qcrypto_tls_session_read(QCryptoTLSSession *session,
                          char *buf,
-                         size_t len)
+                         size_t len,
+                         bool gracefulTermination,
+                         Error **errp)
 {
     ssize_t ret = gnutls_record_recv(session->handle, buf, len);
 
     if (ret < 0) {
-        switch (ret) {
-        case GNUTLS_E_AGAIN:
-            errno = EAGAIN;
-            break;
-        case GNUTLS_E_INTERRUPTED:
-            errno = EINTR;
-            break;
-        case GNUTLS_E_PREMATURE_TERMINATION:
-            errno = ECONNABORTED;
-            break;
-        default:
-            errno = EIO;
-            break;
+        if (ret == GNUTLS_E_AGAIN) {
+            return QCRYPTO_TLS_SESSION_ERR_BLOCK;
+        } else if ((ret == GNUTLS_E_PREMATURE_TERMINATION) &&
+                   gracefulTermination){
+            return 0;
+        } else {
+            error_setg(errp,
+                       "Cannot read from TLS channel: %s", gnutls_strerror(ret));
+            return -1;
         }
-        ret = -1;
     }
 
     return ret;
diff --git a/include/crypto/tlssession.h b/include/crypto/tlssession.h
index 571049bd0e..291e602540 100644
--- a/include/crypto/tlssession.h
+++ b/include/crypto/tlssession.h
@@ -107,6 +107,7 @@
 
 typedef struct QCryptoTLSSession QCryptoTLSSession;
 
+#define QCRYPTO_TLS_SESSION_ERR_BLOCK -2
 
 /**
  * qcrypto_tls_session_new:
@@ -212,6 +213,7 @@ void qcrypto_tls_session_set_callbacks(QCryptoTLSSession *sess,
  * @sess: the TLS session object
  * @buf: the plain text to send
  * @len: the length of @buf
+ * @errp: pointer to hold returned error object
  *
  * Encrypt @len bytes of the data in @buf and send
  * it to the remote peer using the callback previously
@@ -221,32 +223,45 @@ void qcrypto_tls_session_set_callbacks(QCryptoTLSSession *sess,
  * qcrypto_tls_session_get_handshake_status() returns
  * QCRYPTO_TLS_HANDSHAKE_COMPLETE
  *
- * Returns: the number of bytes sent, or -1 on error
+ * Returns: the number of bytes sent,
+ * or QCRYPTO_TLS_SESSION_ERR_BLOCK if the write would block,
+ * or -1 on error.
  */
 ssize_t qcrypto_tls_session_write(QCryptoTLSSession *sess,
                                   const char *buf,
-                                  size_t len);
+                                  size_t len,
+                                  Error **errp);
 
 /**
  * qcrypto_tls_session_read:
  * @sess: the TLS session object
  * @buf: to fill with plain text received
  * @len: the length of @buf
+ * @gracefulTermination: treat premature termination as graceful EOF
+ * @errp: pointer to hold returned error object
  *
  * Receive up to @len bytes of data from the remote peer
  * using the callback previously registered with
  * qcrypto_tls_session_set_callbacks(), decrypt it and
  * store it in @buf.
  *
+ * If @gracefulTermination is true, then a premature termination
+ * of the TLS session will be treated as indicating EOF, as
+ * opposed to an error.
+ *
  * It is an error to call this before
  * qcrypto_tls_session_get_handshake_status() returns
  * QCRYPTO_TLS_HANDSHAKE_COMPLETE
  *
- * Returns: the number of bytes received, or -1 on error
+ * Returns: the number of bytes received,
+ * or QCRYPTO_TLS_SESSION_ERR_BLOCK if the receive would block,
+ * or -1 on error.
  */
 ssize_t qcrypto_tls_session_read(QCryptoTLSSession *sess,
                                  char *buf,
-                                 size_t len);
+                                 size_t len,
+                                 bool gracefulTermination,
+                                 Error **errp);
 
 /**
  * qcrypto_tls_session_check_pending:
diff --git a/io/channel-tls.c b/io/channel-tls.c
index 67b9700006..eab64b0706 100644
--- a/io/channel-tls.c
+++ b/io/channel-tls.c
@@ -279,22 +279,17 @@ static ssize_t qio_channel_tls_readv(QIOChannel *ioc,
     for (i = 0 ; i < niov ; i++) {
         ssize_t ret = qcrypto_tls_session_read(tioc->session,
                                                iov[i].iov_base,
-                                               iov[i].iov_len);
-        if (ret < 0) {
-            if (errno == EAGAIN) {
-                if (got) {
-                    return got;
-                } else {
-                    return QIO_CHANNEL_ERR_BLOCK;
-                }
-            } else if (errno == ECONNABORTED &&
-                       (qatomic_load_acquire(&tioc->shutdown) &
-                        QIO_CHANNEL_SHUTDOWN_READ)) {
-                return 0;
+                                               iov[i].iov_len,
+                                               (qatomic_load_acquire(&tioc->shutdown) &
+                                                QIO_CHANNEL_SHUTDOWN_READ),
+                                               errp);
+        if (ret == QCRYPTO_TLS_SESSION_ERR_BLOCK) {
+            if (got) {
+                return got;
+            } else {
+                return QIO_CHANNEL_ERR_BLOCK;
             }
-
-            error_setg_errno(errp, errno,
-                             "Cannot read from TLS channel");
+        } else if (ret < 0) {
             return -1;
         }
         got += ret;
@@ -321,18 +316,15 @@ static ssize_t qio_channel_tls_writev(QIOChannel *ioc,
     for (i = 0 ; i < niov ; i++) {
         ssize_t ret = qcrypto_tls_session_write(tioc->session,
                                                 iov[i].iov_base,
-                                                iov[i].iov_len);
-        if (ret <= 0) {
-            if (errno == EAGAIN) {
-                if (done) {
-                    return done;
-                } else {
-                    return QIO_CHANNEL_ERR_BLOCK;
-                }
+                                                iov[i].iov_len,
+                                                errp);
+        if (ret == QCRYPTO_TLS_SESSION_ERR_BLOCK) {
+            if (done) {
+                return done;
+            } else {
+                return QIO_CHANNEL_ERR_BLOCK;
             }
-
-            error_setg_errno(errp, errno,
-                             "Cannot write to TLS channel");
+        } else if (ret < 0) {
             return -1;
         }
         done += ret;
-- 
2.45.2

Re: [PATCH 4/5] crypto: push error reporting into TLS session I/O APIs

[Philippe Mathieu-Daudé]

On 22/7/24 15:16, Daniel P. Berrangé wrote:
> The current TLS session I/O APIs just return a synthetic errno
> value on error, which has been translated from a gnutls error
> value. This looses a large amount of valuable information that
> distinguishes different scenarios.
> 
> Pushing population of the "Error *errp" object into the TLS
> session I/O APIs gives more detailed error information.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   crypto/tlssession.c         | 47 ++++++++++++++++---------------------
>   include/crypto/tlssession.h | 23 ++++++++++++++----
>   io/channel-tls.c            | 44 ++++++++++++++--------------------
>   3 files changed, 57 insertions(+), 57 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 5/5] crypto: propagate errors from TLS session I/O callbacks

[Daniel P. Berrangé]

GNUTLS doesn't know how to perform I/O on anything other than plain
FDs, so the TLS session provides it with some I/O callbacks. The
GNUTLS API design requires these callbacks to return a unix errno
value, which means we're currently loosing the useful QEMU "Error"
object.

This changes the I/O callbacks in QEMU to stash the "Error" object
in the QCryptoTLSSession class, and fetch it when seeing an I/O
error returned from GNUTLS, thus preserving useful error messages.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 crypto/tlssession.c                 | 71 +++++++++++++++++++++++++----
 include/crypto/tlssession.h         | 10 +++-
 io/channel-tls.c                    | 18 ++++----
 tests/unit/test-crypto-tlssession.c | 28 ++++++++++--
 4 files changed, 103 insertions(+), 24 deletions(-)

diff --git a/crypto/tlssession.c b/crypto/tlssession.c
index f0076a4add..e5cbe86bf0 100644
--- a/crypto/tlssession.c
+++ b/crypto/tlssession.c
@@ -44,6 +44,13 @@ struct QCryptoTLSSession {
     QCryptoTLSSessionReadFunc readFunc;
     void *opaque;
     char *peername;
+
+    /*
+     * Allow concurrent reads and writes, so track
+     * errors separately
+     */
+    Error *rerr;
+    Error *werr;
 };
 
 
@@ -54,6 +61,9 @@ qcrypto_tls_session_free(QCryptoTLSSession *session)
         return;
     }
 
+    error_free(session->rerr);
+    error_free(session->werr);
+
     gnutls_deinit(session->handle);
     g_free(session->hostname);
     g_free(session->peername);
@@ -67,13 +77,26 @@ static ssize_t
 qcrypto_tls_session_push(void *opaque, const void *buf, size_t len)
 {
     QCryptoTLSSession *session = opaque;
+    ssize_t ret;
 
     if (!session->writeFunc) {
         errno = EIO;
         return -1;
     };
 
-    return session->writeFunc(buf, len, session->opaque);
+    error_free(session->werr);
+    session->werr = NULL;
+
+    ret = session->writeFunc(buf, len, session->opaque, &session->werr);
+    if (ret == QCRYPTO_TLS_SESSION_ERR_BLOCK) {
+        errno = EAGAIN;
+        return -1;
+    } else if (ret < 0) {
+        errno = EIO;
+        return -1;
+    } else {
+        return ret;
+    }
 }
 
 
@@ -81,13 +104,26 @@ static ssize_t
 qcrypto_tls_session_pull(void *opaque, void *buf, size_t len)
 {
     QCryptoTLSSession *session = opaque;
+    ssize_t ret;
 
     if (!session->readFunc) {
         errno = EIO;
         return -1;
     };
 
-    return session->readFunc(buf, len, session->opaque);
+    error_free(session->rerr);
+    session->rerr = NULL;
+
+    ret = session->readFunc(buf, len, session->opaque, &session->rerr);
+    if (ret == QCRYPTO_TLS_SESSION_ERR_BLOCK) {
+        errno = EAGAIN;
+        return -1;
+    } else if (ret < 0) {
+        errno = EIO;
+        return -1;
+    } else {
+        return ret;
+    }
 }
 
 #define TLS_PRIORITY_ADDITIONAL_ANON "+ANON-DH"
@@ -450,8 +486,13 @@ qcrypto_tls_session_write(QCryptoTLSSession *session,
         if (ret == GNUTLS_E_AGAIN) {
             return QCRYPTO_TLS_SESSION_ERR_BLOCK;
         } else {
-            error_setg(errp,
-                       "Cannot write to TLS channel: %s", gnutls_strerror(ret));
+            if (session->werr) {
+                error_propagate(errp, session->werr);
+                session->werr = NULL;
+            } else {
+                error_setg(errp,
+                           "Cannot write to TLS channel: %s", gnutls_strerror(ret));
+            }
             return -1;
         }
     }
@@ -476,8 +517,13 @@ qcrypto_tls_session_read(QCryptoTLSSession *session,
                    gracefulTermination){
             return 0;
         } else {
-            error_setg(errp,
-                       "Cannot read from TLS channel: %s", gnutls_strerror(ret));
+            if (session->rerr) {
+                error_propagate(errp, session->rerr);
+                session->rerr = NULL;
+            } else {
+                error_setg(errp,
+                           "Cannot read from TLS channel: %s", gnutls_strerror(ret));
+            }
             return -1;
         }
     }
@@ -505,11 +551,20 @@ qcrypto_tls_session_handshake(QCryptoTLSSession *session,
             ret == GNUTLS_E_AGAIN) {
             ret = 1;
         } else {
-            error_setg(errp, "TLS handshake failed: %s",
-                       gnutls_strerror(ret));
+            if (session->rerr || session->werr) {
+                error_setg(errp, "TLS handshake failed: %s: %s",
+                           gnutls_strerror(ret),
+                           error_get_pretty(session->rerr ? session->rerr : session->werr));
+            } else {
+                error_setg(errp, "TLS handshake failed: %s",
+                           gnutls_strerror(ret));
+            }
             ret = -1;
         }
     }
+    error_free(session->rerr);
+    error_free(session->werr);
+    session->rerr = session->werr = NULL;
 
     return ret;
 }
diff --git a/include/crypto/tlssession.h b/include/crypto/tlssession.h
index 291e602540..f694a5c3c5 100644
--- a/include/crypto/tlssession.h
+++ b/include/crypto/tlssession.h
@@ -178,12 +178,18 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(QCryptoTLSSession, qcrypto_tls_session_free)
 int qcrypto_tls_session_check_credentials(QCryptoTLSSession *sess,
                                           Error **errp);
 
+/*
+ * These must return QCRYPTO_TLS_SESSION_ERR_BLOCK if the I/O
+ * would block, but on other errors, must fill 'errp'
+ */
 typedef ssize_t (*QCryptoTLSSessionWriteFunc)(const char *buf,
                                               size_t len,
-                                              void *opaque);
+                                              void *opaque,
+                                              Error **errp);
 typedef ssize_t (*QCryptoTLSSessionReadFunc)(char *buf,
                                              size_t len,
-                                             void *opaque);
+                                             void *opaque,
+                                             Error **errp);
 
 /**
  * qcrypto_tls_session_set_callbacks:
diff --git a/io/channel-tls.c b/io/channel-tls.c
index eab64b0706..980dc5b89f 100644
--- a/io/channel-tls.c
+++ b/io/channel-tls.c
@@ -28,17 +28,16 @@
 
 static ssize_t qio_channel_tls_write_handler(const char *buf,
                                              size_t len,
-                                             void *opaque)
+                                             void *opaque,
+                                             Error **errp)
 {
     QIOChannelTLS *tioc = QIO_CHANNEL_TLS(opaque);
     ssize_t ret;
 
-    ret = qio_channel_write(tioc->master, buf, len, NULL);
+    ret = qio_channel_write(tioc->master, buf, len, errp);
     if (ret == QIO_CHANNEL_ERR_BLOCK) {
-        errno = EAGAIN;
-        return -1;
+        return QCRYPTO_TLS_SESSION_ERR_BLOCK;
     } else if (ret < 0) {
-        errno = EIO;
         return -1;
     }
     return ret;
@@ -46,17 +45,16 @@ static ssize_t qio_channel_tls_write_handler(const char *buf,
 
 static ssize_t qio_channel_tls_read_handler(char *buf,
                                             size_t len,
-                                            void *opaque)
+                                            void *opaque,
+                                            Error **errp)
 {
     QIOChannelTLS *tioc = QIO_CHANNEL_TLS(opaque);
     ssize_t ret;
 
-    ret = qio_channel_read(tioc->master, buf, len, NULL);
+    ret = qio_channel_read(tioc->master, buf, len, errp);
     if (ret == QIO_CHANNEL_ERR_BLOCK) {
-        errno = EAGAIN;
-        return -1;
+        return QCRYPTO_TLS_SESSION_ERR_BLOCK;
     } else if (ret < 0) {
-        errno = EIO;
         return -1;
     }
     return ret;
diff --git a/tests/unit/test-crypto-tlssession.c b/tests/unit/test-crypto-tlssession.c
index b12e7b6879..34f1f4e1a4 100644
--- a/tests/unit/test-crypto-tlssession.c
+++ b/tests/unit/test-crypto-tlssession.c
@@ -35,18 +35,38 @@
 #define PSKFILE WORKDIR "keys.psk"
 #define KEYFILE WORKDIR "key-ctx.pem"
 
-static ssize_t testWrite(const char *buf, size_t len, void *opaque)
+static ssize_t testWrite(const char *buf, size_t len, void *opaque, Error **errp)
 {
     int *fd = opaque;
+    int ret;
 
-    return write(*fd, buf, len);
+    ret = write(*fd, buf, len);
+    if (ret < 0) {
+        if (errno == EAGAIN) {
+            return QCRYPTO_TLS_SESSION_ERR_BLOCK;
+        } else {
+            error_setg_errno(errp, errno, "unable to write");
+            return -1;
+        }
+    }
+    return ret;
 }
 
-static ssize_t testRead(char *buf, size_t len, void *opaque)
+static ssize_t testRead(char *buf, size_t len, void *opaque, Error **errp)
 {
     int *fd = opaque;
+    int ret;
 
-    return read(*fd, buf, len);
+    ret = read(*fd, buf, len);
+    if (ret < 0) {
+        if (errno == EAGAIN) {
+            return QCRYPTO_TLS_SESSION_ERR_BLOCK;
+        } else {
+            error_setg_errno(errp, errno, "unable to read");
+            return -1;
+        }
+    }
+    return ret;
 }
 
 static QCryptoTLSCreds *test_tls_creds_psk_create(
-- 
2.45.2

Re: [PATCH 5/5] crypto: propagate errors from TLS session I/O callbacks

[Philippe Mathieu-Daudé]

On 22/7/24 15:16, Daniel P. Berrangé wrote:
> GNUTLS doesn't know how to perform I/O on anything other than plain
> FDs, so the TLS session provides it with some I/O callbacks. The
> GNUTLS API design requires these callbacks to return a unix errno
> value, which means we're currently loosing the useful QEMU "Error"
> object.
> 
> This changes the I/O callbacks in QEMU to stash the "Error" object
> in the QCryptoTLSSession class, and fetch it when seeing an I/O
> error returned from GNUTLS, thus preserving useful error messages.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   crypto/tlssession.c                 | 71 +++++++++++++++++++++++++----
>   include/crypto/tlssession.h         | 10 +++-
>   io/channel-tls.c                    | 18 ++++----
>   tests/unit/test-crypto-tlssession.c | 28 ++++++++++--
>   4 files changed, 103 insertions(+), 24 deletions(-)


> @@ -505,11 +551,20 @@ qcrypto_tls_session_handshake(QCryptoTLSSession *session,
>               ret == GNUTLS_E_AGAIN) {
>               ret = 1;
>           } else {
> -            error_setg(errp, "TLS handshake failed: %s",
> -                       gnutls_strerror(ret));
> +            if (session->rerr || session->werr) {
> +                error_setg(errp, "TLS handshake failed: %s: %s",
> +                           gnutls_strerror(ret),
> +                           error_get_pretty(session->rerr ? session->rerr : session->werr));

Could leverage ternary operator here.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> +            } else {
> +                error_setg(errp, "TLS handshake failed: %s",
> +                           gnutls_strerror(ret));
> +            }
>               ret = -1;
>           }
>       }
> +    error_free(session->rerr);
> +    error_free(session->werr);
> +    session->rerr = session->werr = NULL;
>   
>       return ret;
>   }