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 <dh.herrmann@gmail.com>

diff --git a/connection.c b/connection.c
index 104234a68122205f303e14827723fadb55bea51f..27cb265a40f9df65aa2492c0c7f0b14d47f13c47 100644 (file)
--- 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 9533fd7e1e42b5982ec153dc3487eae73095fd12..8e69cb356dd9246e524453212b587436a1c983cb 100644 (file)
--- 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");