reply: fix ref-leak

Introduce kdbus_reply_link/unlink() which take care of the list-owned
reference to the reply object. Fix all the callsides to use it and
properly let each caller own its own reference now. We no longer have to
be aware of the queue-state at all times, instead, the list-state owns its
own reference.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>

diff --git a/connection.c b/connection.c
index 4c7d3de020e3f300df751ee5e586b8b1c023d52b..19a41dcc65c83b91df1c3ea57a3eeda719ae0059 100644 (file)
--- a/connection.c
+++ b/connection.c
@@ -165,17 +165,15 @@ int kdbus_cmd_msg_recv(struct kdbus_conn *conn,
                         */
                        mutex_lock(&reply->reply_dst->lock);
                        if (!list_empty(&reply->entry)) {
-                               if (reply->sync) {
+                               kdbus_reply_unlink(reply);
+                               if (reply->sync)
                                        kdbus_sync_reply_wakeup(reply, -EPIPE);
-                               } else {
-                                       list_del_init(&reply->entry);
+                               else
                                        kdbus_notify_reply_dead(conn->ep->bus,
                                                        entry->msg.src_id,
                                                        entry->msg.cookie);
-                               }
                        }
                        mutex_unlock(&reply->reply_dst->lock);
-                       kdbus_reply_unref(reply);
                }
 
                kdbus_notify_flush(conn->ep->bus);
@@ -255,11 +253,9 @@ static int kdbus_conn_check_access(struct kdbus_conn *conn_src,
                mutex_lock(&conn_dst->lock);
                r = kdbus_reply_find(conn_src, conn_dst, msg->cookie_reply);
                if (r) {
-                       list_del_init(&r->entry);
                        if (r->sync)
                                *reply_wake = kdbus_reply_ref(r);
-                       else
-                               kdbus_reply_unref(r);
+                       kdbus_reply_unlink(r);
                }
                mutex_unlock(&conn_dst->lock);
 
@@ -341,8 +337,8 @@ static int kdbus_conn_entry_sync_attach(struct kdbus_conn *conn_dst,
                remote_ret = -EREMOTEIO;
 
        kdbus_sync_reply_wakeup(reply_wake, remote_ret);
+       kdbus_reply_unlink(reply_wake);
        mutex_unlock(&reply_wake->reply_dst->lock);
-       kdbus_reply_unref(reply_wake);
 
        return ret;
 }
@@ -400,7 +396,7 @@ int kdbus_conn_entry_insert(struct kdbus_conn *conn_src,
        entry->reply = kdbus_reply_ref(reply);
 
        if (reply) {
-               list_add(&reply->entry, &conn_src->reply_list);
+               kdbus_reply_link(reply);
                if (!reply->sync)
                        schedule_delayed_work(&conn_src->work, 0);
        }
@@ -541,7 +537,6 @@ static int kdbus_conn_wait_reply(struct kdbus_conn *conn_src,
        }
 
        mutex_lock(&conn_src->lock);
-       list_del_init(&reply_wait->entry);
        reply_wait->waiting = false;
        entry = reply_wait->queue_entry;
        if (entry) {
@@ -553,6 +548,7 @@ static int kdbus_conn_wait_reply(struct kdbus_conn *conn_src,
                kdbus_pool_slice_release(entry->slice);
                kdbus_queue_entry_free(entry);
        }
+       kdbus_reply_unlink(reply_wait);
        mutex_unlock(&conn_src->lock);
 
        return ret;
@@ -708,10 +704,12 @@ int kdbus_cmd_msg_send(struct kdbus_conn *conn_src,
                        reply_wait = kdbus_reply_find(conn_dst, conn_src,
                                                      kmsg->msg.cookie);
                        if (reply_wait) {
-                               if (reply_wait->interrupted)
+                               if (reply_wait->interrupted) {
+                                       kdbus_reply_ref(reply_wait);
                                        reply_wait->interrupted = false;
-                               else
+                               } else {
                                        reply_wait = NULL;
+                               }
                        }
                        mutex_unlock(&conn_src->lock);
 
@@ -827,6 +825,7 @@ wait_sync:
 
 exit_unref:
        kdbus_reply_unref(reply_wait);
+       kdbus_reply_unref(reply_wake);
        kdbus_conn_unref(conn_dst);
 exit_name_unlock:
        kdbus_name_unlock(bus->name_registry, name_entry);
@@ -926,10 +925,8 @@ int kdbus_conn_disconnect(struct kdbus_conn *conn, bool ensure_queue_empty)
                kdbus_queue_entry_free(entry);
        }
 
-       list_for_each_entry_safe(r, r_tmp, &conn->reply_list, entry) {
-               list_del_init(&r->entry);
-               kdbus_reply_unref(r);
-       }
+       list_for_each_entry_safe(r, r_tmp, &conn->reply_list, entry)
+               kdbus_reply_unlink(r);
        mutex_unlock(&conn->lock);
 
        /* lock order: domain -> bus -> ep -> names -> conn */
@@ -940,14 +937,13 @@ int kdbus_conn_disconnect(struct kdbus_conn *conn, bool ensure_queue_empty)
                        if (r->reply_src == conn) {
                                if (r->sync) {
                                        kdbus_sync_reply_wakeup(r, -EPIPE);
+                                       kdbus_reply_unlink(r);
                                        continue;
                                }
 
                                /* send a 'connection dead' notification */
                                kdbus_notify_reply_dead(bus, c->id, r->cookie);
-
-                               list_del_init(&r->entry);
-                               kdbus_reply_unref(r);
+                               kdbus_reply_unlink(r);
                        }
                }
                mutex_unlock(&c->lock);

diff --git a/queue.c b/queue.c
index cfaae5e598af62304d743fbca936299516fccdb1..8f36dddde67600557e4d416176965bb699a14930 100644 (file)
--- a/queue.c
+++ b/queue.c
@@ -35,6 +35,7 @@
 #include "message.h"
 #include "metadata.h"
 #include "queue.h"
+#include "reply.h"
 
 /**
  * kdbus_queue_entry_add() - Add an queue entry to a queue
@@ -483,6 +484,7 @@ void kdbus_queue_entry_free(struct kdbus_queue_entry *entry)
 {
        kdbus_msg_resources_unref(entry->msg_res);
        kdbus_meta_unref(entry->meta);
+       kdbus_reply_unref(entry->reply);
        kfree(entry->msg_extra);
        kfree(entry);
 }

diff --git a/reply.c b/reply.c
index 6186eb5baee47d07a6771d2dc200a5a502b8f24c..d979a0b48fc2d2de18cbcf3d992a059e4c388ade 100644 (file)
--- a/reply.c
+++ b/reply.c
@@ -51,6 +51,7 @@ struct kdbus_reply *kdbus_reply_new(struct kdbus_conn *reply_src,
        }
 
        kref_init(&r->kref);
+       INIT_LIST_HEAD(&r->entry);
        r->reply_src = kdbus_conn_ref(reply_src);
        r->reply_dst = kdbus_conn_ref(reply_dst);
        r->cookie = msg->cookie;
@@ -108,6 +109,31 @@ struct kdbus_reply *kdbus_reply_unref(struct kdbus_reply *r)
        return NULL;
 }
 
+/**
+ * kdbus_reply_link() - Link reply object into target connection
+ * @r:         Reply to link
+ */
+void kdbus_reply_link(struct kdbus_reply *r)
+{
+       if (WARN_ON(!list_empty(&r->entry)))
+               return;
+
+       list_add(&r->entry, &r->reply_dst->reply_list);
+       kdbus_reply_ref(r);
+}
+
+/**
+ * kdbus_reply_unlink() - Unlink reply object from target connection
+ * @r:         Reply to unlink
+ */
+void kdbus_reply_unlink(struct kdbus_reply *r)
+{
+       if (!list_empty(&r->entry)) {
+               list_del_init(&r->entry);
+               kdbus_reply_unref(r);
+       }
+}
+
 /**
  * kdbus_sync_reply_wakeup() - Wake a synchronously blocking reply
  * @reply:     The reply object
@@ -122,7 +148,6 @@ void kdbus_sync_reply_wakeup(struct kdbus_reply *reply, int err)
        if (WARN_ON(!reply->sync))
                return;
 
-       list_del_init(&reply->entry);
        reply->waiting = false;
        reply->err = err;
        wake_up_interruptible(&reply->reply_dst->wait);
@@ -217,8 +242,7 @@ void kdbus_reply_list_scan(struct kdbus_conn *conn)
                        kdbus_notify_reply_timeout(conn->ep->bus, conn->id,
                                                   reply->cookie);
 
-               list_del_init(&reply->entry);
-               kdbus_reply_unref(reply);
+               kdbus_reply_unlink(reply);
        }
 
        /* rearm delayed work with next timeout */

diff --git a/reply.h b/reply.h
index 7d003ef9d0eb00cd88e3ceb886644df6386e49eb..832fccb09ce6b3dad6e46ce811d8d32c2f94fbab 100644 (file)
--- a/reply.h
+++ b/reply.h
@@ -55,6 +55,9 @@ struct kdbus_reply *kdbus_reply_new(struct kdbus_conn *reply_src,
 struct kdbus_reply *kdbus_reply_ref(struct kdbus_reply *r);
 struct kdbus_reply *kdbus_reply_unref(struct kdbus_reply *r);
 
+void kdbus_reply_link(struct kdbus_reply *r);
+void kdbus_reply_unlink(struct kdbus_reply *r);
+
 struct kdbus_reply *kdbus_reply_find(struct kdbus_conn *replying,
                                     struct kdbus_conn *reply_dst,
                                     u64 cookie);