[PATCH] drm/core: Fix ordering in drm_mode_config_cleanup.

[PATCH] drm/core: Fix ordering in drm_mode_config_cleanup.

[Maarten Lankhorst]

__drm_atomic_helper_plane_destroy_state calls
drm_framebuffer_unreference, which means that if drm_framebuffer_free
is called before plane->destroy freed memory will be accessed.

A similar case happens for the blob list, which was freed before the
crtc state was, resulting in the unreference_blob from crtc_destroy_state
pointing to garbage memory causing another opportunity for a GPF.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 51c5a00ffdff..5a13b1afccbe 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -5958,6 +5958,15 @@ void drm_mode_config_cleanup(struct drm_device *dev)
 		drm_property_destroy(dev, property);
 	}
 
+	list_for_each_entry_safe(plane, plt, &dev->mode_config.plane_list,
+				 head) {
+		plane->funcs->destroy(plane);
+	}
+
+	list_for_each_entry_safe(crtc, ct, &dev->mode_config.crtc_list, head) {
+		crtc->funcs->destroy(crtc);
+	}
+
 	list_for_each_entry_safe(blob, bt, &dev->mode_config.property_blob_list,
 				 head_global) {
 		drm_property_unreference_blob(blob);
@@ -5976,15 +5985,6 @@ void drm_mode_config_cleanup(struct drm_device *dev)
 		drm_framebuffer_free(&fb->refcount);
 	}
 
-	list_for_each_entry_safe(plane, plt, &dev->mode_config.plane_list,
-				 head) {
-		plane->funcs->destroy(plane);
-	}
-
-	list_for_each_entry_safe(crtc, ct, &dev->mode_config.crtc_list, head) {
-		crtc->funcs->destroy(crtc);
-	}
-
 	ida_destroy(&dev->mode_config.connector_ida);
 	idr_destroy(&dev->mode_config.tile_idr);
 	idr_destroy(&dev->mode_config.crtc_idr);
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/core: Fix ordering in drm_mode_config_cleanup.

[Ville Syrjälä]

On Tue, Mar 22, 2016 at 03:42:14PM +0100, Maarten Lankhorst wrote:
> __drm_atomic_helper_plane_destroy_state calls
> drm_framebuffer_unreference, which means that if drm_framebuffer_free
> is called before plane->destroy freed memory will be accessed.
> 
> A similar case happens for the blob list, which was freed before the
> crtc state was, resulting in the unreference_blob from crtc_destroy_state
> pointing to garbage memory causing another opportunity for a GPF.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_crtc.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 51c5a00ffdff..5a13b1afccbe 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -5958,6 +5958,15 @@ void drm_mode_config_cleanup(struct drm_device *dev)
>  		drm_property_destroy(dev, property);
>  	}

And what about props? Any chance of explosion due to those being gone?

>  
> +	list_for_each_entry_safe(plane, plt, &dev->mode_config.plane_list,
> +				 head) {
> +		plane->funcs->destroy(plane);
> +	}
> +
> +	list_for_each_entry_safe(crtc, ct, &dev->mode_config.crtc_list, head) {
> +		crtc->funcs->destroy(crtc);
> +	}
> +
>  	list_for_each_entry_safe(blob, bt, &dev->mode_config.property_blob_list,
>  				 head_global) {
>  		drm_property_unreference_blob(blob);
> @@ -5976,15 +5985,6 @@ void drm_mode_config_cleanup(struct drm_device *dev)
>  		drm_framebuffer_free(&fb->refcount);
>  	}
>  
> -	list_for_each_entry_safe(plane, plt, &dev->mode_config.plane_list,
> -				 head) {
> -		plane->funcs->destroy(plane);
> -	}
> -
> -	list_for_each_entry_safe(crtc, ct, &dev->mode_config.crtc_list, head) {
> -		crtc->funcs->destroy(crtc);
> -	}
> -
>  	ida_destroy(&dev->mode_config.connector_ida);
>  	idr_destroy(&dev->mode_config.tile_idr);
>  	idr_destroy(&dev->mode_config.crtc_idr);
> -- 
> 2.1.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/core: Fix ordering in drm_mode_config_cleanup.

[Maarten Lankhorst]

Op 22-03-16 om 15:58 schreef Ville Syrjälä:
> On Tue, Mar 22, 2016 at 03:42:14PM +0100, Maarten Lankhorst wrote:
>> __drm_atomic_helper_plane_destroy_state calls
>> drm_framebuffer_unreference, which means that if drm_framebuffer_free
>> is called before plane->destroy freed memory will be accessed.
>>
>> A similar case happens for the blob list, which was freed before the
>> crtc state was, resulting in the unreference_blob from crtc_destroy_state
>> pointing to garbage memory causing another opportunity for a GPF.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/drm_crtc.c | 18 +++++++++---------
>>  1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index 51c5a00ffdff..5a13b1afccbe 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -5958,6 +5958,15 @@ void drm_mode_config_cleanup(struct drm_device *dev)
>>  		drm_property_destroy(dev, property);
>>  	}
> And what about props? Any chance of explosion due to those being gone?
>
Not as far as I'm aware of.

If you use something like a crtc_x property, only the value gets written to crtc_state, the value is an int that would still be valid.

~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/core: Fix ordering in drm_mode_config_cleanup.

[Patchwork]

== Series Details ==

Series: drm/core: Fix ordering in drm_mode_config_cleanup.
URL   : https://patchwork.freedesktop.org/series/4758/
State : success

== Summary ==

Series 4758v1 drm/core: Fix ordering in drm_mode_config_cleanup.
http://patchwork.freedesktop.org/api/1.0/series/4758/revisions/1/mbox/

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-c:
                dmesg-warn -> PASS       (bsw-nuc-2)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                dmesg-warn -> PASS       (byt-nuc)
        Subgroup basic-rte:
                pass       -> DMESG-WARN (byt-nuc) UNSTABLE

bdw-nuci7        total:192  pass:180  dwarn:0   dfail:0   fail:0   skip:12 
bdw-ultra        total:192  pass:171  dwarn:0   dfail:0   fail:0   skip:21 
bsw-nuc-2        total:192  pass:154  dwarn:1   dfail:0   fail:0   skip:37 
byt-nuc          total:192  pass:156  dwarn:1   dfail:0   fail:0   skip:35 
hsw-brixbox      total:192  pass:170  dwarn:0   dfail:0   fail:0   skip:22 
hsw-gt2          total:192  pass:175  dwarn:0   dfail:0   fail:0   skip:17 
ivb-t430s        total:192  pass:167  dwarn:0   dfail:0   fail:0   skip:25 
skl-i5k-2        total:192  pass:169  dwarn:0   dfail:0   fail:0   skip:23 
skl-i7k-2        total:192  pass:169  dwarn:0   dfail:0   fail:0   skip:23 
skl-nuci5        total:192  pass:181  dwarn:0   dfail:0   fail:0   skip:11 
snb-dellxps      total:192  pass:158  dwarn:0   dfail:0   fail:0   skip:34 
snb-x220t        total:192  pass:158  dwarn:0   dfail:0   fail:1   skip:33 

Results at /archive/results/CI_IGT_test/Patchwork_1679/

83ed25fa1b956275542da63eb98dc8fd2291329d drm-intel-nightly: 2016y-03m-22d-15h-20m-55s UTC integration manifest
2fb96a0c0489a7c3c751ae69e7d13bf503d2ce25 drm/core: Fix ordering in drm_mode_config_cleanup.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/core: Fix ordering in drm_mode_config_cleanup.

[Ville Syrjälä]

On Tue, Mar 22, 2016 at 04:08:39PM +0100, Maarten Lankhorst wrote:
> Op 22-03-16 om 15:58 schreef Ville Syrjälä:
> > On Tue, Mar 22, 2016 at 03:42:14PM +0100, Maarten Lankhorst wrote:
> >> __drm_atomic_helper_plane_destroy_state calls
> >> drm_framebuffer_unreference, which means that if drm_framebuffer_free
> >> is called before plane->destroy freed memory will be accessed.
> >>
> >> A similar case happens for the blob list, which was freed before the
> >> crtc state was, resulting in the unreference_blob from crtc_destroy_state
> >> pointing to garbage memory causing another opportunity for a GPF.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/drm_crtc.c | 18 +++++++++---------
> >>  1 file changed, 9 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> >> index 51c5a00ffdff..5a13b1afccbe 100644
> >> --- a/drivers/gpu/drm/drm_crtc.c
> >> +++ b/drivers/gpu/drm/drm_crtc.c
> >> @@ -5958,6 +5958,15 @@ void drm_mode_config_cleanup(struct drm_device *dev)
> >>  		drm_property_destroy(dev, property);
> >>  	}
> > And what about props? Any chance of explosion due to those being gone?
> >
> Not as far as I'm aware of.
> 
> If you use something like a crtc_x property, only the value gets written to crtc_state, the value is an int that would still be valid.

I was too lazy to confirm this for all drivers. But at least i915 looked
clean on that front. So

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> ~Maarten

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/core: Fix ordering in drm_mode_config_cleanup.

[Daniel Vetter]

On Fri, Apr 01, 2016 at 11:04:10PM +0300, Ville Syrjälä wrote:
> On Tue, Mar 22, 2016 at 04:08:39PM +0100, Maarten Lankhorst wrote:
> > Op 22-03-16 om 15:58 schreef Ville Syrjälä:
> > > On Tue, Mar 22, 2016 at 03:42:14PM +0100, Maarten Lankhorst wrote:
> > >> __drm_atomic_helper_plane_destroy_state calls
> > >> drm_framebuffer_unreference, which means that if drm_framebuffer_free
> > >> is called before plane->destroy freed memory will be accessed.
> > >>
> > >> A similar case happens for the blob list, which was freed before the
> > >> crtc state was, resulting in the unreference_blob from crtc_destroy_state
> > >> pointing to garbage memory causing another opportunity for a GPF.
> > >>
> > >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > >> ---
> > >>  drivers/gpu/drm/drm_crtc.c | 18 +++++++++---------
> > >>  1 file changed, 9 insertions(+), 9 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > >> index 51c5a00ffdff..5a13b1afccbe 100644
> > >> --- a/drivers/gpu/drm/drm_crtc.c
> > >> +++ b/drivers/gpu/drm/drm_crtc.c
> > >> @@ -5958,6 +5958,15 @@ void drm_mode_config_cleanup(struct drm_device *dev)
> > >>  		drm_property_destroy(dev, property);
> > >>  	}
> > > And what about props? Any chance of explosion due to those being gone?
> > >
> > Not as far as I'm aware of.
> > 
> > If you use something like a crtc_x property, only the value gets written to crtc_state, the value is an int that would still be valid.
> 
> I was too lazy to confirm this for all drivers. But at least i915 looked
> clean on that front. So
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Applied to drm-misc, thanks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx