| Message ID | 20130219074339.GA5456@avionic-0098.mockup.avionic-design.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Feb 19, 2013 at 8:43 AM, Thierry Reding <thierry.reding@avionic-design.de> wrote: > On Fri, Feb 15, 2013 at 11:24:35AM +0100, Daniel Vetter wrote: >> /me grabs a few brown paper bags >> >> So it looks like I've broken compilation in >> >> commit 6aed8ec3f76a22217c9ae183d32b1aa990bed069 >> Author: Daniel Vetter <daniel.vetter@ffwll.ch> >> Date: Sun Jan 20 17:32:21 2013 +0100 >> >> drm: review locking for drm_fb_helper_restore_fbdev_mode >> >> Fix it up again. >> >> Reported-by: Wu Fengguang <fengguang.wu@intel.com> >> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> >> --- >> drivers/gpu/drm/drm_fb_cma_helper.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c >> index e851658..ef68e34 100644 >> --- a/drivers/gpu/drm/drm_fb_cma_helper.c >> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c >> @@ -377,6 +377,8 @@ EXPORT_SYMBOL_GPL(drm_fbdev_cma_fini); >> */ >> void drm_fbdev_cma_restore_mode(struct drm_fbdev_cma *fbdev_cma) >> { >> + struct drm_device *dev = fbdev_cma->fb_helper.dev; >> + >> drm_modeset_lock_all(dev); >> if (fbdev_cma) >> drm_fb_helper_restore_fbdev_mode(&fbdev_cma->fb_helper); > > The above check indicates that fbdev_cma might be NULL, so you're > potentially dereferencing NULL when assigning the dev variable. Right, looks like a need more brown-paper bags. > Perhaps a better way would be to move the locking into the if block, as > in the patch below. Otoh I think it's a bit funny that you can pass NULL to restore_mode and hotplug_event in the cma fb helper code. Imo it would be better to just ditch those checks ... Laurent? I'll update the patch meanwhile. -Daniel > > Thierry > > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c > index e851658..54a250f 100644 > --- a/drivers/gpu/drm/drm_fb_cma_helper.c > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c > @@ -377,10 +377,11 @@ EXPORT_SYMBOL_GPL(drm_fbdev_cma_fini); > */ > void drm_fbdev_cma_restore_mode(struct drm_fbdev_cma *fbdev_cma) > { > - drm_modeset_lock_all(dev); > - if (fbdev_cma) > + if (fbdev_cma) { > + drm_modeset_lock_all(fbdev_cma->fb_helper.dev); > drm_fb_helper_restore_fbdev_mode(&fbdev_cma->fb_helper); > - drm_modeset_unlock_all(dev); > + drm_modeset_unlock_all(fbdev_cma->fb_helper.dev); > + } > } > EXPORT_SYMBOL_GPL(drm_fbdev_cma_restore_mode);
On Tue, Feb 19, 2013 at 11:16:08AM +0100, Daniel Vetter wrote: > On Tue, Feb 19, 2013 at 8:43 AM, Thierry Reding > <thierry.reding@avionic-design.de> wrote: > > On Fri, Feb 15, 2013 at 11:24:35AM +0100, Daniel Vetter wrote: > >> /me grabs a few brown paper bags > >> > >> So it looks like I've broken compilation in > >> > >> commit 6aed8ec3f76a22217c9ae183d32b1aa990bed069 > >> Author: Daniel Vetter <daniel.vetter@ffwll.ch> > >> Date: Sun Jan 20 17:32:21 2013 +0100 > >> > >> drm: review locking for drm_fb_helper_restore_fbdev_mode > >> > >> Fix it up again. > >> > >> Reported-by: Wu Fengguang <fengguang.wu@intel.com> > >> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > >> --- > >> drivers/gpu/drm/drm_fb_cma_helper.c | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c > >> index e851658..ef68e34 100644 > >> --- a/drivers/gpu/drm/drm_fb_cma_helper.c > >> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c > >> @@ -377,6 +377,8 @@ EXPORT_SYMBOL_GPL(drm_fbdev_cma_fini); > >> */ > >> void drm_fbdev_cma_restore_mode(struct drm_fbdev_cma *fbdev_cma) > >> { > >> + struct drm_device *dev = fbdev_cma->fb_helper.dev; > >> + > >> drm_modeset_lock_all(dev); > >> if (fbdev_cma) > >> drm_fb_helper_restore_fbdev_mode(&fbdev_cma->fb_helper); > > > > The above check indicates that fbdev_cma might be NULL, so you're > > potentially dereferencing NULL when assigning the dev variable. > > Right, looks like a need more brown-paper bags. > > > Perhaps a better way would be to move the locking into the if block, as > > in the patch below. > > Otoh I think it's a bit funny that you can pass NULL to restore_mode > and hotplug_event in the cma fb helper code. Imo it would be better to > just ditch those checks ... Laurent? I'll update the patch meanwhile. I agree, it is probably safe to drop the checks and consider it a driver error (and let it oops accordingly) if NULL is ever passed in. I expect drivers will usually bail out if drm_fbdev_cma_init() fails. If they choose to continue maybe they should be responsible for not calling drm_fbdev_cma_restore_mode() if fbdev is NULL. Thierry
diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c index e851658..54a250f 100644 --- a/drivers/gpu/drm/drm_fb_cma_helper.c +++ b/drivers/gpu/drm/drm_fb_cma_helper.c @@ -377,10 +377,11 @@ EXPORT_SYMBOL_GPL(drm_fbdev_cma_fini); */ void drm_fbdev_cma_restore_mode(struct drm_fbdev_cma *fbdev_cma) { - drm_modeset_lock_all(dev); - if (fbdev_cma) + if (fbdev_cma) { + drm_modeset_lock_all(fbdev_cma->fb_helper.dev); drm_fb_helper_restore_fbdev_mode(&fbdev_cma->fb_helper); - drm_modeset_unlock_all(dev); + drm_modeset_unlock_all(fbdev_cma->fb_helper.dev); + } } EXPORT_SYMBOL_GPL(drm_fbdev_cma_restore_mode);