From fb328f4ff487ce65237646a58f62c0c345ad5f16 Mon Sep 17 00:00:00 2001 From: Kay Sievers Date: Wed, 25 Dec 2013 16:33:21 +0100 Subject: [PATCH] handle timeouts only handled for expected replies, not for queued messages --- bus.h | 1 - connection.c | 90 ++++++++++++++++++++--------------------------- kdbus.h | 28 ++++++--------- message.c | 9 +---- test/kdbus-util.c | 15 +++++--- 5 files changed, 61 insertions(+), 82 deletions(-) diff --git a/bus.h b/bus.h index dfeea98..1d58b45 100644 --- a/bus.h +++ b/bus.h @@ -78,6 +78,5 @@ struct kdbus_bus *kdbus_bus_unref(struct kdbus_bus *bus); void kdbus_bus_disconnect(struct kdbus_bus *bus); bool kdbus_bus_uid_is_privileged(const struct kdbus_bus *bus); -void kdbus_bus_scan_timeout_list(struct kdbus_bus *bus); struct kdbus_conn *kdbus_bus_find_conn_by_id(struct kdbus_bus *bus, u64 id); #endif diff --git a/connection.c b/connection.c index 2d13658..08f8905 100644 --- a/connection.c +++ b/connection.c @@ -50,7 +50,6 @@ * @fds: Offset to array where to update the installed fd number * @fds_fp: Array passed files queued up for this message * @fds_count: Number of files - * @deadline_ns: Timeout for this message, used for replies * @src_id: The ID of the sender * @cookie: Message cookie, used for replies * @src_name_id: The sequence number of the name this message is @@ -69,7 +68,6 @@ struct kdbus_conn_queue { struct file **fds_fp; unsigned int fds_count; - u64 deadline_ns; u64 src_id; u64 cookie; u64 dst_name_id; @@ -329,8 +327,7 @@ static void kdbus_conn_queue_cleanup(struct kdbus_conn_queue *queue) /* enqueue a message into the receiver's pool */ static int kdbus_conn_queue_insert(struct kdbus_conn *conn, - struct kdbus_kmsg *kmsg, - u64 deadline_ns) + struct kdbus_kmsg *kmsg) { struct kdbus_conn_queue *queue; u64 msg_size; @@ -352,7 +349,6 @@ static int kdbus_conn_queue_insert(struct kdbus_conn *conn, return -ENOMEM; /* copy message properties we need for the queue management */ - queue->deadline_ns = deadline_ns; queue->src_id = kmsg->msg.src_id; queue->cookie = kmsg->msg.cookie; @@ -507,11 +503,10 @@ exit_unlock: static void kdbus_conn_scan_timeout(struct kdbus_conn *conn) { - struct kdbus_conn_queue *queue, *queue_tmp; struct kdbus_conn_reply_entry *reply, *reply_tmp; LIST_HEAD(notify_list); LIST_HEAD(reply_list); - u64 deadline = -1; + u64 deadline = ~0ULL; struct timespec ts; u64 now; @@ -519,34 +514,34 @@ static void kdbus_conn_scan_timeout(struct kdbus_conn *conn) now = timespec_to_ns(&ts); mutex_lock(&conn->lock); - list_for_each_entry_safe(queue, queue_tmp, &conn->msg_list, entry) { - if (queue->deadline_ns == 0) - continue; - - if (queue->deadline_ns <= now) { - kdbus_pool_free_range(conn->pool, queue->off); - list_del(&queue->entry); - kdbus_conn_queue_cleanup(queue); - continue; - } + list_for_each_entry_safe(reply, reply_tmp, &conn->reply_list, entry) { + if (reply->deadline_ns <= now) { - if (queue->deadline_ns < deadline) - deadline = queue->deadline_ns; - } + /* + * Move to temporary cleanup list; we cannot unref and + * possibly cleanup a connection that is holding a ref + * back to us, while we are locking ourselves. + */ + list_move_tail(&reply->entry, &reply_list); - list_for_each_entry_safe(reply, reply_tmp, &conn->reply_list, entry) { - if (reply->deadline_ns > now) - continue; + /* + * A zero deadline means the connection died, was + * cleaned up already and the notify sent. + */ + if (reply->deadline_ns == 0) + continue; - /* - * The connection died and cleared the timeout; the - * was already sent; just clean up. - */ - if (reply->deadline_ns > 0) kdbus_notify_reply_timeout(reply->conn->id, - reply->cookie, ¬ify_list); + reply->cookie, + ¬ify_list); + continue; + } - list_move_tail(&reply->entry, &reply_list); + /* remember next timeout */ + if (reply->deadline_ns < deadline) { + deadline = reply->deadline_ns; + continue; + } } mutex_unlock(&conn->lock); @@ -555,8 +550,10 @@ static void kdbus_conn_scan_timeout(struct kdbus_conn *conn) list_for_each_entry_safe(reply, reply_tmp, &reply_list, entry) kdbus_conn_reply_entry_free(reply); - if (deadline != -1) { + /* rearm timer with next timeout */ + if (deadline != (~0ULL)) { u64 usecs = deadline - now; + do_div(usecs, 1000ULL); mod_timer(&conn->timer, jiffies + usecs_to_jiffies(usecs)); } @@ -670,7 +667,6 @@ int kdbus_conn_kmsg_send(struct kdbus_ep *ep, const struct kdbus_msg *msg = &kmsg->msg; struct kdbus_conn *conn_dst = NULL; struct kdbus_conn *c; - u64 deadline_ns = 0; int ret; /* non-kernel senders append credentials/metadata */ @@ -710,7 +706,7 @@ int kdbus_conn_kmsg_send(struct kdbus_ep *ep, kdbus_meta_append(kmsg->meta, conn_src, conn_dst->attach_flags); - kdbus_conn_queue_insert(conn_dst, kmsg, 0); + kdbus_conn_queue_insert(conn_dst, kmsg); } mutex_unlock(&ep->bus->lock); @@ -757,22 +753,10 @@ int kdbus_conn_kmsg_send(struct kdbus_ep *ep, } } - /* - * If the message has a timeout value set, calculate the deadline here, - * based on the current time. Note that this only takes care for the - * timeout of the actual message, and has nothing to do with a potential - * reply that may be expected. - */ - if (msg->src_id != KDBUS_SRC_ID_KERNEL && msg->timeout_ns > 0) { - struct timespec ts; - - ktime_get_ts(&ts); - deadline_ns = timespec_to_ns(&ts) + msg->timeout_ns; - } - /* If the message expects a reply, add a kdbus_conn_reply_entry */ if (conn_src && (msg->flags & KDBUS_MSG_FLAGS_EXPECT_REPLY)) { struct kdbus_conn_reply_entry *reply; + struct timespec ts; reply = kzalloc(sizeof(*reply), GFP_KERNEL); if (!reply) { @@ -781,14 +765,19 @@ int kdbus_conn_kmsg_send(struct kdbus_ep *ep, } INIT_LIST_HEAD(&reply->entry); - reply->deadline_ns = deadline_ns; reply->conn = kdbus_conn_ref(conn_dst); atomic_inc(&reply->conn->reply_count); reply->cookie = msg->cookie; + /* calculate the deadline based on the current time */ + ktime_get_ts(&ts); + reply->deadline_ns = timespec_to_ns(&ts) + msg->timeout_ns; + mutex_lock(&conn_src->lock); list_add(&reply->entry, &conn_src->reply_list); mutex_unlock(&conn_src->lock); + + kdbus_conn_timeout_schedule_scan(conn_dst); } if (conn_src) { @@ -804,16 +793,13 @@ int kdbus_conn_kmsg_send(struct kdbus_ep *ep, */ mutex_lock(&ep->bus->lock); list_for_each_entry(c, &ep->bus->monitors_list, monitor_entry) - kdbus_conn_queue_insert(c, kmsg, 0); + kdbus_conn_queue_insert(c, kmsg); mutex_unlock(&ep->bus->lock); - ret = kdbus_conn_queue_insert(conn_dst, kmsg, deadline_ns); + ret = kdbus_conn_queue_insert(conn_dst, kmsg); if (ret < 0) goto exit_unref; - if (deadline_ns > 0) - kdbus_conn_timeout_schedule_scan(conn_dst); - exit_unref: /* conn_dst got an extra ref from kdbus_conn_get_conn_dst */ kdbus_conn_unref(conn_dst); diff --git a/kdbus.h b/kdbus.h index 71bf86d..71cfe06 100644 --- a/kdbus.h +++ b/kdbus.h @@ -345,21 +345,13 @@ enum kdbus_payload_type { * @payload_type: Payload type (KDBUS_PAYLOAD_*) * @cookie: Userspace-supplied cookie, for the connection * to identify its messages - * @cookie_reply: A reply to the message with the same cookie. The - * reply itself has its own cookie, @cookie_reply - * corresponds to the cookie of the request message - * @timeout_ns: For non-kernel-generated messages, this denotes the - * message timeout in nanoseconds. A message has to be - * received with KDBUS_CMD_MSG_RECV by the destination - * connection within this time frame. For messages that - * have KDBUS_MSG_FLAGS_EXPECT_REPLY set in @flags, - * this value also denotes the timeout for the reply to - * this message. If there is no reply, or the message is - * not received in time by the other side, a - * kernel-generated message with an attached - * KDBUS_ITEM_REPLY_TIMEOUT item is sent to @src_id. - * A 0-value is only valid if KDBUS_MSG_FLAGS_EXPECT_REPLY - * is unset in @flags. + * @cookie_reply: A reply to the requesting message with the same + * cookie. The requesting connection can match its + * request and the reply with this value + * @timeout_ns: The time to wait for a message reply from the peer. + * If there is no reply, a kernel-generated message + * with an attached KDBUS_ITEM_REPLY_TIMEOUT item + * is sent to @src_id. * @items: A list of kdbus_items containing the message payload */ struct kdbus_msg { @@ -369,8 +361,10 @@ struct kdbus_msg { __u64 src_id; __u64 payload_type; __u64 cookie; - __u64 cookie_reply; - __u64 timeout_ns; + union { + __u64 cookie_reply; + __u64 timeout_ns; + }; struct kdbus_item items[0]; } __attribute__((aligned(8))); diff --git a/message.c b/message.c index f6d3bc6..2bbcc40 100644 --- a/message.c +++ b/message.c @@ -219,7 +219,7 @@ static int kdbus_msg_scan_items(struct kdbus_conn *conn, return -EBADMSG; /* timeouts are not allowed for broadcasts */ - if (msg->timeout_ns) + if (msg->timeout_ns > 0) return -ENOTUNIQ; } @@ -276,13 +276,6 @@ int kdbus_kmsg_new_from_user(struct kdbus_conn *conn, goto exit_free; } - /* replies cannot request a reply themselves */ - if (m->msg.flags & KDBUS_MSG_FLAGS_EXPECT_REPLY && - m->msg.cookie_reply > 0) { - ret = -EINVAL; - goto exit_free; - } - ret = kdbus_msg_scan_items(conn, m); if (ret < 0) goto exit_free; diff --git a/test/kdbus-util.c b/test/kdbus-util.c index 6e1b565..942ee8b 100644 --- a/test/kdbus-util.c +++ b/test/kdbus-util.c @@ -212,12 +212,19 @@ void msg_dump(const struct conn *conn, const struct kdbus_msg *msg) const struct kdbus_item *item = msg->items; char buf_src[32]; char buf_dst[32]; + uint64_t timeout = 0; + uint64_t cookie_reply = 0; - printf("MESSAGE: %s (%llu bytes) flags=0x%08llx, %s → %s, cookie=%llu, timeout=%llu\n", - enum_PAYLOAD(msg->payload_type), (unsigned long long) msg->size, - (unsigned long long) msg->flags, + if (msg->flags & KDBUS_MSG_FLAGS_EXPECT_REPLY) + timeout = msg->timeout_ns; + else + cookie_reply = msg->cookie_reply; + + printf("MESSAGE: %s (%llu bytes) flags=0x%08llx, %s → %s, cookie=%llu, timeout=%llu cookie_reply=%llu\n", + enum_PAYLOAD(msg->payload_type), (unsigned long long)msg->size, + (unsigned long long)msg->flags, msg_id(msg->src_id, buf_src), msg_id(msg->dst_id, buf_dst), - (unsigned long long) msg->cookie, (unsigned long long) msg->timeout_ns); + (unsigned long long)msg->cookie, (unsigned long long)timeout, (unsigned long long)cookie_reply); KDBUS_ITEM_FOREACH(item, msg, items) { if (item->size <= KDBUS_ITEM_HEADER_SIZE) { -- 2.34.1