On Thu, 20 Feb 2025 15:20:10 -0500 Steven Rostedt <[email protected]> wrote:
> From: Steven Rostedt <[email protected]> > > 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) <[email protected]> Thanks, > Cc: [email protected] > Fixes: 5fccc7552ccbc ("ftrace: Add subops logic to allow one ops to manage > many") > Signed-off-by: Steven Rostedt (Google) <[email protected]> > --- > Changes since v2: https://lore.kernel.org/[email protected] > > - 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 > > -- Masami Hiramatsu (Google) <[email protected]>
