| Message ID | 20240307203015.870254-3-bvanassche@acm.org |
|---|---|
| State | New |
| Headers | show |
| Series | scsi_debug improvements | expand |
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@interlog.com> > Cc: John Garry <john.g.garry@oracle.com> > Fixes: 1107c7b24ee3 ("scsi: scsi_debug: Dynamically allocate sdebug_queued_cmd") > Cc: stable@vger.kernel.org > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > 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
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. */ + return cancel_work(&sd_dp->ew.work); } else if (defer_t == SDEB_DEFER_POLL) { return true; }
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@interlog.com> Cc: John Garry <john.g.garry@oracle.com> Fixes: 1107c7b24ee3 ("scsi: scsi_debug: Dynamically allocate sdebug_queued_cmd") Cc: stable@vger.kernel.org Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/scsi/scsi_debug.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)