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

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

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

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>