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@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx>
> > > > ---
> > > > 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
|