From patchwork Fri May 5 11:47:59 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Maxime Bizon X-Patchwork-Id: 13232622 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 13FEFC77B7C for ; Fri, 5 May 2023 11:48:33 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231648AbjEELsc (ORCPT ); Fri, 5 May 2023 07:48:32 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46876 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231616AbjEELsb (ORCPT ); Fri, 5 May 2023 07:48:31 -0400 Received: from ns.iliad.fr (ns.iliad.fr [212.27.33.1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 641FE1A130 for ; Fri, 5 May 2023 04:48:28 -0700 (PDT) Received: from ns.iliad.fr (localhost [127.0.0.1]) by ns.iliad.fr (Postfix) with ESMTP id AC6C42029C; Fri, 5 May 2023 13:48:26 +0200 (CEST) Received: from sakura.iliad.local (freebox.vlq16.iliad.fr [213.36.7.13]) by ns.iliad.fr (Postfix) with ESMTP id 9356E20173; Fri, 5 May 2023 13:48:26 +0200 (CEST) From: Maxime Bizon To: linux-usb@vger.kernel.org Cc: Greg Kroah-Hartman , Maxime Bizon , Alan Stern Subject: [PATCH] usb-storage: fix deadlock when a scsi command timeouts more than once Date: Fri, 5 May 2023 13:47:59 +0200 Message-Id: <20230505114759.1189741-1-mbizon@freebox.fr> X-Mailer: git-send-email 2.34.1 MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: linux-usb@vger.kernel.org With faulty usb-storage devices, read/write can timeout, in that case the SCSI layer will abort and re-issue the command. USB storage has no internal timeout, it relies on SCSI layer aborting commands via .eh_abort_handler() for non those responsive devices. After two consecutive timeouts of the same command, SCSI layer calls .eh_device_reset_handler(), without calling .eh_abort_handler() first. With usb-storage, this causes a deadlock: -> .eh_device_reset_handler -> device_reset -> mutex_lock(&(us->dev_mutex)); mutex already by usb_stor_control_thread(), which is waiting for command completion: -> usb_stor_control_thread (mutex taken here) -> usb_stor_invoke_transport -> usb_stor_Bulk_transport -> usb_stor_bulk_srb -> usb_stor_bulk_transfer_sglist -> usb_sg_wait Make sure we cancel any pending command in .eh_device_reset_handler() to avoid this. Signed-off-by: Maxime Bizon Cc: Alan Stern Cc: linux-usb@vger.kernel.org Link: https://lore.kernel.org/all/ZEllnjMKT8ulZbJh@sakura/ Reviewed-by: Alan Stern Acked-by: Alan Stern --- drivers/usb/storage/scsiglue.c | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c index 8931df5a85fd..c54e9805da53 100644 --- a/drivers/usb/storage/scsiglue.c +++ b/drivers/usb/storage/scsiglue.c @@ -406,22 +406,25 @@ static DEF_SCSI_QCMD(queuecommand) ***********************************************************************/ /* Command timeout and abort */ -static int command_abort(struct scsi_cmnd *srb) +static int command_abort_matching(struct us_data *us, struct scsi_cmnd *srb_match) { - struct us_data *us = host_to_us(srb->device->host); - - usb_stor_dbg(us, "%s called\n", __func__); - /* * us->srb together with the TIMED_OUT, RESETTING, and ABORTING * bits are protected by the host lock. */ scsi_lock(us_to_host(us)); - /* Is this command still active? */ - if (us->srb != srb) { + /* is there any active pending command to abort ? */ + if (!us->srb) { scsi_unlock(us_to_host(us)); usb_stor_dbg(us, "-- nothing to abort\n"); + return SUCCESS; + } + + /* Does the command match the passed srb if any ? */ + if (srb_match && us->srb != srb_match) { + scsi_unlock(us_to_host(us)); + usb_stor_dbg(us, "-- pending command mismatch\n"); return FAILED; } @@ -444,6 +447,14 @@ static int command_abort(struct scsi_cmnd *srb) return SUCCESS; } +static int command_abort(struct scsi_cmnd *srb) +{ + struct us_data *us = host_to_us(srb->device->host); + + usb_stor_dbg(us, "%s called\n", __func__); + return command_abort_matching(us, srb); +} + /* * This invokes the transport reset mechanism to reset the state of the * device @@ -455,6 +466,9 @@ static int device_reset(struct scsi_cmnd *srb) usb_stor_dbg(us, "%s called\n", __func__); + /* abort any pending command before reset */ + command_abort_matching(us, NULL); + /* lock the device pointers and do the reset */ mutex_lock(&(us->dev_mutex)); result = us->transport_reset(us);