> On Oct 24, 2025, at 9:21 AM, Steven Rostedt <[email protected]> wrote:
>
> On Fri, 24 Oct 2025 15:47:04 +0000
> Song Liu <[email protected]> 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.
Acked that removing check for other cases is not ideal.
I looked at the code a bit more and got a slightly different
version (attached below).
The basic idea is to leave existing ftrace_hash_ipmodify_*
logically the same, and call __ftrace_hash_update_ipmodify
directly from __modify_ftrace_direct().
I think this is logically cleaner.
Does this make sense?
Thanks,
Song
diff --git i/kernel/trace/ftrace.c w/kernel/trace/ftrace.c
index 370f620734cf..4f6745dddc35 100644
--- i/kernel/trace/ftrace.c
+++ w/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_target)
{
struct ftrace_page *pg;
struct dyn_ftrace *rec, *end = NULL;
@@ -2006,10 +2007,13 @@ static int __ftrace_hash_update_ipmodify(struct
ftrace_ops *ops,
if (rec->flags & FTRACE_FL_DISABLED)
continue;
- /* We need to update only differences of filter_hash */
+ /*
+ * Unless we are updating the target of a direct function,
+ * we only need to update differences of filter_hash
+ */
in_old = !!ftrace_lookup_ip(old_hash, rec->ip);
in_new = !!ftrace_lookup_ip(new_hash, rec->ip);
- if (in_old == in_new)
+ if (!update_target && (in_old == in_new))
continue;
if (in_new) {
@@ -2020,6 +2024,17 @@ 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_target &&
+ (rec->flags & FTRACE_FL_DIRECT));
+
/*
* Another ops with IPMODIFY is already
* attached. We are now attaching a direct
@@ -2074,7 +2089,7 @@ static int ftrace_hash_ipmodify_enable(struct ftrace_ops
*ops)
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, false);
}
/* Disabling always succeeds */
@@ -2085,7 +2100,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 +2114,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)
@@ -6106,7 +6121,7 @@ EXPORT_SYMBOL_GPL(unregister_ftrace_direct);
static int
__modify_ftrace_direct(struct ftrace_ops *ops, unsigned long addr)
{
- struct ftrace_hash *hash;
+ struct ftrace_hash *hash = ops->func_hash->filter_hash;
struct ftrace_func_entry *entry, *iter;
static struct ftrace_ops tmp_ops = {
.func = ftrace_stub,
@@ -6131,7 +6146,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_update_ipmodify(ops, hash, hash, true);
if (err)
goto out;
@@ -6141,7 +6156,6 @@ __modify_ftrace_direct(struct ftrace_ops *ops, unsigned
long addr)
*/
mutex_lock(&ftrace_lock);
- hash = ops->func_hash->filter_hash;
size = 1 << hash->size_bits;
for (i = 0; i < size; i++) {
hlist_for_each_entry(iter, &hash->buckets[i], hlist) {
