[PATCH] scsi: core: Don't free dev_name() manually

[PATCH] scsi: core: Don't free dev_name() manually

[Tzung-Bi Shih]

scsi_host_alloc() is designed to hold initial reference count of
`&shost->shost_gendev` and `&shost->shost_dev`.  In the error handling
paths [1], only drop a reference count to `&shost->shost_gendev` is
sufficient as scsi_host_dev_release() will be called and the reference
count of `&shost->shost_dev` should be dropped at that time.

Drivers shouldn't need to free the device name and hold a reference
count to its parent device as the driver core automatically handles
that.  Remove them.

[1] Either at "fail" label in scsi_host_alloc() or in SCSI drivers that
    a subsequent scsi_add_host{,_with_dma}() fails.

Fixes: b49493f99690 ("Fix a memory leak in scsi_host_dev_release()")
Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
 drivers/scsi/hosts.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 1b3fbd328277..b88d553cdde6 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -55,7 +55,6 @@ static DEFINE_IDA(host_index_ida);
 
 static void scsi_host_cls_release(struct device *dev)
 {
-	put_device(&class_to_shost(dev)->shost_gendev);
 }
 
 static struct class shost_class = {
@@ -279,11 +278,9 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
 		goto out_disable_runtime_pm;
 
 	scsi_host_set_state(shost, SHOST_RUNNING);
-	get_device(shost->shost_gendev.parent);
 
 	device_enable_async_suspend(&shost->shost_dev);
 
-	get_device(&shost->shost_gendev);
 	error = device_add(&shost->shost_dev);
 	if (error)
 		goto out_del_gendev;
@@ -352,7 +349,6 @@ EXPORT_SYMBOL(scsi_add_host_with_dma);
 static void scsi_host_dev_release(struct device *dev)
 {
 	struct Scsi_Host *shost = dev_to_shost(dev);
-	struct device *parent = dev->parent;
 
 	/* Wait for functions invoked through call_rcu(&scmd->rcu, ...) */
 	rcu_barrier();
@@ -366,22 +362,20 @@ static void scsi_host_dev_release(struct device *dev)
 
 	if (shost->shost_state == SHOST_CREATED) {
 		/*
-		 * Free the shost_dev device name and remove the proc host dir
+		 * Drop the reference to shost_dev and remove the proc host dir
 		 * here if scsi_host_{alloc,put}() have been called but neither
-		 * scsi_host_add() nor scsi_remove_host() has been called.
+		 * scsi_add_host() nor scsi_remove_host() has been called.
 		 * This avoids that the memory allocated for the shost_dev
 		 * name as well as the proc dir structure are leaked.
 		 */
 		scsi_proc_hostdir_rm(shost->hostt);
-		kfree(dev_name(&shost->shost_dev));
+		put_device(&shost->shost_dev);
 	}
 
 	kfree(shost->shost_data);
 
 	ida_free(&host_index_ida, shost->host_no);
 
-	if (shost->shost_state != SHOST_CREATED)
-		put_device(parent);
 	kfree(shost);
 }
 
@@ -550,8 +544,8 @@ struct Scsi_Host *scsi_host_alloc(const struct scsi_host_template *sht, int priv
  fail:
 	/*
 	 * Host state is still SHOST_CREATED and that is enough to release
-	 * ->shost_gendev. scsi_host_dev_release() will free
-	 * dev_name(&shost->shost_dev).
+	 * ->shost_gendev. scsi_host_dev_release() will
+	 * put_device(&shost->shost_dev).
 	 */
 	put_device(&shost->shost_gendev);
 
-- 
2.48.1

Re: [PATCH] scsi: core: Don't free dev_name() manually

[James Bottomley]

On Sun, 2026-01-18 at 03:32 +0800, Tzung-Bi Shih wrote:
> > scsi_host_alloc() is designed to hold initial reference count of
> > `&shost->shost_gendev` and `&shost->shost_dev`.  In the error
> > handling paths [1], only drop a reference count to `&shost-
> > >shost_gendev` is sufficient as scsi_host_dev_release() will be
> > called and the reference count of `&shost->shost_dev` should be
> > dropped at that time.
> > 
> > Drivers shouldn't need to free the device name and hold a reference
> > count to its parent device as the driver core automatically handles
> > that.  Remove them.
> > 
> > [1] Either at "fail" label in scsi_host_alloc() or in SCSI drivers
> > that
> >     a subsequent scsi_add_host{,_with_dma}() fails.

This commit description seems to bear almost no relation to what's
going on in the commit ... please describe why you're doing what you're
doing (like eliminating the class based device get and the parent get
and, apparently, trying to flatten the device tree in the host).

> > 
> > Fixes: b49493f99690 ("Fix a memory leak in
> > scsi_host_dev_release()")
> > Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
> > ---
> >  drivers/scsi/hosts.c | 16 +++++-----------
> >  1 file changed, 5 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> > index 1b3fbd328277..b88d553cdde6 100644
> > --- a/drivers/scsi/hosts.c
> > +++ b/drivers/scsi/hosts.c
> > @@ -55,7 +55,6 @@ static DEFINE_IDA(host_index_ida);
> >  
> >  static void scsi_host_cls_release(struct device *dev)
> >  {
> > - put_device(&class_to_shost(dev)->shost_gendev);
> >  }

An empty release function can simply become a NULL pointer.  I assume
the reason this one doesn't is because the device core will complain if
a device has no release function ... in which case a comment why we
don't need one should be here.

But there's a reason for the warning: a bigger problem with this is the
parenting goes

shost_dev -> shost_gendev -> underlying device

And shost_dev is visible in the sysfs tree so it could possibly by held
in place by user space.  If that happens, since you've now removed the
reference it took on shost_gendev, what stops shost_gendev (and the
rest of the host) being freed?

> >  
> >  static struct class shost_class = {
> > @@ -279,11 +278,9 @@ int scsi_add_host_with_dma(struct Scsi_Host
> > *shost, struct device *dev,
> >   goto out_disable_runtime_pm;
> >  
> >   scsi_host_set_state(shost, SHOST_RUNNING);
> > - get_device(shost->shost_gendev.parent);

We need a reference to the parent to prevent surprise removal ... where
else is the reference held?

> >  
> >   device_enable_async_suspend(&shost->shost_dev);
> >  
> > - get_device(&shost->shost_gendev);

I assume this is matched to the class dev_release which is gone?  If
so, say in the commit message.

Re: [PATCH] scsi: core: Don't free dev_name() manually

[Tzung-Bi Shih]

On Sun, Jan 18, 2026 at 09:45:26AM -0500, James Bottomley wrote:
> On Sun, 2026-01-18 at 03:32 +0800, Tzung-Bi Shih wrote:
> > > scsi_host_alloc() is designed to hold initial reference count of
> > > `&shost->shost_gendev` and `&shost->shost_dev`.  In the error
> > > handling paths [1], only drop a reference count to `&shost-
> > > >shost_gendev` is sufficient as scsi_host_dev_release() will be
> > > called and the reference count of `&shost->shost_dev` should be
> > > dropped at that time.
> > > 
> > > Drivers shouldn't need to free the device name and hold a reference
> > > count to its parent device as the driver core automatically handles
> > > that.  Remove them.
> > > 
> > > [1] Either at "fail" label in scsi_host_alloc() or in SCSI drivers
> > > that
> > >     a subsequent scsi_add_host{,_with_dma}() fails.
> 
> This commit description seems to bear almost no relation to what's
> going on in the commit ... please describe why you're doing what you're
> doing (like eliminating the class based device get and the parent get
> and, apparently, trying to flatten the device tree in the host).

An attempt to fix in v2[2].

[2] https://lore.kernel.org/all/20260119142306.33676-1-tzungbi@kernel.org/T/#u.

> 
> > > 
> > > Fixes: b49493f99690 ("Fix a memory leak in
> > > scsi_host_dev_release()")
> > > Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
> > > ---
> > >  drivers/scsi/hosts.c | 16 +++++-----------
> > >  1 file changed, 5 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> > > index 1b3fbd328277..b88d553cdde6 100644
> > > --- a/drivers/scsi/hosts.c
> > > +++ b/drivers/scsi/hosts.c
> > > @@ -55,7 +55,6 @@ static DEFINE_IDA(host_index_ida);
> > >  
> > >  static void scsi_host_cls_release(struct device *dev)
> > >  {
> > > - put_device(&class_to_shost(dev)->shost_gendev);
> > >  }
> 
> An empty release function can simply become a NULL pointer.  I assume
> the reason this one doesn't is because the device core will complain if
> a device has no release function ... in which case a comment why we
> don't need one should be here.

Fixed in v2.

> But there's a reason for the warning: a bigger problem with this is the
> parenting goes
> 
> shost_dev -> shost_gendev -> underlying device
> 
> And shost_dev is visible in the sysfs tree so it could possibly by held
> in place by user space.  If that happens, since you've now removed the
> reference it took on shost_gendev, what stops shost_gendev (and the
> rest of the host) being freed?
> 
> > >  
> > >  static struct class shost_class = {
> > > @@ -279,11 +278,9 @@ int scsi_add_host_with_dma(struct Scsi_Host
> > > *shost, struct device *dev,
> > >   goto out_disable_runtime_pm;
> > >  
> > >   scsi_host_set_state(shost, SHOST_RUNNING);
> > > - get_device(shost->shost_gendev.parent);
> 
> We need a reference to the parent to prevent surprise removal ... where
> else is the reference held?

It looks to me the same question as above.  IIUC, device_add() holds a
reference count to its parent[3].  Drivers don't need to do it explicitly.

[3] https://elixir.bootlin.com/linux/v6.18/source/drivers/base/core.c#L3612

> 
> > >  
> > >   device_enable_async_suspend(&shost->shost_dev);
> > >  
> > > - get_device(&shost->shost_gendev);
> 
> I assume this is matched to the class dev_release which is gone?  If
> so, say in the commit message.

Fixed in v2.

Re: [PATCH] scsi: core: Don't free dev_name() manually

[James Bottomley]

On Mon, 2026-01-19 at 22:28 +0800, Tzung-Bi Shih wrote:
> On Sun, Jan 18, 2026 at 09:45:26AM -0500, James Bottomley wrote:
> > On Sun, 2026-01-18 at 03:32 +0800, Tzung-Bi Shih wrote:
[...]
> > > >  
> > > >  static struct class shost_class = {
> > > > @@ -279,11 +278,9 @@ int scsi_add_host_with_dma(struct
> > > > Scsi_Host
> > > > *shost, struct device *dev,
> > > >   goto out_disable_runtime_pm;
> > > >  
> > > >   scsi_host_set_state(shost, SHOST_RUNNING);
> > > > - get_device(shost->shost_gendev.parent);
> > 
> > We need a reference to the parent to prevent surprise removal ...
> > where else is the reference held?
> 
> It looks to me the same question as above.  IIUC, device_add() holds
> a reference count to its parent[3].  Drivers don't need to do it
> explicitly.

That's not good enough for SCSI: we have a rather complicated state
model for hosts.  device_add() doesn't occur until the host moves out
of the SHOST_CREATED state, which can be quite a time after device
_initialize() so something has to pin the resources until then, which
is why these references are taken.   You're certainly free to suggest a
different way of doing this, but you can't just get rid of the existing
mechanism without replacing it with something else.

Regards,

Jaems

Re: [PATCH] scsi: core: Don't free dev_name() manually

[Tzung-Bi Shih]

On Mon, Jan 19, 2026 at 11:02:55AM -0500, James Bottomley wrote:
> On Mon, 2026-01-19 at 22:28 +0800, Tzung-Bi Shih wrote:
> > On Sun, Jan 18, 2026 at 09:45:26AM -0500, James Bottomley wrote:
> > > On Sun, 2026-01-18 at 03:32 +0800, Tzung-Bi Shih wrote:
> [...]
> > > > >  
> > > > >  static struct class shost_class = {
> > > > > @@ -279,11 +278,9 @@ int scsi_add_host_with_dma(struct
> > > > > Scsi_Host
> > > > > *shost, struct device *dev,
> > > > >   goto out_disable_runtime_pm;
> > > > >  
> > > > >   scsi_host_set_state(shost, SHOST_RUNNING);
> > > > > - get_device(shost->shost_gendev.parent);
> > > 
> > > We need a reference to the parent to prevent surprise removal ...
> > > where else is the reference held?
> > 
> > It looks to me the same question as above.  IIUC, device_add() holds
> > a reference count to its parent[3].  Drivers don't need to do it
> > explicitly.
> 
> That's not good enough for SCSI: we have a rather complicated state
> model for hosts.  device_add() doesn't occur until the host moves out
> of the SHOST_CREATED state, which can be quite a time after device
> _initialize() so something has to pin the resources until then, which
> is why these references are taken.   You're certainly free to suggest a
> different way of doing this, but you can't just get rid of the existing
> mechanism without replacing it with something else.

I may misunderstand: isn't the initial reference count from
device_initialize() held for the purpose (i.e., pin the resource)?  The
driver calls scsi_host_put() to drop the reference count when the underlying
chip is removing.

The proposed code to remove the get_device() just right before device_add()
in scsi_add_host_with_dma().  I don't see what else resources it can pin.

Re: [PATCH] scsi: core: Don't free dev_name() manually

[James Bottomley]

On Tue, 2026-01-20 at 21:11 +0800, Tzung-Bi Shih wrote:
> On Mon, Jan 19, 2026 at 11:02:55AM -0500, James Bottomley wrote:
> > On Mon, 2026-01-19 at 22:28 +0800, Tzung-Bi Shih wrote:
> > > On Sun, Jan 18, 2026 at 09:45:26AM -0500, James Bottomley wrote:
> > > > On Sun, 2026-01-18 at 03:32 +0800, Tzung-Bi Shih wrote:
> > [...]
> > > > > >  
> > > > > >  static struct class shost_class = {
> > > > > > @@ -279,11 +278,9 @@ int scsi_add_host_with_dma(struct
> > > > > > Scsi_Host
> > > > > > *shost, struct device *dev,
> > > > > >   goto out_disable_runtime_pm;
> > > > > >  
> > > > > >   scsi_host_set_state(shost, SHOST_RUNNING);
> > > > > > - get_device(shost->shost_gendev.parent);
> > > > 
> > > > We need a reference to the parent to prevent surprise removal
> > > > ...
> > > > where else is the reference held?
> > > 
> > > It looks to me the same question as above.  IIUC, device_add()
> > > holds
> > > a reference count to its parent[3].  Drivers don't need to do it
> > > explicitly.
> > 
> > That's not good enough for SCSI: we have a rather complicated state
> > model for hosts.  device_add() doesn't occur until the host moves
> > out of the SHOST_CREATED state, which can be quite a time after
> > device _initialize() so something has to pin the resources until
> > then, which is why these references are taken.   You're certainly
> > free to suggest a different way of doing this, but you can't just
> > get rid of the existing mechanism without replacing it with
> > something else.
> 
> I may misunderstand: isn't the initial reference count from
> device_initialize() held for the purpose (i.e., pin the resource)? 
> The driver calls scsi_host_put() to drop the reference count when the
> underlying chip is removing.
> 
> The proposed code to remove the get_device() just right before
> device_add() in scsi_add_host_with_dma().  I don't see what else
> resources it can pin.

And the reverse, when the host is going away and device_del gets called
but something has the sysfs node open?

Regards,

James