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 <[email protected]>
---
kernel/trace/trace_events_synth.c | 198 +++++++++++++++---------------
1 file changed, 98 insertions(+), 100 deletions(-)
diff --git a/kernel/trace/trace_events_synth.c b/kernel/trace/trace_events_synth.c
index b2588a5650c9..a79c17b97add 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,18 +579,14 @@ 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;
- int len, ret = -ENOMEM;
+ int len, consumed, ret = -ENOMEM;
+ struct synth_field *field;
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,20 @@ static struct synth_field *parse_synth_field(int argc, const char **argv,
prefix = "unsigned ";
field_type = argv[1];
field_name = argv[2];
- *consumed = 3;
+ consumed = 3;
} else {
field_name = argv[1];
- *consumed = 2;
+ consumed = 2;
+ }
+
+ if (consumed < argc) {
+ synth_err(SYNTH_ERR_INVALID_FIELD, errpos(field_type));
+ return ERR_PTR(-EINVAL);
+ }
+
+ if (!field_name) {
+ synth_err(SYNTH_ERR_INVALID_FIELD, errpos(field_type));
+ return ERR_PTR(-EINVAL);
}
field = kzalloc(sizeof(*field), GFP_KERNEL);
@@ -613,8 +619,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 +630,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 +646,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;
@@ -1160,46 +1159,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)
{
+ char **argv, *field_str, *tmp_fields, *saved_fields = NULL;
struct synth_field *field, *fields[SYNTH_FIELDS_MAX];
+ int i, argc, n_fields = 0, ret = 0;
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 +1173,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 +1194,41 @@ 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;
- if (n_fields == SYNTH_FIELDS_MAX) {
- synth_err(SYNTH_ERR_TOO_MANY_FIELDS, 0);
- ret = -EINVAL;
+ tmp_fields = saved_fields = kstrdup(raw_fields, GFP_KERNEL);
+ if (!tmp_fields) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ while ((field_str = strsep(&tmp_fields, ";")) != NULL) {
+ argv = argv_split(GFP_KERNEL, field_str, &argc);
+ if (!argv) {
+ ret = -ENOMEM;
goto err;
}
- field = parse_synth_field(argc - i, &argv[i], &consumed);
+ 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 (n_fields == SYNTH_FIELDS_MAX) {
+ synth_err(SYNTH_ERR_TOO_MANY_FIELDS, 0);
+ ret = -EINVAL;
+ goto err;
+ }
}
- 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 +1247,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 +1368,38 @@ 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;
+
+ last_cmd_set(raw_command);
- if (!argc)
+ p = strpbrk(raw_command, " \t");
+ if (!p && raw_command[0] != '!') {
+ synth_err(SYNTH_ERR_CMD_INCOMPLETE, 0);
+ ret = -EINVAL;
goto free;
+ }
- name = argv[0];
+ name = kmemdup_nul(raw_command, p ? p - raw_command : strlen(raw_command), GFP_KERNEL);
+ if (!name)
+ return -ENOMEM;
- /* 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);
+ fields = skip_spaces(p);
+
+ 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 +1945,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
On Thu, 21 Jan 2021 11:01:05 -0600
Tom Zanussi <[email protected]> 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().
>
This looks good to me.
Reviewed-by: Masami Hiramatsu <[email protected]>
Thank you,
> Signed-off-by: Tom Zanussi <[email protected]>
> ---
> kernel/trace/trace_events_synth.c | 198 +++++++++++++++---------------
> 1 file changed, 98 insertions(+), 100 deletions(-)
>
> diff --git a/kernel/trace/trace_events_synth.c b/kernel/trace/trace_events_synth.c
> index b2588a5650c9..a79c17b97add 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,18 +579,14 @@ 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;
> - int len, ret = -ENOMEM;
> + int len, consumed, ret = -ENOMEM;
> + struct synth_field *field;
> 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,20 @@ static struct synth_field *parse_synth_field(int argc, const char **argv,
> prefix = "unsigned ";
> field_type = argv[1];
> field_name = argv[2];
> - *consumed = 3;
> + consumed = 3;
> } else {
> field_name = argv[1];
> - *consumed = 2;
> + consumed = 2;
> + }
> +
> + if (consumed < argc) {
> + synth_err(SYNTH_ERR_INVALID_FIELD, errpos(field_type));
> + return ERR_PTR(-EINVAL);
> + }
> +
> + if (!field_name) {
> + synth_err(SYNTH_ERR_INVALID_FIELD, errpos(field_type));
> + return ERR_PTR(-EINVAL);
> }
>
> field = kzalloc(sizeof(*field), GFP_KERNEL);
> @@ -613,8 +619,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 +630,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 +646,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;
>
> @@ -1160,46 +1159,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)
> {
> + char **argv, *field_str, *tmp_fields, *saved_fields = NULL;
> struct synth_field *field, *fields[SYNTH_FIELDS_MAX];
> + int i, argc, n_fields = 0, ret = 0;
> 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 +1173,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 +1194,41 @@ 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;
> - if (n_fields == SYNTH_FIELDS_MAX) {
> - synth_err(SYNTH_ERR_TOO_MANY_FIELDS, 0);
> - ret = -EINVAL;
> + tmp_fields = saved_fields = kstrdup(raw_fields, GFP_KERNEL);
> + if (!tmp_fields) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + while ((field_str = strsep(&tmp_fields, ";")) != NULL) {
> + argv = argv_split(GFP_KERNEL, field_str, &argc);
> + if (!argv) {
> + ret = -ENOMEM;
> goto err;
> }
>
> - field = parse_synth_field(argc - i, &argv[i], &consumed);
> + 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 (n_fields == SYNTH_FIELDS_MAX) {
> + synth_err(SYNTH_ERR_TOO_MANY_FIELDS, 0);
> + ret = -EINVAL;
> + goto err;
> + }
> }
>
> - 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 +1247,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 +1368,38 @@ 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;
> +
> + last_cmd_set(raw_command);
>
> - if (!argc)
> + p = strpbrk(raw_command, " \t");
> + if (!p && raw_command[0] != '!') {
> + synth_err(SYNTH_ERR_CMD_INCOMPLETE, 0);
> + ret = -EINVAL;
> goto free;
> + }
>
> - name = argv[0];
> + name = kmemdup_nul(raw_command, p ? p - raw_command : strlen(raw_command), GFP_KERNEL);
> + if (!name)
> + return -ENOMEM;
>
> - /* 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);
> + fields = skip_spaces(p);
> +
> + 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 +1945,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
>
--
Masami Hiramatsu <[email protected]>
On Thu, 21 Jan 2021 11:01:05 -0600
Tom Zanussi <[email protected]> wrote:
> @@ -1208,13 +1173,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);
I'm curious, why is the event_mutex taken here? I'm guessing it is first
needed for the find_synth_event() call, in which case, it can be moved
after the is_good_name() check. I don't see why the goto out is required
here or for the is_good_name() check.
-- Steve
> +
> + 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;
On Fri, 2021-01-22 at 22:16 +0900, Masami Hiramatsu wrote:
> On Thu, 21 Jan 2021 11:01:05 -0600
> Tom Zanussi <[email protected]> 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().
> >
>
> This looks good to me.
>
> Reviewed-by: Masami Hiramatsu <[email protected]>
>
> Thank you,
>
Thanks, Masami.
Tom
> > Signed-off-by: Tom Zanussi <[email protected]>
> > ---
> > kernel/trace/trace_events_synth.c | 198 +++++++++++++++-----------
> > ----
> > 1 file changed, 98 insertions(+), 100 deletions(-)
> >
> > diff --git a/kernel/trace/trace_events_synth.c
> > b/kernel/trace/trace_events_synth.c
> > index b2588a5650c9..a79c17b97add 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,18 +579,14 @@ 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;
> > - int len, ret = -ENOMEM;
> > + int len, consumed, ret = -ENOMEM;
> > + struct synth_field *field;
> > 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,20 @@ static struct synth_field
> > *parse_synth_field(int argc, const char **argv,
> > prefix = "unsigned ";
> > field_type = argv[1];
> > field_name = argv[2];
> > - *consumed = 3;
> > + consumed = 3;
> > } else {
> > field_name = argv[1];
> > - *consumed = 2;
> > + consumed = 2;
> > + }
> > +
> > + if (consumed < argc) {
> > + synth_err(SYNTH_ERR_INVALID_FIELD, errpos(field_type));
> > + return ERR_PTR(-EINVAL);
> > + }
> > +
> > + if (!field_name) {
> > + synth_err(SYNTH_ERR_INVALID_FIELD, errpos(field_type));
> > + return ERR_PTR(-EINVAL);
> > }
> >
> > field = kzalloc(sizeof(*field), GFP_KERNEL);
> > @@ -613,8 +619,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 +630,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 +646,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;
> >
> > @@ -1160,46 +1159,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)
> > {
> > + char **argv, *field_str, *tmp_fields, *saved_fields = NULL;
> > struct synth_field *field, *fields[SYNTH_FIELDS_MAX];
> > + int i, argc, n_fields = 0, ret = 0;
> > 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 +1173,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 +1194,41 @@ 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;
> > - if (n_fields == SYNTH_FIELDS_MAX) {
> > - synth_err(SYNTH_ERR_TOO_MANY_FIELDS, 0);
> > - ret = -EINVAL;
> > + tmp_fields = saved_fields = kstrdup(raw_fields, GFP_KERNEL);
> > + if (!tmp_fields) {
> > + ret = -ENOMEM;
> > + goto out;
> > + }
> > +
> > + while ((field_str = strsep(&tmp_fields, ";")) != NULL) {
> > + argv = argv_split(GFP_KERNEL, field_str, &argc);
> > + if (!argv) {
> > + ret = -ENOMEM;
> > goto err;
> > }
> >
> > - field = parse_synth_field(argc - i, &argv[i],
> > &consumed);
> > + 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 (n_fields == SYNTH_FIELDS_MAX) {
> > + synth_err(SYNTH_ERR_TOO_MANY_FIELDS, 0);
> > + ret = -EINVAL;
> > + goto err;
> > + }
> > }
> >
> > - 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 +1247,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 +1368,38 @@ 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;
> > +
> > + last_cmd_set(raw_command);
> >
> > - if (!argc)
> > + p = strpbrk(raw_command, " \t");
> > + if (!p && raw_command[0] != '!') {
> > + synth_err(SYNTH_ERR_CMD_INCOMPLETE, 0);
> > + ret = -EINVAL;
> > goto free;
> > + }
> >
> > - name = argv[0];
> > + name = kmemdup_nul(raw_command, p ? p - raw_command :
> > strlen(raw_command), GFP_KERNEL);
> > + if (!name)
> > + return -ENOMEM;
> >
> > - /* 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);
> > + fields = skip_spaces(p);
> > +
> > + 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 +1945,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
> >
>
>
Hi Steve,
On Fri, 2021-01-22 at 16:00 -0500, Steven Rostedt wrote:
> On Thu, 21 Jan 2021 11:01:05 -0600
> Tom Zanussi <[email protected]> wrote:
>
> > @@ -1208,13 +1173,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);
>
> I'm curious, why is the event_mutex taken here? I'm guessing it is
> first
> needed for the find_synth_event() call, in which case, it can be
> moved
> after the is_good_name() check. I don't see why the goto out is
> required
> here or for the is_good_name() check.
>
Yes, it's for the find_synth_event() call, and yes, it should come
after the is_good_name() check. I'll move it and fix up the goto
changes as a result.
Thanks,
Tom
> -- Steve
>
> > +
> > + 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;