Revision - 2aeb188 - perf/core: Fix locking for children siblings group read

Revision 2aeb1883547626d82c597cce2c99f0b9c62e2425 authored by Jiri Olsa on 20 July 2017, 14:14:55 UTC, committed by Ingo Molnar on 21 July 2017, 07:54:23 UTC

perf/core: Fix locking for children siblings group read

We're missing ctx lock when iterating children siblings
within the perf_read path for group reading. Following
race and crash can happen:

User space doing read syscall on event group leader:

T1:
  perf_read
    lock event->ctx->mutex
    perf_read_group
      lock leader->child_mutex
      __perf_read_group_add(child)
        list_for_each_entry(sub, &leader->sibling_list, group_entry)

---->   sub might be invalid at this point, because it could
        get removed via perf_event_exit_task_context in T2

Child exiting and cleaning up its events:

T2:
  perf_event_exit_task_context
    lock ctx->mutex
    list_for_each_entry_safe(child_event, next, &child_ctx->event_list,...
      perf_event_exit_event(child)
        lock ctx->lock
        perf_group_detach(child)
        unlock ctx->lock

---->   child is removed from sibling_list without any sync
        with T1 path above

        ...
        free_event(child)

Before the child is removed from the leader's child_list,
(and thus is omitted from perf_read_group processing), we
need to ensure that perf_read_group touches child's
siblings under its ctx->lock.

Peter further notes:

| One additional note; this bug got exposed by commit:
|
|   ba5213ae6b88 ("perf/core: Correct event creation with PERF_FORMAT_GROUP")
|
| which made it possible to actually trigger this code-path.

Tested-by: Andi Kleen <ak@linux.intel.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Fixes: ba5213ae6b88 ("perf/core: Correct event creation with PERF_FORMAT_GROUP")
Link: http://lkml.kernel.org/r/20170720141455.2106-1-jolsa@kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>

1 parent 3bda69c

Files
Changes

Permalinks

amdtp-stream-trace.h

/*
 * amdtp-stream-trace.h - tracepoint definitions to dump a part of packet data
 *
 * Copyright (c) 2016 Takashi Sakamoto
 * Licensed under the terms of the GNU General Public License, version 2.
 */

#undef TRACE_SYSTEM
#define TRACE_SYSTEM		snd_firewire_lib

#if !defined(_AMDTP_STREAM_TRACE_H) || defined(TRACE_HEADER_MULTI_READ)
#define _AMDTP_STREAM_TRACE_H

#include <linux/tracepoint.h>

TRACE_EVENT(in_packet,
	TP_PROTO(const struct amdtp_stream *s, u32 cycles, u32 cip_header[2], unsigned int payload_length, unsigned int index),
	TP_ARGS(s, cycles, cip_header, payload_length, index),
	TP_STRUCT__entry(
		__field(unsigned int, second)
		__field(unsigned int, cycle)
		__field(int, channel)
		__field(int, src)
		__field(int, dest)
		__field(u32, cip_header0)
		__field(u32, cip_header1)
		__field(unsigned int, payload_quadlets)
		__field(unsigned int, packet_index)
		__field(unsigned int, irq)
		__field(unsigned int, index)
	),
	TP_fast_assign(
		__entry->second = cycles / CYCLES_PER_SECOND;
		__entry->cycle = cycles % CYCLES_PER_SECOND;
		__entry->channel = s->context->channel;
		__entry->src = fw_parent_device(s->unit)->node_id;
		__entry->dest = fw_parent_device(s->unit)->card->node_id;
		__entry->cip_header0 = cip_header[0];
		__entry->cip_header1 = cip_header[1];
		__entry->payload_quadlets = payload_length / 4;
		__entry->packet_index = s->packet_index;
		__entry->irq = !!in_interrupt();
		__entry->index = index;
	),
	TP_printk(
		"%02u %04u %04x %04x %02d %08x %08x %03u %02u %01u %02u",
		__entry->second,
		__entry->cycle,
		__entry->src,
		__entry->dest,
		__entry->channel,
		__entry->cip_header0,
		__entry->cip_header1,
		__entry->payload_quadlets,
		__entry->packet_index,
		__entry->irq,
		__entry->index)
);

TRACE_EVENT(out_packet,
	TP_PROTO(const struct amdtp_stream *s, u32 cycles, __be32 *cip_header, unsigned int payload_length, unsigned int index),
	TP_ARGS(s, cycles, cip_header, payload_length, index),
	TP_STRUCT__entry(
		__field(unsigned int, second)
		__field(unsigned int, cycle)
		__field(int, channel)
		__field(int, src)
		__field(int, dest)
		__field(u32, cip_header0)
		__field(u32, cip_header1)
		__field(unsigned int, payload_quadlets)
		__field(unsigned int, packet_index)
		__field(unsigned int, irq)
		__field(unsigned int, index)
	),
	TP_fast_assign(
		__entry->second = cycles / CYCLES_PER_SECOND;
		__entry->cycle = cycles % CYCLES_PER_SECOND;
		__entry->channel = s->context->channel;
		__entry->src = fw_parent_device(s->unit)->card->node_id;
		__entry->dest = fw_parent_device(s->unit)->node_id;
		__entry->cip_header0 = be32_to_cpu(cip_header[0]);
		__entry->cip_header1 = be32_to_cpu(cip_header[1]);
		__entry->payload_quadlets = payload_length / 4;
		__entry->packet_index = s->packet_index;
		__entry->irq = !!in_interrupt();
		__entry->index = index;
	),
	TP_printk(
		"%02u %04u %04x %04x %02d %08x %08x %03u %02u %01u %02u",
		__entry->second,
		__entry->cycle,
		__entry->src,
		__entry->dest,
		__entry->channel,
		__entry->cip_header0,
		__entry->cip_header1,
		__entry->payload_quadlets,
		__entry->packet_index,
		__entry->irq,
		__entry->index)
);

TRACE_EVENT(in_packet_without_header,
	TP_PROTO(const struct amdtp_stream *s, u32 cycles, unsigned int payload_quadlets, unsigned int data_blocks, unsigned int index),
	TP_ARGS(s, cycles, payload_quadlets, data_blocks, index),
	TP_STRUCT__entry(
		__field(unsigned int, second)
		__field(unsigned int, cycle)
		__field(int, channel)
		__field(int, src)
		__field(int, dest)
		__field(unsigned int, payload_quadlets)
		__field(unsigned int, data_blocks)
		__field(unsigned int, data_block_counter)
		__field(unsigned int, packet_index)
		__field(unsigned int, irq)
		__field(unsigned int, index)
	),
	TP_fast_assign(
		__entry->second = cycles / CYCLES_PER_SECOND;
		__entry->cycle = cycles % CYCLES_PER_SECOND;
		__entry->channel = s->context->channel;
		__entry->src = fw_parent_device(s->unit)->node_id;
		__entry->dest = fw_parent_device(s->unit)->card->node_id;
		__entry->payload_quadlets = payload_quadlets;
		__entry->data_blocks = data_blocks,
		__entry->data_block_counter = s->data_block_counter,
		__entry->packet_index = s->packet_index;
		__entry->irq = !!in_interrupt();
		__entry->index = index;
	),
	TP_printk(
		"%02u %04u %04x %04x %02d %03u %3u %3u %02u %01u %02u",
		__entry->second,
		__entry->cycle,
		__entry->src,
		__entry->dest,
		__entry->channel,
		__entry->payload_quadlets,
		__entry->data_blocks,
		__entry->data_block_counter,
		__entry->packet_index,
		__entry->irq,
		__entry->index)
);

TRACE_EVENT(out_packet_without_header,
	TP_PROTO(const struct amdtp_stream *s, u32 cycles, unsigned int payload_length, unsigned int data_blocks, unsigned int index),
	TP_ARGS(s, cycles, payload_length, data_blocks, index),
	TP_STRUCT__entry(
		__field(unsigned int, second)
		__field(unsigned int, cycle)
		__field(int, channel)
		__field(int, src)
		__field(int, dest)
		__field(unsigned int, payload_quadlets)
		__field(unsigned int, data_blocks)
		__field(unsigned int, data_block_counter)
		__field(unsigned int, packet_index)
		__field(unsigned int, irq)
		__field(unsigned int, index)
	),
	TP_fast_assign(
		__entry->second = cycles / CYCLES_PER_SECOND;
		__entry->cycle = cycles % CYCLES_PER_SECOND;
		__entry->channel = s->context->channel;
		__entry->src = fw_parent_device(s->unit)->card->node_id;
		__entry->dest = fw_parent_device(s->unit)->node_id;
		__entry->payload_quadlets = payload_length / 4;
		__entry->data_blocks = data_blocks,
		__entry->data_blocks = s->data_block_counter,
		__entry->packet_index = s->packet_index;
		__entry->irq = !!in_interrupt();
		__entry->index = index;
	),
	TP_printk(
		"%02u %04u %04x %04x %02d %03u %02u %03u %02u %01u %02u",
		__entry->second,
		__entry->cycle,
		__entry->src,
		__entry->dest,
		__entry->channel,
		__entry->payload_quadlets,
		__entry->data_blocks,
		__entry->data_block_counter,
		__entry->packet_index,
		__entry->irq,
		__entry->index)
);

#endif

#undef TRACE_INCLUDE_PATH
#define TRACE_INCLUDE_PATH	.
#undef TRACE_INCLUDE_FILE
#define TRACE_INCLUDE_FILE	amdtp-stream-trace
#include <trace/define_trace.h>

Showing with 0 additions and 0 deletions (0 / 0 diffs computed)

Computing file changes ...