| Message ID | 20210531050727.2353973-4-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: two fixes in scsi_add_host_with_dma | expand |
On 5/31/21 7:07 AM, Ming Lei wrote: > get_device(shost->shost_gendev.parent) is called in > scsi_add_host_with_dma(), but its counter pair isn't called in the failure > path, so fix it by calling put_device(shost->shost_gendev.parent) in its > failure path. > > Reported-by: John Garry <john.garry@huawei.com> > Cc: Bart Van Assche <bvanassche@acm.org> > Cc: Hannes Reinecke <hare@suse.de> > Signed-off-by: Ming Lei <ming.lei@redhat.com> > --- > drivers/scsi/hosts.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c > index 6cbc3eb16525..6cc43c51b7b3 100644 > --- a/drivers/scsi/hosts.c > +++ b/drivers/scsi/hosts.c > @@ -298,6 +298,7 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev, > out_del_dev: > device_del(&shost->shost_dev); > out_del_gendev: > + put_device(shost->shost_gendev.parent); > device_del(&shost->shost_gendev); > out_disable_runtime_pm: > device_disable_async_suspend(&shost->shost_gendev); > This really needs to be folded into the first patch as it's really a bugfix for that. Cheers, Hannes
On Mon, May 31, 2021 at 08:28:49AM +0200, Hannes Reinecke wrote: > On 5/31/21 7:07 AM, Ming Lei wrote: > > get_device(shost->shost_gendev.parent) is called in > > scsi_add_host_with_dma(), but its counter pair isn't called in the failure > > path, so fix it by calling put_device(shost->shost_gendev.parent) in its > > failure path. > > > > Reported-by: John Garry <john.garry@huawei.com> > > Cc: Bart Van Assche <bvanassche@acm.org> > > Cc: Hannes Reinecke <hare@suse.de> > > Signed-off-by: Ming Lei <ming.lei@redhat.com> > > --- > > drivers/scsi/hosts.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c > > index 6cbc3eb16525..6cc43c51b7b3 100644 > > --- a/drivers/scsi/hosts.c > > +++ b/drivers/scsi/hosts.c > > @@ -298,6 +298,7 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev, > > out_del_dev: > > device_del(&shost->shost_dev); > > out_del_gendev: > > + put_device(shost->shost_gendev.parent); > > device_del(&shost->shost_gendev); > > out_disable_runtime_pm: > > device_disable_async_suspend(&shost->shost_gendev); > > > This really needs to be folded into the first patch as it's really a bugfix > for that. All three are bug fixes, and this one may leak .shost_gendev's parent, but the 1st one leaks .shost_gendev->kobj.name, so we needn't to fold the 3rd into the 1st one. Thanks, Ming
On 5/31/21 9:56 AM, Ming Lei wrote: > On Mon, May 31, 2021 at 08:28:49AM +0200, Hannes Reinecke wrote: >> On 5/31/21 7:07 AM, Ming Lei wrote: >>> get_device(shost->shost_gendev.parent) is called in >>> scsi_add_host_with_dma(), but its counter pair isn't called in the failure >>> path, so fix it by calling put_device(shost->shost_gendev.parent) in its >>> failure path. >>> >>> Reported-by: John Garry <john.garry@huawei.com> >>> Cc: Bart Van Assche <bvanassche@acm.org> >>> Cc: Hannes Reinecke <hare@suse.de> >>> Signed-off-by: Ming Lei <ming.lei@redhat.com> >>> --- >>> drivers/scsi/hosts.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c >>> index 6cbc3eb16525..6cc43c51b7b3 100644 >>> --- a/drivers/scsi/hosts.c >>> +++ b/drivers/scsi/hosts.c >>> @@ -298,6 +298,7 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev, >>> out_del_dev: >>> device_del(&shost->shost_dev); >>> out_del_gendev: >>> + put_device(shost->shost_gendev.parent); >>> device_del(&shost->shost_gendev); >>> out_disable_runtime_pm: >>> device_disable_async_suspend(&shost->shost_gendev); >>> >> This really needs to be folded into the first patch as it's really a bugfix >> for that. > > All three are bug fixes, and this one may leak .shost_gendev's parent, > but the 1st one leaks .shost_gendev->kobj.name, so we needn't to fold > the 3rd into the 1st one. > I beg to disagree. The first patch removes the 'get_device()' from scsi_add_host_with_dma(), but does not remove the corresponding 'put_device()' in the error path. So the first patch introduces a reference imbalance which is fixed by the third patch. Hence my suggestion to merge those two patches. Cheers, Hannes
On Mon, May 31, 2021 at 10:23:43AM +0200, Hannes Reinecke wrote: > On 5/31/21 9:56 AM, Ming Lei wrote: > > On Mon, May 31, 2021 at 08:28:49AM +0200, Hannes Reinecke wrote: > > > On 5/31/21 7:07 AM, Ming Lei wrote: > > > > get_device(shost->shost_gendev.parent) is called in > > > > scsi_add_host_with_dma(), but its counter pair isn't called in the failure > > > > path, so fix it by calling put_device(shost->shost_gendev.parent) in its > > > > failure path. > > > > > > > > Reported-by: John Garry <john.garry@huawei.com> > > > > Cc: Bart Van Assche <bvanassche@acm.org> > > > > Cc: Hannes Reinecke <hare@suse.de> > > > > Signed-off-by: Ming Lei <ming.lei@redhat.com> > > > > --- > > > > drivers/scsi/hosts.c | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c > > > > index 6cbc3eb16525..6cc43c51b7b3 100644 > > > > --- a/drivers/scsi/hosts.c > > > > +++ b/drivers/scsi/hosts.c > > > > @@ -298,6 +298,7 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev, > > > > out_del_dev: > > > > device_del(&shost->shost_dev); > > > > out_del_gendev: > > > > + put_device(shost->shost_gendev.parent); > > > > device_del(&shost->shost_gendev); > > > > out_disable_runtime_pm: > > > > device_disable_async_suspend(&shost->shost_gendev); > > > > > > > This really needs to be folded into the first patch as it's really a bugfix > > > for that. > > > > All three are bug fixes, and this one may leak .shost_gendev's parent, > > but the 1st one leaks .shost_gendev->kobj.name, so we needn't to fold > > the 3rd into the 1st one. > > > I beg to disagree. > The first patch removes the 'get_device()' from > scsi_add_host_with_dma(), but does not remove the corresponding > 'put_device()' in the error path. There isn't such 'put_device' in the error path of scsi_add_host_with_dma(), is there? > So the first patch introduces a reference imbalance which is fixed by the > third patch. Hence my suggestion to merge those two patches. No, that isn't true, please look at current code of scsi_add_host_with_dma(). Thanks, Ming
diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c index 6cbc3eb16525..6cc43c51b7b3 100644 --- a/drivers/scsi/hosts.c +++ b/drivers/scsi/hosts.c @@ -298,6 +298,7 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev, out_del_dev: device_del(&shost->shost_dev); out_del_gendev: + put_device(shost->shost_gendev.parent); device_del(&shost->shost_gendev); out_disable_runtime_pm: device_disable_async_suspend(&shost->shost_gendev);
get_device(shost->shost_gendev.parent) is called in scsi_add_host_with_dma(), but its counter pair isn't called in the failure path, so fix it by calling put_device(shost->shost_gendev.parent) in its failure path. Reported-by: John Garry <john.garry@huawei.com> Cc: Bart Van Assche <bvanassche@acm.org> Cc: Hannes Reinecke <hare@suse.de> Signed-off-by: Ming Lei <ming.lei@redhat.com> --- drivers/scsi/hosts.c | 1 + 1 file changed, 1 insertion(+)