Re: [PATCH v3] ftrace: Complete subops teardown on shutdown failure

[sashiko-bot@kernel.org]

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 <bestswngs@gmail.com>

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, &notrace_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, &notrace_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/20261031175213.704021-1-bestswngs@gmail.com?part=1