apparent bogosity in unregister_ftrace_function_probe_func()

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).

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...

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 -

(quoted diff) +4 -2

-               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

(quoted diff) +3 -2

-       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?

Quoted 6 lines

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..

Quoted 11 lines

Quoted 6 lines

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..

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...

Quoted 11 lines · Dmitry Safonov

Quoted 6 lines

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..

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...

Quoted 42 lines · On Sat, 27 Jan 2018 17:07:48 +0000 Al Viro <[email protected]> wrote:

Quoted 11 lines · Dmitry Safonov

Quoted 6 lines

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..

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;

Quoted 2 lines

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!

Quoted 48 lines · On Mon, 29 Jan 2018 22:59:42 +0900 Masami Hiramatsu <[email protected]> wrote:

Quoted 44 lines · On Sat, 27 Jan 2018 17:07:48 +0000 Al Viro <[email protected]> wrote:

Quoted 11 lines · Dmitry Safonov

Quoted 6 lines

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..

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.

Quoted 2 lines

Looks nice to me!

I'll implement this code giving Al credit and referencing this email thread. Anyone have objections to that?

Quoted 48 lines · On Mon, 29 Jan 2018 22:59:42 +0900 Masami Hiramatsu <[email protected]> wrote:

Quoted 44 lines · On Sat, 27 Jan 2018 17:07:48 +0000 Al Viro <[email protected]> wrote:

Quoted 11 lines · Dmitry Safonov

Quoted 6 lines

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..

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.

Quoted 2 lines

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?

Quoted 2 lines · On Tue, 6 Feb 2018 11:26:14 +0900 Masami Hiramatsu <[email protected]> wrote:

No, that code looks good to me. :)

Quoted 2 lines

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 :)

Quoted 4 lines · 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.

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.