Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux

usb-storage: fix deadlock involving host lock and scsi_done

Christoph Hellwig says that since version 4.12, the kernel switched to
using blk-mq by default. The old code used a softirq for handling
request completions, but blk-mq can handle completions in the caller's
context. This may cause a problem for usb-storage, because it invokes
the ->scsi_done callback while holding the host lock, and the
completion routine sometimes tries to acquire the same lock (when
running the error handler, for example).

The consequence is that the existing code will sometimes deadlock upon
error completion of a SCSI command (with a lockdep warning).

This is easy enough to fix, since usb-storage doesn't really need to
hold the host lock while the callback runs. It was simpler to write
it that way, but moving the call outside the locked region is pretty
easy and there's no downside. That's what this patch does.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Reported-and-tested-by: Arthur Marsh <arthur.marsh@internode.on.net>
CC: Christoph Hellwig <hch@lst.de>
CC: <stable@vger.kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

authored by

Alan Stern and committed by
Greg Kroah-Hartman
8b52291a 89f23d51

+12 -6
+12 -6
drivers/usb/storage/usb.c
··· 315 315 { 316 316 struct us_data *us = (struct us_data *)__us; 317 317 struct Scsi_Host *host = us_to_host(us); 318 + struct scsi_cmnd *srb; 318 319 319 320 for (;;) { 320 321 usb_stor_dbg(us, "*** thread sleeping\n"); ··· 331 330 scsi_lock(host); 332 331 333 332 /* When we are called with no command pending, we're done */ 333 + srb = us->srb; 334 334 if (us->srb == NULL) { 335 335 scsi_unlock(host); 336 336 mutex_unlock(&us->dev_mutex); ··· 400 398 /* lock access to the state */ 401 399 scsi_lock(host); 402 400 403 - /* indicate that the command is done */ 404 - if (us->srb->result != DID_ABORT << 16) { 405 - usb_stor_dbg(us, "scsi cmd done, result=0x%x\n", 406 - us->srb->result); 407 - us->srb->scsi_done(us->srb); 408 - } else { 401 + /* was the command aborted? */ 402 + if (us->srb->result == DID_ABORT << 16) { 409 403 SkipForAbort: 410 404 usb_stor_dbg(us, "scsi command aborted\n"); 405 + srb = NULL; /* Don't call srb->scsi_done() */ 411 406 } 412 407 413 408 /* ··· 428 429 429 430 /* unlock the device pointers */ 430 431 mutex_unlock(&us->dev_mutex); 432 + 433 + /* now that the locks are released, notify the SCSI core */ 434 + if (srb) { 435 + usb_stor_dbg(us, "scsi cmd done, result=0x%x\n", 436 + srb->result); 437 + srb->scsi_done(srb); 438 + } 431 439 } /* for (;;) */ 432 440 433 441 /* Wait until we are told to stop */