1. Patchew
  2. linux
  3. [PATCHv2 bpf-next 0/9] ftrace,bpf: Use single direct ops for bpf trampolines
  4. View patch

[PATCHv2 bpf-next 5/8] ftrace: Add update_ftrace_direct_mod function

Jiri Olsa posted 9 patches 2 months, 3 weeks ago
Only 8 patches received!
There is a newer version of this series
[PATCHv2 bpf-next 5/8] ftrace: Add update_ftrace_direct_mod function
Posted by Jiri Olsa 2 months, 3 weeks ago 
Adding update_ftrace_direct_mod function that modifies all entries
(ip -> direct) provided in hash argument to direct ftrace ops and
updates its attachments.

The difference to current modify_ftrace_direct is:
- hash argument that allows to modify multiple ip -> direct
  entries at once

This change will allow us to have simple ftrace_ops for all bpf
direct interface users in following changes.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 include/linux/ftrace.h |  6 ++++
 kernel/trace/ftrace.c  | 68 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 433c36c3af3b..bacb6d9ab426 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -544,6 +544,7 @@ int modify_ftrace_direct_nolock(struct ftrace_ops *ops, unsigned long addr);
 
 int update_ftrace_direct_add(struct ftrace_ops *ops, struct ftrace_hash *hash);
 int update_ftrace_direct_del(struct ftrace_ops *ops, struct ftrace_hash *hash);
+int update_ftrace_direct_mod(struct ftrace_ops *ops, struct ftrace_hash *hash, bool do_direct_lock);
 
 void ftrace_stub_direct_tramp(void);
 
@@ -581,6 +582,11 @@ int update_ftrace_direct_del(struct ftrace_ops *ops, struct ftrace_hash *hash)
 	return -ENODEV;
 }
 
+int modify_ftrace_direct_hash(struct ftrace_ops *ops, struct ftrace_hash *hash, bool do_direct_lock)
+{
+	return -ENODEV;
+}
+
 /*
  * This must be implemented by the architecture.
  * It is the way the ftrace direct_ops helper, when called
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 3dd1e69aceca..e701516a4a65 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -6475,6 +6475,74 @@ int update_ftrace_direct_del(struct ftrace_ops *ops, struct ftrace_hash *hash)
 	return err;
 }
 
+int update_ftrace_direct_mod(struct ftrace_ops *ops, struct ftrace_hash *hash, bool do_direct_lock)
+{
+	struct ftrace_hash *orig_hash = ops->func_hash->filter_hash;
+	struct ftrace_func_entry *entry, *tmp;
+	static struct ftrace_ops tmp_ops = {
+		.func		= ftrace_stub,
+		.flags		= FTRACE_OPS_FL_STUB,
+	};
+	unsigned long size, i;
+	int err;
+
+	if (!hash_count(hash))
+		return 0;
+	if (check_direct_multi(ops))
+		return -EINVAL;
+	if (!(ops->flags & FTRACE_OPS_FL_ENABLED))
+		return -EINVAL;
+	if (direct_functions == EMPTY_HASH)
+		return -EINVAL;
+
+	if (do_direct_lock)
+		mutex_lock(&direct_mutex);
+
+	/* Enable the tmp_ops to have the same functions as the direct ops */
+	ftrace_ops_init(&tmp_ops);
+	tmp_ops.func_hash = ops->func_hash;
+
+	err = register_ftrace_function_nolock(&tmp_ops);
+	if (err)
+		goto unlock;
+
+	/*
+	 * Call __ftrace_hash_update_ipmodify() here, so that we can call
+	 * ops->ops_func for the ops. This is needed because the above
+	 * register_ftrace_function_nolock() worked on tmp_ops.
+	 */
+	err = __ftrace_hash_update_ipmodify(ops, orig_hash, orig_hash, true);
+	if (err)
+		goto out;
+
+	/*
+	 * Now the ftrace_ops_list_func() is called to do the direct callers.
+	 * We can safely change the direct functions attached to each entry.
+	 */
+	mutex_lock(&ftrace_lock);
+
+	size = 1 << hash->size_bits;
+	for (i = 0; i < size; i++) {
+		hlist_for_each_entry(entry, &hash->buckets[i], hlist) {
+			tmp = __ftrace_lookup_ip(direct_functions, entry->ip);
+			if (!tmp)
+				continue;
+			tmp->direct = entry->direct;
+		}
+	}
+
+	mutex_unlock(&ftrace_lock);
+
+out:
+	/* Removing the tmp_ops will add the updated direct callers to the functions */
+	unregister_ftrace_function(&tmp_ops);
+
+unlock:
+	if (do_direct_lock)
+		mutex_unlock(&direct_mutex);
+	return err;
+}
+
 #endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */
 
 /**
-- 
2.51.1
Re: [PATCHv2 bpf-next 5/8] ftrace: Add update_ftrace_direct_mod function
Posted by bot+bpf-ci@kernel.org 2 months, 3 weeks ago 
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 433c36c3a..bacb6d9ab 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -544,6 +544,7 @@ int modify_ftrace_direct_nolock(struct ftrace_ops *ops, unsigned long addr);
>
>  int update_ftrace_direct_add(struct ftrace_ops *ops, struct ftrace_hash *hash);
>  int update_ftrace_direct_del(struct ftrace_ops *ops, struct ftrace_hash *hash);
> +int update_ftrace_direct_mod(struct ftrace_ops *ops, struct ftrace_hash *hash, bool do_direct_lock);
>
>  void ftrace_stub_direct_tramp(void);
>
> @@ -581,6 +582,11 @@ int update_ftrace_direct_del(struct ftrace_ops *ops, struct ftrace_hash *hash)
>  	return -ENODEV;
>  }
>
> +int modify_ftrace_direct_hash(struct ftrace_ops *ops, struct ftrace_hash *hash, bool do_direct_lock)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^
The stub function name doesn't match the declaration. Should this be
update_ftrace_direct_mod() instead of modify_ftrace_direct_hash()?

> +{
> +	return -ENODEV;
> +}
> +

[ ... ]

> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 345f3fdd2..627d05b57 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -6445,6 +6445,74 @@ int update_ftrace_direct_del(struct ftrace_ops *ops, struct ftrace_hash *hash)
>  	return err;
>  }
>
> +int update_ftrace_direct_mod(struct ftrace_ops *ops, struct ftrace_hash *hash, bool do_direct_lock)
> +{
> +	struct ftrace_hash *orig_hash = ops->func_hash->filter_hash;
> +	struct ftrace_func_entry *entry, *tmp;
> +	static struct ftrace_ops tmp_ops = {
> +		.func		= ftrace_stub,
> +		.flags		= FTRACE_OPS_FL_STUB,
> +	};
> +	unsigned long size, i;
> +	int err;
> +
> +	if (!hash_count(hash))
> +		return 0;
> +	if (check_direct_multi(ops))
> +		return -EINVAL;
> +	if (!(ops->flags & FTRACE_OPS_FL_ENABLED))
> +		return -EINVAL;
> +	if (direct_functions == EMPTY_HASH)
> +		return -EINVAL;
> +
> +	if (do_direct_lock)
> +		mutex_lock(&direct_mutex);
> +
> +	/* Enable the tmp_ops to have the same functions as the direct ops */
> +	ftrace_ops_init(&tmp_ops);
> +	tmp_ops.func_hash = ops->func_hash;
> +
> +	err = register_ftrace_function_nolock(&tmp_ops);
> +	if (err)
> +		goto unlock;
> +
> +	/*
> +	 * Call __ftrace_hash_update_ipmodify() here, so that we can call
> +	 * ops->ops_func for the ops. This is needed because the above
> +	 * register_ftrace_function_nolock() worked on tmp_ops.
> +	 */
> +	err = __ftrace_hash_update_ipmodify(ops, orig_hash, orig_hash, true);
                                                                        ^^^^
Does __ftrace_hash_update_ipmodify() accept a fourth argument? The
function signature shows it only takes three parameters (ops, old_hash,
new_hash). This looks like a compilation error.

> +	if (err)
> +		goto out;

[ ... ]

> +
> +out:
> +	/* Removing the tmp_ops will add the updated direct callers to the functions */
> +	unregister_ftrace_function(&tmp_ops);
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Can this deadlock when do_direct_lock is true? Looking at the code:

  update_ftrace_direct_mod() with do_direct_lock=true
    -> mutex_lock(&direct_mutex)
    -> register_ftrace_function_nolock(&tmp_ops) succeeds
    -> __ftrace_hash_update_ipmodify() fails
    -> goto out (still holding direct_mutex)
    -> unregister_ftrace_function(&tmp_ops)
       -> cleanup_direct_functions_after_ipmodify()
          -> mutex_lock(&direct_mutex) /* deadlock */

The unregister path tries to reacquire direct_mutex that we're already
holding.

> +
> +unlock:
> +	if (do_direct_lock)
> +		mutex_unlock(&direct_mutex);
> +	return err;
> +}
> +
>  #endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/19332026793
Re: [PATCHv2 bpf-next 5/8] ftrace: Add update_ftrace_direct_mod function
Posted by Jiri Olsa 2 months, 3 weeks ago 
On Thu, Nov 13, 2025 at 01:02:17PM +0000, bot+bpf-ci@kernel.org wrote:
> > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> > index 433c36c3a..bacb6d9ab 100644
> > --- a/include/linux/ftrace.h
> > +++ b/include/linux/ftrace.h
> > @@ -544,6 +544,7 @@ int modify_ftrace_direct_nolock(struct ftrace_ops *ops, unsigned long addr);
> >
> >  int update_ftrace_direct_add(struct ftrace_ops *ops, struct ftrace_hash *hash);
> >  int update_ftrace_direct_del(struct ftrace_ops *ops, struct ftrace_hash *hash);
> > +int update_ftrace_direct_mod(struct ftrace_ops *ops, struct ftrace_hash *hash, bool do_direct_lock);
> >
> >  void ftrace_stub_direct_tramp(void);
> >
> > @@ -581,6 +582,11 @@ int update_ftrace_direct_del(struct ftrace_ops *ops, struct ftrace_hash *hash)
> >  	return -ENODEV;
> >  }
> >
> > +int modify_ftrace_direct_hash(struct ftrace_ops *ops, struct ftrace_hash *hash, bool do_direct_lock)
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^
> The stub function name doesn't match the declaration. Should this be
> update_ftrace_direct_mod() instead of modify_ftrace_direct_hash()?

it should, my bad

> 
> > +{
> > +	return -ENODEV;
> > +}
> > +
> 
> [ ... ]
> 
> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index 345f3fdd2..627d05b57 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
> > @@ -6445,6 +6445,74 @@ int update_ftrace_direct_del(struct ftrace_ops *ops, struct ftrace_hash *hash)
> >  	return err;
> >  }
> >
> > +int update_ftrace_direct_mod(struct ftrace_ops *ops, struct ftrace_hash *hash, bool do_direct_lock)
> > +{
> > +	struct ftrace_hash *orig_hash = ops->func_hash->filter_hash;
> > +	struct ftrace_func_entry *entry, *tmp;
> > +	static struct ftrace_ops tmp_ops = {
> > +		.func		= ftrace_stub,
> > +		.flags		= FTRACE_OPS_FL_STUB,
> > +	};
> > +	unsigned long size, i;
> > +	int err;
> > +
> > +	if (!hash_count(hash))
> > +		return 0;
> > +	if (check_direct_multi(ops))
> > +		return -EINVAL;
> > +	if (!(ops->flags & FTRACE_OPS_FL_ENABLED))
> > +		return -EINVAL;
> > +	if (direct_functions == EMPTY_HASH)
> > +		return -EINVAL;
> > +
> > +	if (do_direct_lock)
> > +		mutex_lock(&direct_mutex);
> > +
> > +	/* Enable the tmp_ops to have the same functions as the direct ops */
> > +	ftrace_ops_init(&tmp_ops);
> > +	tmp_ops.func_hash = ops->func_hash;
> > +
> > +	err = register_ftrace_function_nolock(&tmp_ops);
> > +	if (err)
> > +		goto unlock;
> > +
> > +	/*
> > +	 * Call __ftrace_hash_update_ipmodify() here, so that we can call
> > +	 * ops->ops_func for the ops. This is needed because the above
> > +	 * register_ftrace_function_nolock() worked on tmp_ops.
> > +	 */
> > +	err = __ftrace_hash_update_ipmodify(ops, orig_hash, orig_hash, true);
>                                                                         ^^^^
> Does __ftrace_hash_update_ipmodify() accept a fourth argument? The
> function signature shows it only takes three parameters (ops, old_hash,
> new_hash). This looks like a compilation error.

the whole patchset is based on bpf-next/master plus Song's livepatch
fixes which change modify_ftrace_direct_hash function, it's mentioned
in the cover letter

> 
> > +	if (err)
> > +		goto out;
> 
> [ ... ]
> 
> > +
> > +out:
> > +	/* Removing the tmp_ops will add the updated direct callers to the functions */
> > +	unregister_ftrace_function(&tmp_ops);
>          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Can this deadlock when do_direct_lock is true? Looking at the code:
> 
>   update_ftrace_direct_mod() with do_direct_lock=true
>     -> mutex_lock(&direct_mutex)
>     -> register_ftrace_function_nolock(&tmp_ops) succeeds
>     -> __ftrace_hash_update_ipmodify() fails
>     -> goto out (still holding direct_mutex)
>     -> unregister_ftrace_function(&tmp_ops)
>        -> cleanup_direct_functions_after_ipmodify()
>           -> mutex_lock(&direct_mutex) /* deadlock */
> 
> The unregister path tries to reacquire direct_mutex that we're already
> holding.

I don't think this can happen because tmp_ops does not have
FTRACE_OPS_FL_IPMODIFY flag set

thanks,
jirka

> 
> > +
> > +unlock:
> > +	if (do_direct_lock)
> > +		mutex_unlock(&direct_mutex);
> > +	return err;
> > +}
> > +
> >  #endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */
> 
> 
> ---
> AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
> See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
> 
> CI run summary: https://github.com/kernel-patches/bpf/actions/runs/19332026793
Re: [PATCHv2 bpf-next 5/8] ftrace: Add update_ftrace_direct_mod function
Posted by Alexei Starovoitov 2 months, 3 weeks ago 
On Thu, Nov 13, 2025 at 8:00 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Thu, Nov 13, 2025 at 01:02:17PM +0000, bot+bpf-ci@kernel.org wrote:
> > > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> > > index 433c36c3a..bacb6d9ab 100644
> > > --- a/include/linux/ftrace.h
> > > +++ b/include/linux/ftrace.h
> > > @@ -544,6 +544,7 @@ int modify_ftrace_direct_nolock(struct ftrace_ops *ops, unsigned long addr);
> > >
> > >  int update_ftrace_direct_add(struct ftrace_ops *ops, struct ftrace_hash *hash);
> > >  int update_ftrace_direct_del(struct ftrace_ops *ops, struct ftrace_hash *hash);
> > > +int update_ftrace_direct_mod(struct ftrace_ops *ops, struct ftrace_hash *hash, bool do_direct_lock);
> > >
> > >  void ftrace_stub_direct_tramp(void);
> > >
> > > @@ -581,6 +582,11 @@ int update_ftrace_direct_del(struct ftrace_ops *ops, struct ftrace_hash *hash)
> > >     return -ENODEV;
> > >  }
> > >
> > > +int modify_ftrace_direct_hash(struct ftrace_ops *ops, struct ftrace_hash *hash, bool do_direct_lock)
> >                 ^^^^^^^^^^^^^^^^^^^^^^^^^^
> > The stub function name doesn't match the declaration. Should this be
> > update_ftrace_direct_mod() instead of modify_ftrace_direct_hash()?
>
> it should, my bad
>
> >
> > > +{
> > > +   return -ENODEV;
> > > +}
> > > +
> >
> > [ ... ]
> >
> > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > > index 345f3fdd2..627d05b57 100644
> > > --- a/kernel/trace/ftrace.c
> > > +++ b/kernel/trace/ftrace.c
> > > @@ -6445,6 +6445,74 @@ int update_ftrace_direct_del(struct ftrace_ops *ops, struct ftrace_hash *hash)
> > >     return err;
> > >  }
> > >
> > > +int update_ftrace_direct_mod(struct ftrace_ops *ops, struct ftrace_hash *hash, bool do_direct_lock)
> > > +{
> > > +   struct ftrace_hash *orig_hash = ops->func_hash->filter_hash;
> > > +   struct ftrace_func_entry *entry, *tmp;
> > > +   static struct ftrace_ops tmp_ops = {
> > > +           .func           = ftrace_stub,
> > > +           .flags          = FTRACE_OPS_FL_STUB,
> > > +   };
> > > +   unsigned long size, i;
> > > +   int err;
> > > +
> > > +   if (!hash_count(hash))
> > > +           return 0;
> > > +   if (check_direct_multi(ops))
> > > +           return -EINVAL;
> > > +   if (!(ops->flags & FTRACE_OPS_FL_ENABLED))
> > > +           return -EINVAL;
> > > +   if (direct_functions == EMPTY_HASH)
> > > +           return -EINVAL;
> > > +
> > > +   if (do_direct_lock)
> > > +           mutex_lock(&direct_mutex);
> > > +
> > > +   /* Enable the tmp_ops to have the same functions as the direct ops */
> > > +   ftrace_ops_init(&tmp_ops);
> > > +   tmp_ops.func_hash = ops->func_hash;
> > > +
> > > +   err = register_ftrace_function_nolock(&tmp_ops);
> > > +   if (err)
> > > +           goto unlock;
> > > +
> > > +   /*
> > > +    * Call __ftrace_hash_update_ipmodify() here, so that we can call
> > > +    * ops->ops_func for the ops. This is needed because the above
> > > +    * register_ftrace_function_nolock() worked on tmp_ops.
> > > +    */
> > > +   err = __ftrace_hash_update_ipmodify(ops, orig_hash, orig_hash, true);
> >                                                                         ^^^^
> > Does __ftrace_hash_update_ipmodify() accept a fourth argument? The
> > function signature shows it only takes three parameters (ops, old_hash,
> > new_hash). This looks like a compilation error.
>
> the whole patchset is based on bpf-next/master plus Song's livepatch
> fixes which change modify_ftrace_direct_hash function, it's mentioned
> in the cover letter

Ohh. Will send bpf PR to Linus today and merge into bpf-next afterwards.

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.