| Message ID | 20250220202055.060300046@goodmis.org (mailing list archive) |
|---|---|
| State | Queued |
| Headers | show |
| Series | ftrace: Fix fprobe with function graph accounting | expand |
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 > >
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;