linux-kernel - Re: [PATCHv4 bpf-next 4/9] ftrace: Add update_ftrace_direct

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <d0d8e0d78f614eb8b443772b50fb36093d48c7f43374316246ad3ad836b5c9ce@mail.kernel.org>
Date: Wed,  3 Dec 2025 08:47:45 +0000 (UTC)
From: bot+bpf-ci@...nel.org
To: jolsa@...nel.org,rostedt@...nel.org,revest@...gle.com,mark.rutland@....com
Cc: bpf@...r.kernel.org,linux-kernel@...r.kernel.org,linux-trace-kernel@...r.kernel.org,linux-arm-kernel@...ts.infradead.org,ast@...nel.org,daniel@...earbox.net,andrii@...nel.org,menglong8.dong@...il.com,song@...nel.org,ast@...nel.org,andrii@...nel.org,daniel@...earbox.net,martin.lau@...nel.org,eddyz87@...il.com,yonghong.song@...ux.dev,clm@...a.com,ihor.solodrai@...ux.dev
Subject: Re: [PATCHv4 bpf-next 4/9] ftrace: Add update_ftrace_direct_add function

> 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

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/19887401362