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

[PATCH] 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>
---
 kernel/trace/trace_eprobe.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
index 67e854979d53..ba9a28bc773f 100644
--- a/kernel/trace/trace_eprobe.c
+++ b/kernel/trace/trace_eprobe.c
@@ -702,8 +702,12 @@ 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) {
+			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] kernel/trace: Fix cleanup logic of enable_trace_eprobe

[Steven Rostedt]

On Wed, 28 Jun 2023 15:18:11 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> 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>
> ---
>  kernel/trace/trace_eprobe.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
> index 67e854979d53..ba9a28bc773f 100644
> --- a/kernel/trace/trace_eprobe.c
> +++ b/kernel/trace/trace_eprobe.c
> @@ -702,8 +702,12 @@ 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) {

If one was enabled and the second one failed, that should only happen
if there's a bug in the kernel (unless the failure was due to a memory
problem).

I wonder if we should add:

			int cnt = 0;

> +			list_for_each_entry(pos, trace_probe_probe_list(tp), list) {

				/*
				 * It's a bug if one failed for something other than memory
				 * not being available but another eprobe succeeded.
				 */
				WARN_ON_ONCE(cnt++ && ret != -ENOMEM);

-- Steve


> +				ep = container_of(pos, struct trace_eprobe, tp);
> +				disable_eprobe(ep, file->tr);
> +			}
> +		}
>  		if (file)
>  			trace_probe_remove_file(tp, file);
>  		else

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

[Masami Hiramatsu]

On Wed, 28 Jun 2023 15:18:11 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

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

Thanks, this looks good to me.

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

> 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>
> ---
>  kernel/trace/trace_eprobe.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
> index 67e854979d53..ba9a28bc773f 100644
> --- a/kernel/trace/trace_eprobe.c
> +++ b/kernel/trace/trace_eprobe.c
> @@ -702,8 +702,12 @@ 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) {
> +			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
> 


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

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

[Tzvetomir Stoyanov]

On Wed, Jun 28, 2023 at 3:44 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Wed, 28 Jun 2023 15:18:11 +0300
> "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:
>
> > 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>
> > ---
> >  kernel/trace/trace_eprobe.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
> > index 67e854979d53..ba9a28bc773f 100644
> > --- a/kernel/trace/trace_eprobe.c
> > +++ b/kernel/trace/trace_eprobe.c
> > @@ -702,8 +702,12 @@ 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) {
>
> If one was enabled and the second one failed, that should only happen
> if there's a bug in the kernel (unless the failure was due to a memory
> problem).
>
> I wonder if we should add:
>
>                         int cnt = 0;
>
> > +                     list_for_each_entry(pos, trace_probe_probe_list(tp), list) {
>
>                                 /*
>                                  * It's a bug if one failed for something other than memory
>                                  * not being available but another eprobe succeeded.
>                                  */
>                                 WARN_ON_ONCE(cnt++ && ret != -ENOMEM);

That makes sense, I can send v2 with it. What is the idea of this cnt
counter, why not just:
                                 WARN_ON_ONCE(ret != -ENOMEM);
outside of the loop? If enabled is true and ret is not ENOMEM, the bug
is already there.

>
> -- Steve
>
>
> > +                             ep = container_of(pos, struct trace_eprobe, tp);
> > +                             disable_eprobe(ep, file->tr);
> > +                     }
> > +             }
> >               if (file)
> >                       trace_probe_remove_file(tp, file);
> >               else
>


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

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

[Steven Rostedt]

On Thu, 29 Jun 2023 17:31:24 +0300
Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote:

> On Wed, Jun 28, 2023 at 3:44 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > On Wed, 28 Jun 2023 15:18:11 +0300
> > "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:
>> > > --- a/kernel/trace/trace_eprobe.c
> > > +++ b/kernel/trace/trace_eprobe.c
> > > @@ -702,8 +702,12 @@ 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) {  
> >
> > If one was enabled and the second one failed, that should only happen
> > if there's a bug in the kernel (unless the failure was due to a memory
> > problem).
> >
> > I wonder if we should add:
> >
> >                         int cnt = 0;
> >  
> > > +                     list_for_each_entry(pos, trace_probe_probe_list(tp), list) {  
> >
> >                                 /*
> >                                  * It's a bug if one failed for something other than memory
> >                                  * not being available but another eprobe succeeded.
> >                                  */
> >                                 WARN_ON_ONCE(cnt++ && ret != -ENOMEM);  
> 
> That makes sense, I can send v2 with it. What is the idea of this cnt
> counter, why not just:
>                                  WARN_ON_ONCE(ret != -ENOMEM);
> outside of the loop? If enabled is true and ret is not ENOMEM, the bug
> is already there.

Failing for something other than ENOMEM should not cause a warning by
itself. The idea is, if one fails for something other than ENOMEM, they
all should fail with the same error. That is, they all should succeed
or they all should fail.

Actually, the above isn't right. The counter should be in the original
loop, and if one or more succeeds to enable, but another fails due to
some other error, that needs to be looked at, hence the warning.

Does that make sense?

-- Steve

> >
> >  
> > > +                             ep = container_of(pos, struct trace_eprobe, tp);
> > > +                             disable_eprobe(ep, file->tr);
> > > +                     }
> > > +             }
> > >               if (file)
> > >                       trace_probe_remove_file(tp, file);
> > >               else  
> >  
> 
> 

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

[Tzvetomir Stoyanov]

On Fri, Jun 30, 2023 at 10:33 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Thu, 29 Jun 2023 17:31:24 +0300
> Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote:
>
> > On Wed, Jun 28, 2023 at 3:44 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> > >
> > > On Wed, 28 Jun 2023 15:18:11 +0300
> > > "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:
> >> > > --- a/kernel/trace/trace_eprobe.c
> > > > +++ b/kernel/trace/trace_eprobe.c
> > > > @@ -702,8 +702,12 @@ 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) {
> > >
> > > If one was enabled and the second one failed, that should only happen
> > > if there's a bug in the kernel (unless the failure was due to a memory
> > > problem).
> > >
> > > I wonder if we should add:
> > >
> > >                         int cnt = 0;
> > >
> > > > +                     list_for_each_entry(pos, trace_probe_probe_list(tp), list) {
> > >
> > >                                 /*
> > >                                  * It's a bug if one failed for something other than memory
> > >                                  * not being available but another eprobe succeeded.
> > >                                  */
> > >                                 WARN_ON_ONCE(cnt++ && ret != -ENOMEM);
> >
> > That makes sense, I can send v2 with it. What is the idea of this cnt
> > counter, why not just:
> >                                  WARN_ON_ONCE(ret != -ENOMEM);
> > outside of the loop? If enabled is true and ret is not ENOMEM, the bug
> > is already there.
>
> Failing for something other than ENOMEM should not cause a warning by
> itself. The idea is, if one fails for something other than ENOMEM, they
> all should fail with the same error. That is, they all should succeed
> or they all should fail.
>
> Actually, the above isn't right. The counter should be in the original
> loop, and if one or more succeeds to enable, but another fails due to
> some other error, that needs to be looked at, hence the warning.
>
> Does that make sense?

Yes, it makes sense. But the original loop will break on the first
failure.  If there is an error (ret is not 0) and at least one eprobe
was enabled successfully (enabled is true),
the warning should be emitted, only if that error is not ENOMEM:
                                  WARN_ON_ONCE(ret != -ENOMEM);


>
> -- Steve
>
> > >
> > >
> > > > +                             ep = container_of(pos, struct trace_eprobe, tp);
> > > > +                             disable_eprobe(ep, file->tr);
> > > > +                     }
> > > > +             }
> > > >               if (file)
> > > >                       trace_probe_remove_file(tp, file);
> > > >               else
> > >
> >
> >
>


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

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

[Steven Rostedt]

On Fri, 30 Jun 2023 12:57:36 +0300
Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote:

> > Does that make sense?  
> 
> Yes, it makes sense. But the original loop will break on the first
> failure.  If there is an error (ret is not 0) and at least one eprobe
> was enabled successfully (enabled is true),
> the warning should be emitted, only if that error is not ENOMEM:
>                                   WARN_ON_ONCE(ret != -ENOMEM);

Ah, I missed the logic for the "enabled" variable. Yeah, when that is
set it does the same as the cnt variable I mentioned. Thanks for
pointing that out. (I blame still being a bit jetlagged and lack of
sleep for not seeing that ;-)

Yeah, if "enabled" is set, then we can do the WARN_ON_ONCE().

I'll wait for your new patch.

Thanks Tzvetomir!

-- Steve