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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20090413222951.GG8514@elte.hu>
Date:	Tue, 14 Apr 2009 00:29:51 +0200
From:	Ingo Molnar <mingo@...e.hu>
To:	Frederic Weisbecker <fweisbec@...il.com>
Cc:	Li Zefan <lizf@...fujitsu.com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Tom Zanussi <tzanussi@...il.com>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/3] tracing/filters: allow user-input to be
	integer-like string


* Frederic Weisbecker <fweisbec@...il.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@...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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/