(2013/07/27 2:25), Oleg Nesterov wrote:
> event_filter_read/write() are racy, ftrace_event_call can be already
> freed by trace_remove_event_call() callers.
>
> 1. Shift mutex_lock(event_mutex) from print/apply_event_filter to
> the callers.
>
> 2. Change the callers, event_filter_read() and event_filter_write()
> to read i_private under this mutex and abort if it is NULL.
>
> This fixes nothing, but now we can change debugfs_remove("filter")
> callers to nullify ->i_private and fix the the problem.
Looks good for me. (Note: this also has some cosmetic changes)
Reviewed-by: Masami Hiramatsu <masami.hiramatsu...@hitachi.com>
Thanks!
>
> Signed-off-by: Oleg Nesterov <o...@redhat.com>
> ---
> kernel/trace/trace_events.c | 28 +++++++++++++++++++---------
> kernel/trace/trace_events_filter.c | 17 ++++++-----------
> 2 files changed, 25 insertions(+), 20 deletions(-)
>
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 39cb049..b5144c4 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -978,24 +978,30 @@ static ssize_t
> event_filter_read(struct file *filp, char __user *ubuf, size_t cnt,
> loff_t *ppos)
> {
> - struct ftrace_event_call *call = filp->private_data;
> + struct ftrace_event_call *call;
> struct trace_seq *s;
> - int r;
> + int r = -ENODEV;
>
> if (*ppos)
> return 0;
>
> s = kmalloc(sizeof(*s), GFP_KERNEL);
> +
> if (!s)
> return -ENOMEM;
>
> trace_seq_init(s);
>
> - print_event_filter(call, s);
> - r = simple_read_from_buffer(ubuf, cnt, ppos, s->buffer, s->len);
> + mutex_lock(&event_mutex);
> + call = event_file_data(filp);
> + if (call)
> + print_event_filter(call, s);
> + mutex_unlock(&event_mutex);
>
> - kfree(s);
> + if (call)
> + r = simple_read_from_buffer(ubuf, cnt, ppos, s->buffer, s->len);
>
> + kfree(s);
> return r;
> }
>
> @@ -1003,9 +1009,9 @@ static ssize_t
> event_filter_write(struct file *filp, const char __user *ubuf, size_t cnt,
> loff_t *ppos)
> {
> - struct ftrace_event_call *call = filp->private_data;
> + struct ftrace_event_call *call;
> char *buf;
> - int err;
> + int err = -ENODEV;
>
> if (cnt >= PAGE_SIZE)
> return -EINVAL;
> @@ -1020,13 +1026,17 @@ event_filter_write(struct file *filp, const char
> __user *ubuf, size_t cnt,
> }
> buf[cnt] = '\0';
>
> - err = apply_event_filter(call, buf);
> + mutex_lock(&event_mutex);
> + call = event_file_data(filp);
> + if (call)
> + err = apply_event_filter(call, buf);
> + mutex_unlock(&event_mutex);
> +
> free_page((unsigned long) buf);
> if (err < 0)
> return err;
>
> *ppos += cnt;
> -
> return cnt;
> }
>
> diff --git a/kernel/trace/trace_events_filter.c
> b/kernel/trace/trace_events_filter.c
> index 0c7b75a..97daa8c 100644
> --- a/kernel/trace/trace_events_filter.c
> +++ b/kernel/trace/trace_events_filter.c
> @@ -637,17 +637,15 @@ static void append_filter_err(struct filter_parse_state
> *ps,
> free_page((unsigned long) buf);
> }
>
> +/* caller must hold event_mutex */
> void print_event_filter(struct ftrace_event_call *call, struct trace_seq *s)
> {
> - struct event_filter *filter;
> + struct event_filter *filter = call->filter;
>
> - mutex_lock(&event_mutex);
> - filter = call->filter;
> if (filter && filter->filter_string)
> trace_seq_printf(s, "%s\n", filter->filter_string);
> else
> trace_seq_puts(s, "none\n");
> - mutex_unlock(&event_mutex);
> }
>
> void print_subsystem_event_filter(struct event_subsystem *system,
> @@ -1841,23 +1839,22 @@ static int create_system_filter(struct
> event_subsystem *system,
> return err;
> }
>
> +/* caller must hold event_mutex */
> int apply_event_filter(struct ftrace_event_call *call, char *filter_string)
> {
> struct event_filter *filter;
> - int err = 0;
> -
> - mutex_lock(&event_mutex);
> + int err;
>
> if (!strcmp(strstrip(filter_string), "0")) {
> filter_disable(call);
> filter = call->filter;
> if (!filter)
> - goto out_unlock;
> + return 0;
> RCU_INIT_POINTER(call->filter, NULL);
> /* Make sure the filter is not being used */
> synchronize_sched();
> __free_filter(filter);
> - goto out_unlock;
> + return 0;
> }
>
> err = create_filter(call, filter_string, true, &filter);
> @@ -1884,8 +1881,6 @@ int apply_event_filter(struct ftrace_event_call *call,
> char *filter_string)
> __free_filter(tmp);
> }
> }
> -out_unlock:
> - mutex_unlock(&event_mutex);
>
> return err;
> }
>
--
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu...@hitachi.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
