[PATCH] scsi: core: Rework the DEF_SCSI_QCMD() implementation

[PATCH] scsi: core: Rework the DEF_SCSI_QCMD() implementation

[Bart Van Assche]

In DEF_SCSI_QCMD(), make the following changes:
 - Replace shost with cmd->device->host because func_name##_lck()
   implementations will be annotated with
   __must_hold(&cmd->device->host->host_lock).
 - Do not save and restore the IRQ flags. Since support for the legacy
   block layer has been removed, .queue_rq() implementations are always
   called from thread context and with interrupts enabled. Hence, saving
   and restoring the interrupt state is not necessary.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 include/scsi/scsi_host.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 16243ec376cd..c018f78ce3ce 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -531,12 +531,11 @@ struct scsi_host_template {
 	enum scsi_qc_status func_name(struct Scsi_Host *shost,		\
 				      struct scsi_cmnd *cmd)		\
 	{								\
-		unsigned long irq_flags;				\
 		enum scsi_qc_status rc;					\
 									\
-		spin_lock_irqsave(shost->host_lock, irq_flags);		\
+		spin_lock_irq(cmd->device->host->host_lock);		\
 		rc = func_name##_lck(cmd);				\
-		spin_unlock_irqrestore(shost->host_lock, irq_flags);	\
+		spin_unlock_irq(cmd->device->host->host_lock);		\
 		return rc;						\
 	}
 

Re: [PATCH] scsi: core: Rework the DEF_SCSI_QCMD() implementation

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Accessing `cmd->device` in `spin_unlock_irq` after the command may have been completed results in a Use-After-Free.
- [Low] The `shost` parameter in the macro-generated function is completely unused, which triggers compiler warnings.
--

commit 6119bc222730902c8f828977bcf9466e8f555a95
Author: Bart Van Assche <bvanassche@acm.org>

scsi: core: Rework the DEF_SCSI_QCMD() implementation

This commit modifies the DEF_SCSI_QCMD() macro to lock against
cmd->device->host->host_lock directly instead of using the provided shost.
It also removes the saving and restoring of IRQ flags since the queue
function is always called from thread context.

> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -531,12 +531,11 @@ struct scsi_host_template {
>  	enum scsi_qc_status func_name(struct Scsi_Host *shost,		\
>  				      struct scsi_cmnd *cmd)		\

[Severity: Low]
Does this leave the shost parameter completely unused within the function
body? 

It seems this could trigger compiler warnings for unused parameters when
compiling drivers utilizing this macro.

>  	{								\
> -		unsigned long irq_flags;				\
>  		enum scsi_qc_status rc;					\
>  									\
> -		spin_lock_irqsave(shost->host_lock, irq_flags);		\
> +		spin_lock_irq(cmd->device->host->host_lock);		\
>  		rc = func_name##_lck(cmd);				\
> -		spin_unlock_irqrestore(shost->host_lock, irq_flags);	\
> +		spin_unlock_irq(cmd->device->host->host_lock);		\

[Severity: Critical]
Can this cause a use-after-free if the command is completed during the
execution of func_name##_lck(cmd)?

If a driver's implementation processes the command and completes it by
calling scsi_done(cmd) before returning, the block layer can reclaim and
recycle the cmd structure immediately. 

When the wrapper then executes spin_unlock_irq(cmd->device->host->host_lock),
cmd->device may already be a dangling pointer.

Using the passed shost parameter for the unlock operation would avoid
dereferencing cmd after it may have been handed off.

>  		return rc;						\
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/61df3d2ad6e65f188469708fb05b679b49596cef.1788204517.git.bvanassche@acm.org?part=1