[PATCH 06/15] drm: Drop modeset_lock_all from the getproperty ioctl

[Alex Deucher]

On Mon, Apr 3, 2017 at 4:32 AM, Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
> Properties, i.e. the struct drm_property specifying the type and value
> range of a property, not the instantiation on a given object, are
> invariant over the lifetime of a driver.
>
> Hence no locking at all is needed, we can just remove it.
>
> While at it give the function some love and simplify it, to get it
> under the 80 char limit:
> - Straighten the loops to reduce the nesting.
> - use u64_to_user_ptr casting helper
> - use put_user for fixed u64 copies.
>
> Note there's a small behavioural change in that we now copy parts of
> the values to userspace if the arrays are a bit too small. Since
> userspace will immediately retry anyway, this doesn't matter.
>
> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>

This causes a segfault in our ddx:
https://bugs.freedesktop.org/show_bug.cgi?id=100673

Alex


> ---
>  drivers/gpu/drm/drm_property.c | 72 +++++++++++++++++-------------------------
>  1 file changed, 29 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c
> index b17959c3e099..3feef0659940 100644
> --- a/drivers/gpu/drm/drm_property.c
> +++ b/drivers/gpu/drm/drm_property.c
> @@ -442,8 +442,7 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev,
>         struct drm_property *property;
>         int enum_count = 0;
>         int value_count = 0;
> -       int ret = 0, i;
> -       int copied;
> +       int i, copied;
>         struct drm_property_enum *prop_enum;
>         struct drm_mode_property_enum __user *enum_ptr;
>         uint64_t __user *values_ptr;
> @@ -451,55 +450,43 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev,
>         if (!drm_core_check_feature(dev, DRIVER_MODESET))
>                 return -EINVAL;
>
> -       drm_modeset_lock_all(dev);
>         property = drm_property_find(dev, out_resp->prop_id);
> -       if (!property) {
> -               ret = -ENOENT;
> -               goto done;
> -       }
> -
> -       if (drm_property_type_is(property, DRM_MODE_PROP_ENUM) ||
> -                       drm_property_type_is(property, DRM_MODE_PROP_BITMASK)) {
> -               list_for_each_entry(prop_enum, &property->enum_list, head)
> -                       enum_count++;
> -       }
> -
> -       value_count = property->num_values;
> +       if (!property)
> +               return -ENOENT;
>
>         strncpy(out_resp->name, property->name, DRM_PROP_NAME_LEN);
>         out_resp->name[DRM_PROP_NAME_LEN-1] = 0;
>         out_resp->flags = property->flags;
>
> -       if ((out_resp->count_values >= value_count) && value_count) {
> -               values_ptr = (uint64_t __user *)(unsigned long)out_resp->values_ptr;
> -               for (i = 0; i < value_count; i++) {
> -                       if (copy_to_user(values_ptr + i, &property->values[i], sizeof(uint64_t))) {
> -                               ret = -EFAULT;
> -                               goto done;
> -                       }
> +       value_count = property->num_values;
> +       values_ptr = u64_to_user_ptr(out_resp->values_ptr);
> +
> +       for (i = 0; i < value_count; i++) {
> +               if (i < out_resp->count_values &&
> +                   put_user(property->values[i], values_ptr + i)) {
> +                       return -EFAULT;
>                 }
>         }
>         out_resp->count_values = value_count;
>
> +       copied = 0;
> +       enum_ptr = u64_to_user_ptr(out_resp->enum_blob_ptr);
> +
>         if (drm_property_type_is(property, DRM_MODE_PROP_ENUM) ||
> -                       drm_property_type_is(property, DRM_MODE_PROP_BITMASK)) {
> -               if ((out_resp->count_enum_blobs >= enum_count) && enum_count) {
> -                       copied = 0;
> -                       enum_ptr = (struct drm_mode_property_enum __user *)(unsigned long)out_resp->enum_blob_ptr;
> -                       list_for_each_entry(prop_enum, &property->enum_list, head) {
> -
> -                               if (copy_to_user(&enum_ptr[copied].value, &prop_enum->value, sizeof(uint64_t))) {
> -                                       ret = -EFAULT;
> -                                       goto done;
> -                               }
> -
> -                               if (copy_to_user(&enum_ptr[copied].name,
> -                                                &prop_enum->name, DRM_PROP_NAME_LEN)) {
> -                                       ret = -EFAULT;
> -                                       goto done;
> -                               }
> -                               copied++;
> -                       }
> +           drm_property_type_is(property, DRM_MODE_PROP_BITMASK)) {
> +               list_for_each_entry(prop_enum, &property->enum_list, head) {
> +                       enum_count++;
> +                       if (out_resp->count_enum_blobs <= enum_count)
> +                               continue;
> +
> +                       if (copy_to_user(&enum_ptr[copied].value,
> +                                        &prop_enum->value, sizeof(uint64_t)))
> +                               return -EFAULT;
> +
> +                       if (copy_to_user(&enum_ptr[copied].name,
> +                                        &prop_enum->name, DRM_PROP_NAME_LEN))
> +                               return -EFAULT;
> +                       copied++;
>                 }
>                 out_resp->count_enum_blobs = enum_count;
>         }
> @@ -514,9 +501,8 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev,
>          */
>         if (drm_property_type_is(property, DRM_MODE_PROP_BLOB))
>                 out_resp->count_enum_blobs = 0;
> -done:
> -       drm_modeset_unlock_all(dev);
> -       return ret;
> +
> +       return 0;
>  }
>
>  static void drm_property_free_blob(struct kref *kref)
> --
> 2.11.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel