[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

[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