| Message ID | 20221017171147.3300575-1-khazhy@google.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | scsi: fix crash in scsi_remove_host after alloc failure | expand |
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?
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; }
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(-)