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

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

[Weiming Shi]

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.

Cc: stable@vger.kernel.org
Fixes: 0ae6b8ce200d ("ftrace: Fix accounting of subop hashes")
Reported-by: Xiang Mei <xmei5@asu.edu>
Assisted-by: OpenAI Codex <noreply@openai.com>
Signed-off-by: Weiming Shi <bestswngs@gmail.com>
---
 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);
 }
 
@@ -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);
 	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)
+{
+	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;
 		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);
+		}
 	}
 	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;
-
 	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);
 		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;
-		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, &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;
 	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 */
 
-- 
2.56.0

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

[sashiko-bot]

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

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

[Weiming Shi]

Please ignore and disregard this patch ([PATCH v3] ftrace: Complete
subops teardown on shutdown failure). It was sent by mistake.

I apologize for any inconvenience caused.

Best regards,
Weiming Shi