Re: [RFC 5/5] x86,perf: Only allow rdpmc if a perf_event is mapped

[Peter Zijlstra <peterz@infradead.org>]

On Mon, Oct 20, 2014 at 10:39:48AM -0700, Andy Lutomirski wrote:
> On Mon, Oct 20, 2014 at 9:49 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> > On Mon, Oct 20, 2014 at 1:33 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> >> On Sun, Oct 19, 2014 at 03:57:54PM -0700, Andy Lutomirski wrote:
> >>> > Maybe, but at that point we commit to yet another ABI... I'd rather just
> >>> > put a 'sane' implementation in a library or so.
> >>>
> >>> This cuts both ways, though.  For vdso timekeeping, the underlying
> >>> data structure has changed repeatedly, sometimes to add features, and
> >>> sometimes for performance, and the vdso has done a good job insulating
> >>> userspace from it.  (In fact, until 3.16, even the same exact kernel
> >>> version couldn't be relied on to have the same data structure with
> >>> different configs, and even now, no one really wants to teach user
> >>> libraries how to parse the pvclock data structures.)
> >>
> >> Fair enough, but as it stands we've already committed to the data
> >> structure exposed to userspace.
> >
> > True.
> >
> > OTOH, if a vdso function gets added, a few releases go by, and all the
> > userspace tools get updated, then the old data structure could be
> > dropped if needed by clearing cap_user_rdpmc.
> >
> > Anyway, this is so far out of scope for the current project that I'm
> > going to ignore it.
> 
> OK, I lied.
> 
> I haven't tested it, but it looks like any existing users of
> cap_user_rdpmc may have serious issues.  That flag is set to 1 for
> essentially all perf_events on x86, even events that aren't part of
> the x86_pmu.  Since the default .event_idx callback doesn't return
> zero, lots of other events will appear to be rdpmcable.  This includes
> the AMD uncore pmu, which looks like it actually supports rdpmc, but
> hwc->idx seems to be missing an offset.
> 
> If this is the case, then user code can't reliably use the userpage
> rdpmc mechanism, so maybe it should be deprecated (or at least get a
> new flag bit).

Seeing how we've not actually had bugreports on this, I suspect we can
still fix that.