Re: [PATCH] tracing/fprobe: Check the same type fprobe on table as the unregistered one

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

On Wed, 8 Apr 2026 11:02:37 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> Hmm,
> 
> I also found another problem when probing module with fprobe.
> 
> /sys/kernel/tracing # insmod xt_LOG.ko 
> /sys/kernel/tracing # echo 'f:test log_tg*' > dynamic_events 
> /sys/kernel/tracing # echo 1 > events/fprobes/test/enable 
> /sys/kernel/tracing # cat enabled_functions 
> log_tg [xt_LOG] (1)             tramp: 0xffffffffa0004000 (fprobe_ftrace_entry+0x0/0x490) ->fprobe_ftrace_entry+0x0/0x490
> log_tg_check [xt_LOG] (1)               tramp: 0xffffffffa0004000 (fprobe_ftrace_entry+0x0/0x490) ->fprobe_ftrace_entry+0x0/0x490
> log_tg_destroy [xt_LOG] (1)             tramp: 0xffffffffa0004000 (fprobe_ftrace_entry+0x0/0x490) ->fprobe_ftrace_entry+0x0/0x490
> /sys/kernel/tracing # wc -l enabled_functions 
> 3 enabled_functions
> /sys/kernel/tracing # rmmod xt_LOG
> /sys/kernel/tracing # wc -l enabled_functions 
> 34085 enabled_functions
> 
> It seems to reverse the selected functions if the hash is empty...

So I think there are several ways to fix this issue.

 - Introduce a new flag for ftrace_ops, which is "empty means empty".

 - Improve ftrace to automatically remove module function entries
   from registered ftrace_ops. (currently it is done for the ftrace_ops
   in trace_array list, but not for ftrace_ops list.)

 - While walking over the fprobe_ip_tables, try to find existing
   enabled fprobe node. If it does not exist, unregister ftrace_ops.

 - Count the ftrace_ops hash table size and if it is expected to be 0,
   unregister the ftrace_ops.

The first 2 are changing ftrace (but cleaner). The latter 2 only
changes fprobes, but relatively ugly. I think the 2nd one will be
the best because we already have done for ftrace_ops of ring-buffer
instances.

Thanks,


> 
> Thanks,
> 
> On Tue,  7 Apr 2026 19:24:12 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> 
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > 
> > Commit 2c67dc457bc6 ("tracing: fprobe: optimization for entry only case")
> > introduced a different ftrace_ops for entry-only fprobes.
> > 
> > However, when unregistering an fprobe, the kernel only checks if another
> > fprobe exists at the same address, without checking which type of fprobe
> > it is.
> > If different fprobes are registered at the same address, the same address
> > will be registered in both fgraph_ops and ftrace_ops, but only one of
> > them will be deleted when unregistering. (the one removed first will not
> > be deleted from the ops).
> > 
> > This results in junk entries remaining in either fgraph_ops or ftrace_ops.
> > For example:
> >  =======
> >  cd /sys/kernel/tracing
> > 
> >  # 'Add entry and exit events on the same place'
> >  echo 'f:event1 vfs_read' >> dynamic_events
> >  echo 'f:event2 vfs_read%return' >> dynamic_events
> > 
> >  # 'Enable both of them'
> >  echo 1 > events/fprobes/enable
> >  cat enabled_functions
> > vfs_read (2)            ->arch_ftrace_ops_list_func+0x0/0x210
> > 
> >  # 'Disable and remove exit event'
> >  echo 0 > events/fprobes/event2/enable
> >  echo -:event2 >> dynamic_events
> > 
> >  # 'Disable and remove all events'
> >  echo 0 > events/fprobes/enable
> >  echo > dynamic_events
> > 
> >  # 'Add another event'
> >  echo 'f:event3 vfs_open%return' > dynamic_events
> >  cat dynamic_events
> > f:fprobes/event3 vfs_open%return
> > 
> >  echo 1 > events/fprobes/enable
> >  cat enabled_functions
> > vfs_open (1)            tramp: 0xffffffffa0001000 (ftrace_graph_func+0x0/0x60) ->ftrace_graph_func+0x0/0x60    subops: {ent:fprobe_fgraph_entry+0x0/0x620 ret:fprobe_return+0x0/0x150}
> > vfs_read (1)            tramp: 0xffffffffa0001000 (ftrace_graph_func+0x0/0x60) ->ftrace_graph_func+0x0/0x60    subops: {ent:fprobe_fgraph_entry+0x0/0x620 ret:fprobe_return+0x0/0x150}
> >  =======
> > 
> > As you can see, an entry for the vfs_read remains.
> > 
> > To fix this issue, when unregistering, the kernel should also check if
> > there is the same type of fprobes still exist at the same address, and
> > if not, delete its entry from either fgraph_ops or ftrace_ops.
> > 
> > Fixes: 2c67dc457bc6 ("tracing: fprobe: optimization for entry only case")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > ---
> >  kernel/trace/fprobe.c |   77 +++++++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 62 insertions(+), 15 deletions(-)
> > 
> > diff --git a/kernel/trace/fprobe.c b/kernel/trace/fprobe.c
> > index dcadf1d23b8a..7f75e6e4462c 100644
> > --- a/kernel/trace/fprobe.c
> > +++ b/kernel/trace/fprobe.c
> > @@ -85,11 +85,9 @@ static int insert_fprobe_node(struct fprobe_hlist_node *node)
> >  	return rhltable_insert(&fprobe_ip_table, &node->hlist, fprobe_rht_params);
> >  }
> >  
> > -/* Return true if there are synonims */
> > -static bool delete_fprobe_node(struct fprobe_hlist_node *node)
> > +static void delete_fprobe_node(struct fprobe_hlist_node *node)
> >  {
> >  	lockdep_assert_held(&fprobe_mutex);
> > -	bool ret;
> >  
> >  	/* Avoid double deleting */
> >  	if (READ_ONCE(node->fp) != NULL) {
> > @@ -97,13 +95,6 @@ static bool delete_fprobe_node(struct fprobe_hlist_node *node)
> >  		rhltable_remove(&fprobe_ip_table, &node->hlist,
> >  				fprobe_rht_params);
> >  	}
> > -
> > -	rcu_read_lock();
> > -	ret = !!rhltable_lookup(&fprobe_ip_table, &node->addr,
> > -				fprobe_rht_params);
> > -	rcu_read_unlock();
> > -
> > -	return ret;
> >  }
> >  
> >  /* Check existence of the fprobe */
> > @@ -337,6 +328,32 @@ static bool fprobe_is_ftrace(struct fprobe *fp)
> >  	return !fp->exit_handler;
> >  }
> >  
> > +static bool fprobe_exists_on_hash(unsigned long ip, bool ftrace)
> > +{
> > +	struct rhlist_head *head, *pos;
> > +	struct fprobe_hlist_node *node;
> > +	struct fprobe *fp;
> > +
> > +	guard(rcu)();
> > +	head = rhltable_lookup(&fprobe_ip_table, &ip,
> > +				fprobe_rht_params);
> > +	if (!head)
> > +		return false;
> > +	/* We have to check the same type on the list. */
> > +	rhl_for_each_entry_rcu(node, pos, head, hlist) {
> > +		if (node->addr != ip)
> > +			break;
> > +		fp = READ_ONCE(node->fp);
> > +		if (likely(fp)) {
> > +			if ((!ftrace && fp->exit_handler) ||
> > +			    (ftrace && !fp->exit_handler))
> > +				return true;
> > +		}
> > +	}
> > +
> > +	return false;
> > +}
> > +
> >  #ifdef CONFIG_MODULES
> >  static void fprobe_set_ips(unsigned long *ips, unsigned int cnt, int remove,
> >  			   int reset)
> > @@ -360,6 +377,29 @@ static bool fprobe_is_ftrace(struct fprobe *fp)
> >  	return false;
> >  }
> >  
> > +static bool fprobe_exists_on_hash(unsigned long ip, bool ftrace __maybe_unused)
> > +{
> > +	struct rhlist_head *head, *pos;
> > +	struct fprobe_hlist_node *node;
> > +	struct fprobe *fp;
> > +
> > +	guard(rcu)();
> > +	head = rhltable_lookup(&fprobe_ip_table, &ip,
> > +				fprobe_rht_params);
> > +	if (!head)
> > +		return false;
> > +	/* We only need to check fp is there. */
> > +	rhl_for_each_entry_rcu(node, pos, head, hlist) {
> > +		if (node->addr != ip)
> > +			break;
> > +		fp = READ_ONCE(node->fp);
> > +		if (likely(fp))
> > +			return true;
> > +	}
> > +
> > +	return false;
> > +}
> > +
> >  #ifdef CONFIG_MODULES
> >  static void fprobe_set_ips(unsigned long *ips, unsigned int cnt, int remove,
> >  			   int reset)
> > @@ -574,15 +614,20 @@ static int fprobe_addr_list_add(struct fprobe_addr_list *alist, unsigned long ad
> >  static void fprobe_remove_node_in_module(struct module *mod, struct fprobe_hlist_node *node,
> >  					 struct fprobe_addr_list *alist)
> >  {
> > +	lockdep_assert_in_rcu_read_lock();
> > +
> >  	if (!within_module(node->addr, mod))
> >  		return;
> > -	if (delete_fprobe_node(node))
> > -		return;
> > +
> > +	delete_fprobe_node(node);
> >  	/*
> > -	 * If failed to update alist, just continue to update hlist.
> > +	 * Ignore failure of updating alist, but continue to update hlist.
> >  	 * Therefore, at list user handler will not hit anymore.
> > +	 * And don't care the type here, because all fprobes on the same
> > +	 * address must be removed eventually.
> >  	 */
> > -	fprobe_addr_list_add(alist, node->addr);
> > +	if (!rhltable_lookup(&fprobe_ip_table, &node->addr, fprobe_rht_params))
> > +		fprobe_addr_list_add(alist, node->addr);
> >  }
> >  
> >  /* Handle module unloading to manage fprobe_ip_table. */
> > @@ -943,7 +988,9 @@ int unregister_fprobe(struct fprobe *fp)
> >  	/* Remove non-synonim ips from table and hash */
> >  	count = 0;
> >  	for (i = 0; i < hlist_array->size; i++) {
> > -		if (!delete_fprobe_node(&hlist_array->array[i]))
> > +		delete_fprobe_node(&hlist_array->array[i]);
> > +		if (!fprobe_exists_on_hash(hlist_array->array[i].addr,
> > +					   fprobe_is_ftrace(fp)))
> >  			addrs[count++] = hlist_array->array[i].addr;
> >  	}
> >  	del_fprobe_hash(fp);
> > 
> 
> 
> -- 
> Masami Hiramatsu (Google) <mhiramat@kernel.org>


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>