Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1222280 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [RFC][PATCH 2/3] perf: Fix u16 overflows Peter Zijlstra <peterz@infradead.org> - 2015-09-10 18:30 +0200
    Re: [RFC][PATCH 2/3] perf: Fix u16 overflows Ingo Molnar <mingo@kernel.org> - 2015-09-12 10:20 +0200
      Re: [RFC][PATCH 2/3] perf: Fix u16 overflows Peter Zijlstra <peterz@infradead.org> - 2015-09-14 11:10 +0200
        Re: [RFC][PATCH 2/3] perf: Fix u16 overflows Ingo Molnar <mingo@kernel.org> - 2015-09-14 11:40 +0200

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

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.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1223349

* 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1223958

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1223978

* 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web