Re: [Ksummit-discuss] [TECH TOPIC] Fix devm_kzalloc, its users, or both

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Fri, Jul 31, 2015 at 06:57:17PM +0200, Julia Lawall wrote:
> 
> 
> On Fri, 31 Jul 2015, Dmitry Torokhov wrote:
> 
> > On Fri, Jul 31, 2015 at 06:34:21PM +0200, Julia Lawall wrote:
> > >
> > >
> > > On Fri, 31 Jul 2015, Laurent Pinchart wrote:
> > >
> > > > Hello,
> > > >
> > > > It recently came to my attention that the way devm_kzalloc() is used by most
> > > > drivers is broken. I've raised the topic on LKML (see
> > > > http://lkml.org/lkml/2015/7/14/741) in the hope that my findings were simply
> > > > wrong, but it turned out I was unfortunately right. As the topic spans lots of
> > > > subsystems I believe it would be a good technical topic for the Kernel Summit.
> > > >
> > > > The issue occurs when drivers use devm_kzalloc() to allocate data structures
> > > > that can be accessed through file operations on a device node. The following
> > > > sequence of events will then lead to a crash.
> > > >
> > > > 1. Get a device bound to its driver
> > > > 2. Open the corresponding device node in userspace and keep it open
> > > > 3. Unbind the device from its driver through sysfs using for instance
> > > >
> > > > 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.
> > >
> > > How is this different from the free happening explicitly in the remove
> > > function?
> >
> > It is not, but often devm* is "sold" as the greatest thing since sliced
> > bread and people use it by default everywhere without a second thought.
> > I see quite a few patches from newer contributors making conversion of
> > drivers to devm and quite a few of them are wrong. I also see quite
> > often suggestions to submitters encouraging using devm* which would be
> > also wrong in those particular scenarios.
> 
> I know about the problem with the interaction with interrupts, but this
> seems to be something else?  Is there a concrete example?  Or are all

I do not have concrete example, but let's say you have driver data that
you allocate with devm and use dev_set_drvdata() to attach to the
device. Then you have character device, and in open you attach your
device to the file structure and in read you do:

	dev = file->private_data;
	drvdata = dev_get_drvdata(dev);


Now that drvdata will not be there once device is unbound, but file may
still be active until userspace closes it.

> cases wrong, because freeing things in the remove function is wrong in the
> first place?

Yes... No... Maybe... ;)

Thanks.

-- 
Dmitry