| Message ID | 20170403083304.9083-7-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Apr 3, 2017 at 4:32 AM, Daniel Vetter <daniel.vetter@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@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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Apr 13, 2017 at 04:03:16PM -0400, Alex Deucher wrote: > On Mon, Apr 3, 2017 at 4:32 AM, Daniel Vetter <daniel.vetter@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@intel.com> > > This causes a segfault in our ddx: > https://bugs.freedesktop.org/show_bug.cgi?id=100673 Should be fixed by: commit 8cb68c83ab99a474ae847602f0c514d0fe17cc82 Author: Daniel Vetter <daniel.vetter@ffwll.ch> Date: Mon Apr 10 13:54:45 2017 +0200 drm: Fix get_property logic fumble Yet again I've proven that I can't negate conditions :( Testcase: igt/kms_properties/get_property-sanity Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Reviewed-by: Sean Paul <seanpaul@chromium.org> Fixes: eb8eb02ed850 ("drm: Drop modeset_lock_all from the getproperty ioctl") Does that help? -Chris
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)
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@intel.com> --- drivers/gpu/drm/drm_property.c | 72 +++++++++++++++++------------------------- 1 file changed, 29 insertions(+), 43 deletions(-)