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)
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
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
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
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
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)
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
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
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Emil Lundmark -
Sean Paul -
Stéphane Marchesin