[PATCH 1/3] tracing/filters: allow user-input to be integer-like string

[PATCH 1/3] tracing/filters: allow user-input to be integer-like string

[Li Zefan]

Suppose we would like to trace all tasks named '123', but this
will fail:
 # echo 'parent_comm == 123' > events/sched/sched_process_fork/filter
 bash: echo: write error: Invalid argument

Don't guess the type of the filter pred in filter_parse(), but instead
we check it in filter_add_pred().

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

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 9f8ecca..a63f965 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -229,6 +229,7 @@ static int is_string_field(const char *type)
 int filter_add_pred(struct ftrace_event_call *call, struct filter_pred *pred)
 {
 	struct ftrace_event_field *field;
+	unsigned long long val;
 
 	field = find_event_field(call, pred->field_name);
 	if (!field)
@@ -237,14 +238,14 @@ int filter_add_pred(struct ftrace_event_call *call, struct filter_pred *pred)
 	pred->offset = field->offset;
 
 	if (is_string_field(field->type)) {
-		if (!pred->str_val)
-			return -EINVAL;
 		pred->fn = filter_pred_string;
 		pred->str_len = field->size;
 		return __filter_add_pred(call, pred);
 	} else {
-		if (pred->str_val)
+		if (strict_strtoull(pred->str_val, 0, &val))
 			return -EINVAL;
+		pred->val = val;
+		kfree(pred->str_val);
 	}
 
 	switch (field->size) {
@@ -351,12 +352,16 @@ oom:
 	return -ENOMEM;
 }
 
+/*
+ * The filter format can be
+ *   - 0, which means remove all filter preds
+ *   - [||/&&] <field> ==/!= <val>
+ */
 int filter_parse(char **pbuf, struct filter_pred *pred)
 {
-	char *tmp, *tok, *val_str = NULL;
+	char *tok, *val_str = NULL;
 	int tok_n = 0;
 
-	/* field ==/!= number, or/and field ==/!= number, number */
 	while ((tok = strsep(pbuf, " \n"))) {
 		if (tok_n == 0) {
 			if (!strcmp(tok, "0")) {
@@ -419,15 +424,10 @@ int filter_parse(char **pbuf, struct filter_pred *pred)
 	if (!pred->field_name)
 		return -ENOMEM;
 
-	pred->val = simple_strtoull(val_str, &tmp, 0);
-	if (tmp == val_str) {
-		pred->str_val = kstrdup(val_str, GFP_KERNEL);
-		if (!pred->str_val)
-			return -ENOMEM;
-	} else if (*tmp != '\0')
-		return -EINVAL;
+	pred->str_val = kstrdup(val_str, GFP_KERNEL);
+	if (!pred->str_val)
+		return -ENOMEM;
 
 	return 0;
 }
 
-
-- 
1.5.4.rc3

[PATCH 2/3] tracing/filters: disallow newline as delimeter

[Li Zefan]

I guess because user input is often ended with '\n' (like "echo xxx"),
thus '\n' is used as a delimeter besides ' ' ? But we can just strip
tailing spaces.

One of the effects of this patch, is fixing this inconsistency:

 # echo -n 'parent_comm ==' > filter
 bash: echo: write error: Invalid argument

 # echo 'parent_comm ==' > filter
 # cat filter
 parent_comm ==

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

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index a63f965..9ef35c1 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -362,7 +362,9 @@ int filter_parse(char **pbuf, struct filter_pred *pred)
 	char *tok, *val_str = NULL;
 	int tok_n = 0;
 
-	while ((tok = strsep(pbuf, " \n"))) {
+	strstrip(*pbuf);
+
+	while ((tok = strsep(pbuf, " "))) {
 		if (tok_n == 0) {
 			if (!strcmp(tok, "0")) {
 				pred->clear = 1;
-- 
1.5.4.rc3

[PATCH 3/3] tracing/filters: don't remove old filters when failed to write subsys->filter

[Li Zefan]

If writing subsys->filter returns EINVAL or ENOSPC, the original
filters in subsys/ and subsys/events/ will be removed. This is
definitely wrong.

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---

This bug is not revealed in mainline, since subsys/filter is disabled,
and it's re-enabled in -tip by a002587bf18c3b316bcf48542fccbc4ae2fc972e:

"tracing/filters: add run-time field descriptions to TRACE_EVENT_FORMAT event"

---
 kernel/trace/trace_events.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 789e14e..4511cdb 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -591,7 +591,6 @@ subsystem_filter_write(struct file *filp, const char __user *ubuf, size_t cnt,
 
 	err = filter_add_subsystem_pred(system, pred);
 	if (err < 0) {
-		filter_free_subsystem_preds(system);
 		filter_free_pred(pred);
 		return err;
 	}
-- 
1.5.4.rc3

Re: [PATCH 1/3] tracing/filters: allow user-input to be integer-like string

[Frederic Weisbecker]

On Mon, Apr 13, 2009 at 10:38:12AM +0800, Li Zefan wrote:
> Suppose we would like to trace all tasks named '123', but this
> will fail:
>  # echo 'parent_comm == 123' > events/sched/sched_process_fork/filter
>  bash: echo: write error: Invalid argument
> 
> Don't guess the type of the filter pred in filter_parse(), but instead
> we check it in filter_add_pred().
> 
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
>  kernel/trace/trace_events_filter.c |   26 +++++++++++++-------------
>  1 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
> index 9f8ecca..a63f965 100644
> --- a/kernel/trace/trace_events_filter.c
> +++ b/kernel/trace/trace_events_filter.c
> @@ -229,6 +229,7 @@ static int is_string_field(const char *type)
>  int filter_add_pred(struct ftrace_event_call *call, struct filter_pred *pred)
>  {
>  	struct ftrace_event_field *field;
> +	unsigned long long val;
>  
>  	field = find_event_field(call, pred->field_name);
>  	if (!field)
> @@ -237,14 +238,14 @@ int filter_add_pred(struct ftrace_event_call *call, struct filter_pred *pred)
>  	pred->offset = field->offset;
>  
>  	if (is_string_field(field->type)) {
> -		if (!pred->str_val)
> -			return -EINVAL;
>  		pred->fn = filter_pred_string;
>  		pred->str_len = field->size;
>  		return __filter_add_pred(call, pred);
>  	} else {
> -		if (pred->str_val)
> +		if (strict_strtoull(pred->str_val, 0, &val))
>  			return -EINVAL;
> +		pred->val = val;
> +		kfree(pred->str_val);


And you might also want to do
pred->str_val = NULL;
Otherwise filter_free_pred() may crash later:

void filter_free_pred(struct filter_pred *pred)
{
	if (!pred)
		return;

	kfree(pred->field_name);
	kfree(pred->str_val);
	kfree(pred);
}


Other than that, it looks good.

Thanks,
Frederic.



>  	}
>  
>  	switch (field->size) {
> @@ -351,12 +352,16 @@ oom:
>  	return -ENOMEM;
>  }
>  
> +/*
> + * The filter format can be
> + *   - 0, which means remove all filter preds
> + *   - [||/&&] <field> ==/!= <val>
> + */
>  int filter_parse(char **pbuf, struct filter_pred *pred)
>  {
> -	char *tmp, *tok, *val_str = NULL;
> +	char *tok, *val_str = NULL;
>  	int tok_n = 0;
>  
> -	/* field ==/!= number, or/and field ==/!= number, number */
>  	while ((tok = strsep(pbuf, " \n"))) {
>  		if (tok_n == 0) {
>  			if (!strcmp(tok, "0")) {
> @@ -419,15 +424,10 @@ int filter_parse(char **pbuf, struct filter_pred *pred)
>  	if (!pred->field_name)
>  		return -ENOMEM;
>  
> -	pred->val = simple_strtoull(val_str, &tmp, 0);
> -	if (tmp == val_str) {
> -		pred->str_val = kstrdup(val_str, GFP_KERNEL);
> -		if (!pred->str_val)
> -			return -ENOMEM;
> -	} else if (*tmp != '\0')
> -		return -EINVAL;
> +	pred->str_val = kstrdup(val_str, GFP_KERNEL);
> +	if (!pred->str_val)
> +		return -ENOMEM;
>  
>  	return 0;
>  }
>  
> -
> -- 
> 1.5.4.rc3
> 

Re: [PATCH 1/3] tracing/filters: allow user-input to be integer-like string

[Ingo Molnar]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> On Mon, Apr 13, 2009 at 10:38:12AM +0800, Li Zefan wrote:
> > Suppose we would like to trace all tasks named '123', but this
> > will fail:
> >  # echo 'parent_comm == 123' > events/sched/sched_process_fork/filter
> >  bash: echo: write error: Invalid argument
> > 
> > Don't guess the type of the filter pred in filter_parse(), but instead
> > we check it in filter_add_pred().
> > 
> > Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> > ---
> >  kernel/trace/trace_events_filter.c |   26 +++++++++++++-------------
> >  1 files changed, 13 insertions(+), 13 deletions(-)
> > 
> > diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
> > index 9f8ecca..a63f965 100644
> > --- a/kernel/trace/trace_events_filter.c
> > +++ b/kernel/trace/trace_events_filter.c
> > @@ -229,6 +229,7 @@ static int is_string_field(const char *type)
> >  int filter_add_pred(struct ftrace_event_call *call, struct filter_pred *pred)
> >  {
> >  	struct ftrace_event_field *field;
> > +	unsigned long long val;
> >  
> >  	field = find_event_field(call, pred->field_name);
> >  	if (!field)
> > @@ -237,14 +238,14 @@ int filter_add_pred(struct ftrace_event_call *call, struct filter_pred *pred)
> >  	pred->offset = field->offset;
> >  
> >  	if (is_string_field(field->type)) {
> > -		if (!pred->str_val)
> > -			return -EINVAL;
> >  		pred->fn = filter_pred_string;
> >  		pred->str_len = field->size;
> >  		return __filter_add_pred(call, pred);
> >  	} else {
> > -		if (pred->str_val)
> > +		if (strict_strtoull(pred->str_val, 0, &val))
> >  			return -EINVAL;
> > +		pred->val = val;
> > +		kfree(pred->str_val);
> 
> 
> And you might also want to do
> pred->str_val = NULL;
> Otherwise filter_free_pred() may crash later:
> 
> void filter_free_pred(struct filter_pred *pred)
> {
> 	if (!pred)
> 		return;
> 
> 	kfree(pred->field_name);
> 	kfree(pred->str_val);
> 	kfree(pred);
> }
> 
> 
> Other than that, it looks good.

This also conflicts with Tom's rewrite of the filter engine to make 
it on-the-fly modifiable.

Li, mind reworking your series against latest -tip? (it has Tom's 
patch included already)

Also, it would be nice to hear Tom's opinion about this series as 
well. I know there's new parsing features planned - if it's better 
that way then Tom you might want to pick up Li's patches and submit 
them together with any pending or upcoming changes? We can apply 
them directly too if that's easier to you.

Thanks,

	Ingo

Re: [PATCH 1/3] tracing/filters: allow user-input to be integer-like string

[Tom Zanussi]

On Tue, 2009-04-14 at 00:29 +0200, Ingo Molnar wrote:
> * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> 
> > On Mon, Apr 13, 2009 at 10:38:12AM +0800, Li Zefan wrote:
> > > Suppose we would like to trace all tasks named '123', but this
> > > will fail:
> > >  # echo 'parent_comm == 123' > events/sched/sched_process_fork/filter
> > >  bash: echo: write error: Invalid argument
> > > 
> > > Don't guess the type of the filter pred in filter_parse(), but instead
> > > we check it in filter_add_pred().
> > > 
> > > Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> > > ---
> > >  kernel/trace/trace_events_filter.c |   26 +++++++++++++-------------
> > >  1 files changed, 13 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
> > > index 9f8ecca..a63f965 100644
> > > --- a/kernel/trace/trace_events_filter.c
> > > +++ b/kernel/trace/trace_events_filter.c
> > > @@ -229,6 +229,7 @@ static int is_string_field(const char *type)
> > >  int filter_add_pred(struct ftrace_event_call *call, struct filter_pred *pred)
> > >  {
> > >  	struct ftrace_event_field *field;
> > > +	unsigned long long val;
> > >  
> > >  	field = find_event_field(call, pred->field_name);
> > >  	if (!field)
> > > @@ -237,14 +238,14 @@ int filter_add_pred(struct ftrace_event_call *call, struct filter_pred *pred)
> > >  	pred->offset = field->offset;
> > >  
> > >  	if (is_string_field(field->type)) {
> > > -		if (!pred->str_val)
> > > -			return -EINVAL;
> > >  		pred->fn = filter_pred_string;
> > >  		pred->str_len = field->size;
> > >  		return __filter_add_pred(call, pred);
> > >  	} else {
> > > -		if (pred->str_val)
> > > +		if (strict_strtoull(pred->str_val, 0, &val))
> > >  			return -EINVAL;
> > > +		pred->val = val;
> > > +		kfree(pred->str_val);
> > 
> > 
> > And you might also want to do
> > pred->str_val = NULL;
> > Otherwise filter_free_pred() may crash later:
> > 
> > void filter_free_pred(struct filter_pred *pred)
> > {
> > 	if (!pred)
> > 		return;
> > 
> > 	kfree(pred->field_name);
> > 	kfree(pred->str_val);
> > 	kfree(pred);
> > }
> > 
> > 
> > Other than that, it looks good.
> 
> This also conflicts with Tom's rewrite of the filter engine to make 
> it on-the-fly modifiable.
> 
> Li, mind reworking your series against latest -tip? (it has Tom's 
> patch included already)
> 
> Also, it would be nice to hear Tom's opinion about this series as 
> well. I know there's new parsing features planned - if it's better 
> that way then Tom you might want to pick up Li's patches and submit 
> them together with any pending or upcoming changes? We can apply 
> them directly too if that's easier to you.
> 

I had started on the new parser a couple weeks ago, but got sidetracked
by other things.  I'll be getting back to it this week, but until it's
ready it wouldn't hurt for you to pick up Li's patches, and I'll make
any changes I need to in the new code.

What this patch does will be needed by the new code too, so it would be
good to have it, though it might get moved around.  Patch 2 won't hurt
either, though everything in filter_parse() will probably change.  And
patch 3 fixes a bug that needs to be fixed regardless...

Thanks,

Tom

> Thanks,
> 
> 	Ingo

Re: [PATCH 1/3] tracing/filters: allow user-input to be integer-like string

[Li Zefan]

>> This also conflicts with Tom's rewrite of the filter engine to make 
>> it on-the-fly modifiable.
>>
>> Li, mind reworking your series against latest -tip? (it has Tom's 
>> patch included already)
>>
>> Also, it would be nice to hear Tom's opinion about this series as 
>> well. I know there's new parsing features planned - if it's better 
>> that way then Tom you might want to pick up Li's patches and submit 
>> them together with any pending or upcoming changes? We can apply 
>> them directly too if that's easier to you.
>>
> 
> I had started on the new parser a couple weeks ago, but got sidetracked
> by other things.  I'll be getting back to it this week, but until it's
> ready it wouldn't hurt for you to pick up Li's patches, and I'll make
> any changes I need to in the new code.
> 

Ok, I'll rebase those patches and send to Ingo.

> What this patch does will be needed by the new code too, so it would be
> good to have it, though it might get moved around.  Patch 2 won't hurt
> either, though everything in filter_parse() will probably change.  And
> patch 3 fixes a bug that needs to be fixed regardless...
> 

Re: [PATCH 1/3] tracing/filters: allow user-input to be integer-like string

[Ingo Molnar]

* Li Zefan <lizf@cn.fujitsu.com> wrote:

> >> This also conflicts with Tom's rewrite of the filter engine to make 
> >> it on-the-fly modifiable.
> >>
> >> Li, mind reworking your series against latest -tip? (it has Tom's 
> >> patch included already)
> >>
> >> Also, it would be nice to hear Tom's opinion about this series as 
> >> well. I know there's new parsing features planned - if it's better 
> >> that way then Tom you might want to pick up Li's patches and submit 
> >> them together with any pending or upcoming changes? We can apply 
> >> them directly too if that's easier to you.
> >>
> > 
> > I had started on the new parser a couple weeks ago, but got sidetracked
> > by other things.  I'll be getting back to it this week, but until it's
> > ready it wouldn't hurt for you to pick up Li's patches, and I'll make
> > any changes I need to in the new code.
> > 
> 
> Ok, I'll rebase those patches and send to Ingo.

Thanks!

	Ingo