[PATCH v8 02/12] drm/i915: Add i915 perf infrastructure

[Robert Bragg]

On Mon, Oct 31, 2016 at 5:13 PM, Matthew Auld <
matthew.william.auld at gmail.com> wrote:

> On 31 October 2016 at 16:27, Robert Bragg <robert at sixbynine.org> wrote:
> >
> >
> > On Fri, Oct 28, 2016 at 3:27 PM, Matthew Auld
> > <matthew.william.auld at gmail.com> wrote:
> >>
> >> > +/* Note we copy the properties from userspace outside of the i915
> perf
> >> > + * mutex to avoid an awkward lockdep with mmap_sem.
> >> > + *
> >> > + * Note this function only validates properties in isolation it
> doesn't
> >> > + * validate that the combination of properties makes sense or that
> all
> >> > + * properties necessary for a particular kind of stream have been
> set.
> >> > + */
> >> > +static int read_properties_unlocked(struct drm_i915_private
> *dev_priv,
> >> > +                                   u64 __user *uprops,
> >> > +                                   u32 n_props,
> >> > +                                   struct perf_open_properties
> *props)
> >> > +{
> >> > +       u64 __user *uprop = uprops;
> >> > +       int i;
> >> > +
> >> > +       memset(props, 0, sizeof(struct perf_open_properties));
> >> > +
> >> > +       if (!n_props) {
> >> > +               DRM_ERROR("No i915 perf properties given");
> >> > +               return -EINVAL;
> >> > +       }
> >> > +
> >> > +       if (n_props > DRM_I915_PERF_PROP_MAX) {
> >> Ah but DRM_I915_PERF_PROP_MAX is not a property itself.
> >
> >
> > I'm not sure I follow what your implied concern is?
> >
> > This is just a sanity check for the number properties given by userspace,
> > based on the assumption that there's currently no reason for multiple
> values
> > with a particular property id.
> >
> All I meant was should it not be n_props >= DRM_I915_PERF_PROP_MAX ?


> So with that fixed, or if I'm completely mad:
> Reviewed-by: Matthew Auld <matthew.auld at intel.com>
>

Ah, I see. Actually tbh I think either is reasonable...

The check is mainly about ruling out the silly large values that could be
given, imposing a upper-bound to the number of properties expected from
userspace. It might help catch userspace giving garbage/undefined data, or
block attempts to get the kernel parsing huge amounts of property data
which should never be necessary for configuring a stream. It doesn't e.g.
stop userspace specifying duplicate property IDs even if they supply less
than the maximum allowed. So even if it allowed say 2x the number of
properties I think it would still pretty much do its job.

I could imagine in the future the same check might become much more fuzzy
if we have a case where userspace might need to legitimately specify the
same property ID multiple times (where the sequential order is relevant).

_PERF_PROP_MAX is the last in the enum whereby we can interpret it as an
upper bound on the number of properties while we don't currently expect to
see property IDs duplicated.

The detail here though is that ID 0 is reserved so _PERF_PROP_MAX is more
like ('the maximum number of properties' + 1) - and so this is what you're
essentially highlighting.

I can change this - maybe with a comment about ID 0 being reserved and
explaining the assumption that property ID duplicates aren't currently
expected

Thanks for the review!
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20161031/058022e6/attachment.html>