Re: [PATCH v3 2/4] scsi: scsi_debug: Do not sleep in atomic sections

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 07/03/2024 20:30, Bart Van Assche wrote:
stop_qc_helper() is called while a spinlock is held. cancel_work_sync()
may sleep. Sleeping in atomic sections is not allowed. Hence change the
cancel_work_sync() call into a cancel_work() call.

Cc: Douglas Gilbert <dgilbert@xxxxxxxxxxxx>
Cc: John Garry <john.g.garry@xxxxxxxxxx>
Fixes: 1107c7b24ee3 ("scsi: scsi_debug: Dynamically allocate sdebug_queued_cmd")
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Bart Van Assche <bvanassche@xxxxxxx>
---
  drivers/scsi/scsi_debug.c | 9 +++------
  1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 36368c71221b..7a0b7402b715 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -5648,7 +5648,7 @@ static void scsi_debug_slave_destroy(struct scsi_device *sdp)
  	sdp->hostdata = NULL;
  }
-/* Returns true if we require the queued memory to be freed by the caller. */
+/* Returns true if the queued command memory should be freed by the caller. */
  static bool stop_qc_helper(struct sdebug_defer *sd_dp,
  			   enum sdeb_defer_type defer_t)
  {
@@ -5664,11 +5664,8 @@ static bool stop_qc_helper(struct sdebug_defer *sd_dp,
  			return true;
  		}
  	} else if (defer_t == SDEB_DEFER_WQ) {
-		/* Cancel if pending */
-		if (cancel_work_sync(&sd_dp->ew.work))
-			return true;
-		/* Was not pending, so it must have run */
-		return false;
+		/* The caller must free qcmd if cancellation succeeds. */


We were relying on the work CB not running or runnable when we return from this function, and that is why there is cancel_work_sync() [which is obviously bad under a spinlock]

Otherwise, sdebug_q_cmd_wq_complete() -> sdebug_q_cmd_wq_complete() may be running and reference the scsi_cmnd - that should not be done, because...

Checking the comment on scsi_try_to_abort_cmd(), it reads:
" SUCCESS ... indicates that the LLDDs has cleared all references to that command"

So, if we change to cancel_work(), we really should ensure scsi_debug_abort() -> scsi_debug_abort_cmnd() returns FALSE/FAILED to upper layer for when cancel_work() returns false. Effectively the (!sqcp) check in scsi_debug_stop_cmnd() checks for already run or not even queued. However, we still need to consider when the WQ callback is running.

Cheers,
John





[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux