| 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
| ||
|
Message-Id: <1248154585.6323.26.camel@tropicana>
Date: Tue, 21 Jul 2009 00:36:25 -0500
From: Tom Zanussi <tzanussi@...il.com>
To: Li Zefan <lizf@...fujitsu.com>
Cc: Frederic Weisbecker <fweisbec@...il.com>,
Ingo Molnar <mingo@...e.hu>,
Steven Rostedt <rostedt@...dmis.org>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] tracing/filters: Improve subsystem filter
Hi,
On Mon, 2009-07-20 at 10:20 +0800, Li Zefan wrote:
> Currently a subsystem filter should be applicable to all events
> under the subsystem, and if it failed, all the event filters
> will be cleared. Those behaviors make subsys filter much less
> useful:
>
> # echo 'vec == 1' > irq/softirq_entry/filter
> # echo 'irq == 5' > irq/filter
> bash: echo: write error: Invalid argument
> # cat irq/softirq_entry/filter
> none
>
> I'd expect it set the filter for irq_handler_entry/exit, and
> not touch softirq_entry/exit.
>
> The basic idea is, try to see if the filter can be applied
> to which events, and then just apply to the those events:
>
> # echo 'vec == 1' > softirq_entry/filter
> # echo 'irq == 5' > filter
> # cat irq_handler_entry/filter
> irq == 5
> # cat softirq_entry/filter
> vec == 1
>
It looks like it accomplishes the goal, but I wonder if you could
simplify it by getting rid of the no_reset flag and all the state-saving
while keeping the dry_run param on filter_add_pred to use from
filter_add_subsysystem_pred() e.g. something like this:
filter_add_subsystem_pred(...)
{
...
list_for_each_entry(call, &ftrace_events, list) {
err = filter_add_pred(ps, call, pred, dry_run = 1);
if (!err)
filter_add_pred(ps, call, pred, dry_run = 0);
...
}
}
IIRC the state-saving was necessary if the idea was to back out of (or
avoid setting) all the filters if one failed, but the new model is to
set whichever apply while leaving the rest alone. I think the simpler
approach above accomplishes that, but I may be missing something or have
forgotten the original motivation for doing it this way.
Tom
> Changelog for v2:
> - do some cleanups to address Frederic's comments.
>
> Inspied-by: Steven Rostedt <srostedt@...hat.com>
> Signed-off-by: Li Zefan <lizf@...fujitsu.com>
> ---
> include/linux/ftrace_event.h | 4 +-
> kernel/trace/trace.h | 3 +-
> kernel/trace/trace_events_filter.c | 124 ++++++++++++++++++++++++------------
> 3 files changed, 87 insertions(+), 44 deletions(-)
>
> diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
> index 5c093ff..26d3673 100644
> --- a/include/linux/ftrace_event.h
> +++ b/include/linux/ftrace_event.h
> @@ -101,6 +101,8 @@ void trace_current_buffer_discard_commit(struct ring_buffer_event *event);
>
> void tracing_record_cmdline(struct task_struct *tsk);
>
> +struct event_filter;
> +
> struct ftrace_event_call {
> struct list_head list;
> char *name;
> @@ -116,7 +118,7 @@ struct ftrace_event_call {
> int (*define_fields)(void);
> struct list_head fields;
> int filter_active;
> - void *filter;
> + struct event_filter *filter;
> void *mod;
>
> #ifdef CONFIG_EVENT_PROFILE
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 06886a0..3a87d46 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -768,13 +768,14 @@ struct event_filter {
> int n_preds;
> struct filter_pred **preds;
> char *filter_string;
> + bool no_reset;
> };
>
> struct event_subsystem {
> struct list_head list;
> const char *name;
> struct dentry *entry;
> - void *filter;
> + struct event_filter *filter;
> int nr_events;
> };
>
> diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
> index b9aae72..c7ed427 100644
> --- a/kernel/trace/trace_events_filter.c
> +++ b/kernel/trace/trace_events_filter.c
> @@ -418,7 +418,14 @@ oom:
> }
> EXPORT_SYMBOL_GPL(init_preds);
>
> -static void filter_free_subsystem_preds(struct event_subsystem *system)
> +enum {
> + FILTER_DISABLE_ALL,
> + FILTER_INIT_NO_RESET,
> + FILTER_SKIP_NO_RESET,
> +};
> +
> +static void filter_free_subsystem_preds(struct event_subsystem *system,
> + int flag)
> {
> struct ftrace_event_call *call;
>
> @@ -426,6 +433,14 @@ static void filter_free_subsystem_preds(struct event_subsystem *system)
> if (!call->define_fields)
> continue;
>
> + if (flag == FILTER_INIT_NO_RESET) {
> + call->filter->no_reset = false;
> + continue;
> + }
> +
> + if (flag == FILTER_SKIP_NO_RESET && call->filter->no_reset)
> + continue;
> +
> if (!strcmp(call->system, system->name)) {
> filter_disable_preds(call);
> remove_filter_string(call->filter);
> @@ -527,7 +542,8 @@ static filter_pred_fn_t select_comparison_fn(int op, int field_size,
>
> static int filter_add_pred(struct filter_parse_state *ps,
> struct ftrace_event_call *call,
> - struct filter_pred *pred)
> + struct filter_pred *pred,
> + bool dry_run)
> {
> struct ftrace_event_field *field;
> filter_pred_fn_t fn;
> @@ -539,10 +555,12 @@ static int filter_add_pred(struct filter_parse_state *ps,
>
> if (pred->op == OP_AND) {
> pred->pop_n = 2;
> - return filter_add_pred_fn(ps, call, pred, filter_pred_and);
> + fn = filter_pred_and;
> + goto add_pred_fn;
> } else if (pred->op == OP_OR) {
> pred->pop_n = 2;
> - return filter_add_pred_fn(ps, call, pred, filter_pred_or);
> + fn = filter_pred_or;
> + goto add_pred_fn;
> }
>
> field = find_event_field(call, pred->field_name);
> @@ -565,9 +583,6 @@ static int filter_add_pred(struct filter_parse_state *ps,
> else
> fn = filter_pred_strloc;
> pred->str_len = field->size;
> - if (pred->op == OP_NE)
> - pred->not = 1;
> - return filter_add_pred_fn(ps, call, pred, fn);
> } else {
> if (field->is_signed)
> ret = strict_strtoll(pred->str_val, 0, &val);
> @@ -578,27 +593,33 @@ static int filter_add_pred(struct filter_parse_state *ps,
> return -EINVAL;
> }
> pred->val = val;
> - }
>
> - fn = select_comparison_fn(pred->op, field->size, field->is_signed);
> - if (!fn) {
> - parse_error(ps, FILT_ERR_INVALID_OP, 0);
> - return -EINVAL;
> + fn = select_comparison_fn(pred->op, field->size,
> + field->is_signed);
> + if (!fn) {
> + parse_error(ps, FILT_ERR_INVALID_OP, 0);
> + return -EINVAL;
> + }
> }
>
> if (pred->op == OP_NE)
> pred->not = 1;
>
> - return filter_add_pred_fn(ps, call, pred, fn);
> +add_pred_fn:
> + if (!dry_run)
> + return filter_add_pred_fn(ps, call, pred, fn);
> + return 0;
> }
>
> static int filter_add_subsystem_pred(struct filter_parse_state *ps,
> struct event_subsystem *system,
> struct filter_pred *pred,
> - char *filter_string)
> + char *filter_string,
> + bool dry_run)
> {
> struct ftrace_event_call *call;
> int err = 0;
> + bool fail = true;
>
> list_for_each_entry(call, &ftrace_events, list) {
>
> @@ -608,16 +629,24 @@ static int filter_add_subsystem_pred(struct filter_parse_state *ps,
> if (strcmp(call->system, system->name))
> continue;
>
> - err = filter_add_pred(ps, call, pred);
> - if (err) {
> - filter_free_subsystem_preds(system);
> - parse_error(ps, FILT_ERR_BAD_SUBSYS_FILTER, 0);
> - goto out;
> - }
> - replace_filter_string(call->filter, filter_string);
> + if (call->filter->no_reset)
> + continue;
> +
> + err = filter_add_pred(ps, call, pred, dry_run);
> + if (err)
> + call->filter->no_reset = true;
> + else
> + fail = false;
> +
> + if (!dry_run)
> + replace_filter_string(call->filter, filter_string);
> }
> -out:
> - return err;
> +
> + if (fail) {
> + parse_error(ps, FILT_ERR_BAD_SUBSYS_FILTER, 0);
> + return err;
> + }
> + return 0;
> }
>
> static void parse_init(struct filter_parse_state *ps,
> @@ -976,12 +1005,14 @@ static int check_preds(struct filter_parse_state *ps)
> static int replace_preds(struct event_subsystem *system,
> struct ftrace_event_call *call,
> struct filter_parse_state *ps,
> - char *filter_string)
> + char *filter_string,
> + bool dry_run)
> {
> char *operand1 = NULL, *operand2 = NULL;
> struct filter_pred *pred;
> struct postfix_elt *elt;
> int err;
> + int n_preds = 0;
>
> err = check_preds(ps);
> if (err)
> @@ -1000,19 +1031,14 @@ static int replace_preds(struct event_subsystem *system,
> continue;
> }
>
> + if (n_preds++ == MAX_FILTER_PRED) {
> + parse_error(ps, FILT_ERR_TOO_MANY_PREDS, 0);
> + return -ENOSPC;
> + }
> +
> if (elt->op == OP_AND || elt->op == OP_OR) {
> pred = create_logical_pred(elt->op);
> - if (call)
> - err = filter_add_pred(ps, call, pred);
> - else
> - err = filter_add_subsystem_pred(ps, system,
> - pred, filter_string);
> - filter_free_pred(pred);
> - if (err)
> - return err;
> -
> - operand1 = operand2 = NULL;
> - continue;
> + goto add_pred;
> }
>
> if (!operand1 || !operand2) {
> @@ -1021,11 +1047,12 @@ static int replace_preds(struct event_subsystem *system,
> }
>
> pred = create_pred(elt->op, operand1, operand2);
> +add_pred:
> if (call)
> - err = filter_add_pred(ps, call, pred);
> + err = filter_add_pred(ps, call, pred, false);
> else
> err = filter_add_subsystem_pred(ps, system, pred,
> - filter_string);
> + filter_string, dry_run);
> filter_free_pred(pred);
> if (err)
> return err;
> @@ -1066,7 +1093,7 @@ int apply_event_filter(struct ftrace_event_call *call, char *filter_string)
> goto out;
> }
>
> - err = replace_preds(NULL, call, ps, filter_string);
> + err = replace_preds(NULL, call, ps, filter_string, false);
> if (err)
> append_filter_err(ps, call->filter);
>
> @@ -1090,7 +1117,7 @@ int apply_subsystem_event_filter(struct event_subsystem *system,
> mutex_lock(&event_mutex);
>
> if (!strcmp(strstrip(filter_string), "0")) {
> - filter_free_subsystem_preds(system);
> + filter_free_subsystem_preds(system, FILTER_DISABLE_ALL);
> remove_filter_string(system->filter);
> mutex_unlock(&event_mutex);
> return 0;
> @@ -1101,7 +1128,6 @@ int apply_subsystem_event_filter(struct event_subsystem *system,
> if (!ps)
> goto out_unlock;
>
> - filter_free_subsystem_preds(system);
> replace_filter_string(system->filter, filter_string);
>
> parse_init(ps, filter_ops, filter_string);
> @@ -1111,9 +1137,23 @@ int apply_subsystem_event_filter(struct event_subsystem *system,
> goto out;
> }
>
> - err = replace_preds(system, NULL, ps, filter_string);
> - if (err)
> + filter_free_subsystem_preds(system, FILTER_INIT_NO_RESET);
> +
> + /* try to see the filter can be applied to which events */
> + err = replace_preds(system, NULL, ps, filter_string, true);
> + if (err) {
> + append_filter_err(ps, system->filter);
> + goto out;
> + }
> +
> + filter_free_subsystem_preds(system, FILTER_SKIP_NO_RESET);
> +
> + /* really apply the filter to the events */
> + err = replace_preds(system, NULL, ps, filter_string, false);
> + if (err) {
> append_filter_err(ps, system->filter);
> + filter_free_subsystem_preds(system, 2);
> + }
>
> out:
> filter_opstack_clear(ps);
--
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/
Powered by blists - more mailing lists