From dfdedadc2caf0b0098544069f69446f0201da107 Mon Sep 17 00:00:00 2001 From: David Herrmann Date: Thu, 23 Oct 2014 15:11:43 +0200 Subject: [PATCH] connection: keep SYNC messages alive on EINTR If a SYNC-SEND is interrupted by a signal, there is no way we can restart the syscall. If we returned ERESTARTSYS, we'd queue the message again on restart. This is very irritating, therefore, we never support restarting syscalls. Instead, we return EINPROGRESS if the message was queued but no reply was received, yet. Internally, we turn the 'sync' reply_wait into an 'async' reply. This way, it will be treated the same way as any other asynchronous reply. Signed-off-by: David Herrmann --- connection.c | 61 +++++++++++++++++++++++++++++++++++++----------- test/test-sync.c | 39 +++++++++++++++---------------- 2 files changed, 66 insertions(+), 34 deletions(-) diff --git a/connection.c b/connection.c index 104234a..27cb265 100644 --- a/connection.c +++ b/connection.c @@ -100,12 +100,11 @@ static int kdbus_conn_reply_new(struct kdbus_conn_reply **reply_wait, r->reply_dst = kdbus_conn_ref(reply_dst); r->cookie = msg->cookie; r->name_id = name_entry ? name_entry->name_id : 0; + r->deadline_ns = msg->timeout_ns; if (sync) { r->sync = true; r->waiting = true; - } else { - r->deadline_ns = msg->timeout_ns; } *reply_wait = r; @@ -641,6 +640,7 @@ static int kdbus_conn_wait_reply(struct kdbus_ep *ep, u64 timeout_ns) { struct kdbus_queue_entry *entry; + bool waiting; int r, ret; /* @@ -651,10 +651,34 @@ static int kdbus_conn_wait_reply(struct kdbus_ep *ep, r = wait_event_interruptible_timeout(reply_wait->reply_dst->wait, !reply_wait->waiting || !kdbus_conn_active(conn_src), nsecs_to_jiffies(timeout_ns)); + if (r < 0) { + /* + * We got interrupted by a signal. We have to return to + * user-space, but we cannot support SA_RESTART. If we returned + * ERESTARTSYS, we would queue the message again on restart + * instead of only waiting for a response. + * Therefore, we require callers of SEND to always handle + * EINPROGRESS if they use SYNC_REPLY. This means, the message + * got queued but no response was received, yet. They must use + * normal RECV to retrieve it. + */ + mutex_lock(&conn_src->lock); + waiting = reply_wait->waiting; + if (waiting) { + reply_wait->waiting = false; + reply_wait->sync = false; + schedule_delayed_work(&conn_dst->work, 0); + } + mutex_unlock(&conn_src->lock); + + if (waiting) + return -EINPROGRESS; + + r = 1; + } + if (r == 0) ret = -ETIMEDOUT; - else if (r < 0) - ret = -EINTR; else if (!kdbus_conn_active(conn_src)) ret = -ECONNRESET; else @@ -817,20 +841,29 @@ int kdbus_conn_kmsg_send(struct kdbus_ep *ep, * The connection's queue will never get to see it. */ mutex_lock(&conn_dst->lock); - if (reply_wake->waiting && kdbus_conn_active(conn_dst)) - ret = kdbus_queue_entry_alloc(conn_dst, kmsg, - &reply_wake->queue_entry); - else - ret = -ECONNRESET; - - kdbus_conn_reply_sync(reply_wake, ret); + if (reply_wake->sync) { + if (reply_wake->waiting && kdbus_conn_active(conn_dst)) + ret = kdbus_queue_entry_alloc(conn_dst, kmsg, + &reply_wake->queue_entry); + else + ret = -ECONNRESET; + + kdbus_conn_reply_sync(reply_wake, ret); + kdbus_conn_reply_unref(reply_wake); + } else { + /* Object went into async mode; unref reply_wake and + * fall-through to normal queuing below. */ + kdbus_conn_reply_unref(reply_wake); + reply_wake = NULL; + ret = 0; + } mutex_unlock(&conn_dst->lock); - kdbus_conn_reply_unref(reply_wake); - if (ret < 0) goto exit_unref; - } else { + } + + if (!reply_wake) { /* * Otherwise, put it in the queue and wait for the connection * to dequeue and receive the message. diff --git a/test/test-sync.c b/test/test-sync.c index 9533fd7..8e69cb3 100644 --- a/test/test-sync.c +++ b/test/test-sync.c @@ -71,15 +71,14 @@ static int send_reply(const struct kdbus_conn *conn, } static int interrupt_sync(struct kdbus_conn *conn_src, - struct kdbus_conn *conn_dst, - int sa_flags) + struct kdbus_conn *conn_dst) { pid_t pid; int ret, status; struct kdbus_msg *msg = NULL; struct sigaction sa = { .sa_handler = nop_handler, - .sa_flags = sa_flags, + .sa_flags = SA_NOCLDSTOP|SA_RESTART, }; cookie++; @@ -93,8 +92,20 @@ static int interrupt_sync(struct kdbus_conn *conn_src, ret = kdbus_msg_send(conn_dst, NULL, cookie, KDBUS_MSG_FLAGS_EXPECT_REPLY | KDBUS_MSG_FLAGS_SYNC_REPLY, - 5000000000ULL, 0, conn_src->id); - ASSERT_EXIT(ret == -EINTR); + 100000000ULL, 0, conn_src->id); + ASSERT_EXIT(ret == -EINPROGRESS); + + /* conn_reply is now async, thus we will receive a timeout */ + + ret = kdbus_msg_recv_poll(conn_dst, 100, &msg, NULL); + ASSERT_EXIT(ret >= 0); + + ASSERT_EXIT(msg->size >= sizeof(struct kdbus_msg) + + KDBUS_ITEM_HEADER_SIZE); + ASSERT_EXIT(msg->items[0].size >= KDBUS_ITEM_HEADER_SIZE); + ASSERT_EXIT(msg->items[0].type == KDBUS_ITEM_REPLY_TIMEOUT); + + kdbus_msg_free(msg); _exit(EXIT_SUCCESS); } @@ -113,17 +124,8 @@ static int interrupt_sync(struct kdbus_conn *conn_src, if (WIFSIGNALED(status)) return TEST_ERR; - if (sa_flags & SA_RESTART) { - /* - * Our SYNC logic do not support SA_RESTART flag, so we - * don't receive the same packet again. We fail with - * ETIMEDOUT. - * - * For more information, please check "man 7 signal". - */ - ret = kdbus_msg_recv_poll(conn_src, 100, NULL, NULL); - ASSERT_RETURN(ret == -ETIMEDOUT); - } + ret = kdbus_msg_recv_poll(conn_src, 100, NULL, NULL); + ASSERT_RETURN(ret == -ETIMEDOUT); return (status == EXIT_SUCCESS) ? TEST_OK : TEST_ERR; } @@ -161,10 +163,7 @@ int kdbus_test_sync_reply(struct kdbus_test_env *env) pthread_join(thread, NULL); ASSERT_RETURN(ret == 0); - ret = interrupt_sync(conn_a, conn_b, SA_NOCLDSTOP); - ASSERT_RETURN(ret == 0); - - ret = interrupt_sync(conn_a, conn_b, SA_NOCLDSTOP|SA_RESTART); + ret = interrupt_sync(conn_a, conn_b); ASSERT_RETURN(ret == 0); kdbus_printf("-- closing bus connections\n"); -- 2.34.1