* [PATCH v4 1/5] tracing/dynevent: Delegate parsing to create function
2020-12-17 21:14 [PATCH v4 0/5] tracing: More synthetic event error fixes Tom Zanussi
@ 2020-12-17 21:14 ` Tom Zanussi
2020-12-17 21:14 ` [PATCH v4 2/5] tracing: Rework synthetic event command parsing Tom Zanussi
` (3 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Tom Zanussi @ 2020-12-17 21:14 UTC (permalink / raw)
To: rostedt, axelrasmussen; +Cc: mhiramat, linux-kernel
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 eb5205e48733..e9cde7a3e0a6 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -9426,30 +9426,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;
@@ -9497,7 +9478,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 e448d2da0b99..98211637e545 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -1830,10 +1830,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 4f967d5cd917..dc971a68dda4 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 d6f72dcb7269..7754936b57ee 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 5a8bc0b421f1..b2588a5650c9 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);
@@ -1383,18 +1383,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;
}
@@ -1403,7 +1415,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;
@@ -1939,23 +1951,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 b29f92c51b1a..1c1f089191e4 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -35,7 +35,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);
@@ -711,7 +711,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:
@@ -908,20 +908,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);
}
/**
@@ -1082,7 +1087,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);
@@ -1885,7 +1890,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);
@@ -1979,8 +1984,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++;
@@ -2001,8 +2005,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++;
@@ -2075,13 +2078,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
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH v4 2/5] tracing: Rework synthetic event command parsing
2020-12-17 21:14 [PATCH v4 0/5] tracing: More synthetic event error fixes Tom Zanussi
2020-12-17 21:14 ` [PATCH v4 1/5] tracing/dynevent: Delegate parsing to create function Tom Zanussi
@ 2020-12-17 21:14 ` Tom Zanussi
2020-12-17 21:14 ` [PATCH v4 3/5] tracing: Update synth command errors Tom Zanussi
` (2 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Tom Zanussi @ 2020-12-17 21:14 UTC (permalink / raw)
To: rostedt, axelrasmussen; +Cc: mhiramat, linux-kernel
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 | 184 +++++++++++++++---------------
1 file changed, 90 insertions(+), 94 deletions(-)
diff --git a/kernel/trace/trace_events_synth.c b/kernel/trace/trace_events_synth.c
index b2588a5650c9..66ccbab3483b 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,7 +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,
+static struct synth_field *parse_synth_field(int argc, char **argv,
int *consumed)
{
struct synth_field *field;
@@ -588,9 +588,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));
@@ -605,6 +602,11 @@ static struct synth_field *parse_synth_field(int argc, const char **argv,
*consumed = 2;
}
+ if (!field_name) {
+ synth_err(SYNTH_ERR_INVALID_FIELD, errpos(field_type));
+ return ERR_PTR(-EINVAL);
+ }
+
field = kzalloc(sizeof(*field), GFP_KERNEL);
if (!field)
return ERR_PTR(-ENOMEM);
@@ -613,8 +615,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)
@@ -626,8 +626,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)
@@ -644,11 +642,8 @@ 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;
@@ -656,7 +651,6 @@ static struct synth_field *parse_synth_field(int argc, const char **argv,
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) {
@@ -1160,46 +1154,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, consumed;
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:
@@ -1208,13 +1168,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;
@@ -1228,26 +1189,47 @@ 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, &consumed);
if (IS_ERR(field)) {
+ argv_free(argv);
ret = PTR_ERR(field);
goto err;
}
+
+ argv_free(argv);
+
+ if (consumed < argc) {
+ ret = -EINVAL;
+ goto err;
+ }
+
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;
}
@@ -1266,6 +1248,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++)
@@ -1385,29 +1369,35 @@ 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];
+ p = strpbrk(raw_command, " \t");
+ if (!p)
+ return -EINVAL;
+
+ name = kmemdup_nul(raw_command, p - raw_command, GFP_KERNEL);
+ if (!name)
+ return -ENOMEM;
+
+ 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)
@@ -1953,39 +1943,45 @@ 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);
+ if (!name)
+ return -ENOMEM;
+
+ ret = __create_synth_event(name, fields);
+
+ kfree(name);
return ret;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH v4 3/5] tracing: Update synth command errors
2020-12-17 21:14 [PATCH v4 0/5] tracing: More synthetic event error fixes Tom Zanussi
2020-12-17 21:14 ` [PATCH v4 1/5] tracing/dynevent: Delegate parsing to create function Tom Zanussi
2020-12-17 21:14 ` [PATCH v4 2/5] tracing: Rework synthetic event command parsing Tom Zanussi
@ 2020-12-17 21:14 ` Tom Zanussi
2020-12-17 21:14 ` [PATCH v4 4/5] tracing: Add a backward-compatibility check for synthetic event creation Tom Zanussi
2020-12-17 21:14 ` [PATCH v4 5/5] selftests/ftrace: Update synthetic event syntax errors Tom Zanussi
4 siblings, 0 replies; 7+ messages in thread
From: Tom Zanussi @ 2020-12-17 21:14 UTC (permalink / raw)
To: rostedt, axelrasmussen; +Cc: mhiramat, linux-kernel
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 | 71 +++++++++++++++++++++++++++----
1 file changed, 63 insertions(+), 8 deletions(-)
diff --git a/kernel/trace/trace_events_synth.c b/kernel/trace/trace_events_synth.c
index 66ccbab3483b..2a9c8bf74bb2 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
@@ -651,6 +652,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) {
@@ -1171,7 +1176,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;
}
@@ -1221,6 +1226,7 @@ static int __create_synth_event(const char *name, const char *raw_fields)
argv_free(argv);
if (consumed < argc) {
+ synth_err(SYNTH_ERR_INVALID_CMD, 0);
ret = -EINVAL;
goto err;
}
@@ -1229,7 +1235,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;
}
@@ -1367,6 +1373,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;
@@ -1378,9 +1415,17 @@ static int create_or_delete_synth_event(const char *raw_command)
last_cmd_set(raw_command);
+ ret = check_command(raw_command);
+ 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);
if (!name)
@@ -1954,8 +1999,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);
@@ -1968,13 +2015,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);
if (!name)
return -ENOMEM;
--
2.17.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH v4 4/5] tracing: Add a backward-compatibility check for synthetic event creation
2020-12-17 21:14 [PATCH v4 0/5] tracing: More synthetic event error fixes Tom Zanussi
` (2 preceding siblings ...)
2020-12-17 21:14 ` [PATCH v4 3/5] tracing: Update synth command errors Tom Zanussi
@ 2020-12-17 21:14 ` Tom Zanussi
2020-12-18 6:39 ` kernel test robot
2020-12-17 21:14 ` [PATCH v4 5/5] selftests/ftrace: Update synthetic event syntax errors Tom Zanussi
4 siblings, 1 reply; 7+ messages in thread
From: Tom Zanussi @ 2020-12-17 21:14 UTC (permalink / raw)
To: rostedt, axelrasmussen; +Cc: mhiramat, linux-kernel
The synthetic event parsing rework requiring semicolons between
synthetic event fields. That requirement breaks existing users who
might already have used the old form, so this adds a pre-parsing pass
that adds semicolons between fields to any string missing them. If
none are required, the original string is used.
In the future, if/when new features are added, the requirement will be
that any string containing the new feature will be required to use
semicolons, and the audit_old_buffer() check can check for those and
avoid the pre-parsing semicolon pass altogether.
As it stands, the pre-parsing pass creates a new string with
semicolons only if one or more semicolons were actually needed and
only if no errors were found in pre-parsing. The assumption is that
the real parsing pass will find and flag any errors and the user
should see them in reference to the original unmodified string.
Signed-off-by: Tom Zanussi <zanussi@kernel.org>
---
kernel/trace/trace_events_synth.c | 294 ++++++++++++++++++++++++++++--
1 file changed, 274 insertions(+), 20 deletions(-)
diff --git a/kernel/trace/trace_events_synth.c b/kernel/trace/trace_events_synth.c
index 2a9c8bf74bb2..6bff54ed31ce 100644
--- a/kernel/trace/trace_events_synth.c
+++ b/kernel/trace/trace_events_synth.c
@@ -1373,6 +1373,245 @@ int synth_event_delete(const char *event_name)
}
EXPORT_SYMBOL_GPL(synth_event_delete);
+static int save_synth_field(int argc, char **argv, int *consumed,
+ struct seq_buf *s, bool *added_semi)
+{
+ const char *prefix = NULL, *field_name, *field_type = argv[0];
+ const char *save_field_type, *array, *next_tok;
+ int len, ret = -EINVAL;
+ struct seq_buf f;
+ ssize_t size;
+ char *tmp;
+
+ *added_semi = false;
+
+ if (field_type[0] == ';')
+ field_type++;
+
+ if (!strcmp(field_type, "unsigned")) {
+ if (argc < 3)
+ goto out;
+ prefix = "unsigned";
+ field_type = argv[1];
+ field_name = argv[2];
+ *consumed = 3;
+ } else {
+ field_type = argv[0];
+ field_name = argv[1];
+ *consumed = 2;
+ }
+
+ len = strlen(field_name);
+ array = strchr(field_name, '[');
+ if (array)
+ len -= strlen(array);
+ else if (field_name[len - 1] == ';')
+ len--;
+
+ tmp = kmemdup_nul(field_name, len, GFP_KERNEL);
+ if (!tmp) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ if (!is_good_name(tmp)) {
+ kfree(tmp);
+ goto out;
+ }
+
+ kfree(tmp);
+
+ save_field_type = field_type;
+ if (field_type[0] == ';')
+ field_type++;
+ len = strlen(field_type) + 1;
+
+ if (array)
+ len += strlen(array);
+
+ if (prefix)
+ len += strlen(prefix) + 1;
+
+ tmp = kzalloc(len, GFP_KERNEL);
+ if (!tmp) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ seq_buf_init(&f, tmp, len);
+ if (prefix) {
+ seq_buf_puts(&f, prefix);
+ seq_buf_putc(&f, ' ');
+ }
+ seq_buf_puts(&f, field_type);
+ if (array) {
+ seq_buf_puts(&f, array);
+ if (f.buffer[f.len - 1] == ';')
+ f.len--;
+ }
+ if (WARN_ON_ONCE(!seq_buf_buffer_left(&f))) {
+ kfree(tmp);
+ goto out;
+ }
+
+ f.buffer[f.len] = '\0';
+
+ field_type = save_field_type;
+
+ size = synth_field_size(tmp);
+ if (size < 0 || ((size == 0) && (!synth_field_is_string(tmp)))) {
+ kfree(tmp);
+ goto out;
+ }
+
+ kfree(tmp);
+
+ if (prefix) {
+ seq_buf_puts(s, prefix);
+ seq_buf_putc(s, ' ');
+ }
+ seq_buf_puts(s, field_type);
+ seq_buf_putc(s, ' ');
+ seq_buf_puts(s, field_name);
+ if (field_name[strlen(field_name) - 1] == ';')
+ seq_buf_putc(s, ' ');
+
+ if (*consumed < argc) {
+ next_tok = argv[*consumed];
+ if (field_name[strlen(field_name) - 1] != ';' &&
+ next_tok[0] != ';') {
+ seq_buf_puts(s, "; ");
+ *added_semi = true;
+ }
+ }
+
+ ret = 0;
+ out:
+ return ret;
+}
+
+static char *insert_semicolons(const char *raw_command)
+{
+ int i, argc, consumed = 0, n_fields = 0, semis_added = 0;
+ char *name, **argv, **save_argv;
+ int ret = -EINVAL;
+ struct seq_buf s;
+ bool added_semi;
+ char *buf;
+
+ argc = 0;
+
+ argv = argv_split(GFP_KERNEL, raw_command, &argc);
+ if (!argv)
+ return NULL;
+
+ if (!argc)
+ goto out;
+
+ name = argv[0];
+ save_argv = argv;
+ argv++;
+ argc--;
+
+ buf = kzalloc(MAX_DYNEVENT_CMD_LEN, GFP_KERNEL);
+ if (!buf) {
+ ret = -ENOMEM;
+ goto err;
+ }
+
+ seq_buf_init(&s, buf, MAX_DYNEVENT_CMD_LEN);
+
+ seq_buf_puts(&s, name);
+ seq_buf_putc(&s, ' ');
+
+ if (name[0] == '\0' || argc < 1)
+ goto err;
+
+ for (i = 0; i < argc - 1; i++) {
+ if (strcmp(argv[i], ";") == 0) {
+ seq_buf_puts(&s, " ; ");
+ continue;
+ }
+
+ if (n_fields == SYNTH_FIELDS_MAX)
+ goto err;
+
+ ret = save_synth_field(argc - i, &argv[i], &consumed,
+ &s, &added_semi);
+ if (ret)
+ goto err;
+
+ if (added_semi)
+ semis_added++;
+
+ i += consumed - 1;
+ }
+
+ if (i < argc && strcmp(argv[i], ";") != 0)
+ goto err;
+
+ if (!semis_added) {
+ kfree(buf);
+ buf = NULL;
+ goto out;
+ }
+
+ if (WARN_ON_ONCE(!seq_buf_buffer_left(&s)))
+ goto err;
+
+ buf[s.len] = '\0';
+ out:
+ argv_free(save_argv);
+
+ return buf;
+ err:
+ kfree(buf);
+ buf = ERR_PTR(ret);
+
+ goto out;
+}
+
+static bool audit_old_buffer(const char *cmd)
+{
+ /* as of now, every cmd is an old cmd */
+ return true;
+}
+
+/**
+ * get_parseable_cmd - Return a modifiable string for parsing
+ * @raw_command: The command to start with
+ *
+ * Create a cmd string that can be modified by the caller for command
+ * parsing purposes. If successful, the caller must free the command
+ * returned.
+ *
+ * The input string will first be checked to see whether or not it can
+ * be considered an 'old command' - a command that doesn't require
+ * semicolons between fields - for which backward compatibility must
+ * be maintained. If it can be considered an old command, a semicolon
+ * will be added between any two fields missing one. If no semicolons
+ * were required, or if the preparsing required for the pass
+ * encountered errors, a modifiable copy of the original string will
+ * be returned.
+ *
+ * Return: parseable cmd if successful, error or NULL string otherwise.
+ */
+static char *get_parseable_cmd(const char *raw_command)
+{
+ char *cmd = NULL;
+
+ if (audit_old_buffer(raw_command))
+ cmd = insert_semicolons(raw_command);
+
+ if (IS_ERR_OR_NULL(cmd)) {
+ cmd = kstrdup(raw_command, GFP_KERNEL);
+ if (!cmd)
+ cmd = ERR_PTR(-ENOMEM);
+ }
+
+ return cmd;
+}
+
static int check_command(const char *raw_command)
{
char **argv = NULL, *cmd, *saved_cmd, *name_and_field;
@@ -1406,28 +1645,33 @@ static int check_command(const char *raw_command)
static int create_or_delete_synth_event(const char *raw_command)
{
- char *name = NULL, *fields, *p;
+ char *name = NULL, *fields, *p, *cmd;
int ret = 0;
raw_command = skip_spaces(raw_command);
if (raw_command[0] == '\0')
return ret;
- last_cmd_set(raw_command);
+ cmd = get_parseable_cmd(raw_command);
+ if (IS_ERR(cmd))
+ return PTR_ERR(cmd);
- ret = check_command(raw_command);
+ last_cmd_set(cmd);
+
+ ret = check_command(cmd);
if (ret) {
synth_err(SYNTH_ERR_INVALID_CMD, 0);
- return ret;
+ goto free;
}
- p = strpbrk(raw_command, " \t");
+ p = strpbrk(cmd, " \t");
if (!p) {
synth_err(SYNTH_ERR_INVALID_CMD, 0);
- return -EINVAL;
+ ret = -EINVAL;
+ goto free;
}
- name = kmemdup_nul(raw_command, p - raw_command, GFP_KERNEL);
+ name = kmemdup_nul(cmd, p - cmd, GFP_KERNEL);
if (!name)
return -ENOMEM;
@@ -1441,6 +1685,7 @@ static int create_or_delete_synth_event(const char *raw_command)
ret = __create_synth_event(name, fields);
free:
kfree(name);
+ kfree(cmd);
return ret;
}
@@ -1988,7 +2233,7 @@ EXPORT_SYMBOL_GPL(synth_event_trace_end);
static int create_synth_event(const char *raw_command)
{
- char *fields, *p;
+ char *fields, *p, *cmd;
const char *name;
int len, ret = 0;
@@ -1996,20 +2241,27 @@ static int create_synth_event(const char *raw_command)
if (raw_command[0] == '\0')
return ret;
- last_cmd_set(raw_command);
+ cmd = get_parseable_cmd(raw_command);
+ if (IS_ERR(cmd))
+ return PTR_ERR(cmd);
+
+ last_cmd_set(cmd);
- p = strpbrk(raw_command, " \t");
+ p = strpbrk(cmd, " \t");
if (!p) {
synth_err(SYNTH_ERR_INVALID_CMD, 0);
- return -EINVAL;
+ ret = -EINVAL;
+ goto free;
}
fields = skip_spaces(p);
- name = raw_command;
+ name = cmd;
- if (name[0] != 's' || name[1] != ':')
- return -ECANCELED;
+ if (name[0] != 's' || name[1] != ':') {
+ ret = -ECANCELED;
+ goto free;
+ }
name += 2;
/* This interface accepts group name prefix */
@@ -2017,26 +2269,28 @@ static int create_synth_event(const char *raw_command)
len = str_has_prefix(name, SYNTH_SYSTEM "/");
if (len == 0) {
synth_err(SYNTH_ERR_INVALID_DYN_CMD, 0);
- return -EINVAL;
+ ret = -EINVAL;
+ goto free;
}
name += len;
}
- len = name - raw_command;
+ len = name - cmd;
- ret = check_command(raw_command + len);
+ ret = check_command(cmd + len);
if (ret) {
synth_err(SYNTH_ERR_INVALID_CMD, 0);
- return ret;
+ goto free;
}
- name = kmemdup_nul(raw_command + len, p - raw_command - len, GFP_KERNEL);
+ name = kmemdup_nul(cmd + len, p - cmd - len, GFP_KERNEL);
if (!name)
return -ENOMEM;
ret = __create_synth_event(name, fields);
-
kfree(name);
+ free:
+ kfree(cmd);
return ret;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v4 4/5] tracing: Add a backward-compatibility check for synthetic event creation
2020-12-17 21:14 ` [PATCH v4 4/5] tracing: Add a backward-compatibility check for synthetic event creation Tom Zanussi
@ 2020-12-18 6:39 ` kernel test robot
0 siblings, 0 replies; 7+ messages in thread
From: kernel test robot @ 2020-12-18 6:39 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 13420 bytes --]
Hi Tom,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on trace/for-next]
[also build test WARNING on linux/master linus/master v5.10 next-20201217]
[cannot apply to tip/perf/core]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Tom-Zanussi/tracing-More-synthetic-event-error-fixes/20201218-052110
base: https://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git for-next
config: riscv-randconfig-r014-20201217 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project cee1e7d14f4628d6174b33640d502bff3b54ae45)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install riscv cross compiling tool for clang build
# apt-get install binutils-riscv64-linux-gnu
# https://github.com/0day-ci/linux/commit/aa73dabdbd071e2cb537b974c0b4308d24f8d4ef
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Tom-Zanussi/tracing-More-synthetic-event-error-fixes/20201218-052110
git checkout aa73dabdbd071e2cb537b974c0b4308d24f8d4ef
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=riscv
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from include/linux/hardirq.h:10:
In file included from ./arch/riscv/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:13:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/riscv/include/asm/io.h:149:
include/asm-generic/io.h:564:9: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
return inw(addr);
^~~~~~~~~
arch/riscv/include/asm/io.h:56:76: note: expanded from macro 'inw'
#define inw(c) ({ u16 __v; __io_pbr(); __v = readw_cpu((void*)(PCI_IOBASE + (c))); __io_par(__v); __v; })
~~~~~~~~~~ ^
arch/riscv/include/asm/mmio.h:88:76: note: expanded from macro 'readw_cpu'
#define readw_cpu(c) ({ u16 __r = le16_to_cpu((__force __le16)__raw_readw(c)); __r; })
^
include/uapi/linux/byteorder/little_endian.h:36:51: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
^
In file included from kernel/trace/trace_events_synth.c:18:
In file included from include/linux/trace_events.h:9:
In file included from include/linux/hardirq.h:10:
In file included from ./arch/riscv/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:13:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/riscv/include/asm/io.h:149:
include/asm-generic/io.h:572:9: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
return inl(addr);
^~~~~~~~~
arch/riscv/include/asm/io.h:57:76: note: expanded from macro 'inl'
#define inl(c) ({ u32 __v; __io_pbr(); __v = readl_cpu((void*)(PCI_IOBASE + (c))); __io_par(__v); __v; })
~~~~~~~~~~ ^
arch/riscv/include/asm/mmio.h:89:76: note: expanded from macro 'readl_cpu'
#define readl_cpu(c) ({ u32 __r = le32_to_cpu((__force __le32)__raw_readl(c)); __r; })
^
include/uapi/linux/byteorder/little_endian.h:34:51: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
^
In file included from kernel/trace/trace_events_synth.c:18:
In file included from include/linux/trace_events.h:9:
In file included from include/linux/hardirq.h:10:
In file included from ./arch/riscv/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:13:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/riscv/include/asm/io.h:149:
include/asm-generic/io.h:580:2: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
outb(value, addr);
^~~~~~~~~~~~~~~~~
arch/riscv/include/asm/io.h:59:68: note: expanded from macro 'outb'
#define outb(v,c) ({ __io_pbw(); writeb_cpu((v),(void*)(PCI_IOBASE + (c))); __io_paw(); })
~~~~~~~~~~ ^
arch/riscv/include/asm/mmio.h:91:52: note: expanded from macro 'writeb_cpu'
#define writeb_cpu(v, c) ((void)__raw_writeb((v), (c)))
^
In file included from kernel/trace/trace_events_synth.c:18:
In file included from include/linux/trace_events.h:9:
In file included from include/linux/hardirq.h:10:
In file included from ./arch/riscv/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:13:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/riscv/include/asm/io.h:149:
include/asm-generic/io.h:588:2: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
outw(value, addr);
^~~~~~~~~~~~~~~~~
arch/riscv/include/asm/io.h:60:68: note: expanded from macro 'outw'
#define outw(v,c) ({ __io_pbw(); writew_cpu((v),(void*)(PCI_IOBASE + (c))); __io_paw(); })
~~~~~~~~~~ ^
arch/riscv/include/asm/mmio.h:92:76: note: expanded from macro 'writew_cpu'
#define writew_cpu(v, c) ((void)__raw_writew((__force u16)cpu_to_le16(v), (c)))
^
In file included from kernel/trace/trace_events_synth.c:18:
In file included from include/linux/trace_events.h:9:
In file included from include/linux/hardirq.h:10:
In file included from ./arch/riscv/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:13:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/riscv/include/asm/io.h:149:
include/asm-generic/io.h:596:2: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
outl(value, addr);
^~~~~~~~~~~~~~~~~
arch/riscv/include/asm/io.h:61:68: note: expanded from macro 'outl'
#define outl(v,c) ({ __io_pbw(); writel_cpu((v),(void*)(PCI_IOBASE + (c))); __io_paw(); })
~~~~~~~~~~ ^
arch/riscv/include/asm/mmio.h:93:76: note: expanded from macro 'writel_cpu'
#define writel_cpu(v, c) ((void)__raw_writel((__force u32)cpu_to_le32(v), (c)))
^
In file included from kernel/trace/trace_events_synth.c:18:
In file included from include/linux/trace_events.h:9:
In file included from include/linux/hardirq.h:10:
In file included from ./arch/riscv/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:13:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/riscv/include/asm/io.h:149:
include/asm-generic/io.h:1005:55: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
return (port > MMIO_UPPER_LIMIT) ? NULL : PCI_IOBASE + port;
~~~~~~~~~~ ^
>> kernel/trace/trace_events_synth.c:1508:6: warning: variable 'save_argv' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
if (!argc)
^~~~~
kernel/trace/trace_events_synth.c:1564:12: note: uninitialized use occurs here
argv_free(save_argv);
^~~~~~~~~
kernel/trace/trace_events_synth.c:1508:2: note: remove the 'if' if its condition is always false
if (!argc)
^~~~~~~~~~
kernel/trace/trace_events_synth.c:1496:33: note: initialize the variable 'save_argv' to silence this warning
char *name, **argv, **save_argv;
^
= NULL
>> kernel/trace/trace_events_synth.c:1508:6: warning: variable 'buf' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
if (!argc)
^~~~~
kernel/trace/trace_events_synth.c:1566:9: note: uninitialized use occurs here
return buf;
^~~
kernel/trace/trace_events_synth.c:1508:2: note: remove the 'if' if its condition is always false
if (!argc)
^~~~~~~~~~
kernel/trace/trace_events_synth.c:1500:11: note: initialize the variable 'buf' to silence this warning
char *buf;
^
= NULL
9 warnings generated.
/tmp/trace_events_synth-97fa74.s: Assembler messages:
/tmp/trace_events_synth-97fa74.s:3701: Error: unrecognized opcode `zext.b a2,a2'
/tmp/trace_events_synth-97fa74.s:4380: Error: unrecognized opcode `zext.b s1,s1'
/tmp/trace_events_synth-97fa74.s:4474: Error: unrecognized opcode `zext.b a5,a0'
/tmp/trace_events_synth-97fa74.s:4705: Error: unrecognized opcode `zext.b a1,a1'
/tmp/trace_events_synth-97fa74.s:5065: Error: unrecognized opcode `zext.b a5,a0'
/tmp/trace_events_synth-97fa74.s:5399: Error: unrecognized opcode `zext.b a5,a0'
/tmp/trace_events_synth-97fa74.s:5459: Error: unrecognized opcode `zext.b a5,a0'
/tmp/trace_events_synth-97fa74.s:5516: Error: unrecognized opcode `zext.b a5,a0'
/tmp/trace_events_synth-97fa74.s:5571: Error: unrecognized opcode `zext.b a5,a0'
/tmp/trace_events_synth-97fa74.s:5648: Error: unrecognized opcode `zext.b a5,a0'
/tmp/trace_events_synth-97fa74.s:5682: Error: unrecognized opcode `zext.b a5,a0'
/tmp/trace_events_synth-97fa74.s:7129: Error: unrecognized opcode `zext.b a3,a3'
clang-12: error: assembler command failed with exit code 1 (use -v to see invocation)
vim +1508 kernel/trace/trace_events_synth.c
1492
1493 static char *insert_semicolons(const char *raw_command)
1494 {
1495 int i, argc, consumed = 0, n_fields = 0, semis_added = 0;
1496 char *name, **argv, **save_argv;
1497 int ret = -EINVAL;
1498 struct seq_buf s;
1499 bool added_semi;
1500 char *buf;
1501
1502 argc = 0;
1503
1504 argv = argv_split(GFP_KERNEL, raw_command, &argc);
1505 if (!argv)
1506 return NULL;
1507
> 1508 if (!argc)
1509 goto out;
1510
1511 name = argv[0];
1512 save_argv = argv;
1513 argv++;
1514 argc--;
1515
1516 buf = kzalloc(MAX_DYNEVENT_CMD_LEN, GFP_KERNEL);
1517 if (!buf) {
1518 ret = -ENOMEM;
1519 goto err;
1520 }
1521
1522 seq_buf_init(&s, buf, MAX_DYNEVENT_CMD_LEN);
1523
1524 seq_buf_puts(&s, name);
1525 seq_buf_putc(&s, ' ');
1526
1527 if (name[0] == '\0' || argc < 1)
1528 goto err;
1529
1530 for (i = 0; i < argc - 1; i++) {
1531 if (strcmp(argv[i], ";") == 0) {
1532 seq_buf_puts(&s, " ; ");
1533 continue;
1534 }
1535
1536 if (n_fields == SYNTH_FIELDS_MAX)
1537 goto err;
1538
1539 ret = save_synth_field(argc - i, &argv[i], &consumed,
1540 &s, &added_semi);
1541 if (ret)
1542 goto err;
1543
1544 if (added_semi)
1545 semis_added++;
1546
1547 i += consumed - 1;
1548 }
1549
1550 if (i < argc && strcmp(argv[i], ";") != 0)
1551 goto err;
1552
1553 if (!semis_added) {
1554 kfree(buf);
1555 buf = NULL;
1556 goto out;
1557 }
1558
1559 if (WARN_ON_ONCE(!seq_buf_buffer_left(&s)))
1560 goto err;
1561
1562 buf[s.len] = '\0';
1563 out:
1564 argv_free(save_argv);
1565
1566 return buf;
1567 err:
1568 kfree(buf);
1569 buf = ERR_PTR(ret);
1570
1571 goto out;
1572 }
1573
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 19162 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v4 5/5] selftests/ftrace: Update synthetic event syntax errors
2020-12-17 21:14 [PATCH v4 0/5] tracing: More synthetic event error fixes Tom Zanussi
` (3 preceding siblings ...)
2020-12-17 21:14 ` [PATCH v4 4/5] tracing: Add a backward-compatibility check for synthetic event creation Tom Zanussi
@ 2020-12-17 21:14 ` Tom Zanussi
4 siblings, 0 replies; 7+ messages in thread
From: Tom Zanussi @ 2020-12-17 21:14 UTC (permalink / raw)
To: rostedt, axelrasmussen; +Cc: mhiramat, linux-kernel
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
^ permalink raw reply related [flat|nested] 7+ messages in thread