linux-kernel - Re: [PATCH] scsi: core: Don't free dev

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aW4_fbfNUMTDTAN1@tzungbi-laptop>
Date: Mon, 19 Jan 2026 22:28:13 +0800
From: Tzung-Bi Shih <tzungbi@...nel.org>
To: James Bottomley <James.Bottomley@...senpartnership.com>
Cc: "Martin K. Petersen" <martin.petersen@...cle.com>,
	Greg KH <gregkh@...uxfoundation.org>, linux-scsi@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] scsi: core: Don't free dev_name() manually

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@...nel.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.