Skip to content
  • Alan Stern's avatar
    USB: g_mass_storage: Fix deadlock when driver is unbound · 1fbbb78f
    Alan Stern authored
    
    
    As a holdover from the old g_file_storage gadget, the g_mass_storage
    legacy gadget driver attempts to unregister itself when its main
    operating thread terminates (if it hasn't been unregistered already).
    This is not strictly necessary; it was never more than an attempt to
    have the gadget fail cleanly if something went wrong and the main
    thread was killed.
    
    However, now that the UDC core manages gadget drivers independently of
    UDC drivers, this scheme doesn't work any more.  A simple test:
    
    	modprobe dummy-hcd
    	modprobe g-mass-storage file=...
    	rmmod dummy-hcd
    
    ends up in a deadlock with the following backtrace:
    
     sysrq: SysRq : Show Blocked State
       task                PC stack   pid father
     file-storage    D    0  1130      2 0x00000000
     Call Trace:
      __schedule+0x53e/0x58c
      schedule+0x6e/0x77
      schedule_preempt_disabled+0xd/0xf
      __mutex_lock.isra.1+0x129/0x224
      ? _raw_spin_unlock_irqrestore+0x12/0x14
      __mutex_lock_slowpath+0x12/0x14
      mutex_lock+0x28/0x2b
      usb_gadget_unregister_driver+0x29/0x9b [udc_core]
      usb_composite_unregister+0x10/0x12 [libcomposite]
      msg_cleanup+0x1d/0x20 [g_mass_storage]
      msg_thread_exits+0xd/0xdd7 [g_mass_storage]
      fsg_main_thread+0x1395/0x13d6 [usb_f_mass_storage]
      ? __schedule+0x573/0x58c
      kthread+0xd9/0xdb
      ? do_set_interface+0x25c/0x25c [usb_f_mass_storage]
      ? init_completion+0x1e/0x1e
      ret_from_fork+0x19/0x24
     rmmod           D    0  1155    683 0x00000000
     Call Trace:
      __schedule+0x53e/0x58c
      schedule+0x6e/0x77
      schedule_timeout+0x26/0xbc
      ? __schedule+0x573/0x58c
      do_wait_for_common+0xb3/0x128
      ? usleep_range+0x81/0x81
      ? wake_up_q+0x3f/0x3f
      wait_for_common+0x2e/0x45
      wait_for_completion+0x17/0x19
      fsg_common_put+0x34/0x81 [usb_f_mass_storage]
      fsg_free_inst+0x13/0x1e [usb_f_mass_storage]
      usb_put_function_instance+0x1a/0x25 [libcomposite]
      msg_unbind+0x2a/0x42 [g_mass_storage]
      __composite_unbind+0x4a/0x6f [libcomposite]
      composite_unbind+0x12/0x14 [libcomposite]
      usb_gadget_remove_driver+0x4f/0x77 [udc_core]
      usb_del_gadget_udc+0x52/0xcc [udc_core]
      dummy_udc_remove+0x27/0x2c [dummy_hcd]
      platform_drv_remove+0x1d/0x31
      device_release_driver_internal+0xe9/0x16d
      device_release_driver+0x11/0x13
      bus_remove_device+0xd2/0xe2
      device_del+0x19f/0x221
      ? selinux_capable+0x22/0x27
      platform_device_del+0x21/0x63
      platform_device_unregister+0x10/0x1a
      cleanup+0x20/0x817 [dummy_hcd]
      SyS_delete_module+0x10c/0x197
      ? ____fput+0xd/0xf
      ? task_work_run+0x55/0x62
      ? prepare_exit_to_usermode+0x65/0x75
      do_fast_syscall_32+0x86/0xc3
      entry_SYSENTER_32+0x4e/0x7c
    
    What happens is that removing the dummy-hcd driver causes the UDC core
    to unbind the gadget driver, which it does while holding the udc_lock
    mutex.  The unbind routine in g_mass_storage tells the main thread to
    exit and waits for it to terminate.
    
    But as mentioned above, when the main thread exits it tries to
    unregister the mass-storage function driver.  Via the composite
    framework this ends up calling usb_gadget_unregister_driver(), which
    tries to acquire the udc_lock mutex.  The result is deadlock.
    
    The simplest way to fix the problem is not to be so clever: The main
    thread doesn't have to unregister the function driver.  The side
    effects won't be so terrible; if the gadget is still attached to a USB
    host when the main thread is killed, it will appear to the host as
    though the gadget's firmware has crashed -- a reasonably accurate
    interpretation, and an all-too-common occurrence for USB mass-storage
    devices.
    
    In fact, the code to unregister the driver when the main thread exits
    is specific to g-mass-storage; it is not used when f-mass-storage is
    included as a function in a larger composite device.  Therefore the
    entire mechanism responsible for this (the fsg_operations structure
    with its ->thread_exits method, the fsg_common_set_ops() routine, and
    the msg_thread_exits() callback routine) can all be eliminated.  Even
    the msg_registered bitflag can be removed, because now the driver is
    unregistered in only one place rather than in two places.
    
    Signed-off-by: default avatarAlan Stern <stern@rowland.harvard.edu>
    CC: <stable@vger.kernel.org>
    Acked-by: default avatarFelipe Balbi <felipe.balbi@linux.intel.com>
    Acked-by: default avatarMichal Nazarewicz <mina86@mina86.com>
    Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
    1fbbb78f