* [bug report] drm: Add helpers to kick off self refresh mode in drivers @ 2019-06-19 9:39 Dan Carpenter 2019-06-19 9:41 ` Dan Carpenter 0 siblings, 1 reply; 5+ messages in thread From: Dan Carpenter @ 2019-06-19 9:39 UTC (permalink / raw) To: seanpaul; +Cc: dri-devel Hello Sean Paul, The patch 1452c25b0e60: "drm: Add helpers to kick off self refresh mode in drivers" from Jun 12, 2019, leads to the following static checker warning: drivers/gpu/drm/drm_self_refresh_helper.c:118 drm_self_refresh_helper_entry_work() error: we previously assumed 'state' could be null (see line 77) drivers/gpu/drm/drm_self_refresh_helper.c 60 static void drm_self_refresh_helper_entry_work(struct work_struct *work) 61 { 62 struct drm_self_refresh_data *sr_data = container_of( 63 to_delayed_work(work), 64 struct drm_self_refresh_data, entry_work); 65 struct drm_crtc *crtc = sr_data->crtc; 66 struct drm_device *dev = crtc->dev; 67 struct drm_modeset_acquire_ctx ctx; 68 struct drm_atomic_state *state; 69 struct drm_connector *conn; 70 struct drm_connector_state *conn_state; 71 struct drm_crtc_state *crtc_state; 72 int i, ret; 73 74 drm_modeset_acquire_init(&ctx, 0); 75 76 state = drm_atomic_state_alloc(dev); 77 if (!state) { 78 ret = -ENOMEM; 79 goto out; ^^^^^^^^ The allocation failed. 80 } 81 82 retry: 83 state->acquire_ctx = &ctx; 84 85 crtc_state = drm_atomic_get_crtc_state(state, crtc); 86 if (IS_ERR(crtc_state)) { 87 ret = PTR_ERR(crtc_state); 88 goto out; 89 } 90 91 if (!crtc_state->enable) 92 goto out; 93 94 ret = drm_atomic_add_affected_connectors(state, crtc); 95 if (ret) 96 goto out; 97 98 for_each_new_connector_in_state(state, conn, conn_state, i) { 99 if (!conn_state->self_refresh_aware) 100 goto out; 101 } 102 103 crtc_state->active = false; 104 crtc_state->self_refresh_active = true; 105 106 ret = drm_atomic_commit(state); 107 if (ret) 108 goto out; 109 110 out: 111 if (ret == -EDEADLK) { 112 drm_atomic_state_clear(state); 113 ret = drm_modeset_backoff(&ctx); 114 if (!ret) 115 goto retry; 116 } 117 118 drm_atomic_state_put(state); ^^^^^ NULL dereference. 119 drm_modeset_drop_locks(&ctx); 120 drm_modeset_acquire_fini(&ctx); 121 } regards, dan carpenter _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] drm: Add helpers to kick off self refresh mode in drivers 2019-06-19 9:39 [bug report] drm: Add helpers to kick off self refresh mode in drivers Dan Carpenter @ 2019-06-19 9:41 ` Dan Carpenter 2019-06-19 18:19 ` [PATCH] drm/self_refresh: Fix possible NULL deref in failure path Sean Paul 0 siblings, 1 reply; 5+ messages in thread From: Dan Carpenter @ 2019-06-19 9:41 UTC (permalink / raw) To: seanpaul; +Cc: dri-devel On Wed, Jun 19, 2019 at 12:39:37PM +0300, Dan Carpenter wrote: > 72 int i, ret; > 73 > 74 drm_modeset_acquire_init(&ctx, 0); > 75 > 76 state = drm_atomic_state_alloc(dev); > 77 if (!state) { > 78 ret = -ENOMEM; > 79 goto out; > ^^^^^^^^ > The allocation failed. > > 80 } > 81 > 82 retry: > 83 state->acquire_ctx = &ctx; > 84 > 85 crtc_state = drm_atomic_get_crtc_state(state, crtc); > 86 if (IS_ERR(crtc_state)) { > 87 ret = PTR_ERR(crtc_state); > 88 goto out; > 89 } > 90 > 91 if (!crtc_state->enable) > 92 goto out; Oh... Also we need to set "ret" here. > 93 > 94 ret = drm_atomic_add_affected_connectors(state, crtc); > 95 if (ret) > 96 goto out; > 97 > 98 for_each_new_connector_in_state(state, conn, conn_state, i) { > 99 if (!conn_state->self_refresh_aware) > 100 goto out; > 101 } > 102 > 103 crtc_state->active = false; > 104 crtc_state->self_refresh_active = true; > 105 > 106 ret = drm_atomic_commit(state); > 107 if (ret) > 108 goto out; > 109 > 110 out: > 111 if (ret == -EDEADLK) { > 112 drm_atomic_state_clear(state); > 113 ret = drm_modeset_backoff(&ctx); > 114 if (!ret) > 115 goto retry; > 116 } > 117 > 118 drm_atomic_state_put(state); > ^^^^^ > NULL dereference. > > 119 drm_modeset_drop_locks(&ctx); > 120 drm_modeset_acquire_fini(&ctx); > 121 } regards, dan carpenter _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] drm/self_refresh: Fix possible NULL deref in failure path 2019-06-19 9:41 ` Dan Carpenter @ 2019-06-19 18:19 ` Sean Paul 2019-06-20 11:28 ` Daniel Vetter 0 siblings, 1 reply; 5+ messages in thread From: Sean Paul @ 2019-06-19 18:19 UTC (permalink / raw) To: dri-devel Cc: Zain Wang, Maxime Ripard, Sean Paul, Jose Souza, Tomasz Figa, David Airlie, Sean Paul, Dan Carpenter, Sam Ravnborg From: Sean Paul <seanpaul@chromium.org> If state allocation fails, we still try to give back the reference on it. Also initialize ret in case the crtc is not enabled and we hit the eject button. Fixes: 1452c25b0e60 ("drm: Add helpers to kick off self refresh mode in drivers") Cc: Daniel Vetter <daniel@ffwll.ch> Cc: Jose Souza <jose.souza@intel.com> Cc: Zain Wang <wzz@rock-chips.com> Cc: Tomasz Figa <tfiga@chromium.org> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Sam Ravnborg <sam@ravnborg.org> Cc: Sean Paul <seanpaul@chromium.org> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Maxime Ripard <maxime.ripard@bootlin.com> Cc: Sean Paul <sean@poorly.run> Cc: David Airlie <airlied@linux.ie> Cc: dri-devel@lists.freedesktop.org Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Sean Paul <seanpaul@chromium.org> --- drivers/gpu/drm/drm_self_refresh_helper.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_self_refresh_helper.c b/drivers/gpu/drm/drm_self_refresh_helper.c index e0d2ad1f070cb..4b9424a8f1f1c 100644 --- a/drivers/gpu/drm/drm_self_refresh_helper.c +++ b/drivers/gpu/drm/drm_self_refresh_helper.c @@ -69,14 +69,14 @@ static void drm_self_refresh_helper_entry_work(struct work_struct *work) struct drm_connector *conn; struct drm_connector_state *conn_state; struct drm_crtc_state *crtc_state; - int i, ret; + int i, ret = 0; drm_modeset_acquire_init(&ctx, 0); state = drm_atomic_state_alloc(dev); if (!state) { ret = -ENOMEM; - goto out; + goto out_drop_locks; } retry: @@ -116,6 +116,8 @@ static void drm_self_refresh_helper_entry_work(struct work_struct *work) } drm_atomic_state_put(state); + +out_drop_locks: drm_modeset_drop_locks(&ctx); drm_modeset_acquire_fini(&ctx); } -- Sean Paul, Software Engineer, Google / Chromium OS _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/self_refresh: Fix possible NULL deref in failure path 2019-06-19 18:19 ` [PATCH] drm/self_refresh: Fix possible NULL deref in failure path Sean Paul @ 2019-06-20 11:28 ` Daniel Vetter 2019-06-20 14:47 ` Sean Paul 0 siblings, 1 reply; 5+ messages in thread From: Daniel Vetter @ 2019-06-20 11:28 UTC (permalink / raw) To: Sean Paul Cc: Zain Wang, Maxime Ripard, Jose Souza, Tomasz Figa, David Airlie, Sean Paul, dri-devel, Dan Carpenter, Sam Ravnborg On Wed, Jun 19, 2019 at 02:19:47PM -0400, Sean Paul wrote: > From: Sean Paul <seanpaul@chromium.org> > > If state allocation fails, we still try to give back the reference on > it. Also initialize ret in case the crtc is not enabled and we hit the > eject button. > > Fixes: 1452c25b0e60 ("drm: Add helpers to kick off self refresh mode in drivers") > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: Jose Souza <jose.souza@intel.com> > Cc: Zain Wang <wzz@rock-chips.com> > Cc: Tomasz Figa <tfiga@chromium.org> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Sam Ravnborg <sam@ravnborg.org> > Cc: Sean Paul <seanpaul@chromium.org> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Maxime Ripard <maxime.ripard@bootlin.com> > Cc: Sean Paul <sean@poorly.run> > Cc: David Airlie <airlied@linux.ie> > Cc: dri-devel@lists.freedesktop.org > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > Signed-off-by: Sean Paul <seanpaul@chromium.org> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/drm_self_refresh_helper.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_self_refresh_helper.c b/drivers/gpu/drm/drm_self_refresh_helper.c > index e0d2ad1f070cb..4b9424a8f1f1c 100644 > --- a/drivers/gpu/drm/drm_self_refresh_helper.c > +++ b/drivers/gpu/drm/drm_self_refresh_helper.c > @@ -69,14 +69,14 @@ static void drm_self_refresh_helper_entry_work(struct work_struct *work) > struct drm_connector *conn; > struct drm_connector_state *conn_state; > struct drm_crtc_state *crtc_state; > - int i, ret; > + int i, ret = 0; > > drm_modeset_acquire_init(&ctx, 0); > > state = drm_atomic_state_alloc(dev); > if (!state) { > ret = -ENOMEM; > - goto out; > + goto out_drop_locks; > } > > retry: > @@ -116,6 +116,8 @@ static void drm_self_refresh_helper_entry_work(struct work_struct *work) > } > > drm_atomic_state_put(state); > + > +out_drop_locks: > drm_modeset_drop_locks(&ctx); > drm_modeset_acquire_fini(&ctx); > } > -- > Sean Paul, Software Engineer, Google / Chromium OS > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/self_refresh: Fix possible NULL deref in failure path 2019-06-20 11:28 ` Daniel Vetter @ 2019-06-20 14:47 ` Sean Paul 0 siblings, 0 replies; 5+ messages in thread From: Sean Paul @ 2019-06-20 14:47 UTC (permalink / raw) To: Daniel Vetter Cc: Zain Wang, Maxime Ripard, Sam Ravnborg, Jose Souza, Tomasz Figa, David Airlie, Sean Paul, dri-devel, Dan Carpenter, Sean Paul On Thu, Jun 20, 2019 at 01:28:55PM +0200, Daniel Vetter wrote: > On Wed, Jun 19, 2019 at 02:19:47PM -0400, Sean Paul wrote: > > From: Sean Paul <seanpaul@chromium.org> > > > > If state allocation fails, we still try to give back the reference on > > it. Also initialize ret in case the crtc is not enabled and we hit the > > eject button. > > > > Fixes: 1452c25b0e60 ("drm: Add helpers to kick off self refresh mode in drivers") > > Cc: Daniel Vetter <daniel@ffwll.ch> > > Cc: Jose Souza <jose.souza@intel.com> > > Cc: Zain Wang <wzz@rock-chips.com> > > Cc: Tomasz Figa <tfiga@chromium.org> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Sam Ravnborg <sam@ravnborg.org> > > Cc: Sean Paul <seanpaul@chromium.org> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: Maxime Ripard <maxime.ripard@bootlin.com> > > Cc: Sean Paul <sean@poorly.run> > > Cc: David Airlie <airlied@linux.ie> > > Cc: dri-devel@lists.freedesktop.org > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > > Signed-off-by: Sean Paul <seanpaul@chromium.org> > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Applied to -misc-next, thanks! Sean > > --- > > drivers/gpu/drm/drm_self_refresh_helper.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_self_refresh_helper.c b/drivers/gpu/drm/drm_self_refresh_helper.c > > index e0d2ad1f070cb..4b9424a8f1f1c 100644 > > --- a/drivers/gpu/drm/drm_self_refresh_helper.c > > +++ b/drivers/gpu/drm/drm_self_refresh_helper.c > > @@ -69,14 +69,14 @@ static void drm_self_refresh_helper_entry_work(struct work_struct *work) > > struct drm_connector *conn; > > struct drm_connector_state *conn_state; > > struct drm_crtc_state *crtc_state; > > - int i, ret; > > + int i, ret = 0; > > > > drm_modeset_acquire_init(&ctx, 0); > > > > state = drm_atomic_state_alloc(dev); > > if (!state) { > > ret = -ENOMEM; > > - goto out; > > + goto out_drop_locks; > > } > > > > retry: > > @@ -116,6 +116,8 @@ static void drm_self_refresh_helper_entry_work(struct work_struct *work) > > } > > > > drm_atomic_state_put(state); > > + > > +out_drop_locks: > > drm_modeset_drop_locks(&ctx); > > drm_modeset_acquire_fini(&ctx); > > } > > -- > > Sean Paul, Software Engineer, Google / Chromium OS > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch -- Sean Paul, Software Engineer, Google / Chromium OS _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-06-20 14:47 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-06-19 9:39 [bug report] drm: Add helpers to kick off self refresh mode in drivers Dan Carpenter 2019-06-19 9:41 ` Dan Carpenter 2019-06-19 18:19 ` [PATCH] drm/self_refresh: Fix possible NULL deref in failure path Sean Paul 2019-06-20 11:28 ` Daniel Vetter 2019-06-20 14:47 ` Sean Paul
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).