LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] drm: udl: Destroy framebuffer only if it was initialized
@ 2018-04-20 11:50 Emil Lundmark
  2018-04-20 13:55 ` Sean Paul
  2018-05-28 14:27 ` [PATCH v2] " Emil Lundmark
  0 siblings, 2 replies; 8+ messages in thread
From: Emil Lundmark @ 2018-04-20 11:50 UTC (permalink / raw)
  To: dri-devel; +Cc: Dave Airlie, Sean Paul, linux-kernel, Emil Lundmark

This fixes a NULL pointer dereference that can happen if the UDL
driver is unloaded before the framebuffer is initialized. This can
happen e.g. if the USB device is unplugged right after it was plugged
in.

Signed-off-by: Emil Lundmark <lndmrk@chromium.org>
---
 drivers/gpu/drm/udl/udl_fb.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
index 2ebdc6d5a76e..5754e37f741b 100644
--- a/drivers/gpu/drm/udl/udl_fb.c
+++ b/drivers/gpu/drm/udl/udl_fb.c
@@ -426,9 +426,11 @@ static void udl_fbdev_destroy(struct drm_device *dev,
 {
 	drm_fb_helper_unregister_fbi(&ufbdev->helper);
 	drm_fb_helper_fini(&ufbdev->helper);
-	drm_framebuffer_unregister_private(&ufbdev->ufb.base);
-	drm_framebuffer_cleanup(&ufbdev->ufb.base);
-	drm_gem_object_put_unlocked(&ufbdev->ufb.obj->base);
+	if (ufbdev->ufb.obj) {
+		drm_framebuffer_unregister_private(&ufbdev->ufb.base);
+		drm_framebuffer_cleanup(&ufbdev->ufb.base);
+		drm_gem_object_put_unlocked(&ufbdev->ufb.obj->base);
+	}
 }
 
 int udl_fbdev_init(struct drm_device *dev)
-- 
2.17.0.484.g0c8726318c-goog

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] drm: udl: Destroy framebuffer only if it was initialized
  2018-04-20 11:50 [PATCH] drm: udl: Destroy framebuffer only if it was initialized Emil Lundmark
@ 2018-04-20 13:55 ` Sean Paul
  2018-04-24 13:04   ` Daniel Vetter
  2018-05-28 14:27 ` [PATCH v2] " Emil Lundmark
  1 sibling, 1 reply; 8+ messages in thread
From: Sean Paul @ 2018-04-20 13:55 UTC (permalink / raw)
  To: Emil Lundmark; +Cc: dri-devel, Dave Airlie, Sean Paul, linux-kernel

On Fri, Apr 20, 2018 at 01:50:01PM +0200, Emil Lundmark wrote:
> This fixes a NULL pointer dereference that can happen if the UDL
> driver is unloaded before the framebuffer is initialized. This can
> happen e.g. if the USB device is unplugged right after it was plugged
> in.
> 

JFYI, in future, if someone makes a suggestion on how to fix a bug, it's good
practice to add a Suggested-by tag to give credit.

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> Signed-off-by: Emil Lundmark <lndmrk@chromium.org>
> ---
>  drivers/gpu/drm/udl/udl_fb.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
> index 2ebdc6d5a76e..5754e37f741b 100644
> --- a/drivers/gpu/drm/udl/udl_fb.c
> +++ b/drivers/gpu/drm/udl/udl_fb.c
> @@ -426,9 +426,11 @@ static void udl_fbdev_destroy(struct drm_device *dev,
>  {
>  	drm_fb_helper_unregister_fbi(&ufbdev->helper);
>  	drm_fb_helper_fini(&ufbdev->helper);
> -	drm_framebuffer_unregister_private(&ufbdev->ufb.base);
> -	drm_framebuffer_cleanup(&ufbdev->ufb.base);
> -	drm_gem_object_put_unlocked(&ufbdev->ufb.obj->base);
> +	if (ufbdev->ufb.obj) {
> +		drm_framebuffer_unregister_private(&ufbdev->ufb.base);
> +		drm_framebuffer_cleanup(&ufbdev->ufb.base);
> +		drm_gem_object_put_unlocked(&ufbdev->ufb.obj->base);
> +	}
>  }
>  
>  int udl_fbdev_init(struct drm_device *dev)
> -- 
> 2.17.0.484.g0c8726318c-goog
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] drm: udl: Destroy framebuffer only if it was initialized
  2018-04-20 13:55 ` Sean Paul
@ 2018-04-24 13:04   ` Daniel Vetter
  2018-04-25 22:56     ` Stéphane Marchesin
  0 siblings, 1 reply; 8+ messages in thread
From: Daniel Vetter @ 2018-04-24 13:04 UTC (permalink / raw)
  To: Sean Paul; +Cc: Emil Lundmark, Dave Airlie, linux-kernel, dri-devel

On Fri, Apr 20, 2018 at 09:55:32AM -0400, Sean Paul wrote:
> On Fri, Apr 20, 2018 at 01:50:01PM +0200, Emil Lundmark wrote:
> > This fixes a NULL pointer dereference that can happen if the UDL
> > driver is unloaded before the framebuffer is initialized. This can
> > happen e.g. if the USB device is unplugged right after it was plugged
> > in.
> > 
> 
> JFYI, in future, if someone makes a suggestion on how to fix a bug, it's good
> practice to add a Suggested-by tag to give credit.
> 
> Reviewed-by: Sean Paul <seanpaul@chromium.org>

I think a bit more detail in the commit message on why this is even
possible would be good. I think it can only happen when you only plug in
the udl, without anything connected.

In that case fbdev setup will be delayed until something shows up (so we
don't pin a too small fb for it, a feature requested by soc people). This
can easily be tested:
First:
- plug in udl device
- wait a minute or so (to make it clear it's not a race)
- unplug

And then:
- plug in an udl device, but with something connected.
- unplug right away.

I expect that in the first case you'll always blow up, but in the 2nd one
you'll never blow up (no matter how fast you unplug).

Cheers, Daniel



> 
> > Signed-off-by: Emil Lundmark <lndmrk@chromium.org>
> > ---
> >  drivers/gpu/drm/udl/udl_fb.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
> > index 2ebdc6d5a76e..5754e37f741b 100644
> > --- a/drivers/gpu/drm/udl/udl_fb.c
> > +++ b/drivers/gpu/drm/udl/udl_fb.c
> > @@ -426,9 +426,11 @@ static void udl_fbdev_destroy(struct drm_device *dev,
> >  {
> >  	drm_fb_helper_unregister_fbi(&ufbdev->helper);
> >  	drm_fb_helper_fini(&ufbdev->helper);
> > -	drm_framebuffer_unregister_private(&ufbdev->ufb.base);
> > -	drm_framebuffer_cleanup(&ufbdev->ufb.base);
> > -	drm_gem_object_put_unlocked(&ufbdev->ufb.obj->base);
> > +	if (ufbdev->ufb.obj) {
> > +		drm_framebuffer_unregister_private(&ufbdev->ufb.base);
> > +		drm_framebuffer_cleanup(&ufbdev->ufb.base);
> > +		drm_gem_object_put_unlocked(&ufbdev->ufb.obj->base);
> > +	}
> >  }
> >  
> >  int udl_fbdev_init(struct drm_device *dev)
> > -- 
> > 2.17.0.484.g0c8726318c-goog
> > 
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] drm: udl: Destroy framebuffer only if it was initialized
  2018-04-24 13:04   ` Daniel Vetter
@ 2018-04-25 22:56     ` Stéphane Marchesin
  2018-04-26  9:50       ` Daniel Vetter
  0 siblings, 1 reply; 8+ messages in thread
From: Stéphane Marchesin @ 2018-04-25 22:56 UTC (permalink / raw)
  To: Sean Paul, Emil Lundmark, Dave Airlie, Linux Kernel list, dri-devel

On Tue, Apr 24, 2018 at 6:04 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, Apr 20, 2018 at 09:55:32AM -0400, Sean Paul wrote:
>> On Fri, Apr 20, 2018 at 01:50:01PM +0200, Emil Lundmark wrote:
>> > This fixes a NULL pointer dereference that can happen if the UDL
>> > driver is unloaded before the framebuffer is initialized. This can
>> > happen e.g. if the USB device is unplugged right after it was plugged
>> > in.
>> >
>>
>> JFYI, in future, if someone makes a suggestion on how to fix a bug, it's good
>> practice to add a Suggested-by tag to give credit.
>>
>> Reviewed-by: Sean Paul <seanpaul@chromium.org>
>
> I think a bit more detail in the commit message on why this is even
> possible would be good. I think it can only happen when you only plug in
> the udl, without anything connected.

Hehe, I was just reviewing this code internally, so I can answer that one :)

It happens when fbdev is disabled (which is the case for Chrome OS).
Even though intialization of the fbdev part is optional (it's done in
udlfb_create which is the callback for fb_probe()), the teardown isn't
optional (udl_driver_unload -> udl_fbdev_cleanup ->
udl_fbdev_destroy).

Note that udl_fbdev_cleanup *tries* to be conditional (you can see it
does if (!udl->fbdev)) but that doesn't work, because udl->fbdev is
always set during udl_fbdev_init.

Stéphane


>
> In that case fbdev setup will be delayed until something shows up (so we
> don't pin a too small fb for it, a feature requested by soc people). This
> can easily be tested:
> First:
> - plug in udl device
> - wait a minute or so (to make it clear it's not a race)
> - unplug
>
> And then:
> - plug in an udl device, but with something connected.
> - unplug right away.
>
> I expect that in the first case you'll always blow up, but in the 2nd one
> you'll never blow up (no matter how fast you unplug).
>
> Cheers, Daniel
>
>
>
>>
>> > Signed-off-by: Emil Lundmark <lndmrk@chromium.org>
>> > ---
>> >  drivers/gpu/drm/udl/udl_fb.c | 8 +++++---
>> >  1 file changed, 5 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
>> > index 2ebdc6d5a76e..5754e37f741b 100644
>> > --- a/drivers/gpu/drm/udl/udl_fb.c
>> > +++ b/drivers/gpu/drm/udl/udl_fb.c
>> > @@ -426,9 +426,11 @@ static void udl_fbdev_destroy(struct drm_device *dev,
>> >  {
>> >     drm_fb_helper_unregister_fbi(&ufbdev->helper);
>> >     drm_fb_helper_fini(&ufbdev->helper);
>> > -   drm_framebuffer_unregister_private(&ufbdev->ufb.base);
>> > -   drm_framebuffer_cleanup(&ufbdev->ufb.base);
>> > -   drm_gem_object_put_unlocked(&ufbdev->ufb.obj->base);
>> > +   if (ufbdev->ufb.obj) {
>> > +           drm_framebuffer_unregister_private(&ufbdev->ufb.base);
>> > +           drm_framebuffer_cleanup(&ufbdev->ufb.base);
>> > +           drm_gem_object_put_unlocked(&ufbdev->ufb.obj->base);
>> > +   }
>> >  }
>> >
>> >  int udl_fbdev_init(struct drm_device *dev)
>> > --
>> > 2.17.0.484.g0c8726318c-goog
>> >
>>
>> --
>> Sean Paul, Software Engineer, Google / Chromium OS
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] drm: udl: Destroy framebuffer only if it was initialized
  2018-04-25 22:56     ` Stéphane Marchesin
@ 2018-04-26  9:50       ` Daniel Vetter
  0 siblings, 0 replies; 8+ messages in thread
From: Daniel Vetter @ 2018-04-26  9:50 UTC (permalink / raw)
  To: Stéphane Marchesin
  Cc: Sean Paul, Emil Lundmark, Dave Airlie, Linux Kernel list, dri-devel

On Thu, Apr 26, 2018 at 12:56 AM, Stéphane Marchesin
<stephane.marchesin@gmail.com> wrote:
> On Tue, Apr 24, 2018 at 6:04 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Fri, Apr 20, 2018 at 09:55:32AM -0400, Sean Paul wrote:
>>> On Fri, Apr 20, 2018 at 01:50:01PM +0200, Emil Lundmark wrote:
>>> > This fixes a NULL pointer dereference that can happen if the UDL
>>> > driver is unloaded before the framebuffer is initialized. This can
>>> > happen e.g. if the USB device is unplugged right after it was plugged
>>> > in.
>>> >
>>>
>>> JFYI, in future, if someone makes a suggestion on how to fix a bug, it's good
>>> practice to add a Suggested-by tag to give credit.
>>>
>>> Reviewed-by: Sean Paul <seanpaul@chromium.org>
>>
>> I think a bit more detail in the commit message on why this is even
>> possible would be good. I think it can only happen when you only plug in
>> the udl, without anything connected.
>
> Hehe, I was just reviewing this code internally, so I can answer that one :)
>
> It happens when fbdev is disabled (which is the case for Chrome OS).
> Even though intialization of the fbdev part is optional (it's done in
> udlfb_create which is the callback for fb_probe()), the teardown isn't
> optional (udl_driver_unload -> udl_fbdev_cleanup ->
> udl_fbdev_destroy).
>
> Note that udl_fbdev_cleanup *tries* to be conditional (you can see it
> does if (!udl->fbdev)) but that doesn't work, because udl->fbdev is
> always set during udl_fbdev_init.

Ah right, that's another scenario where fbdev never gets fully
initialized, totally forgot about it :-)

It's a bit unfortunately that we don't have a simple fix for this,
i915 has a bunch of messy such checks too. Hopefully Noralf's work to
make fbdev emulation look more like a normal kms client (which just
happens to run within the kernel) will help here in the long run.
-Daniel


>
> Stéphane
>
>
>>
>> In that case fbdev setup will be delayed until something shows up (so we
>> don't pin a too small fb for it, a feature requested by soc people). This
>> can easily be tested:
>> First:
>> - plug in udl device
>> - wait a minute or so (to make it clear it's not a race)
>> - unplug
>>
>> And then:
>> - plug in an udl device, but with something connected.
>> - unplug right away.
>>
>> I expect that in the first case you'll always blow up, but in the 2nd one
>> you'll never blow up (no matter how fast you unplug).
>>
>> Cheers, Daniel
>>
>>
>>
>>>
>>> > Signed-off-by: Emil Lundmark <lndmrk@chromium.org>
>>> > ---
>>> >  drivers/gpu/drm/udl/udl_fb.c | 8 +++++---
>>> >  1 file changed, 5 insertions(+), 3 deletions(-)
>>> >
>>> > diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
>>> > index 2ebdc6d5a76e..5754e37f741b 100644
>>> > --- a/drivers/gpu/drm/udl/udl_fb.c
>>> > +++ b/drivers/gpu/drm/udl/udl_fb.c
>>> > @@ -426,9 +426,11 @@ static void udl_fbdev_destroy(struct drm_device *dev,
>>> >  {
>>> >     drm_fb_helper_unregister_fbi(&ufbdev->helper);
>>> >     drm_fb_helper_fini(&ufbdev->helper);
>>> > -   drm_framebuffer_unregister_private(&ufbdev->ufb.base);
>>> > -   drm_framebuffer_cleanup(&ufbdev->ufb.base);
>>> > -   drm_gem_object_put_unlocked(&ufbdev->ufb.obj->base);
>>> > +   if (ufbdev->ufb.obj) {
>>> > +           drm_framebuffer_unregister_private(&ufbdev->ufb.base);
>>> > +           drm_framebuffer_cleanup(&ufbdev->ufb.base);
>>> > +           drm_gem_object_put_unlocked(&ufbdev->ufb.obj->base);
>>> > +   }
>>> >  }
>>> >
>>> >  int udl_fbdev_init(struct drm_device *dev)
>>> > --
>>> > 2.17.0.484.g0c8726318c-goog
>>> >
>>>
>>> --
>>> Sean Paul, Software Engineer, Google / Chromium OS
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2] drm: udl: Destroy framebuffer only if it was initialized
  2018-04-20 11:50 [PATCH] drm: udl: Destroy framebuffer only if it was initialized Emil Lundmark
  2018-04-20 13:55 ` Sean Paul
@ 2018-05-28 14:27 ` Emil Lundmark
  2018-05-29  6:46   ` Daniel Vetter
  2018-09-10 14:23   ` Sean Paul
  1 sibling, 2 replies; 8+ messages in thread
From: Emil Lundmark @ 2018-05-28 14:27 UTC (permalink / raw)
  To: dri-devel
  Cc: Dave Airlie, Sean Paul, linux-kernel, Emil Lundmark,
	Daniel Vetter, Stéphane Marchesin

This fixes a NULL pointer dereference that can happen if the UDL
driver is unloaded before the framebuffer is initialized. This can
happen e.g. if the USB device is unplugged right after it was plugged
in.

As explained by Stéphane Marchesin:

It happens when fbdev is disabled (which is the case for Chrome OS).
Even though intialization of the fbdev part is optional (it's done in
udlfb_create which is the callback for fb_probe()), the teardown isn't
optional (udl_driver_unload -> udl_fbdev_cleanup ->
udl_fbdev_destroy).

Note that udl_fbdev_cleanup *tries* to be conditional (you can see it
does if (!udl->fbdev)) but that doesn't work, because udl->fbdev is
always set during udl_fbdev_init.

Suggested-by: Sean Paul <seanpaul@chromium.org>
Signed-off-by: Emil Lundmark <lndmrk@chromium.org>
---
Changes in v2:
- Updated commit message with explanation from Stéphane Marchesin

 drivers/gpu/drm/udl/udl_fb.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
index 2ebdc6d5a76e..5754e37f741b 100644
--- a/drivers/gpu/drm/udl/udl_fb.c
+++ b/drivers/gpu/drm/udl/udl_fb.c
@@ -426,9 +426,11 @@ static void udl_fbdev_destroy(struct drm_device *dev,
 {
 	drm_fb_helper_unregister_fbi(&ufbdev->helper);
 	drm_fb_helper_fini(&ufbdev->helper);
-	drm_framebuffer_unregister_private(&ufbdev->ufb.base);
-	drm_framebuffer_cleanup(&ufbdev->ufb.base);
-	drm_gem_object_put_unlocked(&ufbdev->ufb.obj->base);
+	if (ufbdev->ufb.obj) {
+		drm_framebuffer_unregister_private(&ufbdev->ufb.base);
+		drm_framebuffer_cleanup(&ufbdev->ufb.base);
+		drm_gem_object_put_unlocked(&ufbdev->ufb.obj->base);
+	}
 }
 
 int udl_fbdev_init(struct drm_device *dev)
-- 
2.17.0.921.gf22659ad46-goog

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] drm: udl: Destroy framebuffer only if it was initialized
  2018-05-28 14:27 ` [PATCH v2] " Emil Lundmark
@ 2018-05-29  6:46   ` Daniel Vetter
  2018-09-10 14:23   ` Sean Paul
  1 sibling, 0 replies; 8+ messages in thread
From: Daniel Vetter @ 2018-05-29  6:46 UTC (permalink / raw)
  To: Emil Lundmark
  Cc: dri-devel, Dave Airlie, Sean Paul, linux-kernel, Daniel Vetter,
	Stéphane Marchesin

On Mon, May 28, 2018 at 04:27:11PM +0200, Emil Lundmark wrote:
> This fixes a NULL pointer dereference that can happen if the UDL
> driver is unloaded before the framebuffer is initialized. This can
> happen e.g. if the USB device is unplugged right after it was plugged
> in.
> 
> As explained by Stéphane Marchesin:
> 
> It happens when fbdev is disabled (which is the case for Chrome OS).
> Even though intialization of the fbdev part is optional (it's done in
> udlfb_create which is the callback for fb_probe()), the teardown isn't
> optional (udl_driver_unload -> udl_fbdev_cleanup ->
> udl_fbdev_destroy).
> 
> Note that udl_fbdev_cleanup *tries* to be conditional (you can see it
> does if (!udl->fbdev)) but that doesn't work, because udl->fbdev is
> always set during udl_fbdev_init.
> 
> Suggested-by: Sean Paul <seanpaul@chromium.org>
> Signed-off-by: Emil Lundmark <lndmrk@chromium.org>

You lost the r-b from Sean when resending, I'll leave that to Sean to
readd when he merges. Anyway, lgtm now with the more detailed explanation.

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> Changes in v2:
> - Updated commit message with explanation from Stéphane Marchesin
> 
>  drivers/gpu/drm/udl/udl_fb.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
> index 2ebdc6d5a76e..5754e37f741b 100644
> --- a/drivers/gpu/drm/udl/udl_fb.c
> +++ b/drivers/gpu/drm/udl/udl_fb.c
> @@ -426,9 +426,11 @@ static void udl_fbdev_destroy(struct drm_device *dev,
>  {
>  	drm_fb_helper_unregister_fbi(&ufbdev->helper);
>  	drm_fb_helper_fini(&ufbdev->helper);
> -	drm_framebuffer_unregister_private(&ufbdev->ufb.base);
> -	drm_framebuffer_cleanup(&ufbdev->ufb.base);
> -	drm_gem_object_put_unlocked(&ufbdev->ufb.obj->base);
> +	if (ufbdev->ufb.obj) {
> +		drm_framebuffer_unregister_private(&ufbdev->ufb.base);
> +		drm_framebuffer_cleanup(&ufbdev->ufb.base);
> +		drm_gem_object_put_unlocked(&ufbdev->ufb.obj->base);
> +	}
>  }
>  
>  int udl_fbdev_init(struct drm_device *dev)
> -- 
> 2.17.0.921.gf22659ad46-goog
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] drm: udl: Destroy framebuffer only if it was initialized
  2018-05-28 14:27 ` [PATCH v2] " Emil Lundmark
  2018-05-29  6:46   ` Daniel Vetter
@ 2018-09-10 14:23   ` Sean Paul
  1 sibling, 0 replies; 8+ messages in thread
From: Sean Paul @ 2018-09-10 14:23 UTC (permalink / raw)
  To: lndmrk
  Cc: dri-devel, Dave Airlie, Linux Kernel Mailing List, Daniel Vetter,
	Stéphane Marchesin

On Mon, May 28, 2018 at 10:27 AM Emil Lundmark <lndmrk@chromium.org> wrote:
>
> This fixes a NULL pointer dereference that can happen if the UDL
> driver is unloaded before the framebuffer is initialized. This can
> happen e.g. if the USB device is unplugged right after it was plugged
> in.
>
> As explained by Stéphane Marchesin:
>
> It happens when fbdev is disabled (which is the case for Chrome OS).
> Even though intialization of the fbdev part is optional (it's done in
> udlfb_create which is the callback for fb_probe()), the teardown isn't
> optional (udl_driver_unload -> udl_fbdev_cleanup ->
> udl_fbdev_destroy).
>
> Note that udl_fbdev_cleanup *tries* to be conditional (you can see it
> does if (!udl->fbdev)) but that doesn't work, because udl->fbdev is
> always set during udl_fbdev_init.
>
> Suggested-by: Sean Paul <seanpaul@chromium.org>
> Signed-off-by: Emil Lundmark <lndmrk@chromium.org>

Many apologies, Emil, I completely dropped the ball on this one. I've
just applied it to -misc-fixes.

Sean


> ---
> Changes in v2:
> - Updated commit message with explanation from Stéphane Marchesin
>
>  drivers/gpu/drm/udl/udl_fb.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
> index 2ebdc6d5a76e..5754e37f741b 100644
> --- a/drivers/gpu/drm/udl/udl_fb.c
> +++ b/drivers/gpu/drm/udl/udl_fb.c
> @@ -426,9 +426,11 @@ static void udl_fbdev_destroy(struct drm_device *dev,
>  {
>         drm_fb_helper_unregister_fbi(&ufbdev->helper);
>         drm_fb_helper_fini(&ufbdev->helper);
> -       drm_framebuffer_unregister_private(&ufbdev->ufb.base);
> -       drm_framebuffer_cleanup(&ufbdev->ufb.base);
> -       drm_gem_object_put_unlocked(&ufbdev->ufb.obj->base);
> +       if (ufbdev->ufb.obj) {
> +               drm_framebuffer_unregister_private(&ufbdev->ufb.base);
> +               drm_framebuffer_cleanup(&ufbdev->ufb.base);
> +               drm_gem_object_put_unlocked(&ufbdev->ufb.obj->base);
> +       }
>  }
>
>  int udl_fbdev_init(struct drm_device *dev)
> --
> 2.17.0.921.gf22659ad46-goog
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2018-09-10 14:24 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-20 11:50 [PATCH] drm: udl: Destroy framebuffer only if it was initialized Emil Lundmark
2018-04-20 13:55 ` Sean Paul
2018-04-24 13:04   ` Daniel Vetter
2018-04-25 22:56     ` Stéphane Marchesin
2018-04-26  9:50       ` Daniel Vetter
2018-05-28 14:27 ` [PATCH v2] " Emil Lundmark
2018-05-29  6:46   ` Daniel Vetter
2018-09-10 14:23   ` Sean Paul

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).