Re: Is devm_* broken ?

[Laurent Pinchart]

Hello Takashi-san,

On Wednesday 15 July 2015 18:34:13 Takashi Iwai wrote:
> On Wed, 15 Jul 2015 18:27:42 +0200, Laurent Pinchart wrote:
> > On Wednesday 15 July 2015 18:20:02 Takashi Iwai wrote:
> >> On Wed, 15 Jul 2015 18:08:34 +0200, Laurent Pinchart wrote:
> >>> On Wednesday 15 July 2015 17:51:28 Takashi Iwai wrote:
> >>>> On Wed, 15 Jul 2015 00:34:53 +0200, Laurent Pinchart wrote:
> >>>>> Hello,
> >>>>> 
> >>>>> I came to realize not too long ago that the following sequence of
> >>>>> events will lead to a crash with any platform driver that uses
> >>>>> devm_* and creates device nodes.
> >>>>> 
> >>>>> 1. Get a platform device bound it its driver
> >>>>> 2. Open the corresponding device node in userspace and keep it
> >>>>> open
> >>>>> 3. Unbind the platform device from its driver through sysfs
> >>>>> 
> >>>>> echo <device-name> >
> >>>>> /sys/bus/platform/drivers/<driver-name>/unbind
> >>>>> 
> >>>>> (or for hotpluggable devices just unplug the device)
> >>>>> 
> >>>>> 4. Close the device node
> >>>>> 5. Enjoy the fireworks
> >>>>> 
> >>>>> While having a device node open prevents modules from being
> >>>>> unloaded, it doesn't prevent devices from being unbound from
> >>>>> drivers. If the driver uses devm_* helpers to allocate memory the
> >>>>> memory will be freed when the device is unbound from the driver,
> >>>>> but that memory will still be used by any operation touching an
> >>>>> open device node.
> >>>>> 
> >>>>> Is devm_* inherently broken ? It's so widely used, tell me I'm
> >>>>> missing something obvious.
> >>>> 
> >>>> I don't think this is specific to devm_*() but it's about the
> >>>> resource management in general.  After bus or driver's remove
> >>>> callback, all device resources that have been assigned by the driver
> >>>> are supposed to be freed, or ready to be freed.
> >>> 
> >>> The remove callback notifies drivers that the device has been removed
> >>> and that it's time to clean up. However, drivers have no control over
> >>> userspace, so they can't force applications to close all open file
> >>> handles, unmap memory and otherwise free all device-related resources
> >>> immediately and synchronously. The best a driver can do is prevent any
> >>> new reference to a resource from being taken by userspace (returning
> >>> an error from open() for instance) and wait until all existing
> >>> references get released before finally freeing resources. This is
> >>> where devm_* hurts as a driver can't delay freeing resources until
> >>> after all references held by userspace are released.
> >> 
> >> Right, and this is what ALSA drivers does in general.
> > 
> > Does that mean that an ALSA driver that uses devm_* will crash if the
> > device is unbound from the driver (possibly because it gets disconnected)
> > while userspace uses the ALSA device ? Isn't that considered as an issue
> > ?
>
> No, usually the driver calls snd_card_remove() and it blocks until all
> resources are closed/freed, thus devm_*() (that will be released after
> that) won't crash.  For the disconnection, there is an asynchronous
> version, and the rest is managed at the last close.

Most frameworks clean up at last close time as well, at least when the drivers 
cooperate. My point was that once the .disconnect() handler returns the devm_* 
allocate resources will be freed automatically, which can happen long before 
last close.

> >>> If I were to switch the uvcvideo driver from kzalloc to devm_kzalloc
> >>> it would crash if the webcam gets disconnected while userspace has the
> >>> V4L2 device node open.
> >> 
> >> The disconnection is a bit different story, but I see your concern.
> > 
> > From a resources release point of view disconnection and unbind are
> > similar.
>
> Similar but slightly different, IIRC.  The disconnect (in USB) is a
> part of unbind, but not vice versa.

The USB interface disconnect operation is called from usb_unbind_interface() 
and the USB device disconnect operation from usb_unbind_device(), respectively 
handling the USB interface driver or the USB device driver .remove() operation 
(usb_unbind_interface() is also called from usb_driver_release_interface()). 
Both .remove() operations are called by the driver core, either from an unbind 
request through sysfs or from a device disconnection, going through 
usb_disconnect() in that case.

usb_disconnect() will not be called in case of a sysfs unbind, but the 
driver's .disconnect() will be called.

-- 
Regards,

Laurent Pinchart

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