Re: [PATCH v6 2/5] tracing/fprobe: Remove fprobe from hash in failure path

[Google]

On Wed, 15 Apr 2026 17:47:11 +0800
Menglong Dong <menglong.dong@xxxxxxxxx> wrote:

> On 2026/4/14 17:14 Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx> write:
> > From: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
> > 
> > When register_fprobe_ips() fails, it tries to remove a list of
> > fprobe_hash_node from fprobe_ip_table, but it missed to remove
> > fprobe itself from fprobe_table. Moreover, when removing
> > the fprobe_hash_node which is added to rhltable once, it must
> > use kfree_rcu() after removing from rhltable.
> > 
> > To fix these issues, this reuses unregister_fprobe() internal
> > code to rollback the half-way registered fprobe.
> > 
> > Fixes: 4346ba160409 ("fprobe: Rewrite fprobe on function-graph tracer")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
> > ---
> [...]
> >  
> > +static int unregister_fprobe_nolock(struct fprobe *fp, bool force);
> > +
> >  /**
> >   * register_fprobe_ips() - Register fprobe to ftrace by address.
> >   * @fp: A fprobe data structure to be registered.
> > @@ -847,29 +855,26 @@ int register_fprobe_ips(struct fprobe *fp, unsigned long *addrs, int num)
> >  	if (ret)
> >  		return ret;
> 
> Hi, Masami. The logic of unregister_fprobe_nolock() looks a little
> messy. How about we make the logic here like this:
> 
> for (i = 0; i < hlist_array->size; i++) {
>     // The node->fp is NULL, so it's safe to add the node before
>     // fprobe_ftrace_add_ips(), right?
>     ret = insert_fprobe_node(&hlist_array->array[i], fp);
>     if (ret)
>         goto fallback_err;
> }
> 
> if (fprobe_is_ftrace(fp))
>     ret = fprobe_ftrace_add_ips(addrs, num);
> else
>     ret = fprobe_graph_add_ips(addrs, num);
> if (ret)
>     goto fallback_err;
> 
> add_fprobe_hash(fp);
> for (i = 0; i < hlist_array->size; i++)
>     WRITE_ONCE(hlist_array->array[i].fp, fp);
> 
> return 0;
> 
> fallback_err:
> for (i--; i >= 0; i--)
>     delete_fprobe_node(&hlist_array->array[i]);
> fprobe_fail_cleanup(fp);
> return ret;
> 
> Then, we don't need to change unregister_fprobe_nolock and
> insert_fprobe_node.

Thanks for the idea, but I don't like repeat it.
It is better to do the same thing(unregister) in the same code.
Above seems a bit optimized for fixing a problem.
(Maybe revisit it later for optimization)

Thank you,


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