* [Intel-gfx] [PATCH] drm/i915: Fix NULL ptr deref by checking new_crtc_state @ 2023-05-05 8:22 Stanislav Lisovskiy 2023-05-05 9:11 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork ` (3 more replies) 0 siblings, 4 replies; 30+ messages in thread From: Stanislav Lisovskiy @ 2023-05-05 8:22 UTC (permalink / raw) To: intel-gfx intel_atomic_get_new_crtc_state can return NULL, unless crtc state wasn't obtained previously with intel_atomic_get_crtc_state, so we must check it for NULLness here, just as in many other places, where we can't guarantee that intel_atomic_get_crtc_state was called. We are currently getting NULL ptr deref because of that, so this fix was confirmed to help. Fixes: 74a75dc90869 ("drm/i915/display: move plane prepare/cleanup to intel_atomic_plane.c") Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> --- drivers/gpu/drm/i915/display/intel_atomic_plane.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c index 9f670dcfe76e..4125ee07a271 100644 --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c @@ -1029,7 +1029,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, int ret; if (old_obj) { - const struct intel_crtc_state *crtc_state = + const struct intel_crtc_state *new_crtc_state = intel_atomic_get_new_crtc_state(state, to_intel_crtc(old_plane_state->hw.crtc)); @@ -1044,7 +1044,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, * This should only fail upon a hung GPU, in which case we * can safely continue. */ - if (intel_crtc_needs_modeset(crtc_state)) { + if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state)) { ret = i915_sw_fence_await_reservation(&state->commit_ready, old_obj->base.resv, false, 0, -- 2.37.3 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915: Fix NULL ptr deref by checking new_crtc_state 2023-05-05 8:22 [Intel-gfx] [PATCH] drm/i915: Fix NULL ptr deref by checking new_crtc_state Stanislav Lisovskiy @ 2023-05-05 9:11 ` Patchwork 2023-05-05 9:29 ` [Intel-gfx] [PATCH] " Andrzej Hajda ` (2 subsequent siblings) 3 siblings, 0 replies; 30+ messages in thread From: Patchwork @ 2023-05-05 9:11 UTC (permalink / raw) To: Stanislav Lisovskiy; +Cc: intel-gfx [-- Attachment #1: Type: text/plain, Size: 3837 bytes --] == Series Details == Series: drm/i915: Fix NULL ptr deref by checking new_crtc_state URL : https://patchwork.freedesktop.org/series/117369/ State : success == Summary == CI Bug Log - changes from CI_DRM_13110 -> Patchwork_117369v1 ==================================================== Summary ------- **SUCCESS** No regressions found. External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117369v1/index.html Participating hosts (40 -> 39) ------------------------------ Missing (1): fi-snb-2520m Known issues ------------ Here are the changes found in Patchwork_117369v1 that come from known issues: ### IGT changes ### #### Issues hit #### * igt@i915_selftest@live@reset: - bat-rpls-1: [PASS][1] -> [ABORT][2] ([i915#4983] / [i915#7461] / [i915#8347] / [i915#8384]) [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13110/bat-rpls-1/igt@i915_selftest@live@reset.html [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117369v1/bat-rpls-1/igt@i915_selftest@live@reset.html - bat-rpls-2: [PASS][3] -> [ABORT][4] ([i915#4983] / [i915#7461] / [i915#7913] / [i915#8347]) [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13110/bat-rpls-2/igt@i915_selftest@live@reset.html [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117369v1/bat-rpls-2/igt@i915_selftest@live@reset.html #### Possible fixes #### * igt@i915_selftest@live@requests: - {bat-mtlp-6}: [ABORT][5] ([i915#4983] / [i915#7920]) -> [PASS][6] [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13110/bat-mtlp-6/igt@i915_selftest@live@requests.html [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117369v1/bat-mtlp-6/igt@i915_selftest@live@requests.html * igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence@pipe-d-dp-1: - bat-dg2-8: [FAIL][7] ([i915#7932]) -> [PASS][8] +1 similar issue [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13110/bat-dg2-8/igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence@pipe-d-dp-1.html [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117369v1/bat-dg2-8/igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence@pipe-d-dp-1.html {name}: This element is suppressed. This means it is ignored when computing the status of the difference (SUCCESS, WARNING, or FAILURE). [i915#1845]: https://gitlab.freedesktop.org/drm/intel/issues/1845 [i915#4078]: https://gitlab.freedesktop.org/drm/intel/issues/4078 [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983 [i915#6367]: https://gitlab.freedesktop.org/drm/intel/issues/6367 [i915#6645]: https://gitlab.freedesktop.org/drm/intel/issues/6645 [i915#7461]: https://gitlab.freedesktop.org/drm/intel/issues/7461 [i915#7699]: https://gitlab.freedesktop.org/drm/intel/issues/7699 [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828 [i915#7913]: https://gitlab.freedesktop.org/drm/intel/issues/7913 [i915#7920]: https://gitlab.freedesktop.org/drm/intel/issues/7920 [i915#7932]: https://gitlab.freedesktop.org/drm/intel/issues/7932 [i915#8347]: https://gitlab.freedesktop.org/drm/intel/issues/8347 [i915#8384]: https://gitlab.freedesktop.org/drm/intel/issues/8384 Build changes ------------- * Linux: CI_DRM_13110 -> Patchwork_117369v1 CI-20190529: 20190529 CI_DRM_13110: 0fc2261e97d6fe498f17bcd9120fed98778ff0d8 @ git://anongit.freedesktop.org/gfx-ci/linux IGT_7277: 1cb3507f3ff28d11bd5cfabcde576fe78ddab571 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git Patchwork_117369v1: 0fc2261e97d6fe498f17bcd9120fed98778ff0d8 @ git://anongit.freedesktop.org/gfx-ci/linux ### Linux commits 6ecb4c73487a drm/i915: Fix NULL ptr deref by checking new_crtc_state == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117369v1/index.html [-- Attachment #2: Type: text/html, Size: 4381 bytes --] ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix NULL ptr deref by checking new_crtc_state 2023-05-05 8:22 [Intel-gfx] [PATCH] drm/i915: Fix NULL ptr deref by checking new_crtc_state Stanislav Lisovskiy 2023-05-05 9:11 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork @ 2023-05-05 9:29 ` Andrzej Hajda 2023-05-05 10:28 ` Lisovskiy, Stanislav 2023-05-05 10:54 ` Ville Syrjälä 2023-05-06 0:38 ` [Intel-gfx] ✓ Fi.CI.IGT: success for " Patchwork 3 siblings, 1 reply; 30+ messages in thread From: Andrzej Hajda @ 2023-05-05 9:29 UTC (permalink / raw) To: Stanislav Lisovskiy, intel-gfx On 05.05.2023 10:22, Stanislav Lisovskiy wrote: > intel_atomic_get_new_crtc_state can return NULL, unless crtc state wasn't > obtained previously with intel_atomic_get_crtc_state, so we must check it > for NULLness here, just as in many other places, where we can't guarantee > that intel_atomic_get_crtc_state was called. > We are currently getting NULL ptr deref because of that, so this fix was > confirmed to help. > > Fixes: 74a75dc90869 ("drm/i915/display: move plane prepare/cleanup to intel_atomic_plane.c") > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> I wonder if it wouldn't be safer to move the check to intel_crtc_needs_modeset, up to you. Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com> Regards Andrzej > --- > drivers/gpu/drm/i915/display/intel_atomic_plane.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > index 9f670dcfe76e..4125ee07a271 100644 > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > @@ -1029,7 +1029,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > int ret; > > if (old_obj) { > - const struct intel_crtc_state *crtc_state = > + const struct intel_crtc_state *new_crtc_state = > intel_atomic_get_new_crtc_state(state, > to_intel_crtc(old_plane_state->hw.crtc)); > > @@ -1044,7 +1044,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > * This should only fail upon a hung GPU, in which case we > * can safely continue. > */ > - if (intel_crtc_needs_modeset(crtc_state)) { > + if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state)) { > ret = i915_sw_fence_await_reservation(&state->commit_ready, > old_obj->base.resv, > false, 0, ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix NULL ptr deref by checking new_crtc_state 2023-05-05 9:29 ` [Intel-gfx] [PATCH] " Andrzej Hajda @ 2023-05-05 10:28 ` Lisovskiy, Stanislav 0 siblings, 0 replies; 30+ messages in thread From: Lisovskiy, Stanislav @ 2023-05-05 10:28 UTC (permalink / raw) To: Andrzej Hajda; +Cc: intel-gfx On Fri, May 05, 2023 at 11:29:22AM +0200, Andrzej Hajda wrote: > On 05.05.2023 10:22, Stanislav Lisovskiy wrote: > > intel_atomic_get_new_crtc_state can return NULL, unless crtc state wasn't > > obtained previously with intel_atomic_get_crtc_state, so we must check it > > for NULLness here, just as in many other places, where we can't guarantee > > that intel_atomic_get_crtc_state was called. > > We are currently getting NULL ptr deref because of that, so this fix was > > confirmed to help. > > > > Fixes: 74a75dc90869 ("drm/i915/display: move plane prepare/cleanup to intel_atomic_plane.c") > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > I wonder if it wouldn't be safer to move the check to > intel_crtc_needs_modeset, up to you. > > Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com> Hi Andrzej, Thank you for the review, I would probably still prefer to leave that check to the calling site, because depending on new_crtc_state availability calling site might need to do different actions, so it anyway has to be dealt with. Stan > > Regards > Andrzej > > > --- > > drivers/gpu/drm/i915/display/intel_atomic_plane.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > index 9f670dcfe76e..4125ee07a271 100644 > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > @@ -1029,7 +1029,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > int ret; > > if (old_obj) { > > - const struct intel_crtc_state *crtc_state = > > + const struct intel_crtc_state *new_crtc_state = > > intel_atomic_get_new_crtc_state(state, > > to_intel_crtc(old_plane_state->hw.crtc)); > > @@ -1044,7 +1044,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > * This should only fail upon a hung GPU, in which case we > > * can safely continue. > > */ > > - if (intel_crtc_needs_modeset(crtc_state)) { > > + if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state)) { > > ret = i915_sw_fence_await_reservation(&state->commit_ready, > > old_obj->base.resv, > > false, 0, > ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix NULL ptr deref by checking new_crtc_state 2023-05-05 8:22 [Intel-gfx] [PATCH] drm/i915: Fix NULL ptr deref by checking new_crtc_state Stanislav Lisovskiy 2023-05-05 9:11 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork 2023-05-05 9:29 ` [Intel-gfx] [PATCH] " Andrzej Hajda @ 2023-05-05 10:54 ` Ville Syrjälä 2023-05-05 10:58 ` Lisovskiy, Stanislav 2023-05-06 0:38 ` [Intel-gfx] ✓ Fi.CI.IGT: success for " Patchwork 3 siblings, 1 reply; 30+ messages in thread From: Ville Syrjälä @ 2023-05-05 10:54 UTC (permalink / raw) To: Stanislav Lisovskiy; +Cc: intel-gfx On Fri, May 05, 2023 at 11:22:12AM +0300, Stanislav Lisovskiy wrote: > intel_atomic_get_new_crtc_state can return NULL, unless crtc state wasn't > obtained previously with intel_atomic_get_crtc_state, so we must check it > for NULLness here, just as in many other places, where we can't guarantee > that intel_atomic_get_crtc_state was called. > We are currently getting NULL ptr deref because of that, so this fix was > confirmed to help. > > Fixes: 74a75dc90869 ("drm/i915/display: move plane prepare/cleanup to intel_atomic_plane.c") > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > --- > drivers/gpu/drm/i915/display/intel_atomic_plane.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > index 9f670dcfe76e..4125ee07a271 100644 > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > @@ -1029,7 +1029,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > int ret; > > if (old_obj) { > - const struct intel_crtc_state *crtc_state = > + const struct intel_crtc_state *new_crtc_state = > intel_atomic_get_new_crtc_state(state, > to_intel_crtc(old_plane_state->hw.crtc)); > > @@ -1044,7 +1044,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > * This should only fail upon a hung GPU, in which case we > * can safely continue. > */ > - if (intel_crtc_needs_modeset(crtc_state)) { > + if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state)) { NAK. We need to fix the bug instead of paparing over it. > ret = i915_sw_fence_await_reservation(&state->commit_ready, > old_obj->base.resv, > false, 0, > -- > 2.37.3 -- Ville Syrjälä Intel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix NULL ptr deref by checking new_crtc_state 2023-05-05 10:54 ` Ville Syrjälä @ 2023-05-05 10:58 ` Lisovskiy, Stanislav 2023-05-05 11:02 ` Ville Syrjälä 0 siblings, 1 reply; 30+ messages in thread From: Lisovskiy, Stanislav @ 2023-05-05 10:58 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx On Fri, May 05, 2023 at 01:54:14PM +0300, Ville Syrjälä wrote: > On Fri, May 05, 2023 at 11:22:12AM +0300, Stanislav Lisovskiy wrote: > > intel_atomic_get_new_crtc_state can return NULL, unless crtc state wasn't > > obtained previously with intel_atomic_get_crtc_state, so we must check it > > for NULLness here, just as in many other places, where we can't guarantee > > that intel_atomic_get_crtc_state was called. > > We are currently getting NULL ptr deref because of that, so this fix was > > confirmed to help. > > > > Fixes: 74a75dc90869 ("drm/i915/display: move plane prepare/cleanup to intel_atomic_plane.c") > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_atomic_plane.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > index 9f670dcfe76e..4125ee07a271 100644 > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > @@ -1029,7 +1029,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > int ret; > > > > if (old_obj) { > > - const struct intel_crtc_state *crtc_state = > > + const struct intel_crtc_state *new_crtc_state = > > intel_atomic_get_new_crtc_state(state, > > to_intel_crtc(old_plane_state->hw.crtc)); > > > > @@ -1044,7 +1044,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > * This should only fail upon a hung GPU, in which case we > > * can safely continue. > > */ > > - if (intel_crtc_needs_modeset(crtc_state)) { > > + if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state)) { > > NAK. We need to fix the bug instead of paparing over it. I had pushed this already. Moreover as I understand we need to check that new_crtc_state for being NULL anyway. We do check it for being NULL in other places. But if you have another solution - go for it. Stan > > > ret = i915_sw_fence_await_reservation(&state->commit_ready, > > old_obj->base.resv, > > false, 0, > > -- > > 2.37.3 > > -- > Ville Syrjälä > Intel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix NULL ptr deref by checking new_crtc_state 2023-05-05 10:58 ` Lisovskiy, Stanislav @ 2023-05-05 11:02 ` Ville Syrjälä 2023-05-05 11:05 ` Lisovskiy, Stanislav 0 siblings, 1 reply; 30+ messages in thread From: Ville Syrjälä @ 2023-05-05 11:02 UTC (permalink / raw) To: Lisovskiy, Stanislav; +Cc: intel-gfx On Fri, May 05, 2023 at 01:58:03PM +0300, Lisovskiy, Stanislav wrote: > On Fri, May 05, 2023 at 01:54:14PM +0300, Ville Syrjälä wrote: > > On Fri, May 05, 2023 at 11:22:12AM +0300, Stanislav Lisovskiy wrote: > > > intel_atomic_get_new_crtc_state can return NULL, unless crtc state wasn't > > > obtained previously with intel_atomic_get_crtc_state, so we must check it > > > for NULLness here, just as in many other places, where we can't guarantee > > > that intel_atomic_get_crtc_state was called. > > > We are currently getting NULL ptr deref because of that, so this fix was > > > confirmed to help. > > > > > > Fixes: 74a75dc90869 ("drm/i915/display: move plane prepare/cleanup to intel_atomic_plane.c") > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_atomic_plane.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > index 9f670dcfe76e..4125ee07a271 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > @@ -1029,7 +1029,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > int ret; > > > > > > if (old_obj) { > > > - const struct intel_crtc_state *crtc_state = > > > + const struct intel_crtc_state *new_crtc_state = > > > intel_atomic_get_new_crtc_state(state, > > > to_intel_crtc(old_plane_state->hw.crtc)); > > > > > > @@ -1044,7 +1044,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > * This should only fail upon a hung GPU, in which case we > > > * can safely continue. > > > */ > > > - if (intel_crtc_needs_modeset(crtc_state)) { > > > + if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state)) { > > > > NAK. We need to fix the bug instead of paparing over it. > > I had pushed this already. It didn't even finish CI. Please revert. > Moreover as I understand we need to check that new_crtc_state > for being NULL anyway. We do check it for being NULL in other places. > But if you have another solution - go for it. > > Stan > > > > > > > ret = i915_sw_fence_await_reservation(&state->commit_ready, > > > old_obj->base.resv, > > > false, 0, > > > -- > > > 2.37.3 > > > > -- > > Ville Syrjälä > > Intel -- Ville Syrjälä Intel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix NULL ptr deref by checking new_crtc_state 2023-05-05 11:02 ` Ville Syrjälä @ 2023-05-05 11:05 ` Lisovskiy, Stanislav 2023-05-05 11:06 ` Ville Syrjälä 0 siblings, 1 reply; 30+ messages in thread From: Lisovskiy, Stanislav @ 2023-05-05 11:05 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx On Fri, May 05, 2023 at 02:02:43PM +0300, Ville Syrjälä wrote: > On Fri, May 05, 2023 at 01:58:03PM +0300, Lisovskiy, Stanislav wrote: > > On Fri, May 05, 2023 at 01:54:14PM +0300, Ville Syrjälä wrote: > > > On Fri, May 05, 2023 at 11:22:12AM +0300, Stanislav Lisovskiy wrote: > > > > intel_atomic_get_new_crtc_state can return NULL, unless crtc state wasn't > > > > obtained previously with intel_atomic_get_crtc_state, so we must check it > > > > for NULLness here, just as in many other places, where we can't guarantee > > > > that intel_atomic_get_crtc_state was called. > > > > We are currently getting NULL ptr deref because of that, so this fix was > > > > confirmed to help. > > > > > > > > Fixes: 74a75dc90869 ("drm/i915/display: move plane prepare/cleanup to intel_atomic_plane.c") > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/display/intel_atomic_plane.c | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > index 9f670dcfe76e..4125ee07a271 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > @@ -1029,7 +1029,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > int ret; > > > > > > > > if (old_obj) { > > > > - const struct intel_crtc_state *crtc_state = > > > > + const struct intel_crtc_state *new_crtc_state = > > > > intel_atomic_get_new_crtc_state(state, > > > > to_intel_crtc(old_plane_state->hw.crtc)); > > > > > > > > @@ -1044,7 +1044,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > * This should only fail upon a hung GPU, in which case we > > > > * can safely continue. > > > > */ > > > > - if (intel_crtc_needs_modeset(crtc_state)) { > > > > + if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state)) { > > > > > > NAK. We need to fix the bug instead of paparing over it. > > > > I had pushed this already. > > It didn't even finish CI. Please revert. Swati did run CI and verified that fix helps. I'm _not_ going to revert. > > > Moreover as I understand we need to check that new_crtc_state > > for being NULL anyway. We do check it for being NULL in other places. > > But if you have another solution - go for it. > > > > Stan > > > > > > > > > > > ret = i915_sw_fence_await_reservation(&state->commit_ready, > > > > old_obj->base.resv, > > > > false, 0, > > > > -- > > > > 2.37.3 > > > > > > -- > > > Ville Syrjälä > > > Intel > > -- > Ville Syrjälä > Intel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix NULL ptr deref by checking new_crtc_state 2023-05-05 11:05 ` Lisovskiy, Stanislav @ 2023-05-05 11:06 ` Ville Syrjälä 2023-05-05 11:08 ` Lisovskiy, Stanislav 2023-05-05 11:20 ` Lisovskiy, Stanislav 0 siblings, 2 replies; 30+ messages in thread From: Ville Syrjälä @ 2023-05-05 11:06 UTC (permalink / raw) To: Lisovskiy, Stanislav; +Cc: intel-gfx On Fri, May 05, 2023 at 02:05:27PM +0300, Lisovskiy, Stanislav wrote: > On Fri, May 05, 2023 at 02:02:43PM +0300, Ville Syrjälä wrote: > > On Fri, May 05, 2023 at 01:58:03PM +0300, Lisovskiy, Stanislav wrote: > > > On Fri, May 05, 2023 at 01:54:14PM +0300, Ville Syrjälä wrote: > > > > On Fri, May 05, 2023 at 11:22:12AM +0300, Stanislav Lisovskiy wrote: > > > > > intel_atomic_get_new_crtc_state can return NULL, unless crtc state wasn't > > > > > obtained previously with intel_atomic_get_crtc_state, so we must check it > > > > > for NULLness here, just as in many other places, where we can't guarantee > > > > > that intel_atomic_get_crtc_state was called. > > > > > We are currently getting NULL ptr deref because of that, so this fix was > > > > > confirmed to help. > > > > > > > > > > Fixes: 74a75dc90869 ("drm/i915/display: move plane prepare/cleanup to intel_atomic_plane.c") > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/display/intel_atomic_plane.c | 4 ++-- > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > index 9f670dcfe76e..4125ee07a271 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > @@ -1029,7 +1029,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > > int ret; > > > > > > > > > > if (old_obj) { > > > > > - const struct intel_crtc_state *crtc_state = > > > > > + const struct intel_crtc_state *new_crtc_state = > > > > > intel_atomic_get_new_crtc_state(state, > > > > > to_intel_crtc(old_plane_state->hw.crtc)); > > > > > > > > > > @@ -1044,7 +1044,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > > * This should only fail upon a hung GPU, in which case we > > > > > * can safely continue. > > > > > */ > > > > > - if (intel_crtc_needs_modeset(crtc_state)) { > > > > > + if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state)) { > > > > > > > > NAK. We need to fix the bug instead of paparing over it. > > > > > > I had pushed this already. > > > > It didn't even finish CI. Please revert. > > Swati did run CI and verified that fix helps. I'm _not_ going to revert. Fine. I'll do it. > > > > > > Moreover as I understand we need to check that new_crtc_state > > > for being NULL anyway. We do check it for being NULL in other places. > > > But if you have another solution - go for it. > > > > > > Stan > > > > > > > > > > > > > > > ret = i915_sw_fence_await_reservation(&state->commit_ready, > > > > > old_obj->base.resv, > > > > > false, 0, > > > > > -- > > > > > 2.37.3 > > > > > > > > -- > > > > Ville Syrjälä > > > > Intel > > > > -- > > Ville Syrjälä > > Intel -- Ville Syrjälä Intel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix NULL ptr deref by checking new_crtc_state 2023-05-05 11:06 ` Ville Syrjälä @ 2023-05-05 11:08 ` Lisovskiy, Stanislav 2023-05-05 11:20 ` Lisovskiy, Stanislav 1 sibling, 0 replies; 30+ messages in thread From: Lisovskiy, Stanislav @ 2023-05-05 11:08 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx On Fri, May 05, 2023 at 02:06:34PM +0300, Ville Syrjälä wrote: > On Fri, May 05, 2023 at 02:05:27PM +0300, Lisovskiy, Stanislav wrote: > > On Fri, May 05, 2023 at 02:02:43PM +0300, Ville Syrjälä wrote: > > > On Fri, May 05, 2023 at 01:58:03PM +0300, Lisovskiy, Stanislav wrote: > > > > On Fri, May 05, 2023 at 01:54:14PM +0300, Ville Syrjälä wrote: > > > > > On Fri, May 05, 2023 at 11:22:12AM +0300, Stanislav Lisovskiy wrote: > > > > > > intel_atomic_get_new_crtc_state can return NULL, unless crtc state wasn't > > > > > > obtained previously with intel_atomic_get_crtc_state, so we must check it > > > > > > for NULLness here, just as in many other places, where we can't guarantee > > > > > > that intel_atomic_get_crtc_state was called. > > > > > > We are currently getting NULL ptr deref because of that, so this fix was > > > > > > confirmed to help. > > > > > > > > > > > > Fixes: 74a75dc90869 ("drm/i915/display: move plane prepare/cleanup to intel_atomic_plane.c") > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > > --- > > > > > > drivers/gpu/drm/i915/display/intel_atomic_plane.c | 4 ++-- > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > index 9f670dcfe76e..4125ee07a271 100644 > > > > > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > @@ -1029,7 +1029,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > > > int ret; > > > > > > > > > > > > if (old_obj) { > > > > > > - const struct intel_crtc_state *crtc_state = > > > > > > + const struct intel_crtc_state *new_crtc_state = > > > > > > intel_atomic_get_new_crtc_state(state, > > > > > > to_intel_crtc(old_plane_state->hw.crtc)); > > > > > > > > > > > > @@ -1044,7 +1044,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > > > * This should only fail upon a hung GPU, in which case we > > > > > > * can safely continue. > > > > > > */ > > > > > > - if (intel_crtc_needs_modeset(crtc_state)) { > > > > > > + if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state)) { > > > > > > > > > > NAK. We need to fix the bug instead of paparing over it. > > > > > > > > I had pushed this already. > > > > > > It didn't even finish CI. Please revert. > > > > Swati did run CI and verified that fix helps. I'm _not_ going to revert. > > Fine. I'll do it. Sure. > > > > > > > > > > Moreover as I understand we need to check that new_crtc_state > > > > for being NULL anyway. We do check it for being NULL in other places. > > > > But if you have another solution - go for it. > > > > > > > > Stan > > > > > > > > > > > > > > > > > > > ret = i915_sw_fence_await_reservation(&state->commit_ready, > > > > > > old_obj->base.resv, > > > > > > false, 0, > > > > > > -- > > > > > > 2.37.3 > > > > > > > > > > -- > > > > > Ville Syrjälä > > > > > Intel > > > > > > -- > > > Ville Syrjälä > > > Intel > > -- > Ville Syrjälä > Intel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix NULL ptr deref by checking new_crtc_state 2023-05-05 11:06 ` Ville Syrjälä 2023-05-05 11:08 ` Lisovskiy, Stanislav @ 2023-05-05 11:20 ` Lisovskiy, Stanislav 2023-05-05 11:25 ` Ville Syrjälä 1 sibling, 1 reply; 30+ messages in thread From: Lisovskiy, Stanislav @ 2023-05-05 11:20 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx On Fri, May 05, 2023 at 02:06:34PM +0300, Ville Syrjälä wrote: > On Fri, May 05, 2023 at 02:05:27PM +0300, Lisovskiy, Stanislav wrote: > > On Fri, May 05, 2023 at 02:02:43PM +0300, Ville Syrjälä wrote: > > > On Fri, May 05, 2023 at 01:58:03PM +0300, Lisovskiy, Stanislav wrote: > > > > On Fri, May 05, 2023 at 01:54:14PM +0300, Ville Syrjälä wrote: > > > > > On Fri, May 05, 2023 at 11:22:12AM +0300, Stanislav Lisovskiy wrote: > > > > > > intel_atomic_get_new_crtc_state can return NULL, unless crtc state wasn't > > > > > > obtained previously with intel_atomic_get_crtc_state, so we must check it > > > > > > for NULLness here, just as in many other places, where we can't guarantee > > > > > > that intel_atomic_get_crtc_state was called. > > > > > > We are currently getting NULL ptr deref because of that, so this fix was > > > > > > confirmed to help. > > > > > > > > > > > > Fixes: 74a75dc90869 ("drm/i915/display: move plane prepare/cleanup to intel_atomic_plane.c") > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > > --- > > > > > > drivers/gpu/drm/i915/display/intel_atomic_plane.c | 4 ++-- > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > index 9f670dcfe76e..4125ee07a271 100644 > > > > > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > @@ -1029,7 +1029,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > > > int ret; > > > > > > > > > > > > if (old_obj) { > > > > > > - const struct intel_crtc_state *crtc_state = > > > > > > + const struct intel_crtc_state *new_crtc_state = > > > > > > intel_atomic_get_new_crtc_state(state, > > > > > > to_intel_crtc(old_plane_state->hw.crtc)); > > > > > > > > > > > > @@ -1044,7 +1044,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > > > * This should only fail upon a hung GPU, in which case we > > > > > > * can safely continue. > > > > > > */ > > > > > > - if (intel_crtc_needs_modeset(crtc_state)) { > > > > > > + if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state)) { > > > > > > > > > > NAK. We need to fix the bug instead of paparing over it. > > > > > > > > I had pushed this already. > > > > > > It didn't even finish CI. Please revert. > > > > Swati did run CI and verified that fix helps. I'm _not_ going to revert. > > Fine. I'll do it. Problem is that you don't even care to explain, why this fix is wrong, but simply act in authoritarian way, instead of having constructive discussion. I told that we had verified the fix and that we always do those checks in many places anyway where we get new_crtc_state. However there were no even reasons to reject mentioned here. I don't really think that bringing personality traits and authoritarian discussion style is a professional behaviour. Thanks for cooperation. > > > > > > > > > > Moreover as I understand we need to check that new_crtc_state > > > > for being NULL anyway. We do check it for being NULL in other places. > > > > But if you have another solution - go for it. > > > > > > > > Stan > > > > > > > > > > > > > > > > > > > ret = i915_sw_fence_await_reservation(&state->commit_ready, > > > > > > old_obj->base.resv, > > > > > > false, 0, > > > > > > -- > > > > > > 2.37.3 > > > > > > > > > > -- > > > > > Ville Syrjälä > > > > > Intel > > > > > > -- > > > Ville Syrjälä > > > Intel > > -- > Ville Syrjälä > Intel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix NULL ptr deref by checking new_crtc_state 2023-05-05 11:20 ` Lisovskiy, Stanislav @ 2023-05-05 11:25 ` Ville Syrjälä 2023-05-05 11:41 ` Lisovskiy, Stanislav 0 siblings, 1 reply; 30+ messages in thread From: Ville Syrjälä @ 2023-05-05 11:25 UTC (permalink / raw) To: Lisovskiy, Stanislav; +Cc: intel-gfx On Fri, May 05, 2023 at 02:20:17PM +0300, Lisovskiy, Stanislav wrote: > On Fri, May 05, 2023 at 02:06:34PM +0300, Ville Syrjälä wrote: > > On Fri, May 05, 2023 at 02:05:27PM +0300, Lisovskiy, Stanislav wrote: > > > On Fri, May 05, 2023 at 02:02:43PM +0300, Ville Syrjälä wrote: > > > > On Fri, May 05, 2023 at 01:58:03PM +0300, Lisovskiy, Stanislav wrote: > > > > > On Fri, May 05, 2023 at 01:54:14PM +0300, Ville Syrjälä wrote: > > > > > > On Fri, May 05, 2023 at 11:22:12AM +0300, Stanislav Lisovskiy wrote: > > > > > > > intel_atomic_get_new_crtc_state can return NULL, unless crtc state wasn't > > > > > > > obtained previously with intel_atomic_get_crtc_state, so we must check it > > > > > > > for NULLness here, just as in many other places, where we can't guarantee > > > > > > > that intel_atomic_get_crtc_state was called. > > > > > > > We are currently getting NULL ptr deref because of that, so this fix was > > > > > > > confirmed to help. > > > > > > > > > > > > > > Fixes: 74a75dc90869 ("drm/i915/display: move plane prepare/cleanup to intel_atomic_plane.c") > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > > > --- > > > > > > > drivers/gpu/drm/i915/display/intel_atomic_plane.c | 4 ++-- > > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > index 9f670dcfe76e..4125ee07a271 100644 > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > @@ -1029,7 +1029,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > > > > int ret; > > > > > > > > > > > > > > if (old_obj) { > > > > > > > - const struct intel_crtc_state *crtc_state = > > > > > > > + const struct intel_crtc_state *new_crtc_state = > > > > > > > intel_atomic_get_new_crtc_state(state, > > > > > > > to_intel_crtc(old_plane_state->hw.crtc)); > > > > > > > > > > > > > > @@ -1044,7 +1044,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > > > > * This should only fail upon a hung GPU, in which case we > > > > > > > * can safely continue. > > > > > > > */ > > > > > > > - if (intel_crtc_needs_modeset(crtc_state)) { > > > > > > > + if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state)) { > > > > > > > > > > > > NAK. We need to fix the bug instead of paparing over it. > > > > > > > > > > I had pushed this already. > > > > > > > > It didn't even finish CI. Please revert. > > > > > > Swati did run CI and verified that fix helps. I'm _not_ going to revert. > > > > Fine. I'll do it. > > Problem is that you don't even care to explain, why this fix is wrong, but simply > act in authoritarian way, instead of having constructive discussion. I've explanined this one about a hundred times. The NULL pointer should not happen. Someone needs to actually analyze what is happening instead of just adding randomg NULL checks all over the place. > I told that we had verified the fix and that we always do those checks in > many places anyway where we get new_crtc_state. > However there were no even reasons to reject mentioned here. > I don't really think that bringing personality traits and authoritarian > discussion style is a professional behaviour. > Thanks for cooperation. > > > > > > > > > > > > > > > Moreover as I understand we need to check that new_crtc_state > > > > > for being NULL anyway. We do check it for being NULL in other places. > > > > > But if you have another solution - go for it. > > > > > > > > > > Stan > > > > > > > > > > > > > > > > > > > > > > > ret = i915_sw_fence_await_reservation(&state->commit_ready, > > > > > > > old_obj->base.resv, > > > > > > > false, 0, > > > > > > > -- > > > > > > > 2.37.3 > > > > > > > > > > > > -- > > > > > > Ville Syrjälä > > > > > > Intel > > > > > > > > -- > > > > Ville Syrjälä > > > > Intel > > > > -- > > Ville Syrjälä > > Intel -- Ville Syrjälä Intel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix NULL ptr deref by checking new_crtc_state 2023-05-05 11:25 ` Ville Syrjälä @ 2023-05-05 11:41 ` Lisovskiy, Stanislav 2023-05-05 12:09 ` Ville Syrjälä 0 siblings, 1 reply; 30+ messages in thread From: Lisovskiy, Stanislav @ 2023-05-05 11:41 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx On Fri, May 05, 2023 at 02:25:46PM +0300, Ville Syrjälä wrote: > On Fri, May 05, 2023 at 02:20:17PM +0300, Lisovskiy, Stanislav wrote: > > On Fri, May 05, 2023 at 02:06:34PM +0300, Ville Syrjälä wrote: > > > On Fri, May 05, 2023 at 02:05:27PM +0300, Lisovskiy, Stanislav wrote: > > > > On Fri, May 05, 2023 at 02:02:43PM +0300, Ville Syrjälä wrote: > > > > > On Fri, May 05, 2023 at 01:58:03PM +0300, Lisovskiy, Stanislav wrote: > > > > > > On Fri, May 05, 2023 at 01:54:14PM +0300, Ville Syrjälä wrote: > > > > > > > On Fri, May 05, 2023 at 11:22:12AM +0300, Stanislav Lisovskiy wrote: > > > > > > > > intel_atomic_get_new_crtc_state can return NULL, unless crtc state wasn't > > > > > > > > obtained previously with intel_atomic_get_crtc_state, so we must check it > > > > > > > > for NULLness here, just as in many other places, where we can't guarantee > > > > > > > > that intel_atomic_get_crtc_state was called. > > > > > > > > We are currently getting NULL ptr deref because of that, so this fix was > > > > > > > > confirmed to help. > > > > > > > > > > > > > > > > Fixes: 74a75dc90869 ("drm/i915/display: move plane prepare/cleanup to intel_atomic_plane.c") > > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > > > > --- > > > > > > > > drivers/gpu/drm/i915/display/intel_atomic_plane.c | 4 ++-- > > > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > index 9f670dcfe76e..4125ee07a271 100644 > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > @@ -1029,7 +1029,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > > > > > int ret; > > > > > > > > > > > > > > > > if (old_obj) { > > > > > > > > - const struct intel_crtc_state *crtc_state = > > > > > > > > + const struct intel_crtc_state *new_crtc_state = > > > > > > > > intel_atomic_get_new_crtc_state(state, > > > > > > > > to_intel_crtc(old_plane_state->hw.crtc)); > > > > > > > > > > > > > > > > @@ -1044,7 +1044,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > > > > > * This should only fail upon a hung GPU, in which case we > > > > > > > > * can safely continue. > > > > > > > > */ > > > > > > > > - if (intel_crtc_needs_modeset(crtc_state)) { > > > > > > > > + if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state)) { > > > > > > > > > > > > > > NAK. We need to fix the bug instead of paparing over it. > > > > > > > > > > > > I had pushed this already. > > > > > > > > > > It didn't even finish CI. Please revert. > > > > > > > > Swati did run CI and verified that fix helps. I'm _not_ going to revert. > > > > > > Fine. I'll do it. > > > > Problem is that you don't even care to explain, why this fix is wrong, but simply > > act in authoritarian way, instead of having constructive discussion. > > I've explanined this one about a hundred times. The NULL pointer should > not happen. Someone needs to actually analyze what is happening instead > of just adding randomg NULL checks all over the place. I do get this point. However why are we doing those check in other places then? Moreover I can remember that you told me to do this check even, when were reviewing my other patches. Because we always have to check result of this function, as it can be NULL, in case if intel_atomic_get_crtc_state wasn't called before, which could happen even in normal case, as I understand. If we want to understand why it happens in particular here, great lets investigate, however I don't get why we are having same checks everywhere all over the place then and I can even find your words, that we need to do those checks as well. Also if this doesn't break anything, improves our CI results, not violating our coding practices, because once again worth mentioning we do check new_crtc_state for NULLness in many places.. then why it can't be the fix? If we find better solution thats great, but there are plenty of other things as well, if you haven't noticed. Can we somehow _stop_ these childish kindergarden level review arguing warfare, at least for sake of professional efficiency? For all my next patches I will always add you to CC and _personally_ will ask to review, even though quite often when I do this - I get nothing. Stan > > > I told that we had verified the fix and that we always do those checks in > > many places anyway where we get new_crtc_state. > > However there were no even reasons to reject mentioned here. > > I don't really think that bringing personality traits and authoritarian > > discussion style is a professional behaviour. > > Thanks for cooperation. > > > > > > > > > > > > > > > > > > > > Moreover as I understand we need to check that new_crtc_state > > > > > > for being NULL anyway. We do check it for being NULL in other places. > > > > > > But if you have another solution - go for it. > > > > > > > > > > > > Stan > > > > > > > > > > > > > > > > > > > > > > > > > > > ret = i915_sw_fence_await_reservation(&state->commit_ready, > > > > > > > > old_obj->base.resv, > > > > > > > > false, 0, > > > > > > > > -- > > > > > > > > 2.37.3 > > > > > > > > > > > > > > -- > > > > > > > Ville Syrjälä > > > > > > > Intel > > > > > > > > > > -- > > > > > Ville Syrjälä > > > > > Intel > > > > > > -- > > > Ville Syrjälä > > > Intel > > -- > Ville Syrjälä > Intel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix NULL ptr deref by checking new_crtc_state 2023-05-05 11:41 ` Lisovskiy, Stanislav @ 2023-05-05 12:09 ` Ville Syrjälä 2023-05-05 12:27 ` Lisovskiy, Stanislav 0 siblings, 1 reply; 30+ messages in thread From: Ville Syrjälä @ 2023-05-05 12:09 UTC (permalink / raw) To: Lisovskiy, Stanislav; +Cc: intel-gfx On Fri, May 05, 2023 at 02:41:24PM +0300, Lisovskiy, Stanislav wrote: > On Fri, May 05, 2023 at 02:25:46PM +0300, Ville Syrjälä wrote: > > On Fri, May 05, 2023 at 02:20:17PM +0300, Lisovskiy, Stanislav wrote: > > > On Fri, May 05, 2023 at 02:06:34PM +0300, Ville Syrjälä wrote: > > > > On Fri, May 05, 2023 at 02:05:27PM +0300, Lisovskiy, Stanislav wrote: > > > > > On Fri, May 05, 2023 at 02:02:43PM +0300, Ville Syrjälä wrote: > > > > > > On Fri, May 05, 2023 at 01:58:03PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > On Fri, May 05, 2023 at 01:54:14PM +0300, Ville Syrjälä wrote: > > > > > > > > On Fri, May 05, 2023 at 11:22:12AM +0300, Stanislav Lisovskiy wrote: > > > > > > > > > intel_atomic_get_new_crtc_state can return NULL, unless crtc state wasn't > > > > > > > > > obtained previously with intel_atomic_get_crtc_state, so we must check it > > > > > > > > > for NULLness here, just as in many other places, where we can't guarantee > > > > > > > > > that intel_atomic_get_crtc_state was called. > > > > > > > > > We are currently getting NULL ptr deref because of that, so this fix was > > > > > > > > > confirmed to help. > > > > > > > > > > > > > > > > > > Fixes: 74a75dc90869 ("drm/i915/display: move plane prepare/cleanup to intel_atomic_plane.c") > > > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > > > > > --- > > > > > > > > > drivers/gpu/drm/i915/display/intel_atomic_plane.c | 4 ++-- > > > > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > index 9f670dcfe76e..4125ee07a271 100644 > > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > @@ -1029,7 +1029,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > > > > > > int ret; > > > > > > > > > > > > > > > > > > if (old_obj) { > > > > > > > > > - const struct intel_crtc_state *crtc_state = > > > > > > > > > + const struct intel_crtc_state *new_crtc_state = > > > > > > > > > intel_atomic_get_new_crtc_state(state, > > > > > > > > > to_intel_crtc(old_plane_state->hw.crtc)); > > > > > > > > > > > > > > > > > > @@ -1044,7 +1044,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > > > > > > * This should only fail upon a hung GPU, in which case we > > > > > > > > > * can safely continue. > > > > > > > > > */ > > > > > > > > > - if (intel_crtc_needs_modeset(crtc_state)) { > > > > > > > > > + if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state)) { > > > > > > > > > > > > > > > > NAK. We need to fix the bug instead of paparing over it. > > > > > > > > > > > > > > I had pushed this already. > > > > > > > > > > > > It didn't even finish CI. Please revert. > > > > > > > > > > Swati did run CI and verified that fix helps. I'm _not_ going to revert. > > > > > > > > Fine. I'll do it. > > > > > > Problem is that you don't even care to explain, why this fix is wrong, but simply > > > act in authoritarian way, instead of having constructive discussion. > > > > I've explanined this one about a hundred times. The NULL pointer should > > not happen. Someone needs to actually analyze what is happening instead > > of just adding randomg NULL checks all over the place. > > I do get this point. However why are we doing those check in other places then? We do then when they are actually necessary. > Moreover I can remember that you told me to do this check even, when were reviewing > my other patches. Because we always have to check result of this function, as it > can be NULL, in case if intel_atomic_get_crtc_state wasn't called before, which > could happen even in normal case, as I understand. You can't apply that kind of general rule. Whether the crtc should have already been added to the state or not is case dependent. In this case that should never be the case since the plane was already added to the state, and thus its crtc should also have been added. > > If we want to understand why it happens in particular here, great lets investigate, > however I don't get why we are having same checks everywhere all over the place then > and I can even find your words, that we need to do those checks as well. > > Also if this doesn't break anything, You can't know that. You're trading a clearly reproducible bug with a silent bug that can cause who knows what other issues. That one will be impossible to debug. > improves our CI results, not violating our coding > practices, because once again worth mentioning we do check new_crtc_state for NULLness > in many places.. then why it can't be the fix? > If we find better solution thats great, but there are plenty of other things as well, > if you haven't noticed. > > Can we somehow _stop_ these childish kindergarden level review arguing warfare, at least > for sake of professional efficiency? Not sure what that kindergarten level stuff is. I just NAKed the patch. > > For all my next patches I will always add you to CC and _personally_ will ask to review, > even though quite often when I do this - I get nothing. I can't review everything in detail. But in any case you should at least wait a day or two for review feedback, and you definitely need to wait for CI results as well. -- Ville Syrjälä Intel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix NULL ptr deref by checking new_crtc_state 2023-05-05 12:09 ` Ville Syrjälä @ 2023-05-05 12:27 ` Lisovskiy, Stanislav 2023-05-05 12:46 ` Ville Syrjälä 0 siblings, 1 reply; 30+ messages in thread From: Lisovskiy, Stanislav @ 2023-05-05 12:27 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx On Fri, May 05, 2023 at 03:09:01PM +0300, Ville Syrjälä wrote: > On Fri, May 05, 2023 at 02:41:24PM +0300, Lisovskiy, Stanislav wrote: > > On Fri, May 05, 2023 at 02:25:46PM +0300, Ville Syrjälä wrote: > > > On Fri, May 05, 2023 at 02:20:17PM +0300, Lisovskiy, Stanislav wrote: > > > > On Fri, May 05, 2023 at 02:06:34PM +0300, Ville Syrjälä wrote: > > > > > On Fri, May 05, 2023 at 02:05:27PM +0300, Lisovskiy, Stanislav wrote: > > > > > > On Fri, May 05, 2023 at 02:02:43PM +0300, Ville Syrjälä wrote: > > > > > > > On Fri, May 05, 2023 at 01:58:03PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > On Fri, May 05, 2023 at 01:54:14PM +0300, Ville Syrjälä wrote: > > > > > > > > > On Fri, May 05, 2023 at 11:22:12AM +0300, Stanislav Lisovskiy wrote: > > > > > > > > > > intel_atomic_get_new_crtc_state can return NULL, unless crtc state wasn't > > > > > > > > > > obtained previously with intel_atomic_get_crtc_state, so we must check it > > > > > > > > > > for NULLness here, just as in many other places, where we can't guarantee > > > > > > > > > > that intel_atomic_get_crtc_state was called. > > > > > > > > > > We are currently getting NULL ptr deref because of that, so this fix was > > > > > > > > > > confirmed to help. > > > > > > > > > > > > > > > > > > > > Fixes: 74a75dc90869 ("drm/i915/display: move plane prepare/cleanup to intel_atomic_plane.c") > > > > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > > > > > > --- > > > > > > > > > > drivers/gpu/drm/i915/display/intel_atomic_plane.c | 4 ++-- > > > > > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > index 9f670dcfe76e..4125ee07a271 100644 > > > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > @@ -1029,7 +1029,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > > > > > > > int ret; > > > > > > > > > > > > > > > > > > > > if (old_obj) { > > > > > > > > > > - const struct intel_crtc_state *crtc_state = > > > > > > > > > > + const struct intel_crtc_state *new_crtc_state = > > > > > > > > > > intel_atomic_get_new_crtc_state(state, > > > > > > > > > > to_intel_crtc(old_plane_state->hw.crtc)); > > > > > > > > > > > > > > > > > > > > @@ -1044,7 +1044,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > > > > > > > * This should only fail upon a hung GPU, in which case we > > > > > > > > > > * can safely continue. > > > > > > > > > > */ > > > > > > > > > > - if (intel_crtc_needs_modeset(crtc_state)) { > > > > > > > > > > + if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state)) { > > > > > > > > > > > > > > > > > > NAK. We need to fix the bug instead of paparing over it. > > > > > > > > > > > > > > > > I had pushed this already. > > > > > > > > > > > > > > It didn't even finish CI. Please revert. > > > > > > > > > > > > Swati did run CI and verified that fix helps. I'm _not_ going to revert. > > > > > > > > > > Fine. I'll do it. > > > > > > > > Problem is that you don't even care to explain, why this fix is wrong, but simply > > > > act in authoritarian way, instead of having constructive discussion. > > > > > > I've explanined this one about a hundred times. The NULL pointer should > > > not happen. Someone needs to actually analyze what is happening instead > > > of just adding randomg NULL checks all over the place. > > > > I do get this point. However why are we doing those check in other places then? > > We do then when they are actually necessary. Well but for example when we do check like if(new_bw_state) in intel_bw.c, we are also might be having potentially some silent bugs. Would you guarantee that if we remove all if(crtc_state) and if(new_bw_state) checks in our code, that there won't be NULL pointer dereferences? I bet you don't. But IF you do, then lets remove it everywhere then, why keeping it there, if we are sure! :)) > > > Moreover I can remember that you told me to do this check even, when were reviewing > > my other patches. Because we always have to check result of this function, as it > > can be NULL, in case if intel_atomic_get_crtc_state wasn't called before, which > > could happen even in normal case, as I understand. > > You can't apply that kind of general rule. Whether the crtc should have > already been added to the state or not is case dependent. In this case > that should never be the case since the plane was already added to the > state, and thus its crtc should also have been added. Well it is kinda weird, that we don't have clear rules here. As I understand this is Bigjoiner, so most likely that was the reason why intel_get_crtc_state wasn't called. I mean I was anyway planning to continue investigating that Bigjoiner logic here in fact, however that fix could help at least CI team to continue testing further. > > > > > If we want to understand why it happens in particular here, great lets investigate, > > however I don't get why we are having same checks everywhere all over the place then > > and I can even find your words, that we need to do those checks as well. > > > > Also if this doesn't break anything, > > You can't know that. You're trading a clearly reproducible > bug with a silent bug that can cause who knows what other > issues. That one will be impossible to debug. Answered above... > > > improves our CI results, not violating our coding > > practices, because once again worth mentioning we do check new_crtc_state for NULLness > > in many places.. then why it can't be the fix? > > If we find better solution thats great, but there are plenty of other things as well, > > if you haven't noticed. > > > > Can we somehow _stop_ these childish kindergarden level review arguing warfare, at least > > for sake of professional efficiency? > > Not sure what that kindergarten level stuff is. I just > NAKed the patch. Well, I'm glad, we are at least discussing now, why you NAKed it, initially without having discussion first. > > > > > For all my next patches I will always add you to CC and _personally_ will ask to review, > > even though quite often when I do this - I get nothing. > > I can't review everything in detail. But in any case you should > at least wait a day or two for review feedback, and you definitely > need to wait for CI results as well. Sometimes I wait for weeks. > > -- > Ville Syrjälä > Intel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix NULL ptr deref by checking new_crtc_state 2023-05-05 12:27 ` Lisovskiy, Stanislav @ 2023-05-05 12:46 ` Ville Syrjälä 2023-05-05 12:52 ` Lisovskiy, Stanislav 2023-05-05 12:54 ` Lisovskiy, Stanislav 0 siblings, 2 replies; 30+ messages in thread From: Ville Syrjälä @ 2023-05-05 12:46 UTC (permalink / raw) To: Lisovskiy, Stanislav; +Cc: intel-gfx On Fri, May 05, 2023 at 03:27:51PM +0300, Lisovskiy, Stanislav wrote: > On Fri, May 05, 2023 at 03:09:01PM +0300, Ville Syrjälä wrote: > > On Fri, May 05, 2023 at 02:41:24PM +0300, Lisovskiy, Stanislav wrote: > > > On Fri, May 05, 2023 at 02:25:46PM +0300, Ville Syrjälä wrote: > > > > On Fri, May 05, 2023 at 02:20:17PM +0300, Lisovskiy, Stanislav wrote: > > > > > On Fri, May 05, 2023 at 02:06:34PM +0300, Ville Syrjälä wrote: > > > > > > On Fri, May 05, 2023 at 02:05:27PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > On Fri, May 05, 2023 at 02:02:43PM +0300, Ville Syrjälä wrote: > > > > > > > > On Fri, May 05, 2023 at 01:58:03PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > On Fri, May 05, 2023 at 01:54:14PM +0300, Ville Syrjälä wrote: > > > > > > > > > > On Fri, May 05, 2023 at 11:22:12AM +0300, Stanislav Lisovskiy wrote: > > > > > > > > > > > intel_atomic_get_new_crtc_state can return NULL, unless crtc state wasn't > > > > > > > > > > > obtained previously with intel_atomic_get_crtc_state, so we must check it > > > > > > > > > > > for NULLness here, just as in many other places, where we can't guarantee > > > > > > > > > > > that intel_atomic_get_crtc_state was called. > > > > > > > > > > > We are currently getting NULL ptr deref because of that, so this fix was > > > > > > > > > > > confirmed to help. > > > > > > > > > > > > > > > > > > > > > > Fixes: 74a75dc90869 ("drm/i915/display: move plane prepare/cleanup to intel_atomic_plane.c") > > > > > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > > > > > > > --- > > > > > > > > > > > drivers/gpu/drm/i915/display/intel_atomic_plane.c | 4 ++-- > > > > > > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > > index 9f670dcfe76e..4125ee07a271 100644 > > > > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > > @@ -1029,7 +1029,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > > > > > > > > int ret; > > > > > > > > > > > > > > > > > > > > > > if (old_obj) { > > > > > > > > > > > - const struct intel_crtc_state *crtc_state = > > > > > > > > > > > + const struct intel_crtc_state *new_crtc_state = > > > > > > > > > > > intel_atomic_get_new_crtc_state(state, > > > > > > > > > > > to_intel_crtc(old_plane_state->hw.crtc)); > > > > > > > > > > > > > > > > > > > > > > @@ -1044,7 +1044,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > > > > > > > > * This should only fail upon a hung GPU, in which case we > > > > > > > > > > > * can safely continue. > > > > > > > > > > > */ > > > > > > > > > > > - if (intel_crtc_needs_modeset(crtc_state)) { > > > > > > > > > > > + if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state)) { > > > > > > > > > > > > > > > > > > > > NAK. We need to fix the bug instead of paparing over it. > > > > > > > > > > > > > > > > > > I had pushed this already. > > > > > > > > > > > > > > > > It didn't even finish CI. Please revert. > > > > > > > > > > > > > > Swati did run CI and verified that fix helps. I'm _not_ going to revert. > > > > > > > > > > > > Fine. I'll do it. > > > > > > > > > > Problem is that you don't even care to explain, why this fix is wrong, but simply > > > > > act in authoritarian way, instead of having constructive discussion. > > > > > > > > I've explanined this one about a hundred times. The NULL pointer should > > > > not happen. Someone needs to actually analyze what is happening instead > > > > of just adding randomg NULL checks all over the place. > > > > > > I do get this point. However why are we doing those check in other places then? > > > > We do then when they are actually necessary. > > Well but for example when we do check like if(new_bw_state) in intel_bw.c, > we are also might be having potentially some silent bugs. > Would you guarantee that if we remove all if(crtc_state) and if(new_bw_state) checks > in our code, that there won't be NULL pointer dereferences? I bet you don't. We have the checks where they are needed. The check in intel_bw_atomic_check() (if that's the one you mean) looks entirely correct to me. > > But IF you do, then lets remove it everywhere then, why keeping it there, if we are sure! :)) > > > > > > Moreover I can remember that you told me to do this check even, when were reviewing > > > my other patches. Because we always have to check result of this function, as it > > > can be NULL, in case if intel_atomic_get_crtc_state wasn't called before, which > > > could happen even in normal case, as I understand. > > > > You can't apply that kind of general rule. Whether the crtc should have > > already been added to the state or not is case dependent. In this case > > that should never be the case since the plane was already added to the > > state, and thus its crtc should also have been added. > > Well it is kinda weird, that we don't have clear rules here. > As I understand this is Bigjoiner, so most likely that was the reason why intel_get_crtc_state > wasn't called. > I mean I was anyway planning to continue investigating that Bigjoiner logic here in fact, > however that fix could help at least CI team to continue testing further. What's the point of testing code that is known to be broken in ways no one currently understands. Any results you get are entirely suspect. > > > > > > > > > If we want to understand why it happens in particular here, great lets investigate, > > > however I don't get why we are having same checks everywhere all over the place then > > > and I can even find your words, that we need to do those checks as well. > > > > > > Also if this doesn't break anything, > > > > You can't know that. You're trading a clearly reproducible > > bug with a silent bug that can cause who knows what other > > issues. That one will be impossible to debug. > > Answered above... > > > > > > improves our CI results, not violating our coding > > > practices, because once again worth mentioning we do check new_crtc_state for NULLness > > > in many places.. then why it can't be the fix? > > > If we find better solution thats great, but there are plenty of other things as well, > > > if you haven't noticed. > > > > > > Can we somehow _stop_ these childish kindergarden level review arguing warfare, at least > > > for sake of professional efficiency? > > > > Not sure what that kindergarten level stuff is. I just > > NAKed the patch. > > Well, I'm glad, we are at least discussing now, why you NAKed it, initially without > having discussion first. Like I said, this specific bug has been discussed before, and IIRC we have at least one internal bug report about it, not sure if there's also a gitlab issue. Am I to assume you haven't actually read those? > > > > > > > > > For all my next patches I will always add you to CC and _personally_ will ask to review, > > > even though quite often when I do this - I get nothing. > > > > I can't review everything in detail. But in any case you should > > at least wait a day or two for review feedback, and you definitely > > need to wait for CI results as well. > > Sometimes I wait for weeks. I presume you mean review feedback here rather than CI results? I would say if a week has passed by and you need more input then ping people directly (for me pinging on irc is probably the thing that works best). If you need to wait for CI results for that long then you need to have a serious talk with the CI team. -- Ville Syrjälä Intel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix NULL ptr deref by checking new_crtc_state 2023-05-05 12:46 ` Ville Syrjälä @ 2023-05-05 12:52 ` Lisovskiy, Stanislav 2023-05-05 13:52 ` Ville Syrjälä 2023-05-05 12:54 ` Lisovskiy, Stanislav 1 sibling, 1 reply; 30+ messages in thread From: Lisovskiy, Stanislav @ 2023-05-05 12:52 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx On Fri, May 05, 2023 at 03:46:40PM +0300, Ville Syrjälä wrote: > On Fri, May 05, 2023 at 03:27:51PM +0300, Lisovskiy, Stanislav wrote: > > On Fri, May 05, 2023 at 03:09:01PM +0300, Ville Syrjälä wrote: > > > On Fri, May 05, 2023 at 02:41:24PM +0300, Lisovskiy, Stanislav wrote: > > > > On Fri, May 05, 2023 at 02:25:46PM +0300, Ville Syrjälä wrote: > > > > > On Fri, May 05, 2023 at 02:20:17PM +0300, Lisovskiy, Stanislav wrote: > > > > > > On Fri, May 05, 2023 at 02:06:34PM +0300, Ville Syrjälä wrote: > > > > > > > On Fri, May 05, 2023 at 02:05:27PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > On Fri, May 05, 2023 at 02:02:43PM +0300, Ville Syrjälä wrote: > > > > > > > > > On Fri, May 05, 2023 at 01:58:03PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > > On Fri, May 05, 2023 at 01:54:14PM +0300, Ville Syrjälä wrote: > > > > > > > > > > > On Fri, May 05, 2023 at 11:22:12AM +0300, Stanislav Lisovskiy wrote: > > > > > > > > > > > > intel_atomic_get_new_crtc_state can return NULL, unless crtc state wasn't > > > > > > > > > > > > obtained previously with intel_atomic_get_crtc_state, so we must check it > > > > > > > > > > > > for NULLness here, just as in many other places, where we can't guarantee > > > > > > > > > > > > that intel_atomic_get_crtc_state was called. > > > > > > > > > > > > We are currently getting NULL ptr deref because of that, so this fix was > > > > > > > > > > > > confirmed to help. > > > > > > > > > > > > > > > > > > > > > > > > Fixes: 74a75dc90869 ("drm/i915/display: move plane prepare/cleanup to intel_atomic_plane.c") > > > > > > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > > > > > > > > --- > > > > > > > > > > > > drivers/gpu/drm/i915/display/intel_atomic_plane.c | 4 ++-- > > > > > > > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > > > index 9f670dcfe76e..4125ee07a271 100644 > > > > > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > > > @@ -1029,7 +1029,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > > > > > > > > > int ret; > > > > > > > > > > > > > > > > > > > > > > > > if (old_obj) { > > > > > > > > > > > > - const struct intel_crtc_state *crtc_state = > > > > > > > > > > > > + const struct intel_crtc_state *new_crtc_state = > > > > > > > > > > > > intel_atomic_get_new_crtc_state(state, > > > > > > > > > > > > to_intel_crtc(old_plane_state->hw.crtc)); > > > > > > > > > > > > > > > > > > > > > > > > @@ -1044,7 +1044,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > > > > > > > > > * This should only fail upon a hung GPU, in which case we > > > > > > > > > > > > * can safely continue. > > > > > > > > > > > > */ > > > > > > > > > > > > - if (intel_crtc_needs_modeset(crtc_state)) { > > > > > > > > > > > > + if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state)) { > > > > > > > > > > > > > > > > > > > > > > NAK. We need to fix the bug instead of paparing over it. > > > > > > > > > > > > > > > > > > > > I had pushed this already. > > > > > > > > > > > > > > > > > > It didn't even finish CI. Please revert. > > > > > > > > > > > > > > > > Swati did run CI and verified that fix helps. I'm _not_ going to revert. > > > > > > > > > > > > > > Fine. I'll do it. > > > > > > > > > > > > Problem is that you don't even care to explain, why this fix is wrong, but simply > > > > > > act in authoritarian way, instead of having constructive discussion. > > > > > > > > > > I've explanined this one about a hundred times. The NULL pointer should > > > > > not happen. Someone needs to actually analyze what is happening instead > > > > > of just adding randomg NULL checks all over the place. > > > > > > > > I do get this point. However why are we doing those check in other places then? > > > > > > We do then when they are actually necessary. > > > > Well but for example when we do check like if(new_bw_state) in intel_bw.c, > > we are also might be having potentially some silent bugs. > > Would you guarantee that if we remove all if(crtc_state) and if(new_bw_state) checks > > in our code, that there won't be NULL pointer dereferences? I bet you don't. > > We have the checks where they are needed. The check in > intel_bw_atomic_check() (if that's the one you mean) > looks entirely correct to me. They are needed because there might the case, when intel_atomic_get_crtc might not get called right? > > > > > But IF you do, then lets remove it everywhere then, why keeping it there, if we are sure! :)) > > > > > > > > > Moreover I can remember that you told me to do this check even, when were reviewing > > > > my other patches. Because we always have to check result of this function, as it > > > > can be NULL, in case if intel_atomic_get_crtc_state wasn't called before, which > > > > could happen even in normal case, as I understand. > > > > > > You can't apply that kind of general rule. Whether the crtc should have > > > already been added to the state or not is case dependent. In this case > > > that should never be the case since the plane was already added to the > > > state, and thus its crtc should also have been added. > > > > Well it is kinda weird, that we don't have clear rules here. > > As I understand this is Bigjoiner, so most likely that was the reason why intel_get_crtc_state > > wasn't called. > > I mean I was anyway planning to continue investigating that Bigjoiner logic here in fact, > > however that fix could help at least CI team to continue testing further. > > What's the point of testing code that is known to be broken in > ways no one currently understands. Any results you get are entirely > suspect. Any code has some issues, what we do is trying to gradually fix those. > > > > > > > > > > > > > > If we want to understand why it happens in particular here, great lets investigate, > > > > however I don't get why we are having same checks everywhere all over the place then > > > > and I can even find your words, that we need to do those checks as well. > > > > > > > > Also if this doesn't break anything, > > > > > > You can't know that. You're trading a clearly reproducible > > > bug with a silent bug that can cause who knows what other > > > issues. That one will be impossible to debug. > > > > Answered above... > > > > > > > > > improves our CI results, not violating our coding > > > > practices, because once again worth mentioning we do check new_crtc_state for NULLness > > > > in many places.. then why it can't be the fix? > > > > If we find better solution thats great, but there are plenty of other things as well, > > > > if you haven't noticed. > > > > > > > > Can we somehow _stop_ these childish kindergarden level review arguing warfare, at least > > > > for sake of professional efficiency? > > > > > > Not sure what that kindergarten level stuff is. I just > > > NAKed the patch. > > > > Well, I'm glad, we are at least discussing now, why you NAKed it, initially without > > having discussion first. > > Like I said, this specific bug has been discussed before, and IIRC > we have at least one internal bug report about it, not sure if > there's also a gitlab issue. Am I to assume you haven't actually > read those? Well that is where I started actually. > > > > > > > > > > > > > > For all my next patches I will always add you to CC and _personally_ will ask to review, > > > > even though quite often when I do this - I get nothing. > > > > > > I can't review everything in detail. But in any case you should > > > at least wait a day or two for review feedback, and you definitely > > > need to wait for CI results as well. > > > > Sometimes I wait for weeks. > > I presume you mean review feedback here rather than CI results? > I would say if a week has passed by and you need more input then > ping people directly (for me pinging on irc is probably the > thing that works best). > > If you need to wait for CI results for that long then you need > to have a serious talk with the CI team. Yep, regarding pinging I agree, lets discuss offline regarding how we could improve that. > > -- > Ville Syrjälä > Intel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix NULL ptr deref by checking new_crtc_state 2023-05-05 12:52 ` Lisovskiy, Stanislav @ 2023-05-05 13:52 ` Ville Syrjälä 0 siblings, 0 replies; 30+ messages in thread From: Ville Syrjälä @ 2023-05-05 13:52 UTC (permalink / raw) To: Lisovskiy, Stanislav; +Cc: intel-gfx On Fri, May 05, 2023 at 03:52:12PM +0300, Lisovskiy, Stanislav wrote: > On Fri, May 05, 2023 at 03:46:40PM +0300, Ville Syrjälä wrote: > > On Fri, May 05, 2023 at 03:27:51PM +0300, Lisovskiy, Stanislav wrote: > > > On Fri, May 05, 2023 at 03:09:01PM +0300, Ville Syrjälä wrote: > > > > On Fri, May 05, 2023 at 02:41:24PM +0300, Lisovskiy, Stanislav wrote: > > > > > On Fri, May 05, 2023 at 02:25:46PM +0300, Ville Syrjälä wrote: > > > > > > On Fri, May 05, 2023 at 02:20:17PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > On Fri, May 05, 2023 at 02:06:34PM +0300, Ville Syrjälä wrote: > > > > > > > > On Fri, May 05, 2023 at 02:05:27PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > On Fri, May 05, 2023 at 02:02:43PM +0300, Ville Syrjälä wrote: > > > > > > > > > > On Fri, May 05, 2023 at 01:58:03PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > > > On Fri, May 05, 2023 at 01:54:14PM +0300, Ville Syrjälä wrote: > > > > > > > > > > > > On Fri, May 05, 2023 at 11:22:12AM +0300, Stanislav Lisovskiy wrote: > > > > > > > > > > > > > intel_atomic_get_new_crtc_state can return NULL, unless crtc state wasn't > > > > > > > > > > > > > obtained previously with intel_atomic_get_crtc_state, so we must check it > > > > > > > > > > > > > for NULLness here, just as in many other places, where we can't guarantee > > > > > > > > > > > > > that intel_atomic_get_crtc_state was called. > > > > > > > > > > > > > We are currently getting NULL ptr deref because of that, so this fix was > > > > > > > > > > > > > confirmed to help. > > > > > > > > > > > > > > > > > > > > > > > > > > Fixes: 74a75dc90869 ("drm/i915/display: move plane prepare/cleanup to intel_atomic_plane.c") > > > > > > > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > > > > > > > > > --- > > > > > > > > > > > > > drivers/gpu/drm/i915/display/intel_atomic_plane.c | 4 ++-- > > > > > > > > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > > > > index 9f670dcfe76e..4125ee07a271 100644 > > > > > > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > > > > @@ -1029,7 +1029,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > > > > > > > > > > int ret; > > > > > > > > > > > > > > > > > > > > > > > > > > if (old_obj) { > > > > > > > > > > > > > - const struct intel_crtc_state *crtc_state = > > > > > > > > > > > > > + const struct intel_crtc_state *new_crtc_state = > > > > > > > > > > > > > intel_atomic_get_new_crtc_state(state, > > > > > > > > > > > > > to_intel_crtc(old_plane_state->hw.crtc)); > > > > > > > > > > > > > > > > > > > > > > > > > > @@ -1044,7 +1044,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > > > > > > > > > > * This should only fail upon a hung GPU, in which case we > > > > > > > > > > > > > * can safely continue. > > > > > > > > > > > > > */ > > > > > > > > > > > > > - if (intel_crtc_needs_modeset(crtc_state)) { > > > > > > > > > > > > > + if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state)) { > > > > > > > > > > > > > > > > > > > > > > > > NAK. We need to fix the bug instead of paparing over it. > > > > > > > > > > > > > > > > > > > > > > I had pushed this already. > > > > > > > > > > > > > > > > > > > > It didn't even finish CI. Please revert. > > > > > > > > > > > > > > > > > > Swati did run CI and verified that fix helps. I'm _not_ going to revert. > > > > > > > > > > > > > > > > Fine. I'll do it. > > > > > > > > > > > > > > Problem is that you don't even care to explain, why this fix is wrong, but simply > > > > > > > act in authoritarian way, instead of having constructive discussion. > > > > > > > > > > > > I've explanined this one about a hundred times. The NULL pointer should > > > > > > not happen. Someone needs to actually analyze what is happening instead > > > > > > of just adding randomg NULL checks all over the place. > > > > > > > > > > I do get this point. However why are we doing those check in other places then? > > > > > > > > We do then when they are actually necessary. > > > > > > Well but for example when we do check like if(new_bw_state) in intel_bw.c, > > > we are also might be having potentially some silent bugs. > > > Would you guarantee that if we remove all if(crtc_state) and if(new_bw_state) checks > > > in our code, that there won't be NULL pointer dereferences? I bet you don't. > > > > We have the checks where they are needed. The check in > > intel_bw_atomic_check() (if that's the one you mean) > > looks entirely correct to me. > > They are needed because there might the case, when intel_atomic_get_crtc > might not get called right? intel_bw_get_state() in this case, but yes it may not have been called prior to intel_bw_atomic_check(), and that is fine because in this case it means that there are no changes that need bw/qgv recalculations/etc. > > > > > > > > > But IF you do, then lets remove it everywhere then, why keeping it there, if we are sure! :)) > > > > > > > > > > > > Moreover I can remember that you told me to do this check even, when were reviewing > > > > > my other patches. Because we always have to check result of this function, as it > > > > > can be NULL, in case if intel_atomic_get_crtc_state wasn't called before, which > > > > > could happen even in normal case, as I understand. > > > > > > > > You can't apply that kind of general rule. Whether the crtc should have > > > > already been added to the state or not is case dependent. In this case > > > > that should never be the case since the plane was already added to the > > > > state, and thus its crtc should also have been added. > > > > > > Well it is kinda weird, that we don't have clear rules here. > > > As I understand this is Bigjoiner, so most likely that was the reason why intel_get_crtc_state > > > wasn't called. > > > I mean I was anyway planning to continue investigating that Bigjoiner logic here in fact, > > > however that fix could help at least CI team to continue testing further. > > > > What's the point of testing code that is known to be broken in > > ways no one currently understands. Any results you get are entirely > > suspect. > > Any code has some issues, what we do is trying to gradually fix those. You won't make any meaningful progress if you intentially add the same number of bugs (or more) back that you fixed. Or just paper over easy to diagnose issues, knowingly trading them for much harder to diagnose issues, as is the case here. This kind of thing might also introduce even more problems for the future in case the paper ends up covering more potential sources of bugs (which would also be the case here since now also non-bigjoiner use cases are in danger of failing silently). > > > > > > > > > > > > > > > > > > > > If we want to understand why it happens in particular here, great lets investigate, > > > > > however I don't get why we are having same checks everywhere all over the place then > > > > > and I can even find your words, that we need to do those checks as well. > > > > > > > > > > Also if this doesn't break anything, > > > > > > > > You can't know that. You're trading a clearly reproducible > > > > bug with a silent bug that can cause who knows what other > > > > issues. That one will be impossible to debug. > > > > > > Answered above... > > > > > > > > > > > > improves our CI results, not violating our coding > > > > > practices, because once again worth mentioning we do check new_crtc_state for NULLness > > > > > in many places.. then why it can't be the fix? > > > > > If we find better solution thats great, but there are plenty of other things as well, > > > > > if you haven't noticed. > > > > > > > > > > Can we somehow _stop_ these childish kindergarden level review arguing warfare, at least > > > > > for sake of professional efficiency? > > > > > > > > Not sure what that kindergarten level stuff is. I just > > > > NAKed the patch. > > > > > > Well, I'm glad, we are at least discussing now, why you NAKed it, initially without > > > having discussion first. > > > > Like I said, this specific bug has been discussed before, and IIRC > > we have at least one internal bug report about it, not sure if > > there's also a gitlab issue. Am I to assume you haven't actually > > read those? > > Well that is where I started actually. I'm pretty sure there should have been a comment from me saying the exact same thing I'm saying here. > > > > > > > > > > > > > > > > > > > > For all my next patches I will always add you to CC and _personally_ will ask to review, > > > > > even though quite often when I do this - I get nothing. > > > > > > > > I can't review everything in detail. But in any case you should > > > > at least wait a day or two for review feedback, and you definitely > > > > need to wait for CI results as well. > > > > > > Sometimes I wait for weeks. > > > > I presume you mean review feedback here rather than CI results? > > I would say if a week has passed by and you need more input then > > ping people directly (for me pinging on irc is probably the > > thing that works best). > > > > If you need to wait for CI results for that long then you need > > to have a serious talk with the CI team. > > Yep, regarding pinging I agree, lets discuss offline regarding > how we could improve that. > > > > > -- > > Ville Syrjälä > > Intel -- Ville Syrjälä Intel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix NULL ptr deref by checking new_crtc_state 2023-05-05 12:46 ` Ville Syrjälä 2023-05-05 12:52 ` Lisovskiy, Stanislav @ 2023-05-05 12:54 ` Lisovskiy, Stanislav 2023-05-05 13:11 ` Ville Syrjälä 1 sibling, 1 reply; 30+ messages in thread From: Lisovskiy, Stanislav @ 2023-05-05 12:54 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx On Fri, May 05, 2023 at 03:46:40PM +0300, Ville Syrjälä wrote: > On Fri, May 05, 2023 at 03:27:51PM +0300, Lisovskiy, Stanislav wrote: > > On Fri, May 05, 2023 at 03:09:01PM +0300, Ville Syrjälä wrote: > > > On Fri, May 05, 2023 at 02:41:24PM +0300, Lisovskiy, Stanislav wrote: > > > > On Fri, May 05, 2023 at 02:25:46PM +0300, Ville Syrjälä wrote: > > > > > On Fri, May 05, 2023 at 02:20:17PM +0300, Lisovskiy, Stanislav wrote: > > > > > > On Fri, May 05, 2023 at 02:06:34PM +0300, Ville Syrjälä wrote: > > > > > > > On Fri, May 05, 2023 at 02:05:27PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > On Fri, May 05, 2023 at 02:02:43PM +0300, Ville Syrjälä wrote: > > > > > > > > > On Fri, May 05, 2023 at 01:58:03PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > > On Fri, May 05, 2023 at 01:54:14PM +0300, Ville Syrjälä wrote: > > > > > > > > > > > On Fri, May 05, 2023 at 11:22:12AM +0300, Stanislav Lisovskiy wrote: > > > > > > > > > > > > intel_atomic_get_new_crtc_state can return NULL, unless crtc state wasn't > > > > > > > > > > > > obtained previously with intel_atomic_get_crtc_state, so we must check it > > > > > > > > > > > > for NULLness here, just as in many other places, where we can't guarantee > > > > > > > > > > > > that intel_atomic_get_crtc_state was called. > > > > > > > > > > > > We are currently getting NULL ptr deref because of that, so this fix was > > > > > > > > > > > > confirmed to help. > > > > > > > > > > > > > > > > > > > > > > > > Fixes: 74a75dc90869 ("drm/i915/display: move plane prepare/cleanup to intel_atomic_plane.c") > > > > > > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > > > > > > > > --- > > > > > > > > > > > > drivers/gpu/drm/i915/display/intel_atomic_plane.c | 4 ++-- > > > > > > > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > > > index 9f670dcfe76e..4125ee07a271 100644 > > > > > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > > > @@ -1029,7 +1029,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > > > > > > > > > int ret; > > > > > > > > > > > > > > > > > > > > > > > > if (old_obj) { > > > > > > > > > > > > - const struct intel_crtc_state *crtc_state = > > > > > > > > > > > > + const struct intel_crtc_state *new_crtc_state = > > > > > > > > > > > > intel_atomic_get_new_crtc_state(state, > > > > > > > > > > > > to_intel_crtc(old_plane_state->hw.crtc)); > > > > > > > > > > > > > > > > > > > > > > > > @@ -1044,7 +1044,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > > > > > > > > > * This should only fail upon a hung GPU, in which case we > > > > > > > > > > > > * can safely continue. > > > > > > > > > > > > */ > > > > > > > > > > > > - if (intel_crtc_needs_modeset(crtc_state)) { > > > > > > > > > > > > + if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state)) { > > > > > > > > > > > > > > > > > > > > > > NAK. We need to fix the bug instead of paparing over it. > > > > > > > > > > > > > > > > > > > > I had pushed this already. > > > > > > > > > > > > > > > > > > It didn't even finish CI. Please revert. > > > > > > > > > > > > > > > > Swati did run CI and verified that fix helps. I'm _not_ going to revert. > > > > > > > > > > > > > > Fine. I'll do it. > > > > > > > > > > > > Problem is that you don't even care to explain, why this fix is wrong, but simply > > > > > > act in authoritarian way, instead of having constructive discussion. > > > > > > > > > > I've explanined this one about a hundred times. The NULL pointer should > > > > > not happen. Someone needs to actually analyze what is happening instead > > > > > of just adding randomg NULL checks all over the place. > > > > > > > > I do get this point. However why are we doing those check in other places then? > > > > > > We do then when they are actually necessary. > > > > Well but for example when we do check like if(new_bw_state) in intel_bw.c, > > we are also might be having potentially some silent bugs. > > Would you guarantee that if we remove all if(crtc_state) and if(new_bw_state) checks > > in our code, that there won't be NULL pointer dereferences? I bet you don't. > > We have the checks where they are needed. The check in > intel_bw_atomic_check() (if that's the one you mean) > looks entirely correct to me. Typo in my prev message, I meant intel_atomic_get_bw_state..but common idea is the same. > > > > > But IF you do, then lets remove it everywhere then, why keeping it there, if we are sure! :)) > > > > > > > > > Moreover I can remember that you told me to do this check even, when were reviewing > > > > my other patches. Because we always have to check result of this function, as it > > > > can be NULL, in case if intel_atomic_get_crtc_state wasn't called before, which > > > > could happen even in normal case, as I understand. > > > > > > You can't apply that kind of general rule. Whether the crtc should have > > > already been added to the state or not is case dependent. In this case > > > that should never be the case since the plane was already added to the > > > state, and thus its crtc should also have been added. > > > > Well it is kinda weird, that we don't have clear rules here. > > As I understand this is Bigjoiner, so most likely that was the reason why intel_get_crtc_state > > wasn't called. > > I mean I was anyway planning to continue investigating that Bigjoiner logic here in fact, > > however that fix could help at least CI team to continue testing further. > > What's the point of testing code that is known to be broken in > ways no one currently understands. Any results you get are entirely > suspect. > > > > > > > > > > > > > > If we want to understand why it happens in particular here, great lets investigate, > > > > however I don't get why we are having same checks everywhere all over the place then > > > > and I can even find your words, that we need to do those checks as well. > > > > > > > > Also if this doesn't break anything, > > > > > > You can't know that. You're trading a clearly reproducible > > > bug with a silent bug that can cause who knows what other > > > issues. That one will be impossible to debug. > > > > Answered above... > > > > > > > > > improves our CI results, not violating our coding > > > > practices, because once again worth mentioning we do check new_crtc_state for NULLness > > > > in many places.. then why it can't be the fix? > > > > If we find better solution thats great, but there are plenty of other things as well, > > > > if you haven't noticed. > > > > > > > > Can we somehow _stop_ these childish kindergarden level review arguing warfare, at least > > > > for sake of professional efficiency? > > > > > > Not sure what that kindergarten level stuff is. I just > > > NAKed the patch. > > > > Well, I'm glad, we are at least discussing now, why you NAKed it, initially without > > having discussion first. > > Like I said, this specific bug has been discussed before, and IIRC > we have at least one internal bug report about it, not sure if > there's also a gitlab issue. Am I to assume you haven't actually > read those? > > > > > > > > > > > > > > For all my next patches I will always add you to CC and _personally_ will ask to review, > > > > even though quite often when I do this - I get nothing. > > > > > > I can't review everything in detail. But in any case you should > > > at least wait a day or two for review feedback, and you definitely > > > need to wait for CI results as well. > > > > Sometimes I wait for weeks. > > I presume you mean review feedback here rather than CI results? > I would say if a week has passed by and you need more input then > ping people directly (for me pinging on irc is probably the > thing that works best). > > If you need to wait for CI results for that long then you need > to have a serious talk with the CI team. > > -- > Ville Syrjälä > Intel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix NULL ptr deref by checking new_crtc_state 2023-05-05 12:54 ` Lisovskiy, Stanislav @ 2023-05-05 13:11 ` Ville Syrjälä 2023-05-05 13:21 ` Lisovskiy, Stanislav 0 siblings, 1 reply; 30+ messages in thread From: Ville Syrjälä @ 2023-05-05 13:11 UTC (permalink / raw) To: Lisovskiy, Stanislav; +Cc: intel-gfx On Fri, May 05, 2023 at 03:54:58PM +0300, Lisovskiy, Stanislav wrote: > On Fri, May 05, 2023 at 03:46:40PM +0300, Ville Syrjälä wrote: > > On Fri, May 05, 2023 at 03:27:51PM +0300, Lisovskiy, Stanislav wrote: > > > On Fri, May 05, 2023 at 03:09:01PM +0300, Ville Syrjälä wrote: > > > > On Fri, May 05, 2023 at 02:41:24PM +0300, Lisovskiy, Stanislav wrote: > > > > > On Fri, May 05, 2023 at 02:25:46PM +0300, Ville Syrjälä wrote: > > > > > > On Fri, May 05, 2023 at 02:20:17PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > On Fri, May 05, 2023 at 02:06:34PM +0300, Ville Syrjälä wrote: > > > > > > > > On Fri, May 05, 2023 at 02:05:27PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > On Fri, May 05, 2023 at 02:02:43PM +0300, Ville Syrjälä wrote: > > > > > > > > > > On Fri, May 05, 2023 at 01:58:03PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > > > On Fri, May 05, 2023 at 01:54:14PM +0300, Ville Syrjälä wrote: > > > > > > > > > > > > On Fri, May 05, 2023 at 11:22:12AM +0300, Stanislav Lisovskiy wrote: > > > > > > > > > > > > > intel_atomic_get_new_crtc_state can return NULL, unless crtc state wasn't > > > > > > > > > > > > > obtained previously with intel_atomic_get_crtc_state, so we must check it > > > > > > > > > > > > > for NULLness here, just as in many other places, where we can't guarantee > > > > > > > > > > > > > that intel_atomic_get_crtc_state was called. > > > > > > > > > > > > > We are currently getting NULL ptr deref because of that, so this fix was > > > > > > > > > > > > > confirmed to help. > > > > > > > > > > > > > > > > > > > > > > > > > > Fixes: 74a75dc90869 ("drm/i915/display: move plane prepare/cleanup to intel_atomic_plane.c") > > > > > > > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > > > > > > > > > --- > > > > > > > > > > > > > drivers/gpu/drm/i915/display/intel_atomic_plane.c | 4 ++-- > > > > > > > > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > > > > index 9f670dcfe76e..4125ee07a271 100644 > > > > > > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > > > > @@ -1029,7 +1029,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > > > > > > > > > > int ret; > > > > > > > > > > > > > > > > > > > > > > > > > > if (old_obj) { > > > > > > > > > > > > > - const struct intel_crtc_state *crtc_state = > > > > > > > > > > > > > + const struct intel_crtc_state *new_crtc_state = > > > > > > > > > > > > > intel_atomic_get_new_crtc_state(state, > > > > > > > > > > > > > to_intel_crtc(old_plane_state->hw.crtc)); > > > > > > > > > > > > > > > > > > > > > > > > > > @@ -1044,7 +1044,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > > > > > > > > > > * This should only fail upon a hung GPU, in which case we > > > > > > > > > > > > > * can safely continue. > > > > > > > > > > > > > */ > > > > > > > > > > > > > - if (intel_crtc_needs_modeset(crtc_state)) { > > > > > > > > > > > > > + if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state)) { > > > > > > > > > > > > > > > > > > > > > > > > NAK. We need to fix the bug instead of paparing over it. > > > > > > > > > > > > > > > > > > > > > > I had pushed this already. > > > > > > > > > > > > > > > > > > > > It didn't even finish CI. Please revert. > > > > > > > > > > > > > > > > > > Swati did run CI and verified that fix helps. I'm _not_ going to revert. > > > > > > > > > > > > > > > > Fine. I'll do it. > > > > > > > > > > > > > > Problem is that you don't even care to explain, why this fix is wrong, but simply > > > > > > > act in authoritarian way, instead of having constructive discussion. > > > > > > > > > > > > I've explanined this one about a hundred times. The NULL pointer should > > > > > > not happen. Someone needs to actually analyze what is happening instead > > > > > > of just adding randomg NULL checks all over the place. > > > > > > > > > > I do get this point. However why are we doing those check in other places then? > > > > > > > > We do then when they are actually necessary. > > > > > > Well but for example when we do check like if(new_bw_state) in intel_bw.c, > > > we are also might be having potentially some silent bugs. > > > Would you guarantee that if we remove all if(crtc_state) and if(new_bw_state) checks > > > in our code, that there won't be NULL pointer dereferences? I bet you don't. > > > > We have the checks where they are needed. The check in > > intel_bw_atomic_check() (if that's the one you mean) > > looks entirely correct to me. > > Typo in my prev message, I meant intel_atomic_get_bw_state..but common idea is the same. get_state() vs. get_{new,old}_state() are entirely different things. You use get_state() when you really want the state to be included, and either - know the state isn't included already, or - you don't know wether the might have alerady been included And one must of course remember that get_state() can - fail so error handling is needed - only be used during the check phase, and is illegal during the commit phase. The get_{new,old}_state() (or the various for loop variants) you can use when you either: - know that the state is included already - are fine with the state potentially not being included That's just atomic 101. -- Ville Syrjälä Intel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix NULL ptr deref by checking new_crtc_state 2023-05-05 13:11 ` Ville Syrjälä @ 2023-05-05 13:21 ` Lisovskiy, Stanislav 2023-05-05 13:28 ` Ville Syrjälä 0 siblings, 1 reply; 30+ messages in thread From: Lisovskiy, Stanislav @ 2023-05-05 13:21 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx On Fri, May 05, 2023 at 04:11:52PM +0300, Ville Syrjälä wrote: > On Fri, May 05, 2023 at 03:54:58PM +0300, Lisovskiy, Stanislav wrote: > > On Fri, May 05, 2023 at 03:46:40PM +0300, Ville Syrjälä wrote: > > > On Fri, May 05, 2023 at 03:27:51PM +0300, Lisovskiy, Stanislav wrote: > > > > On Fri, May 05, 2023 at 03:09:01PM +0300, Ville Syrjälä wrote: > > > > > On Fri, May 05, 2023 at 02:41:24PM +0300, Lisovskiy, Stanislav wrote: > > > > > > On Fri, May 05, 2023 at 02:25:46PM +0300, Ville Syrjälä wrote: > > > > > > > On Fri, May 05, 2023 at 02:20:17PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > On Fri, May 05, 2023 at 02:06:34PM +0300, Ville Syrjälä wrote: > > > > > > > > > On Fri, May 05, 2023 at 02:05:27PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > > On Fri, May 05, 2023 at 02:02:43PM +0300, Ville Syrjälä wrote: > > > > > > > > > > > On Fri, May 05, 2023 at 01:58:03PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > > > > On Fri, May 05, 2023 at 01:54:14PM +0300, Ville Syrjälä wrote: > > > > > > > > > > > > > On Fri, May 05, 2023 at 11:22:12AM +0300, Stanislav Lisovskiy wrote: > > > > > > > > > > > > > > intel_atomic_get_new_crtc_state can return NULL, unless crtc state wasn't > > > > > > > > > > > > > > obtained previously with intel_atomic_get_crtc_state, so we must check it > > > > > > > > > > > > > > for NULLness here, just as in many other places, where we can't guarantee > > > > > > > > > > > > > > that intel_atomic_get_crtc_state was called. > > > > > > > > > > > > > > We are currently getting NULL ptr deref because of that, so this fix was > > > > > > > > > > > > > > confirmed to help. > > > > > > > > > > > > > > > > > > > > > > > > > > > > Fixes: 74a75dc90869 ("drm/i915/display: move plane prepare/cleanup to intel_atomic_plane.c") > > > > > > > > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > drivers/gpu/drm/i915/display/intel_atomic_plane.c | 4 ++-- > > > > > > > > > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > > > > > index 9f670dcfe76e..4125ee07a271 100644 > > > > > > > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > > > > > @@ -1029,7 +1029,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > > > > > > > > > > > int ret; > > > > > > > > > > > > > > > > > > > > > > > > > > > > if (old_obj) { > > > > > > > > > > > > > > - const struct intel_crtc_state *crtc_state = > > > > > > > > > > > > > > + const struct intel_crtc_state *new_crtc_state = > > > > > > > > > > > > > > intel_atomic_get_new_crtc_state(state, > > > > > > > > > > > > > > to_intel_crtc(old_plane_state->hw.crtc)); > > > > > > > > > > > > > > > > > > > > > > > > > > > > @@ -1044,7 +1044,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > > > > > > > > > > > * This should only fail upon a hung GPU, in which case we > > > > > > > > > > > > > > * can safely continue. > > > > > > > > > > > > > > */ > > > > > > > > > > > > > > - if (intel_crtc_needs_modeset(crtc_state)) { > > > > > > > > > > > > > > + if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state)) { > > > > > > > > > > > > > > > > > > > > > > > > > > NAK. We need to fix the bug instead of paparing over it. > > > > > > > > > > > > > > > > > > > > > > > > I had pushed this already. > > > > > > > > > > > > > > > > > > > > > > It didn't even finish CI. Please revert. > > > > > > > > > > > > > > > > > > > > Swati did run CI and verified that fix helps. I'm _not_ going to revert. > > > > > > > > > > > > > > > > > > Fine. I'll do it. > > > > > > > > > > > > > > > > Problem is that you don't even care to explain, why this fix is wrong, but simply > > > > > > > > act in authoritarian way, instead of having constructive discussion. > > > > > > > > > > > > > > I've explanined this one about a hundred times. The NULL pointer should > > > > > > > not happen. Someone needs to actually analyze what is happening instead > > > > > > > of just adding randomg NULL checks all over the place. > > > > > > > > > > > > I do get this point. However why are we doing those check in other places then? > > > > > > > > > > We do then when they are actually necessary. > > > > > > > > Well but for example when we do check like if(new_bw_state) in intel_bw.c, > > > > we are also might be having potentially some silent bugs. > > > > Would you guarantee that if we remove all if(crtc_state) and if(new_bw_state) checks > > > > in our code, that there won't be NULL pointer dereferences? I bet you don't. > > > > > > We have the checks where they are needed. The check in > > > intel_bw_atomic_check() (if that's the one you mean) > > > looks entirely correct to me. > > > > Typo in my prev message, I meant intel_atomic_get_bw_state..but common idea is the same. > > get_state() vs. get_{new,old}_state() are entirely different > things. > > You use get_state() when you really want the state to be > included, and either > - know the state isn't included already, or > - you don't know wether the might have alerady been included > > And one must of course remember that get_state() can > - fail so error handling is needed > - only be used during the check phase, and is illegal during the > commit phase. Sure I know this. I even remember we discussed this many times. > > The get_{new,old}_state() (or the various for loop variants) > you can use when you either: > - know that the state is included already > - are fine with the state potentially not being included Don't you see that it is a bit of a contradiction in those 2 above?? You can't be "know that the state is included already" and "are fine with the state potentially not being included" same time :) Those 2 above actually mean that you CANNOT be sure, because you are "fine with the state potentially not being included"! Otherwise second one would have been redundant. So basically in theory you must also use that "you don't know wether the might have alerady been included" mentioned for *_get_state here as well. > > That's just atomic 101. Nope, it is not even that. It is 1st order logic here. Stan > > -- > Ville Syrjälä > Intel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix NULL ptr deref by checking new_crtc_state 2023-05-05 13:21 ` Lisovskiy, Stanislav @ 2023-05-05 13:28 ` Ville Syrjälä 2023-05-05 13:42 ` Lisovskiy, Stanislav 0 siblings, 1 reply; 30+ messages in thread From: Ville Syrjälä @ 2023-05-05 13:28 UTC (permalink / raw) To: Lisovskiy, Stanislav; +Cc: intel-gfx On Fri, May 05, 2023 at 04:21:16PM +0300, Lisovskiy, Stanislav wrote: > On Fri, May 05, 2023 at 04:11:52PM +0300, Ville Syrjälä wrote: > > On Fri, May 05, 2023 at 03:54:58PM +0300, Lisovskiy, Stanislav wrote: > > > On Fri, May 05, 2023 at 03:46:40PM +0300, Ville Syrjälä wrote: > > > > On Fri, May 05, 2023 at 03:27:51PM +0300, Lisovskiy, Stanislav wrote: > > > > > On Fri, May 05, 2023 at 03:09:01PM +0300, Ville Syrjälä wrote: > > > > > > On Fri, May 05, 2023 at 02:41:24PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > On Fri, May 05, 2023 at 02:25:46PM +0300, Ville Syrjälä wrote: > > > > > > > > On Fri, May 05, 2023 at 02:20:17PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > On Fri, May 05, 2023 at 02:06:34PM +0300, Ville Syrjälä wrote: > > > > > > > > > > On Fri, May 05, 2023 at 02:05:27PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > > > On Fri, May 05, 2023 at 02:02:43PM +0300, Ville Syrjälä wrote: > > > > > > > > > > > > On Fri, May 05, 2023 at 01:58:03PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > > > > > On Fri, May 05, 2023 at 01:54:14PM +0300, Ville Syrjälä wrote: > > > > > > > > > > > > > > On Fri, May 05, 2023 at 11:22:12AM +0300, Stanislav Lisovskiy wrote: > > > > > > > > > > > > > > > intel_atomic_get_new_crtc_state can return NULL, unless crtc state wasn't > > > > > > > > > > > > > > > obtained previously with intel_atomic_get_crtc_state, so we must check it > > > > > > > > > > > > > > > for NULLness here, just as in many other places, where we can't guarantee > > > > > > > > > > > > > > > that intel_atomic_get_crtc_state was called. > > > > > > > > > > > > > > > We are currently getting NULL ptr deref because of that, so this fix was > > > > > > > > > > > > > > > confirmed to help. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Fixes: 74a75dc90869 ("drm/i915/display: move plane prepare/cleanup to intel_atomic_plane.c") > > > > > > > > > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > > drivers/gpu/drm/i915/display/intel_atomic_plane.c | 4 ++-- > > > > > > > > > > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > > > > > > index 9f670dcfe76e..4125ee07a271 100644 > > > > > > > > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > > > > > > @@ -1029,7 +1029,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > > > > > > > > > > > > int ret; > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > if (old_obj) { > > > > > > > > > > > > > > > - const struct intel_crtc_state *crtc_state = > > > > > > > > > > > > > > > + const struct intel_crtc_state *new_crtc_state = > > > > > > > > > > > > > > > intel_atomic_get_new_crtc_state(state, > > > > > > > > > > > > > > > to_intel_crtc(old_plane_state->hw.crtc)); > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > @@ -1044,7 +1044,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > > > > > > > > > > > > * This should only fail upon a hung GPU, in which case we > > > > > > > > > > > > > > > * can safely continue. > > > > > > > > > > > > > > > */ > > > > > > > > > > > > > > > - if (intel_crtc_needs_modeset(crtc_state)) { > > > > > > > > > > > > > > > + if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state)) { > > > > > > > > > > > > > > > > > > > > > > > > > > > > NAK. We need to fix the bug instead of paparing over it. > > > > > > > > > > > > > > > > > > > > > > > > > > I had pushed this already. > > > > > > > > > > > > > > > > > > > > > > > > It didn't even finish CI. Please revert. > > > > > > > > > > > > > > > > > > > > > > Swati did run CI and verified that fix helps. I'm _not_ going to revert. > > > > > > > > > > > > > > > > > > > > Fine. I'll do it. > > > > > > > > > > > > > > > > > > Problem is that you don't even care to explain, why this fix is wrong, but simply > > > > > > > > > act in authoritarian way, instead of having constructive discussion. > > > > > > > > > > > > > > > > I've explanined this one about a hundred times. The NULL pointer should > > > > > > > > not happen. Someone needs to actually analyze what is happening instead > > > > > > > > of just adding randomg NULL checks all over the place. > > > > > > > > > > > > > > I do get this point. However why are we doing those check in other places then? > > > > > > > > > > > > We do then when they are actually necessary. > > > > > > > > > > Well but for example when we do check like if(new_bw_state) in intel_bw.c, > > > > > we are also might be having potentially some silent bugs. > > > > > Would you guarantee that if we remove all if(crtc_state) and if(new_bw_state) checks > > > > > in our code, that there won't be NULL pointer dereferences? I bet you don't. > > > > > > > > We have the checks where they are needed. The check in > > > > intel_bw_atomic_check() (if that's the one you mean) > > > > looks entirely correct to me. > > > > > > Typo in my prev message, I meant intel_atomic_get_bw_state..but common idea is the same. > > > > get_state() vs. get_{new,old}_state() are entirely different > > things. > > > > You use get_state() when you really want the state to be > > included, and either > > - know the state isn't included already, or > > - you don't know wether the might have alerady been included > > > > And one must of course remember that get_state() can > > - fail so error handling is needed > > - only be used during the check phase, and is illegal during the > > commit phase. > > Sure I know this. I even remember we discussed this many times. > > > > > The get_{new,old}_state() (or the various for loop variants) > > you can use when you either: > > - know that the state is included already > > - are fine with the state potentially not being included > > Don't you see that it is a bit of a contradiction in those 2 above?? > > You can't be "know that the state is included already" and > "are fine with the state potentially not being included" same time :) > > Those 2 above actually mean that you CANNOT be sure, because you > are "fine with the state potentially not being included"! > Otherwise second one would have been redundant. No. You are either fine with NULL, XOR you know that the state is there already. There is no contradiction. > > So basically in theory you must also use that > "you don't know wether the might have alerady been included" mentioned > for *_get_state here as well. No. This code _expects_ the state to be included. The fact that it is not is a problem. > > > > > That's just atomic 101. > > Nope, it is not even that. It is 1st order logic here. > > Stan > > > > > -- > > Ville Syrjälä > > Intel -- Ville Syrjälä Intel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix NULL ptr deref by checking new_crtc_state 2023-05-05 13:28 ` Ville Syrjälä @ 2023-05-05 13:42 ` Lisovskiy, Stanislav 2023-05-05 13:57 ` Ville Syrjälä 0 siblings, 1 reply; 30+ messages in thread From: Lisovskiy, Stanislav @ 2023-05-05 13:42 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx On Fri, May 05, 2023 at 04:28:50PM +0300, Ville Syrjälä wrote: > On Fri, May 05, 2023 at 04:21:16PM +0300, Lisovskiy, Stanislav wrote: > > On Fri, May 05, 2023 at 04:11:52PM +0300, Ville Syrjälä wrote: > > > On Fri, May 05, 2023 at 03:54:58PM +0300, Lisovskiy, Stanislav wrote: > > > > On Fri, May 05, 2023 at 03:46:40PM +0300, Ville Syrjälä wrote: > > > > > On Fri, May 05, 2023 at 03:27:51PM +0300, Lisovskiy, Stanislav wrote: > > > > > > On Fri, May 05, 2023 at 03:09:01PM +0300, Ville Syrjälä wrote: > > > > > > > On Fri, May 05, 2023 at 02:41:24PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > On Fri, May 05, 2023 at 02:25:46PM +0300, Ville Syrjälä wrote: > > > > > > > > > On Fri, May 05, 2023 at 02:20:17PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > > On Fri, May 05, 2023 at 02:06:34PM +0300, Ville Syrjälä wrote: > > > > > > > > > > > On Fri, May 05, 2023 at 02:05:27PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > > > > On Fri, May 05, 2023 at 02:02:43PM +0300, Ville Syrjälä wrote: > > > > > > > > > > > > > On Fri, May 05, 2023 at 01:58:03PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > > > > > > On Fri, May 05, 2023 at 01:54:14PM +0300, Ville Syrjälä wrote: > > > > > > > > > > > > > > > On Fri, May 05, 2023 at 11:22:12AM +0300, Stanislav Lisovskiy wrote: > > > > > > > > > > > > > > > > intel_atomic_get_new_crtc_state can return NULL, unless crtc state wasn't > > > > > > > > > > > > > > > > obtained previously with intel_atomic_get_crtc_state, so we must check it > > > > > > > > > > > > > > > > for NULLness here, just as in many other places, where we can't guarantee > > > > > > > > > > > > > > > > that intel_atomic_get_crtc_state was called. > > > > > > > > > > > > > > > > We are currently getting NULL ptr deref because of that, so this fix was > > > > > > > > > > > > > > > > confirmed to help. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Fixes: 74a75dc90869 ("drm/i915/display: move plane prepare/cleanup to intel_atomic_plane.c") > > > > > > > > > > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > > > drivers/gpu/drm/i915/display/intel_atomic_plane.c | 4 ++-- > > > > > > > > > > > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > > > > > > > index 9f670dcfe76e..4125ee07a271 100644 > > > > > > > > > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > > > > > > > @@ -1029,7 +1029,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > > > > > > > > > > > > > int ret; > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > if (old_obj) { > > > > > > > > > > > > > > > > - const struct intel_crtc_state *crtc_state = > > > > > > > > > > > > > > > > + const struct intel_crtc_state *new_crtc_state = > > > > > > > > > > > > > > > > intel_atomic_get_new_crtc_state(state, > > > > > > > > > > > > > > > > to_intel_crtc(old_plane_state->hw.crtc)); > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > @@ -1044,7 +1044,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > > > > > > > > > > > > > * This should only fail upon a hung GPU, in which case we > > > > > > > > > > > > > > > > * can safely continue. > > > > > > > > > > > > > > > > */ > > > > > > > > > > > > > > > > - if (intel_crtc_needs_modeset(crtc_state)) { > > > > > > > > > > > > > > > > + if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state)) { > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > NAK. We need to fix the bug instead of paparing over it. > > > > > > > > > > > > > > > > > > > > > > > > > > > > I had pushed this already. > > > > > > > > > > > > > > > > > > > > > > > > > > It didn't even finish CI. Please revert. > > > > > > > > > > > > > > > > > > > > > > > > Swati did run CI and verified that fix helps. I'm _not_ going to revert. > > > > > > > > > > > > > > > > > > > > > > Fine. I'll do it. > > > > > > > > > > > > > > > > > > > > Problem is that you don't even care to explain, why this fix is wrong, but simply > > > > > > > > > > act in authoritarian way, instead of having constructive discussion. > > > > > > > > > > > > > > > > > > I've explanined this one about a hundred times. The NULL pointer should > > > > > > > > > not happen. Someone needs to actually analyze what is happening instead > > > > > > > > > of just adding randomg NULL checks all over the place. > > > > > > > > > > > > > > > > I do get this point. However why are we doing those check in other places then? > > > > > > > > > > > > > > We do then when they are actually necessary. > > > > > > > > > > > > Well but for example when we do check like if(new_bw_state) in intel_bw.c, > > > > > > we are also might be having potentially some silent bugs. > > > > > > Would you guarantee that if we remove all if(crtc_state) and if(new_bw_state) checks > > > > > > in our code, that there won't be NULL pointer dereferences? I bet you don't. > > > > > > > > > > We have the checks where they are needed. The check in > > > > > intel_bw_atomic_check() (if that's the one you mean) > > > > > looks entirely correct to me. > > > > > > > > Typo in my prev message, I meant intel_atomic_get_bw_state..but common idea is the same. > > > > > > get_state() vs. get_{new,old}_state() are entirely different > > > things. > > > > > > You use get_state() when you really want the state to be > > > included, and either > > > - know the state isn't included already, or > > > - you don't know wether the might have alerady been included > > > > > > And one must of course remember that get_state() can > > > - fail so error handling is needed > > > - only be used during the check phase, and is illegal during the > > > commit phase. > > > > Sure I know this. I even remember we discussed this many times. > > > > > > > > The get_{new,old}_state() (or the various for loop variants) > > > you can use when you either: > > > - know that the state is included already > > > - are fine with the state potentially not being included > > > > Don't you see that it is a bit of a contradiction in those 2 above?? > > > > You can't be "know that the state is included already" and > > "are fine with the state potentially not being included" same time :) > > > > Those 2 above actually mean that you CANNOT be sure, because you > > are "fine with the state potentially not being included"! > > Otherwise second one would have been redundant. > > No. You are either fine with NULL, XOR you know that > the state is there already. There is no contradiction. I do get that. But that way of calling the function is veeery counterintuitive. Means that you call it and check for NULLness..if you are fine with NULL and don't check for NULL..if you aren't fine with it and expect the state to be there. That is really probabilistic design. I think we must enumerate all the cases where 1) we expect new_state to be there and then we don't need even any checks to be there, because we will then rely on get_state. 2) we don't expect it to be there and then call get_state always. Because if you are "fine" with new_state being NULL, why even calling it? I don't get what means "fine" here - there should be a reason why it is not there right? crtc wasn't added to the state. get_crtc_state wasn't called - it means either we don't even need to call get_new_*state at all or there _is_ a problem. Stan > > > > > So basically in theory you must also use that > > "you don't know wether the might have alerady been included" mentioned > > for *_get_state here as well. > > No. This code _expects_ the state to be included. The fact > that it is not is a problem. > > > > > > > > > That's just atomic 101. > > > > Nope, it is not even that. It is 1st order logic here. > > > > Stan > > > > > > > > -- > > > Ville Syrjälä > > > Intel > > -- > Ville Syrjälä > Intel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix NULL ptr deref by checking new_crtc_state 2023-05-05 13:42 ` Lisovskiy, Stanislav @ 2023-05-05 13:57 ` Ville Syrjälä 2023-05-05 14:05 ` Lisovskiy, Stanislav 0 siblings, 1 reply; 30+ messages in thread From: Ville Syrjälä @ 2023-05-05 13:57 UTC (permalink / raw) To: Lisovskiy, Stanislav; +Cc: intel-gfx On Fri, May 05, 2023 at 04:42:33PM +0300, Lisovskiy, Stanislav wrote: > On Fri, May 05, 2023 at 04:28:50PM +0300, Ville Syrjälä wrote: > > On Fri, May 05, 2023 at 04:21:16PM +0300, Lisovskiy, Stanislav wrote: > > > On Fri, May 05, 2023 at 04:11:52PM +0300, Ville Syrjälä wrote: > > > > On Fri, May 05, 2023 at 03:54:58PM +0300, Lisovskiy, Stanislav wrote: > > > > > On Fri, May 05, 2023 at 03:46:40PM +0300, Ville Syrjälä wrote: > > > > > > On Fri, May 05, 2023 at 03:27:51PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > On Fri, May 05, 2023 at 03:09:01PM +0300, Ville Syrjälä wrote: > > > > > > > > On Fri, May 05, 2023 at 02:41:24PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > On Fri, May 05, 2023 at 02:25:46PM +0300, Ville Syrjälä wrote: > > > > > > > > > > On Fri, May 05, 2023 at 02:20:17PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > > > On Fri, May 05, 2023 at 02:06:34PM +0300, Ville Syrjälä wrote: > > > > > > > > > > > > On Fri, May 05, 2023 at 02:05:27PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > > > > > On Fri, May 05, 2023 at 02:02:43PM +0300, Ville Syrjälä wrote: > > > > > > > > > > > > > > On Fri, May 05, 2023 at 01:58:03PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > > > > > > > On Fri, May 05, 2023 at 01:54:14PM +0300, Ville Syrjälä wrote: > > > > > > > > > > > > > > > > On Fri, May 05, 2023 at 11:22:12AM +0300, Stanislav Lisovskiy wrote: > > > > > > > > > > > > > > > > > intel_atomic_get_new_crtc_state can return NULL, unless crtc state wasn't > > > > > > > > > > > > > > > > > obtained previously with intel_atomic_get_crtc_state, so we must check it > > > > > > > > > > > > > > > > > for NULLness here, just as in many other places, where we can't guarantee > > > > > > > > > > > > > > > > > that intel_atomic_get_crtc_state was called. > > > > > > > > > > > > > > > > > We are currently getting NULL ptr deref because of that, so this fix was > > > > > > > > > > > > > > > > > confirmed to help. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Fixes: 74a75dc90869 ("drm/i915/display: move plane prepare/cleanup to intel_atomic_plane.c") > > > > > > > > > > > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > > > > drivers/gpu/drm/i915/display/intel_atomic_plane.c | 4 ++-- > > > > > > > > > > > > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > > > > > > > > index 9f670dcfe76e..4125ee07a271 100644 > > > > > > > > > > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > > > > > > > > @@ -1029,7 +1029,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > > > > > > > > > > > > > > int ret; > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > if (old_obj) { > > > > > > > > > > > > > > > > > - const struct intel_crtc_state *crtc_state = > > > > > > > > > > > > > > > > > + const struct intel_crtc_state *new_crtc_state = > > > > > > > > > > > > > > > > > intel_atomic_get_new_crtc_state(state, > > > > > > > > > > > > > > > > > to_intel_crtc(old_plane_state->hw.crtc)); > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > @@ -1044,7 +1044,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > > > > > > > > > > > > > > * This should only fail upon a hung GPU, in which case we > > > > > > > > > > > > > > > > > * can safely continue. > > > > > > > > > > > > > > > > > */ > > > > > > > > > > > > > > > > > - if (intel_crtc_needs_modeset(crtc_state)) { > > > > > > > > > > > > > > > > > + if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state)) { > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > NAK. We need to fix the bug instead of paparing over it. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I had pushed this already. > > > > > > > > > > > > > > > > > > > > > > > > > > > > It didn't even finish CI. Please revert. > > > > > > > > > > > > > > > > > > > > > > > > > > Swati did run CI and verified that fix helps. I'm _not_ going to revert. > > > > > > > > > > > > > > > > > > > > > > > > Fine. I'll do it. > > > > > > > > > > > > > > > > > > > > > > Problem is that you don't even care to explain, why this fix is wrong, but simply > > > > > > > > > > > act in authoritarian way, instead of having constructive discussion. > > > > > > > > > > > > > > > > > > > > I've explanined this one about a hundred times. The NULL pointer should > > > > > > > > > > not happen. Someone needs to actually analyze what is happening instead > > > > > > > > > > of just adding randomg NULL checks all over the place. > > > > > > > > > > > > > > > > > > I do get this point. However why are we doing those check in other places then? > > > > > > > > > > > > > > > > We do then when they are actually necessary. > > > > > > > > > > > > > > Well but for example when we do check like if(new_bw_state) in intel_bw.c, > > > > > > > we are also might be having potentially some silent bugs. > > > > > > > Would you guarantee that if we remove all if(crtc_state) and if(new_bw_state) checks > > > > > > > in our code, that there won't be NULL pointer dereferences? I bet you don't. > > > > > > > > > > > > We have the checks where they are needed. The check in > > > > > > intel_bw_atomic_check() (if that's the one you mean) > > > > > > looks entirely correct to me. > > > > > > > > > > Typo in my prev message, I meant intel_atomic_get_bw_state..but common idea is the same. > > > > > > > > get_state() vs. get_{new,old}_state() are entirely different > > > > things. > > > > > > > > You use get_state() when you really want the state to be > > > > included, and either > > > > - know the state isn't included already, or > > > > - you don't know wether the might have alerady been included > > > > > > > > And one must of course remember that get_state() can > > > > - fail so error handling is needed > > > > - only be used during the check phase, and is illegal during the > > > > commit phase. > > > > > > Sure I know this. I even remember we discussed this many times. > > > > > > > > > > > The get_{new,old}_state() (or the various for loop variants) > > > > you can use when you either: > > > > - know that the state is included already > > > > - are fine with the state potentially not being included > > > > > > Don't you see that it is a bit of a contradiction in those 2 above?? > > > > > > You can't be "know that the state is included already" and > > > "are fine with the state potentially not being included" same time :) > > > > > > Those 2 above actually mean that you CANNOT be sure, because you > > > are "fine with the state potentially not being included"! > > > Otherwise second one would have been redundant. > > > > No. You are either fine with NULL, XOR you know that > > the state is there already. There is no contradiction. > > I do get that. But that way of calling the function is veeery counterintuitive. > Means that you call it and check for NULLness..if you are fine with NULL and > don't check for NULL..if you aren't fine with it and expect the state to be there. > > That is really probabilistic design. > I think we must enumerate all the cases where Not sure what you mean with enumerate. You can't just delcare somewhere globally that in functions X and Y NULL is fine, and in Z it is not. It depends on how X,Y,Z are implemented and it may change any time the implementation is changed. > 1) we expect new_state to be there and > then we don't need even any checks to be there, because we will then rely on get_state. > 2) we don't expect it to be there and then call get_state always. > > Because if you are "fine" with new_state being NULL, why even calling it? Because !NULL -> you have some work to do NULL -> you don't have work to do > I don't get what means "fine" here - there should be a reason why it is not there right? > crtc wasn't added to the state. get_crtc_state wasn't called - it means either we don't even > need to call get_new_*state at all or there _is_ a problem. > > Stan > > > > > > > > > So basically in theory you must also use that > > > "you don't know wether the might have alerady been included" mentioned > > > for *_get_state here as well. > > > > No. This code _expects_ the state to be included. The fact > > that it is not is a problem. > > > > > > > > > > > > > That's just atomic 101. > > > > > > Nope, it is not even that. It is 1st order logic here. > > > > > > Stan > > > > > > > > > > > -- > > > > Ville Syrjälä > > > > Intel > > > > -- > > Ville Syrjälä > > Intel -- Ville Syrjälä Intel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix NULL ptr deref by checking new_crtc_state 2023-05-05 13:57 ` Ville Syrjälä @ 2023-05-05 14:05 ` Lisovskiy, Stanislav 2023-05-05 14:17 ` Ville Syrjälä 0 siblings, 1 reply; 30+ messages in thread From: Lisovskiy, Stanislav @ 2023-05-05 14:05 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx On Fri, May 05, 2023 at 04:57:54PM +0300, Ville Syrjälä wrote: > On Fri, May 05, 2023 at 04:42:33PM +0300, Lisovskiy, Stanislav wrote: > > On Fri, May 05, 2023 at 04:28:50PM +0300, Ville Syrjälä wrote: > > > On Fri, May 05, 2023 at 04:21:16PM +0300, Lisovskiy, Stanislav wrote: > > > > On Fri, May 05, 2023 at 04:11:52PM +0300, Ville Syrjälä wrote: > > > > > On Fri, May 05, 2023 at 03:54:58PM +0300, Lisovskiy, Stanislav wrote: > > > > > > On Fri, May 05, 2023 at 03:46:40PM +0300, Ville Syrjälä wrote: > > > > > > > On Fri, May 05, 2023 at 03:27:51PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > On Fri, May 05, 2023 at 03:09:01PM +0300, Ville Syrjälä wrote: > > > > > > > > > On Fri, May 05, 2023 at 02:41:24PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > > On Fri, May 05, 2023 at 02:25:46PM +0300, Ville Syrjälä wrote: > > > > > > > > > > > On Fri, May 05, 2023 at 02:20:17PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > > > > On Fri, May 05, 2023 at 02:06:34PM +0300, Ville Syrjälä wrote: > > > > > > > > > > > > > On Fri, May 05, 2023 at 02:05:27PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > > > > > > On Fri, May 05, 2023 at 02:02:43PM +0300, Ville Syrjälä wrote: > > > > > > > > > > > > > > > On Fri, May 05, 2023 at 01:58:03PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > > > > > > > > On Fri, May 05, 2023 at 01:54:14PM +0300, Ville Syrjälä wrote: > > > > > > > > > > > > > > > > > On Fri, May 05, 2023 at 11:22:12AM +0300, Stanislav Lisovskiy wrote: > > > > > > > > > > > > > > > > > > intel_atomic_get_new_crtc_state can return NULL, unless crtc state wasn't > > > > > > > > > > > > > > > > > > obtained previously with intel_atomic_get_crtc_state, so we must check it > > > > > > > > > > > > > > > > > > for NULLness here, just as in many other places, where we can't guarantee > > > > > > > > > > > > > > > > > > that intel_atomic_get_crtc_state was called. > > > > > > > > > > > > > > > > > > We are currently getting NULL ptr deref because of that, so this fix was > > > > > > > > > > > > > > > > > > confirmed to help. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Fixes: 74a75dc90869 ("drm/i915/display: move plane prepare/cleanup to intel_atomic_plane.c") > > > > > > > > > > > > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > > > > > drivers/gpu/drm/i915/display/intel_atomic_plane.c | 4 ++-- > > > > > > > > > > > > > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > > > > > > > > > index 9f670dcfe76e..4125ee07a271 100644 > > > > > > > > > > > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > > > > > > > > > @@ -1029,7 +1029,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > > > > > > > > > > > > > > > int ret; > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > if (old_obj) { > > > > > > > > > > > > > > > > > > - const struct intel_crtc_state *crtc_state = > > > > > > > > > > > > > > > > > > + const struct intel_crtc_state *new_crtc_state = > > > > > > > > > > > > > > > > > > intel_atomic_get_new_crtc_state(state, > > > > > > > > > > > > > > > > > > to_intel_crtc(old_plane_state->hw.crtc)); > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > @@ -1044,7 +1044,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > > > > > > > > > > > > > > > * This should only fail upon a hung GPU, in which case we > > > > > > > > > > > > > > > > > > * can safely continue. > > > > > > > > > > > > > > > > > > */ > > > > > > > > > > > > > > > > > > - if (intel_crtc_needs_modeset(crtc_state)) { > > > > > > > > > > > > > > > > > > + if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state)) { > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > NAK. We need to fix the bug instead of paparing over it. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I had pushed this already. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > It didn't even finish CI. Please revert. > > > > > > > > > > > > > > > > > > > > > > > > > > > > Swati did run CI and verified that fix helps. I'm _not_ going to revert. > > > > > > > > > > > > > > > > > > > > > > > > > > Fine. I'll do it. > > > > > > > > > > > > > > > > > > > > > > > > Problem is that you don't even care to explain, why this fix is wrong, but simply > > > > > > > > > > > > act in authoritarian way, instead of having constructive discussion. > > > > > > > > > > > > > > > > > > > > > > I've explanined this one about a hundred times. The NULL pointer should > > > > > > > > > > > not happen. Someone needs to actually analyze what is happening instead > > > > > > > > > > > of just adding randomg NULL checks all over the place. > > > > > > > > > > > > > > > > > > > > I do get this point. However why are we doing those check in other places then? > > > > > > > > > > > > > > > > > > We do then when they are actually necessary. > > > > > > > > > > > > > > > > Well but for example when we do check like if(new_bw_state) in intel_bw.c, > > > > > > > > we are also might be having potentially some silent bugs. > > > > > > > > Would you guarantee that if we remove all if(crtc_state) and if(new_bw_state) checks > > > > > > > > in our code, that there won't be NULL pointer dereferences? I bet you don't. > > > > > > > > > > > > > > We have the checks where they are needed. The check in > > > > > > > intel_bw_atomic_check() (if that's the one you mean) > > > > > > > looks entirely correct to me. > > > > > > > > > > > > Typo in my prev message, I meant intel_atomic_get_bw_state..but common idea is the same. > > > > > > > > > > get_state() vs. get_{new,old}_state() are entirely different > > > > > things. > > > > > > > > > > You use get_state() when you really want the state to be > > > > > included, and either > > > > > - know the state isn't included already, or > > > > > - you don't know wether the might have alerady been included > > > > > > > > > > And one must of course remember that get_state() can > > > > > - fail so error handling is needed > > > > > - only be used during the check phase, and is illegal during the > > > > > commit phase. > > > > > > > > Sure I know this. I even remember we discussed this many times. > > > > > > > > > > > > > > The get_{new,old}_state() (or the various for loop variants) > > > > > you can use when you either: > > > > > - know that the state is included already > > > > > - are fine with the state potentially not being included > > > > > > > > Don't you see that it is a bit of a contradiction in those 2 above?? > > > > > > > > You can't be "know that the state is included already" and > > > > "are fine with the state potentially not being included" same time :) > > > > > > > > Those 2 above actually mean that you CANNOT be sure, because you > > > > are "fine with the state potentially not being included"! > > > > Otherwise second one would have been redundant. > > > > > > No. You are either fine with NULL, XOR you know that > > > the state is there already. There is no contradiction. > > > > I do get that. But that way of calling the function is veeery counterintuitive. > > Means that you call it and check for NULLness..if you are fine with NULL and > > don't check for NULL..if you aren't fine with it and expect the state to be there. > > > > That is really probabilistic design. > > I think we must enumerate all the cases where > > Not sure what you mean with enumerate. You can't just delcare > somewhere globally that in functions X and Y NULL is fine, > and in Z it is not. It depends on how X,Y,Z are implemented > and it may change any time the implementation is changed. > > > > 1) we expect new_state to be there and > > then we don't need even any checks to be there, because we will then rely on get_state. > > 2) we don't expect it to be there and then call get_state always. > > > > Because if you are "fine" with new_state being NULL, why even calling it? > > Because > !NULL -> you have some work to do > NULL -> you don't have work to do Pretty sure we could find a way not to call it at all in case if no work is needed, and call it without any checks, if work is needed. You typically get new bw state to recalculate and compare with old state, however there has to be some place where you decide whether to call get_bw/crtc_state or not. So from there, this could have been propagated to the moment where we decide where to call get_new_bw/crtc_state or not. Then no checks would have been needed. And NULL would always mean a bug. Also that would be a lot more simple, following KISS principle. Stan > > > I don't get what means "fine" here - there should be a reason why it is not there right? > > crtc wasn't added to the state. get_crtc_state wasn't called - it means either we don't even > > need to call get_new_*state at all or there _is_ a problem. > > > > Stan > > > > > > > > > > > > > So basically in theory you must also use that > > > > "you don't know wether the might have alerady been included" mentioned > > > > for *_get_state here as well. > > > > > > No. This code _expects_ the state to be included. The fact > > > that it is not is a problem. > > > > > > > > > > > > > > > > > That's just atomic 101. > > > > > > > > Nope, it is not even that. It is 1st order logic here. > > > > > > > > Stan > > > > > > > > > > > > > > -- > > > > > Ville Syrjälä > > > > > Intel > > > > > > -- > > > Ville Syrjälä > > > Intel > > -- > Ville Syrjälä > Intel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix NULL ptr deref by checking new_crtc_state 2023-05-05 14:05 ` Lisovskiy, Stanislav @ 2023-05-05 14:17 ` Ville Syrjälä 2023-05-05 15:55 ` Lisovskiy, Stanislav 0 siblings, 1 reply; 30+ messages in thread From: Ville Syrjälä @ 2023-05-05 14:17 UTC (permalink / raw) To: Lisovskiy, Stanislav; +Cc: intel-gfx On Fri, May 05, 2023 at 05:05:55PM +0300, Lisovskiy, Stanislav wrote: > On Fri, May 05, 2023 at 04:57:54PM +0300, Ville Syrjälä wrote: > > On Fri, May 05, 2023 at 04:42:33PM +0300, Lisovskiy, Stanislav wrote: > > > On Fri, May 05, 2023 at 04:28:50PM +0300, Ville Syrjälä wrote: > > > > On Fri, May 05, 2023 at 04:21:16PM +0300, Lisovskiy, Stanislav wrote: > > > > > On Fri, May 05, 2023 at 04:11:52PM +0300, Ville Syrjälä wrote: > > > > > > On Fri, May 05, 2023 at 03:54:58PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > On Fri, May 05, 2023 at 03:46:40PM +0300, Ville Syrjälä wrote: > > > > > > > > On Fri, May 05, 2023 at 03:27:51PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > On Fri, May 05, 2023 at 03:09:01PM +0300, Ville Syrjälä wrote: > > > > > > > > > > On Fri, May 05, 2023 at 02:41:24PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > > > On Fri, May 05, 2023 at 02:25:46PM +0300, Ville Syrjälä wrote: > > > > > > > > > > > > On Fri, May 05, 2023 at 02:20:17PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > > > > > On Fri, May 05, 2023 at 02:06:34PM +0300, Ville Syrjälä wrote: > > > > > > > > > > > > > > On Fri, May 05, 2023 at 02:05:27PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > > > > > > > On Fri, May 05, 2023 at 02:02:43PM +0300, Ville Syrjälä wrote: > > > > > > > > > > > > > > > > On Fri, May 05, 2023 at 01:58:03PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > > > > > > > > > On Fri, May 05, 2023 at 01:54:14PM +0300, Ville Syrjälä wrote: > > > > > > > > > > > > > > > > > > On Fri, May 05, 2023 at 11:22:12AM +0300, Stanislav Lisovskiy wrote: > > > > > > > > > > > > > > > > > > > intel_atomic_get_new_crtc_state can return NULL, unless crtc state wasn't > > > > > > > > > > > > > > > > > > > obtained previously with intel_atomic_get_crtc_state, so we must check it > > > > > > > > > > > > > > > > > > > for NULLness here, just as in many other places, where we can't guarantee > > > > > > > > > > > > > > > > > > > that intel_atomic_get_crtc_state was called. > > > > > > > > > > > > > > > > > > > We are currently getting NULL ptr deref because of that, so this fix was > > > > > > > > > > > > > > > > > > > confirmed to help. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Fixes: 74a75dc90869 ("drm/i915/display: move plane prepare/cleanup to intel_atomic_plane.c") > > > > > > > > > > > > > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > > > > > > drivers/gpu/drm/i915/display/intel_atomic_plane.c | 4 ++-- > > > > > > > > > > > > > > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > > > > > > > > > > index 9f670dcfe76e..4125ee07a271 100644 > > > > > > > > > > > > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > > > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > > > > > > > > > > @@ -1029,7 +1029,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > > > > > > > > > > > > > > > > int ret; > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > if (old_obj) { > > > > > > > > > > > > > > > > > > > - const struct intel_crtc_state *crtc_state = > > > > > > > > > > > > > > > > > > > + const struct intel_crtc_state *new_crtc_state = > > > > > > > > > > > > > > > > > > > intel_atomic_get_new_crtc_state(state, > > > > > > > > > > > > > > > > > > > to_intel_crtc(old_plane_state->hw.crtc)); > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > @@ -1044,7 +1044,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > > > > > > > > > > > > > > > > * This should only fail upon a hung GPU, in which case we > > > > > > > > > > > > > > > > > > > * can safely continue. > > > > > > > > > > > > > > > > > > > */ > > > > > > > > > > > > > > > > > > > - if (intel_crtc_needs_modeset(crtc_state)) { > > > > > > > > > > > > > > > > > > > + if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state)) { > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > NAK. We need to fix the bug instead of paparing over it. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I had pushed this already. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > It didn't even finish CI. Please revert. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Swati did run CI and verified that fix helps. I'm _not_ going to revert. > > > > > > > > > > > > > > > > > > > > > > > > > > > > Fine. I'll do it. > > > > > > > > > > > > > > > > > > > > > > > > > > Problem is that you don't even care to explain, why this fix is wrong, but simply > > > > > > > > > > > > > act in authoritarian way, instead of having constructive discussion. > > > > > > > > > > > > > > > > > > > > > > > > I've explanined this one about a hundred times. The NULL pointer should > > > > > > > > > > > > not happen. Someone needs to actually analyze what is happening instead > > > > > > > > > > > > of just adding randomg NULL checks all over the place. > > > > > > > > > > > > > > > > > > > > > > I do get this point. However why are we doing those check in other places then? > > > > > > > > > > > > > > > > > > > > We do then when they are actually necessary. > > > > > > > > > > > > > > > > > > Well but for example when we do check like if(new_bw_state) in intel_bw.c, > > > > > > > > > we are also might be having potentially some silent bugs. > > > > > > > > > Would you guarantee that if we remove all if(crtc_state) and if(new_bw_state) checks > > > > > > > > > in our code, that there won't be NULL pointer dereferences? I bet you don't. > > > > > > > > > > > > > > > > We have the checks where they are needed. The check in > > > > > > > > intel_bw_atomic_check() (if that's the one you mean) > > > > > > > > looks entirely correct to me. > > > > > > > > > > > > > > Typo in my prev message, I meant intel_atomic_get_bw_state..but common idea is the same. > > > > > > > > > > > > get_state() vs. get_{new,old}_state() are entirely different > > > > > > things. > > > > > > > > > > > > You use get_state() when you really want the state to be > > > > > > included, and either > > > > > > - know the state isn't included already, or > > > > > > - you don't know wether the might have alerady been included > > > > > > > > > > > > And one must of course remember that get_state() can > > > > > > - fail so error handling is needed > > > > > > - only be used during the check phase, and is illegal during the > > > > > > commit phase. > > > > > > > > > > Sure I know this. I even remember we discussed this many times. > > > > > > > > > > > > > > > > > The get_{new,old}_state() (or the various for loop variants) > > > > > > you can use when you either: > > > > > > - know that the state is included already > > > > > > - are fine with the state potentially not being included > > > > > > > > > > Don't you see that it is a bit of a contradiction in those 2 above?? > > > > > > > > > > You can't be "know that the state is included already" and > > > > > "are fine with the state potentially not being included" same time :) > > > > > > > > > > Those 2 above actually mean that you CANNOT be sure, because you > > > > > are "fine with the state potentially not being included"! > > > > > Otherwise second one would have been redundant. > > > > > > > > No. You are either fine with NULL, XOR you know that > > > > the state is there already. There is no contradiction. > > > > > > I do get that. But that way of calling the function is veeery counterintuitive. > > > Means that you call it and check for NULLness..if you are fine with NULL and > > > don't check for NULL..if you aren't fine with it and expect the state to be there. > > > > > > That is really probabilistic design. > > > I think we must enumerate all the cases where > > > > Not sure what you mean with enumerate. You can't just delcare > > somewhere globally that in functions X and Y NULL is fine, > > and in Z it is not. It depends on how X,Y,Z are implemented > > and it may change any time the implementation is changed. > > > > > > > 1) we expect new_state to be there and > > > then we don't need even any checks to be there, because we will then rely on get_state. > > > 2) we don't expect it to be there and then call get_state always. > > > > > > Because if you are "fine" with new_state being NULL, why even calling it? > > > > Because > > !NULL -> you have some work to do > > NULL -> you don't have work to do > > Pretty sure we could find a way not to call it at all in case if no work is needed, > and call it without any checks, if work is needed. > > You typically get new bw state to recalculate and compare with old state, however > there has to be some place where you decide whether to call get_bw/crtc_state or not. > So from there, this could have been propagated to the moment where we decide where > to call get_new_bw/crtc_state or not. Then no checks would have been needed. > And NULL would always mean a bug. > Also that would be a lot more simple, following KISS principle. You'd need to separately track each case in some boolean/etc. in the overall atomic state. Doable? Sure. Simpler? Don't see it. It's the exact same code with the NULL check just replaced with some other check. And you must additionally remember to sprinkle those bool assignments around. -- Ville Syrjälä Intel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix NULL ptr deref by checking new_crtc_state 2023-05-05 14:17 ` Ville Syrjälä @ 2023-05-05 15:55 ` Lisovskiy, Stanislav 2023-05-05 16:44 ` Ville Syrjälä 0 siblings, 1 reply; 30+ messages in thread From: Lisovskiy, Stanislav @ 2023-05-05 15:55 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx On Fri, May 05, 2023 at 05:17:06PM +0300, Ville Syrjälä wrote: > On Fri, May 05, 2023 at 05:05:55PM +0300, Lisovskiy, Stanislav wrote: > > On Fri, May 05, 2023 at 04:57:54PM +0300, Ville Syrjälä wrote: > > > On Fri, May 05, 2023 at 04:42:33PM +0300, Lisovskiy, Stanislav wrote: > > > > On Fri, May 05, 2023 at 04:28:50PM +0300, Ville Syrjälä wrote: > > > > > On Fri, May 05, 2023 at 04:21:16PM +0300, Lisovskiy, Stanislav wrote: > > > > > > On Fri, May 05, 2023 at 04:11:52PM +0300, Ville Syrjälä wrote: > > > > > > > On Fri, May 05, 2023 at 03:54:58PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > On Fri, May 05, 2023 at 03:46:40PM +0300, Ville Syrjälä wrote: > > > > > > > > > On Fri, May 05, 2023 at 03:27:51PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > > On Fri, May 05, 2023 at 03:09:01PM +0300, Ville Syrjälä wrote: > > > > > > > > > > > On Fri, May 05, 2023 at 02:41:24PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > > > > On Fri, May 05, 2023 at 02:25:46PM +0300, Ville Syrjälä wrote: > > > > > > > > > > > > > On Fri, May 05, 2023 at 02:20:17PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > > > > > > On Fri, May 05, 2023 at 02:06:34PM +0300, Ville Syrjälä wrote: > > > > > > > > > > > > > > > On Fri, May 05, 2023 at 02:05:27PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > > > > > > > > On Fri, May 05, 2023 at 02:02:43PM +0300, Ville Syrjälä wrote: > > > > > > > > > > > > > > > > > On Fri, May 05, 2023 at 01:58:03PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > > > > > > > > > > On Fri, May 05, 2023 at 01:54:14PM +0300, Ville Syrjälä wrote: > > > > > > > > > > > > > > > > > > > On Fri, May 05, 2023 at 11:22:12AM +0300, Stanislav Lisovskiy wrote: > > > > > > > > > > > > > > > > > > > > intel_atomic_get_new_crtc_state can return NULL, unless crtc state wasn't > > > > > > > > > > > > > > > > > > > > obtained previously with intel_atomic_get_crtc_state, so we must check it > > > > > > > > > > > > > > > > > > > > for NULLness here, just as in many other places, where we can't guarantee > > > > > > > > > > > > > > > > > > > > that intel_atomic_get_crtc_state was called. > > > > > > > > > > > > > > > > > > > > We are currently getting NULL ptr deref because of that, so this fix was > > > > > > > > > > > > > > > > > > > > confirmed to help. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Fixes: 74a75dc90869 ("drm/i915/display: move plane prepare/cleanup to intel_atomic_plane.c") > > > > > > > > > > > > > > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > > > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > > > > > > > drivers/gpu/drm/i915/display/intel_atomic_plane.c | 4 ++-- > > > > > > > > > > > > > > > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > > > > > > > > > > > index 9f670dcfe76e..4125ee07a271 100644 > > > > > > > > > > > > > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > > > > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > > > > > > > > > > > @@ -1029,7 +1029,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > > > > > > > > > > > > > > > > > int ret; > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > if (old_obj) { > > > > > > > > > > > > > > > > > > > > - const struct intel_crtc_state *crtc_state = > > > > > > > > > > > > > > > > > > > > + const struct intel_crtc_state *new_crtc_state = > > > > > > > > > > > > > > > > > > > > intel_atomic_get_new_crtc_state(state, > > > > > > > > > > > > > > > > > > > > to_intel_crtc(old_plane_state->hw.crtc)); > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > @@ -1044,7 +1044,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > > > > > > > > > > > > > > > > > * This should only fail upon a hung GPU, in which case we > > > > > > > > > > > > > > > > > > > > * can safely continue. > > > > > > > > > > > > > > > > > > > > */ > > > > > > > > > > > > > > > > > > > > - if (intel_crtc_needs_modeset(crtc_state)) { > > > > > > > > > > > > > > > > > > > > + if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state)) { > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > NAK. We need to fix the bug instead of paparing over it. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I had pushed this already. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > It didn't even finish CI. Please revert. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Swati did run CI and verified that fix helps. I'm _not_ going to revert. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Fine. I'll do it. > > > > > > > > > > > > > > > > > > > > > > > > > > > > Problem is that you don't even care to explain, why this fix is wrong, but simply > > > > > > > > > > > > > > act in authoritarian way, instead of having constructive discussion. > > > > > > > > > > > > > > > > > > > > > > > > > > I've explanined this one about a hundred times. The NULL pointer should > > > > > > > > > > > > > not happen. Someone needs to actually analyze what is happening instead > > > > > > > > > > > > > of just adding randomg NULL checks all over the place. > > > > > > > > > > > > > > > > > > > > > > > > I do get this point. However why are we doing those check in other places then? > > > > > > > > > > > > > > > > > > > > > > We do then when they are actually necessary. > > > > > > > > > > > > > > > > > > > > Well but for example when we do check like if(new_bw_state) in intel_bw.c, > > > > > > > > > > we are also might be having potentially some silent bugs. > > > > > > > > > > Would you guarantee that if we remove all if(crtc_state) and if(new_bw_state) checks > > > > > > > > > > in our code, that there won't be NULL pointer dereferences? I bet you don't. > > > > > > > > > > > > > > > > > > We have the checks where they are needed. The check in > > > > > > > > > intel_bw_atomic_check() (if that's the one you mean) > > > > > > > > > looks entirely correct to me. > > > > > > > > > > > > > > > > Typo in my prev message, I meant intel_atomic_get_bw_state..but common idea is the same. > > > > > > > > > > > > > > get_state() vs. get_{new,old}_state() are entirely different > > > > > > > things. > > > > > > > > > > > > > > You use get_state() when you really want the state to be > > > > > > > included, and either > > > > > > > - know the state isn't included already, or > > > > > > > - you don't know wether the might have alerady been included > > > > > > > > > > > > > > And one must of course remember that get_state() can > > > > > > > - fail so error handling is needed > > > > > > > - only be used during the check phase, and is illegal during the > > > > > > > commit phase. > > > > > > > > > > > > Sure I know this. I even remember we discussed this many times. > > > > > > > > > > > > > > > > > > > > The get_{new,old}_state() (or the various for loop variants) > > > > > > > you can use when you either: > > > > > > > - know that the state is included already > > > > > > > - are fine with the state potentially not being included > > > > > > > > > > > > Don't you see that it is a bit of a contradiction in those 2 above?? > > > > > > > > > > > > You can't be "know that the state is included already" and > > > > > > "are fine with the state potentially not being included" same time :) > > > > > > > > > > > > Those 2 above actually mean that you CANNOT be sure, because you > > > > > > are "fine with the state potentially not being included"! > > > > > > Otherwise second one would have been redundant. > > > > > > > > > > No. You are either fine with NULL, XOR you know that > > > > > the state is there already. There is no contradiction. > > > > > > > > I do get that. But that way of calling the function is veeery counterintuitive. > > > > Means that you call it and check for NULLness..if you are fine with NULL and > > > > don't check for NULL..if you aren't fine with it and expect the state to be there. > > > > > > > > That is really probabilistic design. > > > > I think we must enumerate all the cases where > > > > > > Not sure what you mean with enumerate. You can't just delcare > > > somewhere globally that in functions X and Y NULL is fine, > > > and in Z it is not. It depends on how X,Y,Z are implemented > > > and it may change any time the implementation is changed. > > > > > > > > > > 1) we expect new_state to be there and > > > > then we don't need even any checks to be there, because we will then rely on get_state. > > > > 2) we don't expect it to be there and then call get_state always. > > > > > > > > Because if you are "fine" with new_state being NULL, why even calling it? > > > > > > Because > > > !NULL -> you have some work to do > > > NULL -> you don't have work to do > > > > Pretty sure we could find a way not to call it at all in case if no work is needed, > > and call it without any checks, if work is needed. > > > > You typically get new bw state to recalculate and compare with old state, however > > there has to be some place where you decide whether to call get_bw/crtc_state or not. > > So from there, this could have been propagated to the moment where we decide where > > to call get_new_bw/crtc_state or not. Then no checks would have been needed. > > And NULL would always mean a bug. > > Also that would be a lot more simple, following KISS principle. > > You'd need to separately track each case in some boolean/etc. > in the overall atomic state. Doable? Sure. Simpler? Don't see > it. It's the exact same code with the NULL check just replaced > with some other check. And you must additionally remember to > sprinkle those bool assignments around. No-no-no. This is how intel_atomic_get_bw_state is called: for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) { new_bw_state = intel_atomic_get_bw_state(state); Basically in any subsequent check, if it is called after that, whenever its called under for_each_new_intel_crtc_in_state, you can be sure that intel_atomic_get_new_bw_state returns non-NULL. I was thinking about something like that, not adding a new boolean check. I'm pretty sure you understand that there might an elegant solution. Anyways, I will get back here, once I have more info, why we don't call intel_atomic_get_crtc in that particular case. > > -- > Ville Syrjälä > Intel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix NULL ptr deref by checking new_crtc_state 2023-05-05 15:55 ` Lisovskiy, Stanislav @ 2023-05-05 16:44 ` Ville Syrjälä 2023-05-05 18:18 ` Lisovskiy, Stanislav 0 siblings, 1 reply; 30+ messages in thread From: Ville Syrjälä @ 2023-05-05 16:44 UTC (permalink / raw) To: Lisovskiy, Stanislav; +Cc: intel-gfx On Fri, May 05, 2023 at 06:55:18PM +0300, Lisovskiy, Stanislav wrote: > On Fri, May 05, 2023 at 05:17:06PM +0300, Ville Syrjälä wrote: > > On Fri, May 05, 2023 at 05:05:55PM +0300, Lisovskiy, Stanislav wrote: > > > On Fri, May 05, 2023 at 04:57:54PM +0300, Ville Syrjälä wrote: > > > > On Fri, May 05, 2023 at 04:42:33PM +0300, Lisovskiy, Stanislav wrote: > > > > > On Fri, May 05, 2023 at 04:28:50PM +0300, Ville Syrjälä wrote: > > > > > > On Fri, May 05, 2023 at 04:21:16PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > On Fri, May 05, 2023 at 04:11:52PM +0300, Ville Syrjälä wrote: > > > > > > > > On Fri, May 05, 2023 at 03:54:58PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > On Fri, May 05, 2023 at 03:46:40PM +0300, Ville Syrjälä wrote: > > > > > > > > > > On Fri, May 05, 2023 at 03:27:51PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > > > On Fri, May 05, 2023 at 03:09:01PM +0300, Ville Syrjälä wrote: > > > > > > > > > > > > On Fri, May 05, 2023 at 02:41:24PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > > > > > On Fri, May 05, 2023 at 02:25:46PM +0300, Ville Syrjälä wrote: > > > > > > > > > > > > > > On Fri, May 05, 2023 at 02:20:17PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > > > > > > > On Fri, May 05, 2023 at 02:06:34PM +0300, Ville Syrjälä wrote: > > > > > > > > > > > > > > > > On Fri, May 05, 2023 at 02:05:27PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > > > > > > > > > On Fri, May 05, 2023 at 02:02:43PM +0300, Ville Syrjälä wrote: > > > > > > > > > > > > > > > > > > On Fri, May 05, 2023 at 01:58:03PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > > > > > > > > > > > On Fri, May 05, 2023 at 01:54:14PM +0300, Ville Syrjälä wrote: > > > > > > > > > > > > > > > > > > > > On Fri, May 05, 2023 at 11:22:12AM +0300, Stanislav Lisovskiy wrote: > > > > > > > > > > > > > > > > > > > > > intel_atomic_get_new_crtc_state can return NULL, unless crtc state wasn't > > > > > > > > > > > > > > > > > > > > > obtained previously with intel_atomic_get_crtc_state, so we must check it > > > > > > > > > > > > > > > > > > > > > for NULLness here, just as in many other places, where we can't guarantee > > > > > > > > > > > > > > > > > > > > > that intel_atomic_get_crtc_state was called. > > > > > > > > > > > > > > > > > > > > > We are currently getting NULL ptr deref because of that, so this fix was > > > > > > > > > > > > > > > > > > > > > confirmed to help. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Fixes: 74a75dc90869 ("drm/i915/display: move plane prepare/cleanup to intel_atomic_plane.c") > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > > > > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > > > > > > > > drivers/gpu/drm/i915/display/intel_atomic_plane.c | 4 ++-- > > > > > > > > > > > > > > > > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > > > > > > > > > > > > index 9f670dcfe76e..4125ee07a271 100644 > > > > > > > > > > > > > > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > > > > > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > > > > > > > > > > > > @@ -1029,7 +1029,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > > > > > > > > > > > > > > > > > > int ret; > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > if (old_obj) { > > > > > > > > > > > > > > > > > > > > > - const struct intel_crtc_state *crtc_state = > > > > > > > > > > > > > > > > > > > > > + const struct intel_crtc_state *new_crtc_state = > > > > > > > > > > > > > > > > > > > > > intel_atomic_get_new_crtc_state(state, > > > > > > > > > > > > > > > > > > > > > to_intel_crtc(old_plane_state->hw.crtc)); > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > @@ -1044,7 +1044,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > > > > > > > > > > > > > > > > > > * This should only fail upon a hung GPU, in which case we > > > > > > > > > > > > > > > > > > > > > * can safely continue. > > > > > > > > > > > > > > > > > > > > > */ > > > > > > > > > > > > > > > > > > > > > - if (intel_crtc_needs_modeset(crtc_state)) { > > > > > > > > > > > > > > > > > > > > > + if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state)) { > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > NAK. We need to fix the bug instead of paparing over it. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I had pushed this already. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > It didn't even finish CI. Please revert. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Swati did run CI and verified that fix helps. I'm _not_ going to revert. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Fine. I'll do it. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Problem is that you don't even care to explain, why this fix is wrong, but simply > > > > > > > > > > > > > > > act in authoritarian way, instead of having constructive discussion. > > > > > > > > > > > > > > > > > > > > > > > > > > > > I've explanined this one about a hundred times. The NULL pointer should > > > > > > > > > > > > > > not happen. Someone needs to actually analyze what is happening instead > > > > > > > > > > > > > > of just adding randomg NULL checks all over the place. > > > > > > > > > > > > > > > > > > > > > > > > > > I do get this point. However why are we doing those check in other places then? > > > > > > > > > > > > > > > > > > > > > > > > We do then when they are actually necessary. > > > > > > > > > > > > > > > > > > > > > > Well but for example when we do check like if(new_bw_state) in intel_bw.c, > > > > > > > > > > > we are also might be having potentially some silent bugs. > > > > > > > > > > > Would you guarantee that if we remove all if(crtc_state) and if(new_bw_state) checks > > > > > > > > > > > in our code, that there won't be NULL pointer dereferences? I bet you don't. > > > > > > > > > > > > > > > > > > > > We have the checks where they are needed. The check in > > > > > > > > > > intel_bw_atomic_check() (if that's the one you mean) > > > > > > > > > > looks entirely correct to me. > > > > > > > > > > > > > > > > > > Typo in my prev message, I meant intel_atomic_get_bw_state..but common idea is the same. > > > > > > > > > > > > > > > > get_state() vs. get_{new,old}_state() are entirely different > > > > > > > > things. > > > > > > > > > > > > > > > > You use get_state() when you really want the state to be > > > > > > > > included, and either > > > > > > > > - know the state isn't included already, or > > > > > > > > - you don't know wether the might have alerady been included > > > > > > > > > > > > > > > > And one must of course remember that get_state() can > > > > > > > > - fail so error handling is needed > > > > > > > > - only be used during the check phase, and is illegal during the > > > > > > > > commit phase. > > > > > > > > > > > > > > Sure I know this. I even remember we discussed this many times. > > > > > > > > > > > > > > > > > > > > > > > The get_{new,old}_state() (or the various for loop variants) > > > > > > > > you can use when you either: > > > > > > > > - know that the state is included already > > > > > > > > - are fine with the state potentially not being included > > > > > > > > > > > > > > Don't you see that it is a bit of a contradiction in those 2 above?? > > > > > > > > > > > > > > You can't be "know that the state is included already" and > > > > > > > "are fine with the state potentially not being included" same time :) > > > > > > > > > > > > > > Those 2 above actually mean that you CANNOT be sure, because you > > > > > > > are "fine with the state potentially not being included"! > > > > > > > Otherwise second one would have been redundant. > > > > > > > > > > > > No. You are either fine with NULL, XOR you know that > > > > > > the state is there already. There is no contradiction. > > > > > > > > > > I do get that. But that way of calling the function is veeery counterintuitive. > > > > > Means that you call it and check for NULLness..if you are fine with NULL and > > > > > don't check for NULL..if you aren't fine with it and expect the state to be there. > > > > > > > > > > That is really probabilistic design. > > > > > I think we must enumerate all the cases where > > > > > > > > Not sure what you mean with enumerate. You can't just delcare > > > > somewhere globally that in functions X and Y NULL is fine, > > > > and in Z it is not. It depends on how X,Y,Z are implemented > > > > and it may change any time the implementation is changed. > > > > > > > > > > > > > 1) we expect new_state to be there and > > > > > then we don't need even any checks to be there, because we will then rely on get_state. > > > > > 2) we don't expect it to be there and then call get_state always. > > > > > > > > > > Because if you are "fine" with new_state being NULL, why even calling it? > > > > > > > > Because > > > > !NULL -> you have some work to do > > > > NULL -> you don't have work to do > > > > > > Pretty sure we could find a way not to call it at all in case if no work is needed, > > > and call it without any checks, if work is needed. > > > > > > You typically get new bw state to recalculate and compare with old state, however > > > there has to be some place where you decide whether to call get_bw/crtc_state or not. > > > So from there, this could have been propagated to the moment where we decide where > > > to call get_new_bw/crtc_state or not. Then no checks would have been needed. > > > And NULL would always mean a bug. > > > Also that would be a lot more simple, following KISS principle. > > > > You'd need to separately track each case in some boolean/etc. > > in the overall atomic state. Doable? Sure. Simpler? Don't see > > it. It's the exact same code with the NULL check just replaced > > with some other check. And you must additionally remember to > > sprinkle those bool assignments around. > > No-no-no. This is how intel_atomic_get_bw_state is called: > > for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) { > new_bw_state = intel_atomic_get_bw_state(state); That's just because we don't need to do anything to the bw state unless some crtc is doing stuff. > > > Basically in any subsequent check, if it is called after that, > whenever its called under for_each_new_intel_crtc_in_state, you > can be sure that intel_atomic_get_new_bw_state returns non-NULL. intel_atomic_get_new_bw_state() is never called from a loop like that. At least I can't immediately see a single place where that would happen. And there is no guarantee anyway that a crtc being part of the commit would imply that bw state is also included. The crtc could have been added to the commit after the code ran which adds the bw state. -- Ville Syrjälä Intel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix NULL ptr deref by checking new_crtc_state 2023-05-05 16:44 ` Ville Syrjälä @ 2023-05-05 18:18 ` Lisovskiy, Stanislav 0 siblings, 0 replies; 30+ messages in thread From: Lisovskiy, Stanislav @ 2023-05-05 18:18 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx On Fri, May 05, 2023 at 07:44:11PM +0300, Ville Syrjälä wrote: > On Fri, May 05, 2023 at 06:55:18PM +0300, Lisovskiy, Stanislav wrote: > > On Fri, May 05, 2023 at 05:17:06PM +0300, Ville Syrjälä wrote: > > > On Fri, May 05, 2023 at 05:05:55PM +0300, Lisovskiy, Stanislav wrote: > > > > On Fri, May 05, 2023 at 04:57:54PM +0300, Ville Syrjälä wrote: > > > > > On Fri, May 05, 2023 at 04:42:33PM +0300, Lisovskiy, Stanislav wrote: > > > > > > On Fri, May 05, 2023 at 04:28:50PM +0300, Ville Syrjälä wrote: > > > > > > > On Fri, May 05, 2023 at 04:21:16PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > On Fri, May 05, 2023 at 04:11:52PM +0300, Ville Syrjälä wrote: > > > > > > > > > On Fri, May 05, 2023 at 03:54:58PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > > On Fri, May 05, 2023 at 03:46:40PM +0300, Ville Syrjälä wrote: > > > > > > > > > > > On Fri, May 05, 2023 at 03:27:51PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > > > > On Fri, May 05, 2023 at 03:09:01PM +0300, Ville Syrjälä wrote: > > > > > > > > > > > > > On Fri, May 05, 2023 at 02:41:24PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > > > > > > On Fri, May 05, 2023 at 02:25:46PM +0300, Ville Syrjälä wrote: > > > > > > > > > > > > > > > On Fri, May 05, 2023 at 02:20:17PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > > > > > > > > On Fri, May 05, 2023 at 02:06:34PM +0300, Ville Syrjälä wrote: > > > > > > > > > > > > > > > > > On Fri, May 05, 2023 at 02:05:27PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > > > > > > > > > > On Fri, May 05, 2023 at 02:02:43PM +0300, Ville Syrjälä wrote: > > > > > > > > > > > > > > > > > > > On Fri, May 05, 2023 at 01:58:03PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > > > > > > > > > > > > On Fri, May 05, 2023 at 01:54:14PM +0300, Ville Syrjälä wrote: > > > > > > > > > > > > > > > > > > > > > On Fri, May 05, 2023 at 11:22:12AM +0300, Stanislav Lisovskiy wrote: > > > > > > > > > > > > > > > > > > > > > > intel_atomic_get_new_crtc_state can return NULL, unless crtc state wasn't > > > > > > > > > > > > > > > > > > > > > > obtained previously with intel_atomic_get_crtc_state, so we must check it > > > > > > > > > > > > > > > > > > > > > > for NULLness here, just as in many other places, where we can't guarantee > > > > > > > > > > > > > > > > > > > > > > that intel_atomic_get_crtc_state was called. > > > > > > > > > > > > > > > > > > > > > > We are currently getting NULL ptr deref because of that, so this fix was > > > > > > > > > > > > > > > > > > > > > > confirmed to help. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Fixes: 74a75dc90869 ("drm/i915/display: move plane prepare/cleanup to intel_atomic_plane.c") > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > > > > > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > > > > > > > > > drivers/gpu/drm/i915/display/intel_atomic_plane.c | 4 ++-- > > > > > > > > > > > > > > > > > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > > > > > > > > > > > > > index 9f670dcfe76e..4125ee07a271 100644 > > > > > > > > > > > > > > > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > > > > > > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > > > > > > > > > > > > > > > > > > > @@ -1029,7 +1029,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > > > > > > > > > > > > > > > > > > > int ret; > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > if (old_obj) { > > > > > > > > > > > > > > > > > > > > > > - const struct intel_crtc_state *crtc_state = > > > > > > > > > > > > > > > > > > > > > > + const struct intel_crtc_state *new_crtc_state = > > > > > > > > > > > > > > > > > > > > > > intel_atomic_get_new_crtc_state(state, > > > > > > > > > > > > > > > > > > > > > > to_intel_crtc(old_plane_state->hw.crtc)); > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > @@ -1044,7 +1044,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane, > > > > > > > > > > > > > > > > > > > > > > * This should only fail upon a hung GPU, in which case we > > > > > > > > > > > > > > > > > > > > > > * can safely continue. > > > > > > > > > > > > > > > > > > > > > > */ > > > > > > > > > > > > > > > > > > > > > > - if (intel_crtc_needs_modeset(crtc_state)) { > > > > > > > > > > > > > > > > > > > > > > + if (new_crtc_state && intel_crtc_needs_modeset(new_crtc_state)) { > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > NAK. We need to fix the bug instead of paparing over it. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I had pushed this already. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > It didn't even finish CI. Please revert. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Swati did run CI and verified that fix helps. I'm _not_ going to revert. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Fine. I'll do it. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Problem is that you don't even care to explain, why this fix is wrong, but simply > > > > > > > > > > > > > > > > act in authoritarian way, instead of having constructive discussion. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I've explanined this one about a hundred times. The NULL pointer should > > > > > > > > > > > > > > > not happen. Someone needs to actually analyze what is happening instead > > > > > > > > > > > > > > > of just adding randomg NULL checks all over the place. > > > > > > > > > > > > > > > > > > > > > > > > > > > > I do get this point. However why are we doing those check in other places then? > > > > > > > > > > > > > > > > > > > > > > > > > > We do then when they are actually necessary. > > > > > > > > > > > > > > > > > > > > > > > > Well but for example when we do check like if(new_bw_state) in intel_bw.c, > > > > > > > > > > > > we are also might be having potentially some silent bugs. > > > > > > > > > > > > Would you guarantee that if we remove all if(crtc_state) and if(new_bw_state) checks > > > > > > > > > > > > in our code, that there won't be NULL pointer dereferences? I bet you don't. > > > > > > > > > > > > > > > > > > > > > > We have the checks where they are needed. The check in > > > > > > > > > > > intel_bw_atomic_check() (if that's the one you mean) > > > > > > > > > > > looks entirely correct to me. > > > > > > > > > > > > > > > > > > > > Typo in my prev message, I meant intel_atomic_get_bw_state..but common idea is the same. > > > > > > > > > > > > > > > > > > get_state() vs. get_{new,old}_state() are entirely different > > > > > > > > > things. > > > > > > > > > > > > > > > > > > You use get_state() when you really want the state to be > > > > > > > > > included, and either > > > > > > > > > - know the state isn't included already, or > > > > > > > > > - you don't know wether the might have alerady been included > > > > > > > > > > > > > > > > > > And one must of course remember that get_state() can > > > > > > > > > - fail so error handling is needed > > > > > > > > > - only be used during the check phase, and is illegal during the > > > > > > > > > commit phase. > > > > > > > > > > > > > > > > Sure I know this. I even remember we discussed this many times. > > > > > > > > > > > > > > > > > > > > > > > > > > The get_{new,old}_state() (or the various for loop variants) > > > > > > > > > you can use when you either: > > > > > > > > > - know that the state is included already > > > > > > > > > - are fine with the state potentially not being included > > > > > > > > > > > > > > > > Don't you see that it is a bit of a contradiction in those 2 above?? > > > > > > > > > > > > > > > > You can't be "know that the state is included already" and > > > > > > > > "are fine with the state potentially not being included" same time :) > > > > > > > > > > > > > > > > Those 2 above actually mean that you CANNOT be sure, because you > > > > > > > > are "fine with the state potentially not being included"! > > > > > > > > Otherwise second one would have been redundant. > > > > > > > > > > > > > > No. You are either fine with NULL, XOR you know that > > > > > > > the state is there already. There is no contradiction. > > > > > > > > > > > > I do get that. But that way of calling the function is veeery counterintuitive. > > > > > > Means that you call it and check for NULLness..if you are fine with NULL and > > > > > > don't check for NULL..if you aren't fine with it and expect the state to be there. > > > > > > > > > > > > That is really probabilistic design. > > > > > > I think we must enumerate all the cases where > > > > > > > > > > Not sure what you mean with enumerate. You can't just delcare > > > > > somewhere globally that in functions X and Y NULL is fine, > > > > > and in Z it is not. It depends on how X,Y,Z are implemented > > > > > and it may change any time the implementation is changed. > > > > > > > > > > > > > > > > 1) we expect new_state to be there and > > > > > > then we don't need even any checks to be there, because we will then rely on get_state. > > > > > > 2) we don't expect it to be there and then call get_state always. > > > > > > > > > > > > Because if you are "fine" with new_state being NULL, why even calling it? > > > > > > > > > > Because > > > > > !NULL -> you have some work to do > > > > > NULL -> you don't have work to do > > > > > > > > Pretty sure we could find a way not to call it at all in case if no work is needed, > > > > and call it without any checks, if work is needed. > > > > > > > > You typically get new bw state to recalculate and compare with old state, however > > > > there has to be some place where you decide whether to call get_bw/crtc_state or not. > > > > So from there, this could have been propagated to the moment where we decide where > > > > to call get_new_bw/crtc_state or not. Then no checks would have been needed. > > > > And NULL would always mean a bug. > > > > Also that would be a lot more simple, following KISS principle. > > > > > > You'd need to separately track each case in some boolean/etc. > > > in the overall atomic state. Doable? Sure. Simpler? Don't see > > > it. It's the exact same code with the NULL check just replaced > > > with some other check. And you must additionally remember to > > > sprinkle those bool assignments around. > > > > No-no-no. This is how intel_atomic_get_bw_state is called: > > > > for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) { > > new_bw_state = intel_atomic_get_bw_state(state); > > That's just because we don't need to do anything to the > bw state unless some crtc is doing stuff. > > > > > > > Basically in any subsequent check, if it is called after that, > > whenever its called under for_each_new_intel_crtc_in_state, you > > can be sure that intel_atomic_get_new_bw_state returns non-NULL. > > intel_atomic_get_new_bw_state() is never called from a loop > like that. At least I can't immediately see a single place > where that would happen. We used to do this before, however here I just put this as an example. > > And there is no guarantee anyway that a crtc being part > of the commit would imply that bw state is also included. > The crtc could have been added to the commit after the > code ran which adds the bw state. Well-well, crtc has been added to the state after code which adds the bw state ran.. Does it mean that we are actually then getting intel_atomic_get_new_bw_state as NULL, despite we have a crtc in state? Sounds like you just described one of the possible similar scenarios, why we are having this bug. I.e we ran that code: for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) { new_bw_state = intel_atomic_get_bw_state(state); but as you mentioned this doesn't mean that we got a bw state because there might have been no crtc. Then it gets added later and then we call intel_atomic_get_new_bw_state and bum. But then checking for NULL is also wrong, because we should have called intel_atomic_get_bw_state for the newly added crtc?.. Sometimes I think, we should make some kind of a doc, with a guidelines, similar like we have for some other areas, describing how should code flow be in each of the typical scenarios, plus the guidelines, how to use it. Stan > > -- > Ville Syrjälä > Intel ^ permalink raw reply [flat|nested] 30+ messages in thread
* [Intel-gfx] ✓ Fi.CI.IGT: success for drm/i915: Fix NULL ptr deref by checking new_crtc_state 2023-05-05 8:22 [Intel-gfx] [PATCH] drm/i915: Fix NULL ptr deref by checking new_crtc_state Stanislav Lisovskiy ` (2 preceding siblings ...) 2023-05-05 10:54 ` Ville Syrjälä @ 2023-05-06 0:38 ` Patchwork 3 siblings, 0 replies; 30+ messages in thread From: Patchwork @ 2023-05-06 0:38 UTC (permalink / raw) To: Lisovskiy, Stanislav; +Cc: intel-gfx [-- Attachment #1: Type: text/plain, Size: 10319 bytes --] == Series Details == Series: drm/i915: Fix NULL ptr deref by checking new_crtc_state URL : https://patchwork.freedesktop.org/series/117369/ State : success == Summary == CI Bug Log - changes from CI_DRM_13110_full -> Patchwork_117369v1_full ==================================================== Summary ------- **SUCCESS** No regressions found. Participating hosts (7 -> 7) ------------------------------ No changes in participating hosts Known issues ------------ Here are the changes found in Patchwork_117369v1_full that come from known issues: ### IGT changes ### #### Issues hit #### * igt@gem_barrier_race@remote-request@rcs0: - shard-glk: [PASS][1] -> [ABORT][2] ([i915#7461] / [i915#8211]) [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13110/shard-glk9/igt@gem_barrier_race@remote-request@rcs0.html [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117369v1/shard-glk2/igt@gem_barrier_race@remote-request@rcs0.html * igt@gem_lmem_swapping@parallel-random-engines: - shard-glk: NOTRUN -> [SKIP][3] ([fdo#109271] / [i915#4613]) [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117369v1/shard-glk3/igt@gem_lmem_swapping@parallel-random-engines.html * igt@gem_lmem_swapping@verify-random-ccs: - shard-apl: NOTRUN -> [SKIP][4] ([fdo#109271] / [i915#4613]) +1 similar issue [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117369v1/shard-apl3/igt@gem_lmem_swapping@verify-random-ccs.html * igt@gem_render_copy@x-tiled-to-vebox-yf-tiled: - shard-apl: NOTRUN -> [SKIP][5] ([fdo#109271]) +45 similar issues [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117369v1/shard-apl3/igt@gem_render_copy@x-tiled-to-vebox-yf-tiled.html * igt@kms_atomic@plane-primary-overlay-mutable-zpos: - shard-glk: NOTRUN -> [SKIP][6] ([fdo#109271]) +36 similar issues [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117369v1/shard-glk3/igt@kms_atomic@plane-primary-overlay-mutable-zpos.html * igt@kms_ccs@pipe-a-random-ccs-data-y_tiled_gen12_rc_ccs_cc: - shard-apl: NOTRUN -> [SKIP][7] ([fdo#109271] / [i915#3886]) [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117369v1/shard-apl3/igt@kms_ccs@pipe-a-random-ccs-data-y_tiled_gen12_rc_ccs_cc.html * igt@kms_ccs@pipe-c-bad-pixel-format-y_tiled_gen12_rc_ccs_cc: - shard-glk: NOTRUN -> [SKIP][8] ([fdo#109271] / [i915#3886]) +1 similar issue [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117369v1/shard-glk3/igt@kms_ccs@pipe-c-bad-pixel-format-y_tiled_gen12_rc_ccs_cc.html * igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions: - shard-glk: [PASS][9] -> [FAIL][10] ([i915#2346]) [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13110/shard-glk4/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions.html [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117369v1/shard-glk2/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions.html * igt@kms_plane_scaling@plane-downscale-with-modifiers-factor-0-5@pipe-b-vga-1: - shard-snb: NOTRUN -> [SKIP][11] ([fdo#109271]) +32 similar issues [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117369v1/shard-snb7/igt@kms_plane_scaling@plane-downscale-with-modifiers-factor-0-5@pipe-b-vga-1.html * igt@kms_psr2_sf@overlay-primary-update-sf-dmg-area: - shard-glk: NOTRUN -> [SKIP][12] ([fdo#109271] / [i915#658]) [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117369v1/shard-glk3/igt@kms_psr2_sf@overlay-primary-update-sf-dmg-area.html * igt@perf@stress-open-close@0-rcs0: - shard-glk: [PASS][13] -> [ABORT][14] ([i915#5213] / [i915#7941]) [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13110/shard-glk3/igt@perf@stress-open-close@0-rcs0.html [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117369v1/shard-glk4/igt@perf@stress-open-close@0-rcs0.html #### Possible fixes #### * igt@gem_exec_fair@basic-none@vecs0: - {shard-rkl}: [FAIL][15] ([i915#2842]) -> [PASS][16] [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13110/shard-rkl-3/igt@gem_exec_fair@basic-none@vecs0.html [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117369v1/shard-rkl-4/igt@gem_exec_fair@basic-none@vecs0.html * igt@gem_exec_fair@basic-pace-share@rcs0: - {shard-tglu}: [FAIL][17] ([i915#2842]) -> [PASS][18] [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13110/shard-tglu-9/igt@gem_exec_fair@basic-pace-share@rcs0.html [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117369v1/shard-tglu-7/igt@gem_exec_fair@basic-pace-share@rcs0.html * igt@gen9_exec_parse@allowed-all: - shard-apl: [ABORT][19] ([i915#5566]) -> [PASS][20] [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13110/shard-apl1/igt@gen9_exec_parse@allowed-all.html [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117369v1/shard-apl3/igt@gen9_exec_parse@allowed-all.html * igt@gen9_exec_parse@allowed-single: - shard-glk: [ABORT][21] ([i915#5566]) -> [PASS][22] [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13110/shard-glk6/igt@gen9_exec_parse@allowed-single.html [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117369v1/shard-glk3/igt@gen9_exec_parse@allowed-single.html * igt@i915_pm_rpm@modeset-non-lpsp-stress: - {shard-rkl}: [SKIP][23] ([i915#1397]) -> [PASS][24] +1 similar issue [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13110/shard-rkl-7/igt@i915_pm_rpm@modeset-non-lpsp-stress.html [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117369v1/shard-rkl-2/igt@i915_pm_rpm@modeset-non-lpsp-stress.html * igt@kms_cursor_legacy@forked-bo@pipe-b: - {shard-rkl}: [INCOMPLETE][25] ([i915#8011]) -> [PASS][26] [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13110/shard-rkl-7/igt@kms_cursor_legacy@forked-bo@pipe-b.html [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117369v1/shard-rkl-2/igt@kms_cursor_legacy@forked-bo@pipe-b.html * igt@kms_fbcon_fbt@fbc-suspend: - {shard-tglu}: [FAIL][27] ([i915#4767]) -> [PASS][28] [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13110/shard-tglu-10/igt@kms_fbcon_fbt@fbc-suspend.html [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117369v1/shard-tglu-8/igt@kms_fbcon_fbt@fbc-suspend.html * igt@kms_flip@flip-vs-expired-vblank@a-dp1: - shard-apl: [FAIL][29] ([i915#79]) -> [PASS][30] [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13110/shard-apl4/igt@kms_flip@flip-vs-expired-vblank@a-dp1.html [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117369v1/shard-apl2/igt@kms_flip@flip-vs-expired-vblank@a-dp1.html * igt@kms_sysfs_edid_timing: - shard-apl: [FAIL][31] ([IGT#2]) -> [PASS][32] [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13110/shard-apl1/igt@kms_sysfs_edid_timing.html [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117369v1/shard-apl6/igt@kms_sysfs_edid_timing.html {name}: This element is suppressed. This means it is ignored when computing the status of the difference (SUCCESS, WARNING, or FAILURE). [IGT#2]: https://gitlab.freedesktop.org/drm/igt-gpu-tools/issues/2 [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271 [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285 [fdo#110189]: https://bugs.freedesktop.org/show_bug.cgi?id=110189 [fdo#111825]: https://bugs.freedesktop.org/show_bug.cgi?id=111825 [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072 [i915#1397]: https://gitlab.freedesktop.org/drm/intel/issues/1397 [i915#2346]: https://gitlab.freedesktop.org/drm/intel/issues/2346 [i915#2842]: https://gitlab.freedesktop.org/drm/intel/issues/2842 [i915#3371]: https://gitlab.freedesktop.org/drm/intel/issues/3371 [i915#3458]: https://gitlab.freedesktop.org/drm/intel/issues/3458 [i915#3689]: https://gitlab.freedesktop.org/drm/intel/issues/3689 [i915#3886]: https://gitlab.freedesktop.org/drm/intel/issues/3886 [i915#3955]: https://gitlab.freedesktop.org/drm/intel/issues/3955 [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077 [i915#4078]: https://gitlab.freedesktop.org/drm/intel/issues/4078 [i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083 [i915#4098]: https://gitlab.freedesktop.org/drm/intel/issues/4098 [i915#4349]: https://gitlab.freedesktop.org/drm/intel/issues/4349 [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613 [i915#4767]: https://gitlab.freedesktop.org/drm/intel/issues/4767 [i915#5176]: https://gitlab.freedesktop.org/drm/intel/issues/5176 [i915#5213]: https://gitlab.freedesktop.org/drm/intel/issues/5213 [i915#5235]: https://gitlab.freedesktop.org/drm/intel/issues/5235 [i915#5354]: https://gitlab.freedesktop.org/drm/intel/issues/5354 [i915#5566]: https://gitlab.freedesktop.org/drm/intel/issues/5566 [i915#5784]: https://gitlab.freedesktop.org/drm/intel/issues/5784 [i915#6095]: https://gitlab.freedesktop.org/drm/intel/issues/6095 [i915#6268]: https://gitlab.freedesktop.org/drm/intel/issues/6268 [i915#658]: https://gitlab.freedesktop.org/drm/intel/issues/658 [i915#7461]: https://gitlab.freedesktop.org/drm/intel/issues/7461 [i915#7711]: https://gitlab.freedesktop.org/drm/intel/issues/7711 [i915#79]: https://gitlab.freedesktop.org/drm/intel/issues/79 [i915#7941]: https://gitlab.freedesktop.org/drm/intel/issues/7941 [i915#8011]: https://gitlab.freedesktop.org/drm/intel/issues/8011 [i915#8211]: https://gitlab.freedesktop.org/drm/intel/issues/8211 Build changes ------------- * Linux: CI_DRM_13110 -> Patchwork_117369v1 CI-20190529: 20190529 CI_DRM_13110: 0fc2261e97d6fe498f17bcd9120fed98778ff0d8 @ git://anongit.freedesktop.org/gfx-ci/linux IGT_7277: 1cb3507f3ff28d11bd5cfabcde576fe78ddab571 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git Patchwork_117369v1: 0fc2261e97d6fe498f17bcd9120fed98778ff0d8 @ git://anongit.freedesktop.org/gfx-ci/linux piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117369v1/index.html [-- Attachment #2: Type: text/html, Size: 10791 bytes --] ^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2023-05-06 0:38 UTC | newest] Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-05-05 8:22 [Intel-gfx] [PATCH] drm/i915: Fix NULL ptr deref by checking new_crtc_state Stanislav Lisovskiy 2023-05-05 9:11 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork 2023-05-05 9:29 ` [Intel-gfx] [PATCH] " Andrzej Hajda 2023-05-05 10:28 ` Lisovskiy, Stanislav 2023-05-05 10:54 ` Ville Syrjälä 2023-05-05 10:58 ` Lisovskiy, Stanislav 2023-05-05 11:02 ` Ville Syrjälä 2023-05-05 11:05 ` Lisovskiy, Stanislav 2023-05-05 11:06 ` Ville Syrjälä 2023-05-05 11:08 ` Lisovskiy, Stanislav 2023-05-05 11:20 ` Lisovskiy, Stanislav 2023-05-05 11:25 ` Ville Syrjälä 2023-05-05 11:41 ` Lisovskiy, Stanislav 2023-05-05 12:09 ` Ville Syrjälä 2023-05-05 12:27 ` Lisovskiy, Stanislav 2023-05-05 12:46 ` Ville Syrjälä 2023-05-05 12:52 ` Lisovskiy, Stanislav 2023-05-05 13:52 ` Ville Syrjälä 2023-05-05 12:54 ` Lisovskiy, Stanislav 2023-05-05 13:11 ` Ville Syrjälä 2023-05-05 13:21 ` Lisovskiy, Stanislav 2023-05-05 13:28 ` Ville Syrjälä 2023-05-05 13:42 ` Lisovskiy, Stanislav 2023-05-05 13:57 ` Ville Syrjälä 2023-05-05 14:05 ` Lisovskiy, Stanislav 2023-05-05 14:17 ` Ville Syrjälä 2023-05-05 15:55 ` Lisovskiy, Stanislav 2023-05-05 16:44 ` Ville Syrjälä 2023-05-05 18:18 ` Lisovskiy, Stanislav 2023-05-06 0:38 ` [Intel-gfx] ✓ Fi.CI.IGT: success for " Patchwork
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.