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] [day] [month] [year] [list]

Message-ID: <aTCcqGiWnEmGGuRI@krava>
Date: Wed, 3 Dec 2025 21:25:12 +0100
From: Jiri Olsa <olsajiri@...il.com>
To: bot+bpf-ci@...nel.org
Cc: rostedt@...nel.org, revest@...gle.com, mark.rutland@....com,
	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, 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

On Wed, Dec 03, 2025 at 08:47:45AM +0000, bot+bpf-ci@...nel.org wrote:
> > 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.

mamma mia, can't get this right apparently.. yea, will fix in next
version, thanks

jirka


> 
> 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