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

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

[Tzvetomir Stoyanov (VMware)]

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.

 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);
+			}
+		}
 		if (file)
 			trace_probe_remove_file(tp, file);
 		else
-- 
2.41.0

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

[Steven Rostedt]

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;
			}
		}

Thoughts?

-- Steve



> +		}
>  		if (file)
>  			trace_probe_remove_file(tp, file);
>  		else

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

[Masami Hiramatsu]

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>

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

[Tzvetomir Stoyanov]

On Sun, Jul 2, 2023 at 5:50 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> 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.
>

Ok, I'll send v3 with the counter, although I think it is a bit
overengineering - that optimization is in code that is unlikely to be
executed.

> Thank you,
>
>
> >
> > Thoughts?
> >
> > -- Steve
> >
> >
> >
> > > +           }
> > >             if (file)
> > >                     trace_probe_remove_file(tp, file);
> > >             else
> >
>
>
> --
> Masami Hiramatsu (Google) <mhiramat@kernel.org>



-- 
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center

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

[Steven Rostedt]

On Mon, 3 Jul 2023 06:47:12 +0300
Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote:

> Ok, I'll send v3 with the counter, although I think it is a bit
> overengineering - that optimization is in code that is unlikely to be
> executed.

It's not really over-engineering. We have this type of logic all over the
kernel. When rolling back something, you really only want to rollback what
you did, and not more. It prevents future bugs and makes things a bit more
robust.

I'll go pick up v3 now.

Thanks Tzvetomir!

-- Steve

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

[Steven Rostedt]

On Wed, 5 Jul 2023 11:03:02 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Mon, 3 Jul 2023 06:47:12 +0300
> Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote:
> 
> > Ok, I'll send v3 with the counter, although I think it is a bit
> > overengineering - that optimization is in code that is unlikely to be
> > executed.  
> 
> It's not really over-engineering. We have this type of logic all over the
> kernel. When rolling back something, you really only want to rollback what
> you did, and not more. It prevents future bugs and makes things a bit more
> robust.
> 
> I'll go pick up v3 now.
> 

Masami, I see you delegated this patch to yourself. If you have something
you are working on to send to Linus soon, I'll let you take it.

Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>

-- Steve