On Thu, May 27, 2021 at 06:50:57PM +0100, John Garry wrote:
> On 26/05/2021 09:10, Ming Lei wrote:
> > When scsi_add_host_with_dma() return failure, the caller will call
> > scsi_host_put(shost) to release everything allocated for this host
> > instance. So we can't free allocated stuff in scsi_add_host_with_dma(),
> > otherwise double free will be caused.
> >
> > Strictly speaking, these host resources allocation should have been
> > moved to scsi_host_alloc(), but the allocation may need driver's
> > info which can be built between calling scsi_host_alloc() and
> > scsi_add_host(), so just keep the allocations in
> > scsi_add_host_with_dma().
> >
>
> Hi Ming,
>
> I did an experiment by making scsi_add_host_with_dma() fail by hacking the
> code, like:
>
> snprintf(shost->work_q_name, sizeof(shost->work_q_name),
> "scsi_wq_%d", shost->host_no);
> #if 0
> shost->work_q = alloc_workqueue("%s",
> WQ_SYSFS | __WQ_LEGACY | WQ_MEM_RECLAIM |
> WQ_UNBOUND,
> 1, shost->work_q_name);
> #endif
>
> I was finding that the shost gendev kobj kref count was 2 at the "fail"
> label - I would expect 1.
>
> Did you actually ever see the release function - scsi_host_dev_release() -
> being called and causing the double free?
There is one new leak issue in the failure path and the following patch
should address it:
diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index ea50856cb203..47b4ba16b017 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -296,6 +296,7 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
*/
out_del_dev:
device_del(&shost->shost_dev);
+ put_device(&shost->shost_gendev);
out_del_gendev:
device_del(&shost->shost_gendev);
out_disable_runtime_pm:
Thanks,
Ming
