Re: [PATCH] mmc: vub300: defer reset until cmd_mutex is unlocked

On Wed, Jun 17, 2026 at 5:23 PM Runyu Xiao <[email protected]> wrote:
>
> vub300_cmndwork_thread() holds cmd_mutex while it sends a command and
> waits for the command response.  If the response wait times out,
> __vub300_command_response() kills the command URBs and then synchronously
> resets the USB device through usb_reset_device().
>
> That reset path re-enters the driver through vub300_pre_reset(), which
> also takes cmd_mutex.  The worker therefore tries to acquire the same
> mutex recursively while it is still holding it from the command path.
>
> This issue was found by our static analysis tool and then manually
> reviewed against the current tree.
>
> The grounded PoC kept the real worker and timeout/reset carrier:
>
>   vub300_cmndwork_thread()
>   __vub300_command_response()
>   usb_lock_device_for_reset()
>   usb_reset_device()
>   vub300_pre_reset()
>
> Lockdep reported the same-task recursive acquisition on cmd_mutex:
>
>   WARNING: possible recursive locking detected
>   ... (&test_vub300.cmd_mutex) ... at: usb_reset_device... [vuln_msv]
>   ... (&test_vub300.cmd_mutex) ... at: vub300_cmndwork_thread+0x12/0x20 [vuln_msv]
>   Workqueue: vub300_cmd_wq vub300_cmndwork_thread [vuln_msv]
>   *** DEADLOCK ***
>
> Return a flag from __vub300_command_response() when the timeout path needs
> a device reset, then perform the reset after vub300_cmndwork_thread() has
> cleared the in-flight command state and dropped cmd_mutex.  The reset is
> still attempted before mmc_request_done(), preserving the existing request
> completion ordering while avoiding the recursive lock.
>
> Fixes: 88095e7b473a ("mmc: Add new VUB300 USB-to-SD/SDIO/MMC driver")
> Cc: [email protected]
> Signed-off-by: Runyu Xiao <[email protected]>

Applied for fixes, thanks!

Kind regards
Uffe


> ---
>  drivers/mmc/host/vub300.c | 26 +++++++++++++++++---------
>  1 file changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/mmc/host/vub300.c b/drivers/mmc/host/vub300.c
> index ff49d0770506..1c335e070741 100644
> --- a/drivers/mmc/host/vub300.c
> +++ b/drivers/mmc/host/vub300.c
> @@ -1583,7 +1583,7 @@ static int __command_write_data(struct vub300_mmc_host *vub300,
>         return linear_length;
>  }
>
> -static void __vub300_command_response(struct vub300_mmc_host *vub300,
> +static bool __vub300_command_response(struct vub300_mmc_host *vub300,
>                                       struct mmc_command *cmd,
>                                       struct mmc_data *data, int data_length)
>  {
> @@ -1595,17 +1595,11 @@ static void __vub300_command_response(struct vub300_mmc_host *vub300,
>                                             msecs_to_jiffies(msec_timeout));
>         if (respretval == 0) { /* TIMED OUT */
>                 /* we don't know which of "out" and "res" if any failed */
> -               int result;
>                 vub300->usb_timed_out = 1;
>                 usb_kill_urb(vub300->command_out_urb);
>                 usb_kill_urb(vub300->command_res_urb);
>                 cmd->error = -ETIMEDOUT;
> -               result = usb_lock_device_for_reset(vub300->udev,
> -                                                  vub300->interface);
> -               if (result == 0) {
> -                       result = usb_reset_device(vub300->udev);
> -                       usb_unlock_device(vub300->udev);
> -               }
> +               return true;
>         } else if (respretval < 0) {
>                 /* we don't know which of "out" and "res" if any failed */
>                 usb_kill_urb(vub300->command_out_urb);
> @@ -1701,6 +1695,8 @@ static void __vub300_command_response(struct vub300_mmc_host *vub300,
>         } else {
>                 cmd->error = -EINVAL;
>         }
> +
> +       return false;
>  }
>
>  static void construct_request_response(struct vub300_mmc_host *vub300,
> @@ -1746,6 +1742,7 @@ static void vub300_cmndwork_thread(struct work_struct *work)
>                 struct mmc_request *req = vub300->req;
>                 struct mmc_command *cmd = vub300->cmd;
>                 struct mmc_data *data = vub300->data;
> +               bool reset_device;
>                 int data_length;
>                 mutex_lock(&vub300->cmd_mutex);
>                 init_completion(&vub300->command_complete);
> @@ -1768,7 +1765,8 @@ static void vub300_cmndwork_thread(struct work_struct *work)
>                         data_length = __command_read_data(vub300, cmd, data);
>                 else
>                         data_length = __command_write_data(vub300, cmd, data);
> -               __vub300_command_response(vub300, cmd, data, data_length);
> +               reset_device = __vub300_command_response(vub300, cmd,
> +                                                        data, data_length);
>                 vub300->req = NULL;
>                 vub300->cmd = NULL;
>                 vub300->data = NULL;
> @@ -1776,6 +1774,16 @@ static void vub300_cmndwork_thread(struct work_struct *work)
>                         if (cmd->error == -ENOMEDIUM)
>                                 check_vub300_port_status(vub300);
>                         mutex_unlock(&vub300->cmd_mutex);
> +                       if (reset_device) {
> +                               int result;
> +
> +                               result = usb_lock_device_for_reset(vub300->udev,
> +                                                                  vub300->interface);
> +                               if (result == 0) {
> +                                       result = usb_reset_device(vub300->udev);
> +                                       usb_unlock_device(vub300->udev);
> +                               }
> +                       }
>                         mmc_request_done(vub300->mmc, req);
>                         kref_put(&vub300->kref, vub300_delete);
>                         return;
> --
> 2.34.1