linux-kernel - Re: [PATCH] tracepoints: prevents null probe from being added

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] [day] [month] [year] [list]
Message-ID: <CA+KhAHbq31Txj2C-XjTNfyUfd-g8Jy83q89KdGWAvw0NVKpzYQ@mail.gmail.com>
Date:	Thu, 21 Mar 2013 13:25:03 +0900
From:	Keun-O Park <kpark3469@...il.com>
To:	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
Cc:	Steven Rostedt <rostedt@...dmis.org>, keun-o.park@...driver.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] tracepoints: prevents null probe from being added

On Thu, Mar 21, 2013 at 12:33 PM, Mathieu Desnoyers
<mathieu.desnoyers@...icios.com> wrote:
> * Keun-O Park (kpark3469@...il.com) wrote:
>> On Thu, Mar 21, 2013 at 11:45 AM, Mathieu Desnoyers
>> <mathieu.desnoyers@...icios.com> wrote:
>> > * Keun-O Park (kpark3469@...il.com) wrote:
>> >> On Thu, Mar 21, 2013 at 8:01 AM, Steven Rostedt <rostedt@...dmis.org> wrote:
>> >> > On Wed, 2013-03-20 at 14:01 -0400, Mathieu Desnoyers wrote:
>> >> >> * Steven Rostedt (rostedt@...dmis.org) wrote:
>> >> >> > On Wed, 2013-03-20 at 12:18 +0900, kpark3469@...il.com wrote:
>> >> >> > > From: Sahara <keun-o.park@...driver.com>
>> >> >> > >
>> >> >> > > Somehow tracepoint_entry_add/remove_probe functions allow a null probe
>> >> >> > > function.
>> >> >> >
>> >> >> > You actually hit this in practice, or is this just something that you
>> >> >> > observe from code review?
>> >> >> >
>> >> >> > >  Especially on getting a null probe in remove function, it seems
>> >> >> > > to be used to remove all probe functions in the entry.
>> >> >> >
>> >> >> > Hmm, that actually sounds like a feature.
>> >> >>
>> >> >> Yep. It's been a long time since I wrote this code, but the removal code
>> >> >> seems to use NULL probe pointer to remove all probes for a given
>> >> >> tracepoint.
>> >> >>
>> >> >> I'd be tempted to just validate non-NULL probe within
>> >> >> tracepoint_entry_add_probe() and let other sites as is, just in case
>> >> >> anyone would be using this feature.
>> >> >>
>> >> >> I cannot say that I have personally used this "remove all" feature much
>> >> >> though.
>> >> >>
>> >> >
>> >> > I agree. I don't see anything wrong in leaving the null probe feature in
>> >> > the removal code. But updating the add code looks like a proper change.
>> >> >
>> >> > -- Steve
>> >> >
>> >> >
>> >>
>> >> Hello Steve & Mathieu,
>> >> If we want to leave the null probe feature enabled, I think it would
>> >> be better modifying the code like the following for code efficiency.
>> >>
>> >> @@ -112,7 +112,8 @@ tracepoint_entry_add_probe(struct tracepoint_entry *entry,
>> >>         int nr_probes = 0;
>> >>         struct tracepoint_func *old, *new;
>> >>
>> >> -       WARN_ON(!probe);
>> >> +       if (WARN_ON(!probe))
>> >> +               return ERR_PTR(-EINVAL);
>> >>
>> >>         debug_print_probes(entry);
>> >>         old = entry->funcs;
>> >> @@ -152,14 +153,15 @@ tracepoint_entry_remove_probe(struct tracepoint_entry *ent
>> >>
>> >>         debug_print_probes(entry);
>> >>         /* (N -> M), (N > 1, M >= 0) probes */
>> >> -       for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
>> >> -               if (!probe ||
>> >> -                   (old[nr_probes].func == probe &&
>> >> -                    old[nr_probes].data == data))
>> >> -                       nr_del++;
>> >> +       if (probe) {
>> >> +               for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
>> >> +                       if (old[nr_probes].func == probe &&
>> >> +                            old[nr_probes].data == data)
>> >> +                               nr_del++;
>> >> +               }
>> >>         }
>> >>
>> >> -       if (nr_probes - nr_del == 0) {
>> >> +       if (!probe || nr_probes - nr_del == 0) {
>> >
>> > We might want to do:
>> >
>> > if (probe) {
>> >   ...
>> > } else {
>> >   nr_del = nr_probes;
>> > }
>> >
>> > if (nr_probes - nr_del == 0) {
>> >    ...
>> > }
>>
>> This code has a problem.
>> nr_probes is initialized as zero.
>
> yes,
>
>> And, in order to get correct count of probes,
>> we need to go through the for-loop even though probe is null.
>> So with above code, nr_del will be zero. Anyhow, the code will fall
>> through if-clause(nr_probes-nr_del==0).
>> It looks odd to me.
>
> Ah, I see what you mean: the nr_del = nr_probes assignment is useless,
> because both nr_probes and nr_del are equal to 0. So we could go for:
>
>         if (probe) {
>                 for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
>                         if (old[nr_probes].func == probe &&
>                              old[nr_probes].data == data)
>                                 nr_del++;
>                 }
>         }
>
>         if (nr_probes - nr_del == 0) {
>                 ...
>         } else {
>                 ...
>         }
>
> Does it look better ?
>
> Thanks,
>
> Mathieu

Yes, it does, only if you don't think this code is hard to follow. ;-)
Thanks.

-- Kpark
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/