ftrace,bpf: Use single direct ops for bpf trampolines

[kernel-patches-daemon-bpf]

Pull request for series with
subject: ftrace,bpf: Use single direct ops for bpf trampolines
version: 1
url: https://patchwork.kernel.org/project/netdevbpf/list/?series=1030012

[kernel-patches-daemon-bpf]

Upstream branch: ff34657
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1030012
version: 1

[kernel-patches-review-bot]

commit dab46cc6e7ca008f3b8f49b874361f956b6e8e6a
Author: Jiri Olsa <[email protected]>

ftrace: Add update_ftrace_direct_add function

This patch adds update_ftrace_direct_add() which allows registering
multiple ip->addr entries at once via a hash argument, and can be
called multiple times on the same ftrace_ops object.

> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 8c034b028..eb60d971e 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h

[ ... ]

> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 84aee9096..1660a8754 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c

[ ... ]

> +int update_ftrace_direct_add(struct ftrace_ops *ops, struct ftrace_hash *hash)
> +{
> +	struct ftrace_hash *old_direct_functions = NULL, *new_direct_functions = NULL;
> +	struct ftrace_hash *old_filter_hash = NULL, *new_filter_hash = NULL;
> +	struct ftrace_func_entry *entry;
> +	int i, size, err = -EINVAL;
> +	bool reg;
> +
> +	if (!hash_count(hash))
> +		return -EINVAL;
> +
> +	mutex_lock(&direct_mutex);
> +
> +	/* Make sure requested entries are not already registered. */
> +	size = 1 << hash->size_bits;
> +	for (i = 0; i < size; i++) {
> +		hlist_for_each_entry(entry, &hash->buckets[i], hlist) {
> +			if (__ftrace_lookup_ip(direct_functions, entry->ip))
> +				goto out_unlock;
> +		}
> +	}
> +
> +	old_filter_hash = ops->func_hash ? ops->func_hash->filter_hash : NULL;
> +	old_direct_functions = direct_functions;
> +
> +	/* If there's nothing in filter_hash we need to register the ops. */
> +	reg = hash_count(old_filter_hash) == 0;
> +	if (reg) {
> +		if (ops->func || ops->trampoline)
> +			goto out_unlock;
> +		if (ops->flags & FTRACE_OPS_FL_ENABLED)
> +			goto out_unlock;
> +	}
> +
> +	err = -ENOMEM;
> +	new_filter_hash = hash_add(old_filter_hash, hash);
> +	if (!new_filter_hash)
> +		goto out_unlock;
> +
> +	new_direct_functions = hash_add(old_direct_functions, hash);
> +	if (!new_direct_functions)
> +		goto out_unlock;

Can new_filter_hash leak here?  If the second hash_add() fails but the
first one succeeds, update_ftrace_direct_add() will goto out_unlock
with new_filter_hash pointing to allocated memory.  At out_unlock, only
old_filter_hash gets freed, not new_filter_hash.

register_ftrace_direct() handles this by freeing new_hash on error:

    out_unlock:
        mutex_unlock(&direct_mutex);

        if (free_hash && free_hash != EMPTY_HASH)
            call_rcu_tasks(&free_hash->rcu, register_ftrace_direct_cb);

        if (new_hash)
            free_ftrace_hash(new_hash);

        return err;

> +
> +	rcu_assign_pointer(direct_functions, new_direct_functions);
> +
> +	if (reg) {
> +		ops->func = call_direct_funcs;
> +		ops->flags |= MULTI_FLAGS;
> +		ops->trampoline = FTRACE_REGS_ADDR;
> +		ops->local_hash.filter_hash = new_filter_hash;
> +
> +		err = register_ftrace_function_nolock(ops);
> +		if (err) {
> +			/* restore old filter on error */
> +			ops->local_hash.filter_hash = old_filter_hash;
> +			old_filter_hash = new_filter_hash;
> +
> +			/* cleanup for possible another register call */
> +			ops->func = NULL;
> +			ops->trampoline = 0;
> +		}
> +	} else {
> +		err = ftrace_update_ops(ops, new_filter_hash, EMPTY_HASH);
> +		/*
> +		 * new_filter_hash is dup-ed, so we need to release it anyway,
> +		 * old_filter_hash either stays on error or is released already
> +		 */
> +		old_filter_hash = new_filter_hash;
> +	}
> +
> +	if (err) {
> +		/* reset direct_functions and free the new one */
> +		rcu_assign_pointer(direct_functions, old_direct_functions);
> +		old_direct_functions = new_direct_functions;
> +	}
> +
> + out_unlock:
> +	mutex_unlock(&direct_mutex);
> +
> +	if (old_direct_functions && old_direct_functions != EMPTY_HASH)
> +		call_rcu_tasks(&old_direct_functions->rcu, register_ftrace_direct_cb);
> +	if (old_filter_hash)
> +		free_ftrace_hash(old_filter_hash);
> +
> +	return err;
> +}

---

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

In-Reply-To-Subject: ftrace: Add update_ftrace_direct_add function
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/19887401362

[kernel-patches-daemon-bpf]

Forwarding comment 3605698383 via email
In-Reply-To: [email protected]
Patch: https://patchwork.kernel.org/project/netdevbpf/patch/[email protected]/

[kernel-patches-daemon-bpf]

Upstream branch: ff34657
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1030012
version: 1

[kernel-patches-daemon-bpf]

Upstream branch: 8f7aa3d
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1030012
version: 1

[kernel-patches-daemon-bpf]

Upstream branch: 835a507
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1030012
version: 1

[kernel-patches-daemon-bpf]

Upstream branch: 835a507
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1030012
version: 1

[kernel-patches-daemon-bpf]

Upstream branch: 835a507
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1030012
version: 1

[kernel-patches-daemon-bpf]

Upstream branch: 81f88f6
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1030012
version: 1

[kernel-patches-daemon-bpf]

Upstream branch: 5d9fb42
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1030012
version: 1