Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] ftrace: register_ftrace_graph() rollback enables direct mode on stale hashes - [High] ftrace: UAF in ftrace_shutdown_subops() freeing manager hashes without RCU - [High] ftrace: UAF of subop and filter hashes after ftrace_shutdown_subops() error - [Medium] ftrace: ftrace_startup_subops() merges new subop into stale manager hashes --
commit 1d6a2634131e25d978349313b44d8bb9b5af4677 Author: Weiming Shi <[email protected]> ftrace: Complete subops teardown on shutdown failure This commit updates ftrace_shutdown_subops() and function graph registration to always detach subops when shutdown begins, keeping conservative manager hashes and disabling direct graph dispatch when hash updates fail. > diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c > index ed455b53513be..1a8e092ec3cf0 100644 > --- a/kernel/trace/fgraph.c > +++ b/kernel/trace/fgraph.c [ ... ] > @@ -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; > gops->saved_func = NULL; > fgraph_lru_release_index(i); > - if (!ftrace_graph_active) > + if (ftrace_graph_active == 1) { > + ftrace_graph_enable_direct(true, NULL); [Severity: High] Can this error rollback in register_ftrace_graph() re-enable direct dispatch while graph_ops.func_hash still holds stale conservative hashes? Suppose ftrace_graph_active is 1, and a prior unregister_ftrace_graph() failed in ftrace_shutdown_subops() (or ftrace_hash_move_and_update_subops() failed), leaving graph_ops.func_hash as a conservative superset with fgraph_do_direct disabled so function_graph_enter_regs() falls back to checking ftrace_ops_test(&gops->ops, func, NULL). If a subsequent register_ftrace_graph() increments ftrace_graph_active from 1 to 2 and then fails in ftrace_startup_subops() (for example, if add_next_hash() or ftrace_update_ops() returns -ENOMEM), this error path decrements ftrace_graph_active back to 1 and unconditionally calls ftrace_graph_enable_direct(true, NULL). With fgraph_do_direct enabled against the wider graph_ops.func_hash, won't function_graph_enter_regs() take the static_branch_likely(&fgraph_do_direct) fast path and invoke the remaining gops->entryfunc on functions outside its filter without calling ftrace_ops_test()? > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > index c7d74b33d36de..b63bb90da9326 100644 > --- a/kernel/trace/ftrace.c > +++ b/kernel/trace/ftrace.c [ ... ] > @@ -3732,36 +3737,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); > 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; > } > > - 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; [Severity: High] When ftrace_disabled is set or ftrace_shutdown() fails, can freeing ops->func_hash->filter_hash and ops->func_hash->notrace_hash here cause a use-after-free in ftrace_ops_test()? When ftrace_shutdown() does not run ftrace_run_update_code(), patched callsites remain active and can still invoke __ftrace_ops_list_func() -> ftrace_ops_test(), which reads ops->func_hash->filter_hash and ops->func_hash->notrace_hash under preempt_disable(). Here, ftrace_shutdown_subops() removes ops from ftrace_ops_list without calling update_ftrace_function() or waiting for an RCU grace period, and calls synchronous free_ftrace_hash() before resetting filter_hash and notrace_hash to EMPTY_HASH. Should the pointers be assigned to EMPTY_HASH first and the old hashes retired via free_ftrace_hash_rcu() (or after proper synchronization)? > + 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; [Severity: High] Does detaching subops and clearing FTRACE_OPS_FL_ENABLED on error here allow callers to free subops and its filter hashes while other CPUs are still using them? In unregister_ftrace_graph(): fgraph_array[gops->idx] = &fgraph_stub; ftrace_graph_active--; ... ret = ftrace_shutdown_subops(&graph_ops, &gops->ops, command); On the normal success path, ftrace_shutdown_subops() calls ftrace_shutdown() or ftrace_update_ops(), which runs ftrace_run_update_code() and synchronizes across CPUs before unregister_ftrace_graph() returns. However, if rebuild_hashes() or ftrace_update_ops() fails (or ftrace_disabled is set), ftrace_shutdown_subops() now jumps to out, clears FTRACE_OPS_FL_ENABLED, and returns without any cross-CPU synchronization having occurred after fgraph_array[gops->idx] was set to &fgraph_stub. Callers such as __fprobe_graph_unregister() and ftrace_destroy_function_files() immediately follow unregister_ftrace_graph() with ftrace_free_filter(&gops->ops) (which synchronously kfrees filter_hash and notrace_hash once FTRACE_OPS_FL_ENABLED is cleared) or kfree(tr->gops). Can a concurrent CPU that loaded gops from fgraph_array[] in function_graph_enter_regs() or __ftrace_return_to_handler() just before it was replaced with &fgraph_stub suffer a use-after-free? [Severity: Medium] Also, when rebuild_hashes() or ftrace_update_ops() fails while other subops remain on ops->subop_list, subops is unlinked from ops->subop_list while ops->func_hash keeps the old conservative hashes. If another subop is later registered via ftrace_startup_subops() while ops->subop_list is still non-empty: ret = add_next_hash(&filter_hash, ¬race_hash, ops->func_hash, subops->func_hash); Because ftrace_startup_subops() merges subops->func_hash into ops->func_hash via add_next_hash() rather than rebuilding from ops->subop_list via rebuild_hashes(), won't the stale entries from the previously detached subop be permanently carried forward into the new manager hashes? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
