Peter Xu <[email protected]> writes: > No issue I hit, the change is only from code observation when I am looking > at a TLS premature termination issue. > > qio_channel_tls_bye() API needs to be synchronous. When it's not, the > previous impl will attach an asynchronous task retrying but only until when > the channel gets the relevant GIO event. It may be problematic, because the > caller of qio_channel_tls_bye() may have invoked channel close() before > that, leading to premature termination of the TLS session. >
I'm not super versed on socket APIs, so bear with me: Wouldn't the subsequent shutdown() before close() ensure that the io watch gets triggered? Assuming we're atomically installing the watch before the shutdown() (at the moment, we're not). > Remove the asynchronous handling, instead retry it immediately. Currently, > the only two possible cases that may lead to async task is either INTERRUPT > or EAGAIN. It should be suffice to spin retry as of now, until a solid > proof showing that a more complicated retry logic is needed. > > With that, we can remove the whole async model for the bye task. > With the bye() being synchronous, do we still have the issue when migration fails? I guess it depends on what the answer to my question above is... > When at it, making the function return bool, which looks like a common > pattern in QEMU when errp is used. > > Side note on the tracepoints: previously the tracepoint bye_complete() > isn't used. Start to use it in this patch. bye_pending() and bye_cancel() > can be dropped now. Adding bye_retry() instead. > > Signed-off-by: Peter Xu <[email protected]> > --- > include/io/channel-tls.h | 5 ++- > io/channel-tls.c | 86 +++++----------------------------------- > io/trace-events | 3 +- > 3 files changed, 15 insertions(+), 79 deletions(-) > > diff --git a/include/io/channel-tls.h b/include/io/channel-tls.h > index 7e9023570d..bcd14ffbd6 100644 > --- a/include/io/channel-tls.h > +++ b/include/io/channel-tls.h > @@ -49,7 +49,6 @@ struct QIOChannelTLS { > QCryptoTLSSession *session; > QIOChannelShutdown shutdown; > guint hs_ioc_tag; > - guint bye_ioc_tag; > }; > > /** > @@ -60,8 +59,10 @@ struct QIOChannelTLS { > * Perform the TLS session termination. This method will return > * immediately and the termination will continue in the background, > * provided the main loop is running. > + * > + * Returns: true on success, false on error (with errp set) > */ > -void qio_channel_tls_bye(QIOChannelTLS *ioc, Error **errp); > +bool qio_channel_tls_bye(QIOChannelTLS *ioc, Error **errp); > > /** > * qio_channel_tls_new_server: > diff --git a/io/channel-tls.c b/io/channel-tls.c > index 5a2c8188ce..8510a187a8 100644 > --- a/io/channel-tls.c > +++ b/io/channel-tls.c > @@ -253,84 +253,25 @@ void qio_channel_tls_handshake(QIOChannelTLS *ioc, > qio_channel_tls_handshake_task(ioc, task, context); > } > > -static gboolean qio_channel_tls_bye_io(QIOChannel *ioc, GIOCondition > condition, > - gpointer user_data); > - > -static void qio_channel_tls_bye_task(QIOChannelTLS *ioc, QIOTask *task, > - GMainContext *context) > +bool qio_channel_tls_bye(QIOChannelTLS *ioc, Error **errp) > { > - GIOCondition condition; > - QIOChannelTLSData *data; > int status; > - Error *err = NULL; > > - status = qcrypto_tls_session_bye(ioc->session, &err); > + trace_qio_channel_tls_bye_start(ioc); > +retry: > + status = qcrypto_tls_session_bye(ioc->session, errp); > > if (status < 0) { > trace_qio_channel_tls_bye_fail(ioc); > - qio_task_set_error(task, err); > - qio_task_complete(task); > - return; > - } > - > - if (status == QCRYPTO_TLS_BYE_COMPLETE) { > - qio_task_complete(task); > - return; > - } > - > - data = g_new0(typeof(*data), 1); > - data->task = task; > - data->context = context; > - > - if (context) { > - g_main_context_ref(context); > - } > - > - if (status == QCRYPTO_TLS_BYE_SENDING) { > - condition = G_IO_OUT; > - } else { > - condition = G_IO_IN; > - } > - > - trace_qio_channel_tls_bye_pending(ioc, status); > - ioc->bye_ioc_tag = qio_channel_add_watch_full(ioc->master, condition, > - qio_channel_tls_bye_io, > - data, NULL, context); > -} > - > - > -static gboolean qio_channel_tls_bye_io(QIOChannel *ioc, GIOCondition > condition, > - gpointer user_data) > -{ > - QIOChannelTLSData *data = user_data; > - QIOTask *task = data->task; > - GMainContext *context = data->context; > - QIOChannelTLS *tioc = QIO_CHANNEL_TLS(qio_task_get_source(task)); > - > - tioc->bye_ioc_tag = 0; > - g_free(data); > - qio_channel_tls_bye_task(tioc, task, context); > - > - if (context) { > - g_main_context_unref(context); > + return false; > + } else if (status != QCRYPTO_TLS_BYE_COMPLETE) { > + /* BYE event must be synchronous, retry immediately */ > + trace_qio_channel_tls_bye_retry(ioc, status); > + goto retry; > } > > - return FALSE; > -} > - > -static void propagate_error(QIOTask *task, gpointer opaque) > -{ > - qio_task_propagate_error(task, opaque); > -} > - > -void qio_channel_tls_bye(QIOChannelTLS *ioc, Error **errp) > -{ > - QIOTask *task; > - > - task = qio_task_new(OBJECT(ioc), propagate_error, errp, NULL); > - > - trace_qio_channel_tls_bye_start(ioc); > - qio_channel_tls_bye_task(ioc, task, NULL); > + trace_qio_channel_tls_bye_complete(ioc); > + return true; > } > > static void qio_channel_tls_init(Object *obj G_GNUC_UNUSED) > @@ -482,11 +423,6 @@ static int qio_channel_tls_close(QIOChannel *ioc, > g_clear_handle_id(&tioc->hs_ioc_tag, g_source_remove); > } > > - if (tioc->bye_ioc_tag) { > - trace_qio_channel_tls_bye_cancel(ioc); > - g_clear_handle_id(&tioc->bye_ioc_tag, g_source_remove); > - } > - > return qio_channel_close(tioc->master, errp); > } > > diff --git a/io/trace-events b/io/trace-events > index dc3a63ba1f..67b3814192 100644 > --- a/io/trace-events > +++ b/io/trace-events > @@ -45,10 +45,9 @@ qio_channel_tls_handshake_fail(void *ioc) "TLS handshake > fail ioc=%p" > qio_channel_tls_handshake_complete(void *ioc) "TLS handshake complete ioc=%p" > qio_channel_tls_handshake_cancel(void *ioc) "TLS handshake cancel ioc=%p" > qio_channel_tls_bye_start(void *ioc) "TLS termination start ioc=%p" > -qio_channel_tls_bye_pending(void *ioc, int status) "TLS termination pending > ioc=%p status=%d" > +qio_channel_tls_bye_retry(void *ioc, int status) "TLS termination pending > ioc=%p status=%d" > qio_channel_tls_bye_fail(void *ioc) "TLS termination fail ioc=%p" > qio_channel_tls_bye_complete(void *ioc) "TLS termination complete ioc=%p" > -qio_channel_tls_bye_cancel(void *ioc) "TLS termination cancel ioc=%p" > qio_channel_tls_credentials_allow(void *ioc) "TLS credentials allow ioc=%p" > qio_channel_tls_credentials_deny(void *ioc) "TLS credentials deny ioc=%p"
