Re: [PATCH bpf-next 2/3] ftrace: bpf: Fix IPMODIFY + DIRECT in modify_ftrace_direct()

[Steven Rostedt <rostedt@goodmis.org>]

On Fri, 24 Oct 2025 15:47:04 +0000
Song Liu <songliubraving@meta.com> wrote:

> >> --- a/kernel/trace/ftrace.c
> >> +++ b/kernel/trace/ftrace.c
> >> @@ -2020,8 +2020,6 @@ static int __ftrace_hash_update_ipmodify(struct ftrace_ops *ops,
> >> if (is_ipmodify)
> >> goto rollback;
> >> 
> >> - FTRACE_WARN_ON(rec->flags & FTRACE_FL_DIRECT);  
> > 
> > why is this needed?  
> 
> This is needed for the modify_ftrace_direct case, because 
> the record already have a direct function (BPF trampoline)
> attached. 

I don't like the fact that it's removing a check for other cases.

It needs to be denoted that this use case is "OK" where as other use cases
are not. That way the check is still there for other cases and only "OK"
for this use case.

Something like this:

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 370f620734cf..51b205bafe80 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1971,7 +1971,8 @@ static void ftrace_hash_rec_enable_modify(struct ftrace_ops *ops)
  */
 static int __ftrace_hash_update_ipmodify(struct ftrace_ops *ops,
 					 struct ftrace_hash *old_hash,
-					 struct ftrace_hash *new_hash)
+					 struct ftrace_hash *new_hash,
+					 bool update)
 {
 	struct ftrace_page *pg;
 	struct dyn_ftrace *rec, *end = NULL;
@@ -2020,6 +2021,16 @@ static int __ftrace_hash_update_ipmodify(struct ftrace_ops *ops,
 				if (is_ipmodify)
 					goto rollback;
 
+				/*
+				 * If this is called by __modify_ftrace_direct()
+				 * then it is only chaning where the direct
+				 * pointer is jumping to, and the record already
+				 * points to a direct trampoline. If it isn't
+				 * then it is a bug to update ipmodify on a direct
+				 * caller.
+				 */
+				FTRACE_WARN_ON(!update &&
+					       (rec->flags & FTRACE_FL_DIRECT));
 				/*
 				 * Another ops with IPMODIFY is already
 				 * attached. We are now attaching a direct
@@ -2067,14 +2078,14 @@ static int __ftrace_hash_update_ipmodify(struct ftrace_ops *ops,
 	return -EBUSY;
 }
 
-static int ftrace_hash_ipmodify_enable(struct ftrace_ops *ops)
+static int ftrace_hash_ipmodify_enable(struct ftrace_ops *ops, bool update)
 {
 	struct ftrace_hash *hash = ops->func_hash->filter_hash;
 
 	if (ftrace_hash_empty(hash))
 		hash = NULL;
 
-	return __ftrace_hash_update_ipmodify(ops, EMPTY_HASH, hash);
+	return __ftrace_hash_update_ipmodify(ops, EMPTY_HASH, hash, update);
 }
 
 /* Disabling always succeeds */
@@ -2085,7 +2096,7 @@ static void ftrace_hash_ipmodify_disable(struct ftrace_ops *ops)
 	if (ftrace_hash_empty(hash))
 		hash = NULL;
 
-	__ftrace_hash_update_ipmodify(ops, hash, EMPTY_HASH);
+	__ftrace_hash_update_ipmodify(ops, hash, EMPTY_HASH, false);
 }
 
 static int ftrace_hash_ipmodify_update(struct ftrace_ops *ops,
@@ -2099,7 +2110,7 @@ static int ftrace_hash_ipmodify_update(struct ftrace_ops *ops,
 	if (ftrace_hash_empty(new_hash))
 		new_hash = NULL;
 
-	return __ftrace_hash_update_ipmodify(ops, old_hash, new_hash);
+	return __ftrace_hash_update_ipmodify(ops, old_hash, new_hash, false);
 }
 
 static void print_ip_ins(const char *fmt, const unsigned char *p)
@@ -3059,7 +3070,7 @@ int ftrace_startup(struct ftrace_ops *ops, int command)
 	 */
 	ops->flags |= FTRACE_OPS_FL_ENABLED | FTRACE_OPS_FL_ADDING;
 
-	ret = ftrace_hash_ipmodify_enable(ops);
+	ret = ftrace_hash_ipmodify_enable(ops, false);
 	if (ret < 0) {
 		/* Rollback registration process */
 		__unregister_ftrace_function(ops);
@@ -6131,7 +6142,7 @@ __modify_ftrace_direct(struct ftrace_ops *ops, unsigned long addr)
 	 * ops->ops_func for the ops. This is needed because the above
 	 * register_ftrace_function_nolock() worked on tmp_ops.
 	 */
-	err = ftrace_hash_ipmodify_enable(ops);
+	err = ftrace_hash_ipmodify_enable(ops, true);
 	if (err)
 		goto out;
 

-- Steve