linux-kernel - Re: [PATCH 4/7] tracing/filters: allow user to specify a filter val to be 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: <20090411143533.GA5977@nowhere>
Date:	Sat, 11 Apr 2009 16:35:34 +0200
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	Li Zefan <lizf@...fujitsu.com>
Cc:	Ingo Molnar <mingo@...e.hu>, Tom Zanussi <tzanussi@...il.com>,
	Steven Rostedt <rostedt@...dmis.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 4/7] tracing/filters: allow user to specify a filter
	val to be string

Hi Li,

On Sat, Apr 11, 2009 at 03:53:05PM +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
> 
> With this patch, we allow it by:
>  # echo 'parent_comm == \123' > events/sched/sched_process_fork/filter
>  # cat events/sched/sched_process_fork/filter
>  parent_comm == 123



Well, IMHO, it would be rather better to just echo 'parent_comm == 123'
and let it answer depending of which filter_pred_*() callback we have
for the concerned field.

The culprit is this part in filter_parse():

pred->val = simple_strtoull(val_str, &tmp, 10);
if (tmp == val_str) {
	pred->str_val = kstrdup(val_str, GFP_KERNEL);
	if (!pred->str_val)
		return -ENOMEM;
}

The idea would be to not anymore base the check on simple_strtoull to
guess whether this is a number or a string, making it act subsequently
to this assumption, which is not the good assumption we must base our
parsing yet.

Instead, we could let filter_parse only do the job of extracting the tokens
and then fill the whole pred struct without yet bothering about the type
of the value.

Thereafter we may defer the real value type checking on filter_add_pred()
depending on the type of the concerned field:

if (is_string_field()) {
	add it as a string value;
} else {
	do the check with simple_strtoull
	looks good? Then go to the number size switch....
	...
}

You see?

I think it would be a saner basis of parsing.

Thanks,
Frederic.


> 
> Signed-off-by: Li Zefan <lizf@...fujitsu.com>
> ---
>  kernel/trace/trace_events_filter.c |   11 ++++++++++-
>  1 files changed, 10 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
> index 49b3ef5..2bf4481 100644
> --- a/kernel/trace/trace_events_filter.c
> +++ b/kernel/trace/trace_events_filter.c
> @@ -351,12 +351,19 @@ oom:
>  	return -ENOMEM;
>  }
>  
> +/*
> + * The filter format can be
> + *   - 0, which means remove all filter preds
> + *   - [||/&&] <field> ==/!= [\]<val>
> + *
> + * Note: '\' prevent a string type value beginning with a digit to
> + *       be treated as a number
> + */
>  int filter_parse(char **pbuf, struct filter_pred *pred)
>  {
>  	char *tmp, *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")) {
> @@ -421,6 +428,8 @@ int filter_parse(char **pbuf, struct filter_pred *pred)
>  
>  	pred->val = simple_strtoull(val_str, &tmp, 0);
>  	if (tmp == val_str) {
> +		if (*val_str == '\\')
> +			val_str++;
>  		pred->str_val = kstrdup(val_str, GFP_KERNEL);
>  		if (!pred->str_val)
>  			return -ENOMEM;
> -- 
> 1.5.4.rc3
> 

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