2020-10-09 19:52:21

by Tom Zanussi

[permalink] [raw]
Subject: [PATCH 0/5] tracing: Synthetic event dynamic string fixes

These patches provide fixes for the problems observed by Masami in the
new synthetic event dynamic string patchset.

The first patch (tracing: Don't show dynamic string internals in
synthetic event description) removes the __data_loc from the event
description but leaves it in the format.

The patch (tracing: Add synthetic event error logging) addresses the
lack of error messages when parse errors occur.

The remaining three patches address the other problems Masami noted
which result from allowing illegal characters in synthetic event and
field names when defining an event. The is_good_name() function is
used to check that's not possible for the probe events, but should
also be used for the synthetic events as well.

(tracing: Move is_good_name() from trace_probe.h to trace.h) makes
that function available to other trace subsystems by putting it in
trace.h. (tracing: Check that the synthetic event and field names are
legal) applies it to the synthetic events, and (selftests/ftrace:
Change synthetic event name for inter-event-combined test) changes a
testcase that now fails because it uses an illegal name.

The following changes since commit 848183553e431e6e9c2ea2f72421a7a1bbc6532e:

tracing: Fix synthetic print fmt check for use of __get_str() (2020-10-08 15:29:07 -0400)

are available in the Git repository at:

git://git.kernel.org/pub/scm/linux/kernel/git/zanussi/linux-trace.git ftrace/dynstring-fixes-v1

Tom Zanussi (5):
tracing: Don't show dynamic string internals in synthetic event
description
tracing: Move is_good_name() from trace_probe.h to trace.h
tracing: Check that the synthetic event and field names are legal
tracing: Add synthetic event error logging
selftests/ftrace: Change synthetic event name for inter-event-combined
test

kernel/trace/trace.h | 13 ++
kernel/trace/trace_events_synth.c | 133 +++++++++++++++++-
kernel/trace/trace_probe.h | 13 --
.../trigger-inter-event-combined-hist.tc | 8 +-
4 files changed, 147 insertions(+), 20 deletions(-)

--
2.17.1


2020-10-09 19:52:25

by Tom Zanussi

[permalink] [raw]
Subject: [PATCH 2/5] tracing: Move is_good_name() from trace_probe.h to trace.h

is_good_name() is useful for other trace infrastructure, such as
synthetic events, so make it available via trace.h.

Signed-off-by: Tom Zanussi <[email protected]>
---
kernel/trace/trace.h | 13 +++++++++++++
kernel/trace/trace_probe.h | 13 -------------
2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 5b0e797cacdd..a94852838491 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -19,6 +19,7 @@
#include <linux/glob.h>
#include <linux/irq_work.h>
#include <linux/workqueue.h>
+#include <linux/ctype.h>

#ifdef CONFIG_FTRACE_SYSCALLS
#include <asm/unistd.h> /* For NR_SYSCALLS */
@@ -2090,4 +2091,16 @@ static __always_inline void trace_iterator_reset(struct trace_iterator *iter)
iter->pos = -1;
}

+/* Check the name is good for event/group/fields */
+static inline bool is_good_name(const char *name)
+{
+ if (!isalpha(*name) && *name != '_')
+ return false;
+ while (*++name != '\0') {
+ if (!isalpha(*name) && !isdigit(*name) && *name != '_')
+ return false;
+ }
+ return true;
+}
+
#endif /* _LINUX_KERNEL_TRACE_H */
diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
index 04d00987da69..2f703a20c724 100644
--- a/kernel/trace/trace_probe.h
+++ b/kernel/trace/trace_probe.h
@@ -16,7 +16,6 @@
#include <linux/tracefs.h>
#include <linux/types.h>
#include <linux/string.h>
-#include <linux/ctype.h>
#include <linux/ptrace.h>
#include <linux/perf_event.h>
#include <linux/kprobes.h>
@@ -348,18 +347,6 @@ bool trace_probe_match_command_args(struct trace_probe *tp,
#define trace_probe_for_each_link_rcu(pos, tp) \
list_for_each_entry_rcu(pos, &(tp)->event->files, list)

-/* Check the name is good for event/group/fields */
-static inline bool is_good_name(const char *name)
-{
- if (!isalpha(*name) && *name != '_')
- return false;
- while (*++name != '\0') {
- if (!isalpha(*name) && !isdigit(*name) && *name != '_')
- return false;
- }
- return true;
-}
-
#define TPARG_FL_RETURN BIT(0)
#define TPARG_FL_KERNEL BIT(1)
#define TPARG_FL_FENTRY BIT(2)
--
2.17.1

2020-10-10 04:30:25

by Tom Zanussi

[permalink] [raw]
Subject: Re: [PATCH 0/5] tracing: Synthetic event dynamic string fixes

Hi Axel,

On Fri, 2020-10-09 at 13:35 -0700, Axel Rasmussen wrote:
> On Fri, Oct 9, 2020 at 8:17 AM Tom Zanussi <[email protected]>
> wrote:
> >
> > These patches provide fixes for the problems observed by Masami in
> > the
> > new synthetic event dynamic string patchset.
> >
> > The first patch (tracing: Don't show dynamic string internals in
> > synthetic event description) removes the __data_loc from the event
> > description but leaves it in the format.
> >
> > The patch (tracing: Add synthetic event error logging) addresses
> > the
> > lack of error messages when parse errors occur.
> >
> > The remaining three patches address the other problems Masami noted
> > which result from allowing illegal characters in synthetic event
> > and
> > field names when defining an event. The is_good_name() function is
> > used to check that's not possible for the probe events, but should
> > also be used for the synthetic events as well.
> >
> > (tracing: Move is_good_name() from trace_probe.h to trace.h) makes
> > that function available to other trace subsystems by putting it in
> > trace.h. (tracing: Check that the synthetic event and field names
> > are
> > legal) applies it to the synthetic events, and (selftests/ftrace:
> > Change synthetic event name for inter-event-combined test) changes
> > a
> > testcase that now fails because it uses an illegal name.
> >
> > The following changes since commit
> > 848183553e431e6e9c2ea2f72421a7a1bbc6532e:
> >
> > tracing: Fix synthetic print fmt check for use of __get_str()
> > (2020-10-08 15:29:07 -0400)
> >
> > are available in the Git repository at:
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/zanussi/linux-
> > trace.git ftrace/dynstring-fixes-v1
>
> I applied this series, and then my mmap_lock tracepoints series, onto
> v5.9-rc8. I played with the edge cases Masami raised in the other
> thread, and I also tried constructing a synthetic event as we
> discussed on the thread about my series.
>
> As far as I can see, this addresses the edge cases Masami pointed
> out,
> and it all seems to work as intended. It works fine with the kind of
> synthetic event I'm hoping to define for my particular use case.
>
> So, for what it's worth:
>
> Tested-By: Axel Rasmussen <[email protected]>
>

Great, thanks for (re-)testing!

Tom

> >
> > Tom Zanussi (5):
> > tracing: Don't show dynamic string internals in synthetic event
> > description
> > tracing: Move is_good_name() from trace_probe.h to trace.h
> > tracing: Check that the synthetic event and field names are legal
> > tracing: Add synthetic event error logging
> > selftests/ftrace: Change synthetic event name for inter-event-
> > combined
> > test
> >
> > kernel/trace/trace.h | 13 ++
> > kernel/trace/trace_events_synth.c | 133
> > +++++++++++++++++-
> > kernel/trace/trace_probe.h | 13 --
> > .../trigger-inter-event-combined-hist.tc | 8 +-
> > 4 files changed, 147 insertions(+), 20 deletions(-)
> >
> > --
> > 2.17.1
> >

2020-10-10 22:54:29

by Axel Rasmussen

[permalink] [raw]
Subject: Re: [PATCH 0/5] tracing: Synthetic event dynamic string fixes

On Fri, Oct 9, 2020 at 8:17 AM Tom Zanussi <[email protected]> wrote:
>
> These patches provide fixes for the problems observed by Masami in the
> new synthetic event dynamic string patchset.
>
> The first patch (tracing: Don't show dynamic string internals in
> synthetic event description) removes the __data_loc from the event
> description but leaves it in the format.
>
> The patch (tracing: Add synthetic event error logging) addresses the
> lack of error messages when parse errors occur.
>
> The remaining three patches address the other problems Masami noted
> which result from allowing illegal characters in synthetic event and
> field names when defining an event. The is_good_name() function is
> used to check that's not possible for the probe events, but should
> also be used for the synthetic events as well.
>
> (tracing: Move is_good_name() from trace_probe.h to trace.h) makes
> that function available to other trace subsystems by putting it in
> trace.h. (tracing: Check that the synthetic event and field names are
> legal) applies it to the synthetic events, and (selftests/ftrace:
> Change synthetic event name for inter-event-combined test) changes a
> testcase that now fails because it uses an illegal name.
>
> The following changes since commit 848183553e431e6e9c2ea2f72421a7a1bbc6532e:
>
> tracing: Fix synthetic print fmt check for use of __get_str() (2020-10-08 15:29:07 -0400)
>
> are available in the Git repository at:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/zanussi/linux-trace.git ftrace/dynstring-fixes-v1

I applied this series, and then my mmap_lock tracepoints series, onto
v5.9-rc8. I played with the edge cases Masami raised in the other
thread, and I also tried constructing a synthetic event as we
discussed on the thread about my series.

As far as I can see, this addresses the edge cases Masami pointed out,
and it all seems to work as intended. It works fine with the kind of
synthetic event I'm hoping to define for my particular use case.

So, for what it's worth:

Tested-By: Axel Rasmussen <[email protected]>

>
> Tom Zanussi (5):
> tracing: Don't show dynamic string internals in synthetic event
> description
> tracing: Move is_good_name() from trace_probe.h to trace.h
> tracing: Check that the synthetic event and field names are legal
> tracing: Add synthetic event error logging
> selftests/ftrace: Change synthetic event name for inter-event-combined
> test
>
> kernel/trace/trace.h | 13 ++
> kernel/trace/trace_events_synth.c | 133 +++++++++++++++++-
> kernel/trace/trace_probe.h | 13 --
> .../trigger-inter-event-combined-hist.tc | 8 +-
> 4 files changed, 147 insertions(+), 20 deletions(-)
>
> --
> 2.17.1
>

2020-10-10 23:11:09

by Masami Hiramatsu

[permalink] [raw]
Subject: Re: [PATCH 2/5] tracing: Move is_good_name() from trace_probe.h to trace.h

On Fri, 9 Oct 2020 10:17:08 -0500
Tom Zanussi <[email protected]> wrote:

> is_good_name() is useful for other trace infrastructure, such as
> synthetic events, so make it available via trace.h.
>

This looks good to me.

Acked-by: Masami Hiramatsu <[email protected]>

Thanks!

> Signed-off-by: Tom Zanussi <[email protected]>
> ---
> kernel/trace/trace.h | 13 +++++++++++++
> kernel/trace/trace_probe.h | 13 -------------
> 2 files changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 5b0e797cacdd..a94852838491 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -19,6 +19,7 @@
> #include <linux/glob.h>
> #include <linux/irq_work.h>
> #include <linux/workqueue.h>
> +#include <linux/ctype.h>
>
> #ifdef CONFIG_FTRACE_SYSCALLS
> #include <asm/unistd.h> /* For NR_SYSCALLS */
> @@ -2090,4 +2091,16 @@ static __always_inline void trace_iterator_reset(struct trace_iterator *iter)
> iter->pos = -1;
> }
>
> +/* Check the name is good for event/group/fields */
> +static inline bool is_good_name(const char *name)
> +{
> + if (!isalpha(*name) && *name != '_')
> + return false;
> + while (*++name != '\0') {
> + if (!isalpha(*name) && !isdigit(*name) && *name != '_')
> + return false;
> + }
> + return true;
> +}
> +
> #endif /* _LINUX_KERNEL_TRACE_H */
> diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
> index 04d00987da69..2f703a20c724 100644
> --- a/kernel/trace/trace_probe.h
> +++ b/kernel/trace/trace_probe.h
> @@ -16,7 +16,6 @@
> #include <linux/tracefs.h>
> #include <linux/types.h>
> #include <linux/string.h>
> -#include <linux/ctype.h>
> #include <linux/ptrace.h>
> #include <linux/perf_event.h>
> #include <linux/kprobes.h>
> @@ -348,18 +347,6 @@ bool trace_probe_match_command_args(struct trace_probe *tp,
> #define trace_probe_for_each_link_rcu(pos, tp) \
> list_for_each_entry_rcu(pos, &(tp)->event->files, list)
>
> -/* Check the name is good for event/group/fields */
> -static inline bool is_good_name(const char *name)
> -{
> - if (!isalpha(*name) && *name != '_')
> - return false;
> - while (*++name != '\0') {
> - if (!isalpha(*name) && !isdigit(*name) && *name != '_')
> - return false;
> - }
> - return true;
> -}
> -
> #define TPARG_FL_RETURN BIT(0)
> #define TPARG_FL_KERNEL BIT(1)
> #define TPARG_FL_FENTRY BIT(2)
> --
> 2.17.1
>


--
Masami Hiramatsu <[email protected]>

2020-10-13 05:51:00

by Steven Rostedt

[permalink] [raw]
Subject: Re: [PATCH 0/5] tracing: Synthetic event dynamic string fixes

On Fri, 9 Oct 2020 10:17:06 -0500
Tom Zanussi <[email protected]> wrote:

> These patches provide fixes for the problems observed by Masami in the
> new synthetic event dynamic string patchset.
>
> The first patch (tracing: Don't show dynamic string internals in
> synthetic event description) removes the __data_loc from the event
> description but leaves it in the format.
>
> The patch (tracing: Add synthetic event error logging) addresses the
> lack of error messages when parse errors occur.
>
> The remaining three patches address the other problems Masami noted
> which result from allowing illegal characters in synthetic event and
> field names when defining an event. The is_good_name() function is
> used to check that's not possible for the probe events, but should
> also be used for the synthetic events as well.
>
> (tracing: Move is_good_name() from trace_probe.h to trace.h) makes
> that function available to other trace subsystems by putting it in
> trace.h. (tracing: Check that the synthetic event and field names are
> legal) applies it to the synthetic events, and (selftests/ftrace:
> Change synthetic event name for inter-event-combined test) changes a
> testcase that now fails because it uses an illegal name.
>


Hi Tom,

Would you be able to address Masami's concerns on patches 1 and 4?

-- Steve

2020-10-13 06:37:51

by Tom Zanussi

[permalink] [raw]
Subject: Re: [PATCH 0/5] tracing: Synthetic event dynamic string fixes

Hi Steve,

On Mon, 2020-10-12 at 11:13 -0400, Steven Rostedt wrote:
> On Fri, 9 Oct 2020 10:17:06 -0500
> Tom Zanussi <[email protected]> wrote:
>
> > These patches provide fixes for the problems observed by Masami in
> > the
> > new synthetic event dynamic string patchset.
> >
> > The first patch (tracing: Don't show dynamic string internals in
> > synthetic event description) removes the __data_loc from the event
> > description but leaves it in the format.
> >
> > The patch (tracing: Add synthetic event error logging) addresses
> > the
> > lack of error messages when parse errors occur.
> >
> > The remaining three patches address the other problems Masami noted
> > which result from allowing illegal characters in synthetic event
> > and
> > field names when defining an event. The is_good_name() function is
> > used to check that's not possible for the probe events, but should
> > also be used for the synthetic events as well.
> >
> > (tracing: Move is_good_name() from trace_probe.h to trace.h) makes
> > that function available to other trace subsystems by putting it in
> > trace.h. (tracing: Check that the synthetic event and field names
> > are
> > legal) applies it to the synthetic events, and (selftests/ftrace:
> > Change synthetic event name for inter-event-combined test) changes
> > a
> > testcase that now fails because it uses an illegal name.
> >
>
>
> Hi Tom,
>
> Would you be able to address Masami's concerns on patches 1 and 4?

Yes, I'll submit a v2 fixing those soon (today).

Thanks,

Tom

>
> -- Steve