[PATCH 1/4] ftrace: Fix MATCH_END_ONLY filter matching

[PATCH 1/4] ftrace: Fix MATCH_END_ONLY filter matching

[Li Zefan]

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 <lizf@cn.fujitsu.com>
---
 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

[PATCH 2/4] tracing/filters: Fix MATCH_FULL filter matching for PTR_STRING

[Li Zefan]

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 <lizf@cn.fujitsu.com>
---
 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

Re: [PATCH 2/4] tracing/filters: Fix MATCH_FULL filter matching for PTR_STRING

[Frederic Weisbecker]

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 <lizf@cn.fujitsu.com>


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.

Re: [PATCH 2/4] tracing/filters: Fix MATCH_FULL filter matching for PTR_STRING

[Li Zefan]

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 <lizf@cn.fujitsu.com>
> 
> 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.
> 

Re: [PATCH 2/4] tracing/filters: Fix MATCH_FULL filter matching for PTR_STRING

[Frederic Weisbecker]

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 <lizf@cn.fujitsu.com>
> > 
> > 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... :)

Re: [PATCH 2/4] tracing/filters: Fix MATCH_FULL filter matching for PTR_STRING

[Frederic Weisbecker]

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 <lizf@cn.fujitsu.com>
> > 
> > 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 :)

[PATCH 3/4] tracing/filters: Fix MATCH_FRONT_ONLY filter matching

[Li Zefan]

MATCH_FRONT_ONLY matching works exactly as MATCH_FULL.

We should pass the length of the pattern to strncmp().

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 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

Re: [PATCH 3/4] tracing/filters: Fix MATCH_FRONT_ONLY filter matching

[Frederic Weisbecker]

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 <lizf@cn.fujitsu.com>
> ---
>  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.

Re: [PATCH 3/4] tracing/filters: Fix MATCH_FRONT_ONLY filter matching

[Li Zefan]

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 <lizf@cn.fujitsu.com>
>> ---
>>  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.
> 

Re: [PATCH 3/4] tracing/filters: Fix MATCH_FRONT_ONLY filter matching

[Frederic Weisbecker]

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 <lizf@cn.fujitsu.com>
> >> ---
> >>  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.

[PATCH 4/4] tracing/filters: Fix MATCH_EBD_ONLY filter matching

[Li Zefan]

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 <lizf@cn.fujitsu.com>
---
 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

Re: [PATCH 4/4] tracing/filters: Fix MATCH_EBD_ONLY filter matching

[Steven Rostedt]

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 <lizf@cn.fujitsu.com>
> ---
>  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;
>  }

Re: [PATCH 4/4] tracing/filters: Fix MATCH_EBD_ONLY filter matching

[Li Zefan]

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

Re: [PATCH 1/4] ftrace: Fix MATCH_END_ONLY filter matching

[Frederic Weisbecker]

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 <lizf@cn.fujitsu.com>


I have pending fixes in queue, I'm taking this one too.

Thanks!

Re: [PATCH 1/4] ftrace: Fix MATCH_END_ONLY filter matching

[Frederic Riss]

2009/12/11 Li Zefan <lizf@cn.fujitsu.com>:
> 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.

Re: [PATCH 1/4] ftrace: Fix MATCH_END_ONLY filter matching

[Frederic Weisbecker]

On Fri, Dec 11, 2009 at 01:20:04PM +0100, Frederic Riss wrote:
> 2009/12/11 Li Zefan <lizf@cn.fujitsu.com>:
> > 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...

Re: [PATCH 1/4] ftrace: Fix MATCH_END_ONLY filter matching

[Steven Rostedt]

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 <lizf@cn.fujitsu.com>:
> > > 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

Re: [PATCH 1/4] ftrace: Fix MATCH_END_ONLY filter matching

[Frederic Weisbecker]

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 <lizf@cn.fujitsu.com>:
> > > > 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.

Re: [PATCH 1/4] ftrace: Fix MATCH_END_ONLY filter matching

[Li Zefan]

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