Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1753366AbbGOQeQ (ORCPT <rfc822;w@1wt.eu>);
	Wed, 15 Jul 2015 12:34:16 -0400
Received: from cantor2.suse.de ([195.135.220.15]:34874 "EHLO mx2.suse.de"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1752100AbbGOQeO (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Wed, 15 Jul 2015 12:34:14 -0400
Date: Wed, 15 Jul 2015 18:34:13 +0200
Message-ID: <s5hio9ll6re.wl-tiwai@suse.de>
From: Takashi Iwai <tiwai@suse.de>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: linux-kernel@vger.kernel.org
Subject: Re: Is devm_* broken ?
In-Reply-To: <12287299.f8AJmpnxZx@avalon>
References: <1503739.gVWYM3p8QD@avalon>
	<4538487.nB9qxIKry3@avalon>
	<s5hmvyxl7f1.wl-tiwai@suse.de>
	<12287299.f8AJmpnxZx@avalon>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka)
 FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/24.5
 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka")
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 3777
Lines: 78

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.

> > > 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.


Takashi
--
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/