Re: [PATCH v2 1/2] tracing/fprobe: Avoid kcalloc() in rcu_read_lock section

[Google]

On Thu, 09 Apr 2026 20:05:13 +0800
Menglong Dong <menglong.dong@xxxxxxxxx> wrote:

> On 2026/4/9 18:35 Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx> write:
> > From: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
> > 
> > fprobe_remove_node_in_module() is called under RCU read locked, but
> > this invokes kcalloc() if there are more than 8 fprobes installed
> > on the module. Sashiko warns it because kcalloc() can sleep [1].
> > 
> >  [1] https://sashiko.dev/#/patchset/177552432201.853249.5125045538812833325.stgit%40mhiramat.tok.corp.google.com
> > 
> > To fix this issue, expand the batch size to 128 and do not expand
> > the fprobe_addr_list, but just cancel walking on fprobe_ip_table,
> > update fgraph/ftrace_ops and retry the loop again.
> > 
> > Fixes: 0de4c70d04a4 ("tracing: fprobe: use rhltable for fprobe_ip_table")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
> > ---
> >  kernel/trace/fprobe.c |   53 ++++++++++++++++++-------------------------------
> >  1 file changed, 19 insertions(+), 34 deletions(-)
> > 
> > diff --git a/kernel/trace/fprobe.c b/kernel/trace/fprobe.c
> > index 56d145017902..058cf6ef7ebb 100644
> > --- a/kernel/trace/fprobe.c
> > +++ b/kernel/trace/fprobe.c
> > @@ -536,7 +536,7 @@ static void fprobe_graph_remove_ips(unsigned long *addrs, int num)
> 
> Hi, Masami. Thanks for the fixes. Overall, the whole series
> LGTM.
> 
> Some nits below.
> 
> >  
> >  #ifdef CONFIG_MODULES
> >  
> [...]
> >  				  unsigned long val, void *data)
> > @@ -591,6 +567,7 @@ static int fprobe_module_callback(struct notifier_block *nb,
> >  	struct fprobe_hlist_node *node;
> >  	struct rhashtable_iter iter;
> >  	struct module *mod = data;
> > +	bool retry;
> >  
> >  	if (val != MODULE_STATE_GOING)
> >  		return NOTIFY_DONE;
> > @@ -600,13 +577,19 @@ static int fprobe_module_callback(struct notifier_block *nb,
> >  	if (!alist.addrs)
> >  		return NOTIFY_DONE;
> 
> The "retry" confuse me a little. How about we use "again" and "more"
> here:
> 
> +again:
> +	more = false;


OK. And Sashiko pointed out that we can retry right after calling
rhltable_walk_enter(), and it seems true according to 
https://lwn.net/Articles/751374/

We can seep inside rhltable_walk_enter()/exit() but not inside
rhashtable_walk_start()/end().

So let me update it.

> 
> >  
> > +retry:
> > +	retry = false;
> > +	alist.index = 0;
> >  	mutex_lock(&fprobe_mutex);
> >  	rhltable_walk_enter(&fprobe_ip_table, &iter);
> >  	do {
> >  		rhashtable_walk_start(&iter);
> >  
> >  		while ((node = rhashtable_walk_next(&iter)) && !IS_ERR(node))
> > -			fprobe_remove_node_in_module(mod, node, &alist);
> > +			if (fprobe_remove_node_in_module(mod, node, &alist) < 0) {
> > +				retry = true;
> > +				break;
> > +			}
> 
> Wrap the code within the "while" with {}?

OK.

Thank you!

> 
> Thanks!
> Menglong Dong
> 
> >  
> >  		rhashtable_walk_stop(&iter);
> >  	} while (node == ERR_PTR(-EAGAIN));
> > @@ -615,6 +598,8 @@ static int fprobe_module_callback(struct notifier_block *nb,
> >  	if (alist.index > 0)
> >  		fprobe_set_ips(alist.addrs, alist.index, 1, 0);
> >  	mutex_unlock(&fprobe_mutex);
> > +	if (retry)
> > +		goto retry;
> >  
> >  	kfree(alist.addrs);
> >  
> > 
> > 
> > 
> 
> 
> 
> 


-- 
Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>