[PATCH v3 0/5] tracing: More synthetic event error fixes

[PATCH v3 0/5] tracing: More synthetic event error fixes

[Tom Zanussi]

Hi,

This is v3 of the sythetic event error fix patchset.  As suggested by
Masami, I split the 'tracing/dynevent: Delegate parsing to create
function' into two - one containing just the interface changes and the
second with the synthetic event parsing changes the first enabled.

I also replaced a couple argv_split() with strpbrk() as suggested by
Masami, along with removing the no-longer-used consume lines and
another line that tested ECANCELED that was no longer needed.

Also, removed a test case that was no longer needed since the commands
are now stripped of whitespace first.

Thanks, Masami, for the suggestions.

Tom

v2 text:

This is v2 of the previous sythetic event error fix patchset.

This version drops the original ([PATCH 1/4] tracing: Make
trace_*_run_command() more flexible) and (tracing: Use new
trace_run_command() options) patches and replaces them with Masami's
patch (tracing/dynevent: Delegate parsing to create function) [1].
The new version adds in all the synthetic event changes needed to
compile and use the new interface.

A new patch was also added (selftests/ftrace: Add synthetic event
field separators) that fixes more invalid synthetic event syntax I
found while testing.

I also added some more new checks to the synthetic event sytax error
testcase.

As before, I didn't see any problems running the entire ftrace
testsuite or the test modules that also use the things that were
touched here.

[1] https://lore.kernel.org/lkml/20201019001504.70dc3ec608277ed22060d2f7@kernel.org/

Thanks,

Tom


v1 text:

Hi,

This patchset addresses the synthetic event error anomalies reported
by Masami in the last patchset [1].

It turns out that most of the problems boil down to clunky separator
parsing; adding a couple new abilities to trace_run_command() and then
adapting the existing users seemed to me the best way to fix these
things, and also gets rid of some code.

Also, to make things easier for error display, I changed these to
preserve the original command string and pass it through the callback
instead of rebuilding it for error display.

I added some new error strings and removed unused ones as well, and
added a bunch of new test cases to the synthetic parser error test
case.

I didn't see any problems running the entire ftrace testsuite or the
test modules that also use the things that were touched here.

Thanks,

Tom

[1] https://lore.kernel.org/lkml/20201014110636.139df7be275d40a23b523b84@kernel.org/

The following changes since commit b37b1f9a6311469937aae1c039db00e5b75b9fcb:

  tracing, synthetic events: Replace buggy strcat() with seq_buf operations (2020-10-23 19:05:12 -0400)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/zanussi/linux-trace.git ftrace/synth-fixes-v3

Masami Hiramatsu (1):
  tracing/dynevent: Delegate parsing to create function

Tom Zanussi (4):
  tracing: Rework synthetic event command parsing
  tracing: Update synth command errors
  selftests/ftrace: Add synthetic event field separators
  selftests/ftrace: Update synthetic event syntax errors

 kernel/trace/trace.c                          |  23 +-
 kernel/trace/trace.h                          |   3 +-
 kernel/trace/trace_dynevent.c                 |  35 ++-
 kernel/trace/trace_dynevent.h                 |   4 +-
 kernel/trace/trace_events_synth.c             | 235 +++++++++++-------
 kernel/trace/trace_kprobe.c                   |  33 +--
 kernel/trace/trace_probe.c                    |  17 ++
 kernel/trace/trace_probe.h                    |   1 +
 kernel/trace/trace_uprobe.c                   |  17 +-
 .../trigger-inter-event-combined-hist.tc      |   4 +-
 .../trigger-onmatch-action-hist.tc            |   2 +-
 .../trigger-onmatch-onmax-action-hist.tc      |   2 +-
 .../trigger-synthetic_event_syntax_errors.tc  |  35 ++-
 .../inter-event/trigger-trace-action-hist.tc  |   2 +-
 14 files changed, 257 insertions(+), 156 deletions(-)

-- 
2.17.1

[PATCH v3 1/5] tracing/dynevent: Delegate parsing to create function

[Tom Zanussi]

From: Masami Hiramatsu <mhiramat@kernel.org>

Delegate command parsing to each create function so that the
command syntax can be customized.

This requires changes to the kprobe/uprobe/synthetic event handling,
which are also included here.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
[ zanussi@kernel.org: added synthetic event modifications ]
Signed-off-by: Tom Zanussi <zanussi@kernel.org>
---
 kernel/trace/trace.c              | 23 ++----------
 kernel/trace/trace.h              |  3 +-
 kernel/trace/trace_dynevent.c     | 35 +++++++++++-------
 kernel/trace/trace_dynevent.h     |  4 +--
 kernel/trace/trace_events_synth.c | 60 +++++++++++++++++++++++--------
 kernel/trace/trace_kprobe.c       | 33 +++++++++--------
 kernel/trace/trace_probe.c        | 17 +++++++++
 kernel/trace/trace_probe.h        |  1 +
 kernel/trace/trace_uprobe.c       | 17 +++++----
 9 files changed, 120 insertions(+), 73 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 63c97012ed39..277d97220971 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -9367,30 +9367,11 @@ void ftrace_dump(enum ftrace_dump_mode oops_dump_mode)
 }
 EXPORT_SYMBOL_GPL(ftrace_dump);
 
-int trace_run_command(const char *buf, int (*createfn)(int, char **))
-{
-	char **argv;
-	int argc, ret;
-
-	argc = 0;
-	ret = 0;
-	argv = argv_split(GFP_KERNEL, buf, &argc);
-	if (!argv)
-		return -ENOMEM;
-
-	if (argc)
-		ret = createfn(argc, argv);
-
-	argv_free(argv);
-
-	return ret;
-}
-
 #define WRITE_BUFSIZE  4096
 
 ssize_t trace_parse_run_command(struct file *file, const char __user *buffer,
 				size_t count, loff_t *ppos,
-				int (*createfn)(int, char **))
+				int (*createfn)(const char *))
 {
 	char *kbuf, *buf, *tmp;
 	int ret = 0;
@@ -9438,7 +9419,7 @@ ssize_t trace_parse_run_command(struct file *file, const char __user *buffer,
 			if (tmp)
 				*tmp = '\0';
 
-			ret = trace_run_command(buf, createfn);
+			ret = createfn(buf);
 			if (ret)
 				goto out;
 			buf += size;
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 34e0c4d5a6e7..02d7c487a30b 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -1982,10 +1982,9 @@ extern int tracing_set_cpumask(struct trace_array *tr,
 
 #define MAX_EVENT_NAME_LEN	64
 
-extern int trace_run_command(const char *buf, int (*createfn)(int, char**));
 extern ssize_t trace_parse_run_command(struct file *file,
 		const char __user *buffer, size_t count, loff_t *ppos,
-		int (*createfn)(int, char**));
+		int (*createfn)(const char *));
 
 extern unsigned int err_pos(char *cmd, const char *str);
 extern void tracing_log_err(struct trace_array *tr,
diff --git a/kernel/trace/trace_dynevent.c b/kernel/trace/trace_dynevent.c
index 5fa49cfd2bb6..af83bc5447fe 100644
--- a/kernel/trace/trace_dynevent.c
+++ b/kernel/trace/trace_dynevent.c
@@ -31,23 +31,31 @@ int dyn_event_register(struct dyn_event_operations *ops)
 	return 0;
 }
 
-int dyn_event_release(int argc, char **argv, struct dyn_event_operations *type)
+int dyn_event_release(const char *raw_command, struct dyn_event_operations *type)
 {
 	struct dyn_event *pos, *n;
 	char *system = NULL, *event, *p;
-	int ret = -ENOENT;
+	int argc, ret = -ENOENT;
+	char **argv;
+
+	argv = argv_split(GFP_KERNEL, raw_command, &argc);
+	if (!argv)
+		return -ENOMEM;
 
 	if (argv[0][0] == '-') {
-		if (argv[0][1] != ':')
-			return -EINVAL;
+		if (argv[0][1] != ':') {
+			ret = -EINVAL;
+			goto out;
+		}
 		event = &argv[0][2];
 	} else {
 		event = strchr(argv[0], ':');
-		if (!event)
-			return -EINVAL;
+		if (!event) {
+			ret = -EINVAL;
+			goto out;
+		}
 		event++;
 	}
-	argc--; argv++;
 
 	p = strchr(event, '/');
 	if (p) {
@@ -63,7 +71,7 @@ int dyn_event_release(int argc, char **argv, struct dyn_event_operations *type)
 		if (type && type != pos->ops)
 			continue;
 		if (!pos->ops->match(system, event,
-				argc, (const char **)argv, pos))
+				argc - 1, (const char **)argv + 1, pos))
 			continue;
 
 		ret = pos->ops->free(pos);
@@ -71,21 +79,22 @@ int dyn_event_release(int argc, char **argv, struct dyn_event_operations *type)
 			break;
 	}
 	mutex_unlock(&event_mutex);
-
+out:
+	argv_free(argv);
 	return ret;
 }
 
-static int create_dyn_event(int argc, char **argv)
+static int create_dyn_event(const char *raw_command)
 {
 	struct dyn_event_operations *ops;
 	int ret = -ENODEV;
 
-	if (argv[0][0] == '-' || argv[0][0] == '!')
-		return dyn_event_release(argc, argv, NULL);
+	if (raw_command[0] == '-' || raw_command[0] == '!')
+		return dyn_event_release(raw_command, NULL);
 
 	mutex_lock(&dyn_event_ops_mutex);
 	list_for_each_entry(ops, &dyn_event_ops_list, list) {
-		ret = ops->create(argc, (const char **)argv);
+		ret = ops->create(raw_command);
 		if (!ret || ret != -ECANCELED)
 			break;
 	}
diff --git a/kernel/trace/trace_dynevent.h b/kernel/trace/trace_dynevent.h
index d6857a254ede..4f4e03df4cbb 100644
--- a/kernel/trace/trace_dynevent.h
+++ b/kernel/trace/trace_dynevent.h
@@ -39,7 +39,7 @@ struct dyn_event;
  */
 struct dyn_event_operations {
 	struct list_head	list;
-	int (*create)(int argc, const char *argv[]);
+	int (*create)(const char *raw_command);
 	int (*show)(struct seq_file *m, struct dyn_event *ev);
 	bool (*is_busy)(struct dyn_event *ev);
 	int (*free)(struct dyn_event *ev);
@@ -97,7 +97,7 @@ void *dyn_event_seq_start(struct seq_file *m, loff_t *pos);
 void *dyn_event_seq_next(struct seq_file *m, void *v, loff_t *pos);
 void dyn_event_seq_stop(struct seq_file *m, void *v);
 int dyn_events_release_all(struct dyn_event_operations *type);
-int dyn_event_release(int argc, char **argv, struct dyn_event_operations *type);
+int dyn_event_release(const char *raw_command, struct dyn_event_operations *type);
 
 /*
  * for_each_dyn_event	-	iterate over the dyn_event list
diff --git a/kernel/trace/trace_events_synth.c b/kernel/trace/trace_events_synth.c
index bdd427ccdfc5..271811fbf8fb 100644
--- a/kernel/trace/trace_events_synth.c
+++ b/kernel/trace/trace_events_synth.c
@@ -62,7 +62,7 @@ static void synth_err(u8 err_type, u8 err_pos)
 			err_type, err_pos);
 }
 
-static int create_synth_event(int argc, const char **argv);
+static int create_synth_event(const char *raw_command);
 static int synth_event_show(struct seq_file *m, struct dyn_event *ev);
 static int synth_event_release(struct dyn_event *ev);
 static bool synth_event_is_busy(struct dyn_event *ev);
@@ -1385,18 +1385,30 @@ int synth_event_delete(const char *event_name)
 }
 EXPORT_SYMBOL_GPL(synth_event_delete);
 
-static int create_or_delete_synth_event(int argc, char **argv)
+static int create_or_delete_synth_event(const char *raw_command)
 {
-	const char *name = argv[0];
-	int ret;
+	char **argv, *name = NULL;
+	int argc = 0, ret = 0;
+
+	argv = argv_split(GFP_KERNEL, raw_command, &argc);
+	if (!argv)
+		return -ENOMEM;
+
+	if (!argc)
+		goto free;
+
+	name = argv[0];
 
 	/* trace_run_command() ensures argc != 0 */
 	if (name[0] == '!') {
 		ret = synth_event_delete(name + 1);
-		return ret;
+		goto free;
 	}
 
 	ret = __create_synth_event(argc - 1, name, (const char **)argv + 1);
+free:
+	argv_free(argv);
+
 	return ret == -ECANCELED ? -EINVAL : ret;
 }
 
@@ -1405,7 +1417,7 @@ static int synth_event_run_command(struct dynevent_cmd *cmd)
 	struct synth_event *se;
 	int ret;
 
-	ret = trace_run_command(cmd->seq.buffer, create_or_delete_synth_event);
+	ret = create_or_delete_synth_event(cmd->seq.buffer);
 	if (ret)
 		return ret;
 
@@ -1941,23 +1953,43 @@ int synth_event_trace_end(struct synth_event_trace_state *trace_state)
 }
 EXPORT_SYMBOL_GPL(synth_event_trace_end);
 
-static int create_synth_event(int argc, const char **argv)
+static int create_synth_event(const char *raw_command)
 {
-	const char *name = argv[0];
-	int len;
+	char **argv, *name;
+	int len, argc = 0, ret = 0;
+
+	argv = argv_split(GFP_KERNEL, raw_command, &argc);
+	if (!argv) {
+		ret = -ENOMEM;
+		return ret;
+	}
 
-	if (name[0] != 's' || name[1] != ':')
-		return -ECANCELED;
+	if (!argc)
+		goto free;
+
+	name = argv[0];
+
+	if (name[0] != 's' || name[1] != ':') {
+		ret = -ECANCELED;
+		goto free;
+	}
 	name += 2;
 
 	/* This interface accepts group name prefix */
 	if (strchr(name, '/')) {
 		len = str_has_prefix(name, SYNTH_SYSTEM "/");
-		if (len == 0)
-			return -EINVAL;
+		if (len == 0) {
+			ret = -EINVAL;
+			goto free;
+		}
 		name += len;
 	}
-	return __create_synth_event(argc - 1, name, argv + 1);
+
+	ret = __create_synth_event(argc - 1, name, (const char **)argv + 1);
+free:
+	argv_free(argv);
+
+	return ret;
 }
 
 static int synth_event_release(struct dyn_event *ev)
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index b911e9f6d9f5..ddef93e32905 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -34,7 +34,7 @@ static int __init set_kprobe_boot_events(char *str)
 }
 __setup("kprobe_event=", set_kprobe_boot_events);
 
-static int trace_kprobe_create(int argc, const char **argv);
+static int trace_kprobe_create(const char *raw_command);
 static int trace_kprobe_show(struct seq_file *m, struct dyn_event *ev);
 static int trace_kprobe_release(struct dyn_event *ev);
 static bool trace_kprobe_is_busy(struct dyn_event *ev);
@@ -710,7 +710,7 @@ static inline void sanitize_event_name(char *name)
 			*name = '_';
 }
 
-static int trace_kprobe_create(int argc, const char *argv[])
+static int __trace_kprobe_create(int argc, const char *argv[])
 {
 	/*
 	 * Argument syntax:
@@ -907,20 +907,25 @@ static int trace_kprobe_create(int argc, const char *argv[])
 	goto out;
 }
 
-static int create_or_delete_trace_kprobe(int argc, char **argv)
+static int trace_kprobe_create(const char *raw_command)
+{
+	return trace_probe_create(raw_command, __trace_kprobe_create);
+}
+
+static int create_or_delete_trace_kprobe(const char *raw_command)
 {
 	int ret;
 
-	if (argv[0][0] == '-')
-		return dyn_event_release(argc, argv, &trace_kprobe_ops);
+	if (raw_command[0] == '-')
+		return dyn_event_release(raw_command, &trace_kprobe_ops);
 
-	ret = trace_kprobe_create(argc, (const char **)argv);
+	ret = trace_kprobe_create(raw_command);
 	return ret == -ECANCELED ? -EINVAL : ret;
 }
 
 static int trace_kprobe_run_command(struct dynevent_cmd *cmd)
 {
-	return trace_run_command(cmd->seq.buffer, create_or_delete_trace_kprobe);
+	return create_or_delete_trace_kprobe(cmd->seq.buffer);
 }
 
 /**
@@ -1081,7 +1086,7 @@ int kprobe_event_delete(const char *name)
 
 	snprintf(buf, MAX_EVENT_NAME_LEN, "-:%s", name);
 
-	return trace_run_command(buf, create_or_delete_trace_kprobe);
+	return create_or_delete_trace_kprobe(buf);
 }
 EXPORT_SYMBOL_GPL(kprobe_event_delete);
 
@@ -1884,7 +1889,7 @@ static __init void setup_boot_kprobe_events(void)
 		if (p)
 			*p++ = '\0';
 
-		ret = trace_run_command(cmd, create_or_delete_trace_kprobe);
+		ret = create_or_delete_trace_kprobe(cmd);
 		if (ret)
 			pr_warn("Failed to add event(%d): %s\n", ret, cmd);
 		else
@@ -1982,8 +1987,7 @@ static __init int kprobe_trace_self_tests_init(void)
 
 	pr_info("Testing kprobe tracing: ");
 
-	ret = trace_run_command("p:testprobe kprobe_trace_selftest_target $stack $stack0 +0($stack)",
-				create_or_delete_trace_kprobe);
+	ret = create_or_delete_trace_kprobe("p:testprobe kprobe_trace_selftest_target $stack $stack0 +0($stack)");
 	if (WARN_ON_ONCE(ret)) {
 		pr_warn("error on probing function entry.\n");
 		warn++;
@@ -2004,8 +2008,7 @@ static __init int kprobe_trace_self_tests_init(void)
 		}
 	}
 
-	ret = trace_run_command("r:testprobe2 kprobe_trace_selftest_target $retval",
-				create_or_delete_trace_kprobe);
+	ret = create_or_delete_trace_kprobe("r:testprobe2 kprobe_trace_selftest_target $retval");
 	if (WARN_ON_ONCE(ret)) {
 		pr_warn("error on probing function return.\n");
 		warn++;
@@ -2078,13 +2081,13 @@ static __init int kprobe_trace_self_tests_init(void)
 				trace_probe_event_call(&tk->tp), file);
 	}
 
-	ret = trace_run_command("-:testprobe", create_or_delete_trace_kprobe);
+	ret = create_or_delete_trace_kprobe("-:testprobe");
 	if (WARN_ON_ONCE(ret)) {
 		pr_warn("error on deleting a probe.\n");
 		warn++;
 	}
 
-	ret = trace_run_command("-:testprobe2", create_or_delete_trace_kprobe);
+	ret = create_or_delete_trace_kprobe("-:testprobe2");
 	if (WARN_ON_ONCE(ret)) {
 		pr_warn("error on deleting a probe.\n");
 		warn++;
diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
index d2867ccc6aca..ec589a4612df 100644
--- a/kernel/trace/trace_probe.c
+++ b/kernel/trace/trace_probe.c
@@ -1134,3 +1134,20 @@ bool trace_probe_match_command_args(struct trace_probe *tp,
 	}
 	return true;
 }
+
+int trace_probe_create(const char *raw_command, int (*createfn)(int, const char **))
+{
+	int argc = 0, ret = 0;
+	char **argv;
+
+	argv = argv_split(GFP_KERNEL, raw_command, &argc);
+	if (!argv)
+		return -ENOMEM;
+
+	if (argc)
+		ret = createfn(argc, (const char **)argv);
+
+	argv_free(argv);
+
+	return ret;
+}
diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
index 2f703a20c724..7ce4027089ee 100644
--- a/kernel/trace/trace_probe.h
+++ b/kernel/trace/trace_probe.h
@@ -341,6 +341,7 @@ struct event_file_link *trace_probe_get_file_link(struct trace_probe *tp,
 int trace_probe_compare_arg_type(struct trace_probe *a, struct trace_probe *b);
 bool trace_probe_match_command_args(struct trace_probe *tp,
 				    int argc, const char **argv);
+int trace_probe_create(const char *raw_command, int (*createfn)(int, const char **));
 
 #define trace_probe_for_each_link(pos, tp)	\
 	list_for_each_entry(pos, &(tp)->event->files, list)
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 3cf7128e1ad3..e6b56a65f80f 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -34,7 +34,7 @@ struct uprobe_trace_entry_head {
 #define DATAOF_TRACE_ENTRY(entry, is_return)		\
 	((void*)(entry) + SIZEOF_TRACE_ENTRY(is_return))
 
-static int trace_uprobe_create(int argc, const char **argv);
+static int trace_uprobe_create(const char *raw_command);
 static int trace_uprobe_show(struct seq_file *m, struct dyn_event *ev);
 static int trace_uprobe_release(struct dyn_event *ev);
 static bool trace_uprobe_is_busy(struct dyn_event *ev);
@@ -530,7 +530,7 @@ static int register_trace_uprobe(struct trace_uprobe *tu)
  * Argument syntax:
  *  - Add uprobe: p|r[:[GRP/]EVENT] PATH:OFFSET[%return][(REF)] [FETCHARGS]
  */
-static int trace_uprobe_create(int argc, const char **argv)
+static int __trace_uprobe_create(int argc, const char **argv)
 {
 	struct trace_uprobe *tu;
 	const char *event = NULL, *group = UPROBE_EVENT_SYSTEM;
@@ -716,14 +716,19 @@ static int trace_uprobe_create(int argc, const char **argv)
 	return ret;
 }
 
-static int create_or_delete_trace_uprobe(int argc, char **argv)
+int trace_uprobe_create(const char *raw_command)
+{
+	return trace_probe_create(raw_command, __trace_uprobe_create);
+}
+
+static int create_or_delete_trace_uprobe(const char *raw_command)
 {
 	int ret;
 
-	if (argv[0][0] == '-')
-		return dyn_event_release(argc, argv, &trace_uprobe_ops);
+	if (raw_command[0] == '-')
+		return dyn_event_release(raw_command, &trace_uprobe_ops);
 
-	ret = trace_uprobe_create(argc, (const char **)argv);
+	ret = trace_uprobe_create(raw_command);
 	return ret == -ECANCELED ? -EINVAL : ret;
 }
 
-- 
2.17.1

[PATCH v3 2/5] tracing: Rework synthetic event command parsing

[Tom Zanussi]

Now that command parsing has been delegated to the create functions
and we're no longer constrained by argv_split(), we can modify the
synthetic event command parser to better match the higher-level
structure of the synthetic event commands, which is basically an event
name followed by a set of semicolon-separated fields.

Since we're also now passed the raw command, we can also save it
directly and can get rid of save_cmdstr().

Signed-off-by: Tom Zanussi <zanussi@kernel.org>
---
 kernel/trace/trace_events_synth.c | 181 ++++++++++++++----------------
 1 file changed, 83 insertions(+), 98 deletions(-)

diff --git a/kernel/trace/trace_events_synth.c b/kernel/trace/trace_events_synth.c
index 271811fbf8fb..0d2fe4b6bd94 100644
--- a/kernel/trace/trace_events_synth.c
+++ b/kernel/trace/trace_events_synth.c
@@ -48,7 +48,7 @@ static int errpos(const char *str)
 	return err_pos(last_cmd, str);
 }
 
-static void last_cmd_set(char *str)
+static void last_cmd_set(const char *str)
 {
 	if (!str)
 		return;
@@ -579,8 +579,7 @@ static void free_synth_field(struct synth_field *field)
 	kfree(field);
 }
 
-static struct synth_field *parse_synth_field(int argc, const char **argv,
-					     int *consumed)
+static struct synth_field *parse_synth_field(int argc, char **argv)
 {
 	struct synth_field *field;
 	const char *prefix = NULL, *field_type = argv[0], *field_name, *array;
@@ -588,9 +587,6 @@ static struct synth_field *parse_synth_field(int argc, const char **argv,
 	struct seq_buf s;
 	ssize_t size;
 
-	if (field_type[0] == ';')
-		field_type++;
-
 	if (!strcmp(field_type, "unsigned")) {
 		if (argc < 3) {
 			synth_err(SYNTH_ERR_INCOMPLETE_TYPE, errpos(field_type));
@@ -599,10 +595,12 @@ static struct synth_field *parse_synth_field(int argc, const char **argv,
 		prefix = "unsigned ";
 		field_type = argv[1];
 		field_name = argv[2];
-		*consumed = 3;
-	} else {
+	} else
 		field_name = argv[1];
-		*consumed = 2;
+
+	if (!field_name) {
+		synth_err(SYNTH_ERR_INVALID_FIELD, errpos(field_type));
+		return ERR_PTR(-EINVAL);
 	}
 
 	field = kzalloc(sizeof(*field), GFP_KERNEL);
@@ -613,8 +611,6 @@ static struct synth_field *parse_synth_field(int argc, const char **argv,
 	array = strchr(field_name, '[');
 	if (array)
 		len -= strlen(array);
-	else if (field_name[len - 1] == ';')
-		len--;
 
 	field->name = kmemdup_nul(field_name, len, GFP_KERNEL);
 	if (!field->name) {
@@ -627,8 +623,6 @@ static struct synth_field *parse_synth_field(int argc, const char **argv,
 		goto free;
 	}
 
-	if (field_type[0] == ';')
-		field_type++;
 	len = strlen(field_type) + 1;
 
 	if (array)
@@ -646,18 +640,14 @@ static struct synth_field *parse_synth_field(int argc, const char **argv,
 	if (prefix)
 		seq_buf_puts(&s, prefix);
 	seq_buf_puts(&s, field_type);
-	if (array) {
+	if (array)
 		seq_buf_puts(&s, array);
-		if (s.buffer[s.len - 1] == ';')
-			s.len--;
-	}
 	if (WARN_ON_ONCE(!seq_buf_buffer_left(&s)))
 		goto free;
 	s.buffer[s.len] = '\0';
 
 	size = synth_field_size(field->type);
 	if (size < 0) {
-		synth_err(SYNTH_ERR_INVALID_TYPE, errpos(field_type));
 		ret = -EINVAL;
 		goto free;
 	} else if (size == 0) {
@@ -1162,46 +1152,12 @@ int synth_event_gen_cmd_array_start(struct dynevent_cmd *cmd, const char *name,
 }
 EXPORT_SYMBOL_GPL(synth_event_gen_cmd_array_start);
 
-static int save_cmdstr(int argc, const char *name, const char **argv)
-{
-	struct seq_buf s;
-	char *buf;
-	int i;
-
-	buf = kzalloc(MAX_DYNEVENT_CMD_LEN, GFP_KERNEL);
-	if (!buf)
-		return -ENOMEM;
-
-	seq_buf_init(&s, buf, MAX_DYNEVENT_CMD_LEN);
-
-	seq_buf_puts(&s, name);
-
-	for (i = 0; i < argc; i++) {
-		seq_buf_putc(&s, ' ');
-		seq_buf_puts(&s, argv[i]);
-	}
-
-	if (!seq_buf_buffer_left(&s)) {
-		synth_err(SYNTH_ERR_CMD_TOO_LONG, 0);
-		kfree(buf);
-		return -EINVAL;
-	}
-	buf[s.len] = 0;
-	last_cmd_set(buf);
-
-	kfree(buf);
-	return 0;
-}
-
-static int __create_synth_event(int argc, const char *name, const char **argv)
+static int __create_synth_event(const char *name, const char *raw_fields)
 {
+	int i, argc, n_fields = 0, ret = 0;
 	struct synth_field *field, *fields[SYNTH_FIELDS_MAX];
+	char **argv, *field_str, *tmp_fields, *saved_fields = NULL;
 	struct synth_event *event = NULL;
-	int i, consumed = 0, n_fields = 0, ret = 0;
-
-	ret = save_cmdstr(argc, name, argv);
-	if (ret)
-		return ret;
 
 	/*
 	 * Argument syntax:
@@ -1210,13 +1166,14 @@ static int __create_synth_event(int argc, const char *name, const char **argv)
 	 *      where 'field' = type field_name
 	 */
 
-	if (name[0] == '\0' || argc < 1) {
+	mutex_lock(&event_mutex);
+
+	if (name[0] == '\0') {
 		synth_err(SYNTH_ERR_CMD_INCOMPLETE, 0);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto out;
 	}
 
-	mutex_lock(&event_mutex);
-
 	if (!is_good_name(name)) {
 		synth_err(SYNTH_ERR_BAD_NAME, errpos(name));
 		ret = -EINVAL;
@@ -1230,26 +1187,42 @@ static int __create_synth_event(int argc, const char *name, const char **argv)
 		goto out;
 	}
 
-	for (i = 0; i < argc - 1; i++) {
-		if (strcmp(argv[i], ";") == 0)
-			continue;
+	tmp_fields = saved_fields = kstrdup(raw_fields, GFP_KERNEL);
+	if (!tmp_fields) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	while ((field_str = strsep(&tmp_fields, ";")) != NULL) {
 		if (n_fields == SYNTH_FIELDS_MAX) {
 			synth_err(SYNTH_ERR_TOO_MANY_FIELDS, 0);
 			ret = -EINVAL;
 			goto err;
 		}
 
-		field = parse_synth_field(argc - i, &argv[i], &consumed);
+		argv = argv_split(GFP_KERNEL, field_str, &argc);
+		if (!argv) {
+			ret = -ENOMEM;
+			goto err;
+		}
+
+		if (!argc)
+			continue;
+
+		field = parse_synth_field(argc, argv);
 		if (IS_ERR(field)) {
+			argv_free(argv);
 			ret = PTR_ERR(field);
 			goto err;
 		}
+
+		argv_free(argv);
+
 		fields[n_fields++] = field;
-		i += consumed - 1;
 	}
 
-	if (i < argc && strcmp(argv[i], ";") != 0) {
-		synth_err(SYNTH_ERR_INVALID_FIELD, errpos(argv[i]));
+	if (n_fields == 0) {
+		synth_err(SYNTH_ERR_CMD_INCOMPLETE, 0);
 		ret = -EINVAL;
 		goto err;
 	}
@@ -1268,6 +1241,8 @@ static int __create_synth_event(int argc, const char *name, const char **argv)
  out:
 	mutex_unlock(&event_mutex);
 
+	kfree(saved_fields);
+
 	return ret;
  err:
 	for (i = 0; i < n_fields; i++)
@@ -1387,29 +1362,36 @@ EXPORT_SYMBOL_GPL(synth_event_delete);
 
 static int create_or_delete_synth_event(const char *raw_command)
 {
-	char **argv, *name = NULL;
-	int argc = 0, ret = 0;
+	char *name = NULL, *fields, *p;
+	int ret = 0;
 
-	argv = argv_split(GFP_KERNEL, raw_command, &argc);
-	if (!argv)
-		return -ENOMEM;
+	raw_command = skip_spaces(raw_command);
+	if (raw_command[0] == '\0')
+		return ret;
 
-	if (!argc)
-		goto free;
+	last_cmd_set(raw_command);
 
-	name = argv[0];
+	ret = check_command(raw_command);
+	if (ret)
+		return ret;
+
+	p = strpbrk(raw_command, " \t");
+	if (!p)
+		return -EINVAL;
+
+	name = kmemdup_nul(raw_command, p - raw_command, GFP_KERNEL);
+	fields = skip_spaces(p);
 
-	/* trace_run_command() ensures argc != 0 */
 	if (name[0] == '!') {
 		ret = synth_event_delete(name + 1);
 		goto free;
 	}
 
-	ret = __create_synth_event(argc - 1, name, (const char **)argv + 1);
+	ret = __create_synth_event(name, fields);
 free:
-	argv_free(argv);
+	kfree(name);
 
-	return ret == -ECANCELED ? -EINVAL : ret;
+	return ret;
 }
 
 static int synth_event_run_command(struct dynevent_cmd *cmd)
@@ -1955,39 +1937,42 @@ EXPORT_SYMBOL_GPL(synth_event_trace_end);
 
 static int create_synth_event(const char *raw_command)
 {
-	char **argv, *name;
-	int len, argc = 0, ret = 0;
+	char *fields, *p;
+	const char *name;
+	int len, ret = 0;
 
-	argv = argv_split(GFP_KERNEL, raw_command, &argc);
-	if (!argv) {
-		ret = -ENOMEM;
+	raw_command = skip_spaces(raw_command);
+	if (raw_command[0] == '\0')
 		return ret;
-	}
 
-	if (!argc)
-		goto free;
+	last_cmd_set(raw_command);
 
-	name = argv[0];
+	p = strpbrk(raw_command, " \t");
+	if (!p)
+		return -EINVAL;
 
-	if (name[0] != 's' || name[1] != ':') {
-		ret = -ECANCELED;
-		goto free;
-	}
+	fields = skip_spaces(p);
+
+	name = raw_command;
+
+	if (name[0] != 's' || name[1] != ':')
+		return -ECANCELED;
 	name += 2;
 
 	/* This interface accepts group name prefix */
 	if (strchr(name, '/')) {
 		len = str_has_prefix(name, SYNTH_SYSTEM "/");
-		if (len == 0) {
-			ret = -EINVAL;
-			goto free;
-		}
+		if (len == 0)
+			return -EINVAL;
 		name += len;
 	}
 
-	ret = __create_synth_event(argc - 1, name, (const char **)argv + 1);
-free:
-	argv_free(argv);
+	len = name - raw_command;
+
+	name = kmemdup_nul(raw_command + len, p - raw_command - len, GFP_KERNEL);
+	ret = __create_synth_event(name, fields);
+
+	kfree(name);
 
 	return ret;
 }
-- 
2.17.1

[PATCH v3 3/5] tracing: Update synth command errors

[Tom Zanussi]

Since array types are handled differently, errors referencing them
also need to be handled differently.  Add and use a new
INVALID_ARRAY_SPEC error.  Also add INVALID_CMD and INVALID_DYN_CMD to
catch and display the correct form for badly-formed commands, which
can also be used in place of CMD_INCOMPLETE, which is removed, and
remove CMD_TOO_LONG, since it's no longer used.

Signed-off-by: Tom Zanussi <zanussi@kernel.org>
---
 kernel/trace/trace_events_synth.c | 68 +++++++++++++++++++++++++++----
 1 file changed, 59 insertions(+), 9 deletions(-)

diff --git a/kernel/trace/trace_events_synth.c b/kernel/trace/trace_events_synth.c
index 0d2fe4b6bd94..fdf0e85c0b8a 100644
--- a/kernel/trace/trace_events_synth.c
+++ b/kernel/trace/trace_events_synth.c
@@ -23,13 +23,14 @@
 #undef ERRORS
 #define ERRORS	\
 	C(BAD_NAME,		"Illegal name"),		\
-	C(CMD_INCOMPLETE,	"Incomplete command"),		\
+	C(INVALID_CMD,		"Command must be of the form: <name> field[;field] ..."),\
+	C(INVALID_DYN_CMD,	"Command must be of the form: s or -:[synthetic/]<name> field[;field] ..."),\
 	C(EVENT_EXISTS,		"Event already exists"),	\
 	C(TOO_MANY_FIELDS,	"Too many fields"),		\
 	C(INCOMPLETE_TYPE,	"Incomplete type"),		\
 	C(INVALID_TYPE,		"Invalid type"),		\
-	C(INVALID_FIELD,	"Invalid field"),		\
-	C(CMD_TOO_LONG,		"Command too long"),
+	C(INVALID_FIELD,        "Invalid field"),		\
+	C(INVALID_ARRAY_SPEC,	"Invalid array specification"),
 
 #undef C
 #define C(a, b)		SYNTH_ERR_##a
@@ -648,6 +649,10 @@ static struct synth_field *parse_synth_field(int argc, char **argv)
 
 	size = synth_field_size(field->type);
 	if (size < 0) {
+		if (array)
+			synth_err(SYNTH_ERR_INVALID_ARRAY_SPEC, errpos(field_name));
+		else
+			synth_err(SYNTH_ERR_INVALID_TYPE, errpos(field_type));
 		ret = -EINVAL;
 		goto free;
 	} else if (size == 0) {
@@ -1169,7 +1174,7 @@ static int __create_synth_event(const char *name, const char *raw_fields)
 	mutex_lock(&event_mutex);
 
 	if (name[0] == '\0') {
-		synth_err(SYNTH_ERR_CMD_INCOMPLETE, 0);
+		synth_err(SYNTH_ERR_INVALID_CMD, 0);
 		ret = -EINVAL;
 		goto out;
 	}
@@ -1222,7 +1227,7 @@ static int __create_synth_event(const char *name, const char *raw_fields)
 	}
 
 	if (n_fields == 0) {
-		synth_err(SYNTH_ERR_CMD_INCOMPLETE, 0);
+		synth_err(SYNTH_ERR_INVALID_CMD, 0);
 		ret = -EINVAL;
 		goto err;
 	}
@@ -1360,6 +1365,37 @@ int synth_event_delete(const char *event_name)
 }
 EXPORT_SYMBOL_GPL(synth_event_delete);
 
+static int check_command(const char *raw_command)
+{
+	char **argv = NULL, *cmd, *saved_cmd, *name_and_field;
+	int argc, ret = 0;
+
+	cmd = saved_cmd = kstrdup(raw_command, GFP_KERNEL);
+	if (!cmd)
+		return -ENOMEM;
+
+	name_and_field = strsep(&cmd, ";");
+	if (!name_and_field) {
+		ret = -EINVAL;
+		goto free;
+	}
+
+	argv = argv_split(GFP_KERNEL, name_and_field, &argc);
+	if (!argv) {
+		ret = -ENOMEM;
+		goto free;
+	}
+
+	if (argc < 3)
+		ret = -EINVAL;
+free:
+	kfree(saved_cmd);
+	if (argv)
+		argv_free(argv);
+
+	return ret;
+}
+
 static int create_or_delete_synth_event(const char *raw_command)
 {
 	char *name = NULL, *fields, *p;
@@ -1372,12 +1408,16 @@ static int create_or_delete_synth_event(const char *raw_command)
 	last_cmd_set(raw_command);
 
 	ret = check_command(raw_command);
-	if (ret)
+	if (ret) {
+		synth_err(SYNTH_ERR_INVALID_CMD, 0);
 		return ret;
+	}
 
 	p = strpbrk(raw_command, " \t");
-	if (!p)
+	if (!p) {
+		synth_err(SYNTH_ERR_INVALID_CMD, 0);
 		return -EINVAL;
+	}
 
 	name = kmemdup_nul(raw_command, p - raw_command, GFP_KERNEL);
 	fields = skip_spaces(p);
@@ -1948,8 +1988,10 @@ static int create_synth_event(const char *raw_command)
 	last_cmd_set(raw_command);
 
 	p = strpbrk(raw_command, " \t");
-	if (!p)
+	if (!p) {
+		synth_err(SYNTH_ERR_INVALID_CMD, 0);
 		return -EINVAL;
+	}
 
 	fields = skip_spaces(p);
 
@@ -1962,13 +2004,21 @@ static int create_synth_event(const char *raw_command)
 	/* This interface accepts group name prefix */
 	if (strchr(name, '/')) {
 		len = str_has_prefix(name, SYNTH_SYSTEM "/");
-		if (len == 0)
+		if (len == 0) {
+			synth_err(SYNTH_ERR_INVALID_DYN_CMD, 0);
 			return -EINVAL;
+		}
 		name += len;
 	}
 
 	len = name - raw_command;
 
+	ret = check_command(raw_command + len);
+	if (ret) {
+		synth_err(SYNTH_ERR_INVALID_CMD, 0);
+		return ret;
+	}
+
 	name = kmemdup_nul(raw_command + len, p - raw_command - len, GFP_KERNEL);
 	ret = __create_synth_event(name, fields);
 
-- 
2.17.1

[PATCH v3 4/5] selftests/ftrace: Add synthetic event field separators

[Tom Zanussi]

These tests omit field separators from the synthetic event definitions
so don't follow the syntax '<event_name> field[;field] ...' required
for synthetic events.

Fixes: f06eec4d0f2c (selftests: ftrace: Add inter-event hist triggers testcases)
Signed-off-by: Tom Zanussi <zanussi@kernel.org>
---
 .../trigger/inter-event/trigger-inter-event-combined-hist.tc  | 4 ++--
 .../test.d/trigger/inter-event/trigger-onmatch-action-hist.tc | 2 +-
 .../trigger/inter-event/trigger-onmatch-onmax-action-hist.tc  | 2 +-
 .../test.d/trigger/inter-event/trigger-trace-action-hist.tc   | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-inter-event-combined-hist.tc b/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-inter-event-combined-hist.tc
index 9098f1e7433f..29a03ed3377d 100644
--- a/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-inter-event-combined-hist.tc
+++ b/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-inter-event-combined-hist.tc
@@ -10,7 +10,7 @@ fail() { #msg
 
 echo "Test create synthetic event"
 
-echo 'waking_latency  u64 lat pid_t pid' > synthetic_events
+echo 'waking_latency  u64 lat; pid_t pid' > synthetic_events
 if [ ! -d events/synthetic/waking_latency ]; then
     fail "Failed to create waking_latency synthetic event"
 fi
@@ -21,7 +21,7 @@ echo 'hist:keys=pid:ts0=common_timestamp.usecs if comm=="ping"' > events/sched/s
 echo 'hist:keys=pid:waking_lat=common_timestamp.usecs-$ts0:onmatch(sched.sched_waking).waking_latency($waking_lat,pid) if comm=="ping"' > events/sched/sched_wakeup/trigger
 echo 'hist:keys=pid,lat:sort=pid,lat' > events/synthetic/waking_latency/trigger
 
-echo 'wakeup_latency u64 lat pid_t pid' >> synthetic_events
+echo 'wakeup_latency u64 lat; pid_t pid' >> synthetic_events
 echo 'hist:keys=pid:ts1=common_timestamp.usecs if comm=="ping"' >> events/sched/sched_wakeup/trigger
 echo 'hist:keys=next_pid:wakeup_lat=common_timestamp.usecs-$ts1:onmatch(sched.sched_wakeup).wakeup_latency($wakeup_lat,next_pid) if next_comm=="ping"' > events/sched/sched_switch/trigger
 
diff --git a/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-onmatch-action-hist.tc b/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-onmatch-action-hist.tc
index 20e39471052e..5015d0d74de8 100644
--- a/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-onmatch-action-hist.tc
+++ b/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-onmatch-action-hist.tc
@@ -10,7 +10,7 @@ fail() { #msg
 
 echo "Test create synthetic event"
 
-echo 'wakeup_latency  u64 lat pid_t pid char comm[16]' > synthetic_events
+echo 'wakeup_latency  u64 lat; pid_t pid; char comm[16]' > synthetic_events
 if [ ! -d events/synthetic/wakeup_latency ]; then
     fail "Failed to create wakeup_latency synthetic event"
 fi
diff --git a/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-onmatch-onmax-action-hist.tc b/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-onmatch-onmax-action-hist.tc
index f4b03ab7c287..ac7ba2bbce47 100644
--- a/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-onmatch-onmax-action-hist.tc
+++ b/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-onmatch-onmax-action-hist.tc
@@ -10,7 +10,7 @@ fail() { #msg
 
 echo "Test create synthetic event"
 
-echo 'wakeup_latency  u64 lat pid_t pid char comm[16]' > synthetic_events
+echo 'wakeup_latency  u64 lat; pid_t pid; char comm[16]' > synthetic_events
 if [ ! -d events/synthetic/wakeup_latency ]; then
     fail "Failed to create wakeup_latency synthetic event"
 fi
diff --git a/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-trace-action-hist.tc b/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-trace-action-hist.tc
index c126d2350a6d..76a213f197a0 100644
--- a/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-trace-action-hist.tc
+++ b/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-trace-action-hist.tc
@@ -10,7 +10,7 @@ fail() { #msg
 
 echo "Test create synthetic event"
 
-echo 'wakeup_latency  u64 lat pid_t pid char comm[16]' > synthetic_events
+echo 'wakeup_latency  u64 lat; pid_t pid; char comm[16]' > synthetic_events
 if [ ! -d events/synthetic/wakeup_latency ]; then
     fail "Failed to create wakeup_latency synthetic event"
 fi
-- 
2.17.1

[PATCH v3 5/5] selftests/ftrace: Update synthetic event syntax errors

[Tom Zanussi]

Some of the synthetic event errors and positions have changed in the
code - update those and add several more tests.

Also add a runtime check to ensure that the kernel supports dynamic
strings in synthetic events, which these tests require.

Fixes: 81ff92a93d95 (selftests/ftrace: Add test case for synthetic
event syntax errors)

Reported-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Tom Zanussi <zanussi@kernel.org>
---
 .../trigger-synthetic_event_syntax_errors.tc  | 35 ++++++++++++++-----
 1 file changed, 27 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-synthetic_event_syntax_errors.tc b/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-synthetic_event_syntax_errors.tc
index ada594fe16cb..955e3ceea44b 100644
--- a/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-synthetic_event_syntax_errors.tc
+++ b/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-synthetic_event_syntax_errors.tc
@@ -1,19 +1,38 @@
 #!/bin/sh
 # SPDX-License-Identifier: GPL-2.0
 # description: event trigger - test synthetic_events syntax parser errors
-# requires: synthetic_events error_log
+# requires: synthetic_events error_log "char name[]' >> synthetic_events":README
 
 check_error() { # command-with-error-pos-by-^
     ftrace_errlog_check 'synthetic_events' "$1" 'synthetic_events'
 }
 
+check_dyn_error() { # command-with-error-pos-by-^
+    ftrace_errlog_check 'synthetic_events' "$1" 'dynamic_events'
+}
+
 check_error 'myevent ^chr arg'			# INVALID_TYPE
-check_error 'myevent ^char str[];; int v'	# INVALID_TYPE
-check_error 'myevent char ^str]; int v'		# INVALID_NAME
-check_error 'myevent char ^str;[]'		# INVALID_NAME
-check_error 'myevent ^char str[; int v'		# INVALID_TYPE
-check_error '^mye;vent char str[]'		# BAD_NAME
-check_error 'myevent char str[]; ^int'		# INVALID_FIELD
-check_error '^myevent'				# INCOMPLETE_CMD
+check_error 'myevent ^unsigned arg'		# INCOMPLETE_TYPE
+
+check_error 'myevent char ^str]; int v'		# BAD_NAME
+check_error '^mye-vent char str[]'		# BAD_NAME
+check_error 'myevent char ^st-r[]'		# BAD_NAME
+
+check_error 'myevent char str;^[]'		# INVALID_FIELD
+check_error 'myevent char str; ^int'		# INVALID_FIELD
+
+check_error 'myevent char ^str[; int v'		# INVALID_ARRAY_SPEC
+check_error 'myevent char ^str[kdjdk]'		# INVALID_ARRAY_SPEC
+check_error 'myevent char ^str[257]'		# INVALID_ARRAY_SPEC
+
+check_error '^mye;vent char str[]'		# INVALID_CMD
+check_error '^myevent ; char str[]'		# INVALID_CMD
+check_error '^myevent; char str[]'		# INVALID_CMD
+check_error '^myevent ;char str[]'		# INVALID_CMD
+check_error '^; char str[]'			# INVALID_CMD
+check_error '^;myevent char str[]'		# INVALID_CMD
+check_error '^myevent'				# INVALID_CMD
+
+check_dyn_error '^s:junk/myevent char str['	# INVALID_DYN_CMD
 
 exit 0
-- 
2.17.1

Re: [PATCH v3 1/5] tracing/dynevent: Delegate parsing to create function

[Steven Rostedt]

Hi Masami,

You had comments on this patch for v2. Is this one fine for you?

-- Steve


On Mon, 26 Oct 2020 10:06:09 -0500
Tom Zanussi <zanussi@kernel.org> wrote:

> From: Masami Hiramatsu <mhiramat@kernel.org>
> 
> Delegate command parsing to each create function so that the
> command syntax can be customized.
> 
> This requires changes to the kprobe/uprobe/synthetic event handling,
> which are also included here.
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> [ zanussi@kernel.org: added synthetic event modifications ]
> Signed-off-by: Tom Zanussi <zanussi@kernel.org>
> ---
>  kernel/trace/trace.c              | 23 ++----------
>  kernel/trace/trace.h              |  3 +-
>  kernel/trace/trace_dynevent.c     | 35 +++++++++++-------
>  kernel/trace/trace_dynevent.h     |  4 +--
>  kernel/trace/trace_events_synth.c | 60 +++++++++++++++++++++++--------
>  kernel/trace/trace_kprobe.c       | 33 +++++++++--------
>  kernel/trace/trace_probe.c        | 17 +++++++++
>  kernel/trace/trace_probe.h        |  1 +
>  kernel/trace/trace_uprobe.c       | 17 +++++----
>  9 files changed, 120 insertions(+), 73 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 63c97012ed39..277d97220971 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -9367,30 +9367,11 @@ void ftrace_dump(enum ftrace_dump_mode oops_dump_mode)
>  }
>  EXPORT_SYMBOL_GPL(ftrace_dump);
>  
> -int trace_run_command(const char *buf, int (*createfn)(int, char **))
> -{
> -	char **argv;
> -	int argc, ret;
> -
> -	argc = 0;
> -	ret = 0;
> -	argv = argv_split(GFP_KERNEL, buf, &argc);
> -	if (!argv)
> -		return -ENOMEM;
> -
> -	if (argc)
> -		ret = createfn(argc, argv);
> -
> -	argv_free(argv);
> -
> -	return ret;
> -}
> -
>  #define WRITE_BUFSIZE  4096
>  
>  ssize_t trace_parse_run_command(struct file *file, const char __user *buffer,
>  				size_t count, loff_t *ppos,
> -				int (*createfn)(int, char **))
> +				int (*createfn)(const char *))
>  {
>  	char *kbuf, *buf, *tmp;
>  	int ret = 0;
> @@ -9438,7 +9419,7 @@ ssize_t trace_parse_run_command(struct file *file, const char __user *buffer,
>  			if (tmp)
>  				*tmp = '\0';
>  
> -			ret = trace_run_command(buf, createfn);
> +			ret = createfn(buf);
>  			if (ret)
>  				goto out;
>  			buf += size;
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 34e0c4d5a6e7..02d7c487a30b 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -1982,10 +1982,9 @@ extern int tracing_set_cpumask(struct trace_array *tr,
>  
>  #define MAX_EVENT_NAME_LEN	64
>  
> -extern int trace_run_command(const char *buf, int (*createfn)(int, char**));
>  extern ssize_t trace_parse_run_command(struct file *file,
>  		const char __user *buffer, size_t count, loff_t *ppos,
> -		int (*createfn)(int, char**));
> +		int (*createfn)(const char *));
>  
>  extern unsigned int err_pos(char *cmd, const char *str);
>  extern void tracing_log_err(struct trace_array *tr,
> diff --git a/kernel/trace/trace_dynevent.c b/kernel/trace/trace_dynevent.c
> index 5fa49cfd2bb6..af83bc5447fe 100644
> --- a/kernel/trace/trace_dynevent.c
> +++ b/kernel/trace/trace_dynevent.c
> @@ -31,23 +31,31 @@ int dyn_event_register(struct dyn_event_operations *ops)
>  	return 0;
>  }
>  
> -int dyn_event_release(int argc, char **argv, struct dyn_event_operations *type)
> +int dyn_event_release(const char *raw_command, struct dyn_event_operations *type)
>  {
>  	struct dyn_event *pos, *n;
>  	char *system = NULL, *event, *p;
> -	int ret = -ENOENT;
> +	int argc, ret = -ENOENT;
> +	char **argv;
> +
> +	argv = argv_split(GFP_KERNEL, raw_command, &argc);
> +	if (!argv)
> +		return -ENOMEM;
>  
>  	if (argv[0][0] == '-') {
> -		if (argv[0][1] != ':')
> -			return -EINVAL;
> +		if (argv[0][1] != ':') {
> +			ret = -EINVAL;
> +			goto out;
> +		}
>  		event = &argv[0][2];
>  	} else {
>  		event = strchr(argv[0], ':');
> -		if (!event)
> -			return -EINVAL;
> +		if (!event) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
>  		event++;
>  	}
> -	argc--; argv++;
>  
>  	p = strchr(event, '/');
>  	if (p) {
> @@ -63,7 +71,7 @@ int dyn_event_release(int argc, char **argv, struct dyn_event_operations *type)
>  		if (type && type != pos->ops)
>  			continue;
>  		if (!pos->ops->match(system, event,
> -				argc, (const char **)argv, pos))
> +				argc - 1, (const char **)argv + 1, pos))
>  			continue;
>  
>  		ret = pos->ops->free(pos);
> @@ -71,21 +79,22 @@ int dyn_event_release(int argc, char **argv, struct dyn_event_operations *type)
>  			break;
>  	}
>  	mutex_unlock(&event_mutex);
> -
> +out:
> +	argv_free(argv);
>  	return ret;
>  }
>  
> -static int create_dyn_event(int argc, char **argv)
> +static int create_dyn_event(const char *raw_command)
>  {
>  	struct dyn_event_operations *ops;
>  	int ret = -ENODEV;
>  
> -	if (argv[0][0] == '-' || argv[0][0] == '!')
> -		return dyn_event_release(argc, argv, NULL);
> +	if (raw_command[0] == '-' || raw_command[0] == '!')
> +		return dyn_event_release(raw_command, NULL);
>  
>  	mutex_lock(&dyn_event_ops_mutex);
>  	list_for_each_entry(ops, &dyn_event_ops_list, list) {
> -		ret = ops->create(argc, (const char **)argv);
> +		ret = ops->create(raw_command);
>  		if (!ret || ret != -ECANCELED)
>  			break;
>  	}
> diff --git a/kernel/trace/trace_dynevent.h b/kernel/trace/trace_dynevent.h
> index d6857a254ede..4f4e03df4cbb 100644
> --- a/kernel/trace/trace_dynevent.h
> +++ b/kernel/trace/trace_dynevent.h
> @@ -39,7 +39,7 @@ struct dyn_event;
>   */
>  struct dyn_event_operations {
>  	struct list_head	list;
> -	int (*create)(int argc, const char *argv[]);
> +	int (*create)(const char *raw_command);
>  	int (*show)(struct seq_file *m, struct dyn_event *ev);
>  	bool (*is_busy)(struct dyn_event *ev);
>  	int (*free)(struct dyn_event *ev);
> @@ -97,7 +97,7 @@ void *dyn_event_seq_start(struct seq_file *m, loff_t *pos);
>  void *dyn_event_seq_next(struct seq_file *m, void *v, loff_t *pos);
>  void dyn_event_seq_stop(struct seq_file *m, void *v);
>  int dyn_events_release_all(struct dyn_event_operations *type);
> -int dyn_event_release(int argc, char **argv, struct dyn_event_operations *type);
> +int dyn_event_release(const char *raw_command, struct dyn_event_operations *type);
>  
>  /*
>   * for_each_dyn_event	-	iterate over the dyn_event list
> diff --git a/kernel/trace/trace_events_synth.c b/kernel/trace/trace_events_synth.c
> index bdd427ccdfc5..271811fbf8fb 100644
> --- a/kernel/trace/trace_events_synth.c
> +++ b/kernel/trace/trace_events_synth.c
> @@ -62,7 +62,7 @@ static void synth_err(u8 err_type, u8 err_pos)
>  			err_type, err_pos);
>  }
>  
> -static int create_synth_event(int argc, const char **argv);
> +static int create_synth_event(const char *raw_command);
>  static int synth_event_show(struct seq_file *m, struct dyn_event *ev);
>  static int synth_event_release(struct dyn_event *ev);
>  static bool synth_event_is_busy(struct dyn_event *ev);
> @@ -1385,18 +1385,30 @@ int synth_event_delete(const char *event_name)
>  }
>  EXPORT_SYMBOL_GPL(synth_event_delete);
>  
> -static int create_or_delete_synth_event(int argc, char **argv)
> +static int create_or_delete_synth_event(const char *raw_command)
>  {
> -	const char *name = argv[0];
> -	int ret;
> +	char **argv, *name = NULL;
> +	int argc = 0, ret = 0;
> +
> +	argv = argv_split(GFP_KERNEL, raw_command, &argc);
> +	if (!argv)
> +		return -ENOMEM;
> +
> +	if (!argc)
> +		goto free;
> +
> +	name = argv[0];
>  
>  	/* trace_run_command() ensures argc != 0 */
>  	if (name[0] == '!') {
>  		ret = synth_event_delete(name + 1);
> -		return ret;
> +		goto free;
>  	}
>  
>  	ret = __create_synth_event(argc - 1, name, (const char **)argv + 1);
> +free:
> +	argv_free(argv);
> +
>  	return ret == -ECANCELED ? -EINVAL : ret;
>  }
>  
> @@ -1405,7 +1417,7 @@ static int synth_event_run_command(struct dynevent_cmd *cmd)
>  	struct synth_event *se;
>  	int ret;
>  
> -	ret = trace_run_command(cmd->seq.buffer, create_or_delete_synth_event);
> +	ret = create_or_delete_synth_event(cmd->seq.buffer);
>  	if (ret)
>  		return ret;
>  
> @@ -1941,23 +1953,43 @@ int synth_event_trace_end(struct synth_event_trace_state *trace_state)
>  }
>  EXPORT_SYMBOL_GPL(synth_event_trace_end);
>  
> -static int create_synth_event(int argc, const char **argv)
> +static int create_synth_event(const char *raw_command)
>  {
> -	const char *name = argv[0];
> -	int len;
> +	char **argv, *name;
> +	int len, argc = 0, ret = 0;
> +
> +	argv = argv_split(GFP_KERNEL, raw_command, &argc);
> +	if (!argv) {
> +		ret = -ENOMEM;
> +		return ret;
> +	}
>  
> -	if (name[0] != 's' || name[1] != ':')
> -		return -ECANCELED;
> +	if (!argc)
> +		goto free;
> +
> +	name = argv[0];
> +
> +	if (name[0] != 's' || name[1] != ':') {
> +		ret = -ECANCELED;
> +		goto free;
> +	}
>  	name += 2;
>  
>  	/* This interface accepts group name prefix */
>  	if (strchr(name, '/')) {
>  		len = str_has_prefix(name, SYNTH_SYSTEM "/");
> -		if (len == 0)
> -			return -EINVAL;
> +		if (len == 0) {
> +			ret = -EINVAL;
> +			goto free;
> +		}
>  		name += len;
>  	}
> -	return __create_synth_event(argc - 1, name, argv + 1);
> +
> +	ret = __create_synth_event(argc - 1, name, (const char **)argv + 1);
> +free:
> +	argv_free(argv);
> +
> +	return ret;
>  }
>  
>  static int synth_event_release(struct dyn_event *ev)
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index b911e9f6d9f5..ddef93e32905 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -34,7 +34,7 @@ static int __init set_kprobe_boot_events(char *str)
>  }
>  __setup("kprobe_event=", set_kprobe_boot_events);
>  
> -static int trace_kprobe_create(int argc, const char **argv);
> +static int trace_kprobe_create(const char *raw_command);
>  static int trace_kprobe_show(struct seq_file *m, struct dyn_event *ev);
>  static int trace_kprobe_release(struct dyn_event *ev);
>  static bool trace_kprobe_is_busy(struct dyn_event *ev);
> @@ -710,7 +710,7 @@ static inline void sanitize_event_name(char *name)
>  			*name = '_';
>  }
>  
> -static int trace_kprobe_create(int argc, const char *argv[])
> +static int __trace_kprobe_create(int argc, const char *argv[])
>  {
>  	/*
>  	 * Argument syntax:
> @@ -907,20 +907,25 @@ static int trace_kprobe_create(int argc, const char *argv[])
>  	goto out;
>  }
>  
> -static int create_or_delete_trace_kprobe(int argc, char **argv)
> +static int trace_kprobe_create(const char *raw_command)
> +{
> +	return trace_probe_create(raw_command, __trace_kprobe_create);
> +}
> +
> +static int create_or_delete_trace_kprobe(const char *raw_command)
>  {
>  	int ret;
>  
> -	if (argv[0][0] == '-')
> -		return dyn_event_release(argc, argv, &trace_kprobe_ops);
> +	if (raw_command[0] == '-')
> +		return dyn_event_release(raw_command, &trace_kprobe_ops);
>  
> -	ret = trace_kprobe_create(argc, (const char **)argv);
> +	ret = trace_kprobe_create(raw_command);
>  	return ret == -ECANCELED ? -EINVAL : ret;
>  }
>  
>  static int trace_kprobe_run_command(struct dynevent_cmd *cmd)
>  {
> -	return trace_run_command(cmd->seq.buffer, create_or_delete_trace_kprobe);
> +	return create_or_delete_trace_kprobe(cmd->seq.buffer);
>  }
>  
>  /**
> @@ -1081,7 +1086,7 @@ int kprobe_event_delete(const char *name)
>  
>  	snprintf(buf, MAX_EVENT_NAME_LEN, "-:%s", name);
>  
> -	return trace_run_command(buf, create_or_delete_trace_kprobe);
> +	return create_or_delete_trace_kprobe(buf);
>  }
>  EXPORT_SYMBOL_GPL(kprobe_event_delete);
>  
> @@ -1884,7 +1889,7 @@ static __init void setup_boot_kprobe_events(void)
>  		if (p)
>  			*p++ = '\0';
>  
> -		ret = trace_run_command(cmd, create_or_delete_trace_kprobe);
> +		ret = create_or_delete_trace_kprobe(cmd);
>  		if (ret)
>  			pr_warn("Failed to add event(%d): %s\n", ret, cmd);
>  		else
> @@ -1982,8 +1987,7 @@ static __init int kprobe_trace_self_tests_init(void)
>  
>  	pr_info("Testing kprobe tracing: ");
>  
> -	ret = trace_run_command("p:testprobe kprobe_trace_selftest_target $stack $stack0 +0($stack)",
> -				create_or_delete_trace_kprobe);
> +	ret = create_or_delete_trace_kprobe("p:testprobe kprobe_trace_selftest_target $stack $stack0 +0($stack)");
>  	if (WARN_ON_ONCE(ret)) {
>  		pr_warn("error on probing function entry.\n");
>  		warn++;
> @@ -2004,8 +2008,7 @@ static __init int kprobe_trace_self_tests_init(void)
>  		}
>  	}
>  
> -	ret = trace_run_command("r:testprobe2 kprobe_trace_selftest_target $retval",
> -				create_or_delete_trace_kprobe);
> +	ret = create_or_delete_trace_kprobe("r:testprobe2 kprobe_trace_selftest_target $retval");
>  	if (WARN_ON_ONCE(ret)) {
>  		pr_warn("error on probing function return.\n");
>  		warn++;
> @@ -2078,13 +2081,13 @@ static __init int kprobe_trace_self_tests_init(void)
>  				trace_probe_event_call(&tk->tp), file);
>  	}
>  
> -	ret = trace_run_command("-:testprobe", create_or_delete_trace_kprobe);
> +	ret = create_or_delete_trace_kprobe("-:testprobe");
>  	if (WARN_ON_ONCE(ret)) {
>  		pr_warn("error on deleting a probe.\n");
>  		warn++;
>  	}
>  
> -	ret = trace_run_command("-:testprobe2", create_or_delete_trace_kprobe);
> +	ret = create_or_delete_trace_kprobe("-:testprobe2");
>  	if (WARN_ON_ONCE(ret)) {
>  		pr_warn("error on deleting a probe.\n");
>  		warn++;
> diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> index d2867ccc6aca..ec589a4612df 100644
> --- a/kernel/trace/trace_probe.c
> +++ b/kernel/trace/trace_probe.c
> @@ -1134,3 +1134,20 @@ bool trace_probe_match_command_args(struct trace_probe *tp,
>  	}
>  	return true;
>  }
> +
> +int trace_probe_create(const char *raw_command, int (*createfn)(int, const char **))
> +{
> +	int argc = 0, ret = 0;
> +	char **argv;
> +
> +	argv = argv_split(GFP_KERNEL, raw_command, &argc);
> +	if (!argv)
> +		return -ENOMEM;
> +
> +	if (argc)
> +		ret = createfn(argc, (const char **)argv);
> +
> +	argv_free(argv);
> +
> +	return ret;
> +}
> diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
> index 2f703a20c724..7ce4027089ee 100644
> --- a/kernel/trace/trace_probe.h
> +++ b/kernel/trace/trace_probe.h
> @@ -341,6 +341,7 @@ struct event_file_link *trace_probe_get_file_link(struct trace_probe *tp,
>  int trace_probe_compare_arg_type(struct trace_probe *a, struct trace_probe *b);
>  bool trace_probe_match_command_args(struct trace_probe *tp,
>  				    int argc, const char **argv);
> +int trace_probe_create(const char *raw_command, int (*createfn)(int, const char **));
>  
>  #define trace_probe_for_each_link(pos, tp)	\
>  	list_for_each_entry(pos, &(tp)->event->files, list)
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index 3cf7128e1ad3..e6b56a65f80f 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -34,7 +34,7 @@ struct uprobe_trace_entry_head {
>  #define DATAOF_TRACE_ENTRY(entry, is_return)		\
>  	((void*)(entry) + SIZEOF_TRACE_ENTRY(is_return))
>  
> -static int trace_uprobe_create(int argc, const char **argv);
> +static int trace_uprobe_create(const char *raw_command);
>  static int trace_uprobe_show(struct seq_file *m, struct dyn_event *ev);
>  static int trace_uprobe_release(struct dyn_event *ev);
>  static bool trace_uprobe_is_busy(struct dyn_event *ev);
> @@ -530,7 +530,7 @@ static int register_trace_uprobe(struct trace_uprobe *tu)
>   * Argument syntax:
>   *  - Add uprobe: p|r[:[GRP/]EVENT] PATH:OFFSET[%return][(REF)] [FETCHARGS]
>   */
> -static int trace_uprobe_create(int argc, const char **argv)
> +static int __trace_uprobe_create(int argc, const char **argv)
>  {
>  	struct trace_uprobe *tu;
>  	const char *event = NULL, *group = UPROBE_EVENT_SYSTEM;
> @@ -716,14 +716,19 @@ static int trace_uprobe_create(int argc, const char **argv)
>  	return ret;
>  }
>  
> -static int create_or_delete_trace_uprobe(int argc, char **argv)
> +int trace_uprobe_create(const char *raw_command)
> +{
> +	return trace_probe_create(raw_command, __trace_uprobe_create);
> +}
> +
> +static int create_or_delete_trace_uprobe(const char *raw_command)
>  {
>  	int ret;
>  
> -	if (argv[0][0] == '-')
> -		return dyn_event_release(argc, argv, &trace_uprobe_ops);
> +	if (raw_command[0] == '-')
> +		return dyn_event_release(raw_command, &trace_uprobe_ops);
>  
> -	ret = trace_uprobe_create(argc, (const char **)argv);
> +	ret = trace_uprobe_create(raw_command);
>  	return ret == -ECANCELED ? -EINVAL : ret;
>  }
>  

Re: [PATCH v3 2/5] tracing: Rework synthetic event command parsing

[Steven Rostedt]

On Mon, 26 Oct 2020 10:06:10 -0500
Tom Zanussi <zanussi@kernel.org> wrote:

> Now that command parsing has been delegated to the create functions
> and we're no longer constrained by argv_split(), we can modify the
> synthetic event command parser to better match the higher-level
> structure of the synthetic event commands, which is basically an event
> name followed by a set of semicolon-separated fields.
> 
> Since we're also now passed the raw command, we can also save it
> directly and can get rid of save_cmdstr().
> 
> Signed-off-by: Tom Zanussi <zanussi@kernel.org>
> ---
> 

This patch fails to build with:

  CC      kernel/trace/trace_events_synth.o
/work/git/linux-trace.git/kernel/trace/trace_events_synth.c: In function ‘create_or_delete_synth_event’:
/work/git/linux-trace.git/kernel/trace/trace_events_synth.c:1372:8: error: implicit declaration of function ‘check_command’ [-Werror=implicit-function-declaration]
 1372 |  ret = check_command(raw_command);
      |        ^~~~~~~~~~~~~
cc1: some warnings being treated as errors
make[3]: *** [/work/git/linux-trace.git/scripts/Makefile.build:283: kernel/trace/trace_events_synth.o] Error 1
make[2]: *** [/work/git/linux-trace.git/scripts/Makefile.build:500: kernel/trace] Error 2
make[1]: *** [/work/git/linux-trace.git/Makefile:1799: kernel] Error 2
make[1]: Leaving directory '/work/git/nobackup/bxtest/trace'
make: *** [Makefile:185: __sub-make] Error 2

-- Steve

Re: [PATCH v3 3/5] tracing: Update synth command errors

[Steven Rostedt]

On Mon, 26 Oct 2020 10:06:11 -0500
Tom Zanussi <zanussi@kernel.org> wrote:

> Since array types are handled differently, errors referencing them
> also need to be handled differently.  Add and use a new
> INVALID_ARRAY_SPEC error.  Also add INVALID_CMD and INVALID_DYN_CMD to
> catch and display the correct form for badly-formed commands, which
> can also be used in place of CMD_INCOMPLETE, which is removed, and
> remove CMD_TOO_LONG, since it's no longer used.
> 
> Signed-off-by: Tom Zanussi <zanussi@kernel.org>
> ---

Unfortunately, this patch series breaks user space.

I already have scripts that do the histograms, and I'm sure others may
have that too, and if we change how synthetic events are created, it
will break them.

What's the rationale for the new delimiters?

-- Steve

Re: [PATCH v3 1/5] tracing/dynevent: Delegate parsing to create function

[Masami Hiramatsu]

On Mon, 7 Dec 2020 18:33:22 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> 
> Hi Masami,
> 
> You had comments on this patch for v2. Is this one fine for you?

Yes, this part is good for me. v2 [1/4] is separated into v3 [1/5] and [2/5].

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

Thank you,

> 
> -- Steve
> 
> 
> On Mon, 26 Oct 2020 10:06:09 -0500
> Tom Zanussi <zanussi@kernel.org> wrote:
> 
> > From: Masami Hiramatsu <mhiramat@kernel.org>
> > 
> > Delegate command parsing to each create function so that the
> > command syntax can be customized.
> > 
> > This requires changes to the kprobe/uprobe/synthetic event handling,
> > which are also included here.
> > 
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > [ zanussi@kernel.org: added synthetic event modifications ]
> > Signed-off-by: Tom Zanussi <zanussi@kernel.org>
> > ---
> >  kernel/trace/trace.c              | 23 ++----------
> >  kernel/trace/trace.h              |  3 +-
> >  kernel/trace/trace_dynevent.c     | 35 +++++++++++-------
> >  kernel/trace/trace_dynevent.h     |  4 +--
> >  kernel/trace/trace_events_synth.c | 60 +++++++++++++++++++++++--------
> >  kernel/trace/trace_kprobe.c       | 33 +++++++++--------
> >  kernel/trace/trace_probe.c        | 17 +++++++++
> >  kernel/trace/trace_probe.h        |  1 +
> >  kernel/trace/trace_uprobe.c       | 17 +++++----
> >  9 files changed, 120 insertions(+), 73 deletions(-)
> > 
> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > index 63c97012ed39..277d97220971 100644
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c
> > @@ -9367,30 +9367,11 @@ void ftrace_dump(enum ftrace_dump_mode oops_dump_mode)
> >  }
> >  EXPORT_SYMBOL_GPL(ftrace_dump);
> >  
> > -int trace_run_command(const char *buf, int (*createfn)(int, char **))
> > -{
> > -	char **argv;
> > -	int argc, ret;
> > -
> > -	argc = 0;
> > -	ret = 0;
> > -	argv = argv_split(GFP_KERNEL, buf, &argc);
> > -	if (!argv)
> > -		return -ENOMEM;
> > -
> > -	if (argc)
> > -		ret = createfn(argc, argv);
> > -
> > -	argv_free(argv);
> > -
> > -	return ret;
> > -}
> > -
> >  #define WRITE_BUFSIZE  4096
> >  
> >  ssize_t trace_parse_run_command(struct file *file, const char __user *buffer,
> >  				size_t count, loff_t *ppos,
> > -				int (*createfn)(int, char **))
> > +				int (*createfn)(const char *))
> >  {
> >  	char *kbuf, *buf, *tmp;
> >  	int ret = 0;
> > @@ -9438,7 +9419,7 @@ ssize_t trace_parse_run_command(struct file *file, const char __user *buffer,
> >  			if (tmp)
> >  				*tmp = '\0';
> >  
> > -			ret = trace_run_command(buf, createfn);
> > +			ret = createfn(buf);
> >  			if (ret)
> >  				goto out;
> >  			buf += size;
> > diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> > index 34e0c4d5a6e7..02d7c487a30b 100644
> > --- a/kernel/trace/trace.h
> > +++ b/kernel/trace/trace.h
> > @@ -1982,10 +1982,9 @@ extern int tracing_set_cpumask(struct trace_array *tr,
> >  
> >  #define MAX_EVENT_NAME_LEN	64
> >  
> > -extern int trace_run_command(const char *buf, int (*createfn)(int, char**));
> >  extern ssize_t trace_parse_run_command(struct file *file,
> >  		const char __user *buffer, size_t count, loff_t *ppos,
> > -		int (*createfn)(int, char**));
> > +		int (*createfn)(const char *));
> >  
> >  extern unsigned int err_pos(char *cmd, const char *str);
> >  extern void tracing_log_err(struct trace_array *tr,
> > diff --git a/kernel/trace/trace_dynevent.c b/kernel/trace/trace_dynevent.c
> > index 5fa49cfd2bb6..af83bc5447fe 100644
> > --- a/kernel/trace/trace_dynevent.c
> > +++ b/kernel/trace/trace_dynevent.c
> > @@ -31,23 +31,31 @@ int dyn_event_register(struct dyn_event_operations *ops)
> >  	return 0;
> >  }
> >  
> > -int dyn_event_release(int argc, char **argv, struct dyn_event_operations *type)
> > +int dyn_event_release(const char *raw_command, struct dyn_event_operations *type)
> >  {
> >  	struct dyn_event *pos, *n;
> >  	char *system = NULL, *event, *p;
> > -	int ret = -ENOENT;
> > +	int argc, ret = -ENOENT;
> > +	char **argv;
> > +
> > +	argv = argv_split(GFP_KERNEL, raw_command, &argc);
> > +	if (!argv)
> > +		return -ENOMEM;
> >  
> >  	if (argv[0][0] == '-') {
> > -		if (argv[0][1] != ':')
> > -			return -EINVAL;
> > +		if (argv[0][1] != ':') {
> > +			ret = -EINVAL;
> > +			goto out;
> > +		}
> >  		event = &argv[0][2];
> >  	} else {
> >  		event = strchr(argv[0], ':');
> > -		if (!event)
> > -			return -EINVAL;
> > +		if (!event) {
> > +			ret = -EINVAL;
> > +			goto out;
> > +		}
> >  		event++;
> >  	}
> > -	argc--; argv++;
> >  
> >  	p = strchr(event, '/');
> >  	if (p) {
> > @@ -63,7 +71,7 @@ int dyn_event_release(int argc, char **argv, struct dyn_event_operations *type)
> >  		if (type && type != pos->ops)
> >  			continue;
> >  		if (!pos->ops->match(system, event,
> > -				argc, (const char **)argv, pos))
> > +				argc - 1, (const char **)argv + 1, pos))
> >  			continue;
> >  
> >  		ret = pos->ops->free(pos);
> > @@ -71,21 +79,22 @@ int dyn_event_release(int argc, char **argv, struct dyn_event_operations *type)
> >  			break;
> >  	}
> >  	mutex_unlock(&event_mutex);
> > -
> > +out:
> > +	argv_free(argv);
> >  	return ret;
> >  }
> >  
> > -static int create_dyn_event(int argc, char **argv)
> > +static int create_dyn_event(const char *raw_command)
> >  {
> >  	struct dyn_event_operations *ops;
> >  	int ret = -ENODEV;
> >  
> > -	if (argv[0][0] == '-' || argv[0][0] == '!')
> > -		return dyn_event_release(argc, argv, NULL);
> > +	if (raw_command[0] == '-' || raw_command[0] == '!')
> > +		return dyn_event_release(raw_command, NULL);
> >  
> >  	mutex_lock(&dyn_event_ops_mutex);
> >  	list_for_each_entry(ops, &dyn_event_ops_list, list) {
> > -		ret = ops->create(argc, (const char **)argv);
> > +		ret = ops->create(raw_command);
> >  		if (!ret || ret != -ECANCELED)
> >  			break;
> >  	}
> > diff --git a/kernel/trace/trace_dynevent.h b/kernel/trace/trace_dynevent.h
> > index d6857a254ede..4f4e03df4cbb 100644
> > --- a/kernel/trace/trace_dynevent.h
> > +++ b/kernel/trace/trace_dynevent.h
> > @@ -39,7 +39,7 @@ struct dyn_event;
> >   */
> >  struct dyn_event_operations {
> >  	struct list_head	list;
> > -	int (*create)(int argc, const char *argv[]);
> > +	int (*create)(const char *raw_command);
> >  	int (*show)(struct seq_file *m, struct dyn_event *ev);
> >  	bool (*is_busy)(struct dyn_event *ev);
> >  	int (*free)(struct dyn_event *ev);
> > @@ -97,7 +97,7 @@ void *dyn_event_seq_start(struct seq_file *m, loff_t *pos);
> >  void *dyn_event_seq_next(struct seq_file *m, void *v, loff_t *pos);
> >  void dyn_event_seq_stop(struct seq_file *m, void *v);
> >  int dyn_events_release_all(struct dyn_event_operations *type);
> > -int dyn_event_release(int argc, char **argv, struct dyn_event_operations *type);
> > +int dyn_event_release(const char *raw_command, struct dyn_event_operations *type);
> >  
> >  /*
> >   * for_each_dyn_event	-	iterate over the dyn_event list
> > diff --git a/kernel/trace/trace_events_synth.c b/kernel/trace/trace_events_synth.c
> > index bdd427ccdfc5..271811fbf8fb 100644
> > --- a/kernel/trace/trace_events_synth.c
> > +++ b/kernel/trace/trace_events_synth.c
> > @@ -62,7 +62,7 @@ static void synth_err(u8 err_type, u8 err_pos)
> >  			err_type, err_pos);
> >  }
> >  
> > -static int create_synth_event(int argc, const char **argv);
> > +static int create_synth_event(const char *raw_command);
> >  static int synth_event_show(struct seq_file *m, struct dyn_event *ev);
> >  static int synth_event_release(struct dyn_event *ev);
> >  static bool synth_event_is_busy(struct dyn_event *ev);
> > @@ -1385,18 +1385,30 @@ int synth_event_delete(const char *event_name)
> >  }
> >  EXPORT_SYMBOL_GPL(synth_event_delete);
> >  
> > -static int create_or_delete_synth_event(int argc, char **argv)
> > +static int create_or_delete_synth_event(const char *raw_command)
> >  {
> > -	const char *name = argv[0];
> > -	int ret;
> > +	char **argv, *name = NULL;
> > +	int argc = 0, ret = 0;
> > +
> > +	argv = argv_split(GFP_KERNEL, raw_command, &argc);
> > +	if (!argv)
> > +		return -ENOMEM;
> > +
> > +	if (!argc)
> > +		goto free;
> > +
> > +	name = argv[0];
> >  
> >  	/* trace_run_command() ensures argc != 0 */
> >  	if (name[0] == '!') {
> >  		ret = synth_event_delete(name + 1);
> > -		return ret;
> > +		goto free;
> >  	}
> >  
> >  	ret = __create_synth_event(argc - 1, name, (const char **)argv + 1);
> > +free:
> > +	argv_free(argv);
> > +
> >  	return ret == -ECANCELED ? -EINVAL : ret;
> >  }
> >  
> > @@ -1405,7 +1417,7 @@ static int synth_event_run_command(struct dynevent_cmd *cmd)
> >  	struct synth_event *se;
> >  	int ret;
> >  
> > -	ret = trace_run_command(cmd->seq.buffer, create_or_delete_synth_event);
> > +	ret = create_or_delete_synth_event(cmd->seq.buffer);
> >  	if (ret)
> >  		return ret;
> >  
> > @@ -1941,23 +1953,43 @@ int synth_event_trace_end(struct synth_event_trace_state *trace_state)
> >  }
> >  EXPORT_SYMBOL_GPL(synth_event_trace_end);
> >  
> > -static int create_synth_event(int argc, const char **argv)
> > +static int create_synth_event(const char *raw_command)
> >  {
> > -	const char *name = argv[0];
> > -	int len;
> > +	char **argv, *name;
> > +	int len, argc = 0, ret = 0;
> > +
> > +	argv = argv_split(GFP_KERNEL, raw_command, &argc);
> > +	if (!argv) {
> > +		ret = -ENOMEM;
> > +		return ret;
> > +	}
> >  
> > -	if (name[0] != 's' || name[1] != ':')
> > -		return -ECANCELED;
> > +	if (!argc)
> > +		goto free;
> > +
> > +	name = argv[0];
> > +
> > +	if (name[0] != 's' || name[1] != ':') {
> > +		ret = -ECANCELED;
> > +		goto free;
> > +	}
> >  	name += 2;
> >  
> >  	/* This interface accepts group name prefix */
> >  	if (strchr(name, '/')) {
> >  		len = str_has_prefix(name, SYNTH_SYSTEM "/");
> > -		if (len == 0)
> > -			return -EINVAL;
> > +		if (len == 0) {
> > +			ret = -EINVAL;
> > +			goto free;
> > +		}
> >  		name += len;
> >  	}
> > -	return __create_synth_event(argc - 1, name, argv + 1);
> > +
> > +	ret = __create_synth_event(argc - 1, name, (const char **)argv + 1);
> > +free:
> > +	argv_free(argv);
> > +
> > +	return ret;
> >  }
> >  
> >  static int synth_event_release(struct dyn_event *ev)
> > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> > index b911e9f6d9f5..ddef93e32905 100644
> > --- a/kernel/trace/trace_kprobe.c
> > +++ b/kernel/trace/trace_kprobe.c
> > @@ -34,7 +34,7 @@ static int __init set_kprobe_boot_events(char *str)
> >  }
> >  __setup("kprobe_event=", set_kprobe_boot_events);
> >  
> > -static int trace_kprobe_create(int argc, const char **argv);
> > +static int trace_kprobe_create(const char *raw_command);
> >  static int trace_kprobe_show(struct seq_file *m, struct dyn_event *ev);
> >  static int trace_kprobe_release(struct dyn_event *ev);
> >  static bool trace_kprobe_is_busy(struct dyn_event *ev);
> > @@ -710,7 +710,7 @@ static inline void sanitize_event_name(char *name)
> >  			*name = '_';
> >  }
> >  
> > -static int trace_kprobe_create(int argc, const char *argv[])
> > +static int __trace_kprobe_create(int argc, const char *argv[])
> >  {
> >  	/*
> >  	 * Argument syntax:
> > @@ -907,20 +907,25 @@ static int trace_kprobe_create(int argc, const char *argv[])
> >  	goto out;
> >  }
> >  
> > -static int create_or_delete_trace_kprobe(int argc, char **argv)
> > +static int trace_kprobe_create(const char *raw_command)
> > +{
> > +	return trace_probe_create(raw_command, __trace_kprobe_create);
> > +}
> > +
> > +static int create_or_delete_trace_kprobe(const char *raw_command)
> >  {
> >  	int ret;
> >  
> > -	if (argv[0][0] == '-')
> > -		return dyn_event_release(argc, argv, &trace_kprobe_ops);
> > +	if (raw_command[0] == '-')
> > +		return dyn_event_release(raw_command, &trace_kprobe_ops);
> >  
> > -	ret = trace_kprobe_create(argc, (const char **)argv);
> > +	ret = trace_kprobe_create(raw_command);
> >  	return ret == -ECANCELED ? -EINVAL : ret;
> >  }
> >  
> >  static int trace_kprobe_run_command(struct dynevent_cmd *cmd)
> >  {
> > -	return trace_run_command(cmd->seq.buffer, create_or_delete_trace_kprobe);
> > +	return create_or_delete_trace_kprobe(cmd->seq.buffer);
> >  }
> >  
> >  /**
> > @@ -1081,7 +1086,7 @@ int kprobe_event_delete(const char *name)
> >  
> >  	snprintf(buf, MAX_EVENT_NAME_LEN, "-:%s", name);
> >  
> > -	return trace_run_command(buf, create_or_delete_trace_kprobe);
> > +	return create_or_delete_trace_kprobe(buf);
> >  }
> >  EXPORT_SYMBOL_GPL(kprobe_event_delete);
> >  
> > @@ -1884,7 +1889,7 @@ static __init void setup_boot_kprobe_events(void)
> >  		if (p)
> >  			*p++ = '\0';
> >  
> > -		ret = trace_run_command(cmd, create_or_delete_trace_kprobe);
> > +		ret = create_or_delete_trace_kprobe(cmd);
> >  		if (ret)
> >  			pr_warn("Failed to add event(%d): %s\n", ret, cmd);
> >  		else
> > @@ -1982,8 +1987,7 @@ static __init int kprobe_trace_self_tests_init(void)
> >  
> >  	pr_info("Testing kprobe tracing: ");
> >  
> > -	ret = trace_run_command("p:testprobe kprobe_trace_selftest_target $stack $stack0 +0($stack)",
> > -				create_or_delete_trace_kprobe);
> > +	ret = create_or_delete_trace_kprobe("p:testprobe kprobe_trace_selftest_target $stack $stack0 +0($stack)");
> >  	if (WARN_ON_ONCE(ret)) {
> >  		pr_warn("error on probing function entry.\n");
> >  		warn++;
> > @@ -2004,8 +2008,7 @@ static __init int kprobe_trace_self_tests_init(void)
> >  		}
> >  	}
> >  
> > -	ret = trace_run_command("r:testprobe2 kprobe_trace_selftest_target $retval",
> > -				create_or_delete_trace_kprobe);
> > +	ret = create_or_delete_trace_kprobe("r:testprobe2 kprobe_trace_selftest_target $retval");
> >  	if (WARN_ON_ONCE(ret)) {
> >  		pr_warn("error on probing function return.\n");
> >  		warn++;
> > @@ -2078,13 +2081,13 @@ static __init int kprobe_trace_self_tests_init(void)
> >  				trace_probe_event_call(&tk->tp), file);
> >  	}
> >  
> > -	ret = trace_run_command("-:testprobe", create_or_delete_trace_kprobe);
> > +	ret = create_or_delete_trace_kprobe("-:testprobe");
> >  	if (WARN_ON_ONCE(ret)) {
> >  		pr_warn("error on deleting a probe.\n");
> >  		warn++;
> >  	}
> >  
> > -	ret = trace_run_command("-:testprobe2", create_or_delete_trace_kprobe);
> > +	ret = create_or_delete_trace_kprobe("-:testprobe2");
> >  	if (WARN_ON_ONCE(ret)) {
> >  		pr_warn("error on deleting a probe.\n");
> >  		warn++;
> > diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> > index d2867ccc6aca..ec589a4612df 100644
> > --- a/kernel/trace/trace_probe.c
> > +++ b/kernel/trace/trace_probe.c
> > @@ -1134,3 +1134,20 @@ bool trace_probe_match_command_args(struct trace_probe *tp,
> >  	}
> >  	return true;
> >  }
> > +
> > +int trace_probe_create(const char *raw_command, int (*createfn)(int, const char **))
> > +{
> > +	int argc = 0, ret = 0;
> > +	char **argv;
> > +
> > +	argv = argv_split(GFP_KERNEL, raw_command, &argc);
> > +	if (!argv)
> > +		return -ENOMEM;
> > +
> > +	if (argc)
> > +		ret = createfn(argc, (const char **)argv);
> > +
> > +	argv_free(argv);
> > +
> > +	return ret;
> > +}
> > diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
> > index 2f703a20c724..7ce4027089ee 100644
> > --- a/kernel/trace/trace_probe.h
> > +++ b/kernel/trace/trace_probe.h
> > @@ -341,6 +341,7 @@ struct event_file_link *trace_probe_get_file_link(struct trace_probe *tp,
> >  int trace_probe_compare_arg_type(struct trace_probe *a, struct trace_probe *b);
> >  bool trace_probe_match_command_args(struct trace_probe *tp,
> >  				    int argc, const char **argv);
> > +int trace_probe_create(const char *raw_command, int (*createfn)(int, const char **));
> >  
> >  #define trace_probe_for_each_link(pos, tp)	\
> >  	list_for_each_entry(pos, &(tp)->event->files, list)
> > diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> > index 3cf7128e1ad3..e6b56a65f80f 100644
> > --- a/kernel/trace/trace_uprobe.c
> > +++ b/kernel/trace/trace_uprobe.c
> > @@ -34,7 +34,7 @@ struct uprobe_trace_entry_head {
> >  #define DATAOF_TRACE_ENTRY(entry, is_return)		\
> >  	((void*)(entry) + SIZEOF_TRACE_ENTRY(is_return))
> >  
> > -static int trace_uprobe_create(int argc, const char **argv);
> > +static int trace_uprobe_create(const char *raw_command);
> >  static int trace_uprobe_show(struct seq_file *m, struct dyn_event *ev);
> >  static int trace_uprobe_release(struct dyn_event *ev);
> >  static bool trace_uprobe_is_busy(struct dyn_event *ev);
> > @@ -530,7 +530,7 @@ static int register_trace_uprobe(struct trace_uprobe *tu)
> >   * Argument syntax:
> >   *  - Add uprobe: p|r[:[GRP/]EVENT] PATH:OFFSET[%return][(REF)] [FETCHARGS]
> >   */
> > -static int trace_uprobe_create(int argc, const char **argv)
> > +static int __trace_uprobe_create(int argc, const char **argv)
> >  {
> >  	struct trace_uprobe *tu;
> >  	const char *event = NULL, *group = UPROBE_EVENT_SYSTEM;
> > @@ -716,14 +716,19 @@ static int trace_uprobe_create(int argc, const char **argv)
> >  	return ret;
> >  }
> >  
> > -static int create_or_delete_trace_uprobe(int argc, char **argv)
> > +int trace_uprobe_create(const char *raw_command)
> > +{
> > +	return trace_probe_create(raw_command, __trace_uprobe_create);
> > +}
> > +
> > +static int create_or_delete_trace_uprobe(const char *raw_command)
> >  {
> >  	int ret;
> >  
> > -	if (argv[0][0] == '-')
> > -		return dyn_event_release(argc, argv, &trace_uprobe_ops);
> > +	if (raw_command[0] == '-')
> > +		return dyn_event_release(raw_command, &trace_uprobe_ops);
> >  
> > -	ret = trace_uprobe_create(argc, (const char **)argv);
> > +	ret = trace_uprobe_create(raw_command);
> >  	return ret == -ECANCELED ? -EINVAL : ret;
> >  }
> >  
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v3 3/5] tracing: Update synth command errors

[Tom Zanussi]

Hi Steve,

On Mon, 2020-12-07 at 20:13 -0500, Steven Rostedt wrote:
> On Mon, 26 Oct 2020 10:06:11 -0500
> Tom Zanussi <zanussi@kernel.org> wrote:
> 
> > Since array types are handled differently, errors referencing them
> > also need to be handled differently.  Add and use a new
> > INVALID_ARRAY_SPEC error.  Also add INVALID_CMD and INVALID_DYN_CMD
> > to
> > catch and display the correct form for badly-formed commands, which
> > can also be used in place of CMD_INCOMPLETE, which is removed, and
> > remove CMD_TOO_LONG, since it's no longer used.
> > 
> > Signed-off-by: Tom Zanussi <zanussi@kernel.org>
> > ---
> 
> Unfortunately, this patch series breaks user space.
> 
> I already have scripts that do the histograms, and I'm sure others
> may
> have that too, and if we change how synthetic events are created, it
> will break them.
> 
> What's the rationale for the new delimiters?
> 

The overall problem this is trying to fix is that it was probably a
mistake to try to shoehorn the synthetic event parsing into what was
available from  trace_run_command() and trace_parse_run_command(),
which use argv_split() to do the command splitting, and which only
splits on whitespace.  Whereas the synthetic events have a bit of a
higher-level structure which is 'event field; field; field;...'

So this patchset tries to remedy that - the first patch,
(tracing/dynevent: Delegate parsing to create function) is from Masami,
and makes it possible to share code between kprobe/uprobe and synthetic
evnents, and to allow synthetic events to have their own higher-level
parsing, which the next 2 patches do.

The history in more detail:

Initially the problem was to fix the errors mentioned by Masami in
[1]. 

Things like:

  # echo myevent char str[];; int v >> synthetic_events

which was identified as INVALID_TYPE where it should just be a void arg
and

  # echo mye;vent char str[] >> synthetic_events

which was identified as BAD_NAME where it should have been an invalid
command, etc.

I suggested that the way to fix them was to consider semicolon as
additional whitespace and the result was the patchset containing [2],
which also explains the reasons for wanting to enforce semicolon
grouping.

Masami pointed out that it really wasn't correct to do it that way,
and  the commands should be split out first at the higher level by
semicolon and then further processed [3].

Unfortunately, you're correct, if you have a script that creates a
synthetic event without semicolons, this patchset will break it, as I
myself found out and fixed in patch 4 ([PATCH v3 4/5] selftests/ftrace:
Add synthetic event field separators) [4].

So whereas before this would work, even though it shouldn't have in the
first place:

  # echo 'wakeup_latency  u64 lat pid_t pid char comm[16]' >
synthetic_events

it now has to be:

  # echo 'wakeup_latency  u64 lat; pid_t pid; char comm[16]' >
synthetic_events

So yeah, this patchset fixes a set of parsing bugs for things that
shouldn't have been accepted as valid, but shouldn't break things that
are obviously valid.

If it's too late to fix them, though, I guess we'll just have to live
with them, or some other option?

Tom

[1] https://lore.kernel.org/lkml/20201014110636.139df7be275d40a23b523b84@kernel.org/
[2] https://lore.kernel.org/lkml/e29c3ae1fc46892ec792d6f6f910f75d0e12584c.1602883818.git.zanussi@kernel.org/
[3] https://lore.kernel.org/lkml/20201018232011.38e5da51f5cd8e73e6f529ee@kernel.org/
[4] https://lore.kernel.org/lkml/75a2816b4001e04e7d60bcc87aa91477ad5d90b3.1603723933.git.zanussi@kernel.org/



> -- Steve

Re: [PATCH v3 2/5] tracing: Rework synthetic event command parsing

[Tom Zanussi]

Hi Steve,

On Mon, 2020-12-07 at 19:16 -0500, Steven Rostedt wrote:
> On Mon, 26 Oct 2020 10:06:10 -0500
> Tom Zanussi <zanussi@kernel.org> wrote:
> 
> > Now that command parsing has been delegated to the create functions
> > and we're no longer constrained by argv_split(), we can modify the
> > synthetic event command parser to better match the higher-level
> > structure of the synthetic event commands, which is basically an
> > event
> > name followed by a set of semicolon-separated fields.
> > 
> > Since we're also now passed the raw command, we can also save it
> > directly and can get rid of save_cmdstr().
> > 
> > Signed-off-by: Tom Zanussi <zanussi@kernel.org>
> > ---
> > 
> 
> This patch fails to build with:
> 
>   CC      kernel/trace/trace_events_synth.o
> /work/git/linux-trace.git/kernel/trace/trace_events_synth.c: In
> function ‘create_or_delete_synth_event’:
> /work/git/linux-trace.git/kernel/trace/trace_events_synth.c:1372:8:
> error: implicit declaration of function ‘check_command’ [-
> Werror=implicit-function-declaration]
>  1372 |  ret = check_command(raw_command);
>       |        ^~~~~~~~~~~~~
> cc1: some warnings being treated as errors
> make[3]: *** [/work/git/linux-trace.git/scripts/Makefile.build:283:
> kernel/trace/trace_events_synth.o] Error 1
> make[2]: *** [/work/git/linux-trace.git/scripts/Makefile.build:500:
> kernel/trace] Error 2
> make[1]: *** [/work/git/linux-trace.git/Makefile:1799: kernel] Error
> 2
> make[1]: Leaving directory '/work/git/nobackup/bxtest/trace'
> make: *** [Makefile:185: __sub-make] Error 2
> 

Oops, yeah, a stray check_command() call got left behind in that patch
when refactoring, should be moved to the next patch.

Tom

> -- Steve

Re: [PATCH v3 3/5] tracing: Update synth command errors

[Steven Rostedt]

On Tue, 08 Dec 2020 11:34:41 -0600
Tom Zanussi <zanussi@kernel.org> wrote:

> Unfortunately, you're correct, if you have a script that creates a
> synthetic event without semicolons, this patchset will break it, as I
> myself found out and fixed in patch 4 ([PATCH v3 4/5] selftests/ftrace:
> Add synthetic event field separators) [4].
> 
> So whereas before this would work, even though it shouldn't have in the
> first place:
> 
>   # echo 'wakeup_latency  u64 lat pid_t pid char comm[16]' >
> synthetic_events
> 
> it now has to be:
> 
>   # echo 'wakeup_latency  u64 lat; pid_t pid; char comm[16]' >
> synthetic_events
> 
> So yeah, this patchset fixes a set of parsing bugs for things that
> shouldn't have been accepted as valid, but shouldn't break things that
> are obviously valid.
> 
> If it's too late to fix them, though, I guess we'll just have to live
> with them, or some other option?


I would suggest allowing the old interface work (with no new features, for
backward compatibility), but new things like "char comm[16]" we require
semicolons.

One method to do this is to add to the start of reading the string, and
checking if it has semicolons. If it does not, we create a new string with
them, but make sure that the string does not include new changes.

	strncpy_from_user(buffer, user_buff, sizeof(buffer));

	if (!strstr(buffer, ";")) {
		if (!audit_old_buffer(buffer))
			goto error;
		insert_colons(buffer);
	}


That is, if the buffer does not have semicolons, then check if it is a
valid "old format", and if not, we error out. Otherwise, we insert the
colons into the buffer, and process that as if the user put in colons:

That is:

	echo 'wakeup_latency u64 lat pid_t pid' > synthetic_events

would change the buffer to:

	"wakeup_latency u64 lat; pid_t pid;"

And then put it through the normal processing. I think its OK that if the
user were to cat out the synthetic events, it would see the semicolons even
if it did not add them. As I don't think that will break userspace.

Does that make sense?

-- Steve

Re: [PATCH v3 3/5] tracing: Update synth command errors

[Tom Zanussi]

Hi Steve,

On Tue, 2020-12-08 at 12:53 -0500, Steven Rostedt wrote:
> On Tue, 08 Dec 2020 11:34:41 -0600
> Tom Zanussi <zanussi@kernel.org> wrote:
> 
> > Unfortunately, you're correct, if you have a script that creates a
> > synthetic event without semicolons, this patchset will break it, as
> > I
> > myself found out and fixed in patch 4 ([PATCH v3 4/5]
> > selftests/ftrace:
> > Add synthetic event field separators) [4].
> > 
> > So whereas before this would work, even though it shouldn't have in
> > the
> > first place:
> > 
> >   # echo 'wakeup_latency  u64 lat pid_t pid char comm[16]' >
> > synthetic_events
> > 
> > it now has to be:
> > 
> >   # echo 'wakeup_latency  u64 lat; pid_t pid; char comm[16]' >
> > synthetic_events
> > 
> > So yeah, this patchset fixes a set of parsing bugs for things that
> > shouldn't have been accepted as valid, but shouldn't break things
> > that
> > are obviously valid.
> > 
> > If it's too late to fix them, though, I guess we'll just have to
> > live
> > with them, or some other option?
> 
> 
> I would suggest allowing the old interface work (with no new
> features, for
> backward compatibility), but new things like "char comm[16]" we
> require
> semicolons.
> 
> One method to do this is to add to the start of reading the string,
> and
> checking if it has semicolons. If it does not, we create a new string
> with
> them, but make sure that the string does not include new changes.
> 
> 	strncpy_from_user(buffer, user_buff, sizeof(buffer));
> 
> 	if (!strstr(buffer, ";")) {
> 		if (!audit_old_buffer(buffer))
> 			goto error;
> 		insert_colons(buffer);
> 	}
> 
> 
> That is, if the buffer does not have semicolons, then check if it is
> a
> valid "old format", and if not, we error out. Otherwise, we insert
> the
> colons into the buffer, and process that as if the user put in
> colons:
> 
> That is:
> 
> 	echo 'wakeup_latency u64 lat pid_t pid' > synthetic_events
> 
> would change the buffer to:
> 
> 	"wakeup_latency u64 lat; pid_t pid;"
> 
> And then put it through the normal processing. I think its OK that if
> the
> user were to cat out the synthetic events, it would see the
> semicolons even
> if it did not add them. As I don't think that will break userspace.
> 
> Does that make sense?
> 

Yeah, that should work, I'll try adding that.

Thanks,

Tom

> -- Steve

Re: [PATCH v3 3/5] tracing: Update synth command errors

[Masami Hiramatsu]

On Tue, 8 Dec 2020 12:53:40 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue, 08 Dec 2020 11:34:41 -0600
> Tom Zanussi <zanussi@kernel.org> wrote:
> 
> > Unfortunately, you're correct, if you have a script that creates a
> > synthetic event without semicolons, this patchset will break it, as I
> > myself found out and fixed in patch 4 ([PATCH v3 4/5] selftests/ftrace:
> > Add synthetic event field separators) [4].
> > 
> > So whereas before this would work, even though it shouldn't have in the
> > first place:
> > 
> >   # echo 'wakeup_latency  u64 lat pid_t pid char comm[16]' >
> > synthetic_events
> > 
> > it now has to be:
> > 
> >   # echo 'wakeup_latency  u64 lat; pid_t pid; char comm[16]' >
> > synthetic_events
> > 
> > So yeah, this patchset fixes a set of parsing bugs for things that
> > shouldn't have been accepted as valid, but shouldn't break things that
> > are obviously valid.
> > 
> > If it's too late to fix them, though, I guess we'll just have to live
> > with them, or some other option?
> 
> 
> I would suggest allowing the old interface work (with no new features, for
> backward compatibility), but new things like "char comm[16]" we require
> semicolons.
> 
> One method to do this is to add to the start of reading the string, and
> checking if it has semicolons. If it does not, we create a new string with
> them, but make sure that the string does not include new changes.
> 
> 	strncpy_from_user(buffer, user_buff, sizeof(buffer));
> 
> 	if (!strstr(buffer, ";")) {
> 		if (!audit_old_buffer(buffer))
> 			goto error;
> 		insert_colons(buffer);
> 	}
> 
> 
> That is, if the buffer does not have semicolons, then check if it is a
> valid "old format", and if not, we error out. Otherwise, we insert the
> colons into the buffer, and process that as if the user put in colons:
> 
> That is:
> 
> 	echo 'wakeup_latency u64 lat pid_t pid' > synthetic_events
> 
> would change the buffer to:
> 
> 	"wakeup_latency u64 lat; pid_t pid;"
> 
> And then put it through the normal processing. I think its OK that if the
> user were to cat out the synthetic events, it would see the semicolons even
> if it did not add them. As I don't think that will break userspace.
> 
> Does that make sense?

This makes sense. Anyway, what I considered were
- synthetic_events interface doesn't provide syntax error reports
- synthetic_events interface is not self-reproducive*.

*) I meant

$ cat synthetic_events > saved_events
$ cat saved_events > synthetic_events

  should work. But this does *NOT* mean

$ cat user-input > synthetic_events
$ cat synthetic_events > saved_events
$ diff user-input saved_events # no diff

So input and output can be different, but the output can be input again.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v3 3/5] tracing: Update synth command errors

[Steven Rostedt]

On Wed, 9 Dec 2020 22:51:14 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> This makes sense. Anyway, what I considered were
> - synthetic_events interface doesn't provide syntax error reports
> - synthetic_events interface is not self-reproducive*.
> 
> *) I meant
> 
> $ cat synthetic_events > saved_events
> $ cat saved_events > synthetic_events
> 
>   should work. But this does *NOT* mean
> 
> $ cat user-input > synthetic_events
> $ cat synthetic_events > saved_events
> $ diff user-input saved_events # no diff
> 
> So input and output can be different, but the output can be input again.

Totally agree.

Thanks,

-- Steve