| Message ID | 1459846239-8946-1-git-send-email-gnuiyl@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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
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
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
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
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;
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(-)