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).
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...
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 <[email protected]>
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?
Hi Alexander,
2018-01-27 3:17 GMT+00:00 Al Viro <[email protected]>:
> 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 <[email protected]>
> 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
On Sat, Jan 27, 2018 at 01:59:56PM +0000, Dmitry Safonov wrote:
>
> > 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..
For substring - sure, but what about something like "*a*b" and "a*b"?
AFAICS, filter_parse_regex() ends up with identical results in both
cases - MATCH_GLOB and *search = "a*b". And no way for the caller
to tell one from another.
IOW, it's a different bug sometimes obscured by the one in
unregister_ftrace_function_probe_func(). filter_parse_regex()
ought to revert to *search = buff; when it decides to return
MATCH_GLOB. Or something like
for (i = 0; i < len; i++) {
if (buff[i] == '*') {
if (!i) {
type = MATCH_END_ONLY;
} else if (i == len - 1) {
if (type == MATCH_END_ONLY)
type = MATCH_MIDDLE_ONLY;
else
type = MATCH_FRONT_ONLY;
buff[i] = 0;
break;
} else { /* pattern continues, use full glob */
return MATCH_GLOB;
}
} else if (strchr("[?\\", buff[i])) {
return MATCH_GLOB;
}
}
if (buff[0] == '*')
*search = buff + 1;
for that matter - i.e. delay that "we want everything past the first character"
until we are certain it's not a MATCH_GLOB.
That one was introduced by "ftrace: Support full glob matching" in 2016, AFAICS...
On Sat, 27 Jan 2018 17:07:48 +0000
Al Viro <[email protected]> wrote:
Hi Al,
> On Sat, Jan 27, 2018 at 01:59:56PM +0000, Dmitry Safonov wrote:
> >
> > > 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..
>
> For substring - sure, but what about something like "*a*b" and "a*b"?
> AFAICS, filter_parse_regex() ends up with identical results in both
> cases - MATCH_GLOB and *search = "a*b". And no way for the caller
> to tell one from another.
>
> IOW, it's a different bug sometimes obscured by the one in
> unregister_ftrace_function_probe_func(). filter_parse_regex()
> ought to revert to *search = buff; when it decides to return
> MATCH_GLOB. Or something like
> for (i = 0; i < len; i++) {
> if (buff[i] == '*') {
> if (!i) {
> type = MATCH_END_ONLY;
> } else if (i == len - 1) {
> if (type == MATCH_END_ONLY)
> type = MATCH_MIDDLE_ONLY;
> else
> type = MATCH_FRONT_ONLY;
> buff[i] = 0;
> break;
> } else { /* pattern continues, use full glob */
> return MATCH_GLOB;
> }
> } else if (strchr("[?\\", buff[i])) {
> return MATCH_GLOB;
> }
> }
> if (buff[0] == '*')
> *search = buff + 1;
> for that matter - i.e. delay that "we want everything past the first character"
> until we are certain it's not a MATCH_GLOB.
>
> That one was introduced by "ftrace: Support full glob matching" in 2016, AFAICS...
Yep, I totally agree. This code is one of those places that haven't had
the loving it deserved. It was one of our neglected children.
Thanks for taking a look here. I'm a bit embarrassed by this, and
should have audited it more. I'll have to rip into it and see what else
may be incorrect.
-- Steve
On Sat, 27 Jan 2018 03:17:06 +0000
Al Viro <[email protected]> wrote:
> 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.
Ah, right. It must be a bug! func_g.search should be assigned in
filter_parse_regex(), and it should be "glob" without "!" if
it is MATCH_GLOB. Of course above assignment should be removed.
Thank you,
--
Masami Hiramatsu <[email protected]>
On Sat, 27 Jan 2018 17:07:48 +0000
Al Viro <[email protected]> wrote:
> On Sat, Jan 27, 2018 at 01:59:56PM +0000, Dmitry Safonov wrote:
> >
> > > 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..
>
> For substring - sure, but what about something like "*a*b" and "a*b"?
> AFAICS, filter_parse_regex() ends up with identical results in both
> cases - MATCH_GLOB and *search = "a*b". And no way for the caller
> to tell one from another.
>
> IOW, it's a different bug sometimes obscured by the one in
> unregister_ftrace_function_probe_func(). filter_parse_regex()
> ought to revert to *search = buff; when it decides to return
> MATCH_GLOB. Or something like
> for (i = 0; i < len; i++) {
> if (buff[i] == '*') {
> if (!i) {
> type = MATCH_END_ONLY;
> } else if (i == len - 1) {
> if (type == MATCH_END_ONLY)
> type = MATCH_MIDDLE_ONLY;
> else
> type = MATCH_FRONT_ONLY;
> buff[i] = 0;
> break;
> } else { /* pattern continues, use full glob */
> return MATCH_GLOB;
> }
> } else if (strchr("[?\\", buff[i])) {
> return MATCH_GLOB;
> }
> }
> if (buff[0] == '*')
> *search = buff + 1;
> for that matter - i.e. delay that "we want everything past the first character"
> until we are certain it's not a MATCH_GLOB.
Looks nice to me!
> That one was introduced by "ftrace: Support full glob matching" in 2016, AFAICS...
Yes, that was my fault...
Thank you for pointing it out!
--
Masami Hiramatsu <[email protected]>
On Mon, 29 Jan 2018 22:59:42 +0900
Masami Hiramatsu <[email protected]> wrote:
> On Sat, 27 Jan 2018 17:07:48 +0000
> Al Viro <[email protected]> wrote:
>
> > On Sat, Jan 27, 2018 at 01:59:56PM +0000, Dmitry Safonov wrote:
> > >
> > > > 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..
> >
> > For substring - sure, but what about something like "*a*b" and "a*b"?
> > AFAICS, filter_parse_regex() ends up with identical results in both
> > cases - MATCH_GLOB and *search = "a*b". And no way for the caller
> > to tell one from another.
> >
> > IOW, it's a different bug sometimes obscured by the one in
> > unregister_ftrace_function_probe_func(). filter_parse_regex()
> > ought to revert to *search = buff; when it decides to return
> > MATCH_GLOB. Or something like
> > for (i = 0; i < len; i++) {
> > if (buff[i] == '*') {
> > if (!i) {
> > type = MATCH_END_ONLY;
> > } else if (i == len - 1) {
> > if (type == MATCH_END_ONLY)
> > type = MATCH_MIDDLE_ONLY;
> > else
> > type = MATCH_FRONT_ONLY;
> > buff[i] = 0;
> > break;
> > } else { /* pattern continues, use full glob */
> > return MATCH_GLOB;
> > }
> > } else if (strchr("[?\\", buff[i])) {
> > return MATCH_GLOB;
> > }
> > }
> > if (buff[0] == '*')
> > *search = buff + 1;
> > for that matter - i.e. delay that "we want everything past the first character"
> > until we are certain it's not a MATCH_GLOB.
>
> Looks nice to me!
>
I'll implement this code giving Al credit and referencing this email
thread. Anyone have objections to that?
Thanks!
-- Steve
2018-02-05 22:54 GMT+00:00 Steven Rostedt <[email protected]>:
> On Mon, 29 Jan 2018 22:59:42 +0900
> Masami Hiramatsu <[email protected]> wrote:
>
>> On Sat, 27 Jan 2018 17:07:48 +0000
>> Al Viro <[email protected]> wrote:
>>
>> > On Sat, Jan 27, 2018 at 01:59:56PM +0000, Dmitry Safonov wrote:
>> > >
>> > > > 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..
>> >
>> > For substring - sure, but what about something like "*a*b" and "a*b"?
>> > AFAICS, filter_parse_regex() ends up with identical results in both
>> > cases - MATCH_GLOB and *search = "a*b". And no way for the caller
>> > to tell one from another.
>> >
>> > IOW, it's a different bug sometimes obscured by the one in
>> > unregister_ftrace_function_probe_func(). filter_parse_regex()
>> > ought to revert to *search = buff; when it decides to return
>> > MATCH_GLOB. Or something like
>> > for (i = 0; i < len; i++) {
>> > if (buff[i] == '*') {
>> > if (!i) {
>> > type = MATCH_END_ONLY;
>> > } else if (i == len - 1) {
>> > if (type == MATCH_END_ONLY)
>> > type = MATCH_MIDDLE_ONLY;
>> > else
>> > type = MATCH_FRONT_ONLY;
>> > buff[i] = 0;
>> > break;
>> > } else { /* pattern continues, use full glob */
>> > return MATCH_GLOB;
>> > }
>> > } else if (strchr("[?\\", buff[i])) {
>> > return MATCH_GLOB;
>> > }
>> > }
>> > if (buff[0] == '*')
>> > *search = buff + 1;
>> > for that matter - i.e. delay that "we want everything past the first character"
>> > until we are certain it's not a MATCH_GLOB.
>>
>> Looks nice to me!
>>
>
> I'll implement this code giving Al credit and referencing this email
> thread. Anyone have objections to that?
Thank you, Steven.
No objections from me.
--
Dmitry
On Mon, 5 Feb 2018 17:54:33 -0500
Steven Rostedt <[email protected]> wrote:
> On Mon, 29 Jan 2018 22:59:42 +0900
> Masami Hiramatsu <[email protected]> wrote:
>
> > On Sat, 27 Jan 2018 17:07:48 +0000
> > Al Viro <[email protected]> wrote:
> >
> > > On Sat, Jan 27, 2018 at 01:59:56PM +0000, Dmitry Safonov wrote:
> > > >
> > > > > 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..
> > >
> > > For substring - sure, but what about something like "*a*b" and "a*b"?
> > > AFAICS, filter_parse_regex() ends up with identical results in both
> > > cases - MATCH_GLOB and *search = "a*b". And no way for the caller
> > > to tell one from another.
> > >
> > > IOW, it's a different bug sometimes obscured by the one in
> > > unregister_ftrace_function_probe_func(). filter_parse_regex()
> > > ought to revert to *search = buff; when it decides to return
> > > MATCH_GLOB. Or something like
> > > for (i = 0; i < len; i++) {
> > > if (buff[i] == '*') {
> > > if (!i) {
> > > type = MATCH_END_ONLY;
> > > } else if (i == len - 1) {
> > > if (type == MATCH_END_ONLY)
> > > type = MATCH_MIDDLE_ONLY;
> > > else
> > > type = MATCH_FRONT_ONLY;
> > > buff[i] = 0;
> > > break;
> > > } else { /* pattern continues, use full glob */
> > > return MATCH_GLOB;
> > > }
> > > } else if (strchr("[?\\", buff[i])) {
> > > return MATCH_GLOB;
> > > }
> > > }
> > > if (buff[0] == '*')
> > > *search = buff + 1;
> > > for that matter - i.e. delay that "we want everything past the first character"
> > > until we are certain it's not a MATCH_GLOB.
> >
> > Looks nice to me!
> >
>
> I'll implement this code giving Al credit and referencing this email
> thread. Anyone have objections to that?
No, that code looks good to me. :)
BTW, did you also remove "search = buff;" line in
unregister_ftrace_function_probe_func() too?
Thanks!
--
Masami Hiramatsu <[email protected]>
On Tue, 6 Feb 2018 11:26:14 +0900
Masami Hiramatsu <[email protected]> wrote:
> No, that code looks good to me. :)
>
> BTW, did you also remove "search = buff;" line in
> unregister_ftrace_function_probe_func() too?
That's a separate bug, and should be a separate patch. Dmitry mentioned
that he was preparing a patch to fix that.
Dmitry did you have a patch and added selftests to check it? If not,
I'll whip one up.
Thanks,
-- Steve
2018-02-06 2:40 GMT+00:00 Steven Rostedt <[email protected]>:
> On Tue, 6 Feb 2018 11:26:14 +0900
> Masami Hiramatsu <[email protected]> wrote:
>
>> No, that code looks good to me. :)
>>
>> BTW, did you also remove "search = buff;" line in
>> unregister_ftrace_function_probe_func() too?
>
> That's a separate bug, and should be a separate patch. Dmitry mentioned
> that he was preparing a patch to fix that.
>
> Dmitry did you have a patch and added selftests to check it? If not,
> I'll whip one up.
Yes, I've planned to do this..
As it's merge-window now I thought doing this a week later.
So, it's up to you - just let me know so we will not end doing the same fix :)
--
Dmitry
On Tue, 6 Feb 2018 02:44:03 +0000
Dmitry Safonov <[email protected]> wrote:
> Yes, I've planned to do this..
> As it's merge-window now I thought doing this a week later.
> So, it's up to you - just let me know so we will not end doing the same fix :)
>
If you haven't done it already, I'll just do it. As it will not be much
more work with the tests I'll add for Al's updates.
Note, these are bug fixes (and will mark them for stable), and I plan to
send to Linus as soon as they are ready. They don't need to wait for the
merge window.
Thanks!
-- Steve
2018-02-06 2:48 GMT+00:00 Steven Rostedt <[email protected]>:
> On Tue, 6 Feb 2018 02:44:03 +0000
> Dmitry Safonov <[email protected]> wrote:
>
>
>> Yes, I've planned to do this..
>> As it's merge-window now I thought doing this a week later.
>> So, it's up to you - just let me know so we will not end doing the same fix :)
>>
>
> If you haven't done it already, I'll just do it. As it will not be much
> more work with the tests I'll add for Al's updates.
>
> Note, these are bug fixes (and will mark them for stable), and I plan to
> send to Linus as soon as they are ready. They don't need to wait for the
> merge window.
Ok, then please, do it.
Thanks,
Dmitry