Messages in this thread |  | | From | Dmitry Safonov <> | Date | Sat, 27 Jan 2018 13:59:56 +0000 | Subject | Re: [RFC] apparent bogosity in unregister_ftrace_function_probe_func() |
| |
Hi Alexander,
2018-01-27 3:17 GMT+00:00 Al Viro <viro@zeniv.linux.org.uk>: > It contains something very odd: > > func_g.type = filter_parse_regex(glob, strlen(glob), > &func_g.search, ¬); > 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, ¬) > 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@gmail.com> > 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, ¬); > - len = strlen(search); > + func_g.type = filter_parse_regex(glob, strlen(glob), > + &func_g.search, ¬); > + func_g.len = strlen(func_g.search); > + func_g.search = glob; > > Note in the same commit > - type = filter_parse_regex(glob, strlen(glob), &search, ¬); > - len = strlen(search); > + func_g.type = filter_parse_regex(glob, strlen(glob), > + &func_g.search, ¬); > + 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
|  |