tracing/fprobe: Remove fprobe from hash in failure path

When register_fprobe_ips() fails, it tries to remove a list of
fprobe_hash_node from fprobe_ip_table, but it missed to remove
fprobe itself from fprobe_table. Moreover, when removing
the fprobe_hash_node which is added to rhltable once, it must
use kfree_rcu() after removing from rhltable.

To fix these issues, this reuses unregister_fprobe() internal
code to rollback the half-way registered fprobe.

Link: https://lore.kernel.org/all/177669366417.132053.17874946321744910456.stgit@mhiramat.tok.corp.google.com/
Fixes: 4346ba160409 ("fprobe: Rewrite fprobe on function-graph tracer")
Cc: stable@vger.kernel.org
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

diff --git a/kernel/trace/fprobe.c b/kernel/trace/fprobe.c
index a2b659006e0eb25423e33831d687a51c9ef548da..621477ad0947935e369605a90708eae2a805a67f 100644 (file)
--- a/kernel/trace/fprobe.c
+++ b/kernel/trace/fprobe.c
@@ -79,20 +79,27 @@ static const struct rhashtable_params fprobe_rht_params = {
 };
 
 /* Node insertion and deletion requires the fprobe_mutex */
-static int insert_fprobe_node(struct fprobe_hlist_node *node)
+static int insert_fprobe_node(struct fprobe_hlist_node *node, struct fprobe *fp)
 {
+       int ret;
+
        lockdep_assert_held(&fprobe_mutex);
 
-       return rhltable_insert(&fprobe_ip_table, &node->hlist, fprobe_rht_params);
+       ret = rhltable_insert(&fprobe_ip_table, &node->hlist, fprobe_rht_params);
+       /* Set the fprobe pointer if insertion was successful. */
+       if (!ret)
+               WRITE_ONCE(node->fp, fp);
+       return ret;
 }
 
 /* Return true if there are synonims */
 static bool delete_fprobe_node(struct fprobe_hlist_node *node)
 {
-       lockdep_assert_held(&fprobe_mutex);
        bool ret;
 
-       /* Avoid double deleting */
+       lockdep_assert_held(&fprobe_mutex);
+
+       /* Avoid double deleting and non-inserted nodes */
        if (READ_ONCE(node->fp) != NULL) {
                WRITE_ONCE(node->fp, NULL);
                rhltable_remove(&fprobe_ip_table, &node->hlist,
@@ -756,7 +763,6 @@ static int fprobe_init(struct fprobe *fp, unsigned long *addrs, int num)
        fp->hlist_array = hlist_array;
        hlist_array->fp = fp;
        for (i = 0; i < num; i++) {
-               hlist_array->array[i].fp = fp;
                addr = ftrace_location(addrs[i]);
                if (!addr) {
                        fprobe_fail_cleanup(fp);
@@ -820,6 +826,8 @@ int register_fprobe(struct fprobe *fp, const char *filter, const char *notfilter
 }
 EXPORT_SYMBOL_GPL(register_fprobe);
 
+static int unregister_fprobe_nolock(struct fprobe *fp);
+
 /**
  * register_fprobe_ips() - Register fprobe to ftrace by address.
  * @fp: A fprobe data structure to be registered.
@@ -846,28 +854,25 @@ int register_fprobe_ips(struct fprobe *fp, unsigned long *addrs, int num)
        if (ret)
                return ret;
 
-       hlist_array = fp->hlist_array;
        if (fprobe_is_ftrace(fp))
                ret = fprobe_ftrace_add_ips(addrs, num);
        else
                ret = fprobe_graph_add_ips(addrs, num);
-
-       if (!ret) {
-               add_fprobe_hash(fp);
-               for (i = 0; i < hlist_array->size; i++) {
-                       ret = insert_fprobe_node(&hlist_array->array[i]);
-                       if (ret)
-                               break;
-               }
-               /* fallback on insert error */
-               if (ret) {
-                       for (i--; i >= 0; i--)
-                               delete_fprobe_node(&hlist_array->array[i]);
-               }
+       if (ret) {
+               fprobe_fail_cleanup(fp);
+               return ret;
        }
 
-       if (ret)
-               fprobe_fail_cleanup(fp);
+       hlist_array = fp->hlist_array;
+       ret = add_fprobe_hash(fp);
+       for (i = 0; i < hlist_array->size && !ret; i++)
+               ret = insert_fprobe_node(&hlist_array->array[i], fp);
+
+       if (ret) {
+               unregister_fprobe_nolock(fp);
+               /* In error case, wait for clean up safely. */
+               synchronize_rcu();
+       }
 
        return ret;
 }
@@ -911,27 +916,12 @@ bool fprobe_is_registered(struct fprobe *fp)
        return true;
 }
 
-/**
- * unregister_fprobe() - Unregister fprobe.
- * @fp: A fprobe data structure to be unregistered.
- *
- * Unregister fprobe (and remove ftrace hooks from the function entries).
- *
- * Return 0 if @fp is unregistered successfully, -errno if not.
- */
-int unregister_fprobe(struct fprobe *fp)
+static int unregister_fprobe_nolock(struct fprobe *fp)
 {
-       struct fprobe_hlist *hlist_array;
+       struct fprobe_hlist *hlist_array = fp->hlist_array;
        unsigned long *addrs = NULL;
-       int ret = 0, i, count;
+       int i, count;
 
-       mutex_lock(&fprobe_mutex);
-       if (!fp || !fprobe_registered(fp)) {
-               ret = -EINVAL;
-               goto out;
-       }
-
-       hlist_array = fp->hlist_array;
        addrs = kcalloc(hlist_array->size, sizeof(unsigned long), GFP_KERNEL);
        /*
         * This will remove fprobe_hash_node from the hash table even if
@@ -957,12 +947,26 @@ int unregister_fprobe(struct fprobe *fp)
 
        kfree_rcu(hlist_array, rcu);
        fp->hlist_array = NULL;
+       kfree(addrs);
 
-out:
-       mutex_unlock(&fprobe_mutex);
+       return 0;
+}
 
-       kfree(addrs);
-       return ret;
+/**
+ * unregister_fprobe() - Unregister fprobe.
+ * @fp: A fprobe data structure to be unregistered.
+ *
+ * Unregister fprobe (and remove ftrace hooks from the function entries).
+ *
+ * Return 0 if @fp is unregistered successfully, -errno if not.
+ */
+int unregister_fprobe(struct fprobe *fp)
+{
+       guard(mutex)(&fprobe_mutex);
+       if (!fp || !fprobe_registered(fp))
+               return -EINVAL;
+
+       return unregister_fprobe_nolock(fp);
 }
 EXPORT_SYMBOL_GPL(unregister_fprobe);