Re: Is devm_* broken ?

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hello Takashi,

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.

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.

-- 
Regards,

Laurent Pinchart