Re: [PATCH] tracing: a few fields of struct trace_iterator are zeroedby mistake

[David Sharp]

On Fri, Aug 2, 2013 at 10:16 AM, Andrew Vagin <avagin@xxxxxxxxxx> 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 <rostedt@xxxxxxxxxxx>
> Cc: Frederic Weisbecker <fweisbec@xxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: David Sharp <dhsharp@xxxxxxxxxx>
> Cc: Hiraku Toyooka <hiraku.toyooka.gu@xxxxxxxxxxx>
> Cc: Arjan van de Ven <arjan@xxxxxxxxxxxxxxx>
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@xxxxxxxxxxx>
>
> Signed-off-by: Andrew Vagin <avagin@xxxxxxxxxx>
> ---
>  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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/