| Message ID | 1452053525-15053-1-git-send-email-meng.yi@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi guys, Am I loosing the plot here or something feels amiss here ? On 6 January 2016 at 06:12, Meng Yi <meng.yi@nxp.com> wrote: > For state->fb or state->crtc may be NULL in fsl_dcu_drm_plane_atomic_check > function, if so, return 0. > > Signed-off-by: Meng Yi <meng.yi@nxp.com> > Signed-off-by: Jianwei Wang <jianwei.wang.chn@gmail.com> > > --- > > change in v2: > -Add state->crtc check > -return 0 when state->fb or state->crtc is NULL, instead of -EINVAL > Adviced by Daniel Stone > > drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c > index 4b13cf9..8965580 100644 > --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c > +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c > @@ -41,6 +41,9 @@ static int fsl_dcu_drm_plane_atomic_check(struct drm_plane *plane, > { > struct drm_framebuffer *fb = state->fb; > > + if (!state->fb || !state->crtc) > + return 0; > + Namely: if we return success here core drm will end up calling the atomic_update... > switch (fb->pixel_format) { > case DRM_FORMAT_RGB565: > case DRM_FORMAT_RGB888: > @@ -85,9 +88,6 @@ static void fsl_dcu_drm_plane_atomic_update(struct drm_plane *plane, > unsigned int alpha, bpp; > int index, ret; > > - if (!fb) > - return; > - ... which no longer has the !fb check, and we'll crash with null deref a few lines below ? -Emil
On 2016-01-08 01:20, Emil Velikov wrote: > Hi guys, > > Am I loosing the plot here or something feels amiss here ? > > On 6 January 2016 at 06:12, Meng Yi <meng.yi@nxp.com> wrote: >> For state->fb or state->crtc may be NULL in fsl_dcu_drm_plane_atomic_check >> function, if so, return 0. >> >> Signed-off-by: Meng Yi <meng.yi@nxp.com> >> Signed-off-by: Jianwei Wang <jianwei.wang.chn@gmail.com> >> >> --- >> >> change in v2: >> -Add state->crtc check >> -return 0 when state->fb or state->crtc is NULL, instead of -EINVAL >> Adviced by Daniel Stone >> >> drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c >> index 4b13cf9..8965580 100644 >> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c >> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c >> @@ -41,6 +41,9 @@ static int fsl_dcu_drm_plane_atomic_check(struct drm_plane *plane, >> { >> struct drm_framebuffer *fb = state->fb; >> >> + if (!state->fb || !state->crtc) >> + return 0; >> + > Namely: if we return success here core drm will end up calling the > atomic_update... > After atomic_check atomic_disable could be called too. However, this seem not directly depend on state'>fb, but more on plane->state->crtc. >> switch (fb->pixel_format) { >> case DRM_FORMAT_RGB565: >> case DRM_FORMAT_RGB888: >> @@ -85,9 +88,6 @@ static void fsl_dcu_drm_plane_atomic_update(struct drm_plane *plane, >> unsigned int alpha, bpp; >> int index, ret; >> >> - if (!fb) >> - return; >> - > ... which no longer has the !fb check, and we'll crash with null deref > a few lines below ? If there is a legitimate situation where fb is null which also ultimately leads to a atomic_commit, I guess we should keep the return here... -- Stefan
> >> switch (fb->pixel_format) { > >> case DRM_FORMAT_RGB565: > >> case DRM_FORMAT_RGB888: > >> @@ -85,9 +88,6 @@ static void fsl_dcu_drm_plane_atomic_update(struct > drm_plane *plane, > >> unsigned int alpha, bpp; > >> int index, ret; > >> > >> - if (!fb) > >> - return; > >> - > > ... which no longer has the !fb check, and we'll crash with null deref > > a few lines below ? > > > If there is a legitimate situation where fb is null which also ultimately leads to a > atomic_commit, I guess we should keep the return here... I think I made a mistake here, fb check should not be removed . As Stefan mentioned, if fb check in fsl_dcu_drm_plane_atomic_check return 0, fsl_dcu_drm_plane_atomic_update will ultimately called, and we'll crash since plane->state->fb is NULL. > -----Original Message----- > From: Stefan Agner [mailto:stefan@agner.ch] > Sent: Thursday, January 14, 2016 1:54 PM > To: Emil Velikov <emil.l.velikov@gmail.com> > Cc: Meng Yi <meng.yi@nxp.com>; ML dri-devel <dri- > devel@lists.freedesktop.org> > Subject: Re: [RESEND,V2] drm: fsl-dcu: Fix no fb check bug > > On 2016-01-08 01:20, Emil Velikov wrote: > > Hi guys, > > > > Am I loosing the plot here or something feels amiss here ? > > > > On 6 January 2016 at 06:12, Meng Yi <meng.yi@nxp.com> wrote: > >> For state->fb or state->crtc may be NULL in > >> fsl_dcu_drm_plane_atomic_check function, if so, return 0. > >> > >> Signed-off-by: Meng Yi <meng.yi@nxp.com> > >> Signed-off-by: Jianwei Wang <jianwei.wang.chn@gmail.com> > >> > >> --- > >> > >> change in v2: > >> -Add state->crtc check > >> -return 0 when state->fb or state->crtc is NULL, instead of -EINVAL > >> Adviced by Daniel Stone > >> > >> drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c | 6 +++--- > >> 1 file changed, 3 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c > >> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c > >> index 4b13cf9..8965580 100644 > >> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c > >> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c > >> @@ -41,6 +41,9 @@ static int fsl_dcu_drm_plane_atomic_check(struct > >> drm_plane *plane, { > >> struct drm_framebuffer *fb = state->fb; > >> > >> + if (!state->fb || !state->crtc) > >> + return 0; > >> + > > Namely: if we return success here core drm will end up calling the > > atomic_update... > > > > After atomic_check atomic_disable could be called too. However, this seem > not directly depend on state'>fb, but more on plane->state->crtc. > > > > >> switch (fb->pixel_format) { > >> case DRM_FORMAT_RGB565: > >> case DRM_FORMAT_RGB888: > >> @@ -85,9 +88,6 @@ static void fsl_dcu_drm_plane_atomic_update(struct > drm_plane *plane, > >> unsigned int alpha, bpp; > >> int index, ret; > >> > >> - if (!fb) > >> - return; > >> - > > ... which no longer has the !fb check, and we'll crash with null deref > > a few lines below ? > > > If there is a legitimate situation where fb is null which also ultimately leads to a > atomic_commit, I guess we should keep the return here... > > -- > Stefan
On 14 January 2016 at 08:23, Meng Yi <meng.yi@nxp.com> wrote: >> >> switch (fb->pixel_format) { >> >> case DRM_FORMAT_RGB565: >> >> case DRM_FORMAT_RGB888: >> >> @@ -85,9 +88,6 @@ static void fsl_dcu_drm_plane_atomic_update(struct >> drm_plane *plane, >> >> unsigned int alpha, bpp; >> >> int index, ret; >> >> >> >> - if (!fb) >> >> - return; >> >> - >> > ... which no longer has the !fb check, and we'll crash with null deref >> > a few lines below ? >> >> >> If there is a legitimate situation where fb is null which also ultimately leads to a >> atomic_commit, I guess we should keep the return here... > > I think I made a mistake here, fb check should not be removed . As Stefan mentioned, if fb check in fsl_dcu_drm_plane_atomic_check return 0, fsl_dcu_drm_plane_atomic_update will ultimately called, and we'll crash since plane->state->fb is NULL. > I believe you meant "Emil" in the above ;-) But seriously... afaict a fair few drivers do a similar !fb (even !state->crtc) check(s)... which makes me wonder if core DRM isn't the better place for it ? Or perhaps that's intentional as core provides the flexibility for each driver to mangle with the fb between .check and .disable ? Cheers Emil P.S. Please don't top post, use interleaved style [1] [1] https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
On 2016-01-26 13:18, Emil Velikov wrote: > On 14 January 2016 at 08:23, Meng Yi <meng.yi@nxp.com> wrote: >>> >> switch (fb->pixel_format) { >>> >> case DRM_FORMAT_RGB565: >>> >> case DRM_FORMAT_RGB888: >>> >> @@ -85,9 +88,6 @@ static void fsl_dcu_drm_plane_atomic_update(struct >>> drm_plane *plane, >>> >> unsigned int alpha, bpp; >>> >> int index, ret; >>> >> >>> >> - if (!fb) >>> >> - return; >>> >> - >>> > ... which no longer has the !fb check, and we'll crash with null deref >>> > a few lines below ? >>> >>> >>> If there is a legitimate situation where fb is null which also ultimately leads to a >>> atomic_commit, I guess we should keep the return here... >> >> I think I made a mistake here, fb check should not be removed . As Stefan mentioned, if fb check in fsl_dcu_drm_plane_atomic_check return 0, fsl_dcu_drm_plane_atomic_update will ultimately called, and we'll crash since plane->state->fb is NULL. >> > I believe you meant "Emil" in the above ;-) But seriously... afaict a > fair few drivers do a similar !fb (even !state->crtc) check(s)... > which makes me wonder if core DRM isn't the better place for it ? Or > perhaps that's intentional as core provides the flexibility for each > driver to mangle with the fb between .check and .disable ? > There seem to be a consensus to check crtc and fb on atomic_check. However, in atomic_update, some drives do a NULL check on crtc only, and some on both, crtc and fb. The comment in drm_atomic_plane_disabling says CRTC and FB should always be NULL together... So I guess it does not really matter all that much, unless there is anyway a bug. Furthermore, it seems that in the null case, atomic_disable is called anyway (which this driver implements). Not sure if there is another case in which either of this two could be NULL and atomic_update could be called. Since this patch is mainly addressing the null check in atomic_check, I will apply it without the change in atomic_update for now. -- Stefan
diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c index 4b13cf9..8965580 100644 --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c @@ -41,6 +41,9 @@ static int fsl_dcu_drm_plane_atomic_check(struct drm_plane *plane, { struct drm_framebuffer *fb = state->fb; + if (!state->fb || !state->crtc) + return 0; + switch (fb->pixel_format) { case DRM_FORMAT_RGB565: case DRM_FORMAT_RGB888: @@ -85,9 +88,6 @@ static void fsl_dcu_drm_plane_atomic_update(struct drm_plane *plane, unsigned int alpha, bpp; int index, ret; - if (!fb) - return; - index = fsl_dcu_drm_plane_index(plane); if (index < 0) return;