[Intel-gfx] [PATCH v7 12/12] drm/i915: add support for perf configuration queries

[Chris Wilson]

Quoting Lionel Landwerlin (2019-07-09 10:32:08)
> +static int query_perf_config_data(struct drm_i915_private *i915,
> +                                 struct drm_i915_query_item *query_item,
> +                                 bool use_uuid)
> +{
> +       struct drm_i915_query_perf_config __user *user_query_config_ptr =
> +               u64_to_user_ptr(query_item->data_ptr);
> +       struct drm_i915_perf_oa_config __user *user_config_ptr =
> +               u64_to_user_ptr(query_item->data_ptr +
> +                               sizeof(struct drm_i915_query_perf_config));
> +       struct drm_i915_perf_oa_config user_config;
> +       struct i915_oa_config *oa_config = NULL;
> +       u32 flags, total_size;
> +       int ret;
> +
> +       if (!i915->perf.initialized)
> +               return -ENODEV;
> +
> +       total_size = sizeof(struct drm_i915_query_perf_config) +
> +               sizeof(struct drm_i915_perf_oa_config);
> +
> +       if (query_item->length == 0)
> +               return total_size;
> +
> +       if (query_item->length < total_size) {
> +               DRM_DEBUG("Invalid query config data item size=%u expected=%u\n",
> +                         query_item->length, total_size);
> +               return -EINVAL;
> +       }
> +
> +       if (!access_ok(user_query_config_ptr, total_size))
> +               return -EFAULT;
> +
> +       if (__get_user(flags, &user_query_config_ptr->flags))
> +               return -EFAULT;
> +
> +       if (flags != 0)
> +               return -EINVAL;
> +
> +       ret = mutex_lock_interruptible(&i915->perf.metrics_lock);
> +       if (ret)
> +               return ret;
> +
> +       if (use_uuid) {
> +               char uuid[UUID_STRING_LEN + 1] = { 0, };
> +               struct i915_oa_config *tmp;
> +               int id;
> +
> +               BUILD_BUG_ON(sizeof(user_query_config_ptr->uuid) >= sizeof(uuid));
> +
> +               if (__copy_from_user(uuid, user_query_config_ptr->uuid,
> +                                    sizeof(user_query_config_ptr->uuid))) {

__copy_from_user() from inside a mutex brings back nightmares. I think
you are ok, but you are tarnishing this lock with mmap_sem, which is
huge and may bite in future. And it looks superfluous, no? You could do
the copy form user before taking the lock, and then once you have the
config pinned, you can drop the lock again.
-Chris