[PATCH] drm/crtc_helper: Reset empty plane state in drm_helper_crtc_mode_set_base()

[PATCH] drm/crtc_helper: Reset empty plane state in drm_helper_crtc_mode_set_base()

[Liu Ying]

Transitional drivers might access the NULL pointer plane->state in
drm_helper_crtc_mode_set_base(), which causes NULL pointer dereference.
So, let's reset it before handing it over to those drivers.
commit e4f31ad2b713 ("drm: reset empty state in transitional helpers")
did the same thing for other transitional helpers, but it seems this one
was missed.

Signed-off-by: Liu Ying <gnuiyl@gmail.com>
---
 drivers/gpu/drm/drm_crtc_helper.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index 79555d2..66ca313 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -1053,10 +1053,12 @@ int drm_helper_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
 
 	if (plane->funcs->atomic_duplicate_state)
 		plane_state = plane->funcs->atomic_duplicate_state(plane);
-	else if (plane->state)
+	else {
+		if (!plane->state)
+			drm_atomic_helper_plane_reset(plane);
+
 		plane_state = drm_atomic_helper_plane_duplicate_state(plane);
-	else
-		plane_state = kzalloc(sizeof(*plane_state), GFP_KERNEL);
+	}
 	if (!plane_state)
 		return -ENOMEM;
 	plane_state->plane = plane;
-- 
2.5.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/crtc_helper: Reset empty plane state in drm_helper_crtc_mode_set_base()

[Daniel Vetter]

On Tue, Apr 05, 2016 at 04:50:39PM +0800, Liu Ying wrote:
> Transitional drivers might access the NULL pointer plane->state in
> drm_helper_crtc_mode_set_base(), which causes NULL pointer dereference.
> So, let's reset it before handing it over to those drivers.
> commit e4f31ad2b713 ("drm: reset empty state in transitional helpers")
> did the same thing for other transitional helpers, but it seems this one
> was missed.
> 
> Signed-off-by: Liu Ying <gnuiyl@gmail.com>

This is kinda intentionally not done, assuming that drivers (when
transitioning) can't handle the full state stuff yet.

I'm not entirely opposed to this, but then we should do it for both plane
and crtc states, and in all cases I think.

Completely new drivers should never use transitional helpers, but instead
just bring up using native atomic helpers directly. So what exactly do you
need this for?
-Daniel

> ---
>  drivers/gpu/drm/drm_crtc_helper.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> index 79555d2..66ca313 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -1053,10 +1053,12 @@ int drm_helper_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
>  
>  	if (plane->funcs->atomic_duplicate_state)
>  		plane_state = plane->funcs->atomic_duplicate_state(plane);
> -	else if (plane->state)
> +	else {
> +		if (!plane->state)
> +			drm_atomic_helper_plane_reset(plane);
> +
>  		plane_state = drm_atomic_helper_plane_duplicate_state(plane);
> -	else
> -		plane_state = kzalloc(sizeof(*plane_state), GFP_KERNEL);
> +	}
>  	if (!plane_state)
>  		return -ENOMEM;
>  	plane_state->plane = plane;
> -- 
> 2.5.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/crtc_helper: Reset empty plane state in drm_helper_crtc_mode_set_base()

[Ying Liu]

On Wed, Apr 13, 2016 at 12:00 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Apr 05, 2016 at 04:50:39PM +0800, Liu Ying wrote:
>> Transitional drivers might access the NULL pointer plane->state in
>> drm_helper_crtc_mode_set_base(), which causes NULL pointer dereference.
>> So, let's reset it before handing it over to those drivers.
>> commit e4f31ad2b713 ("drm: reset empty state in transitional helpers")
>> did the same thing for other transitional helpers, but it seems this one
>> was missed.
>>
>> Signed-off-by: Liu Ying <gnuiyl@gmail.com>
>
> This is kinda intentionally not done, assuming that drivers (when
> transitioning) can't handle the full state stuff yet.

It's a question whether the assumption is correct or not.
IMHO, during the transition, the old/new state door has been
opened to drivers.

>
> I'm not entirely opposed to this, but then we should do it for both plane
> and crtc states, and in all cases I think.

It looks this doesn't hurt.

>
> Completely new drivers should never use transitional helpers, but instead
> just bring up using native atomic helpers directly. So what exactly do you
> need this for?

This is needed for my WIP imx-drm transitional plane driver.
It would access the fb pointer of the old plane state(the empty plane
state in question) to do atomic check.  Also, ->atomic_update() will
do something like page flip if the fb pointer is not NULL(meaning
that the plane was enabled before the update).  All of this aims to
make the driver behave the similar way after the transition.

Regards,
Liu Ying

> -Daniel
>
>> ---
>>  drivers/gpu/drm/drm_crtc_helper.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
>> index 79555d2..66ca313 100644
>> --- a/drivers/gpu/drm/drm_crtc_helper.c
>> +++ b/drivers/gpu/drm/drm_crtc_helper.c
>> @@ -1053,10 +1053,12 @@ int drm_helper_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
>>
>>       if (plane->funcs->atomic_duplicate_state)
>>               plane_state = plane->funcs->atomic_duplicate_state(plane);
>> -     else if (plane->state)
>> +     else {
>> +             if (!plane->state)
>> +                     drm_atomic_helper_plane_reset(plane);
>> +
>>               plane_state = drm_atomic_helper_plane_duplicate_state(plane);
>> -     else
>> -             plane_state = kzalloc(sizeof(*plane_state), GFP_KERNEL);
>> +     }
>>       if (!plane_state)
>>               return -ENOMEM;
>>       plane_state->plane = plane;
>> --
>> 2.5.0
>>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/crtc_helper: Reset empty plane state in drm_helper_crtc_mode_set_base()

[Daniel Vetter]

On Wed, Apr 13, 2016 at 10:52:34AM +0800, Ying Liu wrote:
> On Wed, Apr 13, 2016 at 12:00 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Tue, Apr 05, 2016 at 04:50:39PM +0800, Liu Ying wrote:
> >> Transitional drivers might access the NULL pointer plane->state in
> >> drm_helper_crtc_mode_set_base(), which causes NULL pointer dereference.
> >> So, let's reset it before handing it over to those drivers.
> >> commit e4f31ad2b713 ("drm: reset empty state in transitional helpers")
> >> did the same thing for other transitional helpers, but it seems this one
> >> was missed.
> >>
> >> Signed-off-by: Liu Ying <gnuiyl@gmail.com>
> >
> > This is kinda intentionally not done, assuming that drivers (when
> > transitioning) can't handle the full state stuff yet.
> 
> It's a question whether the assumption is correct or not.
> IMHO, during the transition, the old/new state door has been
> opened to drivers.
> 
> >
> > I'm not entirely opposed to this, but then we should do it for both plane
> > and crtc states, and in all cases I think.
> 
> It looks this doesn't hurt.
> 
> >
> > Completely new drivers should never use transitional helpers, but instead
> > just bring up using native atomic helpers directly. So what exactly do you
> > need this for?
> 
> This is needed for my WIP imx-drm transitional plane driver.
> It would access the fb pointer of the old plane state(the empty plane
> state in question) to do atomic check.  Also, ->atomic_update() will
> do something like page flip if the fb pointer is not NULL(meaning
> that the plane was enabled before the update).  All of this aims to
> make the driver behave the similar way after the transition.

If you need this in your driver, just make sure you're calling ->reset
hooks yourself as needed? Also this really is just for transitioning, in
the end no driver should permanently end up using these functions. In the
end transitional helpers here are meant as guidelines/crutches, every
driver is slightly different. Adding hacks to make things work smoothly
for all of them is impossible, and imo better to just move real fast to
proper atomic.
-Daniel

> 
> Regards,
> Liu Ying
> 
> > -Daniel
> >
> >> ---
> >>  drivers/gpu/drm/drm_crtc_helper.c | 8 +++++---
> >>  1 file changed, 5 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> >> index 79555d2..66ca313 100644
> >> --- a/drivers/gpu/drm/drm_crtc_helper.c
> >> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> >> @@ -1053,10 +1053,12 @@ int drm_helper_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
> >>
> >>       if (plane->funcs->atomic_duplicate_state)
> >>               plane_state = plane->funcs->atomic_duplicate_state(plane);
> >> -     else if (plane->state)
> >> +     else {
> >> +             if (!plane->state)
> >> +                     drm_atomic_helper_plane_reset(plane);
> >> +
> >>               plane_state = drm_atomic_helper_plane_duplicate_state(plane);
> >> -     else
> >> -             plane_state = kzalloc(sizeof(*plane_state), GFP_KERNEL);
> >> +     }
> >>       if (!plane_state)
> >>               return -ENOMEM;
> >>       plane_state->plane = plane;
> >> --
> >> 2.5.0
> >>
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/crtc_helper: Reset empty plane state in drm_helper_crtc_mode_set_base()

[Ying Liu]

On Wed, Apr 13, 2016 at 6:33 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Apr 13, 2016 at 10:52:34AM +0800, Ying Liu wrote:
>> On Wed, Apr 13, 2016 at 12:00 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> > On Tue, Apr 05, 2016 at 04:50:39PM +0800, Liu Ying wrote:
>> >> Transitional drivers might access the NULL pointer plane->state in
>> >> drm_helper_crtc_mode_set_base(), which causes NULL pointer dereference.
>> >> So, let's reset it before handing it over to those drivers.
>> >> commit e4f31ad2b713 ("drm: reset empty state in transitional helpers")
>> >> did the same thing for other transitional helpers, but it seems this one
>> >> was missed.
>> >>
>> >> Signed-off-by: Liu Ying <gnuiyl@gmail.com>
>> >
>> > This is kinda intentionally not done, assuming that drivers (when
>> > transitioning) can't handle the full state stuff yet.
>>
>> It's a question whether the assumption is correct or not.
>> IMHO, during the transition, the old/new state door has been
>> opened to drivers.
>>
>> >
>> > I'm not entirely opposed to this, but then we should do it for both plane
>> > and crtc states, and in all cases I think.
>>
>> It looks this doesn't hurt.
>>
>> >
>> > Completely new drivers should never use transitional helpers, but instead
>> > just bring up using native atomic helpers directly. So what exactly do you
>> > need this for?
>>
>> This is needed for my WIP imx-drm transitional plane driver.
>> It would access the fb pointer of the old plane state(the empty plane
>> state in question) to do atomic check.  Also, ->atomic_update() will
>> do something like page flip if the fb pointer is not NULL(meaning
>> that the plane was enabled before the update).  All of this aims to
>> make the driver behave the similar way after the transition.
>
> If you need this in your driver, just make sure you're calling ->reset
> hooks yourself as needed? Also this really is just for transitioning, in
> the end no driver should permanently end up using these functions. In the
> end transitional helpers here are meant as guidelines/crutches, every
> driver is slightly different. Adding hacks to make things work smoothly
> for all of them is impossible, and imo better to just move real fast to
> proper atomic.

Granted my driver may call ->reset, it's more appropriate to do that
in the helper.  There are several reasons for this.  First, helper callers
do not bother to do this.  Secondly,  drm_helper_crtc_mode_set_base
is somewhat a counterpart of drm_helper_crtc_mode_set,
drm_plane_helper_update and drm_plane_helper_disable.  Now that
commit e4f31ad2b713 calls ->reset in the three helpers, it looks
there is no reason this cannot be done for their counterpart.
Last but not least, exposing the NULL pointer plane->state to helper
callers is not a good behavior, even worse could be a bug. So, this
patch is not a hack but a somewhat bug fix.

Regards,
Liu Ying

> -Daniel
>
>>
>> Regards,
>> Liu Ying
>>
>> > -Daniel
>> >
>> >> ---
>> >>  drivers/gpu/drm/drm_crtc_helper.c | 8 +++++---
>> >>  1 file changed, 5 insertions(+), 3 deletions(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
>> >> index 79555d2..66ca313 100644
>> >> --- a/drivers/gpu/drm/drm_crtc_helper.c
>> >> +++ b/drivers/gpu/drm/drm_crtc_helper.c
>> >> @@ -1053,10 +1053,12 @@ int drm_helper_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
>> >>
>> >>       if (plane->funcs->atomic_duplicate_state)
>> >>               plane_state = plane->funcs->atomic_duplicate_state(plane);
>> >> -     else if (plane->state)
>> >> +     else {
>> >> +             if (!plane->state)
>> >> +                     drm_atomic_helper_plane_reset(plane);
>> >> +
>> >>               plane_state = drm_atomic_helper_plane_duplicate_state(plane);
>> >> -     else
>> >> -             plane_state = kzalloc(sizeof(*plane_state), GFP_KERNEL);
>> >> +     }
>> >>       if (!plane_state)
>> >>               return -ENOMEM;
>> >>       plane_state->plane = plane;
>> >> --
>> >> 2.5.0
>> >>
>> >
>> > --
>> > Daniel Vetter
>> > Software Engineer, Intel Corporation
>> > http://blog.ffwll.ch
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/crtc_helper: Reset empty plane state in drm_helper_crtc_mode_set_base()

[Daniel Vetter]

On Thu, Apr 14, 2016 at 11:25:00AM +0800, Ying Liu wrote:
> On Wed, Apr 13, 2016 at 6:33 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Wed, Apr 13, 2016 at 10:52:34AM +0800, Ying Liu wrote:
> >> On Wed, Apr 13, 2016 at 12:00 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> >> > On Tue, Apr 05, 2016 at 04:50:39PM +0800, Liu Ying wrote:
> >> >> Transitional drivers might access the NULL pointer plane->state in
> >> >> drm_helper_crtc_mode_set_base(), which causes NULL pointer dereference.
> >> >> So, let's reset it before handing it over to those drivers.
> >> >> commit e4f31ad2b713 ("drm: reset empty state in transitional helpers")
> >> >> did the same thing for other transitional helpers, but it seems this one
> >> >> was missed.
> >> >>
> >> >> Signed-off-by: Liu Ying <gnuiyl@gmail.com>
> >> >
> >> > This is kinda intentionally not done, assuming that drivers (when
> >> > transitioning) can't handle the full state stuff yet.
> >>
> >> It's a question whether the assumption is correct or not.
> >> IMHO, during the transition, the old/new state door has been
> >> opened to drivers.
> >>
> >> >
> >> > I'm not entirely opposed to this, but then we should do it for both plane
> >> > and crtc states, and in all cases I think.
> >>
> >> It looks this doesn't hurt.
> >>
> >> >
> >> > Completely new drivers should never use transitional helpers, but instead
> >> > just bring up using native atomic helpers directly. So what exactly do you
> >> > need this for?
> >>
> >> This is needed for my WIP imx-drm transitional plane driver.
> >> It would access the fb pointer of the old plane state(the empty plane
> >> state in question) to do atomic check.  Also, ->atomic_update() will
> >> do something like page flip if the fb pointer is not NULL(meaning
> >> that the plane was enabled before the update).  All of this aims to
> >> make the driver behave the similar way after the transition.
> >
> > If you need this in your driver, just make sure you're calling ->reset
> > hooks yourself as needed? Also this really is just for transitioning, in
> > the end no driver should permanently end up using these functions. In the
> > end transitional helpers here are meant as guidelines/crutches, every
> > driver is slightly different. Adding hacks to make things work smoothly
> > for all of them is impossible, and imo better to just move real fast to
> > proper atomic.
> 
> Granted my driver may call ->reset, it's more appropriate to do that
> in the helper.  There are several reasons for this.  First, helper callers
> do not bother to do this.  Secondly,  drm_helper_crtc_mode_set_base
> is somewhat a counterpart of drm_helper_crtc_mode_set,
> drm_plane_helper_update and drm_plane_helper_disable.  Now that
> commit e4f31ad2b713 calls ->reset in the three helpers, it looks
> there is no reason this cannot be done for their counterpart.
> Last but not least, exposing the NULL pointer plane->state to helper
> callers is not a good behavior, even worse could be a bug. So, this
> patch is not a hack but a somewhat bug fix.

Hm ok, I was blind and didn't realize that we've done this partially
already. Thanks for insisting, I applied your patch to drm-misc now.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel