Re: [PATCH v9 01/29] scsi: core: Clear cmd->flags for terminal queuing error

[John Garry <john.garry@linux.dev>]

On 11/9/26 20:49, Bart Van Assche wrote:
> On 11/8/26 12:47 PM, John Garry wrote:
>  > If a request can't be queued in scsi_queue_rq(), then we error out
>  > with a blk_status_t code. Such a scenario may be the scsi_device has
>  > its transport offline. For BLK_STS_RESOURCE or BLK_STS_AGAIN, the
>  > request will be requeued in the block layer.
> 
> This is not correct for BLK_STS_AGAIN. Only BLK_STS_RESOURCE and
> BLK_STS_DEV_RESOURCE cause the block layer to requeue a request. For
> all other status codes, including BLK_STS_AGAIN,
> blk_mq_dispatch_rq_list() and blk_mq_try_issue_directly() call
> blk_mq_end_request().

ok, right

> 
>  > For other errors, the block layer ends the request and so we won't
>  > see it again (until recycled). Normally when a request completes
>  > scmd->flags is cleared in scsi_end_request(). However for the error
>  > scenario mentioned above, scsi_end_request() is not called and so
>  > scmd->flags is not cleared. As such, when the request is recycled we
>  > see the old value in scmd->flags.
> 
> Please describe the consequences of this bug in the patch description.
> As far as I can tell there are at least two:
> - SCMD_FAIL_IF_RECOVERING is part of SCMD_PRESERVED_FLAGS and
>    scsi_execute_cmd() ORs new flags into scmd->flags. If a command
>    submitted with SCMD_FAIL_IF_RECOVERING is failed with BLK_STS_OFFLINE
>    because the host is in recovery, the flag leaks into the next request
>    that is allocated with the same tag. That request will then be failed
>    instead of requeued if it is submitted while the SCSI error handler
>    is active.
> - If SCMD_INITIALIZED leaks, scsi_init_command() skips
>    scsi_initialize_rq() for the next filesystem request. Hence,
>    jiffies_at_alloc and retries are not reinitialized and that request
>    may be failed prematurely by scsi_cmd_runtime_exceeded().
 > >  > Fix this issue by clearing scmd->flags in the error scenario 
mentioned
>  > by calling scsi_cleanup_rq().
> 
> Please add Fixes: and Cc: stable tags. For the SCMD_FAIL_IF_RECOVERING
> leak, I think this should be:
> 
> Fixes: 310bcaef6d7e ("scsi: core: Support failing requests while 
> recovering")

I am more inclined to go back to some commit pre-LTS v5.15, which is the 
oldest LTS maintained today. Even if you check 64104f703212f, I'd say 
that the issue existed then.

> 
>  > @@ -1994,8 +1994,8 @@ static blk_status_t scsi_queue_rq(struct 
> blk_mq_hw_ctx *hctx,
>  >           * we hit an error, as we will never see this command
>  >           * again.
>  >           */
>  > -        if (req->rq_flags & RQF_DONTPREP)
>  > -            scsi_mq_uninit_cmd(cmd);
>  > +        scsi_cleanup_rq(req);
>  > +        scsi_run_queue_async(sdev);
> 
> Since the block layer also ends requests for which scsi_queue_rq()
> returns BLK_STS_AGAIN, the BLK_STS_AGAIN case has the same issue.
> Please convert that case too:
> 
>      case BLK_STS_AGAIN:
>          cmd->result = DID_BUS_BUSY << 16;
>          scsi_cleanup_rq(req);
>          break;
> 

ok, fine. BTW, does anyone ever check cmd->result in this case, or are 
we just exercising good practice to set the result to failed?

> Although no SCSI driver returns BLK_STS_AGAIN today, I think both
> error paths should handle this in the same way.
> 
Hi Martin, I would really rather not post yet another version of this 
series. How about send the extra change which Bart requested separately?

Thanks,
John