This patchset fixes some bugs in tracing/filters. Most of the change
goes to the last patch, and others are small ones.
Maybe 3rd to 6th patches can be regarded as small enhancements instead
of bug fixes? But they are behavioural changes.
[PATCH 1/7] tracing/filters: NUL-terminate user input filter
[PATCH 2/7] tracing/filters: fix NULL pointer dereference
[PATCH 3/7] tracing/filters: allow user input integer to be oct or hex
[PATCH 4/7] tracing/filters: allow user to specify a filter val to be string
[PATCH 5/7] tracing/filters: disallow newline as delimeter
[PATCH 6/7] tracing/filters: return proper error code when writing filter file
[PATCH 7/7] tracing/filters: make filter preds RCU safe
kernel/trace/trace.h | 6 +-
kernel/trace/trace_events.c | 16 +++--
kernel/trace/trace_events_filter.c | 131 ++++++++++++++++++++++++++---------
kernel/trace/trace_events_stage_3.h | 10 +++-
4 files changed, 121 insertions(+), 42 deletions(-)
---
Make sure messages from user space are NUL-terminated strings,
otherwise we could dump random memory while reading filter file.
Try this:
# echo 'parent_comm ==' > events/sched/sched_process_fork/filter
# cat events/sched/sched_process_fork/filter
parent_comm == �
Signed-off-by: Li Zefan <[email protected]>
---
kernel/trace/trace_events.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 64ec4d2..054bc18 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -503,6 +503,7 @@ event_filter_write(struct file *filp, const char __user *ubuf, size_t cnt,
if (copy_from_user(&buf, ubuf, cnt))
return -EFAULT;
+ buf[cnt] = '\0';
pred = kzalloc(sizeof(*pred), GFP_KERNEL);
if (!pred)
@@ -569,6 +570,7 @@ subsystem_filter_write(struct file *filp, const char __user *ubuf, size_t cnt,
if (copy_from_user(&buf, ubuf, cnt))
return -EFAULT;
+ buf[cnt] = '\0';
pred = kzalloc(sizeof(*pred), GFP_KERNEL);
if (!pred)
--
1.5.4.rc3
Try this, and you'll see NULL pointer dereference bug:
# echo -n 'parent_comm ==' > sched/sched_process_fork/filter
Because we passed NULL ptr to simple_strtoull().
Signed-off-by: Li Zefan <[email protected]>
---
kernel/trace/trace_events_filter.c | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 026be41..9d2162f 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -410,6 +410,11 @@ int filter_parse(char **pbuf, struct filter_pred *pred)
}
}
+ if (!val_str) {
+ pred->field_name = NULL;
+ return -EINVAL;
+ }
+
pred->field_name = kstrdup(pred->field_name, GFP_KERNEL);
if (!pred->field_name)
return -ENOMEM;
--
1.5.4.rc3
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
Signed-off-by: Li Zefan <[email protected]>
---
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
Before patching:
# echo 'parent_pid == 0x10' > events/sched/sched_process_fork/filter
# cat sched/sched_process_fork/filter
parent_pid == 0
After patching:
# cat sched/sched_process_fork/filter
parent_pid == 16
Also check the input more strictly.
Signed-off-by: Li Zefan <[email protected]>
---
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 9d2162f..49b3ef5 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -419,12 +419,13 @@ int filter_parse(char **pbuf, struct filter_pred *pred)
if (!pred->field_name)
return -ENOMEM;
- pred->val = simple_strtoull(val_str, &tmp, 10);
+ 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;
return 0;
}
--
1.5.4.rc3
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 <[email protected]>
---
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 2bf4481..8e3a095 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -364,7 +364,9 @@ int filter_parse(char **pbuf, struct filter_pred *pred)
char *tmp, *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
- propogate return value of filter_add_pred() to the user
- return -ENOSPC but not -ENOMEM or -EINVAL when the filter array
is full
Signed-off-by: Li Zefan <[email protected]>
---
kernel/trace/trace_events.c | 10 ++++++----
kernel/trace/trace_events_filter.c | 4 ++--
2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 054bc18..576f4fa 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -521,9 +521,10 @@ event_filter_write(struct file *filp, const char __user *ubuf, size_t cnt,
return cnt;
}
- if (filter_add_pred(call, pred)) {
+ err = filter_add_pred(call, pred);
+ if (err < 0) {
filter_free_pred(pred);
- return -EINVAL;
+ return err;
}
*ppos += cnt;
@@ -588,10 +589,11 @@ subsystem_filter_write(struct file *filp, const char __user *ubuf, size_t cnt,
return cnt;
}
- if (filter_add_subsystem_pred(system, pred)) {
+ err = filter_add_subsystem_pred(system, pred);
+ if (err < 0) {
filter_free_subsystem_preds(system);
filter_free_pred(pred);
- return -EINVAL;
+ return err;
}
*ppos += cnt;
diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 8e3a095..f16a921 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -215,7 +215,7 @@ static int __filter_add_pred(struct ftrace_event_call *call,
}
}
- return -ENOMEM;
+ return -ENOSPC;
}
static int is_string_field(const char *type)
@@ -319,7 +319,7 @@ int filter_add_subsystem_pred(struct event_subsystem *system,
}
if (i == MAX_FILTER_PRED)
- return -EINVAL;
+ return -ENOSPC;
events_for_each(call) {
int err;
--
1.5.4.rc3
I noticed ftrace_event_call->preds and ->preds[*] are not protected
by any lock, and I can manage to trigger kernel oops.
This patch adds filter_mutex to protect concurrent access to
event_subsystem->preds/preds[i] or ftrace_event_call->preds/preds[i],
and make the fast-path filter_match_preds() RCU safe.
Signed-off-by: Li Zefan <[email protected]>
---
kernel/trace/trace.h | 6 +-
kernel/trace/trace_events.c | 4 +-
kernel/trace/trace_events_filter.c | 104 +++++++++++++++++++++++++---------
kernel/trace/trace_events_stage_3.h | 10 +++-
4 files changed, 91 insertions(+), 33 deletions(-)
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index e685ac2..ae05fbb 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -847,13 +847,15 @@ struct filter_pred {
int trace_define_field(struct ftrace_event_call *call, char *type,
char *name, int offset, int size);
extern void filter_free_pred(struct filter_pred *pred);
-extern void filter_print_preds(struct filter_pred **preds,
+extern void filter_print_preds(struct ftrace_event_call *call,
struct trace_seq *s);
extern int filter_parse(char **pbuf, struct filter_pred *pred);
extern int filter_add_pred(struct ftrace_event_call *call,
struct filter_pred *pred);
extern void filter_free_preds(struct ftrace_event_call *call);
-extern int filter_match_preds(struct ftrace_event_call *call, void *rec);
+extern int filter_match_preds(struct filter_pred **preds, void *rec);
+extern void filter_print_subsystem_preds(struct event_subsystem *system,
+ struct trace_seq *s);
extern void filter_free_subsystem_preds(struct event_subsystem *system);
extern int filter_add_subsystem_pred(struct event_subsystem *system,
struct filter_pred *pred);
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 576f4fa..ca1a2b0 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -481,7 +481,7 @@ event_filter_read(struct file *filp, char __user *ubuf, size_t cnt,
trace_seq_init(s);
- filter_print_preds(call->preds, s);
+ filter_print_preds(call, s);
r = simple_read_from_buffer(ubuf, cnt, ppos, s->buffer, s->len);
kfree(s);
@@ -549,7 +549,7 @@ subsystem_filter_read(struct file *filp, char __user *ubuf, size_t cnt,
trace_seq_init(s);
- filter_print_preds(system->preds, s);
+ filter_print_subsystem_preds(system, s);
r = simple_read_from_buffer(ubuf, cnt, ppos, s->buffer, s->len);
kfree(s);
diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index f16a921..8e32dd8 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -22,10 +22,14 @@
#include <linux/uaccess.h>
#include <linux/module.h>
#include <linux/ctype.h>
+#include <linux/mutex.h>
+#include <linux/rcupdate.h>
#include "trace.h"
#include "trace_output.h"
+static DEFINE_MUTEX(filter_mutex);
+
static int filter_pred_64(struct filter_pred *pred, void *event)
{
u64 *addr = (u64 *)(event + pred->offset);
@@ -82,15 +86,19 @@ static int filter_pred_string(struct filter_pred *pred, void *event)
return match;
}
-/* return 1 if event matches, 0 otherwise (discard) */
-int filter_match_preds(struct ftrace_event_call *call, void *rec)
+/*
+ * return 1 if event matches, 0 otherwise (discard)
+ *
+ * should be protected by rcu_read_lock()
+ * */
+int filter_match_preds(struct filter_pred **preds, void *rec)
{
int i, matched, and_failed = 0;
struct filter_pred *pred;
for (i = 0; i < MAX_FILTER_PRED; i++) {
- if (call->preds[i]) {
- pred = call->preds[i];
+ pred = rcu_dereference(preds[i]);
+ if (pred) {
if (and_failed && !pred->or)
continue;
matched = pred->fn(pred, rec);
@@ -109,7 +117,8 @@ int filter_match_preds(struct ftrace_event_call *call, void *rec)
return 1;
}
-void filter_print_preds(struct filter_pred **preds, struct trace_seq *s)
+static void __filter_print_preds(struct filter_pred **preds,
+ struct trace_seq *s)
{
char *field_name;
struct filter_pred *pred;
@@ -137,6 +146,21 @@ void filter_print_preds(struct filter_pred **preds, struct trace_seq *s)
}
}
+void filter_print_preds(struct ftrace_event_call *call, struct trace_seq *s)
+{
+ mutex_lock(&filter_mutex);
+ __filter_print_preds(call->preds, s);
+ mutex_unlock(&filter_mutex);
+}
+
+void filter_print_subsystem_preds(struct event_subsystem *system,
+ struct trace_seq *s)
+{
+ mutex_lock(&filter_mutex);
+ __filter_print_preds(system->preds, s);
+ mutex_unlock(&filter_mutex);
+}
+
static struct ftrace_event_field *
find_event_field(struct ftrace_event_call *call, char *name)
{
@@ -160,29 +184,37 @@ void filter_free_pred(struct filter_pred *pred)
kfree(pred);
}
-void filter_free_preds(struct ftrace_event_call *call)
+static void __filter_free_preds(struct filter_pred **preds)
{
int i;
- if (call->preds) {
- for (i = 0; i < MAX_FILTER_PRED; i++)
- filter_free_pred(call->preds[i]);
- kfree(call->preds);
- call->preds = NULL;
- }
+ if (!preds)
+ return;
+
+ for (i = 0; i < MAX_FILTER_PRED; i++)
+ filter_free_pred(preds[i]);
+ kfree(preds);
+}
+
+void filter_free_preds(struct ftrace_event_call *call)
+{
+ struct filter_pred **preds = call->preds;
+
+ mutex_lock(&filter_mutex);
+ rcu_assign_pointer(call->preds, NULL);
+ mutex_unlock(&filter_mutex);
+
+ synchronize_rcu();
+ __filter_free_preds(preds);
}
void filter_free_subsystem_preds(struct event_subsystem *system)
{
struct ftrace_event_call *call = __start_ftrace_events;
- int i;
- if (system->preds) {
- for (i = 0; i < MAX_FILTER_PRED; i++)
- filter_free_pred(system->preds[i]);
- kfree(system->preds);
- system->preds = NULL;
- }
+ mutex_lock(&filter_mutex);
+ __filter_free_preds(system->preds);
+ mutex_unlock(&filter_mutex);
events_for_each(call) {
if (!call->name || !call->regfunc)
@@ -197,25 +229,35 @@ static int __filter_add_pred(struct ftrace_event_call *call,
struct filter_pred *pred)
{
int i;
+ int ret = 0;
+ struct filter_pred **preds;
- if (call->preds && !pred->compound)
+ if (!pred->compound)
filter_free_preds(call);
+ mutex_lock(&filter_mutex);
+
if (!call->preds) {
- call->preds = kzalloc(MAX_FILTER_PRED * sizeof(pred),
- GFP_KERNEL);
- if (!call->preds)
- return -ENOMEM;
+ preds = kzalloc(MAX_FILTER_PRED * sizeof(pred),
+ GFP_KERNEL);
+ if (!preds) {
+ ret = -ENOMEM;
+ goto unlock;
+ }
+ rcu_assign_pointer(call->preds, preds);
}
for (i = 0; i < MAX_FILTER_PRED; i++) {
if (!call->preds[i]) {
- call->preds[i] = pred;
- return 0;
+ rcu_assign_pointer(call->preds[i], pred);
+ goto unlock;
}
}
+ ret = -ENOSPC;
- return -ENOSPC;
+unlock:
+ mutex_unlock(&filter_mutex);
+ return ret;
}
static int is_string_field(const char *type)
@@ -304,11 +346,15 @@ int filter_add_subsystem_pred(struct event_subsystem *system,
if (system->preds && !pred->compound)
filter_free_subsystem_preds(system);
+ mutex_lock(&filter_mutex);
+
if (!system->preds) {
system->preds = kzalloc(MAX_FILTER_PRED * sizeof(pred),
GFP_KERNEL);
- if (!system->preds)
+ if (!system->preds) {
+ mutex_unlock(&filter_mutex);
return -ENOMEM;
+ }
}
for (i = 0; i < MAX_FILTER_PRED; i++) {
@@ -318,6 +364,8 @@ int filter_add_subsystem_pred(struct event_subsystem *system,
}
}
+ mutex_unlock(&filter_mutex);
+
if (i == MAX_FILTER_PRED)
return -ENOSPC;
diff --git a/kernel/trace/trace_events_stage_3.h b/kernel/trace/trace_events_stage_3.h
index 9d2fa78..973941d 100644
--- a/kernel/trace/trace_events_stage_3.h
+++ b/kernel/trace/trace_events_stage_3.h
@@ -207,8 +207,10 @@ static void ftrace_raw_event_##call(proto) \
struct ftrace_event_call *call = &event_##call; \
struct ring_buffer_event *event; \
struct ftrace_raw_##call *entry; \
+ struct filter_pred **preds; \
unsigned long irq_flags; \
int pc; \
+ int matched = 1; \
\
local_save_flags(irq_flags); \
pc = preempt_count(); \
@@ -222,7 +224,13 @@ static void ftrace_raw_event_##call(proto) \
\
assign; \
\
- if (call->preds && !filter_match_preds(call, entry)) \
+ rcu_read_lock(); \
+ preds = rcu_dereference(call->preds); \
+ if (preds) \
+ matched = filter_match_preds(preds, entry); \
+ rcu_read_unlock(); \
+ \
+ if (!matched) \
ring_buffer_event_discard(event); \
\
trace_nowake_buffer_unlock_commit(event, irq_flags, pc); \
--
1.5.4.rc3
Hi,
On Sat, 2009-04-11 at 15:52 +0800, Li Zefan wrote:
> This patchset fixes some bugs in tracing/filters. Most of the change
> goes to the last patch, and others are small ones.
>
At first glance, they look good and fix real problems - thanks for
fixing them. Re patch 7, there's been some discussion about using rcu
for this. See:
http://lkml.org/lkml/2009/4/5/46
>From that discussion, it seems some non-trivial changes to rcu would be
needed for this. I'm playing around with a different idea now to
hopefully avoid the need for that, or the other approach mentioned, of
temporarily stopping tracing while removing/changing the filters.
Basically my thought is to avoid the problem by not allocating or
destroying the preds when removing filters but instead switch out the
pred->fns with a nop version while keeping the fields intact for awhile.
I think that will work for removing filters, but I still need to think
about how it would (or would not) work for replacing them.
Tom
> Maybe 3rd to 6th patches can be regarded as small enhancements instead
> of bug fixes? But they are behavioural changes.
>
>
> [PATCH 1/7] tracing/filters: NUL-terminate user input filter
> [PATCH 2/7] tracing/filters: fix NULL pointer dereference
> [PATCH 3/7] tracing/filters: allow user input integer to be oct or hex
> [PATCH 4/7] tracing/filters: allow user to specify a filter val to be string
> [PATCH 5/7] tracing/filters: disallow newline as delimeter
> [PATCH 6/7] tracing/filters: return proper error code when writing filter file
> [PATCH 7/7] tracing/filters: make filter preds RCU safe
>
> kernel/trace/trace.h | 6 +-
> kernel/trace/trace_events.c | 16 +++--
> kernel/trace/trace_events_filter.c | 131 ++++++++++++++++++++++++++---------
> kernel/trace/trace_events_stage_3.h | 10 +++-
> 4 files changed, 121 insertions(+), 42 deletions(-)
> ---
Tom Zanussi wrote:
> Hi,
>
> On Sat, 2009-04-11 at 15:52 +0800, Li Zefan wrote:
>> This patchset fixes some bugs in tracing/filters. Most of the change
>> goes to the last patch, and others are small ones.
>>
>
> At first glance, they look good and fix real problems - thanks for
> fixing them. Re patch 7, there's been some discussion about using rcu
> for this. See:
>
> http://lkml.org/lkml/2009/4/5/46
>
Hm, I didn't notice it, since I just started looking into tracing/filters
2 days ago.
>>From that discussion, it seems some non-trivial changes to rcu would be
> needed for this. I'm playing around with a different idea now to
> hopefully avoid the need for that, or the other approach mentioned, of
> temporarily stopping tracing while removing/changing the filters.
>
> Basically my thought is to avoid the problem by not allocating or
> destroying the preds when removing filters but instead switch out the
> pred->fns with a nop version while keeping the fields intact for awhile.
> I think that will work for removing filters, but I still need to think
> about how it would (or would not) work for replacing them.
>
I'll keep an eye on this issue.
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 <[email protected]>
> ---
> 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
>
On Sat, Apr 11, 2009 at 04:30:48AM -0500, Tom Zanussi wrote:
> Hi,
>
> On Sat, 2009-04-11 at 15:52 +0800, Li Zefan wrote:
> > This patchset fixes some bugs in tracing/filters. Most of the change
> > goes to the last patch, and others are small ones.
> >
>
> At first glance, they look good and fix real problems - thanks for
> fixing them. Re patch 7, there's been some discussion about using rcu
> for this. See:
>
> http://lkml.org/lkml/2009/4/5/46
>
> >From that discussion, it seems some non-trivial changes to rcu would be
> needed for this. I'm playing around with a different idea now to
> hopefully avoid the need for that, or the other approach mentioned, of
> temporarily stopping tracing while removing/changing the filters.
>
> Basically my thought is to avoid the problem by not allocating or
> destroying the preds when removing filters but instead switch out the
> pred->fns with a nop version while keeping the fields intact for awhile.
> I think that will work for removing filters, but I still need to think
> about how it would (or would not) work for replacing them.
>
> Tom
It would be sort of reinventing rcu :-)
Well, Paul proposed something recently, hmm I should double check
this discussion.
Anyway, the fixes from Li (other than 4 and 7 for which we have comments)
look very good!
Frederic.
>
> > Maybe 3rd to 6th patches can be regarded as small enhancements instead
> > of bug fixes? But they are behavioural changes.
> >
> >
> > [PATCH 1/7] tracing/filters: NUL-terminate user input filter
> > [PATCH 2/7] tracing/filters: fix NULL pointer dereference
> > [PATCH 3/7] tracing/filters: allow user input integer to be oct or hex
> > [PATCH 4/7] tracing/filters: allow user to specify a filter val to be string
> > [PATCH 5/7] tracing/filters: disallow newline as delimeter
> > [PATCH 6/7] tracing/filters: return proper error code when writing filter file
> > [PATCH 7/7] tracing/filters: make filter preds RCU safe
> >
> > kernel/trace/trace.h | 6 +-
> > kernel/trace/trace_events.c | 16 +++--
> > kernel/trace/trace_events_filter.c | 131 ++++++++++++++++++++++++++---------
> > kernel/trace/trace_events_stage_3.h | 10 +++-
> > 4 files changed, 121 insertions(+), 42 deletions(-)
> > ---
>
On Sat, Apr 11, 2009 at 04:48:33PM +0200, Frederic Weisbecker wrote:
> On Sat, Apr 11, 2009 at 04:30:48AM -0500, Tom Zanussi wrote:
> > Hi,
> >
> > On Sat, 2009-04-11 at 15:52 +0800, Li Zefan wrote:
> > > This patchset fixes some bugs in tracing/filters. Most of the change
> > > goes to the last patch, and others are small ones.
> > >
> >
> > At first glance, they look good and fix real problems - thanks for
> > fixing them. Re patch 7, there's been some discussion about using rcu
> > for this. See:
> >
> > http://lkml.org/lkml/2009/4/5/46
> >
> > >From that discussion, it seems some non-trivial changes to rcu would be
> > needed for this. I'm playing around with a different idea now to
> > hopefully avoid the need for that, or the other approach mentioned, of
> > temporarily stopping tracing while removing/changing the filters.
> >
> > Basically my thought is to avoid the problem by not allocating or
> > destroying the preds when removing filters but instead switch out the
> > pred->fns with a nop version while keeping the fields intact for awhile.
> > I think that will work for removing filters, but I still need to think
> > about how it would (or would not) work for replacing them.
> >
> > Tom
>
> It would be sort of reinventing rcu :-)
> Well, Paul proposed something recently, hmm I should double check
> this discussion.
I am actually thinking about doing this. Please do take a look at:
http://lkml.org/lkml/2009/4/6/332
http://lkml.org/lkml/2009/4/6/496
What I need from you is to tell me whether or not the proposed placement
of rcu_idle(), rcu_idle_start(), and rcu_idle_end() makes sense from a
tracing viewpoint.
Thanx, Paul
> Anyway, the fixes from Li (other than 4 and 7 for which we have comments)
> look very good!
>
> Frederic.
>
>
>
> >
> > > Maybe 3rd to 6th patches can be regarded as small enhancements instead
> > > of bug fixes? But they are behavioural changes.
> > >
> > >
> > > [PATCH 1/7] tracing/filters: NUL-terminate user input filter
> > > [PATCH 2/7] tracing/filters: fix NULL pointer dereference
> > > [PATCH 3/7] tracing/filters: allow user input integer to be oct or hex
> > > [PATCH 4/7] tracing/filters: allow user to specify a filter val to be string
> > > [PATCH 5/7] tracing/filters: disallow newline as delimeter
> > > [PATCH 6/7] tracing/filters: return proper error code when writing filter file
> > > [PATCH 7/7] tracing/filters: make filter preds RCU safe
> > >
> > > kernel/trace/trace.h | 6 +-
> > > kernel/trace/trace_events.c | 16 +++--
> > > kernel/trace/trace_events_filter.c | 131 ++++++++++++++++++++++++++---------
> > > kernel/trace/trace_events_stage_3.h | 10 +++-
> > > 4 files changed, 121 insertions(+), 42 deletions(-)
> > > ---
> >
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On Sat, 2009-04-11 at 16:48 +0200, Frederic Weisbecker wrote:
> On Sat, Apr 11, 2009 at 04:30:48AM -0500, Tom Zanussi wrote:
> > Hi,
> >
> > On Sat, 2009-04-11 at 15:52 +0800, Li Zefan wrote:
> > > This patchset fixes some bugs in tracing/filters. Most of the change
> > > goes to the last patch, and others are small ones.
> > >
> >
> > At first glance, they look good and fix real problems - thanks for
> > fixing them. Re patch 7, there's been some discussion about using rcu
> > for this. See:
> >
> > http://lkml.org/lkml/2009/4/5/46
> >
> > >From that discussion, it seems some non-trivial changes to rcu would be
> > needed for this. I'm playing around with a different idea now to
> > hopefully avoid the need for that, or the other approach mentioned, of
> > temporarily stopping tracing while removing/changing the filters.
> >
> > Basically my thought is to avoid the problem by not allocating or
> > destroying the preds when removing filters but instead switch out the
> > pred->fns with a nop version while keeping the fields intact for awhile.
> > I think that will work for removing filters, but I still need to think
> > about how it would (or would not) work for replacing them.
> >
> > Tom
>
>
>
> It would be sort of reinventing rcu :-)
> Well, Paul proposed something recently, hmm I should double check
> this discussion.
>
Well, if so, the resemblance would be only superficial e.g. the
difference between a cheap parlor trick and David Copperfield. ;-) Mine
would be much simpler and local to the filter code. Also, I think I'm
overstating the 'keeping the fields intact for awhile' part - it should
be possible to not deal with that at all at the cost of a small
possibility of a false positive or negative while switching filters.
I'll run it up the flagpole and see if it flies, and hopefully post a
patch for it, sometime this weekend...
Tom
> Anyway, the fixes from Li (other than 4 and 7 for which we have comments)
> look very good!
>
> Frederic.
>
>
>
> >
> > > Maybe 3rd to 6th patches can be regarded as small enhancements instead
> > > of bug fixes? But they are behavioural changes.
> > >
> > >
> > > [PATCH 1/7] tracing/filters: NUL-terminate user input filter
> > > [PATCH 2/7] tracing/filters: fix NULL pointer dereference
> > > [PATCH 3/7] tracing/filters: allow user input integer to be oct or hex
> > > [PATCH 4/7] tracing/filters: allow user to specify a filter val to be string
> > > [PATCH 5/7] tracing/filters: disallow newline as delimeter
> > > [PATCH 6/7] tracing/filters: return proper error code when writing filter file
> > > [PATCH 7/7] tracing/filters: make filter preds RCU safe
> > >
> > > kernel/trace/trace.h | 6 +-
> > > kernel/trace/trace_events.c | 16 +++--
> > > kernel/trace/trace_events_filter.c | 131 ++++++++++++++++++++++++++---------
> > > kernel/trace/trace_events_stage_3.h | 10 +++-
> > > 4 files changed, 121 insertions(+), 42 deletions(-)
> > > ---
> >
>
* Frederic Weisbecker <[email protected]> wrote:
> On Sat, Apr 11, 2009 at 04:30:48AM -0500, Tom Zanussi wrote:
> > Hi,
> >
> > On Sat, 2009-04-11 at 15:52 +0800, Li Zefan wrote:
> > > This patchset fixes some bugs in tracing/filters. Most of the change
> > > goes to the last patch, and others are small ones.
> > >
> >
> > At first glance, they look good and fix real problems - thanks for
> > fixing them. Re patch 7, there's been some discussion about using rcu
> > for this. See:
> >
> > http://lkml.org/lkml/2009/4/5/46
> >
> > >From that discussion, it seems some non-trivial changes to rcu would be
> > needed for this. I'm playing around with a different idea now to
> > hopefully avoid the need for that, or the other approach mentioned, of
> > temporarily stopping tracing while removing/changing the filters.
> >
> > Basically my thought is to avoid the problem by not allocating or
> > destroying the preds when removing filters but instead switch out the
> > pred->fns with a nop version while keeping the fields intact for awhile.
> > I think that will work for removing filters, but I still need to think
> > about how it would (or would not) work for replacing them.
> >
> > Tom
>
>
>
> It would be sort of reinventing rcu :-)
>
> Well, Paul proposed something recently, hmm I should
> double check this discussion.
>
> Anyway, the fixes from Li (other than 4 and 7 for which we
> have comments) look very good!
Ok, i've applied 1,2,3,6 to tip/tracing/urgent (most of them
are fixes needed for current mainline).
5 had dependency on 4 which is being discussed. 7 is being
discussed as well, with Tom having promised a patch.
Thanks,
Ingo
* Frederic Weisbecker <[email protected]> wrote:
> 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.
Agreed. The user input itself is unambigously parse-able -
and we should not work around parsing limitations by
complicating the user input.
The failure situation that Li found should obviously be
fixed.
Ingo
Commit-ID: bcabd91c271e50eebc0cb9220ac92700332b452e
Gitweb: http://git.kernel.org/tip/bcabd91c271e50eebc0cb9220ac92700332b452e
Author: Li Zefan <[email protected]>
AuthorDate: Sat, 11 Apr 2009 15:52:35 +0800
Committer: Ingo Molnar <[email protected]>
CommitDate: Sun, 12 Apr 2009 11:59:28 +0200
tracing/filters: fix NULL pointer dereference
Try this, and you'll see NULL pointer dereference bug:
# echo -n 'parent_comm ==' > sched/sched_process_fork/filter
Because we passed NULL ptr to simple_strtoull().
Signed-off-by: Li Zefan <[email protected]>
Acked-by: Tom Zanussi <[email protected]>
Acked-by: Frederic Weisbecker <[email protected]>
Cc: Steven Rostedt <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
kernel/trace/trace_events_filter.c | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 026be41..9d2162f 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -410,6 +410,11 @@ int filter_parse(char **pbuf, struct filter_pred *pred)
}
}
+ if (!val_str) {
+ pred->field_name = NULL;
+ return -EINVAL;
+ }
+
pred->field_name = kstrdup(pred->field_name, GFP_KERNEL);
if (!pred->field_name)
return -ENOMEM;
Commit-ID: a3e0ab050774117d4a6173087c8bf3888662a83f
Gitweb: http://git.kernel.org/tip/a3e0ab050774117d4a6173087c8bf3888662a83f
Author: Li Zefan <[email protected]>
AuthorDate: Sat, 11 Apr 2009 15:52:51 +0800
Committer: Ingo Molnar <[email protected]>
CommitDate: Sun, 12 Apr 2009 11:59:28 +0200
tracing/filters: allow user input integer to be oct or hex
Before patch:
# echo 'parent_pid == 0x10' > events/sched/sched_process_fork/filter
# cat sched/sched_process_fork/filter
parent_pid == 0
After patch:
# cat sched/sched_process_fork/filter
parent_pid == 16
Also check the input more strictly.
Signed-off-by: Li Zefan <[email protected]>
Acked-by: Tom Zanussi <[email protected]>
Acked-by: Frederic Weisbecker <[email protected]>
Cc: Steven Rostedt <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
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 9d2162f..49b3ef5 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -419,12 +419,13 @@ int filter_parse(char **pbuf, struct filter_pred *pred)
if (!pred->field_name)
return -ENOMEM;
- pred->val = simple_strtoull(val_str, &tmp, 10);
+ 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;
return 0;
}
Commit-ID: 44e9c8b7adc52079f0535f9de0c2c2477831389b
Gitweb: http://git.kernel.org/tip/44e9c8b7adc52079f0535f9de0c2c2477831389b
Author: Li Zefan <[email protected]>
AuthorDate: Sat, 11 Apr 2009 15:55:28 +0800
Committer: Ingo Molnar <[email protected]>
CommitDate: Sun, 12 Apr 2009 11:59:29 +0200
tracing/filters: return proper error code when writing filter file
- propagate return value of filter_add_pred() to the user
- return -ENOSPC but not -ENOMEM or -EINVAL when the filter array
is full
Signed-off-by: Li Zefan <[email protected]>
Acked-by: Tom Zanussi <[email protected]>
Acked-by: Frederic Weisbecker <[email protected]>
Cc: Steven Rostedt <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
kernel/trace/trace_events.c | 10 ++++++----
kernel/trace/trace_events_filter.c | 4 ++--
2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 054bc18..576f4fa 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -521,9 +521,10 @@ event_filter_write(struct file *filp, const char __user *ubuf, size_t cnt,
return cnt;
}
- if (filter_add_pred(call, pred)) {
+ err = filter_add_pred(call, pred);
+ if (err < 0) {
filter_free_pred(pred);
- return -EINVAL;
+ return err;
}
*ppos += cnt;
@@ -588,10 +589,11 @@ subsystem_filter_write(struct file *filp, const char __user *ubuf, size_t cnt,
return cnt;
}
- if (filter_add_subsystem_pred(system, pred)) {
+ err = filter_add_subsystem_pred(system, pred);
+ if (err < 0) {
filter_free_subsystem_preds(system);
filter_free_pred(pred);
- return -EINVAL;
+ return err;
}
*ppos += cnt;
diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 49b3ef5..e03cbf1 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -215,7 +215,7 @@ static int __filter_add_pred(struct ftrace_event_call *call,
}
}
- return -ENOMEM;
+ return -ENOSPC;
}
static int is_string_field(const char *type)
@@ -319,7 +319,7 @@ int filter_add_subsystem_pred(struct event_subsystem *system,
}
if (i == MAX_FILTER_PRED)
- return -EINVAL;
+ return -ENOSPC;
events_for_each(call) {
int err;
> 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?
>
Right! Actually I thought about this, then I found one issue, suppose event
foo and event bar both have a field named fb but one is string and one is
integer. Now do this:
# echo 'fb == 123' > events/foo-bar/filter
This will set both filters, but not only the integer one.
But now I think this hardly happen in real-life, and it's not a big issue
if it does happen. So I agree with you on this issue.
Thanks.
> I think it would be a saner basis of parsing.
>
* Li Zefan <[email protected]> wrote:
> > 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?
> >
>
> Right! Actually I thought about this, then I found one issue,
> suppose event foo and event bar both have a field named fb but one
> is string and one is integer. Now do this:
>
> # echo 'fb == 123' > events/foo-bar/filter
>
> This will set both filters, but not only the integer one.
>
> But now I think this hardly happen in real-life, and it's not a
> big issue if it does happen. So I agree with you on this issue.
Yeah, good point. I think we should avoid such field name aliasing.
Even without the filter assignment ambiguity problem it would be
confusing to users to have two same-name but different-type fields
in two separate events.
Ingo