linux-kernel - Re: [PATCH] usb: ulpi: call put_device if device

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: <20150723200808.GF3683@saruman.tx.rr.com>
Date:	Thu, 23 Jul 2015 15:08:08 -0500
From:	Felipe Balbi <balbi@...com>
To:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC:	Felipe Balbi <balbi@...com>,
	Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
	ChengYi He <chengyihetaipei@...il.com>,
	<linux-usb@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] usb: ulpi: call put_device if device_register fails

Hi,

On Thu, Jul 23, 2015 at 11:00:29AM -0700, Greg Kroah-Hartman wrote:
> On Thu, Jul 23, 2015 at 12:02:40AM -0500, Felipe Balbi wrote:
> > Hi,
> > 
> > On Wed, Jul 22, 2015 at 08:14:46PM -0700, Greg Kroah-Hartman wrote:
> > > On Wed, Jul 22, 2015 at 09:04:40PM -0500, Felipe Balbi wrote:
> > > > On Wed, Jul 22, 2015 at 02:39:34PM -0700, Greg Kroah-Hartman wrote:
> > > > > On Tue, Jun 23, 2015 at 01:57:38PM +0300, Heikki Krogerus wrote:
> > > > > > On Fri, Jun 19, 2015 at 01:12:36AM +0800, ChengYi He wrote:
> > > > > > > put_device is required to release the last reference to the device.
> > > > > > > 
> > > > > > > Signed-off-by: ChengYi He <chengyihetaipei@...il.com>
> > > > > > > ---
> > > > > > >  drivers/usb/common/ulpi.c | 4 +++-
> > > > > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
> > > > > > > index 0e6f968..bd25bdb 100644
> > > > > > > --- a/drivers/usb/common/ulpi.c
> > > > > > > +++ b/drivers/usb/common/ulpi.c
> > > > > > > @@ -184,8 +184,10 @@ static int ulpi_register(struct device *dev, struct ulpi *ulpi)
> > > > > > >  	request_module("ulpi:v%04xp%04x", ulpi->id.vendor, ulpi->id.product);
> > > > > > >  
> > > > > > >  	ret = device_register(&ulpi->dev);
> > > > > > > -	if (ret)
> > > > > > > +	if (ret) {
> > > > > > > +		put_device(&ulpi->dev);
> > > > > > 
> > > > > > If device_register returns failure, put_device has already been
> > > > > > called. Check device_add in drivers/base/core.c.
> > > > > 
> > > > > Yes, please read the function, which says:
> > > > >  * NOTE: _Never_ directly free @dev after calling this function, even
> > > > >  * if it returned an error! Always use put_device() to give up your
> > > > >  * reference instead.
> > > > > 
> > > > > But, the problem is that the ulpi core doesn't "own" that struct device.
> > > > > It comes from elsewhere.  It comes from somewhere deep down in the dw3
> > > > > core, which is where I lost the path.  Something needs to be fixed in
> > > > > dwc3_probe() to properly clean up the device if it fails, which is not
> > > > > happening right now.
> > > > > 
> > > > > So this patch would actually cause much bigger problems than fixing
> > > > > anything, so it's wrong, but for a different reason than you are talking
> > > > > about here.
> > > > > 
> > > > > And ugh, the ulpi and dwc code binding together, what a mess, horrid...
> > > > 
> > > > any suggestions ? DWC *is* the one implementing the bus. If there's a
> > > > better way, we can certainly shuffle code around.
> > > 
> > > As dwc is the only thing using the bus, why is it drivers/usb/core/ ?
> > 
> > musb also has a SW-accessible ULPI bus. And, IIRC, so does DWC2 ;-)
> 
> But they aren't calling ulpi_register(), so how can they be using this
> code?

the thing was just added :-) It didn't exist before.

> > > And the error path here is broken, the bus should be creating the device
> > > (i.e. no subsystem should ever be registering a device it did not
> > > create), so that it can properly clean things up when stuff goes wrong.
> > > 
> > > The whole subsys_init() is also a bad feeling that it's not architected
> > > correctly, that shouldn't be needed, which is why I never took that
> > > patch.  Just noticed it came in through yours, I wanted it "broken" so
> > > it would be fixed "properly" and not papered over like this.
> > 
> > I just felt it would be better to 'fix' it for the -rc until it can be
> > fixed *properly*. A follow up fix should incur no visible changes to
> > drivers anyway.
> 
> I don't like fixes like this because no one now has any pressure to fix
> it "properly".  Are you doing that work?  If not, who is?

Heikki is author, I'd expect him to fix it up. We can also revert the
fix if you prefer, I'm totally fine with that.

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)