diff mbox series

scsi: hosts: check result of scsi_host_set_state() in scsi_add_host_with_dma()

Message ID 812c97ce-9b41-400a-9eb9-ddeef0156bbe@omp.ru
State New
Headers show
Series scsi: hosts: check result of scsi_host_set_state() in scsi_add_host_with_dma() | expand

Commit Message

Sergey Shtylyov Sept. 8, 2023, 8:48 p.m. UTC
SCSI core uses scsi_host_set_state() to control the host's state machine;
this function returns 0 on success and -EINVAL on failure to change host's
state. The only place where the result of scsi_host_set_state() is ignored
is in scsi_add_host_with_dma() -- that blithely continues initializing the
SCSI host even if the host's state couldn't be set to SHOST_RUNNING...
I guess the logic behind this is that scsi_add_host_with_dma() call is
always preceded by scsi_host_alloc() call which leaves the host's state
machine in the SHOST_CREATED state which is a valid previous state for
SHOST_RUNNING. I think we'd better check result of scsi_host_set_state()
always -- better safe than sorry!

Found by Linux Verification Center (linuxtesting.org) with the Svace static
analysis tool.

Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>

---
The patch is against the 'for-next' branch of Martin Petersen's 'scsi.git' repo.

 drivers/scsi/hosts.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Bart Van Assche Sept. 18, 2023, 3:55 p.m. UTC | #1
On 9/8/23 13:48, Sergey Shtylyov wrote:
> --- scsi.orig/drivers/scsi/hosts.c
> +++ scsi/drivers/scsi/hosts.c
> @@ -272,7 +272,10 @@ int scsi_add_host_with_dma(struct Scsi_H
>   	if (error)
>   		goto out_disable_runtime_pm;
>   
> -	scsi_host_set_state(shost, SHOST_RUNNING);
> +	error = scsi_host_set_state(shost, SHOST_RUNNING);
> +	if (error)
> +		goto out_disable_runtime_pm;
> +
>   	get_device(shost->shost_gendev.parent);
>   
>   	device_enable_async_suspend(&shost->shost_dev);

The scsi_host_set_state(shost, SHOST_RUNNING) call shouldn't fail. 
Hence, I think that adding error handling for the failure case will 
confuse anyone who is reading that code.

Thanks,

Bart.
diff mbox series

Patch

Index: scsi/drivers/scsi/hosts.c
===================================================================
--- scsi.orig/drivers/scsi/hosts.c
+++ scsi/drivers/scsi/hosts.c
@@ -272,7 +272,10 @@  int scsi_add_host_with_dma(struct Scsi_H
 	if (error)
 		goto out_disable_runtime_pm;
 
-	scsi_host_set_state(shost, SHOST_RUNNING);
+	error = scsi_host_set_state(shost, SHOST_RUNNING);
+	if (error)
+		goto out_disable_runtime_pm;
+
 	get_device(shost->shost_gendev.parent);
 
 	device_enable_async_suspend(&shost->shost_dev);