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] [day] [month] [year] [list]

Message-ID: <de7b19fe19ccb117cad8cd32d9c51796ee81b752.camel@HansenPartnership.com>
Date: Sun, 18 Jan 2026 09:45:26 -0500
From: James Bottomley <James.Bottomley@...senPartnership.com>
To: Tzung-Bi Shih <tzungbi@...nel.org>, "Martin K. Petersen"
	 <martin.petersen@...cle.com>, Greg KH <gregkh@...uxfoundation.org>
Cc: linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] scsi: core: Don't free dev_name() manually

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

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.