Re: [PATCH 3/3] drm: Use vblank_disable_and_save in drm_vblank_cleanup()

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Aug 06, 2014 at 01:51:41PM +0300, Ville Syrjälä wrote:
> On Wed, Aug 06, 2014 at 03:22:46AM +0200, Mario Kleiner wrote:
> > Calling vblank_disable_fn() will cause that function to no-op
> > if !dev->vblank_disable_allowed for some kms drivers, e.g.,
> > on nouveau-kms. This can cause the gpu vblank irq's to not get
> > disabled before freeing the dev->vblank array, so if a
> > vblank irq fires and calls into drm_handle_vblank() after
> > drm_vblank_cleanup() completes, it will cause use-after-free
> > access to dev->vblank array.
> > 
> > Call vblank_disable_and_save unconditionally, so vblank irqs
> > are guaranteed to be off, before we delete the data structures
> > on which they operate.
> > 
> > Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
> > Cc: stable@vger.kernel.org

Imo cc: stable isn't justified for these patches which fix stuff that
normal users don't really see (driver load failure and module reload for
kms drivers never tends to happen for normal users).

So I've dropped that and pulled the 2 patches Ville reviewd into my
topic/vblank-rework branch for 3.18.

Thanks, Daniel

> 
> No idea what games nouveau is playign with that flag, but this patch
> should be fine at least for drivers that don't do such things.
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> > ---
> >  drivers/gpu/drm/drm_irq.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> > index 89e91e3..22e2bba9 100644
> > --- a/drivers/gpu/drm/drm_irq.c
> > +++ b/drivers/gpu/drm/drm_irq.c
> > @@ -164,6 +164,7 @@ static void vblank_disable_fn(unsigned long arg)
> >  void drm_vblank_cleanup(struct drm_device *dev)
> >  {
> >  	int crtc;
> > +	unsigned long irqflags;
> >  
> >  	/* Bail if the driver didn't call drm_vblank_init() */
> >  	if (dev->num_crtcs == 0)
> > @@ -171,7 +172,9 @@ void drm_vblank_cleanup(struct drm_device *dev)
> >  
> >  	for (crtc = 0; crtc < dev->num_crtcs; crtc++) {
> >  		del_timer_sync(&dev->vblank[crtc].disable_timer);
> > -		vblank_disable_fn((unsigned long)&dev->vblank[crtc]);
> > +		spin_lock_irqsave(&dev->vbl_lock, irqflags);
> > +		vblank_disable_and_save(dev, crtc);
> > +		spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> >  	}
> >  
> >  	kfree(dev->vblank);
> > -- 
> > 1.9.1
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch