For '*foo' pattern, we should allow any string ending with
'foo', but ftrace filter incorrectly disallows strings
like bar_foo_foo:
# echo '*io' > set_ftrace_filter
# cat set_ftrace_filter | grep 'req_bio_endio'
# cat available_filter_functions | grep 'req_bio_endio'
req_bio_endio
Signed-off-by: Li Zefan <[email protected]>
---
kernel/trace/ftrace.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index e51a1bc..613dfb1 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1690,7 +1690,7 @@ ftrace_regex_lseek(struct file *file, loff_t offset, int origin)
static int ftrace_match(char *str, char *regex, int len, int type)
{
int matched = 0;
- char *ptr;
+ int slen;
switch (type) {
case MATCH_FULL:
@@ -1706,8 +1706,8 @@ static int ftrace_match(char *str, char *regex, int len, int type)
matched = 1;
break;
case MATCH_END_ONLY:
- ptr = strstr(str, regex);
- if (ptr && (ptr[len] == 0))
+ slen = strlen(str);
+ if (slen >= len && memcpy(str + slen - len, regex, len))
matched = 1;
break;
}
--
1.6.3
MATCH_FULL matching for PTR_STRING is not working correctly:
# echo 'func == vt' > events/bkl/lock_kernel/filter
# echo 1 > events/bkl/lock_kernel/enable
...
# cat trace
Xorg-1484 [000] 1973.392586: lock_kernel: ... func=vt_ioctl()
gpm-1402 [001] 1974.027740: lock_kernel: ... func=vt_ioctl()
We should pass to regex.match(..., len) the length (including '\0')
of the source string instead of the length of the pattern string.
Signed-off-by: Li Zefan <[email protected]>
---
kernel/trace/trace_events_filter.c | 7 +++----
1 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 50504cb..c8eb1c0 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -211,8 +211,9 @@ static int filter_pred_pchar(struct filter_pred *pred, void *event,
{
char **addr = (char **)(event + pred->offset);
int cmp, match;
+ int len = strlen(*addr) + 1; /* including tailing '\0' */
- cmp = pred->regex.match(*addr, &pred->regex, pred->regex.field_len);
+ cmp = pred->regex.match(*addr, &pred->regex, len);
match = cmp ^ pred->not;
@@ -781,10 +782,8 @@ static int filter_add_pred(struct filter_parse_state *ps,
pred->regex.field_len = field->size;
} else if (field->filter_type == FILTER_DYN_STRING)
fn = filter_pred_strloc;
- else {
+ else
fn = filter_pred_pchar;
- pred->regex.field_len = strlen(pred->regex.pattern);
- }
} else {
if (field->is_signed)
ret = strict_strtoll(pred->regex.pattern, 0, &val);
--
1.6.3
MATCH_FRONT_ONLY matching works exactly as MATCH_FULL.
We should pass the length of the pattern to strncmp().
Signed-off-by: Li Zefan <[email protected]>
---
kernel/trace/trace_events_filter.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index c8eb1c0..bd492ce 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -262,7 +262,7 @@ static int regex_match_full(char *str, struct regex *r, int len)
static int regex_match_front(char *str, struct regex *r, int len)
{
- if (strncmp(str, r->pattern, len) == 0)
+ if (strncmp(str, r->pattern, r->len) == 0)
return 1;
return 0;
}
--
1.6.3
For '*foo' pattern, we should allow any string ending with
'foo', but tracing filter incorrectly disallows strings
matching regex expression ".*foo.*foo".
Signed-off-by: Li Zefan <[email protected]>
---
kernel/trace/trace_events_filter.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index bd492ce..9f96339 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -276,9 +276,10 @@ static int regex_match_middle(char *str, struct regex *r, int len)
static int regex_match_end(char *str, struct regex *r, int len)
{
- char *ptr = strstr(str, r->pattern);
+ int strlen = len - 1;
- if (ptr && (ptr[r->len] == 0))
+ if (strlen >= r->len &&
+ !memcmp(str + strlen - r->len, r->pattern, r->len))
return 1;
return 0;
}
--
1.6.3
On Fri, Dec 11, 2009 at 05:59:28PM +0800, Li Zefan wrote:
> MATCH_FRONT_ONLY matching works exactly as MATCH_FULL.
>
> We should pass the length of the pattern to strncmp().
>
> Signed-off-by: Li Zefan <[email protected]>
> ---
> kernel/trace/trace_events_filter.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
> index c8eb1c0..bd492ce 100644
> --- a/kernel/trace/trace_events_filter.c
> +++ b/kernel/trace/trace_events_filter.c
> @@ -262,7 +262,7 @@ static int regex_match_full(char *str, struct regex *r, int len)
>
> static int regex_match_front(char *str, struct regex *r, int len)
> {
> - if (strncmp(str, r->pattern, len) == 0)
> + if (strncmp(str, r->pattern, r->len) == 0)
> return 1;
> return 0;
> }
But it was working before the fix to match full.
The MATCH_FULL fix is nice but it also brings a new bug that
is fixed in this patch.
Could you perhaps rework the MATCH_FULL fix to avoid that?
Thanks.
On Fri, Dec 11, 2009 at 05:59:06PM +0800, Li Zefan wrote:
> MATCH_FULL matching for PTR_STRING is not working correctly:
>
> # echo 'func == vt' > events/bkl/lock_kernel/filter
> # echo 1 > events/bkl/lock_kernel/enable
> ...
> # cat trace
> Xorg-1484 [000] 1973.392586: lock_kernel: ... func=vt_ioctl()
> gpm-1402 [001] 1974.027740: lock_kernel: ... func=vt_ioctl()
>
> We should pass to regex.match(..., len) the length (including '\0')
> of the source string instead of the length of the pattern string.
>
> Signed-off-by: Li Zefan <[email protected]>
This patch is cool but it seems to also break middle and end
matching (at least what was working with end matching :)
I know you fix that in the subsequent patches, but please
avoid that. A fix should not bring another known bug,
event if it's fixed in the same batch.
Thanks.
On Fri, Dec 11, 2009 at 05:58:40PM +0800, Li Zefan wrote:
> For '*foo' pattern, we should allow any string ending with
> 'foo', but ftrace filter incorrectly disallows strings
> like bar_foo_foo:
>
> # echo '*io' > set_ftrace_filter
> # cat set_ftrace_filter | grep 'req_bio_endio'
> # cat available_filter_functions | grep 'req_bio_endio'
> req_bio_endio
>
> Signed-off-by: Li Zefan <[email protected]>
I have pending fixes in queue, I'm taking this one too.
Thanks!
2009/12/11 Li Zefan <[email protected]>:
> For '*foo' pattern, we should allow any string ending with
> 'foo', but ftrace filter incorrectly disallows strings
> like bar_foo_foo:
[...]
> ? ? ? ?case MATCH_END_ONLY:
> - ? ? ? ? ? ? ? ptr = strstr(str, regex);
> - ? ? ? ? ? ? ? if (ptr && (ptr[len] == 0))
> + ? ? ? ? ? ? ? slen = strlen(str);
> + ? ? ? ? ? ? ? if (slen >= len && memcpy(str + slen - len, regex, len))
> ? ? ? ? ? ? ? ? ? ? ? ?matched = 1;
Shouldn't that be memcmp() == 0? I don't see how memcpy might serve as
a compare operator.
Fred.
On Fri, Dec 11, 2009 at 01:20:04PM +0100, Frederic Riss wrote:
> 2009/12/11 Li Zefan <[email protected]>:
> > For '*foo' pattern, we should allow any string ending with
> > 'foo', but ftrace filter incorrectly disallows strings
> > like bar_foo_foo:
> [...]
> > ? ? ? ?case MATCH_END_ONLY:
> > - ? ? ? ? ? ? ? ptr = strstr(str, regex);
> > - ? ? ? ? ? ? ? if (ptr && (ptr[len] == 0))
> > + ? ? ? ? ? ? ? slen = strlen(str);
> > + ? ? ? ? ? ? ? if (slen >= len && memcpy(str + slen - len, regex, len))
> > ? ? ? ? ? ? ? ? ? ? ? ?matched = 1;
>
> Shouldn't that be memcmp() == 0? I don't see how memcpy might serve as
> a compare operator.
>
> Fred.
Oh you're right, I reviewed it too quickly. Perhaps the star's
alignment made it possible for only one Frederic to review it
the right way...
On Fri, 2009-12-11 at 13:27 +0100, Frederic Weisbecker wrote:
> On Fri, Dec 11, 2009 at 01:20:04PM +0100, Frederic Riss wrote:
> > 2009/12/11 Li Zefan <[email protected]>:
> > > For '*foo' pattern, we should allow any string ending with
> > > 'foo', but ftrace filter incorrectly disallows strings
> > > like bar_foo_foo:
> > [...]
> > > case MATCH_END_ONLY:
> > > - ptr = strstr(str, regex);
> > > - if (ptr && (ptr[len] == 0))
> > > + slen = strlen(str);
> > > + if (slen >= len && memcpy(str + slen - len, regex, len))
> > > matched = 1;
> >
> > Shouldn't that be memcmp() == 0? I don't see how memcpy might serve as
> > a compare operator.
> >
> > Fred.
>
>
> Oh you're right, I reviewed it too quickly. Perhaps the star's
> alignment made it possible for only one Frederic to review it
> the right way...
>
The stars are still fuzzy to me. How does memcpy work here?
-- Steve
On Fri, 2009-12-11 at 17:59 +0800, Li Zefan wrote:
> For '*foo' pattern, we should allow any string ending with
> 'foo', but tracing filter incorrectly disallows strings
> matching regex expression ".*foo.*foo".
>
> Signed-off-by: Li Zefan <[email protected]>
> ---
> kernel/trace/trace_events_filter.c | 5 +++--
> 1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
> index bd492ce..9f96339 100644
> --- a/kernel/trace/trace_events_filter.c
> +++ b/kernel/trace/trace_events_filter.c
> @@ -276,9 +276,10 @@ static int regex_match_middle(char *str, struct regex *r, int len)
>
> static int regex_match_end(char *str, struct regex *r, int len)
> {
> - char *ptr = strstr(str, r->pattern);
> + int strlen = len - 1;
>
> - if (ptr && (ptr[r->len] == 0))
> + if (strlen >= r->len &&
> + !memcmp(str + strlen - r->len, r->pattern, r->len))
Please use memcmp() == 0, I've seen too many bugs with !*cmp as well as
with *cmp, because humans tend to think instinctively when reading this
that ! is not a match. With "== 0" we think that "==" is a match and
"!=" is a miss.
Thanks,
-- Steve
> return 1;
> return 0;
> }
On Fri, Dec 11, 2009 at 10:10:31AM -0500, Steven Rostedt wrote:
> On Fri, 2009-12-11 at 13:27 +0100, Frederic Weisbecker wrote:
> > On Fri, Dec 11, 2009 at 01:20:04PM +0100, Frederic Riss wrote:
> > > 2009/12/11 Li Zefan <[email protected]>:
> > > > For '*foo' pattern, we should allow any string ending with
> > > > 'foo', but ftrace filter incorrectly disallows strings
> > > > like bar_foo_foo:
> > > [...]
> > > > case MATCH_END_ONLY:
> > > > - ptr = strstr(str, regex);
> > > > - if (ptr && (ptr[len] == 0))
> > > > + slen = strlen(str);
> > > > + if (slen >= len && memcpy(str + slen - len, regex, len))
> > > > matched = 1;
> > >
> > > Shouldn't that be memcmp() == 0? I don't see how memcpy might serve as
> > > a compare operator.
> > >
> > > Fred.
> >
> >
> > Oh you're right, I reviewed it too quickly. Perhaps the star's
> > alignment made it possible for only one Frederic to review it
> > the right way...
> >
>
> The stars are still fuzzy to me. How does memcpy work here?
It doesn't, I guess Li intended to use memcmp.
>>>>> For '*foo' pattern, we should allow any string ending with
>>>>> 'foo', but ftrace filter incorrectly disallows strings
>>>>> like bar_foo_foo:
>>>> [...]
>>>>> case MATCH_END_ONLY:
>>>>> - ptr = strstr(str, regex);
>>>>> - if (ptr && (ptr[len] == 0))
>>>>> + slen = strlen(str);
>>>>> + if (slen >= len && memcpy(str + slen - len, regex, len))
>>>>> matched = 1;
>>>> Shouldn't that be memcmp() == 0? I don't see how memcpy might serve as
>>>> a compare operator.
>>>>
>>>> Fred.
>>>
>>> Oh you're right, I reviewed it too quickly. Perhaps the star's
>>> alignment made it possible for only one Frederic to review it
>>> the right way...
>>>
>> The stars are still fuzzy to me. How does memcpy work here?
>
>
> It doesn't, I guess Li intended to use memcmp.
>
Oops, I forgot to fix this before sending the patch out.. :(
>> static int regex_match_end(char *str, struct regex *r, int len)
>> {
>> - char *ptr = strstr(str, r->pattern);
>> + int strlen = len - 1;
>>
>> - if (ptr && (ptr[r->len] == 0))
>> + if (strlen >= r->len &&
>> + !memcmp(str + strlen - r->len, r->pattern, r->len))
>
> Please use memcmp() == 0, I've seen too many bugs with !*cmp as well as
> with *cmp, because humans tend to think instinctively when reading this
> that ! is not a match. With "== 0" we think that "==" is a match and
> "!=" is a miss.
>
Though I sometimes don't care much about this, I do
think memcmp() == 0 is better.
Frederic Weisbecker wrote:
> On Fri, Dec 11, 2009 at 05:59:06PM +0800, Li Zefan wrote:
>> MATCH_FULL matching for PTR_STRING is not working correctly:
>>
>> # echo 'func == vt' > events/bkl/lock_kernel/filter
>> # echo 1 > events/bkl/lock_kernel/enable
>> ...
>> # cat trace
>> Xorg-1484 [000] 1973.392586: lock_kernel: ... func=vt_ioctl()
>> gpm-1402 [001] 1974.027740: lock_kernel: ... func=vt_ioctl()
>>
>> We should pass to regex.match(..., len) the length (including '\0')
>> of the source string instead of the length of the pattern string.
>>
>> Signed-off-by: Li Zefan <[email protected]>
>
> This patch is cool but it seems to also break middle and end
> matching (at least what was working with end matching :)
>
No, it won't, unless I miss something.
I changed what value ptr_string passes the length param to match(),
but this param is not used in match_middle and match_end.
It does break match_front for ptr_string, but that's because
the mixture of 2 bugs happened to make things right. I can
sort this out by reordering the 2 patches.
> I know you fix that in the subsequent patches, but please
> avoid that. A fix should not bring another known bug,
> event if it's fixed in the same batch.
>
Frederic Weisbecker wrote:
> On Fri, Dec 11, 2009 at 05:59:28PM +0800, Li Zefan wrote:
>> MATCH_FRONT_ONLY matching works exactly as MATCH_FULL.
>>
>> We should pass the length of the pattern to strncmp().
>>
>> Signed-off-by: Li Zefan <[email protected]>
>> ---
>> kernel/trace/trace_events_filter.c | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
>> index c8eb1c0..bd492ce 100644
>> --- a/kernel/trace/trace_events_filter.c
>> +++ b/kernel/trace/trace_events_filter.c
>> @@ -262,7 +262,7 @@ static int regex_match_full(char *str, struct regex *r, int len)
>>
>> static int regex_match_front(char *str, struct regex *r, int len)
>> {
>> - if (strncmp(str, r->pattern, len) == 0)
>> + if (strncmp(str, r->pattern, r->len) == 0)
>> return 1;
>> return 0;
>> }
>
>
> But it was working before the fix to match full.
No, it was not working for dyn_string and static_string.
And as I explained it happened to work for ptr_string by
the mixture of 2 bugs.
> The MATCH_FULL fix is nice but it also brings a new bug that
> is fixed in this patch.
>
On Mon, Dec 14, 2009 at 09:40:56AM +0800, Li Zefan wrote:
> Frederic Weisbecker wrote:
> > On Fri, Dec 11, 2009 at 05:59:06PM +0800, Li Zefan wrote:
> >> MATCH_FULL matching for PTR_STRING is not working correctly:
> >>
> >> # echo 'func == vt' > events/bkl/lock_kernel/filter
> >> # echo 1 > events/bkl/lock_kernel/enable
> >> ...
> >> # cat trace
> >> Xorg-1484 [000] 1973.392586: lock_kernel: ... func=vt_ioctl()
> >> gpm-1402 [001] 1974.027740: lock_kernel: ... func=vt_ioctl()
> >>
> >> We should pass to regex.match(..., len) the length (including '\0')
> >> of the source string instead of the length of the pattern string.
> >>
> >> Signed-off-by: Li Zefan <[email protected]>
> >
> > This patch is cool but it seems to also break middle and end
> > matching (at least what was working with end matching :)
> >
>
> No, it won't, unless I miss something.
>
> I changed what value ptr_string passes the length param to match(),
> but this param is not used in match_middle and match_end.
>
> It does break match_front for ptr_string, but that's because
> the mixture of 2 bugs happened to make things right. I can
> sort this out by reordering the 2 patches.
I made a mistake. I meant it breaks match front. As previously it
was taking the pattern length as a parameter and now it takes
the string length to compare.
But well, the things were so broken already that I'm not sure
we need bisectable deltas... :)
On Mon, Dec 14, 2009 at 09:43:37AM +0800, Li Zefan wrote:
> Frederic Weisbecker wrote:
> > On Fri, Dec 11, 2009 at 05:59:28PM +0800, Li Zefan wrote:
> >> MATCH_FRONT_ONLY matching works exactly as MATCH_FULL.
> >>
> >> We should pass the length of the pattern to strncmp().
> >>
> >> Signed-off-by: Li Zefan <[email protected]>
> >> ---
> >> kernel/trace/trace_events_filter.c | 2 +-
> >> 1 files changed, 1 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
> >> index c8eb1c0..bd492ce 100644
> >> --- a/kernel/trace/trace_events_filter.c
> >> +++ b/kernel/trace/trace_events_filter.c
> >> @@ -262,7 +262,7 @@ static int regex_match_full(char *str, struct regex *r, int len)
> >>
> >> static int regex_match_front(char *str, struct regex *r, int len)
> >> {
> >> - if (strncmp(str, r->pattern, len) == 0)
> >> + if (strncmp(str, r->pattern, r->len) == 0)
> >> return 1;
> >> return 0;
> >> }
> >
> >
> > But it was working before the fix to match full.
>
> No, it was not working for dyn_string and static_string.
> And as I explained it happened to work for ptr_string by
> the mixture of 2 bugs.
Ah, you're right.
On Mon, Dec 14, 2009 at 09:40:56AM +0800, Li Zefan wrote:
> Frederic Weisbecker wrote:
> > On Fri, Dec 11, 2009 at 05:59:06PM +0800, Li Zefan wrote:
> >> MATCH_FULL matching for PTR_STRING is not working correctly:
> >>
> >> # echo 'func == vt' > events/bkl/lock_kernel/filter
> >> # echo 1 > events/bkl/lock_kernel/enable
> >> ...
> >> # cat trace
> >> Xorg-1484 [000] 1973.392586: lock_kernel: ... func=vt_ioctl()
> >> gpm-1402 [001] 1974.027740: lock_kernel: ... func=vt_ioctl()
> >>
> >> We should pass to regex.match(..., len) the length (including '\0')
> >> of the source string instead of the length of the pattern string.
> >>
> >> Signed-off-by: Li Zefan <[email protected]>
> >
> > This patch is cool but it seems to also break middle and end
> > matching (at least what was working with end matching :)
> >
>
> No, it won't, unless I miss something.
>
> I changed what value ptr_string passes the length param to match(),
> but this param is not used in match_middle and match_end.
>
> It does break match_front for ptr_string, but that's because
> the mixture of 2 bugs happened to make things right. I can
> sort this out by reordering the 2 patches.
No, no need to. I was just confused :)