diff mbox

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

Message ID 1407288166-19881-4-git-send-email-mario.kleiner.de@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Mario Kleiner Aug. 6, 2014, 1:22 a.m. UTC
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
---
 drivers/gpu/drm/drm_irq.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Ville Syrjälä Aug. 6, 2014, 10:51 a.m. UTC | #1
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

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
Daniel Vetter Aug. 6, 2014, 1:57 p.m. UTC | #2
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
Mario Kleiner Aug. 7, 2014, 12:50 a.m. UTC | #3
On 08/06/2014 03:57 PM, Daniel Vetter wrote:
> 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

Ok, good with me, thanks. Ville, thanks for the review. I'll review and 
test your vblank series next week when i have access to suitable 
machines and enough time. I need to go through this in single-step mode, 
vblank on/off changes always make me nervous, given how dependent my 
main application is on this for its timing, so i want to move through it 
in slow motion.

Btw. wrt to nouveau "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." (Villes comment).

Nouveau currently doesn't support hw vblank counter queries at all. The 
dev->driver->get_vblank_count() is just hooked up to drm_vblank_count(), 
so it's a no-op. Therefore nouveau can't allow disabling of vblank irq 
during "normal" operation as it would lose all vblank counts during the 
off period. That's why it leaves dev->vblank_disable_allowed = false;

Pre NV-50 apparently doesn't have any hw vblank counter register, but 
NV50+ seems to have one. I'll probably give implementing this a try for 
3.18 if nobody else does.

I'm not sure about all the new embedded drivers, if they have hw vblank 
counters?

thanks,
-mario

>
>> 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 Aug. 7, 2014, 6:50 a.m. UTC | #4
On Thu, Aug 7, 2014 at 2:50 AM, Mario Kleiner
<mario.kleiner.de@gmail.com> wrote:
> I'm not sure about all the new embedded drivers, if they have hw vblank
> counters?

Quick grep says a lot don't have it or at least not implemented - they
use drm_vblank_count. Thinking about this, should we use that as a
signal to also set dev->vblnka_disable_allowed = false in
drm_vblank_init?
-Daniel
Daniel Vetter Aug. 7, 2014, 6:57 a.m. UTC | #5
On Thu, Aug 7, 2014 at 2:50 AM, Mario Kleiner
<mario.kleiner.de@gmail.com> wrote:
> Ok, good with me, thanks. Ville, thanks for the review. I'll review and test
> your vblank series next week when i have access to suitable machines and
> enough time. I need to go through this in single-step mode, vblank on/off
> changes always make me nervous, given how dependent my main application is
> on this for its timing, so i want to move through it in slow motion.

btw I've pulled in your patches and the one's from Ville that I think
are sane into a topic branch, in case that helps:

http://cgit.freedesktop.org/drm-intel/log/?h=topic/vblank-rework

Cheers, Daniel
Mario Kleiner Aug. 7, 2014, 3:13 p.m. UTC | #6
On 08/07/2014 08:50 AM, Daniel Vetter wrote:
> On Thu, Aug 7, 2014 at 2:50 AM, Mario Kleiner
> <mario.kleiner.de@gmail.com> wrote:
>> I'm not sure about all the new embedded drivers, if they have hw vblank
>> counters?
> Quick grep says a lot don't have it or at least not implemented - they
> use drm_vblank_count. Thinking about this, should we use that as a
> signal to also set dev->vblnka_disable_allowed = false in
> drm_vblank_init?
> -Daniel

dev->vblank_disable_allowed = false; is already the default set in drm_vblank_init().
I think it was the idea of that flag that drivers which support a somewhat correct vblank dis/enable (= have useable hw vblank counters) opt-in to the vblank disable after some idle time by setting it to true.

The strange thing is that unconditional dev->vblank_disable_allowed = 
true in drm_vblank_post_modeset()?
It's there since the first introduction of the flag.

The i915, gma500, armada and exynos drivers explicitely set the flag to 
true to opt-in to the auto vblank disable. radeon gets it implicitely 
set by calling the vblank_post_modeset function. Tegra also gets it via 
the post_modeset, and nouveau on old cards with nv04 display engine.

armada, exynos and tegra don't have proper hw vblank counter queries, 
but opt-in, so those will lose vblank counts whenever vblank irqs get 
turned off.

It's a bit all over the place.
-mario
diff mbox

Patch

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);