tracing/fprobe: Check the same type fprobe on table as the unregistered one

Commit 2c67dc457bc6 ("tracing: fprobe: optimization for entry only case")
introduced a different ftrace_ops for entry-only fprobes.

However, when unregistering an fprobe, the kernel only checks if another
fprobe exists at the same address, without checking which type of fprobe
it is.
If different fprobes are registered at the same address, the same address
will be registered in both fgraph_ops and ftrace_ops, but only one of
them will be deleted when unregistering. (the one removed first will not
be deleted from the ops).

This results in junk entries remaining in either fgraph_ops or ftrace_ops.
For example:
 =======
 cd /sys/kernel/tracing

 # 'Add entry and exit events on the same place'
 echo 'f:event1 vfs_read' >> dynamic_events
 echo 'f:event2 vfs_read%return' >> dynamic_events

 # 'Enable both of them'
 echo 1 > events/fprobes/enable
 cat enabled_functions
vfs_read (2)            ->arch_ftrace_ops_list_func+0x0/0x210

 # 'Disable and remove exit event'
 echo 0 > events/fprobes/event2/enable
 echo -:event2 >> dynamic_events

 # 'Disable and remove all events'
 echo 0 > events/fprobes/enable
 echo > dynamic_events

 # 'Add another event'
 echo 'f:event3 vfs_open%return' > dynamic_events
 cat dynamic_events
f:fprobes/event3 vfs_open%return

 echo 1 > events/fprobes/enable
 cat enabled_functions
vfs_open (1)            tramp: 0xffffffffa0001000 (ftrace_graph_func+0x0/0x60) ->ftrace_graph_func+0x0/0x60    subops: {ent:fprobe_fgraph_entry+0x0/0x620 ret:fprobe_return+0x0/0x150}
vfs_read (1)            tramp: 0xffffffffa0001000 (ftrace_graph_func+0x0/0x60) ->ftrace_graph_func+0x0/0x60    subops: {ent:fprobe_fgraph_entry+0x0/0x620 ret:fprobe_return+0x0/0x150}
 =======

As you can see, an entry for the vfs_read remains.

To fix this issue, when unregistering, the kernel should also check if
there is the same type of fprobes still exist at the same address, and
if not, delete its entry from either fgraph_ops or ftrace_ops.

Link: https://lore.kernel.org/all/177669367993.132053.10553046138528674802.stgit@mhiramat.tok.corp.google.com/
Fixes: 2c67dc457bc6 ("tracing: fprobe: optimization for entry only case")
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 9b913facfd3637c4ec826b71febafaf583dd7eb5..d0a68a2c5eaf93d720e429b5beac7d852a7eaf75 100644 (file)
--- a/kernel/trace/fprobe.c
+++ b/kernel/trace/fprobe.c
@@ -92,11 +92,8 @@ static int insert_fprobe_node(struct fprobe_hlist_node *node, struct fprobe *fp)
        return ret;
 }
 
-/* Return true if there are synonims */
-static bool delete_fprobe_node(struct fprobe_hlist_node *node)
+static void delete_fprobe_node(struct fprobe_hlist_node *node)
 {
-       bool ret;
-
        lockdep_assert_held(&fprobe_mutex);
 
        /* Avoid double deleting and non-inserted nodes */
@@ -105,13 +102,6 @@ static bool delete_fprobe_node(struct fprobe_hlist_node *node)
                rhltable_remove(&fprobe_ip_table, &node->hlist,
                                fprobe_rht_params);
        }
-
-       rcu_read_lock();
-       ret = !!rhltable_lookup(&fprobe_ip_table, &node->addr,
-                               fprobe_rht_params);
-       rcu_read_unlock();
-
-       return ret;
 }
 
 /* Check existence of the fprobe */
@@ -343,6 +333,32 @@ static bool fprobe_is_ftrace(struct fprobe *fp)
        return !fp->exit_handler;
 }
 
+static bool fprobe_exists_on_hash(unsigned long ip, bool ftrace)
+{
+       struct rhlist_head *head, *pos;
+       struct fprobe_hlist_node *node;
+       struct fprobe *fp;
+
+       guard(rcu)();
+       head = rhltable_lookup(&fprobe_ip_table, &ip,
+                               fprobe_rht_params);
+       if (!head)
+               return false;
+       /* We have to check the same type on the list. */
+       rhl_for_each_entry_rcu(node, pos, head, hlist) {
+               if (node->addr != ip)
+                       break;
+               fp = READ_ONCE(node->fp);
+               if (likely(fp)) {
+                       if ((!ftrace && fp->exit_handler) ||
+                           (ftrace && !fp->exit_handler))
+                               return true;
+               }
+       }
+
+       return false;
+}
+
 #ifdef CONFIG_MODULES
 static void fprobe_remove_ips(unsigned long *ips, unsigned int cnt)
 {
@@ -365,6 +381,29 @@ static bool fprobe_is_ftrace(struct fprobe *fp)
        return false;
 }
 
+static bool fprobe_exists_on_hash(unsigned long ip, bool ftrace __maybe_unused)
+{
+       struct rhlist_head *head, *pos;
+       struct fprobe_hlist_node *node;
+       struct fprobe *fp;
+
+       guard(rcu)();
+       head = rhltable_lookup(&fprobe_ip_table, &ip,
+                               fprobe_rht_params);
+       if (!head)
+               return false;
+       /* We only need to check fp is there. */
+       rhl_for_each_entry_rcu(node, pos, head, hlist) {
+               if (node->addr != ip)
+                       break;
+               fp = READ_ONCE(node->fp);
+               if (likely(fp))
+                       return true;
+       }
+
+       return false;
+}
+
 #ifdef CONFIG_MODULES
 static void fprobe_remove_ips(unsigned long *ips, unsigned int cnt)
 {
@@ -551,18 +590,25 @@ struct fprobe_addr_list {
 static int fprobe_remove_node_in_module(struct module *mod, struct fprobe_hlist_node *node,
                                         struct fprobe_addr_list *alist)
 {
+       lockdep_assert_in_rcu_read_lock();
+
        if (!within_module(node->addr, mod))
                return 0;
 
-       if (delete_fprobe_node(node))
-               return 0;
+       delete_fprobe_node(node);
        /* If no address list is available, we can't track this address. */
        if (!alist->addrs)
                return 0;
+       /*
+        * Don't care the type here, because all fprobes on the same
+        * address must be removed eventually.
+        */
+       if (!rhltable_lookup(&fprobe_ip_table, &node->addr, fprobe_rht_params)) {
+               alist->addrs[alist->index++] = node->addr;
+               if (alist->index == alist->size)
+                       return -ENOSPC;
+       }
 
-       alist->addrs[alist->index++] = node->addr;
-       if (alist->index == alist->size)
-               return -ENOSPC;
        return 0;
 }
 
@@ -933,7 +979,9 @@ static int unregister_fprobe_nolock(struct fprobe *fp)
        /* Remove non-synonim ips from table and hash */
        count = 0;
        for (i = 0; i < hlist_array->size; i++) {
-               if (!delete_fprobe_node(&hlist_array->array[i]) && addrs)
+               delete_fprobe_node(&hlist_array->array[i]);
+               if (addrs && !fprobe_exists_on_hash(hlist_array->array[i].addr,
+                                                   fprobe_is_ftrace(fp)))
                        addrs[count++] = hlist_array->array[i].addr;
        }
        del_fprobe_hash(fp);