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

From: Ingo Molnar
Date: Mon Sep 14 2015 - 05:32:23 EST



* Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:

> On Sat, Sep 12, 2015 at 10:11:20AM +0200, Ingo Molnar wrote:
> >
> > * Peter Zijlstra <peterz@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/