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>

Masami Hiramatsu (Google) 1 month ago 0ac0058a aa72812b

+65 -17

1 changed file

expand all

kernel

trace

fprobe.c

+65 -17

kernel/trace/fprobe.c

··· 92 92 return ret; 93 93 } 94 94 95 - /* Return true if there are synonims */ 96 - static bool delete_fprobe_node(struct fprobe_hlist_node *node) 95 + static void delete_fprobe_node(struct fprobe_hlist_node *node) 97 96 { 98 - bool ret; 99 - 100 97 lockdep_assert_held(&fprobe_mutex); 101 98 102 99 /* Avoid double deleting and non-inserted nodes */ ··· 102 105 rhltable_remove(&fprobe_ip_table, &node->hlist, 103 106 fprobe_rht_params); 104 107 } 105 - 106 - rcu_read_lock(); 107 - ret = !!rhltable_lookup(&fprobe_ip_table, &node->addr, 108 - fprobe_rht_params); 109 - rcu_read_unlock(); 110 - 111 - return ret; 112 108 } 113 109 114 110 /* Check existence of the fprobe */ ··· 333 343 return !fp->exit_handler; 334 344 } 335 345 346 + static bool fprobe_exists_on_hash(unsigned long ip, bool ftrace) 347 + { 348 + struct rhlist_head *head, *pos; 349 + struct fprobe_hlist_node *node; 350 + struct fprobe *fp; 351 + 352 + guard(rcu)(); 353 + head = rhltable_lookup(&fprobe_ip_table, &ip, 354 + fprobe_rht_params); 355 + if (!head) 356 + return false; 357 + /* We have to check the same type on the list. */ 358 + rhl_for_each_entry_rcu(node, pos, head, hlist) { 359 + if (node->addr != ip) 360 + break; 361 + fp = READ_ONCE(node->fp); 362 + if (likely(fp)) { 363 + if ((!ftrace && fp->exit_handler) || 364 + (ftrace && !fp->exit_handler)) 365 + return true; 366 + } 367 + } 368 + 369 + return false; 370 + } 371 + 336 372 #ifdef CONFIG_MODULES 337 373 static void fprobe_remove_ips(unsigned long *ips, unsigned int cnt) 338 374 { ··· 378 362 379 363 static bool fprobe_is_ftrace(struct fprobe *fp) 380 364 { 365 + return false; 366 + } 367 + 368 + static bool fprobe_exists_on_hash(unsigned long ip, bool ftrace __maybe_unused) 369 + { 370 + struct rhlist_head *head, *pos; 371 + struct fprobe_hlist_node *node; 372 + struct fprobe *fp; 373 + 374 + guard(rcu)(); 375 + head = rhltable_lookup(&fprobe_ip_table, &ip, 376 + fprobe_rht_params); 377 + if (!head) 378 + return false; 379 + /* We only need to check fp is there. */ 380 + rhl_for_each_entry_rcu(node, pos, head, hlist) { 381 + if (node->addr != ip) 382 + break; 383 + fp = READ_ONCE(node->fp); 384 + if (likely(fp)) 385 + return true; 386 + } 387 + 381 388 return false; 382 389 } 383 390 ··· 590 551 static int fprobe_remove_node_in_module(struct module *mod, struct fprobe_hlist_node *node, 591 552 struct fprobe_addr_list *alist) 592 553 { 554 + lockdep_assert_in_rcu_read_lock(); 555 + 593 556 if (!within_module(node->addr, mod)) 594 557 return 0; 595 558 596 - if (delete_fprobe_node(node)) 597 - return 0; 559 + delete_fprobe_node(node); 598 560 /* If no address list is available, we can't track this address. */ 599 561 if (!alist->addrs) 600 562 return 0; 563 + /* 564 + * Don't care the type here, because all fprobes on the same 565 + * address must be removed eventually. 566 + */ 567 + if (!rhltable_lookup(&fprobe_ip_table, &node->addr, fprobe_rht_params)) { 568 + alist->addrs[alist->index++] = node->addr; 569 + if (alist->index == alist->size) 570 + return -ENOSPC; 571 + } 601 572 602 - alist->addrs[alist->index++] = node->addr; 603 - if (alist->index == alist->size) 604 - return -ENOSPC; 605 573 return 0; 606 574 } 607 575 ··· 979 933 /* Remove non-synonim ips from table and hash */ 980 934 count = 0; 981 935 for (i = 0; i < hlist_array->size; i++) { 982 - if (!delete_fprobe_node(&hlist_array->array[i]) && addrs) 936 + delete_fprobe_node(&hlist_array->array[i]); 937 + if (addrs && !fprobe_exists_on_hash(hlist_array->array[i].addr, 938 + fprobe_is_ftrace(fp))) 983 939 addrs[count++] = hlist_array->array[i].addr; 984 940 } 985 941 del_fprobe_hash(fp);

Configure Feed

Configure Feed