linux-kernel - Re: [PATCH] driver core: Make it safe to use get

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: <20171214081012.GA16072@kroah.com>
Date:   Thu, 14 Dec 2017 09:10:12 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Jason Yan <yanaijie@...wei.com>
Cc:     martin.petersen@...cle.com, jejb@...ux.vnet.ibm.com,
        linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org,
        Bart Van Assche <bart.vanassche@....com>,
        "Ewan D . Milne" <emilne@...hat.com>,
        Christoph Hellwig <hch@....de>
Subject: Re: [PATCH] driver core: Make it safe to use get_device() if the
 reference count is zero

On Thu, Dec 14, 2017 at 03:56:46PM +0800, Jason Yan wrote:
> 
> On 2017/12/14 15:42, Greg KH wrote:
> > On Thu, Dec 14, 2017 at 11:39:36AM +0800, Jason Yan wrote:
> > > Some driviers may have the chance to increase a reference count that
> > > has dropped to zero when using get_device() because of their design.
> > Then those drivers are broken :)
> > 
> > > We have met such a issue with scsi:
> > > https://www.spinics.net/lists/linux-scsi/msg115295.html
> > > 
> > > The scsi core will keep the scsi device object in the host list after
> > > it has been deleted and the iterator can still find it. All of the
> > > places where need iterating have to check the state of the scsi device
> > > and this makes a lot of code redundancy and complexity.
> > > 
> > > Provide a safe mechanism in get_device() by using
> > > kobject_get_unless_zero().
> > > 
> > > Suggested-by: Bart Van Assche <bart.vanassche@....com>
> > > Signed-off-by: Jason Yan <yanaijie@...wei.com>
> > > CC: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> > > CC: Bart Van Assche <bart.vanassche@....com>
> > > CC: Ewan D. Milne <emilne@...hat.com>
> > > CC: James E.J. Bottomley <jejb@...ux.vnet.ibm.com>
> > > CC: Christoph Hellwig <hch@....de>
> > > ---
> > >   drivers/base/core.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > index 12ebd05..cc74810 100644
> > > --- a/drivers/base/core.c
> > > +++ b/drivers/base/core.c
> > > @@ -1916,7 +1916,7 @@ EXPORT_SYMBOL_GPL(device_register);
> > >    */
> > >   struct device *get_device(struct device *dev)
> > >   {
> > > -	return dev ? kobj_to_dev(kobject_get(&dev->kobj)) : NULL;
> > > +	return dev && kobject_get_unless_zero(&dev->kobj) ? dev : NULL;
> > I really don't want to do this, the bus the device is on should prevent
> > this from happening.
> > 
> > Also, once that reference count drops to zero, the memory should be
> > freed, so you really have a stale pointer here, and this code would fail
> > if you had slab debugging enabled anyway.
> 
> Actually I don't want this either. But the design of scsi core will leave
> the scsi device on the host list after it is deleted, and it can  be
> found later and the refcount have a very big chance to increase from
> zero again. And after a lot of discussion it seems that the scsi layer
> is difficult to change the situation in the near future.

Keeping a 'struct device' reference counted chunk of memory on a list
that has a different lifetime rule from that device itself, is crazy.

And yes, I remember how all of this came about, but I really don't have
the time to work on it myself...

> > So I don't even think this fixes the issue you think it fixes :)
> 
> This issue is very easy to reproduce on my machine and I have tested the
> patch and it really fixes the issue.

Even with slab debugging enabled?  If so, what is keeping that memory
from being freed once the reference count drops to 0?

I think you are just papering over the real issue here, which is one
reason I really do not like the get_unless_zero() function at all.

thanks,

greg k-h