* [PATCH] drm/atomic: Clean old_state/new_state in drm_atomic_state_default_clear()
@ 2018-05-02 18:32 Ville Syrjala
2018-05-02 19:12 ` ✓ Fi.CI.BAT: success for " Patchwork
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Ville Syrjala @ 2018-05-02 18:32 UTC (permalink / raw)
To: dri-devel
Cc: intel-gfx, stable, Maarten Lankhorst, Laurent Pinchart, Abhay Kumar
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
Clear the old_state and new_state pointers for every object in
drm_atomic_state_default_clear(). Otherwise
drm_atomic_get_{new,old}_*_state() will hand out stale pointers to
anyone who hasn't first confirmed that the object is in fact part of
the current atomic transcation, if they are called after we've done
the ww backoff dance while hanging on to the same drm_atomic_state.
For example, handle_conflicting_encoders() looks like it could hit
this since it iterates the full connector list and just calls
drm_atomic_get_new_connector_state() for each.
And I believe we have now witnessed this happening at least once in
i915 check_digital_port_conflicts(). Commit 8b69449d2663 ("drm/i915:
Remove last references to drm_atomic_get_existing* macros") changed
the safe drm_atomic_get_existing_connector_state() to the unsafe
drm_atomic_get_new_connector_state(), which opened the doors for
this particular bug there as well.
Cc: stable@vger.kernel.org
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Abhay Kumar <abhay.kumar@intel.com>
Fixes: 581e49fe6b41 ("drm/atomic: Add new iterators over all state, v3.")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
drivers/gpu/drm/drm_atomic.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 7d25c42f22db..c825c76edc1d 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -155,6 +155,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
state->connectors[i].state);
state->connectors[i].ptr = NULL;
state->connectors[i].state = NULL;
+ state->connectors[i].old_state = NULL;
+ state->connectors[i].new_state = NULL;
drm_connector_put(connector);
}
@@ -169,6 +171,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
state->crtcs[i].ptr = NULL;
state->crtcs[i].state = NULL;
+ state->crtcs[i].old_state = NULL;
+ state->crtcs[i].new_state = NULL;
}
for (i = 0; i < config->num_total_plane; i++) {
@@ -181,6 +185,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
state->planes[i].state);
state->planes[i].ptr = NULL;
state->planes[i].state = NULL;
+ state->planes[i].old_state = NULL;
+ state->planes[i].new_state = NULL;
}
for (i = 0; i < state->num_private_objs; i++) {
@@ -190,6 +196,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
state->private_objs[i].state);
state->private_objs[i].ptr = NULL;
state->private_objs[i].state = NULL;
+ state->private_objs[i].old_state = NULL;
+ state->private_objs[i].new_state = NULL;
}
state->num_private_objs = 0;
--
2.16.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* ✓ Fi.CI.BAT: success for drm/atomic: Clean old_state/new_state in drm_atomic_state_default_clear()
2018-05-02 18:32 [PATCH] drm/atomic: Clean old_state/new_state in drm_atomic_state_default_clear() Ville Syrjala
@ 2018-05-02 19:12 ` Patchwork
2018-05-03 1:35 ` ✓ Fi.CI.IGT: " Patchwork
` (2 subsequent siblings)
3 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2018-05-02 19:12 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
== Series Details ==
Series: drm/atomic: Clean old_state/new_state in drm_atomic_state_default_clear()
URL : https://patchwork.freedesktop.org/series/42588/
State : success
== Summary ==
= CI Bug Log - changes from CI_DRM_4123 -> Patchwork_8880 =
== Summary - SUCCESS ==
No regressions found.
External URL: https://patchwork.freedesktop.org/api/1.0/series/42588/revisions/1/mbox/
== Known issues ==
Here are the changes found in Patchwork_8880 that come from known issues:
=== IGT changes ===
==== Issues hit ====
igt@gem_exec_suspend@basic-s3:
fi-ivb-3520m: PASS -> DMESG-WARN (fdo#106084)
==== Possible fixes ====
igt@kms_chamelium@dp-edid-read:
fi-kbl-7500u: FAIL (fdo#103841) -> PASS
igt@kms_flip@basic-flip-vs-wf_vblank:
fi-cfl-s3: FAIL (fdo#103928, fdo#100368) -> PASS
igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
fi-ivb-3520m: DMESG-WARN (fdo#106084) -> PASS
igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
fi-bxt-dsi: INCOMPLETE (fdo#103927) -> PASS
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#103841 https://bugs.freedesktop.org/show_bug.cgi?id=103841
fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
fdo#103928 https://bugs.freedesktop.org/show_bug.cgi?id=103928
fdo#106084 https://bugs.freedesktop.org/show_bug.cgi?id=106084
== Participating hosts (40 -> 37) ==
Missing (3): fi-ctg-p8600 fi-ilk-m540 fi-skl-6700hq
== Build changes ==
* Linux: CI_DRM_4123 -> Patchwork_8880
CI_DRM_4123: cbb6a0aa933f3323a8deb331aca503b7388abc06 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4456: 43761534c6482dc67b9c3d8eeecd425ef40b3c4c @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_8880: a4fef673b606b7873aa6ec6dfc5546c41f5bcf15 @ git://anongit.freedesktop.org/gfx-ci/linux
piglit_4456: 30b992bdc047073e1fe99b1ac622f026618a8081 @ git://anongit.freedesktop.org/piglit
== Linux commits ==
a4fef673b606 drm/atomic: Clean old_state/new_state in drm_atomic_state_default_clear()
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8880/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* ✓ Fi.CI.IGT: success for drm/atomic: Clean old_state/new_state in drm_atomic_state_default_clear()
2018-05-02 18:32 [PATCH] drm/atomic: Clean old_state/new_state in drm_atomic_state_default_clear() Ville Syrjala
2018-05-02 19:12 ` ✓ Fi.CI.BAT: success for " Patchwork
@ 2018-05-03 1:35 ` Patchwork
2018-05-03 6:35 ` Daniel Vetter
2018-05-03 7:46 ` Maarten Lankhorst
3 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2018-05-03 1:35 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
== Series Details ==
Series: drm/atomic: Clean old_state/new_state in drm_atomic_state_default_clear()
URL : https://patchwork.freedesktop.org/series/42588/
State : success
== Summary ==
= CI Bug Log - changes from CI_DRM_4123_full -> Patchwork_8880_full =
== Summary - SUCCESS ==
No regressions found.
External URL: https://patchwork.freedesktop.org/api/1.0/series/42588/revisions/1/mbox/
== Known issues ==
Here are the changes found in Patchwork_8880_full that come from known issues:
=== IGT changes ===
==== Issues hit ====
igt@kms_cursor_crc@cursor-128x128-suspend:
shard-glk: PASS -> INCOMPLETE (k.org#198133, fdo#103359)
igt@kms_flip@2x-plain-flip-ts-check-interruptible:
shard-hsw: PASS -> FAIL (fdo#100368)
igt@kms_flip@plain-flip-ts-check-interruptible:
shard-glk: PASS -> FAIL (fdo#100368)
igt@perf@blocking:
shard-hsw: PASS -> FAIL (fdo#102252)
==== Possible fixes ====
igt@kms_flip@dpms-vs-vblank-race-interruptible:
shard-glk: FAIL (fdo#103060) -> PASS
igt@kms_flip@flip-vs-absolute-wf_vblank-interruptible:
shard-glk: FAIL (fdo#100368) -> PASS
igt@kms_flip@wf_vblank-ts-check-interruptible:
shard-hsw: FAIL (fdo#103928) -> PASS
=== Piglit changes ===
==== Issues hit ====
spec@glsl-1.40-compat@execution@built-in-constants:
pig-hsw-4770r: NOTRUN -> FAIL (fdo#106277)
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
fdo#103928 https://bugs.freedesktop.org/show_bug.cgi?id=103928
fdo#106277 https://bugs.freedesktop.org/show_bug.cgi?id=106277
k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133
== Participating hosts (7 -> 7) ==
Additional (1): pig-hsw-4770r
Missing (1): shard-kbl
== Build changes ==
* Linux: CI_DRM_4123 -> Patchwork_8880
CI_DRM_4123: cbb6a0aa933f3323a8deb331aca503b7388abc06 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4456: 43761534c6482dc67b9c3d8eeecd425ef40b3c4c @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_8880: a4fef673b606b7873aa6ec6dfc5546c41f5bcf15 @ git://anongit.freedesktop.org/gfx-ci/linux
piglit_4456: 30b992bdc047073e1fe99b1ac622f026618a8081 @ git://anongit.freedesktop.org/piglit
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8880/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/atomic: Clean old_state/new_state in drm_atomic_state_default_clear()
2018-05-02 18:32 [PATCH] drm/atomic: Clean old_state/new_state in drm_atomic_state_default_clear() Ville Syrjala
@ 2018-05-03 6:35 ` Daniel Vetter
2018-05-03 1:35 ` ✓ Fi.CI.IGT: " Patchwork
` (2 subsequent siblings)
3 siblings, 0 replies; 10+ messages in thread
From: Daniel Vetter @ 2018-05-03 6:35 UTC (permalink / raw)
To: Ville Syrjala; +Cc: dri-devel, intel-gfx, Abhay Kumar, Laurent Pinchart, stable
On Wed, May 02, 2018 at 09:32:47PM +0300, Ville Syrjala wrote:
> From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
>
> Clear the old_state and new_state pointers for every object in
> drm_atomic_state_default_clear(). Otherwise
> drm_atomic_get_{new,old}_*_state() will hand out stale pointers to
> anyone who hasn't first confirmed that the object is in fact part of
> the current atomic transcation, if they are called after we've done
> the ww backoff dance while hanging on to the same drm_atomic_state.
>
> For example, handle_conflicting_encoders() looks like it could hit
> this since it iterates the full connector list and just calls
> drm_atomic_get_new_connector_state() for each.
>
> And I believe we have now witnessed this happening at least once in
> i915 check_digital_port_conflicts(). Commit 8b69449d2663 ("drm/i915:
> Remove last references to drm_atomic_get_existing* macros") changed
> the safe drm_atomic_get_existing_connector_state() to the unsafe
> drm_atomic_get_new_connector_state(), which opened the doors for
> this particular bug there as well.
>
> Cc: stable@vger.kernel.org
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Abhay Kumar <abhay.kumar@intel.com>
> Fixes: 581e49fe6b41 ("drm/atomic: Add new iterators over all state, v3.")
> Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
Uh ... that's some bad oversight :-/
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
btw for stable we might want to split this into 1 patch for core objects
and 1 patch for driver private stuff. Feel free to do so while applying
and keep my r-b. The fixes line for the 2nd patch would be:
Fixes: a4370c777406 ("drm/atomic: Make private objs proper objects")
Cc: <stable@vger.kernel.org> # v4.14+
Cheers, Daniel
> ---
> drivers/gpu/drm/drm_atomic.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 7d25c42f22db..c825c76edc1d 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -155,6 +155,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
> state->connectors[i].state);
> state->connectors[i].ptr = NULL;
> state->connectors[i].state = NULL;
> + state->connectors[i].old_state = NULL;
> + state->connectors[i].new_state = NULL;
> drm_connector_put(connector);
> }
>
> @@ -169,6 +171,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
>
> state->crtcs[i].ptr = NULL;
> state->crtcs[i].state = NULL;
> + state->crtcs[i].old_state = NULL;
> + state->crtcs[i].new_state = NULL;
> }
>
> for (i = 0; i < config->num_total_plane; i++) {
> @@ -181,6 +185,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
> state->planes[i].state);
> state->planes[i].ptr = NULL;
> state->planes[i].state = NULL;
> + state->planes[i].old_state = NULL;
> + state->planes[i].new_state = NULL;
> }
>
> for (i = 0; i < state->num_private_objs; i++) {
> @@ -190,6 +196,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
> state->private_objs[i].state);
> state->private_objs[i].ptr = NULL;
> state->private_objs[i].state = NULL;
> + state->private_objs[i].old_state = NULL;
> + state->private_objs[i].new_state = NULL;
> }
> state->num_private_objs = 0;
>
> --
> 2.16.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/atomic: Clean old_state/new_state in drm_atomic_state_default_clear()
@ 2018-05-03 6:35 ` Daniel Vetter
0 siblings, 0 replies; 10+ messages in thread
From: Daniel Vetter @ 2018-05-03 6:35 UTC (permalink / raw)
To: Ville Syrjala; +Cc: stable, intel-gfx, Laurent Pinchart, dri-devel
On Wed, May 02, 2018 at 09:32:47PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Clear the old_state and new_state pointers for every object in
> drm_atomic_state_default_clear(). Otherwise
> drm_atomic_get_{new,old}_*_state() will hand out stale pointers to
> anyone who hasn't first confirmed that the object is in fact part of
> the current atomic transcation, if they are called after we've done
> the ww backoff dance while hanging on to the same drm_atomic_state.
>
> For example, handle_conflicting_encoders() looks like it could hit
> this since it iterates the full connector list and just calls
> drm_atomic_get_new_connector_state() for each.
>
> And I believe we have now witnessed this happening at least once in
> i915 check_digital_port_conflicts(). Commit 8b69449d2663 ("drm/i915:
> Remove last references to drm_atomic_get_existing* macros") changed
> the safe drm_atomic_get_existing_connector_state() to the unsafe
> drm_atomic_get_new_connector_state(), which opened the doors for
> this particular bug there as well.
>
> Cc: stable@vger.kernel.org
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Abhay Kumar <abhay.kumar@intel.com>
> Fixes: 581e49fe6b41 ("drm/atomic: Add new iterators over all state, v3.")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Uh ... that's some bad oversight :-/
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
btw for stable we might want to split this into 1 patch for core objects
and 1 patch for driver private stuff. Feel free to do so while applying
and keep my r-b. The fixes line for the 2nd patch would be:
Fixes: a4370c777406 ("drm/atomic: Make private objs proper objects")
Cc: <stable@vger.kernel.org> # v4.14+
Cheers, Daniel
> ---
> drivers/gpu/drm/drm_atomic.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 7d25c42f22db..c825c76edc1d 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -155,6 +155,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
> state->connectors[i].state);
> state->connectors[i].ptr = NULL;
> state->connectors[i].state = NULL;
> + state->connectors[i].old_state = NULL;
> + state->connectors[i].new_state = NULL;
> drm_connector_put(connector);
> }
>
> @@ -169,6 +171,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
>
> state->crtcs[i].ptr = NULL;
> state->crtcs[i].state = NULL;
> + state->crtcs[i].old_state = NULL;
> + state->crtcs[i].new_state = NULL;
> }
>
> for (i = 0; i < config->num_total_plane; i++) {
> @@ -181,6 +185,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
> state->planes[i].state);
> state->planes[i].ptr = NULL;
> state->planes[i].state = NULL;
> + state->planes[i].old_state = NULL;
> + state->planes[i].new_state = NULL;
> }
>
> for (i = 0; i < state->num_private_objs; i++) {
> @@ -190,6 +196,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
> state->private_objs[i].state);
> state->private_objs[i].ptr = NULL;
> state->private_objs[i].state = NULL;
> + state->private_objs[i].old_state = NULL;
> + state->private_objs[i].new_state = NULL;
> }
> state->num_private_objs = 0;
>
> --
> 2.16.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/atomic: Clean old_state/new_state in drm_atomic_state_default_clear()
2018-05-02 18:32 [PATCH] drm/atomic: Clean old_state/new_state in drm_atomic_state_default_clear() Ville Syrjala
` (2 preceding siblings ...)
2018-05-03 6:35 ` Daniel Vetter
@ 2018-05-03 7:46 ` Maarten Lankhorst
3 siblings, 0 replies; 10+ messages in thread
From: Maarten Lankhorst @ 2018-05-03 7:46 UTC (permalink / raw)
To: Ville Syrjala, dri-devel; +Cc: intel-gfx, stable, Laurent Pinchart, Abhay Kumar
Op 02-05-18 om 20:32 schreef Ville Syrjala:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Clear the old_state and new_state pointers for every object in
> drm_atomic_state_default_clear(). Otherwise
> drm_atomic_get_{new,old}_*_state() will hand out stale pointers to
> anyone who hasn't first confirmed that the object is in fact part of
> the current atomic transcation, if they are called after we've done
> the ww backoff dance while hanging on to the same drm_atomic_state.
>
> For example, handle_conflicting_encoders() looks like it could hit
> this since it iterates the full connector list and just calls
> drm_atomic_get_new_connector_state() for each.
>
> And I believe we have now witnessed this happening at least once in
> i915 check_digital_port_conflicts(). Commit 8b69449d2663 ("drm/i915:
> Remove last references to drm_atomic_get_existing* macros") changed
> the safe drm_atomic_get_existing_connector_state() to the unsafe
> drm_atomic_get_new_connector_state(), which opened the doors for
> this particular bug there as well.
>
> Cc: stable@vger.kernel.org
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Abhay Kumar <abhay.kumar@intel.com>
> Fixes: 581e49fe6b41 ("drm/atomic: Add new iterators over all state, v3.")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
OUCH! Good catch..
~Maarten
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
How come KASAN didn't complain?
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/atomic: Clean old_state/new_state in drm_atomic_state_default_clear()
2018-05-03 6:35 ` Daniel Vetter
@ 2018-05-03 10:41 ` Ville Syrjälä
-1 siblings, 0 replies; 10+ messages in thread
From: Ville Syrjälä @ 2018-05-03 10:41 UTC (permalink / raw)
To: Daniel Vetter; +Cc: dri-devel, intel-gfx, Abhay Kumar, Laurent Pinchart, stable
On Thu, May 03, 2018 at 08:35:30AM +0200, Daniel Vetter wrote:
> On Wed, May 02, 2018 at 09:32:47PM +0300, Ville Syrjala wrote:
> > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> >
> > Clear the old_state and new_state pointers for every object in
> > drm_atomic_state_default_clear(). Otherwise
> > drm_atomic_get_{new,old}_*_state() will hand out stale pointers to
> > anyone who hasn't first confirmed that the object is in fact part of
> > the current atomic transcation, if they are called after we've done
> > the ww backoff dance while hanging on to the same drm_atomic_state.
> >
> > For example, handle_conflicting_encoders() looks like it could hit
> > this since it iterates the full connector list and just calls
> > drm_atomic_get_new_connector_state() for each.
> >
> > And I believe we have now witnessed this happening at least once in
> > i915 check_digital_port_conflicts(). Commit 8b69449d2663 ("drm/i915:
> > Remove last references to drm_atomic_get_existing* macros") changed
> > the safe drm_atomic_get_existing_connector_state() to the unsafe
> > drm_atomic_get_new_connector_state(), which opened the doors for
> > this particular bug there as well.
> >
> > Cc: stable@vger.kernel.org
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Abhay Kumar <abhay.kumar@intel.com>
> > Fixes: 581e49fe6b41 ("drm/atomic: Add new iterators over all state, v3.")
> > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
>
> Uh ... that's some bad oversight :-/
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> btw for stable we might want to split this into 1 patch for core objects
> and 1 patch for driver private stuff.
Good point. I forgot we did that after the new iterators were
introduced. I'll do the split.
> Feel free to do so while applying
> and keep my r-b. The fixes line for the 2nd patch would be:
>
> Fixes: a4370c777406 ("drm/atomic: Make private objs proper objects")
> Cc: <stable@vger.kernel.org> # v4.14+
Ta.
>
> Cheers, Daniel
> > ---
> > drivers/gpu/drm/drm_atomic.c | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 7d25c42f22db..c825c76edc1d 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -155,6 +155,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
> > state->connectors[i].state);
> > state->connectors[i].ptr = NULL;
> > state->connectors[i].state = NULL;
> > + state->connectors[i].old_state = NULL;
> > + state->connectors[i].new_state = NULL;
> > drm_connector_put(connector);
> > }
> >
> > @@ -169,6 +171,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
> >
> > state->crtcs[i].ptr = NULL;
> > state->crtcs[i].state = NULL;
> > + state->crtcs[i].old_state = NULL;
> > + state->crtcs[i].new_state = NULL;
> > }
> >
> > for (i = 0; i < config->num_total_plane; i++) {
> > @@ -181,6 +185,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
> > state->planes[i].state);
> > state->planes[i].ptr = NULL;
> > state->planes[i].state = NULL;
> > + state->planes[i].old_state = NULL;
> > + state->planes[i].new_state = NULL;
> > }
> >
> > for (i = 0; i < state->num_private_objs; i++) {
> > @@ -190,6 +196,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
> > state->private_objs[i].state);
> > state->private_objs[i].ptr = NULL;
> > state->private_objs[i].state = NULL;
> > + state->private_objs[i].old_state = NULL;
> > + state->private_objs[i].new_state = NULL;
> > }
> > state->num_private_objs = 0;
> >
> > --
> > 2.16.1
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
--
Ville Syrj�l�
Intel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/atomic: Clean old_state/new_state in drm_atomic_state_default_clear()
@ 2018-05-03 10:41 ` Ville Syrjälä
0 siblings, 0 replies; 10+ messages in thread
From: Ville Syrjälä @ 2018-05-03 10:41 UTC (permalink / raw)
To: Daniel Vetter; +Cc: stable, intel-gfx, Abhay Kumar, Laurent Pinchart, dri-devel
On Thu, May 03, 2018 at 08:35:30AM +0200, Daniel Vetter wrote:
> On Wed, May 02, 2018 at 09:32:47PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Clear the old_state and new_state pointers for every object in
> > drm_atomic_state_default_clear(). Otherwise
> > drm_atomic_get_{new,old}_*_state() will hand out stale pointers to
> > anyone who hasn't first confirmed that the object is in fact part of
> > the current atomic transcation, if they are called after we've done
> > the ww backoff dance while hanging on to the same drm_atomic_state.
> >
> > For example, handle_conflicting_encoders() looks like it could hit
> > this since it iterates the full connector list and just calls
> > drm_atomic_get_new_connector_state() for each.
> >
> > And I believe we have now witnessed this happening at least once in
> > i915 check_digital_port_conflicts(). Commit 8b69449d2663 ("drm/i915:
> > Remove last references to drm_atomic_get_existing* macros") changed
> > the safe drm_atomic_get_existing_connector_state() to the unsafe
> > drm_atomic_get_new_connector_state(), which opened the doors for
> > this particular bug there as well.
> >
> > Cc: stable@vger.kernel.org
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Abhay Kumar <abhay.kumar@intel.com>
> > Fixes: 581e49fe6b41 ("drm/atomic: Add new iterators over all state, v3.")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Uh ... that's some bad oversight :-/
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> btw for stable we might want to split this into 1 patch for core objects
> and 1 patch for driver private stuff.
Good point. I forgot we did that after the new iterators were
introduced. I'll do the split.
> Feel free to do so while applying
> and keep my r-b. The fixes line for the 2nd patch would be:
>
> Fixes: a4370c777406 ("drm/atomic: Make private objs proper objects")
> Cc: <stable@vger.kernel.org> # v4.14+
Ta.
>
> Cheers, Daniel
> > ---
> > drivers/gpu/drm/drm_atomic.c | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 7d25c42f22db..c825c76edc1d 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -155,6 +155,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
> > state->connectors[i].state);
> > state->connectors[i].ptr = NULL;
> > state->connectors[i].state = NULL;
> > + state->connectors[i].old_state = NULL;
> > + state->connectors[i].new_state = NULL;
> > drm_connector_put(connector);
> > }
> >
> > @@ -169,6 +171,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
> >
> > state->crtcs[i].ptr = NULL;
> > state->crtcs[i].state = NULL;
> > + state->crtcs[i].old_state = NULL;
> > + state->crtcs[i].new_state = NULL;
> > }
> >
> > for (i = 0; i < config->num_total_plane; i++) {
> > @@ -181,6 +185,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
> > state->planes[i].state);
> > state->planes[i].ptr = NULL;
> > state->planes[i].state = NULL;
> > + state->planes[i].old_state = NULL;
> > + state->planes[i].new_state = NULL;
> > }
> >
> > for (i = 0; i < state->num_private_objs; i++) {
> > @@ -190,6 +196,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
> > state->private_objs[i].state);
> > state->private_objs[i].ptr = NULL;
> > state->private_objs[i].state = NULL;
> > + state->private_objs[i].old_state = NULL;
> > + state->private_objs[i].new_state = NULL;
> > }
> > state->num_private_objs = 0;
> >
> > --
> > 2.16.1
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
--
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/atomic: Clean old_state/new_state in drm_atomic_state_default_clear()
2018-05-03 10:41 ` Ville Syrjälä
@ 2018-05-03 14:48 ` Ville Syrjälä
-1 siblings, 0 replies; 10+ messages in thread
From: Ville Syrjälä @ 2018-05-03 14:48 UTC (permalink / raw)
To: Daniel Vetter; +Cc: stable, intel-gfx, Laurent Pinchart, dri-devel
On Thu, May 03, 2018 at 01:41:52PM +0300, Ville Syrj�l� wrote:
> On Thu, May 03, 2018 at 08:35:30AM +0200, Daniel Vetter wrote:
> > On Wed, May 02, 2018 at 09:32:47PM +0300, Ville Syrjala wrote:
> > > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > >
> > > Clear the old_state and new_state pointers for every object in
> > > drm_atomic_state_default_clear(). Otherwise
> > > drm_atomic_get_{new,old}_*_state() will hand out stale pointers to
> > > anyone who hasn't first confirmed that the object is in fact part of
> > > the current atomic transcation, if they are called after we've done
> > > the ww backoff dance while hanging on to the same drm_atomic_state.
> > >
> > > For example, handle_conflicting_encoders() looks like it could hit
> > > this since it iterates the full connector list and just calls
> > > drm_atomic_get_new_connector_state() for each.
> > >
> > > And I believe we have now witnessed this happening at least once in
> > > i915 check_digital_port_conflicts(). Commit 8b69449d2663 ("drm/i915:
> > > Remove last references to drm_atomic_get_existing* macros") changed
> > > the safe drm_atomic_get_existing_connector_state() to the unsafe
> > > drm_atomic_get_new_connector_state(), which opened the doors for
> > > this particular bug there as well.
> > >
> > > Cc: stable@vger.kernel.org
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Cc: Abhay Kumar <abhay.kumar@intel.com>
> > > Fixes: 581e49fe6b41 ("drm/atomic: Add new iterators over all state, v3.")
> > > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> >
> > Uh ... that's some bad oversight :-/
> >
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >
> > btw for stable we might want to split this into 1 patch for core objects
> > and 1 patch for driver private stuff.
>
> Good point. I forgot we did that after the new iterators were
> introduced. I'll do the split.
Pushed the split version for drm-misc-fixes.
As mentioned on irc we don't have any get_{new,old}_private_state()
functions, so the private objs part is pretty theoretical. However
splitting does have the benefit of (hopefully) not requiring manual
intervention when someone tries to cherry-pick this to 4.12/4.13.
Thanks for the reviews.
>
> > Feel free to do so while applying
> > and keep my r-b. The fixes line for the 2nd patch would be:
> >
> > Fixes: a4370c777406 ("drm/atomic: Make private objs proper objects")
> > Cc: <stable@vger.kernel.org> # v4.14+
>
> Ta.
>
> >
> > Cheers, Daniel
> > > ---
> > > drivers/gpu/drm/drm_atomic.c | 8 ++++++++
> > > 1 file changed, 8 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > index 7d25c42f22db..c825c76edc1d 100644
> > > --- a/drivers/gpu/drm/drm_atomic.c
> > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > @@ -155,6 +155,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
> > > state->connectors[i].state);
> > > state->connectors[i].ptr = NULL;
> > > state->connectors[i].state = NULL;
> > > + state->connectors[i].old_state = NULL;
> > > + state->connectors[i].new_state = NULL;
> > > drm_connector_put(connector);
> > > }
> > >
> > > @@ -169,6 +171,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
> > >
> > > state->crtcs[i].ptr = NULL;
> > > state->crtcs[i].state = NULL;
> > > + state->crtcs[i].old_state = NULL;
> > > + state->crtcs[i].new_state = NULL;
> > > }
> > >
> > > for (i = 0; i < config->num_total_plane; i++) {
> > > @@ -181,6 +185,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
> > > state->planes[i].state);
> > > state->planes[i].ptr = NULL;
> > > state->planes[i].state = NULL;
> > > + state->planes[i].old_state = NULL;
> > > + state->planes[i].new_state = NULL;
> > > }
> > >
> > > for (i = 0; i < state->num_private_objs; i++) {
> > > @@ -190,6 +196,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
> > > state->private_objs[i].state);
> > > state->private_objs[i].ptr = NULL;
> > > state->private_objs[i].state = NULL;
> > > + state->private_objs[i].old_state = NULL;
> > > + state->private_objs[i].new_state = NULL;
> > > }
> > > state->num_private_objs = 0;
> > >
> > > --
> > > 2.16.1
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
>
> --
> Ville Syrj�l�
> Intel
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrj�l�
Intel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/atomic: Clean old_state/new_state in drm_atomic_state_default_clear()
@ 2018-05-03 14:48 ` Ville Syrjälä
0 siblings, 0 replies; 10+ messages in thread
From: Ville Syrjälä @ 2018-05-03 14:48 UTC (permalink / raw)
To: Daniel Vetter; +Cc: stable, intel-gfx, Laurent Pinchart, dri-devel
On Thu, May 03, 2018 at 01:41:52PM +0300, Ville Syrjälä wrote:
> On Thu, May 03, 2018 at 08:35:30AM +0200, Daniel Vetter wrote:
> > On Wed, May 02, 2018 at 09:32:47PM +0300, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > Clear the old_state and new_state pointers for every object in
> > > drm_atomic_state_default_clear(). Otherwise
> > > drm_atomic_get_{new,old}_*_state() will hand out stale pointers to
> > > anyone who hasn't first confirmed that the object is in fact part of
> > > the current atomic transcation, if they are called after we've done
> > > the ww backoff dance while hanging on to the same drm_atomic_state.
> > >
> > > For example, handle_conflicting_encoders() looks like it could hit
> > > this since it iterates the full connector list and just calls
> > > drm_atomic_get_new_connector_state() for each.
> > >
> > > And I believe we have now witnessed this happening at least once in
> > > i915 check_digital_port_conflicts(). Commit 8b69449d2663 ("drm/i915:
> > > Remove last references to drm_atomic_get_existing* macros") changed
> > > the safe drm_atomic_get_existing_connector_state() to the unsafe
> > > drm_atomic_get_new_connector_state(), which opened the doors for
> > > this particular bug there as well.
> > >
> > > Cc: stable@vger.kernel.org
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Cc: Abhay Kumar <abhay.kumar@intel.com>
> > > Fixes: 581e49fe6b41 ("drm/atomic: Add new iterators over all state, v3.")
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Uh ... that's some bad oversight :-/
> >
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >
> > btw for stable we might want to split this into 1 patch for core objects
> > and 1 patch for driver private stuff.
>
> Good point. I forgot we did that after the new iterators were
> introduced. I'll do the split.
Pushed the split version for drm-misc-fixes.
As mentioned on irc we don't have any get_{new,old}_private_state()
functions, so the private objs part is pretty theoretical. However
splitting does have the benefit of (hopefully) not requiring manual
intervention when someone tries to cherry-pick this to 4.12/4.13.
Thanks for the reviews.
>
> > Feel free to do so while applying
> > and keep my r-b. The fixes line for the 2nd patch would be:
> >
> > Fixes: a4370c777406 ("drm/atomic: Make private objs proper objects")
> > Cc: <stable@vger.kernel.org> # v4.14+
>
> Ta.
>
> >
> > Cheers, Daniel
> > > ---
> > > drivers/gpu/drm/drm_atomic.c | 8 ++++++++
> > > 1 file changed, 8 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > index 7d25c42f22db..c825c76edc1d 100644
> > > --- a/drivers/gpu/drm/drm_atomic.c
> > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > @@ -155,6 +155,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
> > > state->connectors[i].state);
> > > state->connectors[i].ptr = NULL;
> > > state->connectors[i].state = NULL;
> > > + state->connectors[i].old_state = NULL;
> > > + state->connectors[i].new_state = NULL;
> > > drm_connector_put(connector);
> > > }
> > >
> > > @@ -169,6 +171,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
> > >
> > > state->crtcs[i].ptr = NULL;
> > > state->crtcs[i].state = NULL;
> > > + state->crtcs[i].old_state = NULL;
> > > + state->crtcs[i].new_state = NULL;
> > > }
> > >
> > > for (i = 0; i < config->num_total_plane; i++) {
> > > @@ -181,6 +185,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
> > > state->planes[i].state);
> > > state->planes[i].ptr = NULL;
> > > state->planes[i].state = NULL;
> > > + state->planes[i].old_state = NULL;
> > > + state->planes[i].new_state = NULL;
> > > }
> > >
> > > for (i = 0; i < state->num_private_objs; i++) {
> > > @@ -190,6 +196,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
> > > state->private_objs[i].state);
> > > state->private_objs[i].ptr = NULL;
> > > state->private_objs[i].state = NULL;
> > > + state->private_objs[i].old_state = NULL;
> > > + state->private_objs[i].new_state = NULL;
> > > }
> > > state->num_private_objs = 0;
> > >
> > > --
> > > 2.16.1
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
>
> --
> Ville Syrjälä
> Intel
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-05-03 14:48 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-02 18:32 [PATCH] drm/atomic: Clean old_state/new_state in drm_atomic_state_default_clear() Ville Syrjala
2018-05-02 19:12 ` ✓ Fi.CI.BAT: success for " Patchwork
2018-05-03 1:35 ` ✓ Fi.CI.IGT: " Patchwork
2018-05-03 6:35 ` [PATCH] " Daniel Vetter
2018-05-03 6:35 ` Daniel Vetter
2018-05-03 10:41 ` Ville Syrjälä
2018-05-03 10:41 ` Ville Syrjälä
2018-05-03 14:48 ` [Intel-gfx] " Ville Syrjälä
2018-05-03 14:48 ` Ville Syrjälä
2018-05-03 7:46 ` Maarten Lankhorst
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.