On Tue, Feb 20, 2018 at 03:29:07PM +0200, Oleksandr Andrushchenko wrote:np ;)
On 02/20/2018 02:53 PM, Oleksandr Andrushchenko wrote:1 week of vacation later I have to admit that this WARN_ON is completely
On 02/20/2018 02:49 PM, Daniel Vetter wrote:huh, this indeed solves the NULL pointer dereference, but floods a lot
On Tue, Feb 20, 2018 at 02:36:05PM +0200, Oleksandr Andrushchenko wrote:ok, will update with this additional check
On 02/20/2018 01:17 PM, Daniel Vetter wrote:Right, when the plane is completely off there's no CRTC state. Correct
On Mon, Feb 19, 2018 at 04:58:43PM +0200, OleksandrYes, but I still see cases when crtc_state is NULL, thus
Andrushchenko wrote:
On 02/19/2018 04:30 PM, Daniel Vetter wrote:I wasn't sure, thanks for figuring this out!
On Tue, Feb 13, 2018 at 10:44:16AM +0200, OleksandrYes, it does work if this is what you mean:
Andrushchenko wrote:
From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>Hm, this is a bit annoying, since the can_position =
It is possible that drm_simple_kms_plane_atomic_check called
with no CRTC set, e.g. when user-space
application sets CRTC_ID/FB_ID
to 0 before doing any actual drawing. This leads to NULL pointer
dereference because in this case new CRTC state is NULL and must be
checked before accessing.
Signed-off-by: Oleksandr Andrushchenko
<oleksandr_andrushchenko@xxxxxxxx>
---
ÂÂÂ drivers/gpu/drm/drm_simple_kms_helper.c | 6 ++++--
ÂÂÂ 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git
a/drivers/gpu/drm/drm_simple_kms_helper.c
b/drivers/gpu/drm/drm_simple_kms_helper.c
index 9ca8a4a59b74..a05eca9cec8b 100644
--- a/drivers/gpu/drm/drm_simple_kms_helper.c
+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
@@ -121,8 +121,10 @@ static int
drm_simple_kms_plane_atomic_check(struct
drm_plane *plane,
ÂÂÂÂÂÂÂ pipe = container_of(plane, struct
drm_simple_display_pipe, plane);
ÂÂÂÂÂÂÂ crtc_state =
drm_atomic_get_new_crtc_state(plane_state->state,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &pipe->crtc);
-ÂÂÂ if (!crtc_state->enable)
-ÂÂÂÂÂÂÂ return 0; /* nothing to check when
disabling or disabled */
+
+ÂÂÂ if (!crtc_state || !crtc_state->enable)
+ÂÂÂÂÂÂÂ /* nothing to check when disabling or
disabled or no CRTC set */
+ÂÂÂÂÂÂÂ return 0;
ÂÂÂÂÂÂÂ if (crtc_state->enable)
drm_mode_get_hv_timing(&crtc_state->mode,
false parameter to
drm_atomic_helper_check_plane_state is supposed to
catch all this. Would
moving all the checks after the call to that helper,
and gating them on
plane_state->visible also work?
diff --git a/drivers/gpu/drm/drm_simple_kms_helper.cif (!plane_state->visible) {
b/drivers/gpu/drm/drm_simple_kms_helper.c
index a05eca9cec8b..c48858bb2823 100644
--- a/drivers/gpu/drm/drm_simple_kms_helper.c
+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
@@ -122,14 +122,6 @@ static int
drm_simple_kms_plane_atomic_check(struct
drm_plane *plane,
 Â crtc_state =
drm_atomic_get_new_crtc_state(plane_state->state,
&pipe->crtc);
-ÂÂÂÂÂÂ if (!crtc_state || !crtc_state->enable)
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ /* nothing to check when disabling or
disabled or no CRTC
set */
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return 0;
-
-ÂÂÂÂÂÂ if (crtc_state->enable)
- drm_mode_get_hv_timing(&crtc_state->mode,
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &clip.x2, &clip.y2);
-
 Â ret =
drm_atomic_helper_check_plane_state(plane_state,
crtc_state,
&clip,
DRM_PLANE_HELPER_NO_SCALING,
@@ -138,6 +130,13 @@ static int
drm_simple_kms_plane_atomic_check(struct
drm_plane *plane,
 Â if (ret)
 Â return ret;
+ÂÂÂÂÂÂ if (!plane_state->visible || !crtc_state->enable)
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return 0; /* nothing to check when
disabling or disabled */
ÂÂÂÂWARN_ON(crtc_state->enabled);
ÂÂÂÂreturn 0;
}
The helper call above should guarantee this.
making crtc_state->enable to fail
check should be
ÂÂÂÂWARN_ON(crtc_state && crtc_state->enabled);
with every page flip I have, e.g. !plane_state->visible == true
and crtc_state is not NULL and crtc_state->enable == true,
thus firing WARN_ON.
Is this something wrong with my use-case/driver or it is still legal
to have such a configuration and leave it without WARN_ON and just
return 0?
bogus :-)
Sorry for all the confusion, pls leave it out.
-Daniel
_______________________________________________Thank youlgtm. With or without the bikeshed to pull the crtc_state check into thePlease see the patch under test attached (I believe it is what+Similar here.
+ÂÂÂÂÂÂ if (plane_state->visible && crtc_state->enable)
+ drm_mode_get_hv_timing(&crtc_state->mode,This can now be removed, the plane helper takes care of checking for
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &clip.x2, &clip.y2);
+
 Â if (!plane_state->visible)
 Â return -EINVAL;
plane_state->visible != crtc_state->enable. Please also remove.
Yes please, with my above cleanup suggestions.We'd need to add a guarantee toIt doesn't look at it at the moment
drm_atomic_helper_check_plane_state that
it can cope with crtc_state == NULL, but I think that's a good idea
anyway. Atm it shouldn't end up looking at the
crtc_state pointer in that
case.
Otherwise we'll just go with your fix, but it feelsAt list with the change above it passes my test which failed
all a bit too fragile,
hence why I want to explore more robust options a bit.
before. Although I cannot confirm it works for others, but it
certainly does for me.
-DanielDo you want me to send v1 with the code above?
you mean,
with the only change that
ÂÂÂÂ if (!plane_state->visible) {
ÂÂÂÂ ÂÂÂ *if (crtc_state)*
ÂÂÂÂ ÂÂÂ ÂÂÂ WARN_ON(crtc_state->enable);
ÂÂÂÂ ÂÂÂ return 0;
ÂÂÂÂ }
check is used).
Whith this patch + additional logs I have:
[ÂÂ 18.939204] [drm:drm_ioctl [drm]] pid=2105, dev=0xe200, auth=1,
DRM_IOCTL_MODE_ATOMIC
[...]
[ÂÂ 18.939681] [drm:drm_atomic_set_crtc_for_plane [drm]] Link
plane state
00000000c302cbbf to [NOCRTC]
[ÂÂ 18.939822] [drm:drm_atomic_set_fb_for_plane [drm]] Set
[NOFB] for plane
state 00000000c302cbbf
[ÂÂ 18.939963] [drm:drm_atomic_print_state [drm]] checking
000000000bc224e7
[ÂÂ 18.939988] vdispl vdispl.0: [drm] plane[29]: plane-0
[ÂÂ 18.940003] vdispl vdispl.0: [drm]ÂÂ crtc=(null)
[ÂÂ 18.940018] vdispl vdispl.0: [drm]ÂÂ fb=0
[ÂÂ 18.940032] vdispl vdispl.0: [drm]ÂÂ crtc-pos=0x0+0+0
[ÂÂ 18.940048] vdispl vdispl.0: [drm]
src-pos=0.000000x0.000000+0.000000+0.000000
[ÂÂ 18.940067] vdispl vdispl.0: [drm]ÂÂ rotation=1
[ÂÂ 18.940199] [drm:drm_atomic_check_only [drm]] checking
000000000bc224e7
[ÂÂ 18.940226] ================================= plane_state->visible 0
crtc_stateÂÂÂÂÂÂÂÂÂÂ (null)
[...]
[ÂÂ 18.978146] [drm:drm_atomic_set_crtc_for_plane [drm]] Link
plane state
000000006bd50580 to [CRTC:30:crtc-0]
[ÂÂ 18.978292] [drm:drm_atomic_set_fb_for_plane [drm]] Set
[FB:35] for plane
state 000000006bd50580
[ÂÂ 18.978993] [drm:drm_atomic_set_mode_prop_for_crtc [drm]] Set
[MODE:1024x768] for CRTC state 00000000e5a28f6a
[ÂÂ 18.979425] [drm:drm_atomic_check_only [drm]] checking
000000000bc224e7
[ÂÂ 18.979540] [drm:drm_atomic_helper_check_modeset [drm_kms_helper]]
[CRTC:30:crtc-0] mode changed
[ÂÂ 18.979632] [drm:drm_atomic_helper_check_modeset [drm_kms_helper]]
[CRTC:30:crtc-0] enable changed
[ÂÂ 18.979708] [drm:drm_atomic_helper_check_modeset [drm_kms_helper]]
[CRTC:30:crtc-0] active changed
[ÂÂ 18.979792] [drm:drm_atomic_helper_check_modeset [drm_kms_helper]]
Updating routing for [CONNECTOR:28:Virtual-1]
[ÂÂ 18.979877] [drm:drm_atomic_helper_check_modeset [drm_kms_helper]]
[CONNECTOR:28:Virtual-1] using [ENCODER:31:None-31] on [CRTC:30:crtc-0]
[ÂÂ 18.979960] [drm:drm_atomic_helper_check_modeset [drm_kms_helper]]
[CRTC:30:crtc-0] needs all connectors, enable: y, active: y
[ÂÂ 18.980139] [drm:drm_atomic_add_affected_connectors [drm]]
Adding all
current connectors for [CRTC:30:crtc-0] to 000000000bc224e7
[ÂÂ 18.980184] ================================= plane_state->visible 0
crtc_state 00000000e5a28f6a
[ÂÂ 18.980205] crtc_state->enable 1
*[ÂÂ 19.022608] WARNING: CPU: 1 PID: 2105 at
drivers/gpu/drm/drm_simple_kms_helper.c:137
drm_simple_kms_plane_atomic_check+0xdc/0xf8 [drm_kms_helper]*
[...]
[ÂÂ 19.113601] ================================= plane_state->visible 0
crtc_state 00000000e5a28f6a
[ÂÂ 19.113623] crtc_state->enable 1
[ÂÂ 19.113792] WARNING: CPU: 1 PID: 2105 at
drivers/gpu/drm/drm_simple_kms_helper.c:137
drm_simple_kms_plane_atomic_check+0xdc/0xf8 [drm_kms_helper]
[...]
And finally
[ÂÂ 19.340249] ================================= plane_state->visible 0
crtc_state 0000000036a1b1f5
[ÂÂ 19.340271] crtc_state->enable 0
So, it seems that crtc_state can still be NULL if
"!plane_state->visible"
making
NULL pointer dereference, so we need a check for that.
Yet, !plane_state->visible && crtc_state->enable makes WARN_ON to fire
always. So, probably we may want removing it.
Thanks, DanielThank you,
Oleksandr
ÂFrom dbcce708b237740158a2c16029c56a579324f269 Mon Sep 17 00:00:00 2001
From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
Date: Tue, 13 Feb 2018 10:32:20 +0200
Subject: [PATCH] drm/simple_kms_helper: Fix NULL pointer
dereference with no
 active CRTC
It is possible that drm_simple_kms_plane_atomic_check called
with no CRTC set, e.g. when user-space application sets CRTC_ID/FB_ID
to 0 before doing any actual drawing. This leads to NULL pointer
dereference because in this case new CRTC state is NULL and must be
checked before accessing.
Signed-off-by: Oleksandr Andrushchenko
<oleksandr_andrushchenko@xxxxxxxx>
---
 drivers/gpu/drm/drm_simple_kms_helper.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c
b/drivers/gpu/drm/drm_simple_kms_helper.c
index 9ca8a4a59b74..f54711ff9767 100644
--- a/drivers/gpu/drm/drm_simple_kms_helper.c
+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
@@ -121,12 +121,6 @@ static int
drm_simple_kms_plane_atomic_check(struct drm_plane *plane,
ÂÂÂÂÂ pipe = container_of(plane, struct drm_simple_display_pipe,
plane);
ÂÂÂÂÂ crtc_state = drm_atomic_get_new_crtc_state(plane_state->state,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &pipe->crtc);
-ÂÂÂ if (!crtc_state->enable)
-ÂÂÂÂÂÂÂ return 0; /* nothing to check when disabling or disabled */
-
-ÂÂÂ if (crtc_state->enable)
-ÂÂÂÂÂÂÂ drm_mode_get_hv_timing(&crtc_state->mode,
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &clip.x2, &clip.y2);
 Â ret = drm_atomic_helper_check_plane_state(plane_state,
crtc_state,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &clip,
@@ -136,8 +130,13 @@ static int
drm_simple_kms_plane_atomic_check(struct drm_plane *plane,
ÂÂÂÂÂ if (ret)
ÂÂÂÂÂÂÂÂÂ return ret;
 - if (!plane_state->visible)
-ÂÂÂÂÂÂÂ return -EINVAL;
+ÂÂÂ if (!plane_state->visible) {
+ÂÂÂÂÂÂÂ if (crtc_state)
+ÂÂÂÂÂÂÂÂÂÂÂ WARN_ON(crtc_state->enable);
+ÂÂÂÂÂÂÂ return 0;
+ÂÂÂ }
+
+ÂÂÂ drm_mode_get_hv_timing(&crtc_state->mode, &clip.x2, &clip.y2);
WARN_ON.
Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
Please resubmit as a stand-alone patch, patchwork can't pull patches outoh, that was for demonstration purpose only, so we
of attachments :-/
are on the same page and see the patch we are discussing ;)
-Daniel
 Â if (!pipe->funcs || !pipe->funcs->check)
ÂÂÂÂÂÂÂÂÂ return 0;
--
2.7.4
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel