[PATCH 0/7] tracing: Fix bugs in string filters.

[PATCH 0/7] tracing: Fix bugs in string filters.

[Li Zefan]

This patchset fixes some bugs in string filters.

A new string helper strnstr() is introduced, to be used in
middle matching callback.

Also I add some comments to mainly explain why we should use
length-limited version of strstr() and strcmp().

---
 include/linux/string.h             |    5 ++++-
 kernel/trace/ftrace.c              |    6 +++---
 kernel/trace/trace_events_filter.c |   29 ++++++++++++++++++++---------
 lib/string.c                       |   27 ++++++++++++++++++++++++++-
 4 files changed, 53 insertions(+), 14 deletions(-)

[PATCH 1/7] ftrace: Fix MATCH_END_ONLY function filter

[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..3216ed6 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 && memcmp(str + slen - len, regex, len) == 0)
 			matched = 1;
 		break;
 	}
-- 
1.6.3

[PATCH 2/7] tracing/filters: Fix MATCH_FRONT_ONLY filter matching

[Li Zefan]

MATCH_FRONT_ONLY actually is a full matching.

We should pass the length of the pattern string 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 50504cb..11c3973 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -261,7 +261,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

[PATCH 3/7] tracing/filters: Fix MATCH_END_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 11c3973..49e44dd 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -275,9 +275,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) == 0)
 		return 1;
 	return 0;
 }
-- 
1.6.3

[PATCH 4/7] lib: Introduce strnstr()

[Li Zefan]

It differs strstr() in that it limits the length to be searched
in the first string.

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 include/linux/string.h |    5 ++++-
 lib/string.c           |   27 ++++++++++++++++++++++++++-
 2 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/include/linux/string.h b/include/linux/string.h
index b850886..fa6a5ba 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -64,7 +64,10 @@ extern char * strrchr(const char *,int);
 #endif
 extern char * __must_check strstrip(char *);
 #ifndef __HAVE_ARCH_STRSTR
-extern char * strstr(const char *,const char *);
+extern char * strstr(const char *, const char *);
+#endif
+#ifndef __HAVE_ARCH_STRNSTR
+extern char * strnstr(const char *, const char *, size_t);
 #endif
 #ifndef __HAVE_ARCH_STRLEN
 extern __kernel_size_t strlen(const char *);
diff --git a/lib/string.c b/lib/string.c
index e96421a..a5fff51 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -656,7 +656,7 @@ EXPORT_SYMBOL(memscan);
  */
 char *strstr(const char *s1, const char *s2)
 {
-	int l1, l2;
+	size_t l1, l2;
 
 	l2 = strlen(s2);
 	if (!l2)
@@ -673,6 +673,31 @@ char *strstr(const char *s1, const char *s2)
 EXPORT_SYMBOL(strstr);
 #endif
 
+#ifndef __HAVE_ARCH_STRNSTR
+/**
+ * strnstr - Find the first substring in a length-limited string
+ * @s1: The string to be searched
+ * @s2: The string to search for
+ * @len: the maximum number of characters to search
+ */
+char *strnstr(const char *s1, const char *s2, size_t len)
+{
+	size_t l1 = len, l2;
+
+	l2 = strlen(s2);
+	if (!l2)
+		return (char *)s1;
+	while (l1 >= l2) {
+		l1--;
+		if (!memcmp(s1, s2, l2))
+			return (char *)s1;
+		s1++;
+	}
+	return NULL;
+}
+EXPORT_SYMBOL(strnstr);
+#endif
+
 #ifndef __HAVE_ARCH_MEMCHR
 /**
  * memchr - Find a character in an area of memory.
-- 
1.6.3

[PATCH 5/7] tracing/filters: Fix MATCH_MIDDLE_ONLY filter matching

[Li Zefan]

The @str might not be NULL-terminated if it's of type
DYN_STRING or STATIC_STRING, so we should use strnstr()
instead of strstr().

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 49e44dd..f364b08 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -268,7 +268,7 @@ static int regex_match_front(char *str, struct regex *r, int len)
 
 static int regex_match_middle(char *str, struct regex *r, int len)
 {
-	if (strstr(str, r->pattern))
+	if (strnstr(str, r->pattern, len))
 		return 1;
 	return 0;
 }
-- 
1.6.3

[PATCH 6/7] tracing/filters: Fix MATCH_END_ONLY 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 f364b08..60c2a4e 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;
 
@@ -782,10 +783,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

[PATCH 7/7] tracing/filters: Add comment for match callbacks

[Li Zefan]

We should be clear on 2 things:

- the length parameter of a match callback includes
  tailing '\0'.

- the string to be searched might not be NULL-terminated.

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 kernel/trace/trace_events_filter.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 60c2a4e..e42af9a 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -252,7 +252,18 @@ static int filter_pred_none(struct filter_pred *pred, void *event,
 	return 0;
 }
 
-/* Basic regex callbacks */
+/*
+ * regex_match_foo - Basic regex callbacks
+ *
+ * @str: the string to be searched
+ * @r:   the regex structure containing the pattern string
+ * @len: the length of the string to be searched (including '\0')
+ *
+ * Note:
+ * - @str might not be NULL-terminated if it's of type DYN_STRING
+ *   or STATIC_STRING
+ */
+
 static int regex_match_full(char *str, struct regex *r, int len)
 {
 	if (strncmp(str, r->pattern, len) == 0)
-- 
1.6.3

Re: [PATCH 0/7] tracing: Fix bugs in string filters.

[Frederic Weisbecker]

On Mon, Dec 14, 2009 at 11:57:11AM +0800, Li Zefan wrote:
> This patchset fixes some bugs in string filters.
> 
> A new string helper strnstr() is introduced, to be used in
> middle matching callback.
> 
> Also I add some comments to mainly explain why we should use
> length-limited version of strstr() and strcmp().
> 
> ---
>  include/linux/string.h             |    5 ++++-
>  kernel/trace/ftrace.c              |    6 +++---
>  kernel/trace/trace_events_filter.c |   29 ++++++++++++++++++++---------
>  lib/string.c                       |   27 ++++++++++++++++++++++++++-
>  4 files changed, 53 insertions(+), 14 deletions(-)


Looks pretty good.

Acked-by: Frederic Weisbecker <fweisbec@gmail.com>

Thanks!

Re: [PATCH 0/7] tracing: Fix bugs in string filters.

[Li Zefan]

Frederic Weisbecker wrote:
> On Mon, Dec 14, 2009 at 11:57:11AM +0800, Li Zefan wrote:
>> This patchset fixes some bugs in string filters.
>>
>> A new string helper strnstr() is introduced, to be used in
>> middle matching callback.
>>
>> Also I add some comments to mainly explain why we should use
>> length-limited version of strstr() and strcmp().
>>
>> ---
>>  include/linux/string.h             |    5 ++++-
>>  kernel/trace/ftrace.c              |    6 +++---
>>  kernel/trace/trace_events_filter.c |   29 ++++++++++++++++++++---------
>>  lib/string.c                       |   27 ++++++++++++++++++++++++++-
>>  4 files changed, 53 insertions(+), 14 deletions(-)
> 
> 
> Looks pretty good.
> 
> Acked-by: Frederic Weisbecker <fweisbec@gmail.com>
> 

Hi Steven, could you pick up those patches?

Re: [PATCH 4/7] lib: Introduce strnstr()

[Alex Riesen]

On Mon, Jan 18, 2010 at 15:53, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Sat, 2010-01-16 at 12:12 +0100, Alex Riesen wrote:
>>
>> Are you sure you want to search _past_ the NUL-terminator
>> of s1?
>>
>> > +       l2 = strlen(s2);
>> > +       if (!l2)
>> > +               return (char *)s1;
>> > +       while (l1 >= l2) {
>> > +               l1--;
>
> The first check is len-1, I don't see it searching past the
> NUL-terminator. The loop will stop when l1 == l2 (the size of s2) and s1
> pointing near the end of the string.

I thought that len means "minimum of strlen(s1) and len".

Li already explained that was not intended, even if s2 cannot be made to
contain a \0, as s1 can by specifying len greater than strlen(s1).

Re: [PATCH 4/7] lib: Introduce strnstr()

[Steven Rostedt]

On Sat, 2010-01-16 at 12:12 +0100, Alex Riesen wrote:
> On Thu, Jan 14, 2010 at 03:53, Li Zefan <lizf@cn.fujitsu.com> wrote:
> > @@ -667,7 +667,7 @@ EXPORT_SYMBOL(memscan);
> >  */
> >  char *strstr(const char *s1, const char *s2)
> >  {
> > -       int l1, l2;
> > +       size_t l1, l2;
> >
> 
> This chunk is not related, is it?

Actually it is related. Making both strstr and strnstr use the correct
variable and be consistent. This patch introduces strnstr and in doing
so also makes strstr consistent (and correct) with strnstr.

> 
> > @@ -684,6 +684,31 @@ char *strstr(const char *s1, const char *s2)
> >  EXPORT_SYMBOL(strstr);
> >  #endif
> >
> > +#ifndef __HAVE_ARCH_STRNSTR
> > +/**
> > + * strnstr - Find the first substring in a length-limited string
> > + * @s1: The string to be searched
> > + * @s2: The string to search for
> > + * @len: the maximum number of characters to search
> > + */
> > +char *strnstr(const char *s1, const char *s2, size_t len)
> > +{
> > +       size_t l1 = len, l2;
> 
> Are you sure you want to search _past_ the NUL-terminator
> of s1?
> 
> > +       l2 = strlen(s2);
> > +       if (!l2)
> > +               return (char *)s1;
> > +       while (l1 >= l2) {
> > +               l1--;

The first check is len-1, I don't see it searching past the
NUL-terminator. The loop will stop when l1 == l2 (the size of s2) and s1
pointing near the end of the string.

-- Steve

> > +               if (!memcmp(s1, s2, l2))
> > +                       return (char *)s1;
> > +               s1++;
> > +       }
> > +       return NULL;
> > +}

Re: [PATCH 4/7] lib: Introduce strnstr()

[Alex Riesen]

On Thu, Jan 14, 2010 at 03:53, Li Zefan <lizf@cn.fujitsu.com> wrote:
> @@ -667,7 +667,7 @@ EXPORT_SYMBOL(memscan);
>  */
>  char *strstr(const char *s1, const char *s2)
>  {
> -       int l1, l2;
> +       size_t l1, l2;
>

This chunk is not related, is it?

> @@ -684,6 +684,31 @@ char *strstr(const char *s1, const char *s2)
>  EXPORT_SYMBOL(strstr);
>  #endif
>
> +#ifndef __HAVE_ARCH_STRNSTR
> +/**
> + * strnstr - Find the first substring in a length-limited string
> + * @s1: The string to be searched
> + * @s2: The string to search for
> + * @len: the maximum number of characters to search
> + */
> +char *strnstr(const char *s1, const char *s2, size_t len)
> +{
> +       size_t l1 = len, l2;

Are you sure you want to search _past_ the NUL-terminator
of s1?

> +       l2 = strlen(s2);
> +       if (!l2)
> +               return (char *)s1;
> +       while (l1 >= l2) {
> +               l1--;
> +               if (!memcmp(s1, s2, l2))
> +                       return (char *)s1;
> +               s1++;
> +       }
> +       return NULL;
> +}

[PATCH 4/7] lib: Introduce strnstr()

[Li Zefan]

It differs strstr() in that it limits the length to be searched
in the first string.

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
Acked-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 include/linux/string.h |    5 ++++-
 lib/string.c           |   27 ++++++++++++++++++++++++++-
 2 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/include/linux/string.h b/include/linux/string.h
index 651839a..a716ee2 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -72,7 +72,10 @@ static inline __must_check char *strstrip(char *str)
 }
 
 #ifndef __HAVE_ARCH_STRSTR
-extern char * strstr(const char *,const char *);
+extern char * strstr(const char *, const char *);
+#endif
+#ifndef __HAVE_ARCH_STRNSTR
+extern char * strnstr(const char *, const char *, size_t);
 #endif
 #ifndef __HAVE_ARCH_STRLEN
 extern __kernel_size_t strlen(const char *);
diff --git a/lib/string.c b/lib/string.c
index 9f75b4e..a1cdcfc 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -667,7 +667,7 @@ EXPORT_SYMBOL(memscan);
  */
 char *strstr(const char *s1, const char *s2)
 {
-	int l1, l2;
+	size_t l1, l2;
 
 	l2 = strlen(s2);
 	if (!l2)
@@ -684,6 +684,31 @@ char *strstr(const char *s1, const char *s2)
 EXPORT_SYMBOL(strstr);
 #endif
 
+#ifndef __HAVE_ARCH_STRNSTR
+/**
+ * strnstr - Find the first substring in a length-limited string
+ * @s1: The string to be searched
+ * @s2: The string to search for
+ * @len: the maximum number of characters to search
+ */
+char *strnstr(const char *s1, const char *s2, size_t len)
+{
+	size_t l1 = len, l2;
+
+	l2 = strlen(s2);
+	if (!l2)
+		return (char *)s1;
+	while (l1 >= l2) {
+		l1--;
+		if (!memcmp(s1, s2, l2))
+			return (char *)s1;
+		s1++;
+	}
+	return NULL;
+}
+EXPORT_SYMBOL(strnstr);
+#endif
+
 #ifndef __HAVE_ARCH_MEMCHR
 /**
  * memchr - Find a character in an area of memory.
-- 
1.6.3