| Message ID | 20211209223210.1818-14-beaub@linux.microsoft.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | user_events: Enable user processes to create and write to trace events | expand |
Hi Beau, On Thu, 9 Dec 2021 14:32:10 -0800 Beau Belgrave <beaub@linux.microsoft.com> wrote: > Switch between __get_str and __get_rel_str within the print_fmt of > user_events. Add unit test to ensure print_fmt is correct on known > types. > > Signed-off-by: Beau Belgrave <beaub@linux.microsoft.com> > --- > kernel/trace/trace_events_user.c | 24 ++- > .../selftests/user_events/ftrace_test.c | 166 ++++++++++++++++++ > 2 files changed, 182 insertions(+), 8 deletions(-) > > diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c > index 56eb58ddb4cf..3779fa2ca14a 100644 > --- a/kernel/trace/trace_events_user.c > +++ b/kernel/trace/trace_events_user.c > @@ -257,7 +257,7 @@ static int user_event_add_field(struct user_event *user, const char *type, > goto add_field; > > add_validator: > - if (strstr(type, "char[") != 0) > + if (strstr(type, "char") != 0) > validator_flags |= VALIDATOR_ENSURE_NULL; What is this change for? This seems not related to the other changes. (Also, what happen if it is a single char type?) > > validator = kmalloc(sizeof(*validator), GFP_KERNEL); > @@ -456,14 +456,21 @@ static const char *user_field_format(const char *type) > return "%llu"; > } > > -static bool user_field_is_dyn_string(const char *type) > +static bool user_field_is_dyn_string(const char *type, const char **str_func) > { > - if (str_has_prefix(type, "__data_loc ") || > - str_has_prefix(type, "__rel_loc ")) > - if (strstr(type, "char[") != 0) > - return true; > + if (str_has_prefix(type, "__data_loc ")) { > + *str_func = "__get_str"; > + goto check; > + } > + > + if (str_has_prefix(type, "__rel_loc ")) { > + *str_func = "__get_rel_str"; > + goto check; > + } > > return false; > +check: > + return strstr(type, "char") != 0; > } > > #define LEN_OR_ZERO (len ? len - pos : 0) > @@ -472,6 +479,7 @@ static int user_event_set_print_fmt(struct user_event *user, char *buf, int len) > struct ftrace_event_field *field, *next; > struct list_head *head = &user->fields; > int pos = 0, depth = 0; > + const char *str_func; > > pos += snprintf(buf + pos, LEN_OR_ZERO, "\""); > > @@ -488,9 +496,9 @@ static int user_event_set_print_fmt(struct user_event *user, char *buf, int len) > pos += snprintf(buf + pos, LEN_OR_ZERO, "\""); > > list_for_each_entry_safe_reverse(field, next, head, link) { > - if (user_field_is_dyn_string(field->type)) > + if (user_field_is_dyn_string(field->type, &str_func)) > pos += snprintf(buf + pos, LEN_OR_ZERO, > - ", __get_str(%s)", field->name); > + ", %s(%s)", str_func, field->name); > else > pos += snprintf(buf + pos, LEN_OR_ZERO, > ", REC->%s", field->name); > diff --git a/tools/testing/selftests/user_events/ftrace_test.c b/tools/testing/selftests/user_events/ftrace_test.c Just a nitpick, if possible, please split this part from the kernel update. > index 16aff1fb295a..b2e5c0765a68 100644 > --- a/tools/testing/selftests/user_events/ftrace_test.c > +++ b/tools/testing/selftests/user_events/ftrace_test.c > @@ -20,6 +20,7 @@ const char *data_file = "/sys/kernel/debug/tracing/user_events_data"; > const char *status_file = "/sys/kernel/debug/tracing/user_events_status"; > const char *enable_file = "/sys/kernel/debug/tracing/events/user_events/__test_event/enable"; > const char *trace_file = "/sys/kernel/debug/tracing/trace"; > +const char *fmt_file = "/sys/kernel/debug/tracing/events/user_events/__test_event/format"; > > static int trace_bytes(void) > { > @@ -47,6 +48,61 @@ static int trace_bytes(void) > return bytes; > } > > +static int get_print_fmt(char *buffer, int len) > +{ > + FILE *fp = fopen(fmt_file, "r"); > + int c, index = 0, last = 0; > + > + if (!fp) > + return -1; > + > + /* Read until empty line (Skip Common) */ > + while (true) { > + c = getc(fp); > + > + if (c == EOF) > + break; > + > + if (last == '\n' && c == '\n') > + break; > + > + last = c; > + } Another nitpick, maybe you need a function like skip_until_empty_line(fp) and repeat it like this. if (skip_until_empty_line(fp) < 0) goto out; if (skip_until_empty_line(fp) < 0) goto out; > + > + last = 0; > + > + /* Read until empty line (Skip Properties) */ > + while (true) { > + c = getc(fp); > + > + if (c == EOF) > + break; > + > + if (last == '\n' && c == '\n') > + break; > + > + last = c; > + } > + > + /* Read in print_fmt: */ > + while (len > 1) { > + c = getc(fp); > + > + if (c == EOF || c == '\n') > + break; > + > + buffer[index++] = c; > + > + len--; > + } And here you can use fgets(buffer, len, fp). Thank you, > + > + buffer[index] = 0; > + > + fclose(fp); > + > + return 0; > +} > + > static int clear(void) > { > int fd = open(data_file, O_RDWR); > @@ -63,6 +119,44 @@ static int clear(void) > return 0; > } > > +static int check_print_fmt(const char *event, const char *expected) > +{ > + struct user_reg reg = {0}; > + char print_fmt[256]; > + int ret; > + int fd; > + > + /* Ensure cleared */ > + ret = clear(); > + > + if (ret != 0) > + return ret; > + > + fd = open(data_file, O_RDWR); > + > + if (fd == -1) > + return fd; > + > + reg.size = sizeof(reg); > + reg.name_args = (__u64)event; > + > + /* Register should work */ > + ret = ioctl(fd, DIAG_IOCSREG, ®); > + > + close(fd); > + > + if (ret != 0) > + return ret; > + > + /* Ensure correct print_fmt */ > + ret = get_print_fmt(print_fmt, sizeof(print_fmt)); > + > + if (ret != 0) > + return ret; > + > + return strcmp(print_fmt, expected); > +} > + > FIXTURE(user) { > int status_fd; > int data_fd; > @@ -282,6 +376,78 @@ TEST_F(user, write_validator) { > ASSERT_EQ(EFAULT, errno); > } > > +TEST_F(user, print_fmt) { > + int ret; > + > + ret = check_print_fmt("__test_event __rel_loc char[] data", > + "print fmt: \"data=%s\", __get_rel_str(data)"); > + ASSERT_EQ(0, ret); > + > + ret = check_print_fmt("__test_event __data_loc char[] data", > + "print fmt: \"data=%s\", __get_str(data)"); > + ASSERT_EQ(0, ret); > + > + ret = check_print_fmt("__test_event s64 data", > + "print fmt: \"data=%lld\", REC->data"); > + ASSERT_EQ(0, ret); > + > + ret = check_print_fmt("__test_event u64 data", > + "print fmt: \"data=%llu\", REC->data"); > + ASSERT_EQ(0, ret); > + > + ret = check_print_fmt("__test_event s32 data", > + "print fmt: \"data=%d\", REC->data"); > + ASSERT_EQ(0, ret); > + > + ret = check_print_fmt("__test_event u32 data", > + "print fmt: \"data=%u\", REC->data"); > + ASSERT_EQ(0, ret); > + > + ret = check_print_fmt("__test_event int data", > + "print fmt: \"data=%d\", REC->data"); > + ASSERT_EQ(0, ret); > + > + ret = check_print_fmt("__test_event unsigned int data", > + "print fmt: \"data=%u\", REC->data"); > + ASSERT_EQ(0, ret); > + > + ret = check_print_fmt("__test_event s16 data", > + "print fmt: \"data=%d\", REC->data"); > + ASSERT_EQ(0, ret); > + > + ret = check_print_fmt("__test_event u16 data", > + "print fmt: \"data=%u\", REC->data"); > + ASSERT_EQ(0, ret); > + > + ret = check_print_fmt("__test_event short data", > + "print fmt: \"data=%d\", REC->data"); > + ASSERT_EQ(0, ret); > + > + ret = check_print_fmt("__test_event unsigned short data", > + "print fmt: \"data=%u\", REC->data"); > + ASSERT_EQ(0, ret); > + > + ret = check_print_fmt("__test_event s8 data", > + "print fmt: \"data=%d\", REC->data"); > + ASSERT_EQ(0, ret); > + > + ret = check_print_fmt("__test_event u8 data", > + "print fmt: \"data=%u\", REC->data"); > + ASSERT_EQ(0, ret); > + > + ret = check_print_fmt("__test_event char data", > + "print fmt: \"data=%d\", REC->data"); > + ASSERT_EQ(0, ret); > + > + ret = check_print_fmt("__test_event unsigned char data", > + "print fmt: \"data=%u\", REC->data"); > + ASSERT_EQ(0, ret); > + > + ret = check_print_fmt("__test_event char[4] data", > + "print fmt: \"data=%s\", REC->data"); > + ASSERT_EQ(0, ret); > +} > + > int main(int argc, char **argv) > { > return test_harness_run(argc, argv); > -- > 2.17.1 >
On Fri, Dec 10, 2021 at 10:23:27AM +0900, Masami Hiramatsu wrote: > Hi Beau, > > On Thu, 9 Dec 2021 14:32:10 -0800 > Beau Belgrave <beaub@linux.microsoft.com> wrote: > > > Switch between __get_str and __get_rel_str within the print_fmt of > > user_events. Add unit test to ensure print_fmt is correct on known > > types. > > > > Signed-off-by: Beau Belgrave <beaub@linux.microsoft.com> > > --- > > kernel/trace/trace_events_user.c | 24 ++- > > .../selftests/user_events/ftrace_test.c | 166 ++++++++++++++++++ > > 2 files changed, 182 insertions(+), 8 deletions(-) > > > > diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c > > index 56eb58ddb4cf..3779fa2ca14a 100644 > > --- a/kernel/trace/trace_events_user.c > > +++ b/kernel/trace/trace_events_user.c > > @@ -257,7 +257,7 @@ static int user_event_add_field(struct user_event *user, const char *type, > > goto add_field; > > > > add_validator: > > - if (strstr(type, "char[") != 0) > > + if (strstr(type, "char") != 0) > > validator_flags |= VALIDATOR_ENSURE_NULL; > > What is this change for? This seems not related to the other changes. > (Also, what happen if it is a single char type?) > I'm glad you asked, it appears like __data_loc / __rel_loc can take char as it's type (It doesn't appear to be limited to char[] cases). I wanted to ensure something malicious couldn't sneak past by using this corner case. IE: __data_loc char test In trace_events_filter.c: int filter_assign_type(const char *type) { if (strstr(type, "__data_loc") && strstr(type, "char")) return FILTER_DYN_STRING; if (strchr(type, '[') && strstr(type, "char")) return FILTER_STATIC_STRING; if (strcmp(type, "char *") == 0 || strcmp(type, "const char *") == 0) return FILTER_PTR_STRING; return FILTER_OTHER; } char[ is only checked if __data_loc is not specified. > > > > validator = kmalloc(sizeof(*validator), GFP_KERNEL); > > @@ -456,14 +456,21 @@ static const char *user_field_format(const char *type) > > return "%llu"; > > } > > > > -static bool user_field_is_dyn_string(const char *type) > > +static bool user_field_is_dyn_string(const char *type, const char **str_func) > > { > > - if (str_has_prefix(type, "__data_loc ") || > > - str_has_prefix(type, "__rel_loc ")) > > - if (strstr(type, "char[") != 0) > > - return true; > > + if (str_has_prefix(type, "__data_loc ")) { > > + *str_func = "__get_str"; > > + goto check; > > + } > > + > > + if (str_has_prefix(type, "__rel_loc ")) { > > + *str_func = "__get_rel_str"; > > + goto check; > > + } > > > > return false; > > +check: > > + return strstr(type, "char") != 0; > > } > > > > #define LEN_OR_ZERO (len ? len - pos : 0) > > @@ -472,6 +479,7 @@ static int user_event_set_print_fmt(struct user_event *user, char *buf, int len) > > struct ftrace_event_field *field, *next; > > struct list_head *head = &user->fields; > > int pos = 0, depth = 0; > > + const char *str_func; > > > > pos += snprintf(buf + pos, LEN_OR_ZERO, "\""); > > > > @@ -488,9 +496,9 @@ static int user_event_set_print_fmt(struct user_event *user, char *buf, int len) > > pos += snprintf(buf + pos, LEN_OR_ZERO, "\""); > > > > list_for_each_entry_safe_reverse(field, next, head, link) { > > - if (user_field_is_dyn_string(field->type)) > > + if (user_field_is_dyn_string(field->type, &str_func)) > > pos += snprintf(buf + pos, LEN_OR_ZERO, > > - ", __get_str(%s)", field->name); > > + ", %s(%s)", str_func, field->name); > > else > > pos += snprintf(buf + pos, LEN_OR_ZERO, > > ", REC->%s", field->name); > > diff --git a/tools/testing/selftests/user_events/ftrace_test.c b/tools/testing/selftests/user_events/ftrace_test.c > > Just a nitpick, if possible, please split this part from the kernel update. > I will try to do so, could you help me understand why I would split this out? (For future patches) I thought the intention of each would be to contain it's logical grouping: I wanted to show, yes the code changed, and yes we have a unit test for that new condition. > > index 16aff1fb295a..b2e5c0765a68 100644 > > --- a/tools/testing/selftests/user_events/ftrace_test.c > > +++ b/tools/testing/selftests/user_events/ftrace_test.c > > @@ -20,6 +20,7 @@ const char *data_file = "/sys/kernel/debug/tracing/user_events_data"; > > const char *status_file = "/sys/kernel/debug/tracing/user_events_status"; > > const char *enable_file = "/sys/kernel/debug/tracing/events/user_events/__test_event/enable"; > > const char *trace_file = "/sys/kernel/debug/tracing/trace"; > > +const char *fmt_file = "/sys/kernel/debug/tracing/events/user_events/__test_event/format"; > > > > static int trace_bytes(void) > > { > > @@ -47,6 +48,61 @@ static int trace_bytes(void) > > return bytes; > > } > > > > +static int get_print_fmt(char *buffer, int len) > > +{ > > + FILE *fp = fopen(fmt_file, "r"); > > + int c, index = 0, last = 0; > > + > > + if (!fp) > > + return -1; > > + > > + /* Read until empty line (Skip Common) */ > > + while (true) { > > + c = getc(fp); > > + > > + if (c == EOF) > > + break; > > + > > + if (last == '\n' && c == '\n') > > + break; > > + > > + last = c; > > + } > > Another nitpick, maybe you need a function like skip_until_empty_line(fp) > and repeat it like this. > > if (skip_until_empty_line(fp) < 0) > goto out; > if (skip_until_empty_line(fp) < 0) > goto out; > Sure thing. > > + > > + last = 0; > > + > > + /* Read until empty line (Skip Properties) */ > > + while (true) { > > + c = getc(fp); > > + > > + if (c == EOF) > > + break; > > + > > + if (last == '\n' && c == '\n') > > + break; > > + > > + last = c; > > + } > > + > > + /* Read in print_fmt: */ > > + while (len > 1) { > > + c = getc(fp); > > + > > + if (c == EOF || c == '\n') > > + break; > > + > > + buffer[index++] = c; > > + > > + len--; > > + } > > And here you can use fgets(buffer, len, fp). > Makes sense. > > Thank you, > > [..] > > -- > Masami Hiramatsu <mhiramat@kernel.org> Thanks, -Beau
On Fri, 10 Dec 2021 10:45:51 -0800 Beau Belgrave <beaub@linux.microsoft.com> wrote: > On Fri, Dec 10, 2021 at 10:23:27AM +0900, Masami Hiramatsu wrote: > > Hi Beau, > > > > On Thu, 9 Dec 2021 14:32:10 -0800 > > Beau Belgrave <beaub@linux.microsoft.com> wrote: > > > > > Switch between __get_str and __get_rel_str within the print_fmt of > > > user_events. Add unit test to ensure print_fmt is correct on known > > > types. > > > > > > Signed-off-by: Beau Belgrave <beaub@linux.microsoft.com> > > > --- > > > kernel/trace/trace_events_user.c | 24 ++- > > > .../selftests/user_events/ftrace_test.c | 166 ++++++++++++++++++ > > > 2 files changed, 182 insertions(+), 8 deletions(-) > > > > > > diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c > > > index 56eb58ddb4cf..3779fa2ca14a 100644 > > > --- a/kernel/trace/trace_events_user.c > > > +++ b/kernel/trace/trace_events_user.c > > > @@ -257,7 +257,7 @@ static int user_event_add_field(struct user_event *user, const char *type, > > > goto add_field; > > > > > > add_validator: > > > - if (strstr(type, "char[") != 0) > > > + if (strstr(type, "char") != 0) > > > validator_flags |= VALIDATOR_ENSURE_NULL; > > > > What is this change for? This seems not related to the other changes. > > (Also, what happen if it is a single char type?) > > > > I'm glad you asked, it appears like __data_loc / __rel_loc can take char > as it's type (It doesn't appear to be limited to char[] cases). I wanted > to ensure something malicious couldn't sneak past by using this corner > case. > > IE: __data_loc char test > > In trace_events_filter.c: > int filter_assign_type(const char *type) > { > if (strstr(type, "__data_loc") && strstr(type, "char")) > return FILTER_DYN_STRING; > > if (strchr(type, '[') && strstr(type, "char")) > return FILTER_STATIC_STRING; > > if (strcmp(type, "char *") == 0 || strcmp(type, "const char *") == 0) > return FILTER_PTR_STRING; > > return FILTER_OTHER; > } > > char[ is only checked if __data_loc is not specified. OK, but in that case, is this patch good place for that change? > > > > > > > validator = kmalloc(sizeof(*validator), GFP_KERNEL); > > > @@ -456,14 +456,21 @@ static const char *user_field_format(const char *type) > > > return "%llu"; > > > } > > > > > > -static bool user_field_is_dyn_string(const char *type) > > > +static bool user_field_is_dyn_string(const char *type, const char **str_func) > > > { > > > - if (str_has_prefix(type, "__data_loc ") || > > > - str_has_prefix(type, "__rel_loc ")) > > > - if (strstr(type, "char[") != 0) > > > - return true; > > > + if (str_has_prefix(type, "__data_loc ")) { > > > + *str_func = "__get_str"; > > > + goto check; > > > + } > > > + > > > + if (str_has_prefix(type, "__rel_loc ")) { > > > + *str_func = "__get_rel_str"; > > > + goto check; > > > + } > > > > > > return false; > > > +check: > > > + return strstr(type, "char") != 0; > > > } > > > > > > #define LEN_OR_ZERO (len ? len - pos : 0) > > > @@ -472,6 +479,7 @@ static int user_event_set_print_fmt(struct user_event *user, char *buf, int len) > > > struct ftrace_event_field *field, *next; > > > struct list_head *head = &user->fields; > > > int pos = 0, depth = 0; > > > + const char *str_func; > > > > > > pos += snprintf(buf + pos, LEN_OR_ZERO, "\""); > > > > > > @@ -488,9 +496,9 @@ static int user_event_set_print_fmt(struct user_event *user, char *buf, int len) > > > pos += snprintf(buf + pos, LEN_OR_ZERO, "\""); > > > > > > list_for_each_entry_safe_reverse(field, next, head, link) { > > > - if (user_field_is_dyn_string(field->type)) > > > + if (user_field_is_dyn_string(field->type, &str_func)) > > > pos += snprintf(buf + pos, LEN_OR_ZERO, > > > - ", __get_str(%s)", field->name); > > > + ", %s(%s)", str_func, field->name); > > > else > > > pos += snprintf(buf + pos, LEN_OR_ZERO, > > > ", REC->%s", field->name); > > > diff --git a/tools/testing/selftests/user_events/ftrace_test.c b/tools/testing/selftests/user_events/ftrace_test.c > > > > Just a nitpick, if possible, please split this part from the kernel update. > > > > I will try to do so, could you help me understand why I would split this > out? (For future patches) > > I thought the intention of each would be to contain it's logical grouping: > I wanted to show, yes the code changed, and yes we have a unit test for > that new condition. Hrm, in this specific case, maybe this can be acceptable. Following case you might need to take care of it. - if the feature and the test code are maintained by different maintainer. - if the test code is added much later than the feature. In both case, the piece of patches will be applied separately. The former case, by different maintainer, the latter case by different tree (e.g. stable tree may not have the test case.) BTW, I also think this change is a fix for the previous patches in the series. In that case, please update those patches so that the patch is completely works. That will be good for bisecting. Thank you, > > > > index 16aff1fb295a..b2e5c0765a68 100644 > > > --- a/tools/testing/selftests/user_events/ftrace_test.c > > > +++ b/tools/testing/selftests/user_events/ftrace_test.c > > > @@ -20,6 +20,7 @@ const char *data_file = "/sys/kernel/debug/tracing/user_events_data"; > > > const char *status_file = "/sys/kernel/debug/tracing/user_events_status"; > > > const char *enable_file = "/sys/kernel/debug/tracing/events/user_events/__test_event/enable"; > > > const char *trace_file = "/sys/kernel/debug/tracing/trace"; > > > +const char *fmt_file = "/sys/kernel/debug/tracing/events/user_events/__test_event/format"; > > > > > > static int trace_bytes(void) > > > { > > > @@ -47,6 +48,61 @@ static int trace_bytes(void) > > > return bytes; > > > } > > > > > > +static int get_print_fmt(char *buffer, int len) > > > +{ > > > + FILE *fp = fopen(fmt_file, "r"); > > > + int c, index = 0, last = 0; > > > + > > > + if (!fp) > > > + return -1; > > > + > > > + /* Read until empty line (Skip Common) */ > > > + while (true) { > > > + c = getc(fp); > > > + > > > + if (c == EOF) > > > + break; > > > + > > > + if (last == '\n' && c == '\n') > > > + break; > > > + > > > + last = c; > > > + } > > > > Another nitpick, maybe you need a function like skip_until_empty_line(fp) > > and repeat it like this. > > > > if (skip_until_empty_line(fp) < 0) > > goto out; > > if (skip_until_empty_line(fp) < 0) > > goto out; > > > > Sure thing. > > > > + > > > + last = 0; > > > + > > > + /* Read until empty line (Skip Properties) */ > > > + while (true) { > > > + c = getc(fp); > > > + > > > + if (c == EOF) > > > + break; > > > + > > > + if (last == '\n' && c == '\n') > > > + break; > > > + > > > + last = c; > > > + } > > > + > > > + /* Read in print_fmt: */ > > > + while (len > 1) { > > > + c = getc(fp); > > > + > > > + if (c == EOF || c == '\n') > > > + break; > > > + > > > + buffer[index++] = c; > > > + > > > + len--; > > > + } > > > > And here you can use fgets(buffer, len, fp). > > > > Makes sense. > > > > > Thank you, > > > > > > [..] > > > > > -- > > Masami Hiramatsu <mhiramat@kernel.org> > > Thanks, > -Beau
On Mon, Dec 13, 2021 at 12:12:15AM +0900, Masami Hiramatsu wrote: > On Fri, 10 Dec 2021 10:45:51 -0800 > Beau Belgrave <beaub@linux.microsoft.com> wrote: > > > On Fri, Dec 10, 2021 at 10:23:27AM +0900, Masami Hiramatsu wrote: > > > Hi Beau, > > > > > > On Thu, 9 Dec 2021 14:32:10 -0800 > > > Beau Belgrave <beaub@linux.microsoft.com> wrote: > > > > > > > Switch between __get_str and __get_rel_str within the print_fmt of > > > > user_events. Add unit test to ensure print_fmt is correct on known > > > > types. > > > > > > > > Signed-off-by: Beau Belgrave <beaub@linux.microsoft.com> > > > > --- > > > > kernel/trace/trace_events_user.c | 24 ++- > > > > .../selftests/user_events/ftrace_test.c | 166 ++++++++++++++++++ > > > > 2 files changed, 182 insertions(+), 8 deletions(-) > > > > > > > > diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c > > > > index 56eb58ddb4cf..3779fa2ca14a 100644 > > > > --- a/kernel/trace/trace_events_user.c > > > > +++ b/kernel/trace/trace_events_user.c > > > > @@ -257,7 +257,7 @@ static int user_event_add_field(struct user_event *user, const char *type, > > > > goto add_field; > > > > > > > > add_validator: > > > > - if (strstr(type, "char[") != 0) > > > > + if (strstr(type, "char") != 0) > > > > validator_flags |= VALIDATOR_ENSURE_NULL; > > > > > > What is this change for? This seems not related to the other changes. > > > (Also, what happen if it is a single char type?) > > > > > > > I'm glad you asked, it appears like __data_loc / __rel_loc can take char > > as it's type (It doesn't appear to be limited to char[] cases). I wanted > > to ensure something malicious couldn't sneak past by using this corner > > case. > > > > IE: __data_loc char test > > > > In trace_events_filter.c: > > int filter_assign_type(const char *type) > > { > > if (strstr(type, "__data_loc") && strstr(type, "char")) > > return FILTER_DYN_STRING; > > > > if (strchr(type, '[') && strstr(type, "char")) > > return FILTER_STATIC_STRING; > > > > if (strcmp(type, "char *") == 0 || strcmp(type, "const char *") == 0) > > return FILTER_PTR_STRING; > > > > return FILTER_OTHER; > > } > > > > char[ is only checked if __data_loc is not specified. > > OK, but in that case, is this patch good place for that change? > I'll move this to part 12. > > > > > > > > > > validator = kmalloc(sizeof(*validator), GFP_KERNEL); > > > > @@ -456,14 +456,21 @@ static const char *user_field_format(const char *type) > > > > return "%llu"; > > > > } > > > > > > > > -static bool user_field_is_dyn_string(const char *type) > > > > +static bool user_field_is_dyn_string(const char *type, const char **str_func) > > > > { > > > > - if (str_has_prefix(type, "__data_loc ") || > > > > - str_has_prefix(type, "__rel_loc ")) > > > > - if (strstr(type, "char[") != 0) > > > > - return true; > > > > + if (str_has_prefix(type, "__data_loc ")) { > > > > + *str_func = "__get_str"; > > > > + goto check; > > > > + } > > > > + > > > > + if (str_has_prefix(type, "__rel_loc ")) { > > > > + *str_func = "__get_rel_str"; > > > > + goto check; > > > > + } > > > > > > > > return false; > > > > +check: > > > > + return strstr(type, "char") != 0; > > > > } > > > > > > > > #define LEN_OR_ZERO (len ? len - pos : 0) > > > > @@ -472,6 +479,7 @@ static int user_event_set_print_fmt(struct user_event *user, char *buf, int len) > > > > struct ftrace_event_field *field, *next; > > > > struct list_head *head = &user->fields; > > > > int pos = 0, depth = 0; > > > > + const char *str_func; > > > > > > > > pos += snprintf(buf + pos, LEN_OR_ZERO, "\""); > > > > > > > > @@ -488,9 +496,9 @@ static int user_event_set_print_fmt(struct user_event *user, char *buf, int len) > > > > pos += snprintf(buf + pos, LEN_OR_ZERO, "\""); > > > > > > > > list_for_each_entry_safe_reverse(field, next, head, link) { > > > > - if (user_field_is_dyn_string(field->type)) > > > > + if (user_field_is_dyn_string(field->type, &str_func)) > > > > pos += snprintf(buf + pos, LEN_OR_ZERO, > > > > - ", __get_str(%s)", field->name); > > > > + ", %s(%s)", str_func, field->name); > > > > else > > > > pos += snprintf(buf + pos, LEN_OR_ZERO, > > > > ", REC->%s", field->name); > > > > diff --git a/tools/testing/selftests/user_events/ftrace_test.c b/tools/testing/selftests/user_events/ftrace_test.c > > > > > > Just a nitpick, if possible, please split this part from the kernel update. > > > > > > > I will try to do so, could you help me understand why I would split this > > out? (For future patches) > > > > I thought the intention of each would be to contain it's logical grouping: > > I wanted to show, yes the code changed, and yes we have a unit test for > > that new condition. > > Hrm, in this specific case, maybe this can be acceptable. Following > case you might need to take care of it. > > - if the feature and the test code are maintained by different maintainer. > - if the test code is added much later than the feature. > > In both case, the piece of patches will be applied separately. The former > case, by different maintainer, the latter case by different tree (e.g. > stable tree may not have the test case.) > > BTW, I also think this change is a fix for the previous patches in the series. > In that case, please update those patches so that the patch is completely works. > That will be good for bisecting. > Do you mean you want the rest of this change rolled into 04/13 (print_fmt generation)? And have the char vs char[ rolled into 12/13 (add validators)? I can then roll the unit test for this case under 05/13 (ftrace selftest). Thanks, -Beau
On Mon, 13 Dec 2021 10:47:23 -0800 Beau Belgrave <beaub@linux.microsoft.com> wrote: > On Mon, Dec 13, 2021 at 12:12:15AM +0900, Masami Hiramatsu wrote: > > On Fri, 10 Dec 2021 10:45:51 -0800 > > Beau Belgrave <beaub@linux.microsoft.com> wrote: > > > > > On Fri, Dec 10, 2021 at 10:23:27AM +0900, Masami Hiramatsu wrote: > > > > Hi Beau, > > > > > > > > On Thu, 9 Dec 2021 14:32:10 -0800 > > > > Beau Belgrave <beaub@linux.microsoft.com> wrote: > > > > > > > > > Switch between __get_str and __get_rel_str within the print_fmt of > > > > > user_events. Add unit test to ensure print_fmt is correct on known > > > > > types. > > > > > > > > > > Signed-off-by: Beau Belgrave <beaub@linux.microsoft.com> > > > > > --- > > > > > kernel/trace/trace_events_user.c | 24 ++- > > > > > .../selftests/user_events/ftrace_test.c | 166 ++++++++++++++++++ > > > > > 2 files changed, 182 insertions(+), 8 deletions(-) > > > > > > > > > > diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c > > > > > index 56eb58ddb4cf..3779fa2ca14a 100644 > > > > > --- a/kernel/trace/trace_events_user.c > > > > > +++ b/kernel/trace/trace_events_user.c > > > > > @@ -257,7 +257,7 @@ static int user_event_add_field(struct user_event *user, const char *type, > > > > > goto add_field; > > > > > > > > > > add_validator: > > > > > - if (strstr(type, "char[") != 0) > > > > > + if (strstr(type, "char") != 0) > > > > > validator_flags |= VALIDATOR_ENSURE_NULL; > > > > > > > > What is this change for? This seems not related to the other changes. > > > > (Also, what happen if it is a single char type?) > > > > > > > > > > I'm glad you asked, it appears like __data_loc / __rel_loc can take char > > > as it's type (It doesn't appear to be limited to char[] cases). I wanted > > > to ensure something malicious couldn't sneak past by using this corner > > > case. > > > > > > IE: __data_loc char test > > > > > > In trace_events_filter.c: > > > int filter_assign_type(const char *type) > > > { > > > if (strstr(type, "__data_loc") && strstr(type, "char")) > > > return FILTER_DYN_STRING; > > > > > > if (strchr(type, '[') && strstr(type, "char")) > > > return FILTER_STATIC_STRING; > > > > > > if (strcmp(type, "char *") == 0 || strcmp(type, "const char *") == 0) > > > return FILTER_PTR_STRING; > > > > > > return FILTER_OTHER; > > > } > > > > > > char[ is only checked if __data_loc is not specified. > > > > OK, but in that case, is this patch good place for that change? > > > > I'll move this to part 12. > > > > > > > > > > > > > > validator = kmalloc(sizeof(*validator), GFP_KERNEL); > > > > > @@ -456,14 +456,21 @@ static const char *user_field_format(const char *type) > > > > > return "%llu"; > > > > > } > > > > > > > > > > -static bool user_field_is_dyn_string(const char *type) > > > > > +static bool user_field_is_dyn_string(const char *type, const char **str_func) > > > > > { > > > > > - if (str_has_prefix(type, "__data_loc ") || > > > > > - str_has_prefix(type, "__rel_loc ")) > > > > > - if (strstr(type, "char[") != 0) > > > > > - return true; > > > > > + if (str_has_prefix(type, "__data_loc ")) { > > > > > + *str_func = "__get_str"; > > > > > + goto check; > > > > > + } > > > > > + > > > > > + if (str_has_prefix(type, "__rel_loc ")) { > > > > > + *str_func = "__get_rel_str"; > > > > > + goto check; > > > > > + } > > > > > > > > > > return false; > > > > > +check: > > > > > + return strstr(type, "char") != 0; > > > > > } > > > > > > > > > > #define LEN_OR_ZERO (len ? len - pos : 0) > > > > > @@ -472,6 +479,7 @@ static int user_event_set_print_fmt(struct user_event *user, char *buf, int len) > > > > > struct ftrace_event_field *field, *next; > > > > > struct list_head *head = &user->fields; > > > > > int pos = 0, depth = 0; > > > > > + const char *str_func; > > > > > > > > > > pos += snprintf(buf + pos, LEN_OR_ZERO, "\""); > > > > > > > > > > @@ -488,9 +496,9 @@ static int user_event_set_print_fmt(struct user_event *user, char *buf, int len) > > > > > pos += snprintf(buf + pos, LEN_OR_ZERO, "\""); > > > > > > > > > > list_for_each_entry_safe_reverse(field, next, head, link) { > > > > > - if (user_field_is_dyn_string(field->type)) > > > > > + if (user_field_is_dyn_string(field->type, &str_func)) > > > > > pos += snprintf(buf + pos, LEN_OR_ZERO, > > > > > - ", __get_str(%s)", field->name); > > > > > + ", %s(%s)", str_func, field->name); > > > > > else > > > > > pos += snprintf(buf + pos, LEN_OR_ZERO, > > > > > ", REC->%s", field->name); > > > > > diff --git a/tools/testing/selftests/user_events/ftrace_test.c b/tools/testing/selftests/user_events/ftrace_test.c > > > > > > > > Just a nitpick, if possible, please split this part from the kernel update. > > > > > > > > > > I will try to do so, could you help me understand why I would split this > > > out? (For future patches) > > > > > > I thought the intention of each would be to contain it's logical grouping: > > > I wanted to show, yes the code changed, and yes we have a unit test for > > > that new condition. > > > > Hrm, in this specific case, maybe this can be acceptable. Following > > case you might need to take care of it. > > > > - if the feature and the test code are maintained by different maintainer. > > - if the test code is added much later than the feature. > > > > In both case, the piece of patches will be applied separately. The former > > case, by different maintainer, the latter case by different tree (e.g. > > stable tree may not have the test case.) > > > > BTW, I also think this change is a fix for the previous patches in the series. > > In that case, please update those patches so that the patch is completely works. > > That will be good for bisecting. > > > > Do you mean you want the rest of this change rolled into 04/13 (print_fmt > generation)? > > And have the char vs char[ rolled into 12/13 (add validators)? Sure, both are yes :) > > I can then roll the unit test for this case under 05/13 (ftrace > selftest). That's also good to me :) Thank you! > > Thanks, > -Beau >
diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c index 56eb58ddb4cf..3779fa2ca14a 100644 --- a/kernel/trace/trace_events_user.c +++ b/kernel/trace/trace_events_user.c @@ -257,7 +257,7 @@ static int user_event_add_field(struct user_event *user, const char *type, goto add_field; add_validator: - if (strstr(type, "char[") != 0) + if (strstr(type, "char") != 0) validator_flags |= VALIDATOR_ENSURE_NULL; validator = kmalloc(sizeof(*validator), GFP_KERNEL); @@ -456,14 +456,21 @@ static const char *user_field_format(const char *type) return "%llu"; } -static bool user_field_is_dyn_string(const char *type) +static bool user_field_is_dyn_string(const char *type, const char **str_func) { - if (str_has_prefix(type, "__data_loc ") || - str_has_prefix(type, "__rel_loc ")) - if (strstr(type, "char[") != 0) - return true; + if (str_has_prefix(type, "__data_loc ")) { + *str_func = "__get_str"; + goto check; + } + + if (str_has_prefix(type, "__rel_loc ")) { + *str_func = "__get_rel_str"; + goto check; + } return false; +check: + return strstr(type, "char") != 0; } #define LEN_OR_ZERO (len ? len - pos : 0) @@ -472,6 +479,7 @@ static int user_event_set_print_fmt(struct user_event *user, char *buf, int len) struct ftrace_event_field *field, *next; struct list_head *head = &user->fields; int pos = 0, depth = 0; + const char *str_func; pos += snprintf(buf + pos, LEN_OR_ZERO, "\""); @@ -488,9 +496,9 @@ static int user_event_set_print_fmt(struct user_event *user, char *buf, int len) pos += snprintf(buf + pos, LEN_OR_ZERO, "\""); list_for_each_entry_safe_reverse(field, next, head, link) { - if (user_field_is_dyn_string(field->type)) + if (user_field_is_dyn_string(field->type, &str_func)) pos += snprintf(buf + pos, LEN_OR_ZERO, - ", __get_str(%s)", field->name); + ", %s(%s)", str_func, field->name); else pos += snprintf(buf + pos, LEN_OR_ZERO, ", REC->%s", field->name); diff --git a/tools/testing/selftests/user_events/ftrace_test.c b/tools/testing/selftests/user_events/ftrace_test.c index 16aff1fb295a..b2e5c0765a68 100644 --- a/tools/testing/selftests/user_events/ftrace_test.c +++ b/tools/testing/selftests/user_events/ftrace_test.c @@ -20,6 +20,7 @@ const char *data_file = "/sys/kernel/debug/tracing/user_events_data"; const char *status_file = "/sys/kernel/debug/tracing/user_events_status"; const char *enable_file = "/sys/kernel/debug/tracing/events/user_events/__test_event/enable"; const char *trace_file = "/sys/kernel/debug/tracing/trace"; +const char *fmt_file = "/sys/kernel/debug/tracing/events/user_events/__test_event/format"; static int trace_bytes(void) { @@ -47,6 +48,61 @@ static int trace_bytes(void) return bytes; } +static int get_print_fmt(char *buffer, int len) +{ + FILE *fp = fopen(fmt_file, "r"); + int c, index = 0, last = 0; + + if (!fp) + return -1; + + /* Read until empty line (Skip Common) */ + while (true) { + c = getc(fp); + + if (c == EOF) + break; + + if (last == '\n' && c == '\n') + break; + + last = c; + } + + last = 0; + + /* Read until empty line (Skip Properties) */ + while (true) { + c = getc(fp); + + if (c == EOF) + break; + + if (last == '\n' && c == '\n') + break; + + last = c; + } + + /* Read in print_fmt: */ + while (len > 1) { + c = getc(fp); + + if (c == EOF || c == '\n') + break; + + buffer[index++] = c; + + len--; + } + + buffer[index] = 0; + + fclose(fp); + + return 0; +} + static int clear(void) { int fd = open(data_file, O_RDWR); @@ -63,6 +119,44 @@ static int clear(void) return 0; } +static int check_print_fmt(const char *event, const char *expected) +{ + struct user_reg reg = {0}; + char print_fmt[256]; + int ret; + int fd; + + /* Ensure cleared */ + ret = clear(); + + if (ret != 0) + return ret; + + fd = open(data_file, O_RDWR); + + if (fd == -1) + return fd; + + reg.size = sizeof(reg); + reg.name_args = (__u64)event; + + /* Register should work */ + ret = ioctl(fd, DIAG_IOCSREG, ®); + + close(fd); + + if (ret != 0) + return ret; + + /* Ensure correct print_fmt */ + ret = get_print_fmt(print_fmt, sizeof(print_fmt)); + + if (ret != 0) + return ret; + + return strcmp(print_fmt, expected); +} + FIXTURE(user) { int status_fd; int data_fd; @@ -282,6 +376,78 @@ TEST_F(user, write_validator) { ASSERT_EQ(EFAULT, errno); } +TEST_F(user, print_fmt) { + int ret; + + ret = check_print_fmt("__test_event __rel_loc char[] data", + "print fmt: \"data=%s\", __get_rel_str(data)"); + ASSERT_EQ(0, ret); + + ret = check_print_fmt("__test_event __data_loc char[] data", + "print fmt: \"data=%s\", __get_str(data)"); + ASSERT_EQ(0, ret); + + ret = check_print_fmt("__test_event s64 data", + "print fmt: \"data=%lld\", REC->data"); + ASSERT_EQ(0, ret); + + ret = check_print_fmt("__test_event u64 data", + "print fmt: \"data=%llu\", REC->data"); + ASSERT_EQ(0, ret); + + ret = check_print_fmt("__test_event s32 data", + "print fmt: \"data=%d\", REC->data"); + ASSERT_EQ(0, ret); + + ret = check_print_fmt("__test_event u32 data", + "print fmt: \"data=%u\", REC->data"); + ASSERT_EQ(0, ret); + + ret = check_print_fmt("__test_event int data", + "print fmt: \"data=%d\", REC->data"); + ASSERT_EQ(0, ret); + + ret = check_print_fmt("__test_event unsigned int data", + "print fmt: \"data=%u\", REC->data"); + ASSERT_EQ(0, ret); + + ret = check_print_fmt("__test_event s16 data", + "print fmt: \"data=%d\", REC->data"); + ASSERT_EQ(0, ret); + + ret = check_print_fmt("__test_event u16 data", + "print fmt: \"data=%u\", REC->data"); + ASSERT_EQ(0, ret); + + ret = check_print_fmt("__test_event short data", + "print fmt: \"data=%d\", REC->data"); + ASSERT_EQ(0, ret); + + ret = check_print_fmt("__test_event unsigned short data", + "print fmt: \"data=%u\", REC->data"); + ASSERT_EQ(0, ret); + + ret = check_print_fmt("__test_event s8 data", + "print fmt: \"data=%d\", REC->data"); + ASSERT_EQ(0, ret); + + ret = check_print_fmt("__test_event u8 data", + "print fmt: \"data=%u\", REC->data"); + ASSERT_EQ(0, ret); + + ret = check_print_fmt("__test_event char data", + "print fmt: \"data=%d\", REC->data"); + ASSERT_EQ(0, ret); + + ret = check_print_fmt("__test_event unsigned char data", + "print fmt: \"data=%u\", REC->data"); + ASSERT_EQ(0, ret); + + ret = check_print_fmt("__test_event char[4] data", + "print fmt: \"data=%s\", REC->data"); + ASSERT_EQ(0, ret); +} + int main(int argc, char **argv) { return test_harness_run(argc, argv);
Switch between __get_str and __get_rel_str within the print_fmt of user_events. Add unit test to ensure print_fmt is correct on known types. Signed-off-by: Beau Belgrave <beaub@linux.microsoft.com> --- kernel/trace/trace_events_user.c | 24 ++- .../selftests/user_events/ftrace_test.c | 166 ++++++++++++++++++ 2 files changed, 182 insertions(+), 8 deletions(-)