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

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 3918 bytes --]

> 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