[RFC][PATCH 0/3] variuos perf fixes

[RFC][PATCH 0/3] variuos perf fixes

[Peter Zijlstra]

Hi,

Vince reported some fail here:

  lkml.kernel.org/r/alpine.DEB.2.20.1509021227380.32729@vincent-weaver-1.umelst.maine.edu

These patches are the result of me trying to fix it.

Compile tested only, I'll try them after a sleep..

[RFC][PATCH 1/3] perf: Restructure perf syscall point of no return

[Peter Zijlstra]

[-- Attachment #1: peterz-perf-rework-syscall-error-patch.patch --]
[-- Type: text/plain, Size: 2815 bytes --]

The exclusive_event_installable() stuff only works because its
exclusive with the grouping bits.

Rework the code such that there is a sane place to error out before we
go do things we cannot undo.

Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/events/core.c |  108 ++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 78 insertions(+), 30 deletions(-)

--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -8242,15 +8271,31 @@ SYSCALL_DEFINE5(perf_event_open,
 		goto err_context;
 	}
 
-	if (move_group) {
-		gctx = group_leader->ctx;
+	gctx = group_leader->ctx;
+	if (move_group)
+		mutex_lock_double(&gctx->mutex, &ctx->mutex);
+	else
+		mutex_lock(&ctx->mutex);
 
+	/*
+	 * Must be under the same ctx::mutex as perf_install_in_context(),
+	 * because we need to serialize with concurrent event creation.
+	 */
+	if (!exclusive_event_installable(event, ctx)) {
+		/* exclusive and group stuff are assumed mutually exclusive */
+		WARN_ON_ONCE(move_group);
+
+		err = -EBUSY;
+		goto err_locked;
+	}
+
+	WARN_ON_ONCE(ctx->parent_ctx);
+
+	if (move_group) {
 		/*
 		 * See perf_event_ctx_lock() for comments on the details
 		 * of swizzling perf_event::ctx.
 		 */
-		mutex_lock_double(&gctx->mutex, &ctx->mutex);
-
 		perf_remove_from_context(group_leader, false);
 
 		list_for_each_entry(sibling, &group_leader->sibling_list,
@@ -8258,13 +8308,7 @@ SYSCALL_DEFINE5(perf_event_open,
 			perf_remove_from_context(sibling, false);
 			put_ctx(gctx);
 		}
-	} else {
-		mutex_lock(&ctx->mutex);
-	}
 
-	WARN_ON_ONCE(ctx->parent_ctx);
-
-	if (move_group) {
 		/*
 		 * Wait for everybody to stop referencing the events through
 		 * the old lists, before installing it on new lists.
@@ -8296,22 +8340,20 @@ SYSCALL_DEFINE5(perf_event_open,
 		perf_event__state_init(group_leader);
 		perf_install_in_context(ctx, group_leader, group_leader->cpu);
 		get_ctx(ctx);
-	}
 
-	if (!exclusive_event_installable(event, ctx)) {
-		err = -EBUSY;
-		mutex_unlock(&ctx->mutex);
-		fput(event_file);
-		goto err_context;
+		/*
+		 * Now that all events are installed in @ctx, nothing
+		 * references @gctx anymore, so drop the last reference we have
+		 * on it.
+		 */
+		put_ctx(gctx);
 	}
 
 	perf_install_in_context(ctx, event, event->cpu);
 	perf_unpin_context(ctx);
 
-	if (move_group) {
+	if (move_group)
 		mutex_unlock(&gctx->mutex);
-		put_ctx(gctx);
-	}
 	mutex_unlock(&ctx->mutex);
 
 	put_online_cpus();
@@ -8338,6 +8380,12 @@ SYSCALL_DEFINE5(perf_event_open,
 	fd_install(event_fd, event_file);
 	return event_fd;
 
+err_locked:
+	if (move_group)
+		mutex_unlock(&gctx->mutex);
+	mutex_unlock(&ctx->mutex);
+/* err_file: */
+	fput(event_file);
 err_context:
 	perf_unpin_context(ctx);
 	put_ctx(ctx);

[RFC][PATCH 2/3] perf: Fix u16 overflows

[Peter Zijlstra]

[-- Attachment #1: peterz-perf-fix-record-size-limit.patch --]
[-- Type: text/plain, Size: 3483 bytes --]

Vince reported that its possible to overflow the various size fields
and get weird stuff if you stick too many events in a group.

Put a lid on this by requiring the fixed record size not exceed 16k.
This is still a fair amount of events (silly amount really) and leaves
plenty room for callchains and stack dwarves while also avoiding
overflowing the u16 variables.

Reported-by: Vince Weaver <vincent.weaver@maine.edu>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/events/core.c |   50 ++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 40 insertions(+), 10 deletions(-)

--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -1243,11 +1243,7 @@ static inline void perf_event__state_ini
 					      PERF_EVENT_STATE_INACTIVE;
 }
 
-/*
- * Called at perf_event creation and when events are attached/detached from a
- * group.
- */
-static void perf_event__read_size(struct perf_event *event)
+static void __perf_event_read_size(struct perf_event *event, int nr_siblings)
 {
 	int entry = sizeof(u64); /* value */
 	int size = 0;
@@ -1263,7 +1259,7 @@ static void perf_event__read_size(struct
 		entry += sizeof(u64);
 
 	if (event->attr.read_format & PERF_FORMAT_GROUP) {
-		nr += event->group_leader->nr_siblings;
+		nr += nr_siblings;
 		size += sizeof(u64);
 	}
 
@@ -1271,14 +1267,11 @@ static void perf_event__read_size(struct
 	event->read_size = size;
 }
 
-static void perf_event__header_size(struct perf_event *event)
+static void __perf_event_header_size(struct perf_event *event, u64 sample_type)
 {
 	struct perf_sample_data *data;
-	u64 sample_type = event->attr.sample_type;
 	u16 size = 0;
 
-	perf_event__read_size(event);
-
 	if (sample_type & PERF_SAMPLE_IP)
 		size += sizeof(data->ip);
 
@@ -1303,6 +1296,17 @@ static void perf_event__header_size(stru
 	event->header_size = size;
 }
 
+/*
+ * Called at perf_event creation and when events are attached/detached from a
+ * group.
+ */
+static void perf_event__header_size(struct perf_event *event)
+{
+	__perf_event_read_size(event,
+			       event->group_leader->nr_siblings);
+	__perf_event_header_size(event, event->attr.sample_type);
+}
+
 static void perf_event__id_header_size(struct perf_event *event)
 {
 	struct perf_sample_data *data;
@@ -1330,6 +1334,27 @@ static void perf_event__id_header_size(s
 	event->id_header_size = size;
 }
 
+static bool perf_event_validate_size(struct perf_event *event)
+{
+	/*
+	 * The values computed here will be over-written when we actually
+	 * attach the event.
+	 */
+	__perf_event_read_size(event, event->group_leader->nr_siblings + 1);
+	__perf_event_header_size(event, event->attr.sample_type & ~PERF_SAMPLE_READ);
+	perf_event__id_header_size(event);
+
+	/*
+	 * Sum the lot; should not exceed the 64k limit we have on records.
+	 * Conservative limit to allow for callchains and other variable fields.
+	 */
+	if (event->read_size + event->header_size +
+	    event->id_header_size + sizeof(struct perf_event_header) >= 16*1024)
+		return false;
+
+	return true;
+}
+
 static void perf_group_attach(struct perf_event *event)
 {
 	struct perf_event *group_leader = event->group_leader, *pos;
@@ -8248,6 +8273,11 @@ SYSCALL_DEFINE5(perf_event_open,
 	else
 		mutex_lock(&ctx->mutex);
 
+	if (!perf_event_validate_size(event)) {
+		err = -E2BIG;
+		goto err_locked;
+	}
+
 	/*
 	 * Must be under the same ctx::mutex as perf_install_in_context(),
 	 * because we need to serialize with concurrent event creation.

Re: [RFC][PATCH 2/3] perf: Fix u16 overflows

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> Vince reported that its possible to overflow the various size fields
> and get weird stuff if you stick too many events in a group.
> 
> Put a lid on this by requiring the fixed record size not exceed 16k.
> This is still a fair amount of events (silly amount really) and leaves
> plenty room for callchains and stack dwarves while also avoiding
> overflowing the u16 variables.

Does this leave a natural ABI extension route here, in case in the future it 
becomes a problem? We should take aside a value to mean 'larger record' or such?

Could we list exactly which fields this concerns, in which structures?

Thanks,

	Ingo

Re: [RFC][PATCH 2/3] perf: Fix u16 overflows

[Peter Zijlstra]

On Sat, Sep 12, 2015 at 10:11:20AM +0200, Ingo Molnar wrote:
> 
> * Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > Vince reported that its possible to overflow the various size fields
> > and get weird stuff if you stick too many events in a group.
> > 
> > Put a lid on this by requiring the fixed record size not exceed 16k.
> > This is still a fair amount of events (silly amount really) and leaves
> > plenty room for callchains and stack dwarves while also avoiding
> > overflowing the u16 variables.
> 
> Does this leave a natural ABI extension route here, in case in the future it 
> becomes a problem? We should take aside a value to mean 'larger record' or such?

So this all is a result of:

struct perf_event_header {
	__u32   type;
	__u16   misc;
	__u16   size;
};

And we've not even done the 'sensible' thing of interpreting @size as
@size*8 :/ That is, because entries must be u64 aligned, the lower 3
bits of @size will always be 0.

Now there are of course ways we can 'grow' if we really have to. One
would be to set aside a MISC bit to indicate we should do that *8 thing,
which would allow up to 512 Kb records.

That said, 64k is already quite a lot of data, and I'm not sure we
want to have records bigger than that. Certainly not for samples,
copying that much data on an interrupt is just not going to be fast.

And I'm not sure there's a sensible use-case for having this many events
in a group (and there's good reasons not to do it).

In any case, the patch only pokes at internal stuff, the ABI isn't
affected beyond refusing to create humongous groups.

Re: [RFC][PATCH 2/3] perf: Fix u16 overflows

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Sat, Sep 12, 2015 at 10:11:20AM +0200, Ingo Molnar wrote:
> > 
> > * Peter Zijlstra <peterz@infradead.org> wrote:
> > 
> > > Vince reported that its possible to overflow the various size fields
> > > and get weird stuff if you stick too many events in a group.
> > > 
> > > Put a lid on this by requiring the fixed record size not exceed 16k.
> > > This is still a fair amount of events (silly amount really) and leaves
> > > plenty room for callchains and stack dwarves while also avoiding
> > > overflowing the u16 variables.
> > 
> > Does this leave a natural ABI extension route here, in case in the future it 
> > becomes a problem? We should take aside a value to mean 'larger record' or such?
> 
> So this all is a result of:
> 
> struct perf_event_header {
> 	__u32   type;
> 	__u16   misc;
> 	__u16   size;
> };
> 
> And we've not even done the 'sensible' thing of interpreting @size as
> @size*8 :/ That is, because entries must be u64 aligned, the lower 3
> bits of @size will always be 0.
> 
> Now there are of course ways we can 'grow' if we really have to. One
> would be to set aside a MISC bit to indicate we should do that *8 thing,
> which would allow up to 512 Kb records.
>
> 	__u32   type;
> 	__u16   misc;
> 	__u16   size;
> };

Makes sense!

Btw., it appears that header->type is using only about 4 bits at the moment, out 
of 32.

So future extensions could split it into two and use the other __u16 half as more 
header->misc fields, should we run out of them (we seem to be close to). Such 
user-space requesting extended misc bits would have to parse the new format 
records.

> That said, 64k is already quite a lot of data, and I'm not sure we want to have 
> records bigger than that. Certainly not for samples, copying that much data on 
> an interrupt is just not going to be fast.
> 
> And I'm not sure there's a sensible use-case for having this many events in a 
> group (and there's good reasons not to do it).
> 
> In any case, the patch only pokes at internal stuff, the ABI isn't affected 
> beyond refusing to create humongous groups.

Fair enough!

Thanks,

	Ingo

[RFC][PATCH 3/3] perf: Fix races in computing the header sizes

[Peter Zijlstra]

[-- Attachment #1: peterz-perf-fix-size-compute.patch --]
[-- Type: text/plain, Size: 1615 bytes --]

There are two races with the current code:

 - Another event can join the group and compute a larger header_size
   concurrently, if the smaller store wins we'll have an incorrect
   header_size set.

 - We compute the header_size after the event becomes active,
   therefore its possible to use the size before its computed.

Remedy the first by moving the computation inside the ctx::mutex lock,
and the second by placing it _before_ perf_install_in_context().

Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/events/core.c |   15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -8349,6 +8349,15 @@ SYSCALL_DEFINE5(perf_event_open,
 		put_ctx(gctx);
 	}
 
+	/*
+	 * Precalculate sample_data sizes; do while holding ctx::mutex such
+	 * that we're serialized against further additions and before
+	 * perf_install_in_context() which is the point the event is active and
+	 * can use these values.
+	 */
+	perf_event__header_size(event);
+	perf_event__id_header_size(event);
+
 	perf_install_in_context(ctx, event, event->cpu);
 	perf_unpin_context(ctx);
 
@@ -8365,12 +8374,6 @@ SYSCALL_DEFINE5(perf_event_open,
 	mutex_unlock(&current->perf_event_mutex);
 
 	/*
-	 * Precalculate sample_data sizes
-	 */
-	perf_event__header_size(event);
-	perf_event__id_header_size(event);
-
-	/*
 	 * Drop the reference on the group_event after placing the
 	 * new event on the sibling_list. This ensures destruction
 	 * of the group leader will find the pointer to itself in

Re: [RFC][PATCH 0/3] variuos perf fixes

[Vince Weaver]

On Thu, 10 Sep 2015, Peter Zijlstra wrote:

> Vince reported some fail here:
> 
>   lkml.kernel.org/r/alpine.DEB.2.20.1509021227380.32729@vincent-weaver-1.umelst.maine.edu
> 
> These patches are the result of me trying to fix it.
> 
> Compile tested only, I'll try them after a sleep..

Sorry for the delay testing, your e-mails continue being sent to SPAM by 
gmail for some reason.

I patched up a 4.3-rc1 system with the patches, and pretty much my 
*entire* perf_event test suite causes kernel oopses.

This is on an AMD a10 system.

[ 1564.442224] BUG: unable to handle kernel NULL pointer dereference at 00000000000001e8
[ 1564.450151] IP: [<ffffffff810daf91>] SYSC_perf_event_open+0x60f/0x985
[ 1564.456655] PGD 8d691067 PUD 8d705067 PMD 0 
[ 1564.461013] Oops: 0000 [#1] SMP 
[ 1564.464307] Modules linked in: nfsd auth_rpcgss oid_registry nfs_acl nfs lockd grace fscache sunrpc nls_utf8 nls_cp437 vfat fat snd_hda_codec_realtek snd_hda_codec_generic snd_hda_codec_hdmi kvm_amd kvm ghash_clmulni_intel snd_hda_intel ppdev sha256_generic hmac drbg ansi_cprng snd_hda_codec snd_hda_core tpm_infineon evdev snd_hwdep hp_wmi sparse_keymap aesni_intel aes_x86_64 ablk_helper cryptd lrw gf128mul glue_helper snd_pcm radeon ttm drm_kms_helper drm psmouse serio_raw pcspkr i2c_algo_bit efivars pl2303 fb_sys_fops k10temp snd_timer acpi_cpufreq usbserial parport_pc shpchp i2c_piix4 syscopyarea parport wmi sysfillrect i2c_core snd button processor sysimgblt soundcore tpm_tis tpm sg sr_mod cdrom sd_mod ohci_pci xhci_pci ohci_hcd ehci_pci ahci xhci_hcd ehci_hcd tg3 libahci ptp crc32c_intel pps_core
[ 1564.537217]  libphy libata usbcore scsi_mod usb_common
[ 1564.541187] CPU: 3 PID: 2077 Comm: branches Tainted: G        W       4.3.0-rc1+ #8
[ 1564.548884] Hardware name: Hewlett-Packard HP Compaq Pro 6305 SFF/1850, BIOS K06 v02.57 08/16/2013
[ 1564.557889] task: ffff88009c7c2140 ti: ffff88008d724000 task.ti: ffff88008d724000
[ 1564.565413] RIP: 0010:[<ffffffff810daf91>]  [<ffffffff810daf91>] SYSC_perf_event_open+0x60f/0x985
[ 1564.574360] RSP: 0018:ffff88008d727e78  EFLAGS: 00010287
[ 1564.579701] RAX: ffff880224155400 RBX: ffff8802240e5800 RCX: 0000000000000020
[ 1564.586870] RDX: ffff88022de31cc0 RSI: ffff88022654e060 RDI: ffff88009d1e3918
[ 1564.594037] RBP: 0000000000000000 R08: ffffffff81816d80 R09: 0000000000000000
[ 1564.601204] R10: 0000000000000000 R11: 0000000000000002 R12: ffff88022620bc00
[ 1564.608373] R13: 0000000000000000 R14: 000000002620bc00 R15: ffff88009c7c2140
[ 1564.615542] FS:  00007f5177613700(0000) GS:ffff88022ed80000(0000) knlGS:0000000000000000
[ 1564.623678] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1564.629455] CR2: 00000000000001e8 CR3: 000000008d556000 CR4: 00000000000406e0
[ 1564.636623] Stack:
[ 1564.638647]  ffff880200000002 ffff880224155400 0000000000000000 ffffffff81816d80
[ 1564.646150]  0100000000000003 0000000000000000 ffffffff00000000 0000007000000000
[ 1564.653648]  0000000000000004 0000000000000000 0000000000000000 0000000000000000
[ 1564.661157] Call Trace:
[ 1564.663629]  [<ffffffff8141c01b>] ? entry_SYSCALL_64_fastpath+0x16/0x6e
[ 1564.670280] Code: 48 89 44 24 08 76 24 44 8b 74 24 08 e9 df 00 00 00 48 8b 74 24 10 48 89 df e8 60 bf ff ff 85 c0 41 89 c6 0f 85 c7 00 00 00 eb b6 <48> 8b 85 e8 01 00 00 45 85 ed 4d 8d 7c 24 10 48 89 04 24 74 1f 
[ 1564.690602] RIP  [<ffffffff810daf91>] SYSC_perf_event_open+0x60f/0x985
[ 1564.697183]  RSP <ffff88008d727e78>
[ 1564.700695] CR2: 00000000000001e8
[ 1564.710184] ---[ end trace 5b268a35cc2e53a3 ]---

addr2line -e ./vmlinux 0xffffffff810daf91
/home/vince/research/linux-kernel/linux.git/kernel/events/core.c:8323

        if (IS_ERR(event_file)) {
                err = PTR_ERR(event_file);
                goto err_context;
        }

        gctx = group_leader->ctx;
	^^^^^ = line 8323
        if (move_group)
                mutex_lock_double(&gctx->mutex, &ctx->mutex);
        else
                mutex_lock(&ctx->mutex);



Vince

Re: [RFC][PATCH 0/3] variuos perf fixes

[Peter Zijlstra]

On Sun, Sep 13, 2015 at 07:10:56PM -0400, Vince Weaver wrote:
> On Thu, 10 Sep 2015, Peter Zijlstra wrote:
> 
> > Vince reported some fail here:
> > 
> >   lkml.kernel.org/r/alpine.DEB.2.20.1509021227380.32729@vincent-weaver-1.umelst.maine.edu
> > 
> > These patches are the result of me trying to fix it.
> > 
> > Compile tested only, I'll try them after a sleep..
> 
> Sorry for the delay testing, your e-mails continue being sent to SPAM by 
> gmail for some reason.
> 
> I patched up a 4.3-rc1 system with the patches, and pretty much my 
> *entire* perf_event test suite causes kernel oopses.
> 
> This is on an AMD a10 system.
> 
> [ 1564.442224] BUG: unable to handle kernel NULL pointer dereference at 00000000000001e8

Yeah, I really should just not set out patches when I'm that tired.

What's worse, I already fixed this splat once, but forgot to copy it
over to the right machine etc..

Total fail.