On Sat, 31 Oct 2026 10:52:13 -0700 Weiming Shi <[email protected]> wrote:
> ftrace_shutdown_subops() unlinks a subop before rebuilding the manager > hashes, but leaves it enabled and managed when rebuilding or updating > fails. unregister_ftrace_graph() cannot recover from that partial > teardown, and a later registration can corrupt the empty manager state. > > Always detach and clear the subop once shutdown begins. Keep conservative > manager hashes behind per-subop filtering when an update fails, and disable > direct graph dispatch until those hashes are exact again. If the last > manager cannot be shut down, disable ftrace and normalize its list, flags, > count, and hashes before the caller releases the graph operation. What exactly is the above saying? It looks like pure AI output (non human readable). AKA, AI slop! Is it fixing something when a subop is unregistered but fails? Where exactly is the failure? What failed? Have an example of what happened? Direct calls is mentioned once, but there seems to be a bit of changes for that too. Some of it looks like it should be a separate patch. > > Cc: [email protected] > Fixes: 0ae6b8ce200d ("ftrace: Fix accounting of subop hashes") > Reported-by: Xiang Mei <[email protected]> > Assisted-by: OpenAI Codex <[email protected]> > Signed-off-by: Weiming Shi <[email protected]> > --- > kernel/trace/fgraph.c | 35 +++++++++++++++++---- > kernel/trace/ftrace.c | 57 +++++++++++++++++++++------------- > kernel/trace/ftrace_internal.h | 3 ++ > 3 files changed, 68 insertions(+), 27 deletions(-) > > diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c > index ed455b535..1a8e092ec 100644 > --- a/kernel/trace/fgraph.c > +++ b/kernel/trace/fgraph.c > @@ -1306,7 +1306,7 @@ static void ftrace_graph_enable_direct(bool > enable_branch, struct fgraph_ops *go > > static_call_update(fgraph_func, func); > static_call_update(fgraph_retfunc, retfunc); > - if (enable_branch) > + if (enable_branch && !static_key_enabled(&fgraph_do_direct.key)) > static_branch_enable(&fgraph_do_direct); Ug that is horrible. If anything static_branch_enable() should handle this. Or we have to have another type of accounting. > } > > @@ -1315,13 +1315,27 @@ static void ftrace_graph_disable_direct(bool > disable_branch) > if (FGRAPH_NO_DIRECT) > return; > > - if (disable_branch) > + if (disable_branch && static_key_enabled(&fgraph_do_direct.key)) > static_branch_disable(&fgraph_do_direct); Same. > static_call_update(fgraph_func, ftrace_graph_entry_stub); > static_call_update(fgraph_retfunc, ftrace_graph_ret_stub); > fgraph_direct_gops = &fgraph_stub; > } > > +void fgraph_update_direct_func(struct ftrace_ops *ops, bool enable) What is this for? It requires a kerneldoc as it is not a static function. It also looks unrelated to the partial shutdown. Is his needed as a separate patch? > +{ > + struct fgraph_ops *gops; > + > + if (ftrace_graph_active != 1 || !(ops->flags & FTRACE_OPS_FL_GRAPH)) > + return; > + > + gops = container_of(ops, struct fgraph_ops, ops); > + if (enable) > + ftrace_graph_enable_direct(true, gops); > + else > + ftrace_graph_disable_direct(true); > +} > + > /* The cpu_boot init_task->ret_stack will never be freed */ > static int fgraph_cpu_init(unsigned int cpu) > { > @@ -1406,16 +1420,24 @@ int register_ftrace_graph(struct fgraph_ops *gops) > gops->ops.flags |= FTRACE_OPS_FL_GRAPH; > > ret = ftrace_startup_subops(&graph_ops, &gops->ops, command); > - if (!ret) > + if (!ret) { > fgraph_array[i] = gops; > + if (ftrace_graph_active == 1) > + ftrace_graph_enable_direct(true, gops); > + } > > error: > if (ret) { > ftrace_graph_active--; > + gops->ops.flags &= ~FTRACE_OPS_FL_GRAPH; Hmm, clearing the flag looks like a separate bug that should be fixed in its own patch. > gops->saved_func = NULL; > fgraph_lru_release_index(i); > - if (!ftrace_graph_active) > + if (ftrace_graph_active == 1) { > + ftrace_graph_enable_direct(true, NULL); > + } else if (!ftrace_graph_active) { > + ftrace_graph_disable_direct(false); > unregister_pm_notifier(&ftrace_suspend_notifier); > + } Nit, but I would have the above opposite. That is, don't make the current if statement the else, unless there's a good reason to. Because it makes reviewing harder. if (!ftrace_graph_active) { ftrace_graph_disable_direct(false); unregister_pm_notifier(&ftrace_suspend_notifier); } else if (ftrace_graph_active == 1) { ftrace_graph_enable_direct(true, NULL); } > } > return ret; > } > @@ -1423,6 +1445,7 @@ int register_ftrace_graph(struct fgraph_ops *gops) > void unregister_ftrace_graph(struct fgraph_ops *gops) > { > int command = 0; > + int ret; > > if (WARN_ONCE(!(gops->ops.flags & FTRACE_OPS_FL_GRAPH), > "function graph ops unregistered without registering")) > @@ -1447,9 +1470,9 @@ void unregister_ftrace_graph(struct fgraph_ops *gops) > if (!ftrace_graph_active) > command = FTRACE_STOP_FUNC_RET; > > - ftrace_shutdown_subops(&graph_ops, &gops->ops, command); > + ret = ftrace_shutdown_subops(&graph_ops, &gops->ops, command); > > - if (ftrace_graph_active == 1) > + if (ftrace_graph_active == 1 && !ret) > ftrace_graph_enable_direct(true, NULL); > else if (!ftrace_graph_active) > ftrace_graph_disable_direct(false); > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > index 673a54fdf..831f04de6 100644 > --- a/kernel/trace/ftrace.c > +++ b/kernel/trace/ftrace.c > @@ -3667,6 +3667,8 @@ static int rebuild_hashes(struct ftrace_hash > **filter_hash, struct ftrace_hash * > bool first = true; > int ret; > > + *filter_hash = EMPTY_HASH; > + *notrace_hash = EMPTY_HASH; > temp_hash.filter_hash = EMPTY_HASH; > temp_hash.notrace_hash = EMPTY_HASH; > > @@ -3708,7 +3710,13 @@ static int rebuild_hashes(struct ftrace_hash > **filter_hash, struct ftrace_hash * > * will not affect functions that are being traced by other subops that > * still exist in @ops. > * > - * If the last subops is removed from @ops, then @ops is shutdown normally. > + * Once removal starts, @subops is detached even if rebuilding the manager > + * hashes fails. The old manager hashes are conservative in that case and the > + * remaining subops continue to filter callbacks. If the last-manager > shutdown > + * fails, ftrace is disabled before the manager state is discarded. > + * > + * Return: 0 on success or a negative error code if the manager could not be > + * updated or shut down. > */ > int ftrace_shutdown_subops(struct ftrace_ops *ops, struct ftrace_ops > *subops, int command) > { > @@ -3716,9 +3724,6 @@ int ftrace_shutdown_subops(struct ftrace_ops *ops, > struct ftrace_ops *subops, in > struct ftrace_hash *notrace_hash = EMPTY_HASH; > int ret; > > - if (unlikely(ftrace_disabled)) > - return -ENODEV; Wait! Why this??? > - > if (WARN_ON_ONCE(!(subops->flags & FTRACE_OPS_FL_ENABLED))) > return -EINVAL; > > @@ -3727,36 +3732,45 @@ int ftrace_shutdown_subops(struct ftrace_ops *ops, > struct ftrace_ops *subops, in > if (list_empty(&ops->subop_list)) { > /* Last one, just disable the current ops */ > > - ret = ftrace_shutdown(ops, command); > + if (unlikely(ftrace_disabled)) > + ret = -ENODEV; > + else > + ret = ftrace_shutdown(ops, command); Ug no! > if (ret < 0) { > - list_add(&subops->list, &ops->subop_list); > - return ret; > + bool enabled = ops->flags & FTRACE_OPS_FL_ENABLED; > + > + FTRACE_WARN_ON_ONCE(!ftrace_disabled); > + remove_ftrace_ops(&ftrace_ops_list, ops); > + if (enabled && ftrace_start_up > 0) > + ftrace_start_up--; > + ops->flags &= ~(FTRACE_OPS_FL_ENABLED | > + FTRACE_OPS_FL_ADDING | > + FTRACE_OPS_FL_REMOVING); > + ops->func = ops->saved_func; Please explain the above. > } > > - subops->flags &= ~FTRACE_OPS_FL_ENABLED; > - > free_ftrace_hash(ops->func_hash->filter_hash); > free_ftrace_hash(ops->func_hash->notrace_hash); > ops->func_hash->filter_hash = EMPTY_HASH; > ops->func_hash->notrace_hash = EMPTY_HASH; > - subops->flags &= ~(FTRACE_OPS_FL_ENABLED | FTRACE_OPS_FL_SUBOP); > - subops->managed = NULL; > + goto out; > + } > > - return 0; > + if (unlikely(ftrace_disabled)) { > + ret = -ENODEV; > + goto out; > } > > /* Rebuild the hashes without subops */ > ret = rebuild_hashes(&filter_hash, ¬race_hash, ops); > if (ret < 0) > - return ret; > + goto out; > > ret = ftrace_update_ops(ops, filter_hash, notrace_hash); > - if (ret < 0) { > - list_add(&subops->list, &ops->subop_list); > - } else { > - subops->flags &= ~(FTRACE_OPS_FL_ENABLED | FTRACE_OPS_FL_SUBOP); > - subops->managed = NULL; > - } > + > +out: > + subops->flags &= ~(FTRACE_OPS_FL_ENABLED | FTRACE_OPS_FL_SUBOP); > + subops->managed = NULL; The above is doing a lot without really explaining why. -- Steve > free_ftrace_hash(filter_hash); > free_ftrace_hash(notrace_hash); > return ret; > @@ -3767,8 +3781,8 @@ static int ftrace_hash_move_and_update_subops(struct > ftrace_ops *subops, > struct ftrace_hash *hash) > { > struct ftrace_ops *ops = subops->managed; > - struct ftrace_hash *notrace_hash; > - struct ftrace_hash *filter_hash; > + struct ftrace_hash *notrace_hash = EMPTY_HASH; > + struct ftrace_hash *filter_hash = EMPTY_HASH; > struct ftrace_hash *save_hash; > struct ftrace_hash *new_hash; > int ret; > @@ -3800,6 +3814,7 @@ static int ftrace_hash_move_and_update_subops(struct > ftrace_ops *subops, > } else { > free_ftrace_hash_rcu(save_hash); > } > + fgraph_update_direct_func(subops, !ret); > return ret; > } > > diff --git a/kernel/trace/ftrace_internal.h b/kernel/trace/ftrace_internal.h > index 3235470e6..3d5934b1c 100644 > --- a/kernel/trace/ftrace_internal.h > +++ b/kernel/trace/ftrace_internal.h > @@ -16,6 +16,7 @@ int ftrace_startup(struct ftrace_ops *ops, int command); > int ftrace_shutdown(struct ftrace_ops *ops, int command); > int ftrace_ops_test(struct ftrace_ops *ops, unsigned long ip, void *regs); > int ftrace_startup_subops(struct ftrace_ops *ops, struct ftrace_ops *subops, > int command); > +/* Once started, shutdown always detaches @subops, even when it returns an > error. */ > int ftrace_shutdown_subops(struct ftrace_ops *ops, struct ftrace_ops > *subops, int command); > > #else /* !CONFIG_DYNAMIC_FTRACE */ > @@ -52,6 +53,7 @@ static inline int ftrace_shutdown_subops(struct ftrace_ops > *ops, struct ftrace_o > > #ifdef CONFIG_FUNCTION_GRAPH_TRACER > extern int ftrace_graph_active; > +void fgraph_update_direct_func(struct ftrace_ops *ops, bool enable); > # ifdef CONFIG_DYNAMIC_FTRACE > extern void fgraph_update_pid_func(void); > # else > @@ -59,6 +61,7 @@ static inline void fgraph_update_pid_func(void) {} > # endif > #else /* !CONFIG_FUNCTION_GRAPH_TRACER */ > # define ftrace_graph_active 0 > +static inline void fgraph_update_direct_func(struct ftrace_ops *ops, bool > enable) {} > static inline void fgraph_update_pid_func(void) {} > #endif /* CONFIG_FUNCTION_GRAPH_TRACER */ >
