Re: [PATCH v2] kernel/trace: Fix cleanup logic of enable_trace_eprobe

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

On Sat, 1 Jul 2023 09:02:54 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 30 Jun 2023 15:16:27 +0300
> "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:
> 
> 
> Hi Tzvetomir,
> 
> FYI, linux-trace-devel is for the tracing user space code, please Cc to
> linux-trace-kernel for kernel patches. That makes it fall into the
> proper patchwork.
> 
> I noticed this because I couldn't find your patch in:
> 
>   https://patchwork.kernel.org/project/linux-trace-kernel/list/
> 
> Also, the Subject should just start with "tracing:".
> 
> > The enable_trace_eprobe() function enables all event probes, attached
> > to given trace probe. If an error occurs in enabling one of the event
> > probes, all others should be roll backed. There is a bug in that roll
> > back logic - instead of all event probes, only the failed one is
> > disabled.
> > 
> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > Fixes: 7491e2c44278 ("tracing: Add a probe that attaches to trace events")
> > Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> > ---
> > v2: Added one-time warning, as suggested by Steven Rostedt.
> 
> It's always a nice touch (optional, but something I always do) to
> add a link to the previous version:
> 
>  Changes since v2: https://lore.kernel.org/all/20230628121811.338655-1-tz.stoyanov@gmail.com/
>      - Added one-time warning (Steven Rostedt)
> 
> > 
> >  kernel/trace/trace_eprobe.c | 14 ++++++++++++--
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> > 
> > diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
> > index 67e854979d53..6629fa217c99 100644
> > --- a/kernel/trace/trace_eprobe.c
> > +++ b/kernel/trace/trace_eprobe.c
> > @@ -702,8 +702,18 @@ static int enable_trace_eprobe(struct trace_event_call *call,
> >  
> >  	if (ret) {
> >  		/* Failed to enable one of them. Roll back all */
> > -		if (enabled)
> > -			disable_eprobe(ep, file->tr);
> > +		if (enabled) {
> > +			/*
> > +			 * It's a bug if one failed for something other than memory
> > +			 * not being available but another eprobe succeeded.
> > +			 */
> > +			WARN_ON_ONCE(ret != -ENOMEM);
> > +
> > +			list_for_each_entry(pos, trace_probe_probe_list(tp), list) {
> > +				ep = container_of(pos, struct trace_eprobe, tp);
> > +				disable_eprobe(ep, file->tr);
> > +			}
> 
> I think we may need the counter again ;-)
> 
> But for another reason. We only want to call disable for what we
> enabled, to avoid any unforeseen side effects.
> 
> 
> 	cnt = 0;
>         list_for_each_entry(pos, trace_probe_probe_list(tp), list) {
>                 ep = container_of(pos, struct trace_eprobe, tp);
>                 ret = enable_eprobe(ep, file);
>                 if (ret)
>                         break;
>                 enabled = true;
> 		cnt++;
>         }
> 
>         if (ret) {
>                 /* Failed to enable one of them. Roll back all */
>                 if (enabled) {
> 			list_for_each_entry(pos, trace_probe_probe_list(tp), list) {
> 				ep = container_of(pos, struct trace_eprobe, tp);
> 				disable_eprobe(ep, file->tr);
> 				if (!--cnt)
> 					break;
> 			}
> 		}

+1. It seems that enable_eprobe() doesn't change ep, we need a counter to
count how many eprobes are enabled in the first loop for roll-back the
already enabled eprobes in the 2nd loop.

Thank you,


> 
> Thoughts?
> 
> -- Steve
> 
> 
> 
> > +		}
> >  		if (file)
> >  			trace_probe_remove_file(tp, file);
> >  		else
> 


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