linux-kernel - Re: [PATCH] kernel/trace: Fix cleanup logic of enable_trace

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 30 Jun 2023 03:33:30 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Tzvetomir Stoyanov <tz.stoyanov@...il.com>
Cc:     mhiramat@...nel.org, dan.carpenter@...aro.org,
        linux-trace-devel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] kernel/trace: Fix cleanup logic of enable_trace_eprobe

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

> On Wed, Jun 28, 2023 at 3:44 PM Steven Rostedt <rostedt@...dmis.org> wrote:
> >
> > On Wed, 28 Jun 2023 15:18:11 +0300
> > "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@...il.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  
> >  
> 
>