| Message ID | 20170620202837.1701-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jun 20, 2017 at 10:28:37PM +0200, Daniel Vetter wrote: > In > > commit 91eefc05f0ac71902906b2058360e61bd25137fe > Author: Daniel Vetter <daniel.vetter@ffwll.ch> > Date: Wed Dec 14 00:08:10 2016 +0100 > > drm: Tighten locking in drm_mode_getconnector > > I reordered the logic a bit in that IOCTL, but that broke userspace > since it'll get the new mode list, but not the new property values. > Fix that again. > > v2: Fix up the error path handling when copy_to_user for the modes > failes (Dhinakaran). > > Fixes: 91eefc05f0ac ("drm: Tighten locking in drm_mode_getconnector") > Cc: Sean Paul <seanpaul@chromium.org> > Cc: Daniel Vetter <daniel.vetter@intel.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: David Airlie <airlied@linux.ie> > Cc: dri-devel@lists.freedesktop.org > Reported-by: "H.J. Lu" <hjl.tools@gmail.com> > Tested-by: "H.J. Lu" <hjl.tools@gmail.com> > Cc: <stable@vger.kernel.org> # v4.11+ > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100576 > Cc: "H.J. Lu" <hjl.tools@gmail.com> > Cc: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Yep, lgtm Reviewed-by: Sean Paul <seanpaul@chromium.org> > --- > drivers/gpu/drm/drm_connector.c | 38 ++++++++++++++++++++------------------ > 1 file changed, 20 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > index 5cd61aff7857..8072e6e4c62c 100644 > --- a/drivers/gpu/drm/drm_connector.c > +++ b/drivers/gpu/drm/drm_connector.c > @@ -1293,21 +1293,6 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, > if (!connector) > return -ENOENT; > > - drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > - encoder = drm_connector_get_encoder(connector); > - if (encoder) > - out_resp->encoder_id = encoder->base.id; > - else > - out_resp->encoder_id = 0; > - > - ret = drm_mode_object_get_properties(&connector->base, file_priv->atomic, > - (uint32_t __user *)(unsigned long)(out_resp->props_ptr), > - (uint64_t __user *)(unsigned long)(out_resp->prop_values_ptr), > - &out_resp->count_props); > - drm_modeset_unlock(&dev->mode_config.connection_mutex); > - if (ret) > - goto out_unref; > - > for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) > if (connector->encoder_ids[i] != 0) > encoders_count++; > @@ -1320,7 +1305,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, > if (put_user(connector->encoder_ids[i], > encoder_ptr + copied)) { > ret = -EFAULT; > - goto out_unref; > + goto out; > } > copied++; > } > @@ -1364,15 +1349,32 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, > if (copy_to_user(mode_ptr + copied, > &u_mode, sizeof(u_mode))) { > ret = -EFAULT; > + mutex_unlock(&dev->mode_config.mutex); > + > goto out; > } > copied++; > } > } > out_resp->count_modes = mode_count; > -out: > mutex_unlock(&dev->mode_config.mutex); > -out_unref: > + > + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > + encoder = drm_connector_get_encoder(connector); > + if (encoder) > + out_resp->encoder_id = encoder->base.id; > + else > + out_resp->encoder_id = 0; > + > + /* Only grab properties after probing, to make sure EDID and other > + * properties reflect the latest status. */ > + ret = drm_mode_object_get_properties(&connector->base, file_priv->atomic, > + (uint32_t __user *)(unsigned long)(out_resp->props_ptr), > + (uint64_t __user *)(unsigned long)(out_resp->prop_values_ptr), > + &out_resp->count_props); > + drm_modeset_unlock(&dev->mode_config.connection_mutex); > + > +out: > drm_connector_put(connector); > > return ret; > -- > 2.11.0
On Tue, 2017-06-20 at 22:28 +0200, Daniel Vetter wrote: > In > > commit 91eefc05f0ac71902906b2058360e61bd25137fe > Author: Daniel Vetter <daniel.vetter@ffwll.ch> > Date: Wed Dec 14 00:08:10 2016 +0100 > > drm: Tighten locking in drm_mode_getconnector > > I reordered the logic a bit in that IOCTL, but that broke userspace > since it'll get the new mode list, but not the new property values. > Fix that again. > > v2: Fix up the error path handling when copy_to_user for the modes > failes (Dhinakaran). > > Fixes: 91eefc05f0ac ("drm: Tighten locking in drm_mode_getconnector") > Cc: Sean Paul <seanpaul@chromium.org> > Cc: Daniel Vetter <daniel.vetter@intel.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: David Airlie <airlied@linux.ie> > Cc: dri-devel@lists.freedesktop.org > Reported-by: "H.J. Lu" <hjl.tools@gmail.com> > Tested-by: "H.J. Lu" <hjl.tools@gmail.com> > Cc: <stable@vger.kernel.org> # v4.11+ > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100576 > Cc: "H.J. Lu" <hjl.tools@gmail.com> > Cc: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com> If CI is happy, Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> -DK > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/drm_connector.c | 38 ++++++++++++++++++++------------------ > 1 file changed, 20 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > index 5cd61aff7857..8072e6e4c62c 100644 > --- a/drivers/gpu/drm/drm_connector.c > +++ b/drivers/gpu/drm/drm_connector.c > @@ -1293,21 +1293,6 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, > if (!connector) > return -ENOENT; > > - drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > - encoder = drm_connector_get_encoder(connector); > - if (encoder) > - out_resp->encoder_id = encoder->base.id; > - else > - out_resp->encoder_id = 0; > - > - ret = drm_mode_object_get_properties(&connector->base, file_priv->atomic, > - (uint32_t __user *)(unsigned long)(out_resp->props_ptr), > - (uint64_t __user *)(unsigned long)(out_resp->prop_values_ptr), > - &out_resp->count_props); > - drm_modeset_unlock(&dev->mode_config.connection_mutex); > - if (ret) > - goto out_unref; > - > for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) > if (connector->encoder_ids[i] != 0) > encoders_count++; > @@ -1320,7 +1305,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, > if (put_user(connector->encoder_ids[i], > encoder_ptr + copied)) { > ret = -EFAULT; > - goto out_unref; > + goto out; > } > copied++; > } > @@ -1364,15 +1349,32 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, > if (copy_to_user(mode_ptr + copied, > &u_mode, sizeof(u_mode))) { > ret = -EFAULT; > + mutex_unlock(&dev->mode_config.mutex); > + > goto out; > } > copied++; > } > } > out_resp->count_modes = mode_count; > -out: > mutex_unlock(&dev->mode_config.mutex); > -out_unref: > + > + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > + encoder = drm_connector_get_encoder(connector); > + if (encoder) > + out_resp->encoder_id = encoder->base.id; > + else > + out_resp->encoder_id = 0; > + > + /* Only grab properties after probing, to make sure EDID and other > + * properties reflect the latest status. */ > + ret = drm_mode_object_get_properties(&connector->base, file_priv->atomic, > + (uint32_t __user *)(unsigned long)(out_resp->props_ptr), > + (uint64_t __user *)(unsigned long)(out_resp->prop_values_ptr), > + &out_resp->count_props); > + drm_modeset_unlock(&dev->mode_config.connection_mutex); > + > +out: > drm_connector_put(connector); > > return ret;
On Tue, Jun 20, 2017 at 05:40:25PM -0400, Sean Paul wrote: > On Tue, Jun 20, 2017 at 10:28:37PM +0200, Daniel Vetter wrote: > > In > > > > commit 91eefc05f0ac71902906b2058360e61bd25137fe > > Author: Daniel Vetter <daniel.vetter@ffwll.ch> > > Date: Wed Dec 14 00:08:10 2016 +0100 > > > > drm: Tighten locking in drm_mode_getconnector > > > > I reordered the logic a bit in that IOCTL, but that broke userspace > > since it'll get the new mode list, but not the new property values. > > Fix that again. > > > > v2: Fix up the error path handling when copy_to_user for the modes > > failes (Dhinakaran). > > > > Fixes: 91eefc05f0ac ("drm: Tighten locking in drm_mode_getconnector") > > Cc: Sean Paul <seanpaul@chromium.org> > > Cc: Daniel Vetter <daniel.vetter@intel.com> > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > Cc: David Airlie <airlied@linux.ie> > > Cc: dri-devel@lists.freedesktop.org > > Reported-by: "H.J. Lu" <hjl.tools@gmail.com> > > Tested-by: "H.J. Lu" <hjl.tools@gmail.com> > > Cc: <stable@vger.kernel.org> # v4.11+ > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100576 > > Cc: "H.J. Lu" <hjl.tools@gmail.com> > > Cc: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > Yep, lgtm > > > Reviewed-by: Sean Paul <seanpaul@chromium.org> Pushed to -fixes, thanks for the review. -Daniel > > > --- > > drivers/gpu/drm/drm_connector.c | 38 ++++++++++++++++++++------------------ > > 1 file changed, 20 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > > index 5cd61aff7857..8072e6e4c62c 100644 > > --- a/drivers/gpu/drm/drm_connector.c > > +++ b/drivers/gpu/drm/drm_connector.c > > @@ -1293,21 +1293,6 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, > > if (!connector) > > return -ENOENT; > > > > - drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > > - encoder = drm_connector_get_encoder(connector); > > - if (encoder) > > - out_resp->encoder_id = encoder->base.id; > > - else > > - out_resp->encoder_id = 0; > > - > > - ret = drm_mode_object_get_properties(&connector->base, file_priv->atomic, > > - (uint32_t __user *)(unsigned long)(out_resp->props_ptr), > > - (uint64_t __user *)(unsigned long)(out_resp->prop_values_ptr), > > - &out_resp->count_props); > > - drm_modeset_unlock(&dev->mode_config.connection_mutex); > > - if (ret) > > - goto out_unref; > > - > > for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) > > if (connector->encoder_ids[i] != 0) > > encoders_count++; > > @@ -1320,7 +1305,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, > > if (put_user(connector->encoder_ids[i], > > encoder_ptr + copied)) { > > ret = -EFAULT; > > - goto out_unref; > > + goto out; > > } > > copied++; > > } > > @@ -1364,15 +1349,32 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, > > if (copy_to_user(mode_ptr + copied, > > &u_mode, sizeof(u_mode))) { > > ret = -EFAULT; > > + mutex_unlock(&dev->mode_config.mutex); > > + > > goto out; > > } > > copied++; > > } > > } > > out_resp->count_modes = mode_count; > > -out: > > mutex_unlock(&dev->mode_config.mutex); > > -out_unref: > > + > > + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > > + encoder = drm_connector_get_encoder(connector); > > + if (encoder) > > + out_resp->encoder_id = encoder->base.id; > > + else > > + out_resp->encoder_id = 0; > > + > > + /* Only grab properties after probing, to make sure EDID and other > > + * properties reflect the latest status. */ > > + ret = drm_mode_object_get_properties(&connector->base, file_priv->atomic, > > + (uint32_t __user *)(unsigned long)(out_resp->props_ptr), > > + (uint64_t __user *)(unsigned long)(out_resp->prop_values_ptr), > > + &out_resp->count_props); > > + drm_modeset_unlock(&dev->mode_config.connection_mutex); > > + > > +out: > > drm_connector_put(connector); > > > > return ret; > > -- > > 2.11.0 > > -- > Sean Paul, Software Engineer, Google / Chromium OS
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 5cd61aff7857..8072e6e4c62c 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -1293,21 +1293,6 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, if (!connector) return -ENOENT; - drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); - encoder = drm_connector_get_encoder(connector); - if (encoder) - out_resp->encoder_id = encoder->base.id; - else - out_resp->encoder_id = 0; - - ret = drm_mode_object_get_properties(&connector->base, file_priv->atomic, - (uint32_t __user *)(unsigned long)(out_resp->props_ptr), - (uint64_t __user *)(unsigned long)(out_resp->prop_values_ptr), - &out_resp->count_props); - drm_modeset_unlock(&dev->mode_config.connection_mutex); - if (ret) - goto out_unref; - for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) if (connector->encoder_ids[i] != 0) encoders_count++; @@ -1320,7 +1305,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, if (put_user(connector->encoder_ids[i], encoder_ptr + copied)) { ret = -EFAULT; - goto out_unref; + goto out; } copied++; } @@ -1364,15 +1349,32 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, if (copy_to_user(mode_ptr + copied, &u_mode, sizeof(u_mode))) { ret = -EFAULT; + mutex_unlock(&dev->mode_config.mutex); + goto out; } copied++; } } out_resp->count_modes = mode_count; -out: mutex_unlock(&dev->mode_config.mutex); -out_unref: + + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); + encoder = drm_connector_get_encoder(connector); + if (encoder) + out_resp->encoder_id = encoder->base.id; + else + out_resp->encoder_id = 0; + + /* Only grab properties after probing, to make sure EDID and other + * properties reflect the latest status. */ + ret = drm_mode_object_get_properties(&connector->base, file_priv->atomic, + (uint32_t __user *)(unsigned long)(out_resp->props_ptr), + (uint64_t __user *)(unsigned long)(out_resp->prop_values_ptr), + &out_resp->count_props); + drm_modeset_unlock(&dev->mode_config.connection_mutex); + +out: drm_connector_put(connector); return ret;
In commit 91eefc05f0ac71902906b2058360e61bd25137fe Author: Daniel Vetter <daniel.vetter@ffwll.ch> Date: Wed Dec 14 00:08:10 2016 +0100 drm: Tighten locking in drm_mode_getconnector I reordered the logic a bit in that IOCTL, but that broke userspace since it'll get the new mode list, but not the new property values. Fix that again. v2: Fix up the error path handling when copy_to_user for the modes failes (Dhinakaran). Fixes: 91eefc05f0ac ("drm: Tighten locking in drm_mode_getconnector") Cc: Sean Paul <seanpaul@chromium.org> Cc: Daniel Vetter <daniel.vetter@intel.com> Cc: Jani Nikula <jani.nikula@linux.intel.com> Cc: David Airlie <airlied@linux.ie> Cc: dri-devel@lists.freedesktop.org Reported-by: "H.J. Lu" <hjl.tools@gmail.com> Tested-by: "H.J. Lu" <hjl.tools@gmail.com> Cc: <stable@vger.kernel.org> # v4.11+ Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100576 Cc: "H.J. Lu" <hjl.tools@gmail.com> Cc: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/drm_connector.c | 38 ++++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 18 deletions(-)