[PATCH] drm/gm12u320: fix system freeze on surprise unplug

[PATCH] drm/gm12u320: fix system freeze on surprise unplug

[Forest Crossman]

Unplugging (or deauthorizing) a GM12U320 device while it is driving an
output causes the system to freeze. When this happens, other screens
stop updating and USB input (keyboard and mouse) stops working, but
non-USB storage, audio, network and ACPI events are unaffected. Because
the unplug never completes, the only way out is a reboot.

The kernel log shows a NULL pointer dereference on every such unplug:

  BUG: kernel NULL pointer dereference, address: 0000000000000030
  RIP: 0010:drm_mode_object_put+0x9/0x20
  Call Trace:
   drm_atomic_helper_commit_crtc_disable
   drm_atomic_helper_commit_tail
   commit_tail
   drm_atomic_helper_commit
   drm_atomic_commit
   drm_atomic_helper_disable_all
   drm_atomic_helper_shutdown
   gm12u320_usb_disconnect
   usb_unbind_interface
   ...
   usb_set_configuration
   usb_deauthorize_device
   authorized_store

The call trace above was caused by triggering a deauthorization, but a
physical unplug reaches the same gm12u320_usb_disconnect() path through
hub_event/usb_disconnect().

The task faults while holding the USB device mutex
(usb_set_configuration()) and the DRM modeset locks taken by
drm_atomic_helper_shutdown(). Mutexes are not released when a task dies
in an oops, so they stay locked forever, with the USB hub workqueue
(hub_event) and the DRM framebuffer-removal worker both blocking on
them. This is what freezes the system when the driver crashes.

gm12u320_stop_fb_update() drops the cached upload framebuffer
unconditionally:

  old_fb = gm12u320->fb_update.fb;
  gm12u320->fb_update.fb = NULL;
  ...
  drm_framebuffer_put(old_fb);

gm12u320->fb_update.fb is legitimately NULL when no update is queued,
for example when the update was already stopped, and
drm_framebuffer_put() does not accept NULL. On unplug,
gm12u320_usb_disconnect() calls drm_atomic_helper_shutdown(), which
commits a CRTC disable and reaches gm12u320_stop_fb_update() with a NULL
fb.

The NULL check was lost in commit 8f2cb9379fb4 ("drm/gm12u320: Simplify
upload work"). That commit moved the reference release out of
fb_update.lock (so the framebuffer destructor does not run under the
lock), but the previous form guarded the call with "if
(gm12u320->fb_update.fb)" and that check was dropped in the rewrite.
gm12u320_fb_mark_dirty() still has the same guard.

To fix the crash, restore the NULL check before calling
drm_framebuffer_put(). With this, the CRTC disable completes, no locks
are leaked, and unplugging the GM12U320 device while it is in use no
longer freezes the system. Tested by surprise-unplugging the device as
the active output--before the change the kernel oopsed and wedged on
every attempt, after it there is no oops and no stuck task.

Fixes: 8f2cb9379fb4 ("drm/gm12u320: Simplify upload work")
Cc: stable@vger.kernel.org
Signed-off-by: Forest Crossman <cyrozap@gmail.com>
---
 drivers/gpu/drm/tiny/gm12u320.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tiny/gm12u320.c b/drivers/gpu/drm/tiny/gm12u320.c
index 4ad074337af0..5bb6488556de 100644
--- a/drivers/gpu/drm/tiny/gm12u320.c
+++ b/drivers/gpu/drm/tiny/gm12u320.c
@@ -446,7 +446,8 @@ static void gm12u320_stop_fb_update(struct gm12u320_device *gm12u320)
 	iosys_map_clear(&gm12u320->fb_update.src_map);
 	mutex_unlock(&gm12u320->fb_update.lock);
 
-	drm_framebuffer_put(old_fb);
+	if (old_fb)
+		drm_framebuffer_put(old_fb);
 }
 
 static int gm12u320_set_ecomode(struct gm12u320_device *gm12u320)

base-commit: 1717fcc5be575d4768279148ae9465a8b13d4339
-- 
2.55.0