On Fri, Aug 2, 2013 at 10:16 AM, Andrew Vagin <ava...@openvz.org> wrote:
> tracing_read_pipe zeros all fields bellow "seq". The declaration contains
> a comment about that, but it doesn't help.
>
> The first field is "snapshot", it's true when current open file is
> snapshot. Looks obvious, that it should not be zeroed.
>
> The second field is "started". It was converted from cpumask_t to
> cpumask_var_t (v2.6.28-4983-g4462344) or by another words it was
> converted from cpumask to pointer on cpumask.
>
> Currently the reference on "started" memory is lost after the first read
> from tracing_read_pipe and a proper object will never be freed.
>
> The "started" is never dereferenced for trace_pipe, because trace_pipe
> can't have the TRACE_FILE_ANNOTATE options (why?).
>
> Cc: Steven Rostedt <rost...@goodmis.org>
> Cc: Frederic Weisbecker <fweis...@gmail.com>
> Cc: Ingo Molnar <mi...@redhat.com>
> Cc: David Sharp <dhsh...@google.com>
> Cc: Hiraku Toyooka <hiraku.toyooka...@hitachi.com>
> Cc: Arjan van de Ven <ar...@linux.intel.com>
> Cc: Masami Hiramatsu <masami.hiramatsu...@hitachi.com>
>
> Signed-off-by: Andrew Vagin <ava...@openvz.org>
> ---
> include/linux/ftrace_event.h | 10 ++++++----
> kernel/trace/trace.c | 1 +
> 2 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
> index 4372658..44cdc11 100644
> --- a/include/linux/ftrace_event.h
> +++ b/include/linux/ftrace_event.h
> @@ -78,6 +78,11 @@ struct trace_iterator {
> /* trace_seq for __print_flags() and __print_symbolic() etc. */
> struct trace_seq tmp_seq;
>
> + cpumask_var_t started;
> +
> + /* it's true when current open file is snapshot */
> + bool snapshot;
> +
> /* The below is zeroed out in pipe_read */
> struct trace_seq seq;
> struct trace_entry *ent;
> @@ -90,10 +95,7 @@ struct trace_iterator {
> loff_t pos;
> long idx;
>
> - cpumask_var_t started;
> -
> - /* it's true when current open file is snapshot */
> - bool snapshot;
> + /* All new field here will be zeroed out in pipe_read */
Wow. That is a terrible hack. Thanks for noticing it.
If I may suggest a way better idea: put these zeroed-in-pipe_read
members in an embedded anonymous structure, and zero the structure in
pipe_read.
> };
>
> enum trace_iter_flags {
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 0cd500b..897f553 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -4166,6 +4166,7 @@ waitagain:
> memset(&iter->seq, 0,
> sizeof(struct trace_iterator) -
> offsetof(struct trace_iterator, seq));
> + cpumask_clear(iter->started);
> iter->pos = -1;
>
> trace_event_read_lock();
> --
> 1.7.1
>
--
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/
