Re: [RFC 8/8] drm/i915: Support for retrieving MMIO register values alongwith timestamps through perf

From: Chris Wilson <chris@chris-wilson.co.uk>
To: "Gupta, Sourab" <sourab.gupta@intel.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"Wu, Jabin" <jabin.wu@intel.com>,
	"Woo, Insoo" <insoo.woo@intel.com>
Subject: Re: [RFC 8/8] drm/i915: Support for retrieving MMIO register values alongwith timestamps through perf
Date: Wed, 5 Aug 2015 11:30:15 +0100	[thread overview]
Message-ID: <20150805103015.GL10387@nuc-i3427.alporthouse.com> (raw)
In-Reply-To: <1438770072.22294.9.camel@sourabgu-desktop>

On Wed, Aug 05, 2015 at 10:18:50AM +0000, Gupta, Sourab wrote:
> On Wed, 2015-08-05 at 10:03 +0000, Chris Wilson wrote:
> > On Wed, Aug 05, 2015 at 11:25:44AM +0530, sourab.gupta@intel.com wrote:
> > > From: Sourab Gupta <sourab.gupta@intel.com>
> > > 
> > > This patch adds support for retrieving MMIO register values alongwith
> > > timestamps and forwarding them to userspace through perf.
> > > The userspace can request upto 8 MMIO register values to be dumped.
> > > The addresses of upto 8 MMIO registers can be passed through perf attr
> > > config. The registers are checked against a whitelist before passing them
> > > on. The commands to dump the values of these MMIO registers are then
> > > inserted into the ring alongwith commands to dump the timestamps.
> > 
> > The values reported to userspace are deltas across batches right? We
> > don't expose the global value to an unprivileged user? It would be nice
> > to clarify that in perf_init so that the reviewer is aware that
> > the issue of unprivileged information leak is addressed (or at least
> > reminded that the register values do not leak!).
> > -Chris
> > 
> Hi Chris,
> Two things here:
> 1) Only root is allowed to call event_init for gen pmu. This restriction
> is there in event_init. (The thought behind this restriction being that
> we are profiling data across contexts here, so a process wishing to
> listen to global activity happening in system across all contexts ought
> to have root priviliges). Is this thought process correct? Should we be
> supporting non-root users too?

That is not clear in this patch, so you need to address such concerns at
least in the changelog, and preferrably with a reminder in the
whitelist (that these register reads are safe because they are being
done from a privileged context only - we then have a red flag in case we
lower it).

What is the privilege check you are using here exactly?

For gen pmu, I want it user accessible. How long does it take to execute
my batches is a common developer query. We may even be able to make
anonymised information freely available ala top (per-process GPU usage,
memory usage, though cgroups/namespacing rules probably apply here).

> 2) Being already a root, do we need to worry about the unauthorized mmio
> access while exposing these mmio values through the interface?

Yes. See above, the information here can be anonymised and useful for
user processes exactly like TIMESTAMP.

> In the current patches, the full mmio register value is dumped to be
> passed on to userspace (no deltas across batches), provided the register
> is there in the whitelist. Does the question of unpriviliged information
> leak arise here(the user being root)?

Not for root.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx