Problems with get_driver() and driver_attach() (and new_id too)

Problems with get_driver() and driver_attach() (and new_id too)

[Alan Stern]

Greg and Kay:

There are some nasty problems connected with the driver core's
get_driver(), put_driver(), and driver_attach().  Not just
implementation bugs, but deeper conceptual difficulties.

Let's start with get_driver().  Its comment says that it increments the 
driver's refcount, just like get_device() and a lot of other utility 
routines.

But a struct driver is _not_ like a struct device!  It resembles a
piece of code more than a piece of data -- it acts as an encapsulation
of a driver.  Incrementing its refcount doesn't have much meaning
because a driver's lifetime isn't determined by the structure's
refcount; it's determined by when the driver's module gets unloaded.

What really matters for a driver is whether or not it is registered.  
Drivers expect, for example, that none of their methods will be called
after driver_unregister() returns.  It doesn't matter if some other
thread still holds a reference to the driver structure; that reference
mustn't be used for accessing the driver code after unregistration.  
And of course, driver_attach() does access the driver code, by calling 
the probe routine.

An example where this is violated occurs in the usb-serial core.  Each
serial driver module registers (at least) two driver structures, one on
the usb_serial_bus and one on the usb_bus.  The usb_serial_driver
structure contains a pointer to the usb_driver structure, and this
pointer is passed to get_driver() when the serial driver's new_id sysfs
attribute is written to.

Now, udev scripts are capable of writing to sysfs attributes very soon
after the attribute is created.  In the case of USB serial drivers, we
have a bug report of a situation where this write took place after the
usb_serial_driver was registered but before the usb_driver was
registered.  Thus, get_driver() was handed a pointer to a driver
structure that had not even been initialized, let alone registered, and
so naturally it crashed.

Almost as bad is what can happen when a driver is unregistered while
some thread is holding a reference obtained from get_driver().  The
reference prevents the driver structure from being freed, but it
doesn't prevent the thread from calling driver_attach() after the
unregistration is complete, at which time the driver code does not
expect to be invoked.

To fix these problems, we need to change the semantics of get_driver()  
and put_driver().  Instead of taking a reference to the driver
structure, get_driver() should check whether the driver is currently
registered.  If not, return NULL; otherwise, pin the driver (i.e.,
block it from being unregistered) until put_driver() is called.

This will require some code auditing, because there are places where
get_driver() is called without checking the return value (see
drivers/pci/pci_driver.c:pci_add_dynid() for an example; there are
others).  It should be marked __must_check.

Also, there are places that call driver_attach() without first calling
get_driver() (see drivers/input/gameport/gameport.c,
drivers/input/serio/serio.c, and drivers/char/agp/amd64-agp.c).  They
may or may not be safe; I don't know.


One more thing.  The new_id sysfs attribute can cause problems of its 
own.  Writes to it cause a dynamic ID structure to be allocated, and 
these structures will leak unless they are properly deallocated.  
Normally they are freed when the driver is unregistered.  But what if 
registration fails to begin with?  It might fail at a point after the 
new_id attribute was created, which means the attribute could have been 
written to.  The dynamic IDs need to be freed after registration fails, 
but nobody does this currently.

Alan Stern

Re: Problems with get_driver() and driver_attach() (and new_id too)

[Dmitry Torokhov]

Hi Alan,

On Thu, Jan 05, 2012 at 11:31:00AM -0500, Alan Stern wrote:
> Greg and Kay:
> 
> There are some nasty problems connected with the driver core's
> get_driver(), put_driver(), and driver_attach().  Not just
> implementation bugs, but deeper conceptual difficulties.
> 
> Let's start with get_driver().  Its comment says that it increments the 
> driver's refcount, just like get_device() and a lot of other utility 
> routines.
> 
> But a struct driver is _not_ like a struct device!  It resembles a
> piece of code more than a piece of data -- it acts as an encapsulation
> of a driver.  Incrementing its refcount doesn't have much meaning
> because a driver's lifetime isn't determined by the structure's
> refcount; it's determined by when the driver's module gets unloaded.
> 
> What really matters for a driver is whether or not it is registered.  
> Drivers expect, for example, that none of their methods will be called
> after driver_unregister() returns.  It doesn't matter if some other
> thread still holds a reference to the driver structure; that reference
> mustn't be used for accessing the driver code after unregistration.  
> And of course, driver_attach() does access the driver code, by calling 
> the probe routine.

Agree here.

> 
> An example where this is violated occurs in the usb-serial core.  Each
> serial driver module registers (at least) two driver structures, one on
> the usb_serial_bus and one on the usb_bus.  The usb_serial_driver
> structure contains a pointer to the usb_driver structure, and this
> pointer is passed to get_driver() when the serial driver's new_id sysfs
> attribute is written to.
> 
> Now, udev scripts are capable of writing to sysfs attributes very soon
> after the attribute is created.  In the case of USB serial drivers, we
> have a bug report of a situation where this write took place after the
> usb_serial_driver was registered but before the usb_driver was
> registered.  Thus, get_driver() was handed a pointer to a driver
> structure that had not even been initialized, let alone registered, and
> so naturally it crashed.
> 
> Almost as bad is what can happen when a driver is unregistered while
> some thread is holding a reference obtained from get_driver().  The
> reference prevents the driver structure from being freed, but it
> doesn't prevent the thread from calling driver_attach() after the
> unregistration is complete, at which time the driver code does not
> expect to be invoked.
> 
> To fix these problems, we need to change the semantics of get_driver()  
> and put_driver().  Instead of taking a reference to the driver
> structure, get_driver() should check whether the driver is currently
> registered.  If not, return NULL; otherwise, pin the driver (i.e.,
> block it from being unregistered) until put_driver() is called.

Or maybe we should just drop get_driver() and put_driver() and just make
sure that driver_attach() does not race with driver_unregister()?
I think pinning driver so that it can't be unregistered (and
consequently module unload hangs) its a mis-feature.

> 
> This will require some code auditing, because there are places where
> get_driver() is called without checking the return value (see
> drivers/pci/pci_driver.c:pci_add_dynid() for an example; there are
> others).  It should be marked __must_check.
> 
> Also, there are places that call driver_attach() without first calling
> get_driver() (see drivers/input/gameport/gameport.c,
> drivers/input/serio/serio.c, and drivers/char/agp/amd64-agp.c).  They
> may or may not be safe; I don't know.

Serio and gameport are safe as everyting is protected by serio_mutex so
it is not possible to yank the driver our while we are trying to attach
it to a device.

> 
> One more thing.  The new_id sysfs attribute can cause problems of its 
> own.  Writes to it cause a dynamic ID structure to be allocated, and 
> these structures will leak unless they are properly deallocated.  
> Normally they are freed when the driver is unregistered.  But what if 
> registration fails to begin with?  It might fail at a point after the 
> new_id attribute was created, which means the attribute could have been 
> written to.  The dynamic IDs need to be freed after registration fails, 
> but nobody does this currently.
> 

Don't we create corresponding sysfs attributes only after driver
successfully registered? And attributes are the only way to add (and
thus allocate) new ids so I do not see why we'd be leaking here.

Thanks.

-- 
Dmitry

Re: Problems with get_driver() and driver_attach() (and new_id too)

[Alan Stern]

On Thu, 5 Jan 2012, Dmitry Torokhov wrote:

> > To fix these problems, we need to change the semantics of get_driver()  
> > and put_driver().  Instead of taking a reference to the driver
> > structure, get_driver() should check whether the driver is currently
> > registered.  If not, return NULL; otherwise, pin the driver (i.e.,
> > block it from being unregistered) until put_driver() is called.
> 
> Or maybe we should just drop get_driver() and put_driver() and just make
> sure that driver_attach() does not race with driver_unregister()?

If that could be done, it would be best.  But I'm not sure it can be 
done, at least, not without adding a significant amount of mutual 
exclusion.

In the USB serial core, for example, the problem arises because the
usb_serial_driver is always registered _before_ the corresponding
usb_driver.  Changing the order would fix the problem, but I don't know
if there's some good reason for the way it's done now.  Greg is more
familiar with that code than I am; maybe he knows.

(The underlying issue is that the store_new_id method for one driver
ends up calling driver_attach() for the other driver.  You can see how
this easily leads to races.  Adding a mutex could also solve the
problem, at the price of allowing only one USB driver to be registered
at a time.)

> I think pinning driver so that it can't be unregistered (and
> consequently module unload hangs) its a mis-feature.

I suspect that references obtained from get_driver() aren't held very 
long.  However I haven't checked every case.

> > One more thing.  The new_id sysfs attribute can cause problems of its 
> > own.  Writes to it cause a dynamic ID structure to be allocated, and 
> > these structures will leak unless they are properly deallocated.  
> > Normally they are freed when the driver is unregistered.  But what if 
> > registration fails to begin with?  It might fail at a point after the 
> > new_id attribute was created, which means the attribute could have been 
> > written to.  The dynamic IDs need to be freed after registration fails, 
> > but nobody does this currently.
> > 
> 
> Don't we create corresponding sysfs attributes only after driver
> successfully registered?

No, some attribute files are created during registration;
driver_register() calls driver_add_groups().

> And attributes are the only way to add (and
> thus allocate) new ids so I do not see why we'd be leaking here.

Here's one example of what can happen:

	A module calls driver_register()

	The registration routine creates the
	new_id sysfs attribute

					A udev process writes to the 
					new_id attribute, causing a
					dynamic_id structure to be
					allocated

	Creation of some other attribute fails

	The new_id attribute is removed and
	driver_register() returns an error

At the end the driver isn't registered, but the dynamic_id structure 
has been allocated and will never be freed.

Another example, taken from drivers/pci/pci-driver.c:

	__pci_register_driver() calls
	driver_register()

	pci_create_newid_file() creates the new_id
	sysfs attribute

					A udev process writes to the 
					new_id attribute, causing a
					dynamic_id structure to be
					allocated

	pci_create_removeid_file() fails

	__pci_register_driver() calls
	pci_remove_newid_file() and
	driver_unregister(), but it doesn't
	call pci_free_dynids()

Alan Stern

Re: Problems with get_driver() and driver_attach() (and new_id too)

[Dmitry Torokhov]

On Thu, Jan 05, 2012 at 01:55:41PM -0500, Alan Stern wrote:
> On Thu, 5 Jan 2012, Dmitry Torokhov wrote:
> 
> > > To fix these problems, we need to change the semantics of get_driver()  
> > > and put_driver().  Instead of taking a reference to the driver
> > > structure, get_driver() should check whether the driver is currently
> > > registered.  If not, return NULL; otherwise, pin the driver (i.e.,
> > > block it from being unregistered) until put_driver() is called.
> > 
> > Or maybe we should just drop get_driver() and put_driver() and just make
> > sure that driver_attach() does not race with driver_unregister()?
> 
> If that could be done, it would be best.  But I'm not sure it can be 
> done, at least, not without adding a significant amount of mutual 
> exclusion.

So I looked at the users of device_attach():

- we call it from generic bus code when adding a new driver so naturally
  driver is valid there;
- serio and gameport call it by hand but they ensure that the driver is
  valid because they protected by subsystem-private mutex;
- PCI, PCMCI, HID and USB new_id handling is tied to the driver itself
  and attributes are removed when driver is unregistered so there is no
  chance driver will be attached through newid after it has been
  unregistered;
- agp_amd64_init calls it once immediately after registering the driver;
- pci-stub driver is safe as well.

That leaves only usb-serial which is problematic exactly as you
described below. So I think we should remove get_driver() and
put_driver(); document that caller if driver_attach() should ensure
that driver is live and let usb-serial code solve this issue as this
is the only code that plays games with drivers it does not own.

> 
> In the USB serial core, for example, the problem arises because the
> usb_serial_driver is always registered _before_ the corresponding
> usb_driver.  Changing the order would fix the problem, but I don't know
> if there's some good reason for the way it's done now.  Greg is more
> familiar with that code than I am; maybe he knows.
> 
> (The underlying issue is that the store_new_id method for one driver
> ends up calling driver_attach() for the other driver.  You can see how
> this easily leads to races.  Adding a mutex could also solve the
> problem, at the price of allowing only one USB driver to be registered
> at a time.)
> 
> > I think pinning driver so that it can't be unregistered (and
> > consequently module unload hangs) its a mis-feature.
> 
> I suspect that references obtained from get_driver() aren't held very 
> long.  However I haven't checked every case.

Unless we stop exporting them we can not make any assumptions on how
long they will be held - code is changing constantly.

> 
> > > One more thing.  The new_id sysfs attribute can cause problems of its 
> > > own.  Writes to it cause a dynamic ID structure to be allocated, and 
> > > these structures will leak unless they are properly deallocated.  
> > > Normally they are freed when the driver is unregistered.  But what if 
> > > registration fails to begin with?  It might fail at a point after the 
> > > new_id attribute was created, which means the attribute could have been 
> > > written to.  The dynamic IDs need to be freed after registration fails, 
> > > but nobody does this currently.
> > > 
> > 
> > Don't we create corresponding sysfs attributes only after driver
> > successfully registered?
> 
> No, some attribute files are created during registration;
> driver_register() calls driver_add_groups().

But new_id only created by individual bus code after registering the
driver. No individual drivers involved here.

> 
> > And attributes are the only way to add (and
> > thus allocate) new ids so I do not see why we'd be leaking here.
> 
> Here's one example of what can happen:
> 
> 	A module calls driver_register()
> 
> 	The registration routine creates the
> 	new_id sysfs attribute
> 
> 					A udev process writes to the 
> 					new_id attribute, causing a
> 					dynamic_id structure to be
> 					allocated
> 
> 	Creation of some other attribute fails
> 
> 	The new_id attribute is removed and
> 	driver_register() returns an error
> 
> At the end the driver isn't registered, but the dynamic_id structure 
> has been allocated and will never be freed.
> 
> Another example, taken from drivers/pci/pci-driver.c:
> 
> 	__pci_register_driver() calls
> 	driver_register()
> 
> 	pci_create_newid_file() creates the new_id
> 	sysfs attribute
> 
> 					A udev process writes to the 
> 					new_id attribute, causing a
> 					dynamic_id structure to be
> 					allocated
> 
> 	pci_create_removeid_file() fails
> 
> 	__pci_register_driver() calls
> 	pci_remove_newid_file() and
> 	driver_unregister(), but it doesn't
> 	call pci_free_dynids()

So this is a simple bug in pci bus error unwinding code...

Thanks.

-- 
Dmitry

Re: Problems with get_driver() and driver_attach() (and new_id too)

[Alan Stern]

On Thu, 5 Jan 2012, Dmitry Torokhov wrote:

> So I looked at the users of device_attach():
> 
> - we call it from generic bus code when adding a new driver so naturally
>   driver is valid there;
> - serio and gameport call it by hand but they ensure that the driver is
>   valid because they protected by subsystem-private mutex;
> - PCI, PCMCI, HID and USB new_id handling is tied to the driver itself
>   and attributes are removed when driver is unregistered so there is no
>   chance driver will be attached through newid after it has been
>   unregistered;
> - agp_amd64_init calls it once immediately after registering the driver;
> - pci-stub driver is safe as well.

Okay, that's all good.

> That leaves only usb-serial which is problematic exactly as you
> described below. So I think we should remove get_driver() and
> put_driver(); document that caller if driver_attach() should ensure
> that driver is live and let usb-serial code solve this issue as this
> is the only code that plays games with drivers it does not own.

If Greg confirms that there's nothing with registering the usb driver 
before the usb_serial driver, I can fix the usb-serial code easily.

Greg, do you know offhand whether this will break anything?  It really 
seems like the right thing to do, because the usb_serial driver uses 
the usb driver but not vice versa.


> > > Don't we create corresponding sysfs attributes only after driver
> > > successfully registered?
> > 
> > No, some attribute files are created during registration;
> > driver_register() calls driver_add_groups().
> 
> But new_id only created by individual bus code after registering the
> driver. No individual drivers involved here.

In usb/serial/bus.c, the usb_serial_bus_type structure contains a
.drv_attrs field with an entry for the new_id attribute.  Thus
driver_register calls bus_add_driver, which calls driver_add_attrs,
which creates the attribute file -- all during registration.

> > Another example, taken from drivers/pci/pci-driver.c:
> > 
> > 	__pci_register_driver() calls
> > 	driver_register()
> > 
> > 	pci_create_newid_file() creates the new_id
> > 	sysfs attribute
> > 
> > 					A udev process writes to the 
> > 					new_id attribute, causing a
> > 					dynamic_id structure to be
> > 					allocated
> > 
> > 	pci_create_removeid_file() fails
> > 
> > 	__pci_register_driver() calls
> > 	pci_remove_newid_file() and
> > 	driver_unregister(), but it doesn't
> > 	call pci_free_dynids()
> 
> So this is a simple bug in pci bus error unwinding code...

That was my point -- the places that use new_id attributes don't unwind 
their registration errors correctly.  Fortunately there aren't very 
many such places...

Alan Stern

Re: Problems with get_driver() and driver_attach() (and new_id too)

[Greg KH]

On Thu, Jan 05, 2012 at 03:48:19PM -0500, Alan Stern wrote:
> On Thu, 5 Jan 2012, Dmitry Torokhov wrote:
> 
> > So I looked at the users of device_attach():
> > 
> > - we call it from generic bus code when adding a new driver so naturally
> >   driver is valid there;
> > - serio and gameport call it by hand but they ensure that the driver is
> >   valid because they protected by subsystem-private mutex;
> > - PCI, PCMCI, HID and USB new_id handling is tied to the driver itself
> >   and attributes are removed when driver is unregistered so there is no
> >   chance driver will be attached through newid after it has been
> >   unregistered;
> > - agp_amd64_init calls it once immediately after registering the driver;
> > - pci-stub driver is safe as well.
> 
> Okay, that's all good.
> 
> > That leaves only usb-serial which is problematic exactly as you
> > described below. So I think we should remove get_driver() and
> > put_driver(); document that caller if driver_attach() should ensure
> > that driver is live and let usb-serial code solve this issue as this
> > is the only code that plays games with drivers it does not own.
> 
> If Greg confirms that there's nothing with registering the usb driver 
> before the usb_serial driver, I can fix the usb-serial code easily.
> 
> Greg, do you know offhand whether this will break anything?  It really 
> seems like the right thing to do, because the usb_serial driver uses 
> the usb driver but not vice versa.

Hm, there was a reason I ordered the way I did, as I remember having to
go fix up a number of drivers that did it in the reverse order.  Give me
a day or so to dig it up and figure out what is going on here, and to
review this thread.

thanks,

greg k-h

Re: Problems with get_driver() and driver_attach() (and new_id too)

[Alan Stern]

On Thu, 5 Jan 2012, Greg KH wrote:

> > If Greg confirms that there's nothing with registering the usb driver 
> > before the usb_serial driver, I can fix the usb-serial code easily.
> > 
> > Greg, do you know offhand whether this will break anything?  It really 
> > seems like the right thing to do, because the usb_serial driver uses 
> > the usb driver but not vice versa.
> 
> Hm, there was a reason I ordered the way I did, as I remember having to
> go fix up a number of drivers that did it in the reverse order.  Give me
> a day or so to dig it up and figure out what is going on here, and to
> review this thread.

Well, the one obvious problem, if one isn't careful about it, is that
serial devices present before the module is loaded won't bind properly.  
The usb driver will get probed before the usb_serial driver is
registered, and the probe will fail.

I can fix that easily enough by calling driver_attach() for the usb
driver after all the usb_serial drivers are registered.

Alan Stern

Re: Problems with get_driver() and driver_attach() (and new_id too)

[Alan Stern]

On Thu, 5 Jan 2012, Greg KH wrote:

> > Greg, do you know offhand whether this will break anything?  It really 
> > seems like the right thing to do, because the usb_serial driver uses 
> > the usb driver but not vice versa.
> 
> Hm, there was a reason I ordered the way I did, as I remember having to
> go fix up a number of drivers that did it in the reverse order.  Give me
> a day or so to dig it up and figure out what is going on here, and to
> review this thread.

One more thing.  I just noticed that, unlike all the others, the
qcserial and siemens_mpi modules don't set .no_dynamic_id in their
usb_driver structures.  Is that just an oversight?  I can't think of 
any reason for it.

Alan Stern

Re: Problems with get_driver() and driver_attach() (and new_id too)

[Alan Stern]

On Thu, 5 Jan 2012, Dmitry Torokhov wrote:

> > > I think pinning driver so that it can't be unregistered (and
> > > consequently module unload hangs) its a mis-feature.
> > 
> > I suspect that references obtained from get_driver() aren't held very 
> > long.  However I haven't checked every case.
> 
> Unless we stop exporting them we can not make any assumptions on how
> long they will be held - code is changing constantly.

Something we need to watch out for: get_driver and put_driver are used
in a bunch of other places, unrelated to driver_attach.  Here's what
I found:

lib/dma-debug.c:173:  drv = get_driver(dev->driver);
lib/dma-debug.c:188:  put_driver(drv);
drivers/pci/xen-pcifront.c:596:       if (get_driver(&pdrv->driver)) {
drivers/pci/xen-pcifront.c:626:               put_driver(&pdrv->driver);
drivers/media/video/s5p-fimc/fimc-mdevice.c:348:      put_driver(driver);
drivers/media/video/s5p-fimc/fimc-mdevice.c:356:              put_driver(driver);
drivers/media/video/ivtv/ivtvfb.c:1296:       put_driver(drv);
drivers/media/video/ivtv/ivtvfb.c:1313:       put_driver(drv);
drivers/media/video/cx18/cx18-alsa-main.c:288:        put_driver(drv);
drivers/media/video/s5p-tv/mixer_video.c:61:  put_driver(drv);
drivers/s390/cio/ccwgroup.c:583:      get_driver(&cdriver->driver);
drivers/s390/cio/ccwgroup.c:595:      put_driver(&cdriver->driver);
drivers/s390/cio/device.c:1681:       drv = get_driver(&cdrv->driver);
drivers/s390/cio/device.c:1687:       put_driver(drv);
drivers/s390/net/smsgiucv_app.c:199:  put_driver(smsgiucv_drv);
drivers/ssb/main.c:146:               get_driver(&drv->drv);
drivers/ssb/main.c:153:               put_driver(&drv->drv);
drivers/net/phy/phy_device.c:934:     drv = get_driver(phydev->dev.driver);
drivers/net/phy/phy_device.c:975:     put_driver(dev->driver);

I don't think any of those calls actually accomplish anything, but it's
hard to be certain.  Some of them appear to be futile attempts to
prevent the driver from being unregistered or unloaded, others are
there simply to drop the reference taken by driver_find().

In a few of them it's obvious that the driver can't be unregistered 
while the critical section runs, but in the others I can't tell.  On 
the other hand, if a critical section can race with unregistration 
then the code is buggy now.

What do you think?

Alan Stern

Re: Problems with get_driver() and driver_attach() (and new_id too)

[Dmitry Torokhov]

On Fri, Jan 06, 2012 at 03:29:34PM -0500, Alan Stern wrote:
> On Thu, 5 Jan 2012, Dmitry Torokhov wrote:
> 
> > > > I think pinning driver so that it can't be unregistered (and
> > > > consequently module unload hangs) its a mis-feature.
> > > 
:q> > > I suspect that references obtained from get_driver() aren't held very 
> > > long.  However I haven't checked every case.
> > 
> > Unless we stop exporting them we can not make any assumptions on how
> > long they will be held - code is changing constantly.
> 
> Something we need to watch out for: get_driver and put_driver are used
> in a bunch of other places, unrelated to driver_attach.  Here's what
> I found:
> 
> lib/dma-debug.c:173:  drv = get_driver(dev->driver);
> lib/dma-debug.c:188:  put_driver(drv);
> drivers/pci/xen-pcifront.c:596:       if (get_driver(&pdrv->driver)) {
> drivers/pci/xen-pcifront.c:626:               put_driver(&pdrv->driver);
> drivers/media/video/s5p-fimc/fimc-mdevice.c:348:      put_driver(driver);
> drivers/media/video/s5p-fimc/fimc-mdevice.c:356:              put_driver(driver);
> drivers/media/video/ivtv/ivtvfb.c:1296:       put_driver(drv);
> drivers/media/video/ivtv/ivtvfb.c:1313:       put_driver(drv);
> drivers/media/video/cx18/cx18-alsa-main.c:288:        put_driver(drv);
> drivers/media/video/s5p-tv/mixer_video.c:61:  put_driver(drv);
> drivers/s390/cio/ccwgroup.c:583:      get_driver(&cdriver->driver);
> drivers/s390/cio/ccwgroup.c:595:      put_driver(&cdriver->driver);
> drivers/s390/cio/device.c:1681:       drv = get_driver(&cdrv->driver);
> drivers/s390/cio/device.c:1687:       put_driver(drv);
> drivers/s390/net/smsgiucv_app.c:199:  put_driver(smsgiucv_drv);
> drivers/ssb/main.c:146:               get_driver(&drv->drv);
> drivers/ssb/main.c:153:               put_driver(&drv->drv);
> drivers/net/phy/phy_device.c:934:     drv = get_driver(phydev->dev.driver);
> drivers/net/phy/phy_device.c:975:     put_driver(dev->driver);
> 
> I don't think any of those calls actually accomplish anything, but it's
> hard to be certain.  Some of them appear to be futile attempts to
> prevent the driver from being unregistered or unloaded, others are
> there simply to drop the reference taken by driver_find().
> 
> In a few of them it's obvious that the driver can't be unregistered 
> while the critical section runs, but in the others I can't tell.  On 
> the other hand, if a critical section can race with unregistration 
> then the code is buggy now.
> 
> What do you think?

I think we need to audit them and decide on case-by-case basis. For
example drivers/s390/cio/device.c is completely nonsensical: it takes a
reference on a driver that is passed as argument before calling
driver_find_device(). But if passed driver was valid before we called
get_driver it won't become any more valid afterwards and it should not
disappear either.

drivers/s390/cio/ccwgroup.c - calls are useless;

Authors of drivers/net/phy/phy_device.c had their reservations:

        /* Make sure the driver is held.
         * XXX -- Is this correct? */
        drv = get_driver(phydev->dev.driver);

However it is in phydev_probe() and I hope our device core takes care of
not destroying drivers in the middle of binding to a device.

drivers/ssb/main.c seems like needs some protection but does it
incorrectly as we do not wait for drivers to drop all references before
unloading modules.

Thanks.

-- 
Dmitry

Re: Problems with get_driver() and driver_attach() (and new_id too)

[Alan Stern]

On Mon, 9 Jan 2012, Dmitry Torokhov wrote:

> > I don't think any of those calls actually accomplish anything, but it's
> > hard to be certain.  Some of them appear to be futile attempts to
> > prevent the driver from being unregistered or unloaded, others are
> > there simply to drop the reference taken by driver_find().
> > 
> > In a few of them it's obvious that the driver can't be unregistered 
> > while the critical section runs, but in the others I can't tell.  On 
> > the other hand, if a critical section can race with unregistration 
> > then the code is buggy now.
> > 
> > What do you think?
> 
> I think we need to audit them and decide on case-by-case basis. For
> example drivers/s390/cio/device.c is completely nonsensical: it takes a
> reference on a driver that is passed as argument before calling
> driver_find_device(). But if passed driver was valid before we called
> get_driver it won't become any more valid afterwards and it should not
> disappear either.
> 
> drivers/s390/cio/ccwgroup.c - calls are useless;
> 
> Authors of drivers/net/phy/phy_device.c had their reservations:
> 
>         /* Make sure the driver is held.
>          * XXX -- Is this correct? */
>         drv = get_driver(phydev->dev.driver);
> 
> However it is in phydev_probe() and I hope our device core takes care of
> not destroying drivers in the middle of binding to a device.

Yes, it does.  That one looks like a misunderstanding.  It calls
get_driver during phy_probe and put_driver during phy_remove, which
accomplishes nothing.

> drivers/ssb/main.c seems like needs some protection but does it
> incorrectly as we do not wait for drivers to drop all references before
> unloading modules.

Possibly it needs to be replaced with try_module_get.  I'll send out an 
email to the maintainers of these drivers to see what they think.

Alan Stern

Re: Problems with get_driver() and driver_attach() (and new_id too)

[Dmitry Torokhov]

On Mon, Jan 09, 2012 at 11:37:57AM -0500, Alan Stern wrote:
> On Mon, 9 Jan 2012, Dmitry Torokhov wrote:
> 
> > > I don't think any of those calls actually accomplish anything, but it's
> > > hard to be certain.  Some of them appear to be futile attempts to
> > > prevent the driver from being unregistered or unloaded, others are
> > > there simply to drop the reference taken by driver_find().
> > > 
> > > In a few of them it's obvious that the driver can't be unregistered 
> > > while the critical section runs, but in the others I can't tell.  On 
> > > the other hand, if a critical section can race with unregistration 
> > > then the code is buggy now.
> > > 
> > > What do you think?
> > 
> > I think we need to audit them and decide on case-by-case basis. For
> > example drivers/s390/cio/device.c is completely nonsensical: it takes a
> > reference on a driver that is passed as argument before calling
> > driver_find_device(). But if passed driver was valid before we called
> > get_driver it won't become any more valid afterwards and it should not
> > disappear either.
> > 
> > drivers/s390/cio/ccwgroup.c - calls are useless;
> > 
> > Authors of drivers/net/phy/phy_device.c had their reservations:
> > 
> >         /* Make sure the driver is held.
> >          * XXX -- Is this correct? */
> >         drv = get_driver(phydev->dev.driver);
> > 
> > However it is in phydev_probe() and I hope our device core takes care of
> > not destroying drivers in the middle of binding to a device.
> 
> Yes, it does.  That one looks like a misunderstanding.  It calls
> get_driver during phy_probe and put_driver during phy_remove, which
> accomplishes nothing.
> 
> > drivers/ssb/main.c seems like needs some protection but does it
> > incorrectly as we do not wait for drivers to drop all references before
> > unloading modules.
> 
> Possibly it needs to be replaced with try_module_get.  I'll send out an 
> email to the maintainers of these drivers to see what they think.

No, I am not that try_module_get() [alone] is quite what is needed, as
strictly speaking driver lifetime does not need to be the same as module
lifetime. But maybe I am just splitting hair as all drivers are
statically initialized and are tied to their modules...

-- 
Dmitry

Re: Problems with get_driver() and driver_attach() (and new_id too)

[Alan Stern]

On Mon, 9 Jan 2012, Dmitry Torokhov wrote:

> > > drivers/ssb/main.c seems like needs some protection but does it
> > > incorrectly as we do not wait for drivers to drop all references before
> > > unloading modules.
> > 
> > Possibly it needs to be replaced with try_module_get.  I'll send out an 
> > email to the maintainers of these drivers to see what they think.
> 
> No, I am not that try_module_get() [alone] is quite what is needed, as
> strictly speaking driver lifetime does not need to be the same as module
> lifetime. But maybe I am just splitting hair as all drivers are
> statically initialized and are tied to their modules...

In theory you're right; what matters is when the driver is registered
and unregistered.  (And in fact, although the driver structs are indeed
statically initialized, there also is a struct driver_private part
which doesn't get allocated or initialized until the driver is
registered.)

However, I don't know of any drivers that are unregistered before their
module is unloaded.  We could add an API to the driver core for this
(i.e., to block a driver from being unregistered), but there doesn't
seem to be any real point.

Alan Stern

Re: Problems with get_driver() and driver_attach() (and new_id too)

[Dmitry Torokhov]

On Mon, Jan 09, 2012 at 11:37:57AM -0500, Alan Stern wrote:
> On Mon, 9 Jan 2012, Dmitry Torokhov wrote:
> 
> > > I don't think any of those calls actually accomplish anything, but it's
> > > hard to be certain.  Some of them appear to be futile attempts to
> > > prevent the driver from being unregistered or unloaded, others are
> > > there simply to drop the reference taken by driver_find().
> > > 
> > > In a few of them it's obvious that the driver can't be unregistered 
> > > while the critical section runs, but in the others I can't tell.  On 
> > > the other hand, if a critical section can race with unregistration 
> > > then the code is buggy now.
> > > 
> > > What do you think?
> > 
> > I think we need to audit them and decide on case-by-case basis. For
> > example drivers/s390/cio/device.c is completely nonsensical: it takes a
> > reference on a driver that is passed as argument before calling
> > driver_find_device(). But if passed driver was valid before we called
> > get_driver it won't become any more valid afterwards and it should not
> > disappear either.
> > 
> > drivers/s390/cio/ccwgroup.c - calls are useless;
> > 
> > Authors of drivers/net/phy/phy_device.c had their reservations:
> > 
> >         /* Make sure the driver is held.
> >          * XXX -- Is this correct? */
> >         drv = get_driver(phydev->dev.driver);
> > 
> > However it is in phydev_probe() and I hope our device core takes care of
> > not destroying drivers in the middle of binding to a device.
> 
> Yes, it does.  That one looks like a misunderstanding.  It calls
> get_driver during phy_probe and put_driver during phy_remove, which
> accomplishes nothing.
> 
> > drivers/ssb/main.c seems like needs some protection but does it
> > incorrectly as we do not wait for drivers to drop all references before
> > unloading modules.
> 
> Possibly it needs to be replaced with try_module_get.  I'll send out an 
> email to the maintainers of these drivers to see what they think.

Looking at SSB some more they seem to be releasing devices to make sure
nobody accesses them while SPROM is being written, but while they do
release all devices I do not see how they prevent new binding from
taking place. It all should probably replaced by doing
mutex_trylock(&bus->sprom_lock) in their probe() and using device core
iterators to unbind/rebind the drivers (do we even need to do the
bookkeeping of devices previously bound/not bound?).

Thanks.

-- 
Dmitry

Incorrect uses of get_driver()/put_driver()

[Alan Stern]

The get_driver() and put_driver() routines in the device core are not
documented well, and what they really do is quite different from what
people might think they do.  In particular, get_driver() does not
prevent a driver from being unregistered or unloaded -- the API which 
comes closest to doing that is try_module_get().

In fact, get_driver() and put_driver() are pretty much useless for
normal purposes, and Dmitry and I have been discussing getting rid of
them entirely.  But first we need to make sure that doing so won't mess
anything up.

The purpose of this email is to check with the maintainers of the
various drivers that seem to be using these routines in questionable
ways, to make sure nothing will go wrong.  Here are the places we have 
identified:

lib/dma-debug.c:173:  drv = get_driver(dev->driver);
lib/dma-debug.c:188:  put_driver(drv);

Joerg, these calls don't seem to do anything, as far as I can tell.  
Is there any reason to keep them?

drivers/pci/xen-pcifront.c:596:       if (get_driver(&pdrv->driver)) {
drivers/pci/xen-pcifront.c:626:               put_driver(&pdrv->driver);

Konrad, these calls don't seem to do anything either.

drivers/s390/cio/device.c:1681:       drv = get_driver(&cdrv->driver);
drivers/s390/cio/device.c:1687:       put_driver(drv);

Martin, these calls seem to be useless.  The calls in ccwgroup.c are 
definitely useless; there's no reason to take a reference to a driver 
while it's being unregistered, since it can't go away until the 
unregistration is finished.

drivers/ssb/main.c:146:               get_driver(&drv->drv);
drivers/ssb/main.c:153:               put_driver(&drv->drv);

Michael, these are part of ssb_driver_get() and ssb_driver_put(), used
in ssb_devices_freeze() and ssb_devices_thaw().  They don't currently
do anything, but it looks as if they are meant to prevent the driver
from being unloaded.  Should they be replaced with try_module_get()?  
Or would it be good enough to call device_attach() in 
ssb_devices_thaw()?

Alan Stern

Re: Incorrect uses of get_driver()/put_driver()

[Konrad Rzeszutek Wilk]

On Mon, Jan 09, 2012 at 12:35:09PM -0500, Alan Stern wrote:
> The get_driver() and put_driver() routines in the device core are not
> documented well, and what they really do is quite different from what
> people might think they do.  In particular, get_driver() does not
> prevent a driver from being unregistered or unloaded -- the API which 
> comes closest to doing that is try_module_get().
> 
> In fact, get_driver() and put_driver() are pretty much useless for
> normal purposes, and Dmitry and I have been discussing getting rid of
> them entirely.  But first we need to make sure that doing so won't mess
> anything up.
> 
> The purpose of this email is to check with the maintainers of the
> various drivers that seem to be using these routines in questionable
> ways, to make sure nothing will go wrong.  Here are the places we have 
> identified:
> 
> lib/dma-debug.c:173:  drv = get_driver(dev->driver);
> lib/dma-debug.c:188:  put_driver(drv);
> 
> Joerg, these calls don't seem to do anything, as far as I can tell.  
> Is there any reason to keep them?
> 
> drivers/pci/xen-pcifront.c:596:       if (get_driver(&pdrv->driver)) {
> drivers/pci/xen-pcifront.c:626:               put_driver(&pdrv->driver);
> 
> Konrad, these calls don't seem to do anything either.
> 

Looks like they should be replaced with the try_module_get() equivalant
for the 'struct pci_driver'? Is there such one?

Re: Incorrect uses of get_driver()/put_driver()

[Michael Büsch]

On Mon, 9 Jan 2012 12:35:09 -0500 (EST)
Alan Stern <stern@rowland.harvard.edu> wrote:

> drivers/ssb/main.c:146:               get_driver(&drv->drv);
> drivers/ssb/main.c:153:               put_driver(&drv->drv);
> 
> Michael, these are part of ssb_driver_get() and ssb_driver_put(), used
> in ssb_devices_freeze() and ssb_devices_thaw().  They don't currently
> do anything, but it looks as if they are meant to prevent the driver
> from being unloaded.  Should they be replaced with try_module_get()?  
> Or would it be good enough to call device_attach() in 
> ssb_devices_thaw()?

Hm, It seems that this code is trying to pin the ssb_driver, so that
it doesn't become invalid during the freeze period. So it most likely wants
to protect against module unload and driver unbind here. Not sure
if that actually works, though :/

-- 
Greetings, Michael.

Re: Incorrect uses of get_driver()/put_driver()

[Joerg Roedel]

On Mon, Jan 09, 2012 at 12:35:09PM -0500, Alan Stern wrote:
> lib/dma-debug.c:173:  drv = get_driver(dev->driver);
> lib/dma-debug.c:188:  put_driver(drv);
> 
> Joerg, these calls don't seem to do anything, as far as I can tell.  
> Is there any reason to keep them?

No reason to keep them :) It can be replaced with dev->driver, even
reference counting is not needed because only the pointer is kept and
never dereferenced.


	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: Incorrect uses of get_driver()/put_driver()

[Dmitry Torokhov]

On Mon, Jan 09, 2012 at 07:03:21PM +0100, Michael Büsch wrote:
> On Mon, 9 Jan 2012 12:35:09 -0500 (EST)
> Alan Stern <stern@rowland.harvard.edu> wrote:
> 
> > drivers/ssb/main.c:146:               get_driver(&drv->drv);
> > drivers/ssb/main.c:153:               put_driver(&drv->drv);
> > 
> > Michael, these are part of ssb_driver_get() and ssb_driver_put(), used
> > in ssb_devices_freeze() and ssb_devices_thaw().  They don't currently
> > do anything, but it looks as if they are meant to prevent the driver
> > from being unloaded.  Should they be replaced with try_module_get()?  
> > Or would it be good enough to call device_attach() in 
> > ssb_devices_thaw()?
> 
> Hm, It seems that this code is trying to pin the ssb_driver, so that
> it doesn't become invalid during the freeze period. So it most likely wants
> to protect against module unload and driver unbind here. Not sure
> if that actually works, though :/
> 

Not at all ;)

-- 
Dmitry

Re: Incorrect uses of get_driver()/put_driver()

[Dmitry Torokhov]

On Mon, Jan 09, 2012 at 12:48:36PM -0500, Konrad Rzeszutek Wilk wrote:
> On Mon, Jan 09, 2012 at 12:35:09PM -0500, Alan Stern wrote:
> > The get_driver() and put_driver() routines in the device core are not
> > documented well, and what they really do is quite different from what
> > people might think they do.  In particular, get_driver() does not
> > prevent a driver from being unregistered or unloaded -- the API which 
> > comes closest to doing that is try_module_get().
> > 
> > In fact, get_driver() and put_driver() are pretty much useless for
> > normal purposes, and Dmitry and I have been discussing getting rid of
> > them entirely.  But first we need to make sure that doing so won't mess
> > anything up.
> > 
> > The purpose of this email is to check with the maintainers of the
> > various drivers that seem to be using these routines in questionable
> > ways, to make sure nothing will go wrong.  Here are the places we have 
> > identified:
> > 
> > lib/dma-debug.c:173:  drv = get_driver(dev->driver);
> > lib/dma-debug.c:188:  put_driver(drv);
> > 
> > Joerg, these calls don't seem to do anything, as far as I can tell.  
> > Is there any reason to keep them?
> > 
> > drivers/pci/xen-pcifront.c:596:       if (get_driver(&pdrv->driver)) {
> > drivers/pci/xen-pcifront.c:626:               put_driver(&pdrv->driver);
> > 
> > Konrad, these calls don't seem to do anything either.
> > 
> 
> Looks like they should be replaced with the try_module_get() equivalant
> for the 'struct pci_driver'? Is there such one?

You seem to need stronger guarantees that the driver simply present in
memory. You need to make sure that the driver you fetched is kept being
bound to the device for entire duration of pcifront_common_process().

-- 
Dmitry

Re: Incorrect uses of get_driver()/put_driver()

[Konrad Rzeszutek Wilk]

On Mon, Jan 09, 2012 at 10:20:45AM -0800, Dmitry Torokhov wrote:
> On Mon, Jan 09, 2012 at 12:48:36PM -0500, Konrad Rzeszutek Wilk wrote:
> > On Mon, Jan 09, 2012 at 12:35:09PM -0500, Alan Stern wrote:
> > > The get_driver() and put_driver() routines in the device core are not
> > > documented well, and what they really do is quite different from what
> > > people might think they do.  In particular, get_driver() does not
> > > prevent a driver from being unregistered or unloaded -- the API which 
> > > comes closest to doing that is try_module_get().
> > > 
> > > In fact, get_driver() and put_driver() are pretty much useless for
> > > normal purposes, and Dmitry and I have been discussing getting rid of
> > > them entirely.  But first we need to make sure that doing so won't mess
> > > anything up.
> > > 
> > > The purpose of this email is to check with the maintainers of the
> > > various drivers that seem to be using these routines in questionable
> > > ways, to make sure nothing will go wrong.  Here are the places we have 
> > > identified:
> > > 
> > > lib/dma-debug.c:173:  drv = get_driver(dev->driver);
> > > lib/dma-debug.c:188:  put_driver(drv);
> > > 
> > > Joerg, these calls don't seem to do anything, as far as I can tell.  
> > > Is there any reason to keep them?
> > > 
> > > drivers/pci/xen-pcifront.c:596:       if (get_driver(&pdrv->driver)) {
> > > drivers/pci/xen-pcifront.c:626:               put_driver(&pdrv->driver);
> > > 
> > > Konrad, these calls don't seem to do anything either.
> > > 
> > 
> > Looks like they should be replaced with the try_module_get() equivalant
> > for the 'struct pci_driver'? Is there such one?
> 
> You seem to need stronger guarantees that the driver simply present in
> memory. You need to make sure that the driver you fetched is kept being
> bound to the device for entire duration of pcifront_common_process().

OK, any suggestions?

Re: Incorrect uses of get_driver()/put_driver()

[Dmitry Torokhov]

On Mon, Jan 09, 2012 at 01:34:14PM -0500, Konrad Rzeszutek Wilk wrote:
> On Mon, Jan 09, 2012 at 10:20:45AM -0800, Dmitry Torokhov wrote:
> > On Mon, Jan 09, 2012 at 12:48:36PM -0500, Konrad Rzeszutek Wilk wrote:
> > > On Mon, Jan 09, 2012 at 12:35:09PM -0500, Alan Stern wrote:
> > > > The get_driver() and put_driver() routines in the device core are not
> > > > documented well, and what they really do is quite different from what
> > > > people might think they do.  In particular, get_driver() does not
> > > > prevent a driver from being unregistered or unloaded -- the API which 
> > > > comes closest to doing that is try_module_get().
> > > > 
> > > > In fact, get_driver() and put_driver() are pretty much useless for
> > > > normal purposes, and Dmitry and I have been discussing getting rid of
> > > > them entirely.  But first we need to make sure that doing so won't mess
> > > > anything up.
> > > > 
> > > > The purpose of this email is to check with the maintainers of the
> > > > various drivers that seem to be using these routines in questionable
> > > > ways, to make sure nothing will go wrong.  Here are the places we have 
> > > > identified:
> > > > 
> > > > lib/dma-debug.c:173:  drv = get_driver(dev->driver);
> > > > lib/dma-debug.c:188:  put_driver(drv);
> > > > 
> > > > Joerg, these calls don't seem to do anything, as far as I can tell.  
> > > > Is there any reason to keep them?
> > > > 
> > > > drivers/pci/xen-pcifront.c:596:       if (get_driver(&pdrv->driver)) {
> > > > drivers/pci/xen-pcifront.c:626:               put_driver(&pdrv->driver);
> > > > 
> > > > Konrad, these calls don't seem to do anything either.
> > > > 
> > > 
> > > Looks like they should be replaced with the try_module_get() equivalant
> > > for the 'struct pci_driver'? Is there such one?
> > 
> > You seem to need stronger guarantees that the driver simply present in
> > memory. You need to make sure that the driver you fetched is kept being
> > bound to the device for entire duration of pcifront_common_process().
> 
> OK, any suggestions?

Nothing canned I'm afraid...

-- 
Dmitry

Re: Incorrect uses of get_driver()/put_driver()

[Alan Stern]

On Mon, 9 Jan 2012, Dmitry Torokhov wrote:

> > > > > lib/dma-debug.c:173:  drv = get_driver(dev->driver);
> > > > > lib/dma-debug.c:188:  put_driver(drv);
> > > > > 
> > > > > Joerg, these calls don't seem to do anything, as far as I can tell.  
> > > > > Is there any reason to keep them?
> > > > > 
> > > > > drivers/pci/xen-pcifront.c:596:       if (get_driver(&pdrv->driver)) {
> > > > > drivers/pci/xen-pcifront.c:626:               put_driver(&pdrv->driver);
> > > > > 
> > > > > Konrad, these calls don't seem to do anything either.
> > > > > 
> > > > 
> > > > Looks like they should be replaced with the try_module_get() equivalant
> > > > for the 'struct pci_driver'? Is there such one?
> > > 
> > > You seem to need stronger guarantees that the driver simply present in
> > > memory. You need to make sure that the driver you fetched is kept being
> > > bound to the device for entire duration of pcifront_common_process().
> > 
> > OK, any suggestions?
> 
> Nothing canned I'm afraid...

device_lock(&pcidev->dev) will block unbinding.  If you take the lock 
before looking at pcidev->driver, it should be okay.

The drawback is that pdrv->error_handler may end up doing something 
that takes the same lock.  If you can verify that won't happen, there 
won't be any problem.

Alan Stern

Re: Incorrect uses of get_driver()/put_driver()

[Alan Stern]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8, Size: 1486 bytes --]

On Mon, 9 Jan 2012, Dmitry Torokhov wrote:

> On Mon, Jan 09, 2012 at 07:03:21PM +0100, Michael Büsch wrote:
> > On Mon, 9 Jan 2012 12:35:09 -0500 (EST)
> > Alan Stern <stern@rowland.harvard.edu> wrote:
> > 
> > > drivers/ssb/main.c:146:               get_driver(&drv->drv);
> > > drivers/ssb/main.c:153:               put_driver(&drv->drv);
> > > 
> > > Michael, these are part of ssb_driver_get() and ssb_driver_put(), used
> > > in ssb_devices_freeze() and ssb_devices_thaw().  They don't currently
> > > do anything, but it looks as if they are meant to prevent the driver
> > > from being unloaded.  Should they be replaced with try_module_get()?  
> > > Or would it be good enough to call device_attach() in 
> > > ssb_devices_thaw()?
> > 
> > Hm, It seems that this code is trying to pin the ssb_driver, so that
> > it doesn't become invalid during the freeze period. So it most likely wants
> > to protect against module unload and driver unbind here. Not sure
> > if that actually works, though :/
> > 
> 
> Not at all ;)

Maybe you want to call device_lock(&sdev->dev) here?  It will prevent
the driver from being unbound (and therefore from being unloaded), and
it's likely that sdrv's remove and probe routines expect to be called
with this lock held, because that's what the device core does.  The
drawback is that holding the lock prevents other things from happening
as well, like unregistering sdev.

Alternatively, we can simply remove ssb_driver_get/put.

Alan Stern

Re: Incorrect uses of get_driver()/put_driver()

[Michael Büsch]

On Mon, 9 Jan 2012 14:48:15 -0500 (EST)
Alan Stern <stern@rowland.harvard.edu> wrote:

> Maybe you want to call device_lock(&sdev->dev) here?  It will prevent
> the driver from being unbound (and therefore from being unloaded), and
> it's likely that sdrv's remove and probe routines expect to be called
> with this lock held, because that's what the device core does.  The
> drawback is that holding the lock prevents other things from happening
> as well, like unregistering sdev.
> 
> Alternatively, we can simply remove ssb_driver_get/put.

I think in practice it doesn't matter. This function is only
used in the rare case where the EEPROM on the board is written.

-- 
Greetings, Michael.

Re: Incorrect uses of get_driver()/put_driver()

[Alan Stern]

On Mon, 9 Jan 2012, Michael Büsch wrote:

> On Mon, 9 Jan 2012 14:48:15 -0500 (EST)
> Alan Stern <stern@rowland.harvard.edu> wrote:
> 
> > Maybe you want to call device_lock(&sdev->dev) here?  It will prevent
> > the driver from being unbound (and therefore from being unloaded), and
> > it's likely that sdrv's remove and probe routines expect to be called
> > with this lock held, because that's what the device core does.  The
> > drawback is that holding the lock prevents other things from happening
> > as well, like unregistering sdev.
> > 
> > Alternatively, we can simply remove ssb_driver_get/put.
> 
> I think in practice it doesn't matter. This function is only
> used in the rare case where the EEPROM on the board is written.

Okay, then we can just remove those calls and not worry about it for
now, right?

Alan Stern

Re: Incorrect uses of get_driver()/put_driver()

[Michael Büsch]

On Mon, 9 Jan 2012 17:44:24 -0500 (EST)
Alan Stern <stern@rowland.harvard.edu> wrote:

> On Mon, 9 Jan 2012, Michael Büsch wrote:
> 
> > On Mon, 9 Jan 2012 14:48:15 -0500 (EST)
> > Alan Stern <stern@rowland.harvard.edu> wrote:
> > 
> > > Maybe you want to call device_lock(&sdev->dev) here?  It will prevent
> > > the driver from being unbound (and therefore from being unloaded), and
> > > it's likely that sdrv's remove and probe routines expect to be called
> > > with this lock held, because that's what the device core does.  The
> > > drawback is that holding the lock prevents other things from happening
> > > as well, like unregistering sdev.
> > > 
> > > Alternatively, we can simply remove ssb_driver_get/put.
> > 
> > I think in practice it doesn't matter. This function is only
> > used in the rare case where the EEPROM on the board is written.
> 
> Okay, then we can just remove those calls and not worry about it for
> now, right?

This would be acceptable.

-- 
Greetings, Michael.

Re: Incorrect uses of get_driver()/put_driver()

[Martin Schwidefsky]

On Mon, 9 Jan 2012 12:35:09 -0500 (EST)
Alan Stern <stern@rowland.harvard.edu> wrote:

> drivers/s390/cio/device.c:1681:       drv = get_driver(&cdrv->driver);
> drivers/s390/cio/device.c:1687:       put_driver(drv);
> 
> Martin, these calls seem to be useless.  The calls in ccwgroup.c are 
> definitely useless; there's no reason to take a reference to a driver 
> while it's being unregistered, since it can't go away until the 
> unregistration is finished.

The get_driver/put_driver in ccwgroup.c are obviously useless, the caller
passed ccwgroup_driver_unregister a ccwgroup_driver reference.
I am not so sure about the code in device.c. get_ccwdev_by_busid() gets
used e.g. by vmur like this:

static struct ccw_driver ur_driver = { .. };

static struct urdev *urdev_get_from_devno(u16 devno)
{
	..
	sprintf(bus_id, "0.0.%04x", devno);
        cdev = get_ccwdev_by_busid(&ur_driver, bus_id);
	..
}

static int ur_open(struct inode *inode, struct file *file)
{
	..
	urd = urdev_get_from_devno(devno);
	..
}

For vmur we should be safe by the fact that ur_open is only possible if
a try_module_get has been successful and the ccw_driver is unregistered
only in the module exit function. But we have to check if this is true
for all users of the get_ccwdev_by_busid() function.

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Re: Incorrect uses of get_driver()/put_driver()

[Dmitry Torokhov]

On Tue, Jan 10, 2012 at 10:05:41AM +0100, Martin Schwidefsky wrote:
> On Mon, 9 Jan 2012 12:35:09 -0500 (EST)
> Alan Stern <stern@rowland.harvard.edu> wrote:
> 
> > drivers/s390/cio/device.c:1681:       drv = get_driver(&cdrv->driver);
> > drivers/s390/cio/device.c:1687:       put_driver(drv);
> > 
> > Martin, these calls seem to be useless.  The calls in ccwgroup.c are 
> > definitely useless; there's no reason to take a reference to a driver 
> > while it's being unregistered, since it can't go away until the 
> > unregistration is finished.
> 
> The get_driver/put_driver in ccwgroup.c are obviously useless, the caller
> passed ccwgroup_driver_unregister a ccwgroup_driver reference.
> I am not so sure about the code in device.c. get_ccwdev_by_busid() gets
> used e.g. by vmur like this:

It does not matter how it is being used. Either get_ccwdev_by_busid()
gets a valid driver structure or you already lost. You can not say that
get_driver() protects anything, since if there is a chance driver can
disappear it can disappear before we get to executing get_driver() code.

So while you might want to audit callers get/put_driver inside of
get_ccwdev_by_busid() is utterly useless.

Thanks.

-- 
Dmitry

Re: Incorrect uses of get_driver()/put_driver()

[Martin Schwidefsky]

On Tue, 10 Jan 2012 01:20:46 -0800
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> On Tue, Jan 10, 2012 at 10:05:41AM +0100, Martin Schwidefsky wrote:
> > On Mon, 9 Jan 2012 12:35:09 -0500 (EST)
> > Alan Stern <stern@rowland.harvard.edu> wrote:
> > 
> > > drivers/s390/cio/device.c:1681:       drv = get_driver(&cdrv->driver);
> > > drivers/s390/cio/device.c:1687:       put_driver(drv);
> > > 
> > > Martin, these calls seem to be useless.  The calls in ccwgroup.c are 
> > > definitely useless; there's no reason to take a reference to a driver 
> > > while it's being unregistered, since it can't go away until the 
> > > unregistration is finished.
> > 
> > The get_driver/put_driver in ccwgroup.c are obviously useless, the caller
> > passed ccwgroup_driver_unregister a ccwgroup_driver reference.
> > I am not so sure about the code in device.c. get_ccwdev_by_busid() gets
> > used e.g. by vmur like this:
> 
> It does not matter how it is being used. Either get_ccwdev_by_busid()
> gets a valid driver structure or you already lost. You can not say that
> get_driver() protects anything, since if there is a chance driver can
> disappear it can disappear before we get to executing get_driver() code.
> 
> So while you might want to audit callers get/put_driver inside of
> get_ccwdev_by_busid() is utterly useless.
 
Agreed, we have to rely on the module reference counting and the various
owner fields to prevent driver unregister while we in a code path that might
call get_ccwdev_by_busid(). Lets keep our fingers crossed that all owner
fields are set correctly.

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Re: Incorrect uses of get_driver()/put_driver()

[Sebastian Ott]

On Tue, 10 Jan 2012, Dmitry Torokhov wrote:
> On Tue, Jan 10, 2012 at 10:05:41AM +0100, Martin Schwidefsky wrote:
> > On Mon, 9 Jan 2012 12:35:09 -0500 (EST)
> > Alan Stern <stern@rowland.harvard.edu> wrote:
> > 
> > > drivers/s390/cio/device.c:1681:       drv = get_driver(&cdrv->driver);
> > > drivers/s390/cio/device.c:1687:       put_driver(drv);
> > > 
> > > Martin, these calls seem to be useless.  The calls in ccwgroup.c are 
> > > definitely useless; there's no reason to take a reference to a driver 
> > > while it's being unregistered, since it can't go away until the 
> > > unregistration is finished.
> > 
> > The get_driver/put_driver in ccwgroup.c are obviously useless, the caller
> > passed ccwgroup_driver_unregister a ccwgroup_driver reference.
> > I am not so sure about the code in device.c. get_ccwdev_by_busid() gets
> > used e.g. by vmur like this:
> 
> It does not matter how it is being used. Either get_ccwdev_by_busid()
> gets a valid driver structure or you already lost. You can not say that
> get_driver() protects anything, since if there is a chance driver can
> disappear it can disappear before we get to executing get_driver() code.
> 
> So while you might want to audit callers get/put_driver inside of
> get_ccwdev_by_busid() is utterly useless.

Yes, the caller has to ensure valid driver pointers. We have one offender
here:

[PATCH] zfcp: fops add owner

Set the owner member of zfcp_cfdc_fops, to ensure that the
caller of these functions holds a module reference.

Signed-off-by: Sebastian Ott <sebott@linux.vnet.ibm.com>
---
 drivers/s390/scsi/zfcp_cfdc.c |    2 ++
 1 file changed, 2 insertions(+)

--- a/drivers/s390/scsi/zfcp_cfdc.c
+++ b/drivers/s390/scsi/zfcp_cfdc.c
@@ -13,6 +13,7 @@
 
 #include <linux/slab.h>
 #include <linux/types.h>
+#include <linux/module.h>
 #include <linux/miscdevice.h>
 #include <asm/compat.h>
 #include <asm/ccwdev.h>
@@ -249,6 +250,7 @@ static long zfcp_cfdc_dev_ioctl(struct f
 }
 
 static const struct file_operations zfcp_cfdc_fops = {
+	.owner = THIS_MODULE,
 	.open = nonseekable_open,
 	.unlocked_ioctl = zfcp_cfdc_dev_ioctl,
 #ifdef CONFIG_COMPAT



> 
> Thanks.
> 
> -- 
> Dmitry
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
> 

Re: Incorrect uses of get_driver()/put_driver()

[Sebastian Ott]

On Tue, 10 Jan 2012, Martin Schwidefsky wrote:

> On Mon, 9 Jan 2012 12:35:09 -0500 (EST)
> Alan Stern <stern@rowland.harvard.edu> wrote:
> 
> > drivers/s390/cio/device.c:1681:       drv = get_driver(&cdrv->driver);
> > drivers/s390/cio/device.c:1687:       put_driver(drv);
> > 
> > Martin, these calls seem to be useless.  The calls in ccwgroup.c are 
> > definitely useless; there's no reason to take a reference to a driver 
> > while it's being unregistered, since it can't go away until the 
> > unregistration is finished.
> 
> The get_driver/put_driver in ccwgroup.c are obviously useless, the caller
> passed ccwgroup_driver_unregister a ccwgroup_driver reference.
> I am not so sure about the code in device.c. get_ccwdev_by_busid() gets
> used e.g. by vmur like this:
> 
> static struct ccw_driver ur_driver = { .. };
> 
> static struct urdev *urdev_get_from_devno(u16 devno)
> {
> 	..
> 	sprintf(bus_id, "0.0.%04x", devno);
>         cdev = get_ccwdev_by_busid(&ur_driver, bus_id);
> 	..
> }
> 
> static int ur_open(struct inode *inode, struct file *file)
> {
> 	..
> 	urd = urdev_get_from_devno(devno);
> 	..
> }
> 
> For vmur we should be safe by the fact that ur_open is only possible if
> a try_module_get has been successful and the ccw_driver is unregistered
> only in the module exit function. But we have to check if this is true
> for all users of the get_ccwdev_by_busid() function.

Done.
[PATCH] cio: remove {get,put}_driver

Remove useless {get,put}_driver - the caller of the functions
has to ensure valid driver pointers.

Signed-off-by: Sebastian Ott <sebott@linux.vnet.ibm.com>
---
 drivers/s390/cio/ccwgroup.c |    2 --
 drivers/s390/cio/device.c   |    8 +-------
 2 files changed, 1 insertion(+), 9 deletions(-)

--- a/drivers/s390/cio/ccwgroup.c
+++ b/drivers/s390/cio/ccwgroup.c
@@ -580,7 +580,6 @@ void ccwgroup_driver_unregister(struct c
 	struct device *dev;
 
 	/* We don't want ccwgroup devices to live longer than their driver. */
-	get_driver(&cdriver->driver);
 	while ((dev = driver_find_device(&cdriver->driver, NULL, NULL,
 					 __ccwgroup_match_all))) {
 		struct ccwgroup_device *gdev = to_ccwgroupdev(dev);
@@ -592,7 +591,6 @@ void ccwgroup_driver_unregister(struct c
 		mutex_unlock(&gdev->reg_mutex);
 		put_device(dev);
 	}
-	put_driver(&cdriver->driver);
 	driver_unregister(&cdriver->driver);
 }
 EXPORT_SYMBOL(ccwgroup_driver_unregister);
--- a/drivers/s390/cio/device.c
+++ b/drivers/s390/cio/device.c
@@ -1676,15 +1676,9 @@ struct ccw_device *get_ccwdev_by_busid(s
 				       const char *bus_id)
 {
 	struct device *dev;
-	struct device_driver *drv;
 
-	drv = get_driver(&cdrv->driver);
-	if (!drv)
-		return NULL;
-
-	dev = driver_find_device(drv, NULL, (void *)bus_id,
+	dev = driver_find_device(&cdrv->driver, NULL, (void *)bus_id,
 				 __ccwdev_check_busid);
-	put_driver(drv);
 
 	return dev ? to_ccwdev(dev) : NULL;
 }



> 
> -- 
> blue skies,
>    Martin.
> 
> "Reality continues to ruin my life." - Calvin.
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
> 

Re: Incorrect uses of get_driver()/put_driver()

[Alan Stern]

On Tue, 10 Jan 2012, Sebastian Ott wrote:

> [PATCH] cio: remove {get,put}_driver
> 
> Remove useless {get,put}_driver - the caller of the functions
> has to ensure valid driver pointers.
> 
> Signed-off-by: Sebastian Ott <sebott@linux.vnet.ibm.com>
> ---
>  drivers/s390/cio/ccwgroup.c |    2 --
>  drivers/s390/cio/device.c   |    8 +-------
>  2 files changed, 1 insertion(+), 9 deletions(-)
> 
> --- a/drivers/s390/cio/ccwgroup.c
> +++ b/drivers/s390/cio/ccwgroup.c
> @@ -580,7 +580,6 @@ void ccwgroup_driver_unregister(struct c
>  	struct device *dev;
>  
>  	/* We don't want ccwgroup devices to live longer than their driver. */
> -	get_driver(&cdriver->driver);
>  	while ((dev = driver_find_device(&cdriver->driver, NULL, NULL,
>  					 __ccwgroup_match_all))) {
>  		struct ccwgroup_device *gdev = to_ccwgroupdev(dev);
> @@ -592,7 +591,6 @@ void ccwgroup_driver_unregister(struct c
>  		mutex_unlock(&gdev->reg_mutex);
>  		put_device(dev);
>  	}
> -	put_driver(&cdriver->driver);
>  	driver_unregister(&cdriver->driver);
>  }
>  EXPORT_SYMBOL(ccwgroup_driver_unregister);
> --- a/drivers/s390/cio/device.c
> +++ b/drivers/s390/cio/device.c
> @@ -1676,15 +1676,9 @@ struct ccw_device *get_ccwdev_by_busid(s
>  				       const char *bus_id)
>  {
>  	struct device *dev;
> -	struct device_driver *drv;
>  
> -	drv = get_driver(&cdrv->driver);
> -	if (!drv)
> -		return NULL;
> -
> -	dev = driver_find_device(drv, NULL, (void *)bus_id,
> +	dev = driver_find_device(&cdrv->driver, NULL, (void *)bus_id,
>  				 __ccwdev_check_busid);
> -	put_driver(drv);
>  
>  	return dev ? to_ccwdev(dev) : NULL;
>  }

Thanks Sebastian and everybody else.  It looks like there's no reason
not to simply remove get_driver() and put_driver(), so I'll do just
that.  This patch can be part of the job -- unless someone else wants
to commit it first.

To avoid possible conflicts, I'll wait until after the merge window 
closes and things stabilize a little before posting any changes.

Alan Stern