linux-kernel - Re: [PATCHv5 bpf-next 6/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: <20251218104119.311adc09@gandalf.local.home>
Date: Thu, 18 Dec 2025 10:41:19 -0500
From: Steven Rostedt <rostedt@...dmis.org>
To: Jiri Olsa <jolsa@...nel.org>
Cc: Steven Rostedt <rostedt@...nel.org>, Florent Revest <revest@...gle.com>,
 Mark Rutland <mark.rutland@....com>, bpf@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, Alexei Starovoitov <ast@...nel.org>,
 Daniel Borkmann <daniel@...earbox.net>, Andrii Nakryiko
 <andrii@...nel.org>, Menglong Dong <menglong8.dong@...il.com>, Song Liu
 <song@...nel.org>
Subject: Re: [PATCHv5 bpf-next 6/9] ftrace: Add update_ftrace_direct_mod
 function

On Thu, 18 Dec 2025 10:19:42 -0500
Steven Rostedt <rostedt@...dmis.org> wrote:

> On Mon, 15 Dec 2025 22:13:59 +0100
> Jiri Olsa <jolsa@...nel.org> wrote:
> 
> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index 48dc0de5f2ce..95a38fb18ed7 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
> > @@ -6489,6 +6489,78 @@ int update_ftrace_direct_del(struct ftrace_ops *ops, struct ftrace_hash *hash)
> >  	return err;
> >  }
> >    
> 
> Kerneldoc needed.
> 
> > +int update_ftrace_direct_mod(struct ftrace_ops *ops, struct ftrace_hash *hash, bool do_direct_lock)
> > +{
> > +	struct ftrace_func_entry *entry, *tmp;
> > +	static struct ftrace_ops tmp_ops = {
> > +		.func		= ftrace_stub,
> > +		.flags		= FTRACE_OPS_FL_STUB,
> > +	};
> > +	struct ftrace_hash *orig_hash;
> > +	unsigned long size, i;
> > +	int err = -EINVAL;
> > +
> > +	if (!hash_count(hash))
> > +		return -EINVAL;
> > +	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);  
> 
> This optional taking of the direct_mutex lock needs some serious rationale
> and documentation.
> 
> > +
> > +	orig_hash = ops->func_hash ? ops->func_hash->filter_hash : NULL;
> > +	if (!orig_hash)
> > +		goto unlock;
> > +
> > +	/* Enable the tmp_ops to have the same functions as the direct ops */

Add to the comments here:

	 * In order to modify the direct callers, all the functions need to
	 * first be calling the ftrace_ops_list_func() and not be connected
	 * to any direct callers. To do that, create a temporary ops that
	 * attach to the same functions as the direct ops, and attach that
	 * first. Then when adding the direct ops, it will use the
	 * ftrace_ops_list_func(), and this can safely modify what the
	 * direct ops call.

Or something like that. I want this code to be as clear as day to what it
is doing. In a year or two, we will forget, and this will be very confusing
to newcomers.

> > +	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);  
> 
> I'm going to need some time staring at this code. It looks like it may be
> relying on some internals here.
> 
> -- Steve
> 
> 
> > +

I would add a comment here:

	/* Now update the direct functions to point to the new callbacks */

-- Steve

> > +	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 */
> >  
> >  /**