On Thu, 20 Feb 2025 15:20:10 -0500 Steven Rostedt rostedt@goodmis.org wrote:

From: Steven Rostedt rostedt@goodmis.org

Function graph uses a subops and manager ops mechanism to attach to ftrace. The manager ops connects to ftrace and the functions it connects to is defined by a list of subops that it manages.

The function hash that defines what the above ops attaches to limits the functions to attach if the hash has any content. If the hash is empty, it means to trace all functions.

The creation of the manager ops hash is done by iterating over all the subops hashes. If any of the subops hashes is empty, it means that the manager ops hash must trace all functions as well.

The issue is in the creation of the manager ops. When a second subops is attached, a new hash is created by starting it as NULL and adding the subops one at a time. But the NULL ops is mistaken as an empty hash, and once an empty hash is found, it stops the loop of subops and just enables all functions.

# echo "f:myevent1 kernel_clone" >> /sys/kernel/tracing/dynamic_events # cat /sys/kernel/tracing/enabled_functions kernel_clone (1) tramp: 0xffffffffc0309000 (ftrace_graph_func+0x0/0x60) ->ftrace_graph_func+0x0/0x60

# echo "f:myevent2 schedule_timeout" >> /sys/kernel/tracing/dynamic_events # cat /sys/kernel/tracing/enabled_functions trace_initcall_start_cb (1) tramp: 0xffffffffc0309000 (ftrace_graph_func+0x0/0x60) ->ftrace_graph_func+0x0/0x60 run_init_process (1) tramp: 0xffffffffc0309000 (ftrace_graph_func+0x0/0x60) ->ftrace_graph_func+0x0/0x60 try_to_run_init_process (1) tramp: 0xffffffffc0309000 (ftrace_graph_func+0x0/0x60) ->ftrace_graph_func+0x0/0x60 x86_pmu_show_pmu_cap (1) tramp: 0xffffffffc0309000 (ftrace_graph_func+0x0/0x60) ->ftrace_graph_func+0x0/0x60 cleanup_rapl_pmus (1) tramp: 0xffffffffc0309000 (ftrace_graph_func+0x0/0x60) ->ftrace_graph_func+0x0/0x60 uncore_free_pcibus_map (1) tramp: 0xffffffffc0309000 (ftrace_graph_func+0x0/0x60) ->ftrace_graph_func+0x0/0x60 uncore_types_exit (1) tramp: 0xffffffffc0309000 (ftrace_graph_func+0x0/0x60) ->ftrace_graph_func+0x0/0x60 uncore_pci_exit.part.0 (1) tramp: 0xffffffffc0309000 (ftrace_graph_func+0x0/0x60) ->ftrace_graph_func+0x0/0x60 kvm_shutdown (1) tramp: 0xffffffffc0309000 (ftrace_graph_func+0x0/0x60) ->ftrace_graph_func+0x0/0x60 vmx_dump_msrs (1) tramp: 0xffffffffc0309000 (ftrace_graph_func+0x0/0x60) ->ftrace_graph_func+0x0/0x60 vmx_cleanup_l1d_flush (1) tramp: 0xffffffffc0309000 (ftrace_graph_func+0x0/0x60) ->ftrace_graph_func+0x0/0x60 [..]

Fix this by initializing the new hash to NULL and if the hash is NULL do not treat it as an empty hash but instead allocate by copying the content of the first sub ops. Then on subsequent iterations, the new hash will not be NULL, but the content of the previous subops. If that first subops attached to all functions, then new hash may assume that the manager ops also needs to attach to all functions.

Looks good to me.

Reviewed-by: Masami Hiramatsu (Google) mhiramat@kernel.org

Thanks,

Cc: stable@vger.kernel.org Fixes: 5fccc7552ccbc ("ftrace: Add subops logic to allow one ops to manage many") Signed-off-by: Steven Rostedt (Google) rostedt@goodmis.org

---

Changes since v2: https://lore.kernel.org/20250219220510.888959028@goodmis.org

• Have append_hashes() return EMPTY_HASH and not NULL if the resulting new hash is empty.

kernel/trace/ftrace.c | 33 ++++++++++++++++++++++----------- 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c index 728ecda6e8d4..bec54dc27204 100644 --- a/kernel/trace/ftrace.c +++ b/kernel/trace/ftrace.c @@ -3220,15 +3220,22 @@ static struct ftrace_hash *copy_hash(struct ftrace_hash *src)

• The filter_hash updates uses just the append_hash() function
• and the notrace_hash does not.

*/ -static int append_hash(struct ftrace_hash **hash, struct ftrace_hash *new_hash) +static int append_hash(struct ftrace_hash **hash, struct ftrace_hash *new_hash,

•        int size_bits)
  

{ struct ftrace_func_entry *entry; int size; int i;

• /* An empty hash does everything */
• if (ftrace_hash_empty(*hash))

• return 0;
  

• if (*hash) {

• /* An empty hash does everything */
  

• if (ftrace_hash_empty(*hash))
  

• 	return 0;
  

• } else {

• *hash = alloc_ftrace_hash(size_bits);
  

• if (!*hash)
  

• 	return -ENOMEM;
  

• }

/* If new_hash has everything make hash have everything */ if (ftrace_hash_empty(new_hash)) { @@ -3292,16 +3299,18 @@ static int intersect_hash(struct ftrace_hash **hash, struct ftrace_hash *new_has /* Return a new hash that has a union of all @ops->filter_hash entries */ static struct ftrace_hash *append_hashes(struct ftrace_ops *ops) {

• struct ftrace_hash *new_hash;

• struct ftrace_hash *new_hash = NULL; struct ftrace_ops *subops;
• int size_bits; int ret;

• new_hash = alloc_ftrace_hash(ops->func_hash->filter_hash->size_bits);
• if (!new_hash)

• return NULL;
  

• if (ops->func_hash->filter_hash)

• size_bits = ops->func_hash->filter_hash->size_bits;
  

• else

• size_bits = FTRACE_HASH_DEFAULT_BITS;
  

list_for_each_entry(subops, &ops->subop_list, list) {

• ret = append_hash(&new_hash, subops->func_hash->filter_hash);
  

• ret = append_hash(&new_hash, subops->func_hash->filter_hash, size_bits);
  

  if (ret < 0) { free_ftrace_hash(new_hash); return NULL;

@@ -3310,7 +3319,8 @@ static struct ftrace_hash *append_hashes(struct ftrace_ops *ops) if (ftrace_hash_empty(new_hash)) break; }

• return new_hash;

• /* Can't return NULL as that means this failed */
• return new_hash ? : EMPTY_HASH;

} /* Make @ops trace evenything except what all its subops do not trace */ @@ -3505,7 +3515,8 @@ int ftrace_startup_subops(struct ftrace_ops *ops, struct ftrace_ops *subops, int filter_hash = alloc_and_copy_ftrace_hash(size_bits, ops->func_hash->filter_hash); if (!filter_hash) return -ENOMEM;

• ret = append_hash(&filter_hash, subops->func_hash->filter_hash);
  

• ret = append_hash(&filter_hash, subops->func_hash->filter_hash,
  

• 		  size_bits);
  

  if (ret < 0) { free_ftrace_hash(filter_hash); return ret;

-- 2.47.2