| Message ID | 20170620091605.3749-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 20 Jun 2017, Daniel Vetter <daniel.vetter@ffwll.ch> 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. > > 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> > Cc: "H.J. Lu" <hjl.tools@gmail.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Possibly Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100576 > --- > drivers/gpu/drm/drm_connector.c | 33 ++++++++++++++++++--------------- > 1 file changed, 18 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > index 5cd61aff7857..e0fd8168a178 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++; > @@ -1372,6 +1357,24 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, > out_resp->count_modes = mode_count; > out: > mutex_unlock(&dev->mode_config.mutex); > + > + 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); > + if (ret) > + goto out_unref; > + > out_unref: > drm_connector_put(connector);
On Tue, Jun 20, 2017 at 2:32 AM, Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Tue, 20 Jun 2017, Daniel Vetter <daniel.vetter@ffwll.ch> 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. >> >> 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> >> Cc: "H.J. Lu" <hjl.tools@gmail.com> >> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > Possibly > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100576 > >> --- >> drivers/gpu/drm/drm_connector.c | 33 ++++++++++++++++++--------------- >> 1 file changed, 18 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c >> index 5cd61aff7857..e0fd8168a178 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++; >> @@ -1372,6 +1357,24 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, >> out_resp->count_modes = mode_count; >> out: >> mutex_unlock(&dev->mode_config.mutex); >> + >> + 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); >> + if (ret) >> + goto out_unref; >> + >> out_unref: >> drm_connector_put(connector); > Yes, it fixed my issue. Thanks.
On Tue, Jun 20, 2017 at 11:16 AM, Daniel Vetter <daniel.vetter@ffwll.ch> 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. > > 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> > Cc: "H.J. Lu" <hjl.tools@gmail.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Some dim fixes failed me, also needs: Cc: <stable@vger.kernel.org> # v4.11+
On Tue, 2017-06-20 at 11:16 +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. > > 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> > Cc: "H.J. Lu" <hjl.tools@gmail.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/drm_connector.c | 33 ++++++++++++++++++--------------- > 1 file changed, 18 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > index 5cd61aff7857..e0fd8168a178 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++; > @@ -1372,6 +1357,24 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, > out_resp->count_modes = mode_count; > out: > mutex_unlock(&dev->mode_config.mutex); > + > + 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); Isn't this overwriting the -EFAULT return in case copy_to_user() failed while copying the modes ? > + drm_modeset_unlock(&dev->mode_config.connection_mutex); > + if (ret) > + goto out_unref; > + > out_unref: > drm_connector_put(connector); >
On Tue, Jun 20, 2017 at 9:32 PM, Pandiyan, Dhinakaran <dhinakaran.pandiyan@intel.com> wrote: > Isn't this overwriting the -EFAULT return in case copy_to_user() failed > while copying the modes ? Indeed, thanks for spotting this. New patch in-flight. -Daniel
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 5cd61aff7857..e0fd8168a178 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++; @@ -1372,6 +1357,24 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, out_resp->count_modes = mode_count; out: mutex_unlock(&dev->mode_config.mutex); + + 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); + if (ret) + goto out_unref; + out_unref: drm_connector_put(connector);
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. 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> Cc: "H.J. Lu" <hjl.tools@gmail.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/drm_connector.c | 33 ++++++++++++++++++--------------- 1 file changed, 18 insertions(+), 15 deletions(-)