Re: [RFC] apparent bogosity in unregister_ftrace_function_probe_func()

[Dmitry Safonov]

Hi Alexander,

2018-01-27 3:17 GMT+00:00 Al Viro <viro@xxxxxxxxxxxxxxxxxx>:
> It contains something very odd:
>
>                 func_g.type = filter_parse_regex(glob, strlen(glob),
>                                                  &func_g.search, &not);
>                 func_g.len = strlen(func_g.search);
>                 func_g.search = glob;
>
>                 /* we do not support '!' for function probes */
>                 if (WARN_ON(not))
>                         return -EINVAL;
>
> What the hell is the last assignment for?  After that call of
> filter_parse_regex() we could have func_g.search not equal to glob
> only if glob started with '!' or '*'.  In the former case we would've
> buggered off with -EINVAL (not = 1).  In the latter we would've set
> func_g.search equal to glob + 1, calculated the length of that thing
> in func_g.len and proceeded to reset func_g.search back to glob.
>
> Suppose the glob is e.g. *foo*.  We end up with
>         func_g.type = MATCH_MIDDLE_ONLY;
>         func_g.len = 3;
>         func_g.search = "*foo";
> Feeding that to ftrace_match_record() will not do anything sane - we
> will be looking for names containing "*foo" (->len is ignored for that
> one).

Yes, that definitely smells bogus.

> Incidentally, shouldn't filter_parse_regex("*[ab]", 5, &s, &not)
> end up with s = "*[ab]"?  We are returning MATCH_GLOB, after all,
> so we want the entire pattern there...  I would've assumed that
> this is what the code in unregister_ftrace_function_probe_func()
> is trying to compensate for, the first oddity predates MATCH_GLOB...

No, I don't think filter_parse_regex() should return the full regex..
ftrace_match() expects search would be processed string, not a glob.
So, this unnecessary assignment broke unregistering multiple kprobs
with a middle/end pattern..

> In any case, that should be done in filter_parse_regex() itself -
> there are other callers that don't have such compensation and
> it does the wrong thing for MATCH_MIDDLE_ONLY and MATCH_END_ONLY
> cases...
>
> That started in commit 3ba009297149fa45956c33ab5de7c5f4da1f28b8
> Author: Dmitry Safonov <0x7f454c46@xxxxxxxxx>
> Date:   Tue Sep 29 19:46:14 2015 +0300
>
>     ftrace: Introduce ftrace_glob structure
>
> without any explanation -
> -               type = filter_parse_regex(glob, strlen(glob), &search, &not);
> -               len = strlen(search);
> +               func_g.type = filter_parse_regex(glob, strlen(glob),
> +                                                &func_g.search, &not);
> +               func_g.len = strlen(func_g.search);
> +               func_g.search = glob;
>
> Note in the same commit
> -       type = filter_parse_regex(glob, strlen(glob), &search, &not);
> -       len = strlen(search);
> +       func_g.type = filter_parse_regex(glob, strlen(glob),
> +                       &func_g.search, &not);
> +       func_g.len = strlen(func_g.search);
> nearby (in register_ftrace_function_probe()).
>
> What am I missing here?

No, I think you don't miss anything. The patch shouldn't have changed
any behaviour as it was merely an introduction of a new struct.
Ugh, sorry for the bogus - I'll prepare a patch and will check selftests
so they will check this pattern.

Thanks for the report,
             Dmitry