[PATCH] scsi: fix crash in scsi_remove_host after alloc failure

[PATCH] scsi: fix crash in scsi_remove_host after alloc failure

[Khazhismel Kumykov]

If transport_register_device returns error, shost_gendev has already
been cleaned up - however since we ignore the error device setup
continues happily. We will eventually call transport_unregister_device,
attempting to delete shost_gendev again, resulting in a crash.

It looks like when this cleanup behavior was added, iscsi was updated,
but scsi was missed.

Fixes: cd7ea70bb00a ("scsi: drivers: base: Propagate errors through the transport component")

Signed-off-by: Khazhismel Kumykov <khazhy@google.com>
---
 drivers/scsi/scsi_sysfs.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index c95177ca6ed2..722ab042fbd7 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1599,7 +1599,11 @@ EXPORT_SYMBOL(scsi_register_interface);
  **/
 int scsi_sysfs_add_host(struct Scsi_Host *shost)
 {
-	transport_register_device(&shost->shost_gendev);
+	int err;
+
+	err = transport_register_device(&shost->shost_gendev);
+	if (err)
+		return err;
 	transport_configure_device(&shost->shost_gendev);
 	return 0;
 }
-- 
2.38.0.413.g74048e4d9e-goog

Re: [PATCH] scsi: fix crash in scsi_remove_host after alloc failure

[Mike Christie]

On 10/17/22 12:11 PM, Khazhismel Kumykov wrote:
> If transport_register_device returns error, shost_gendev has already
> been cleaned up - however since we ignore the error device setup
> continues happily. We will eventually call transport_unregister_device,
> attempting to delete shost_gendev again, resulting in a crash.
> 
> It looks like when this cleanup behavior was added, iscsi was updated,
> but scsi was missed.
> 
> Fixes: cd7ea70bb00a ("scsi: drivers: base: Propagate errors through the transport component")
> 

Where do you crash? Do we need to handle the cases transport_add_device
is called directly and we don't handle the failure then later call
transport_remove_device directly?

The thing is that transport device addition success was
supposed to be optional where we were supposed to be able to
still at least setup the device, boot and use it. Tools might
be missing some attrs but we were still supposed to run.
I think that's why we didn't propagate errors originally.
We were also supposed to also be able to call transport_configure_device
even if the transport_add_device call failed (see comment in
that function for info). Does that still work?