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

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

[Laurent Pinchart]

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.

Tejun Heo commented that "this really is a general lifetime management 
problem. [...] If a piece of memory isn't attached to the harware side but the 
userland interface side [...], that shouldn't be allocated via devm - it has 
"dev" in its name for a reason."

While I agree that this is how devres operates today, I'm not sure we should 
throw the baby out with the bath water. Using devm_kzalloc() as currently done  
has value, and reverting drivers to the pre-devm memory allocation code would 
make error handling and cleanup code paths more complex again.

Should we introduce a managed allocator for that purpose that would have a 
lifespan explicitly handled by drivers (or, even better, handled automatically 
in a way that would suit our use cases) ? Tejun commented that "a better 
approach would be implementing generic revoke feature and sever open files on 
driver detach so that everything can be shutdown then".

People who might be interested:

- Tejun Heo <tj@kernel.org>
- Shuah Khan <shuah.kh@samsung.com> (for the media controller devres WIP)
- Russell King <linux@arm.linux.org.uk> (for the component framework)
- Anyone who believes that managed memory allocation for driver private 
structures is useful

-- 
Regards,

Laurent Pinchart

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

[Russell King - ARM Linux]

On Fri, Jul 31, 2015 at 06:14:14PM +0300, Laurent Pinchart wrote:
> It recently came to my attention that the way devm_kzalloc() is used by most 
> drivers is broken.

I think blaming devm_kzalloc() is actually wrong here.

> 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

Now think about the pre-devm_kzalloc() or pre-devm_* method - replace
the above devm_kzalloc() with a plain kmalloc() + memset() as we used
to have in drivers.

Then realise that such drivers would kfree() in their removal function
which gets called from the unbind operation.

It's really no different - the problem is just the same.

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

Correct, and when the new module loading code came along, these issues
were known about at that time, because a lot of them really aren't new -
they were known from analysis of module removal.  It's just that there's
soo much going on in the kernel today that those who did know are swamped
and can't look at and analyse everything, and there's no real way to pass
that knowledge on.

> Tejun Heo commented that "this really is a general lifetime management 
> problem. [...] If a piece of memory isn't attached to the harware side
> but the userland interface side [...], that shouldn't be allocated via
> devm - it has "dev" in its name for a reason."

I think that's completely missing the problem.  This has very little to
do with devm_*, as I've illustrated above, and more to do with people
being aware of and properly handling the lifetimes of data structures.

It's been well proven that many kernel programmers just don't have a grasp
of data structure lifetimes - you only have to look at the struggle we've
had with stuff like users of the driver model not implementing a proper
->release callback (eg, lets provide an empty function to shut the kernel's
warning up!)

Even if we went through every driver today, and fixed up these bugs, we
would still be bogged down with lots of people just not having a clue
about this.  I think we need some other solution to it - one where driver
authors don't have to even think about this lifetime issue, though exactly
what that would be, I don't have any idea at the moment.  The alternative
is that we do handle it the way the driver model does, but rather than
repeating the mistake of having the ->release function called as soon as
the refcount falls to zero, it's delayed by a random amount of time to
catch people who think that an empty ->release function is acceptable -
and make that non-optional.

> People who might be interested:
> 
> - Tejun Heo <tj@kernel.org>
> - Shuah Khan <shuah.kh@samsung.com> (for the media controller devres WIP)
> - Russell King <linux@arm.linux.org.uk> (for the component framework)

I don't believe the component helpers are affected by this as they don't
talk to userland in any way.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

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

[Julia Lawall]

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?

(For some reason, I can't access the lkml discussion).

julia


>
> Tejun Heo commented that "this really is a general lifetime management
> problem. [...] If a piece of memory isn't attached to the harware side but the
> userland interface side [...], that shouldn't be allocated via devm - it has
> "dev" in its name for a reason."
>
> While I agree that this is how devres operates today, I'm not sure we should
> throw the baby out with the bath water. Using devm_kzalloc() as currently done
> has value, and reverting drivers to the pre-devm memory allocation code would
> make error handling and cleanup code paths more complex again.
>
> Should we introduce a managed allocator for that purpose that would have a
> lifespan explicitly handled by drivers (or, even better, handled automatically
> in a way that would suit our use cases) ? Tejun commented that "a better
> approach would be implementing generic revoke feature and sever open files on
> driver detach so that everything can be shutdown then".
>
> People who might be interested:
>
> - Tejun Heo <tj@kernel.org>
> - Shuah Khan <shuah.kh@samsung.com> (for the media controller devres WIP)
> - Russell King <linux@arm.linux.org.uk> (for the component framework)
> - Anyone who believes that managed memory allocation for driver private
> structures is useful
>
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> Ksummit-discuss mailing list
> Ksummit-discuss@lists.linuxfoundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/ksummit-discuss
>

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

[Dmitry Torokhov]

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.

> 
> (For some reason, I can't access the lkml discussion).

Me neither.

> 
> julia
> 
> 
> >
> > Tejun Heo commented that "this really is a general lifetime management
> > problem. [...] If a piece of memory isn't attached to the harware side but the
> > userland interface side [...], that shouldn't be allocated via devm - it has
> > "dev" in its name for a reason."
> >
> > While I agree that this is how devres operates today, I'm not sure we should
> > throw the baby out with the bath water. Using devm_kzalloc() as currently done
> > has value, and reverting drivers to the pre-devm memory allocation code would
> > make error handling and cleanup code paths more complex again.
> >
> > Should we introduce a managed allocator for that purpose that would have a
> > lifespan explicitly handled by drivers (or, even better, handled automatically
> > in a way that would suit our use cases) ? Tejun commented that "a better
> > approach would be implementing generic revoke feature and sever open files on
> > driver detach so that everything can be shutdown then".

Revoke will be useful for file-based access, but I am sure there are
other cases where lifetime of one of interfaces implemented by the
driver will last past the point where we unbind the driver from the
device. Maybe it is subsystem fault in the end.

Note that even though I am one of (or the only?) devm "haters" I do
believe that the API is quite useful if used with consideration and
definitely do not want to throw out the baby with the bath water.

Thanks.

-- 
Dmitry

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

[Christoph Hellwig]

On Fri, Jul 31, 2015 at 06:34:21PM +0200, Julia Lawall wrote:
> How is this different from the free happening explicitly in the remove
> function?

It's not.  The real problem is that people don't understand life time
rules and expect magic interfaces to fix it for them.

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

[Julia Lawall]

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
cases wrong, because freeing things in the remove function is wrong in the
first place?

thanks,
julia

>
> >
> > (For some reason, I can't access the lkml discussion).
>
> Me neither.
>
> >
> > julia
> >
> >
> > >
> > > Tejun Heo commented that "this really is a general lifetime management
> > > problem. [...] If a piece of memory isn't attached to the harware side but the
> > > userland interface side [...], that shouldn't be allocated via devm - it has
> > > "dev" in its name for a reason."
> > >
> > > While I agree that this is how devres operates today, I'm not sure we should
> > > throw the baby out with the bath water. Using devm_kzalloc() as currently done
> > > has value, and reverting drivers to the pre-devm memory allocation code would
> > > make error handling and cleanup code paths more complex again.
> > >
> > > Should we introduce a managed allocator for that purpose that would have a
> > > lifespan explicitly handled by drivers (or, even better, handled automatically
> > > in a way that would suit our use cases) ? Tejun commented that "a better
> > > approach would be implementing generic revoke feature and sever open files on
> > > driver detach so that everything can be shutdown then".
>
> Revoke will be useful for file-based access, but I am sure there are
> other cases where lifetime of one of interfaces implemented by the
> driver will last past the point where we unbind the driver from the
> device. Maybe it is subsystem fault in the end.
>
> Note that even though I am one of (or the only?) devm "haters" I do
> believe that the API is quite useful if used with consideration and
> definitely do not want to throw out the baby with the bath water.
>
> Thanks.
>
> --
> Dmitry
>

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

[James Bottomley]

On Fri, 2015-07-31 at 09:53 -0700, Christoph Hellwig wrote:
> On Fri, Jul 31, 2015 at 06:34:21PM +0200, Julia Lawall wrote:
> > How is this different from the free happening explicitly in the remove
> > function?
> 
> It's not.  The real problem is that people don't understand life time
> rules and expect magic interfaces to fix it for them.

So surely the rule is that we do this in module removal.  That doesn't
get called until last put on the module and that can't happen (or
shouldn't happen) while userspace is holding open one of the /sys
or /proc interfaces (usually those objects hold a reference on something
within the driver to prevent this).

There is an alternative way of handling this:  that would be to detach
the file from the backing interface at _del time, so sysfs/kernfs would
take over the interface and return -ENODEV or something  meaning we
could tear down the module even if there was an open interface file.
I'm not advocating this as a solution because I can already see the
problems (like how do you switch interfaces atomically) but if this
really is a serious problem, we should explore alternative solutions.

James

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

[Dmitry Torokhov]

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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2213 bytes --]

On Fri, Jul 31, 2015 at 06:14:14PM +0300, Laurent Pinchart wrote:

> 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

lkml.org is down (again) - can you please provide a subject line?

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

Like Julia says I'm not sure this is really related to devm_ - I would
really expect that the majority of users were already broken prior to
the conversion to devm_ since the natural thing is to free things in the
remove() function which has exactly the same issues, the main problem
here is that the file open after device is removed case is rare for most
devices and requires somewhat obscure handling.

> While I agree that this is how devres operates today, I'm not sure we should 
> throw the baby out with the bath water. Using devm_kzalloc() as currently done  
> has value, and reverting drivers to the pre-devm memory allocation code would 
> make error handling and cleanup code paths more complex again.

> Should we introduce a managed allocator for that purpose that would have a 
> lifespan explicitly handled by drivers (or, even better, handled automatically 
> in a way that would suit our use cases) ? Tejun commented that "a better 
> approach would be implementing generic revoke feature and sever open files on 
> driver detach so that everything can be shutdown then".

Tejun's suggestion seems like the most robust thing here - allocation
issues are only going to be one of the problems with userspace accessing
devices that are going away and there's a complexity cost from having
the partially destroyed cases around.  Off the top of my head there's
anything that attempts to access the hardware if it's genuinely gone
away rather than just been soft unbound for example.  If the device can
just invalidate all open files on the way out then that's going to be
exactly what most things want.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Dmitry Torokhov]

On Fri, Jul 31, 2015 at 10:02:39AM -0700, James Bottomley wrote:
> On Fri, 2015-07-31 at 09:53 -0700, Christoph Hellwig wrote:
> > On Fri, Jul 31, 2015 at 06:34:21PM +0200, Julia Lawall wrote:
> > > How is this different from the free happening explicitly in the remove
> > > function?
> > 
> > It's not.  The real problem is that people don't understand life time
> > rules and expect magic interfaces to fix it for them.
> 
> So surely the rule is that we do this in module removal.  That doesn't
> get called until last put on the module and that can't happen (or
> shouldn't happen) while userspace is holding open one of the /sys
> or /proc interfaces (usually those objects hold a reference on something
> within the driver to prevent this).
> 
> There is an alternative way of handling this:  that would be to detach
> the file from the backing interface at _del time, so sysfs/kernfs would
> take over the interface and return -ENODEV or something  meaning we
> could tear down the module even if there was an open interface file.
> I'm not advocating this as a solution because I can already see the
> problems (like how do you switch interfaces atomically) but if this
> really is a serious problem, we should explore alternative solutions.

Tejun already done such "switching" for sysfs so it should be possible,
however (blasphemy!) there are other entities than files that also may
have different lifetime rules that live past device unbinding.

Thanks.

-- 
Dmitry

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

[James Bottomley]

On Fri, 2015-07-31 at 10:05 -0700, Dmitry Torokhov wrote:
> On Fri, Jul 31, 2015 at 10:02:39AM -0700, James Bottomley wrote:
> > On Fri, 2015-07-31 at 09:53 -0700, Christoph Hellwig wrote:
> > > On Fri, Jul 31, 2015 at 06:34:21PM +0200, Julia Lawall wrote:
> > > > How is this different from the free happening explicitly in the remove
> > > > function?
> > > 
> > > It's not.  The real problem is that people don't understand life time
> > > rules and expect magic interfaces to fix it for them.
> > 
> > So surely the rule is that we do this in module removal.  That doesn't
> > get called until last put on the module and that can't happen (or
> > shouldn't happen) while userspace is holding open one of the /sys
> > or /proc interfaces (usually those objects hold a reference on something
> > within the driver to prevent this).
> > 
> > There is an alternative way of handling this:  that would be to detach
> > the file from the backing interface at _del time, so sysfs/kernfs would
> > take over the interface and return -ENODEV or something  meaning we
> > could tear down the module even if there was an open interface file.
> > I'm not advocating this as a solution because I can already see the
> > problems (like how do you switch interfaces atomically) but if this
> > really is a serious problem, we should explore alternative solutions.
> 
> Tejun already done such "switching" for sysfs so it should be possible,
> however (blasphemy!) there are other entities than files that also may
> have different lifetime rules that live past device unbinding.

By unbinding do you mean when the unbind is called, which is fine, the
interface handler is still there, or when the final module put is
called, which is not fine because that's a lifetime problem.  In the
latter case we need a hunting expedition to have them all caught and
shot.

James

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

[Russell King - ARM Linux]

On Fri, Jul 31, 2015 at 06:04:16PM +0100, Mark Brown wrote:
> On Fri, Jul 31, 2015 at 06:14:14PM +0300, Laurent Pinchart wrote:
> 
> > 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
> 
> lkml.org is down (again) - can you please provide a subject line?

I wish people would stop using lkml.org

> > 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.
> 
> Like Julia says I'm not sure this is really related to devm_ - I would
> really expect that the majority of users were already broken prior to
> the conversion to devm_ since the natural thing is to free things in the
> remove() function which has exactly the same issues, the main problem
> here is that the file open after device is removed case is rare for most
> devices and requires somewhat obscure handling.

I completely agree - this has *nothing* to do with devm_ at all.  Any
bugs that are there as a result of converting to devm_kzalloc() where
there before.  99.9% of the devm_kzalloc() conversions are merely
replacing the kmalloc()/kzalloc() in the probe function with devm_kzalloc()
and removing the free() in the remove function.

If _anything_, converting to devm_kzalloc() means that the lifetime of
the data structure is _slightly_ longer - because rather than the data
structure being freed in the remove() callback, it's freed after the
remove() callback has returned.

However, both are just as buggy.

The devm_* aspect of this thread is just an anti-devm_* smoke-screen.
It's completely irrelevant.

> Tejun's suggestion seems like the most robust thing here - allocation
> issues are only going to be one of the problems with userspace accessing
> devices that are going away and there's a complexity cost from having
> the partially destroyed cases around.  Off the top of my head there's
> anything that attempts to access the hardware if it's genuinely gone
> away rather than just been soft unbound for example.  If the device can
> just invalidate all open files on the way out then that's going to be
> exactly what most things want.

Well, accessing hardware is even more of a problem.  Consider that
ioremap()s will also be cleaned up at the same time (whether in
->remove() or in devm cleanup processing - again, not a devm problem)
thereby removing the mapping for accessing the hardware.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

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

[Dmitry Torokhov]

On Fri, Jul 31, 2015 at 10:13:17AM -0700, James Bottomley wrote:
> On Fri, 2015-07-31 at 10:05 -0700, Dmitry Torokhov wrote:
> > On Fri, Jul 31, 2015 at 10:02:39AM -0700, James Bottomley wrote:
> > > On Fri, 2015-07-31 at 09:53 -0700, Christoph Hellwig wrote:
> > > > On Fri, Jul 31, 2015 at 06:34:21PM +0200, Julia Lawall wrote:
> > > > > How is this different from the free happening explicitly in the remove
> > > > > function?
> > > > 
> > > > It's not.  The real problem is that people don't understand life time
> > > > rules and expect magic interfaces to fix it for them.
> > > 
> > > So surely the rule is that we do this in module removal.  That doesn't
> > > get called until last put on the module and that can't happen (or
> > > shouldn't happen) while userspace is holding open one of the /sys
> > > or /proc interfaces (usually those objects hold a reference on something
> > > within the driver to prevent this).
> > > 
> > > There is an alternative way of handling this:  that would be to detach
> > > the file from the backing interface at _del time, so sysfs/kernfs would
> > > take over the interface and return -ENODEV or something  meaning we
> > > could tear down the module even if there was an open interface file.
> > > I'm not advocating this as a solution because I can already see the
> > > problems (like how do you switch interfaces atomically) but if this
> > > really is a serious problem, we should explore alternative solutions.
> > 
> > Tejun already done such "switching" for sysfs so it should be possible,
> > however (blasphemy!) there are other entities than files that also may
> > have different lifetime rules that live past device unbinding.
> 
> By unbinding do you mean when the unbind is called, which is fine, the
> interface handler is still there, or when the final module put is
> called, which is not fine because that's a lifetime problem.  In the
> latter case we need a hunting expedition to have them all caught and
> shot.

I was talking about former because module is normally stays pinned if it
implements file_operations and will not be unpinned until last release
on file (device) is called.

Thanks.

-- 
Dmitry

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

[James Bottomley]

On Fri, 2015-07-31 at 10:33 -0700, Dmitry Torokhov wrote:
> On Fri, Jul 31, 2015 at 10:13:17AM -0700, James Bottomley wrote:
> > On Fri, 2015-07-31 at 10:05 -0700, Dmitry Torokhov wrote:
> > > On Fri, Jul 31, 2015 at 10:02:39AM -0700, James Bottomley wrote:
> > > > On Fri, 2015-07-31 at 09:53 -0700, Christoph Hellwig wrote:
> > > > > On Fri, Jul 31, 2015 at 06:34:21PM +0200, Julia Lawall wrote:
> > > > > > How is this different from the free happening explicitly in the remove
> > > > > > function?
> > > > > 
> > > > > It's not.  The real problem is that people don't understand life time
> > > > > rules and expect magic interfaces to fix it for them.
> > > > 
> > > > So surely the rule is that we do this in module removal.  That doesn't
> > > > get called until last put on the module and that can't happen (or
> > > > shouldn't happen) while userspace is holding open one of the /sys
> > > > or /proc interfaces (usually those objects hold a reference on something
> > > > within the driver to prevent this).
> > > > 
> > > > There is an alternative way of handling this:  that would be to detach
> > > > the file from the backing interface at _del time, so sysfs/kernfs would
> > > > take over the interface and return -ENODEV or something  meaning we
> > > > could tear down the module even if there was an open interface file.
> > > > I'm not advocating this as a solution because I can already see the
> > > > problems (like how do you switch interfaces atomically) but if this
> > > > really is a serious problem, we should explore alternative solutions.
> > > 
> > > Tejun already done such "switching" for sysfs so it should be possible,
> > > however (blasphemy!) there are other entities than files that also may
> > > have different lifetime rules that live past device unbinding.
> > 
> > By unbinding do you mean when the unbind is called, which is fine, the
> > interface handler is still there, or when the final module put is
> > called, which is not fine because that's a lifetime problem.  In the
> > latter case we need a hunting expedition to have them all caught and
> > shot.
> 
> I was talking about former because module is normally stays pinned if it
> implements file_operations and will not be unpinned until last release
> on file (device) is called.

Yes, I thought so, so that tells us the problem isn't really lifetime
rules per-se (the handlers and module are still present and won't be
released until the pinned object is), it's about the way we handle the
teardown ... effectively the devm_ memory is released at the wrong point
during teardown.  Effectively, if this happens a lot, it's saying our
rules and best practises for this are hard to follow.

James

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

[Dmitry Torokhov]

On Fri, Jul 31, 2015 at 10:36:28AM -0700, James Bottomley wrote:
> On Fri, 2015-07-31 at 10:33 -0700, Dmitry Torokhov wrote:
> > On Fri, Jul 31, 2015 at 10:13:17AM -0700, James Bottomley wrote:
> > > On Fri, 2015-07-31 at 10:05 -0700, Dmitry Torokhov wrote:
> > > > On Fri, Jul 31, 2015 at 10:02:39AM -0700, James Bottomley wrote:
> > > > > On Fri, 2015-07-31 at 09:53 -0700, Christoph Hellwig wrote:
> > > > > > On Fri, Jul 31, 2015 at 06:34:21PM +0200, Julia Lawall wrote:
> > > > > > > How is this different from the free happening explicitly in the remove
> > > > > > > function?
> > > > > > 
> > > > > > It's not.  The real problem is that people don't understand life time
> > > > > > rules and expect magic interfaces to fix it for them.
> > > > > 
> > > > > So surely the rule is that we do this in module removal.  That doesn't
> > > > > get called until last put on the module and that can't happen (or
> > > > > shouldn't happen) while userspace is holding open one of the /sys
> > > > > or /proc interfaces (usually those objects hold a reference on something
> > > > > within the driver to prevent this).
> > > > > 
> > > > > There is an alternative way of handling this:  that would be to detach
> > > > > the file from the backing interface at _del time, so sysfs/kernfs would
> > > > > take over the interface and return -ENODEV or something  meaning we
> > > > > could tear down the module even if there was an open interface file.
> > > > > I'm not advocating this as a solution because I can already see the
> > > > > problems (like how do you switch interfaces atomically) but if this
> > > > > really is a serious problem, we should explore alternative solutions.
> > > > 
> > > > Tejun already done such "switching" for sysfs so it should be possible,
> > > > however (blasphemy!) there are other entities than files that also may
> > > > have different lifetime rules that live past device unbinding.
> > > 
> > > By unbinding do you mean when the unbind is called, which is fine, the
> > > interface handler is still there, or when the final module put is
> > > called, which is not fine because that's a lifetime problem.  In the
> > > latter case we need a hunting expedition to have them all caught and
> > > shot.
> > 
> > I was talking about former because module is normally stays pinned if it
> > implements file_operations and will not be unpinned until last release
> > on file (device) is called.
> 
> Yes, I thought so, so that tells us the problem isn't really lifetime
> rules per-se (the handlers and module are still present and won't be
> released until the pinned object is), it's about the way we handle the
> teardown ... effectively the devm_ memory is released at the wrong point
> during teardown.  Effectively, if this happens a lot, it's saying our
> rules and best practises for this are hard to follow.

I am not sure why you are making distinction between module lifetime
rules and driver data lifetime rules. Both are objects that have certain
lifetimes and when there is a mismatch in lifetimes between objects that
use/being used the havoc ensues.

Thanks.

-- 
Dmitry

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

[James Bottomley]

On Fri, 2015-07-31 at 11:28 -0700, Dmitry Torokhov wrote:
> On Fri, Jul 31, 2015 at 10:36:28AM -0700, James Bottomley wrote:
> > On Fri, 2015-07-31 at 10:33 -0700, Dmitry Torokhov wrote:
> > > On Fri, Jul 31, 2015 at 10:13:17AM -0700, James Bottomley wrote:
> > > > On Fri, 2015-07-31 at 10:05 -0700, Dmitry Torokhov wrote:
> > > > > On Fri, Jul 31, 2015 at 10:02:39AM -0700, James Bottomley wrote:
> > > > > > On Fri, 2015-07-31 at 09:53 -0700, Christoph Hellwig wrote:
> > > > > > > On Fri, Jul 31, 2015 at 06:34:21PM +0200, Julia Lawall wrote:
> > > > > > > > How is this different from the free happening explicitly in the remove
> > > > > > > > function?
> > > > > > > 
> > > > > > > It's not.  The real problem is that people don't understand life time
> > > > > > > rules and expect magic interfaces to fix it for them.
> > > > > > 
> > > > > > So surely the rule is that we do this in module removal.  That doesn't
> > > > > > get called until last put on the module and that can't happen (or
> > > > > > shouldn't happen) while userspace is holding open one of the /sys
> > > > > > or /proc interfaces (usually those objects hold a reference on something
> > > > > > within the driver to prevent this).
> > > > > > 
> > > > > > There is an alternative way of handling this:  that would be to detach
> > > > > > the file from the backing interface at _del time, so sysfs/kernfs would
> > > > > > take over the interface and return -ENODEV or something  meaning we
> > > > > > could tear down the module even if there was an open interface file.
> > > > > > I'm not advocating this as a solution because I can already see the
> > > > > > problems (like how do you switch interfaces atomically) but if this
> > > > > > really is a serious problem, we should explore alternative solutions.
> > > > > 
> > > > > Tejun already done such "switching" for sysfs so it should be possible,
> > > > > however (blasphemy!) there are other entities than files that also may
> > > > > have different lifetime rules that live past device unbinding.
> > > > 
> > > > By unbinding do you mean when the unbind is called, which is fine, the
> > > > interface handler is still there, or when the final module put is
> > > > called, which is not fine because that's a lifetime problem.  In the
> > > > latter case we need a hunting expedition to have them all caught and
> > > > shot.
> > > 
> > > I was talking about former because module is normally stays pinned if it
> > > implements file_operations and will not be unpinned until last release
> > > on file (device) is called.
> > 
> > Yes, I thought so, so that tells us the problem isn't really lifetime
> > rules per-se (the handlers and module are still present and won't be
> > released until the pinned object is), it's about the way we handle the
> > teardown ... effectively the devm_ memory is released at the wrong point
> > during teardown.  Effectively, if this happens a lot, it's saying our
> > rules and best practises for this are hard to follow.
> 
> I am not sure why you are making distinction between module lifetime
> rules and driver data lifetime rules. Both are objects that have certain
> lifetimes and when there is a mismatch in lifetimes between objects that
> use/being used the havoc ensues.

Driver static data (as opposed to the dynamic stuff we allocate to
service requests) falls into two categories: that which can be released
at del_ time and that which can only be released at module last put
time.  It sounds like we have some data in the wrong category.  As
Russell says, this looks to be an orthogonal problem to devm_kzalloc.

James

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

[Dmitry Torokhov]

On Fri, Jul 31, 2015 at 11:40:39AM -0700, James Bottomley wrote:
> On Fri, 2015-07-31 at 11:28 -0700, Dmitry Torokhov wrote:
> > On Fri, Jul 31, 2015 at 10:36:28AM -0700, James Bottomley wrote:
> > > On Fri, 2015-07-31 at 10:33 -0700, Dmitry Torokhov wrote:
> > > > On Fri, Jul 31, 2015 at 10:13:17AM -0700, James Bottomley wrote:
> > > > > On Fri, 2015-07-31 at 10:05 -0700, Dmitry Torokhov wrote:
> > > > > > On Fri, Jul 31, 2015 at 10:02:39AM -0700, James Bottomley wrote:
> > > > > > > On Fri, 2015-07-31 at 09:53 -0700, Christoph Hellwig wrote:
> > > > > > > > On Fri, Jul 31, 2015 at 06:34:21PM +0200, Julia Lawall wrote:
> > > > > > > > > How is this different from the free happening explicitly in the remove
> > > > > > > > > function?
> > > > > > > > 
> > > > > > > > It's not.  The real problem is that people don't understand life time
> > > > > > > > rules and expect magic interfaces to fix it for them.
> > > > > > > 
> > > > > > > So surely the rule is that we do this in module removal.  That doesn't
> > > > > > > get called until last put on the module and that can't happen (or
> > > > > > > shouldn't happen) while userspace is holding open one of the /sys
> > > > > > > or /proc interfaces (usually those objects hold a reference on something
> > > > > > > within the driver to prevent this).
> > > > > > > 
> > > > > > > There is an alternative way of handling this:  that would be to detach
> > > > > > > the file from the backing interface at _del time, so sysfs/kernfs would
> > > > > > > take over the interface and return -ENODEV or something  meaning we
> > > > > > > could tear down the module even if there was an open interface file.
> > > > > > > I'm not advocating this as a solution because I can already see the
> > > > > > > problems (like how do you switch interfaces atomically) but if this
> > > > > > > really is a serious problem, we should explore alternative solutions.
> > > > > > 
> > > > > > Tejun already done such "switching" for sysfs so it should be possible,
> > > > > > however (blasphemy!) there are other entities than files that also may
> > > > > > have different lifetime rules that live past device unbinding.
> > > > > 
> > > > > By unbinding do you mean when the unbind is called, which is fine, the
> > > > > interface handler is still there, or when the final module put is
> > > > > called, which is not fine because that's a lifetime problem.  In the
> > > > > latter case we need a hunting expedition to have them all caught and
> > > > > shot.
> > > > 
> > > > I was talking about former because module is normally stays pinned if it
> > > > implements file_operations and will not be unpinned until last release
> > > > on file (device) is called.
> > > 
> > > Yes, I thought so, so that tells us the problem isn't really lifetime
> > > rules per-se (the handlers and module are still present and won't be
> > > released until the pinned object is), it's about the way we handle the
> > > teardown ... effectively the devm_ memory is released at the wrong point
> > > during teardown.  Effectively, if this happens a lot, it's saying our
> > > rules and best practises for this are hard to follow.
> > 
> > I am not sure why you are making distinction between module lifetime
> > rules and driver data lifetime rules. Both are objects that have certain
> > lifetimes and when there is a mismatch in lifetimes between objects that
> > use/being used the havoc ensues.
> 
> Driver static data (as opposed to the dynamic stuff we allocate to
> service requests) falls into two categories: that which can be released
> at del_ time and that which can only be released at module last put
> time.  It sounds like we have some data in the wrong category.  As

No, all data can only be released when there is noone using it. I am
fairly certain we can come up with multitude of examples where data can
not be released at del_ time but can be released earlier than entire
module unload. Or, in some cases, it actually needs to stay past the
module unload.

>
> Russell says, this looks to be an orthogonal problem to devm_kzalloc.

Yes, it is except that devm_* is magical and people expect it to somehow
release resources only at the right time.

Thanks.

-- 
Dmitry

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

[Laurent Pinchart]

On Friday 31 July 2015 18:04:16 Mark Brown wrote:
> On Fri, Jul 31, 2015 at 06:14:14PM +0300, Laurent Pinchart wrote:
> > 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
>
> lkml.org is down (again) - can you please provide a subject line?

Sure. "Is devm_* broken ?", and 
http://permalink.gmane.org/gmane.linux.kernel/1996077 if that can help.
 
> > 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.
> 
> Like Julia says I'm not sure this is really related to devm_ - I would
> really expect that the majority of users were already broken prior to
> the conversion to devm_ since the natural thing is to free things in the
> remove() function which has exactly the same issues, the main problem
> here is that the file open after device is removed case is rare for most
> devices and requires somewhat obscure handling.

Why do you think it would be rare ? Pretty much any driver that creates a 
character device will suffer from the issue if userspace decides to keep the 
device node open. For hot-pluggable devices such as USB I'd even argue that 
this is a quite common case, the user can easily unplug a webcam while the 
character device node is in use.

> > While I agree that this is how devres operates today, I'm not sure we
> > should throw the baby out with the bath water. Using devm_kzalloc() as
> > currently done has value, and reverting drivers to the pre-devm memory
> > allocation code would make error handling and cleanup code paths more
> > complex again.
> > 
> > Should we introduce a managed allocator for that purpose that would have a
> > lifespan explicitly handled by drivers (or, even better, handled
> > automatically in a way that would suit our use cases) ? Tejun commented
> > that "a better approach would be implementing generic revoke feature and
> > sever open files on driver detach so that everything can be shutdown
> > then".
> 
> Tejun's suggestion seems like the most robust thing here - allocation
> issues are only going to be one of the problems with userspace accessing
> devices that are going away and there's a complexity cost from having
> the partially destroyed cases around.  Off the top of my head there's
> anything that attempts to access the hardware if it's genuinely gone
> away rather than just been soft unbound for example.  If the device can
> just invalidate all open files on the way out then that's going to be
> exactly what most things want.

The way we handle the problem in V4L2 at the moment is to reference count the 
class device and unregister the character device when the last reference goes 
away, after userspace closes all open file handles. The V4L2 core calls a 
driver release callback, where the driver is responsible for cleaning up all 
remaining resources such as memory allocate for driver-specific data 
structures. This model works fine but is sometimes seen as complex by driver 
writers, and prevents usage of devm_kzalloc.

If there was a way to synchronously invalidate open files on the way out, 
ensuring that all pending cdev fops complete and that no new cdev fops call 
can be made, driver-specific memory could be freed at that point. The cdev 
instance could then be left dangling and would be freed by the cdev core when 
the last file handle is closed.

-- 
Regards,

Laurent Pinchart

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

[Julia Lawall]

On Sat, 1 Aug 2015, Laurent Pinchart wrote:

> On Friday 31 July 2015 18:04:16 Mark Brown wrote:
> > On Fri, Jul 31, 2015 at 06:14:14PM +0300, Laurent Pinchart wrote:
> > > 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
> >
> > lkml.org is down (again) - can you please provide a subject line?
> 
> Sure. "Is devm_* broken ?", and 
> http://permalink.gmane.org/gmane.linux.kernel/1996077 if that can help.
>  
> > > 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.
> > 
> > Like Julia says I'm not sure this is really related to devm_ - I would
> > really expect that the majority of users were already broken prior to
> > the conversion to devm_ since the natural thing is to free things in the
> > remove() function which has exactly the same issues, the main problem
> > here is that the file open after device is removed case is rare for most
> > devices and requires somewhat obscure handling.
> 
> Why do you think it would be rare ? Pretty much any driver that creates a 
> character device will suffer from the issue if userspace decides to keep the 
> device node open. For hot-pluggable devices such as USB I'd even argue that 
> this is a quite common case, the user can easily unplug a webcam while the 
> character device node is in use.
> 
> > > While I agree that this is how devres operates today, I'm not sure we
> > > should throw the baby out with the bath water. Using devm_kzalloc() as
> > > currently done has value, and reverting drivers to the pre-devm memory
> > > allocation code would make error handling and cleanup code paths more
> > > complex again.
> > > 
> > > Should we introduce a managed allocator for that purpose that would have a
> > > lifespan explicitly handled by drivers (or, even better, handled
> > > automatically in a way that would suit our use cases) ? Tejun commented
> > > that "a better approach would be implementing generic revoke feature and
> > > sever open files on driver detach so that everything can be shutdown
> > > then".
> > 
> > Tejun's suggestion seems like the most robust thing here - allocation
> > issues are only going to be one of the problems with userspace accessing
> > devices that are going away and there's a complexity cost from having
> > the partially destroyed cases around.  Off the top of my head there's
> > anything that attempts to access the hardware if it's genuinely gone
> > away rather than just been soft unbound for example.  If the device can
> > just invalidate all open files on the way out then that's going to be
> > exactly what most things want.
> 
> The way we handle the problem in V4L2 at the moment is to reference count the 
> class device and unregister the character device when the last reference goes 
> away, after userspace closes all open file handles. The V4L2 core calls a 
> driver release callback, where the driver is responsible for cleaning up all 
> remaining resources such as memory allocate for driver-specific data 
> structures. This model works fine but is sometimes seen as complex by driver 
> writers, and prevents usage of devm_kzalloc.

Would it be possible to call the devres cleanup at this point, rather than 
on the remove function?

julia

> 
> If there was a way to synchronously invalidate open files on the way out, 
> ensuring that all pending cdev fops complete and that no new cdev fops call 
> can be made, driver-specific memory could be freed at that point. The cdev 
> instance could then be left dangling and would be freed by the cdev core when 
> the last file handle is closed.
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> Ksummit-discuss mailing list
> Ksummit-discuss@lists.linuxfoundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/ksummit-discuss
> 

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

[Laurent Pinchart]

Hi Russell,

On Friday 31 July 2015 18:27:32 Russell King - ARM Linux wrote:
> On Fri, Jul 31, 2015 at 06:04:16PM +0100, Mark Brown wrote:
> > On Fri, Jul 31, 2015 at 06:14:14PM +0300, Laurent Pinchart wrote:
> > > 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> 
> > lkml.org is down (again) - can you please provide a subject line?
> 
> I wish people would stop using lkml.org
> 
> > > 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.
> > 
> > Like Julia says I'm not sure this is really related to devm_ - I would
> > really expect that the majority of users were already broken prior to
> > the conversion to devm_ since the natural thing is to free things in the
> > remove() function which has exactly the same issues, the main problem
> > here is that the file open after device is removed case is rare for most
> > devices and requires somewhat obscure handling.
> 
> I completely agree - this has *nothing* to do with devm_ at all.  Any
> bugs that are there as a result of converting to devm_kzalloc() where
> there before.  99.9% of the devm_kzalloc() conversions are merely
> replacing the kmalloc()/kzalloc() in the probe function with devm_kzalloc()
> and removing the free() in the remove function.

In those cases I agree that the bug is not introduced by the conversion. 
However, as we're selling devm_kzalloc() as solving all problems (or, at 
least, as many developers believe it will), many new drivers get submitted 
with blind devm_kzalloc() usage without any thought about proper lifetime 
management for objects allocated by the driver. This gets past (at least some) 
subsystem maintainers, showing that even if the devm_kzalloc() API forbids 
this kind of usage, the message is not conveyed properly. My main concern here 
is that driver developers do the wrong things, and maintainers don't catch the 
problem. In addition to fixing existing code we need to fix that problem.

> If _anything_, converting to devm_kzalloc() means that the lifetime of
> the data structure is _slightly_ longer - because rather than the data
> structure being freed in the remove() callback, it's freed after the
> remove() callback has returned.
> 
> However, both are just as buggy.
> 
> The devm_* aspect of this thread is just an anti-devm_* smoke-screen.
> It's completely irrelevant.
> 
> > Tejun's suggestion seems like the most robust thing here - allocation
> > issues are only going to be one of the problems with userspace accessing
> > devices that are going away and there's a complexity cost from having
> > the partially destroyed cases around.  Off the top of my head there's
> > anything that attempts to access the hardware if it's genuinely gone
> > away rather than just been soft unbound for example.  If the device can
> > just invalidate all open files on the way out then that's going to be
> > exactly what most things want.
> 
> Well, accessing hardware is even more of a problem.  Consider that
> ioremap()s will also be cleaned up at the same time (whether in
> ->remove() or in devm cleanup processing - again, not a devm problem)
> thereby removing the mapping for accessing the hardware.

Drivers are usually in a bit of a better shape when it comes to hardware 
access. Most developers understand that the remove() function is supposed to 
stop the hardware, and most subsystems will prevent operations that touch the 
hardware from reaching drivers after the remove() function unregisters the 
subsystem "class" (for lack of a better word) device. This can still be racy 
if not implemented correctly, but shouldn't be subject to the same issue of 
userspace keeping file handles open long after the device is gone.

This being said, if unregistration/removal/cleanup racing with userspace is a 
problem for which we have solutions, it's still too easy today to implement it 
incorrectly in drivers. That's also something we might want to discuss to 
share best practices among subsystems and come up with a common message we can 
send to driver developers.

-- 
Regards,

Laurent Pinchart

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 520 bytes --]

On Fri, Jul 31, 2015 at 12:41:24PM -0700, Dmitry Torokhov wrote:
> On Fri, Jul 31, 2015 at 11:40:39AM -0700, James Bottomley wrote:

> > Russell says, this looks to be an orthogonal problem to devm_kzalloc.

> Yes, it is except that devm_* is magical and people expect it to somehow
> release resources only at the right time.

Or alternatively it's a really easy to use pattern that eliminates a
large set of potential mistakes and it's therefore worth looking at when
considering how to solve other simlar problems :)

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Laurent Pinchart]

Hi Julia,

On Saturday 01 August 2015 12:55:42 Julia Lawall wrote:
> On Sat, 1 Aug 2015, Laurent Pinchart wrote:
> > On Friday 31 July 2015 18:04:16 Mark Brown wrote:
> > > On Fri, Jul 31, 2015 at 06:14:14PM +0300, Laurent Pinchart wrote:
> > > > 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
> > > 
> > > lkml.org is down (again) - can you please provide a subject line?
> > 
> > Sure. "Is devm_* broken ?", and
> > http://permalink.gmane.org/gmane.linux.kernel/1996077 if that can help.
> > 
> > > > 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.
> > > 
> > > Like Julia says I'm not sure this is really related to devm_ - I would
> > > really expect that the majority of users were already broken prior to
> > > the conversion to devm_ since the natural thing is to free things in the
> > > remove() function which has exactly the same issues, the main problem
> > > here is that the file open after device is removed case is rare for most
> > > devices and requires somewhat obscure handling.
> > 
> > Why do you think it would be rare ? Pretty much any driver that creates a
> > character device will suffer from the issue if userspace decides to keep
> > the device node open. For hot-pluggable devices such as USB I'd even
> > argue that this is a quite common case, the user can easily unplug a
> > webcam while the character device node is in use.
> > 
> > > > While I agree that this is how devres operates today, I'm not sure we
> > > > should throw the baby out with the bath water. Using devm_kzalloc() as
> > > > currently done has value, and reverting drivers to the pre-devm memory
> > > > allocation code would make error handling and cleanup code paths more
> > > > complex again.
> > > > 
> > > > Should we introduce a managed allocator for that purpose that would
> > > > have a lifespan explicitly handled by drivers (or, even better,
> > > > handled automatically in a way that would suit our use cases) ? Tejun
> > > > commented that "a better approach would be implementing generic revoke
> > > > feature and sever open files on driver detach so that everything can
> > > > be shutdown then".
> > > 
> > > Tejun's suggestion seems like the most robust thing here - allocation
> > > issues are only going to be one of the problems with userspace accessing
> > > devices that are going away and there's a complexity cost from having
> > > the partially destroyed cases around.  Off the top of my head there's
> > > anything that attempts to access the hardware if it's genuinely gone
> > > away rather than just been soft unbound for example.  If the device can
> > > just invalidate all open files on the way out then that's going to be
> > > exactly what most things want.
> > 
> > The way we handle the problem in V4L2 at the moment is to reference count
> > the class device and unregister the character device when the last
> > reference goes away, after userspace closes all open file handles. The
> > V4L2 core calls a driver release callback, where the driver is
> > responsible for cleaning up all remaining resources such as memory
> > allocate for driver-specific data structures. This model works fine but
> > is sometimes seen as complex by driver writers, and prevents usage of
> > devm_kzalloc.
> 
> Would it be possible to call the devres cleanup at this point, rather than
> on the remove function?

As devres cleanup will also take care of interrupts and ioremap'ed regions I 
don't think that would be an option. Otherwise we would have trouble when the 
device is reprobed as resources would be busy.

We could possibly create an API to allow drivers to keep references to some 
devres-allocate objects, but that's arguably a hack.

> > If there was a way to synchronously invalidate open files on the way out,
> > ensuring that all pending cdev fops complete and that no new cdev fops
> > call can be made, driver-specific memory could be freed at that point. The
> > cdev instance could then be left dangling and would be freed by the cdev
> > core when the last file handle is closed.

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

On Friday 31 July 2015 09:53:46 Christoph Hellwig wrote:
> On Fri, Jul 31, 2015 at 06:34:21PM +0200, Julia Lawall wrote:
> > How is this different from the free happening explicitly in the remove
> > function?
> 
> It's not.  The real problem is that people don't understand life time
> rules and expect magic interfaces to fix it for them.

Exactly. I see two solutions for that, either making people understand life 
time rules, or creating interfaces that really fix it for them. devm_kzalloc() 
isn't such an interface (and I'm fine if we believe it shouldn't be), but it 
unfortunately gives driver developers the impression that it is supposed to 
fix the problem. If we conclude that the problem is between the chair and the 
keyboard of driver developers then we'll need to start being more vocal about 
it.

-- 
Regards,

Laurent Pinchart

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

[Julia Lawall]

On Sat, 1 Aug 2015, Laurent Pinchart wrote:

> On Friday 31 July 2015 09:53:46 Christoph Hellwig wrote:
> > On Fri, Jul 31, 2015 at 06:34:21PM +0200, Julia Lawall wrote:
> > > How is this different from the free happening explicitly in the remove
> > > function?
> > 
> > It's not.  The real problem is that people don't understand life time
> > rules and expect magic interfaces to fix it for them.
> 
> Exactly. I see two solutions for that, either making people understand life 
> time rules, or creating interfaces that really fix it for them. devm_kzalloc() 
> isn't such an interface (and I'm fine if we believe it shouldn't be), but it 
> unfortunately gives driver developers the impression that it is supposed to 
> fix the problem. If we conclude that the problem is between the chair and the 
> keyboard of driver developers then we'll need to start being more vocal about 
> it.

Currently, is there any documentation about when these functions can be
used?  All I remember seeing is a discussion of what the functions do and
what functions are available.  But nothing about when they should and
should not be used.

julia

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

[Tejun Heo]

Hello,

On Sat, Aug 01, 2015 at 02:01:02PM +0300, Laurent Pinchart wrote:
> > Would it be possible to call the devres cleanup at this point, rather than
> > on the remove function?
> 
> As devres cleanup will also take care of interrupts and ioremap'ed regions I 
> don't think that would be an option. Otherwise we would have trouble when the 
> device is reprobed as resources would be busy.
> 
> We could possibly create an API to allow drivers to keep references to some 
> devres-allocate objects, but that's arguably a hack.

Another factor to consider is that even if the driver separately
allocates e.g. memory which may be accessed after removal, it's mighty
difficult to verify that file operations which take place post removal
don't have any dependency on things which are released on removal.  I
mean, we've always had a lot of drivers which are fairly confused
about lifetime rules.

devm allocated memory *might* make certain failures more obvious as
the memory itself gets freed right away but I'd be very surprised if
there aren't already a bunch of drivers which fail to fully isolate
post-removal file operations regardless of devm.

So, all in all, if we actually want to fix this issue, well at least
most of it, I think the only viable way would be implementing revoke
semantics and drain and sever all operations on removal.  Maybe it'll
end up another devm interface.

Thanks.

-- 
tejun

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

[Russell King - ARM Linux]

On Sat, Aug 01, 2015 at 01:55:54PM +0300, Laurent Pinchart wrote:
> On Friday 31 July 2015 18:27:32 Russell King - ARM Linux wrote:
> > I completely agree - this has *nothing* to do with devm_ at all.  Any
> > bugs that are there as a result of converting to devm_kzalloc() where
> > there before.  99.9% of the devm_kzalloc() conversions are merely
> > replacing the kmalloc()/kzalloc() in the probe function with devm_kzalloc()
> > and removing the free() in the remove function.
> 
> In those cases I agree that the bug is not introduced by the conversion. 
> However, as we're selling devm_kzalloc() as solving all problems (or, at 
> least, as many developers believe it will), many new drivers get submitted 
> with blind devm_kzalloc() usage without any thought about proper lifetime 
> management for objects allocated by the driver.

I don't think anyone has ever said that devm_* solves all problems.  What
I've seen, it has been sold as avoiding problems with memory leaks in the
failed probe path, and as a way to make resource management at driver
unbind time easier.

I don't think it's ever been touted as "the worlds solution to hunger" or
anything of the sort - and as you've already confessed to being a hater
of devm_*, I can only think that the reason you're soo inspired to want to
connect devm_* with this problem is because you have an alterior motive.

> This gets past (at least some) subsystem maintainers, showing that even
> if the devm_kzalloc() API forbids this kind of usage, the message is not
> conveyed properly.

Again, that's not the problem.  Empty release methods for kobjects get past
subsystem maintainers too.  Some subsystem maintainers even create empty
release methods.  It's all exactly the same problem: many kernel developers
_and_ subsystem maintainers do not understand reference counted resource
tracking - that's the _common_ issue here.

That's not necessarily the fault of the subsystem maintainers; if a
subsystem maintainer is overloaded with work, or tired out towards the end
of the day, reviews aren't going to be as thorough as they should be, which
leads to things being missed - and to properly analyse the lifetime of every
structure in a driver submission is Hard.  I know full well, that a lot of
subsystem maintainers struggle with getting finding the basic review points,
so expecting them to properly analyse structure lifetime is, I think,
expecting too much.

Maybe a way forward on this should be that we decide that:

- any structure which gets passed into a driver open() method should be
  kref-counted.
- that structure should be allocated using standard kzalloc()
- the structure should only ever be freed after the last release after
  the driver has been unbound.

It sounds like there is scope there for set of "driver data" helpers,
maybe something like:

struct drvdata {
	struct kref kref;
	u64 data[0];
};

static void *drvdata_fops_data_alloc(size_t size)
{
	struct drvdata *d;

	d = kzalloc(sizeof(*d) + size, GFP_KERNEL);
	if (!d)
		return NULL;

	kref_init(&d->kref);
	d->real_fops = fops;
	return d->data;
}

static void drvdata_fops_release_data(struct kref *kref)
{
	struct drvdata *d = container_of(kref, struct drvdata, kref);
	kfree(d);
}

void drvdata_fops_put(void *drvdata)
{
	struct drvdata *d = container_of(drvdata, struct drvdata, data);
	kref_put(&d->kref, drvdata_fops_release_data);
}

void *drvdata_fops_open(struct file *file)
{
	struct drvdata *d = container_of(file->private_data, struct drvdata, data);

	kref_get(&d->kref);

	return d->data;
}

void drvdata_fops_release(struct file *file)
{
	drvdata_fops_put(file->private_data);
}

The idea being that drvdata_fops_data_alloc() gets called in the driver
probe function, with drvdata_fops_put() in the release function.  Both
of these _could_ have devm_* equivalents to ease driver authors burden
when it comes to error cleanup in the probe.

To get at the driver data, a driver would call drvdata_fops_open() in
their fops open() method, and drvdata_fops_release() in their fops
release() method - it would be nice to hide that from drivers so they
don't have to think about it once they opt-in to this management, though
that makes it too much of a framework rather than a library.

We can make sure that happens if we offset file->private_data with a
(random?  build-time?) cookie to catch anyone who thinks they can
dereference it directly.

> > Well, accessing hardware is even more of a problem.  Consider that
> > ioremap()s will also be cleaned up at the same time (whether in
> > ->remove() or in devm cleanup processing - again, not a devm problem)
> > thereby removing the mapping for accessing the hardware.
> 
> Drivers are usually in a bit of a better shape when it comes to hardware 
> access. Most developers understand that the remove() function is supposed to 
> stop the hardware,

I think you missed the point.

What if a file-operations method provokes some kind of hardware access.
For example, the read() method dereferences the driver data (which has
been freed), obtains a stale pointer to where the hardware was mapped,
and then tries to read a status register to check whether any data has
become available.

The result is, of course, an oops because the io memory has been freed by
the devm_ioremap_resource() cleanup.

What I'm trying to point out where is that this is absolutely no different
from the devm_* problem you're complaining about - exactly the same issues
exist with _any_ accessible resource which is shared between the driver
lifetime and the file-operations lifetime.

That, again, has absolutely nothing to do with the "evil devm" stuff.  We
have _always_ cleaned up such resources in the driver unbind path with an
iounmap(), and I bet that most of these are buggy in almost all drivers
which also access this memory region via the file-operations methods.

So, I think you ought to get off the "devm is evil" bandwagon, and realise
that the problem you have raised has absolutely nothing at all to do with
devm_* but is much more of a general "people don't understand resource
lifetime issues" problem.

Even I will hold my hand up to having written buggy drivers with exactly
the kind of issues raised in this thread (I'm looking at one right now,
though not in mainline...)

Here's an example of a driver which is buggy as we've been describing
here, but does not use devm*: drivers/uio/uio_pruss.c.  In pruss_cleanup(),
it kfree()s the array of struct uio_info structures, which are used by
drivers/uio/uio.c.  This structure can be accessed via the mmap(), write(),
release(), poll(), etc file-operations methods.  None of them would be
safe if the driver has been unbound with the chardev open - and it'd be
dangerous to close the port once the driver had been unbound.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

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

[Guenter Roeck]

On 08/01/2015 08:18 AM, Tejun Heo wrote:
> Hello,
>
> On Sat, Aug 01, 2015 at 02:01:02PM +0300, Laurent Pinchart wrote:
>>> Would it be possible to call the devres cleanup at this point, rather than
>>> on the remove function?
>>
>> As devres cleanup will also take care of interrupts and ioremap'ed regions I
>> don't think that would be an option. Otherwise we would have trouble when the
>> device is reprobed as resources would be busy.
>>
>> We could possibly create an API to allow drivers to keep references to some
>> devres-allocate objects, but that's arguably a hack.
>
> Another factor to consider is that even if the driver separately
> allocates e.g. memory which may be accessed after removal, it's mighty
> difficult to verify that file operations which take place post removal
> don't have any dependency on things which are released on removal.  I
> mean, we've always had a lot of drivers which are fairly confused
> about lifetime rules.
>
> devm allocated memory *might* make certain failures more obvious as
> the memory itself gets freed right away but I'd be very surprised if
> there aren't already a bunch of drivers which fail to fully isolate
> post-removal file operations regardless of devm.
>
> So, all in all, if we actually want to fix this issue, well at least
> most of it, I think the only viable way would be implementing revoke
> semantics and drain and sever all operations on removal.  Maybe it'll
> end up another devm interface.
>

Maybe that is completely nonsense, but how about something like
	devm_get(dev);
to be called whenever a file used by a device is opened, and
	devm_put(dev);
whenever it is closed ?

Guenter

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

[Russell King - ARM Linux]

On Fri, Jul 31, 2015 at 12:41:24PM -0700, Dmitry Torokhov wrote:
> On Fri, Jul 31, 2015 at 11:40:39AM -0700, James Bottomley wrote:
> > Russell says, this looks to be an orthogonal problem to devm_kzalloc.
> 
> Yes, it is except that devm_* is magical and people expect it to somehow
> release resources only at the right time.

Who's been selling it as such?  If it's been mis-sold, then maybe there's
a responsibility on people who see such mis-selling of the API to correct
those creating this fantasy, rather than ignoring it.

People try to create APIs which ease the burden of review, and then other
people go around selling it as the worlds cure to hunger.  If no one
bothers to correct those doing the mis-selling, then that's not the fault
of the API.  That's a community problem.  Don't blame the API.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

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

[Julia Lawall]

On Sun, 2 Aug 2015, Russell King - ARM Linux wrote:

> On Fri, Jul 31, 2015 at 12:41:24PM -0700, Dmitry Torokhov wrote:
> > On Fri, Jul 31, 2015 at 11:40:39AM -0700, James Bottomley wrote:
> > > Russell says, this looks to be an orthogonal problem to devm_kzalloc.
> > 
> > Yes, it is except that devm_* is magical and people expect it to somehow
> > release resources only at the right time.
> 
> Who's been selling it as such?  If it's been mis-sold, then maybe there's
> a responsibility on people who see such mis-selling of the API to correct
> those creating this fantasy, rather than ignoring it.
> 
> People try to create APIs which ease the burden of review, and then other
> people go around selling it as the worlds cure to hunger.  If no one
> bothers to correct those doing the mis-selling, then that's not the fault
> of the API.  That's a community problem.  Don't blame the API.

I don't know who is selling it as a cure to everything, but I have seen 
comments from developers that are along the lines of everyone else is 
using devm_xxx, so my use of it should be correct too.

KVM has a "Code Transitions" page (http://wiki.qemu.org/CodeTransitions).  
Such a page could be a place to collect expertise on when a change can and 
cannot be made.  Of course someone would have to maintain it, but perhaps 
maintainers who saw something done wrong a tiresome number of times would 
take care of it.

julia

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

[Russell King - ARM Linux]

On Sat, Aug 01, 2015 at 05:48:30PM -0700, Guenter Roeck wrote:
> On 08/01/2015 08:18 AM, Tejun Heo wrote:
> >So, all in all, if we actually want to fix this issue, well at least
> >most of it, I think the only viable way would be implementing revoke
> >semantics and drain and sever all operations on removal.  Maybe it'll
> >end up another devm interface.
> 
> Maybe that is completely nonsense, but how about something like
> 	devm_get(dev);
> to be called whenever a file used by a device is opened, and
> 	devm_put(dev);
> whenever it is closed ?

Absolutely not.  There's two problems there:

1) You'd also need to tie the lifetime of 'dev' to the lifetime of the
   character device, which is getting to be an absurd level of lifetime
   complexity - we can't have 'dev' vanishing before the last user of
   the file operations has gone away, otherwise 'dev' becomes a stale
   pointer.

2) Remember that devm encompasses not only driver allocations, but also
   resources as well, like interrupts, iomem mappings and iomem resource
   claiming.  As has already been stated by others, we don't want to delay
   the release of all these resources.

I think a proposition would be to come up with some kind of driver
interface for things like chardev drivers which wraps up the lifetime
issues of private data with the chardev driver, and switch all chardev
drivers over to that.  The existing one is all very well, but it exposes
a large amount of the core kernel APIs and behaviour of those APIs to
driver authors, rather than making it Damned Simple for them.

It's not that easy right now due to the way struct file_operations behaves
- it's difficult to only intercept the ->open and ->release calls while
forwarding the rest to a child file_operations, especially when the
presence/absence of operations is tested for, eg:

        fn = no_llseek;
        if (file->f_mode & FMODE_LSEEK) {
                if (file->f_op->llseek)
                        fn = file->f_op->llseek;
        }
        return fn(file, offset, whence);

and:

        if (filp->f_op->flock)
                filp->f_op->flock(filp, F_SETLKW, &fl);
        else
                flock_lock_file(filp, &fl);

We have lots of drivers trying to solve this issue of private data in
their own ways.  Some use a kref, others use other methods.  Isn't it
about time we had some kind of unification here to ease this burden on
driver authors?

We can have an argument about whether kernel driver authors should know
about the fine details of lifetime issues which many kernel subsystem
maintainers themselves struggle to grasp, and whether they should therefore
be coding for the kernel at all, or whether we should be making it easy
for driver authors to get things right.

Personally, I'd prefer the latter - the simpler the interfaces, the easier
it is for everyone to get stuff correct, and the less obscure bugs there
will be.

Okay, here's a challenge to everyone involved in this thread.  Write a
simple character device driver which attaches to a platform device, where
the character device driver uses private data allocated in the platform
device probe method.  The private data is to contain a string "Hello
World!" initialised by the probe method, and a copy of this string will
be returned by the file_operations read() method according to the offset
in the virtual "file".  Don't use devm_* APIs.  Private data must be
correctly cleaned up.

Let's see how many can come up with correct code. :)

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

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

[Guenter Roeck]

On 08/02/2015 07:30 AM, Russell King - ARM Linux wrote:
> On Sat, Aug 01, 2015 at 05:48:30PM -0700, Guenter Roeck wrote:
>> On 08/01/2015 08:18 AM, Tejun Heo wrote:
>>> So, all in all, if we actually want to fix this issue, well at least
>>> most of it, I think the only viable way would be implementing revoke
>>> semantics and drain and sever all operations on removal.  Maybe it'll
>>> end up another devm interface.
>>
>> Maybe that is completely nonsense, but how about something like
>> 	devm_get(dev);
>> to be called whenever a file used by a device is opened, and
>> 	devm_put(dev);
>> whenever it is closed ?
>
> Absolutely not.  There's two problems there:
>
> 1) You'd also need to tie the lifetime of 'dev' to the lifetime of the
>     character device, which is getting to be an absurd level of lifetime
>     complexity - we can't have 'dev' vanishing before the last user of
>     the file operations has gone away, otherwise 'dev' becomes a stale
>     pointer.
>
Wonder how many drivers access the device data structure in file operations.

> 2) Remember that devm encompasses not only driver allocations, but also
>     resources as well, like interrupts, iomem mappings and iomem resource
>     claiming.  As has already been stated by others, we don't want to delay
>     the release of all these resources.
>
Again, I wonder how many drivers access io mappings and resources in file
operations.

I understand that my idea would tie the lifetime of 'dev' to the lifetime
of a character device; I guess that was the idea. But what is the
alternative ? Also, in practice, doesn't that dependency already exist
in many cases ?

Thanks,
Guenter

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

[Laurent Pinchart]

Hi Russell,

On Saturday 01 August 2015 17:30:48 Russell King - ARM Linux wrote:
> On Sat, Aug 01, 2015 at 01:55:54PM +0300, Laurent Pinchart wrote:
> > On Friday 31 July 2015 18:27:32 Russell King - ARM Linux wrote:
> > > I completely agree - this has *nothing* to do with devm_ at all.  Any
> > > bugs that are there as a result of converting to devm_kzalloc() where
> > > there before.  99.9% of the devm_kzalloc() conversions are merely
> > > replacing the kmalloc()/kzalloc() in the probe function with
> > > devm_kzalloc()
> > > and removing the free() in the remove function.
> > 
> > In those cases I agree that the bug is not introduced by the conversion.
> > However, as we're selling devm_kzalloc() as solving all problems (or, at
> > least, as many developers believe it will), many new drivers get submitted
> > with blind devm_kzalloc() usage without any thought about proper lifetime
> > management for objects allocated by the driver.
> 
> I don't think anyone has ever said that devm_* solves all problems.  What
> I've seen, it has been sold as avoiding problems with memory leaks in the
> failed probe path, and as a way to make resource management at driver
> unbind time easier.
> 
> I don't think it's ever been touted as "the worlds solution to hunger" or
> anything of the sort -

Maybe the message has been received differently by different developers then, 
or altered along the path.

> and as you've already confessed to being a hater of devm_*, I can only think
> that the reason you're soo inspired to want to connect devm_* with this
> problem is because you have an alterior motive.

What could have possibly given you that impression ? I've converted drivers to 
devm_* myself and regularly asked for new submissions to use devm_* during 
review.

> > This gets past (at least some) subsystem maintainers, showing that even
> > if the devm_kzalloc() API forbids this kind of usage, the message is not
> > conveyed properly.
> 
> Again, that's not the problem.  Empty release methods for kobjects get past
> subsystem maintainers too.  Some subsystem maintainers even create empty
> release methods.  It's all exactly the same problem: many kernel developers
> _and_ subsystem maintainers do not understand reference counted resource
> tracking - that's the _common_ issue here.
> 
> That's not necessarily the fault of the subsystem maintainers; if a
> subsystem maintainer is overloaded with work, or tired out towards the end
> of the day, reviews aren't going to be as thorough as they should be, which
> leads to things being missed - and to properly analyse the lifetime of every
> structure in a driver submission is Hard.

I don't think anyone blamed them. What I'm interested in is how we can improve 
that situation.

> I know full well, that a lot of subsystem maintainers struggle with getting
> finding the basic review points, so expecting them to properly analyse
> structure lifetime is, I think, expecting too much.
> 
> Maybe a way forward on this should be that we decide that:
> 
> - any structure which gets passed into a driver open() method should be
>   kref-counted.
> - that structure should be allocated using standard kzalloc()
> - the structure should only ever be freed after the last release after
>   the driver has been unbound.
> 
> It sounds like there is scope there for set of "driver data" helpers,
> maybe something like:
> 
> struct drvdata {
> 	struct kref kref;
> 	u64 data[0];
> };
> 
> static void *drvdata_fops_data_alloc(size_t size)
> {
> 	struct drvdata *d;
> 
> 	d = kzalloc(sizeof(*d) + size, GFP_KERNEL);
> 	if (!d)
> 		return NULL;
> 
> 	kref_init(&d->kref);
> 	d->real_fops = fops;
> 	return d->data;
> }
> 
> static void drvdata_fops_release_data(struct kref *kref)
> {
> 	struct drvdata *d = container_of(kref, struct drvdata, kref);
> 	kfree(d);
> }
> 
> void drvdata_fops_put(void *drvdata)
> {
> 	struct drvdata *d = container_of(drvdata, struct drvdata, data);
> 	kref_put(&d->kref, drvdata_fops_release_data);
> }
> 
> void *drvdata_fops_open(struct file *file)
> {
> 	struct drvdata *d = container_of(file->private_data, struct drvdata, 
data);
> 
> 	kref_get(&d->kref);
> 
> 	return d->data;
> }
> 
> void drvdata_fops_release(struct file *file)
> {
> 	drvdata_fops_put(file->private_data);
> }
> 
> The idea being that drvdata_fops_data_alloc() gets called in the driver
> probe function, with drvdata_fops_put() in the release function.  Both
> of these _could_ have devm_* equivalents to ease driver authors burden
> when it comes to error cleanup in the probe.

I had something similar in mind, but with a callback to the driver for 
drvdata_fops_release_data(), to allow for other custom cleanup to be performed 
when it's time to turn the light off.

The way to tie this with file open/release also needs to be a bit more 
complex, as not all drivers store the main private data pointer in file-
>private_data. Some subsystems also hijack file->private_data, so it's not 
always usable even when no per-open data needs to be allocated and managed.

> To get at the driver data, a driver would call drvdata_fops_open() in
> their fops open() method, and drvdata_fops_release() in their fops
> release() method - it would be nice to hide that from drivers so they
> don't have to think about it once they opt-in to this management, though
> that makes it too much of a framework rather than a library.
> 
> We can make sure that happens if we offset file->private_data with a
> (random?  build-time?) cookie to catch anyone who thinks they can
> dereference it directly.
> 
> > > Well, accessing hardware is even more of a problem.  Consider that
> > > ioremap()s will also be cleaned up at the same time (whether in
> > > ->remove() or in devm cleanup processing - again, not a devm problem)
> > > thereby removing the mapping for accessing the hardware.
> > 
> > Drivers are usually in a bit of a better shape when it comes to hardware
> > access. Most developers understand that the remove() function is supposed
> > to stop the hardware,
> 
> I think you missed the point.
> 
> What if a file-operations method provokes some kind of hardware access.
> For example, the read() method dereferences the driver data (which has
> been freed), obtains a stale pointer to where the hardware was mapped,
> and then tries to read a status register to check whether any data has
> become available.
>
> The result is, of course, an oops because the io memory has been freed by
> the devm_ioremap_resource() cleanup.
> 
> What I'm trying to point out where is that this is absolutely no different
> from the devm_* problem you're complaining about - exactly the same issues
> exist with _any_ accessible resource which is shared between the driver
> lifetime and the file-operations lifetime.
> 
> That, again, has absolutely nothing to do with the "evil devm" stuff.  We
> have _always_ cleaned up such resources in the driver unbind path with an
> iounmap(), and I bet that most of these are buggy in almost all drivers
> which also access this memory region via the file-operations methods.

It really depends on the subsystem (and of course on individual drivers). V4L2 
for instance prevents file operation calls from reaching the driver after 
video devices have been unregistered, which happens at remove() time. The 
driver will then have no occasion to access ioremap'ed regions. This can't be 
extended to solve the driver allocate data problem, as the unregistration 
check performed in the V4L2 core uses the video device structure, which must 
thus be kept until after the last file handle is closed.

I haven't performed a survey of other subsystems to see how they deal with 
device removal, but I assume that some level of assistance to drivers should 
be possible.

> So, I think you ought to get off the "devm is evil" bandwagon, and realise
> that the problem you have raised has absolutely nothing at all to do with
> devm_* but is much more of a general "people don't understand resource
> lifetime issues" problem.

Once again I've never meant to imply that devm is evil. My only point is that 
we have a life time management issue that is often mistakenly assumed to be 
handled by devm, and I'd like to find the best way to get out of that 
situation.

> Even I will hold my hand up to having written buggy drivers with exactly
> the kind of issues raised in this thread (I'm looking at one right now,
> though not in mainline...)

Let's not hold a contest of who has written the most code that doesn't handle 
life time properly, I may win ;-)

> Here's an example of a driver which is buggy as we've been describing
> here, but does not use devm*: drivers/uio/uio_pruss.c.  In pruss_cleanup(),
> it kfree()s the array of struct uio_info structures, which are used by
> drivers/uio/uio.c.  This structure can be accessed via the mmap(), write(),
> release(), poll(), etc file-operations methods.  None of them would be
> safe if the driver has been unbound with the chardev open - and it'd be
> dangerous to close the port once the driver had been unbound.

-- 
Regards,

Laurent Pinchart

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

[Daniel Vetter]

On Sun, Aug 2, 2015 at 4:30 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Sat, Aug 01, 2015 at 05:48:30PM -0700, Guenter Roeck wrote:
>> On 08/01/2015 08:18 AM, Tejun Heo wrote:
>> >So, all in all, if we actually want to fix this issue, well at least
>> >most of it, I think the only viable way would be implementing revoke
>> >semantics and drain and sever all operations on removal.  Maybe it'll
>> >end up another devm interface.
>>
>> Maybe that is completely nonsense, but how about something like
>>       devm_get(dev);
>> to be called whenever a file used by a device is opened, and
>>       devm_put(dev);
>> whenever it is closed ?
>
> Absolutely not.  There's two problems there:
>
> 1) You'd also need to tie the lifetime of 'dev' to the lifetime of the
>    character device, which is getting to be an absurd level of lifetime
>    complexity - we can't have 'dev' vanishing before the last user of
>    the file operations has gone away, otherwise 'dev' becomes a stale
>    pointer.
>
> 2) Remember that devm encompasses not only driver allocations, but also
>    resources as well, like interrupts, iomem mappings and iomem resource
>    claiming.  As has already been stated by others, we don't want to delay
>    the release of all these resources.
>
> I think a proposition would be to come up with some kind of driver
> interface for things like chardev drivers which wraps up the lifetime
> issues of private data with the chardev driver, and switch all chardev
> drivers over to that.  The existing one is all very well, but it exposes
> a large amount of the core kernel APIs and behaviour of those APIs to
> driver authors, rather than making it Damned Simple for them.
>
> It's not that easy right now due to the way struct file_operations behaves
> - it's difficult to only intercept the ->open and ->release calls while
> forwarding the rest to a child file_operations, especially when the
> presence/absence of operations is tested for, eg:
>
>         fn = no_llseek;
>         if (file->f_mode & FMODE_LSEEK) {
>                 if (file->f_op->llseek)
>                         fn = file->f_op->llseek;
>         }
>         return fn(file, offset, whence);
>
> and:
>
>         if (filp->f_op->flock)
>                 filp->f_op->flock(filp, F_SETLKW, &fl);
>         else
>                 flock_lock_file(filp, &fl);
>
> We have lots of drivers trying to solve this issue of private data in
> their own ways.  Some use a kref, others use other methods.  Isn't it
> about time we had some kind of unification here to ease this burden on
> driver authors?
>
> We can have an argument about whether kernel driver authors should know
> about the fine details of lifetime issues which many kernel subsystem
> maintainers themselves struggle to grasp, and whether they should therefore
> be coding for the kernel at all, or whether we should be making it easy
> for driver authors to get things right.
>
> Personally, I'd prefer the latter - the simpler the interfaces, the easier
> it is for everyone to get stuff correct, and the less obscure bugs there
> will be.
>
> Okay, here's a challenge to everyone involved in this thread.  Write a
> simple character device driver which attaches to a platform device, where
> the character device driver uses private data allocated in the platform
> device probe method.  The private data is to contain a string "Hello
> World!" initialised by the probe method, and a copy of this string will
> be returned by the file_operations read() method according to the offset
> in the virtual "file".  Don't use devm_* APIs.  Private data must be
> correctly cleaned up.
>
> Let's see how many can come up with correct code. :)

I'd love to see generic support to revoke/block anything coming in
through a file_ops. Imo it' should't be just chardev but also
sysfs/debugfs/whatever else since afaiui just unregister those files
doesn't make sure that they're all closed already again. I'm very much
aware that all drm drivers are completely broken in this regard and we
started trying to fix things up a bit but honestly doing this for real
is _very_ low on my list of things to fix. There's a lot more things
that blow up daily and really annoy users, whereas hotplug seems
fairly ok with a "hope it works" approach. Mostly it's just used by
developers anyway and for that case just write scripts to stop
everything that might access your driver. Needs some work every time
you update to shut off the latest thing systemd adds, but a lot less
effort than fixing up the drivers themselves ;-)

Unfortunately I don't have the vfs knowledge to create something generic :(
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Takashi Iwai]

On Sun, 02 Aug 2015 16:30:25 +0200,
Russell King - ARM Linux wrote:
> 
> We have lots of drivers trying to solve this issue of private data in
> their own ways.  Some use a kref, others use other methods.  Isn't it
> about time we had some kind of unification here to ease this burden on
> driver authors?
> 
> We can have an argument about whether kernel driver authors should know
> about the fine details of lifetime issues which many kernel subsystem
> maintainers themselves struggle to grasp, and whether they should therefore
> be coding for the kernel at all, or whether we should be making it easy
> for driver authors to get things right.
> 
> Personally, I'd prefer the latter - the simpler the interfaces, the easier
> it is for everyone to get stuff correct, and the less obscure bugs there
> will be.

I bet majority of maintainers would love the latter, too.
After all, the simplicity is the reason why devm_* got more
attraction.


thanks,

Takashi

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

[Russell King - ARM Linux]

On Tue, Aug 04, 2015 at 12:40:00PM +0200, Daniel Vetter wrote:
> I'd love to see generic support to revoke/block anything coming in
> through a file_ops.

Yes, I've been looking at one of my drivers which is definitely broken
in this regard, and that's the conclusion I'm coming to - it's not a
problem of lifetime control of the private driver data structures, but
it's a much bigger issue than that.

If a driver's file_operations methods touch hardware, then we need a
way to prevent those file_operations methods from touching the hardware
once the device driver's ->remove callback has been called.

Yes, we can stick a flag in the driver private data structure to
indicate that it's gone, but do we want to keep on reviewing this in
driver code, or should we make it easier for driver authors, subsystem
authors and core reviewers to get this right by providing a "revoke"
which can be called in the ->remove callback?

However, providing a revoke is not easy: if we look at the tty code,
which has to deal with tty hangups, then we find that it does this:

        spin_lock(&tty_files_lock);
        /* This breaks for file handles being sent over AF_UNIX sockets ? */
        list_for_each_entry(priv, &tty->tty_files, list) {
                filp = priv->file;
                if (filp->f_op->write == redirected_tty_write)
                        cons_filp = filp;
                if (filp->f_op->write != tty_write)
                        continue;
                closecount++;
                __tty_fasync(-1, filp, 0);      /* can't block */
                filp->f_op = &hung_up_tty_fops;
        }
        spin_unlock(&tty_files_lock);

IOW, it replaces the f_op on all files with hung_up_tty_fops.

Now, the filp is protected from going away by being on the list: a
f_op->release will block in tty_del_file() for the above lock to be
released.  That much is safe.

However, what if another thread/cpu is in one of the f_op methods when
filp->f_op is replaced with the "revoked" operations?  What I'm saying
is that there's no guarantee that one of the file's operations won't
be executing on another CPU at the point that we're trying to revoke it,
and we don't want any threads to be there, because they could access
data we're about to free.

That's the difficult part of the problem, and it's one which things like
stop_machine() doesn't solve (since we may have scheduled away from
a method to run the stop_machine() thread.)  We can't wait for the file
to close, because that introduces soft-deadlocks - consider a thread
which holds a reference on the filp, but is trying to unbind the driver
via sysfs... just like the sysfs issues we've had in the past.

Unfortunately, this is where things are much more complex than the module
case - the module has has the advantage that:

rmmod chardev-driver < /dev/that-chardev-driver-node

can return saying "module busy".  We don't have that luxury when it comes
to removing 'struct device' - the driver ->remove callback has no way to
fail (it's return code is always ignored.)

A solution to that would be to drop something like a read-write lock into
almost all f_op methods, which sounds expensive to me in the general case.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

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

[Daniel Vetter]

On Tue, Aug 4, 2015 at 1:18 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> A solution to that would be to drop something like a read-write lock into
> almost all f_op methods, which sounds expensive to me in the general case.

srcu is what I considered since it would be least intrusive and shifts
the overall all to the write. The problem of course is that if you do
that then there will be deadlock gallore - suddenly anything called
from f->ops can stall code called from ->remove. And looking at how
regularly we have lockdep splat in the driver unload code just in i915
that will be really painful.

But I don't see anything else that would work and which would be
semantically different from a reader/writer lock. There's an
additional problem that we need to guarantee that everyone completes
f->ops in finite time, which is a problem if you have blockings
ioctls. And that's a deadlock lockdep won't catch (in general at
least). For i915 that won't be a problem since because of the gpu
reset all our waiting is done interruptibly and all ioctls can be
restarted (userspace has to do it, it's part of the drm abi contract).
But even for drivers who can't do that and might deadlock I think a
deadlock in ->remove is better than randomly oopsing somewhere later
on because some f->ops is accessing freed memory.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Daniel Vetter]

Oh and it's not just file->ops, it's also vma->ops, dma_buf->ops,
fence->ops and anything else that could be shared with other parts of
the kernel. But I think file->ops is a good first candidate since it's
the most widespread thing, so should uncover a lot of the common
troubles. And a lot of the bugs will be fixed by just taking care of
file->ops.
-Daniel


On Tue, Aug 4, 2015 at 1:56 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> On Tue, Aug 4, 2015 at 1:18 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>> A solution to that would be to drop something like a read-write lock into
>> almost all f_op methods, which sounds expensive to me in the general case.
>
> srcu is what I considered since it would be least intrusive and shifts
> the overall all to the write. The problem of course is that if you do
> that then there will be deadlock gallore - suddenly anything called
> from f->ops can stall code called from ->remove. And looking at how
> regularly we have lockdep splat in the driver unload code just in i915
> that will be really painful.
>
> But I don't see anything else that would work and which would be
> semantically different from a reader/writer lock. There's an
> additional problem that we need to guarantee that everyone completes
> f->ops in finite time, which is a problem if you have blockings
> ioctls. And that's a deadlock lockdep won't catch (in general at
> least). For i915 that won't be a problem since because of the gpu
> reset all our waiting is done interruptibly and all ioctls can be
> restarted (userspace has to do it, it's part of the drm abi contract).
> But even for drivers who can't do that and might deadlock I think a
> deadlock in ->remove is better than randomly oopsing somewhere later
> on because some f->ops is accessing freed memory.
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Dan Carpenter]

On Sat, Aug 01, 2015 at 01:21:09PM +0200, Julia Lawall wrote:
> Currently, is there any documentation about when these functions can be
> used?  All I remember seeing is a discussion of what the functions do and
> what functions are available.  But nothing about when they should and
> should not be used.
> 

The documentation for devm_kmalloc() doesn't list common pitfalls.  The
other big one which should be obvious, but happens often is freeing
devm_ memory with kfree().

/**
 * devm_kmalloc - Resource-managed kmalloc
 * @dev: Device to allocate memory for
 * @size: Allocation size
 * @gfp: Allocation gfp flags
 *
 * Managed kmalloc.  Memory allocated with this function is
 * automatically freed on driver detach.  Like all other devres
 * resources, guaranteed alignment is unsigned long long.
 *
 * RETURNS:
 * Pointer to allocated memory on success, NULL on failure.
 */

Another thing which I have seems fairly common is treating it like
kmalloc() and calling devm_kfree() manually.  For example, ssp_parse_dt()

drivers/iio/common/ssp_sensors/ssp_dev.c
   493          return data;
   494  
   495  err_mcu_reset_gpio:
   496          devm_gpio_free(dev, data->mcu_reset_gpio);
   497  err_ap_mcu:
   498          devm_gpio_free(dev, data->ap_mcu_gpio);
   499  err_free_pd:
   500          devm_kfree(dev, data);
   501          return NULL;
   502  }

regards,
dan carpenter

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

[Geert Uytterhoeven]

On Tue, Aug 4, 2015 at 2:55 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> Another thing which I have seems fairly common is treating it like
> kmalloc() and calling devm_kfree() manually.  For example, ssp_parse_dt()
>
> drivers/iio/common/ssp_sensors/ssp_dev.c
>    493          return data;
>    494
>    495  err_mcu_reset_gpio:
>    496          devm_gpio_free(dev, data->mcu_reset_gpio);
>    497  err_ap_mcu:
>    498          devm_gpio_free(dev, data->ap_mcu_gpio);
>    499  err_free_pd:
>    500          devm_kfree(dev, data);
>    501          return NULL;
>    502  }

These indeed look superfluous.

In some cases it may legitimate, though, when freeing memory not tied directly
to the lifetime of the device.
Cfr. drivers/media/platform/soc_camera/soc_camera.c, where the operation
may be retried later.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Tejun Heo]

Hello,

On Tue, Aug 04, 2015 at 01:56:38PM +0200, Daniel Vetter wrote:
> On Tue, Aug 4, 2015 at 1:18 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > A solution to that would be to drop something like a read-write lock into
> > almost all f_op methods, which sounds expensive to me in the general case.
> 
> srcu is what I considered since it would be least intrusive and shifts
> the overall all to the write. The problem of course is that if you do

percpu_ref is likely a better fit.

> that then there will be deadlock gallore - suddenly anything called
> from f->ops can stall code called from ->remove. And looking at how
> regularly we have lockdep splat in the driver unload code just in i915
> that will be really painful.

It's the other side of the same coin.  At least it's a lot easier to
detect with a lot more deterministic failure mode than derefing freed
memory or accessing gone hardware.

Thanks.

-- 
tejun

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

[Dmitry Torokhov]

[-- Attachment #1: Type: text/plain, Size: 1485 bytes --]

On Tue, Aug 4, 2015 at 5:55 AM, Dan Carpenter <dan.carpenter@oracle.com>
wrote:

> On Sat, Aug 01, 2015 at 01:21:09PM +0200, Julia Lawall wrote:
> > Currently, is there any documentation about when these functions can be
> > used?  All I remember seeing is a discussion of what the functions do and
> > what functions are available.  But nothing about when they should and
> > should not be used.
> >
>
> The documentation for devm_kmalloc() doesn't list common pitfalls.  The
> other big one which should be obvious, but happens often is freeing
> devm_ memory with kfree().
>
> /**
>  * devm_kmalloc - Resource-managed kmalloc
>  * @dev: Device to allocate memory for
>  * @size: Allocation size
>  * @gfp: Allocation gfp flags
>  *
>  * Managed kmalloc.  Memory allocated with this function is
>  * automatically freed on driver detach.  Like all other devres
>  * resources, guaranteed alignment is unsigned long long.
>  *
>  * RETURNS:
>  * Pointer to allocated memory on success, NULL on failure.
>  */
>

I wonder if it would be possible to note that the current thread is going
through probe() or remove() code path in driver core and scream if we
encounter devm* call outside of such path. That will give false positives
in cases when there is a legitimate mix of automatic and manual resource
management (i.e. you rely on automatic cleanup in probe()/remove() but need
to free/reallocate object somewhere else), but we can create another call
for that.

Thanks.

-- 
Dmitry

[-- Attachment #2: Type: text/html, Size: 2001 bytes --]

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

[Julia Lawall]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1888 bytes --]

On Tue, 4 Aug 2015, Dmitry Torokhov wrote:

>
>
> On Tue, Aug 4, 2015 at 5:55 AM, Dan Carpenter <dan.carpenter@oracle.com>
> wrote:
>       On Sat, Aug 01, 2015 at 01:21:09PM +0200, Julia Lawall wrote:
>       > Currently, is there any documentation about when these
>       functions can be
>       > used?  All I remember seeing is a discussion of what the
>       functions do and
>       > what functions are available.  But nothing about when they
>       should and
>       > should not be used.
>       >
>
>       The documentation for devm_kmalloc() doesn't list common
>       pitfalls.  The
>       other big one which should be obvious, but happens often is
>       freeing
>       devm_ memory with kfree().
>
>       /**
>        * devm_kmalloc - Resource-managed kmalloc
>        * @dev: Device to allocate memory for
>        * @size: Allocation size
>        * @gfp: Allocation gfp flags
>        *
>        * Managed kmalloc.  Memory allocated with this function is
>        * automatically freed on driver detach.  Like all other devres
>        * resources, guaranteed alignment is unsigned long long.
>        *
>        * RETURNS:
>        * Pointer to allocated memory on success, NULL on failure.
>        */
>
>  
> I wonder if it would be possible to note that the current thread is going
> through probe() or remove() code path in driver core and scream if we
> encounter devm* call outside of such path. That will give false positives in
> cases when there is a legitimate mix of automatic and manual resource
> management (i.e. you rely on automatic cleanup in probe()/remove() but need
> to free/reallocate object somewhere else), but we can create another call
> for that.

This seems easy to do with Coccinelle, at least with respect to the
functions in a single file.  I guess it doesn't solve the file operations
problem, though?

julia

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

[Dmitry Torokhov]

[-- Attachment #1: Type: text/plain, Size: 2263 bytes --]

On Tue, Aug 4, 2015 at 11:03 AM, Julia Lawall <julia.lawall@lip6.fr> wrote:

> On Tue, 4 Aug 2015, Dmitry Torokhov wrote:
>
> >
> >
> > On Tue, Aug 4, 2015 at 5:55 AM, Dan Carpenter <dan.carpenter@oracle.com>
> > wrote:
> >       On Sat, Aug 01, 2015 at 01:21:09PM +0200, Julia Lawall wrote:
> >       > Currently, is there any documentation about when these
> >       functions can be
> >       > used?  All I remember seeing is a discussion of what the
> >       functions do and
> >       > what functions are available.  But nothing about when they
> >       should and
> >       > should not be used.
> >       >
> >
> >       The documentation for devm_kmalloc() doesn't list common
> >       pitfalls.  The
> >       other big one which should be obvious, but happens often is
> >       freeing
> >       devm_ memory with kfree().
> >
> >       /**
> >        * devm_kmalloc - Resource-managed kmalloc
> >        * @dev: Device to allocate memory for
> >        * @size: Allocation size
> >        * @gfp: Allocation gfp flags
> >        *
> >        * Managed kmalloc.  Memory allocated with this function is
> >        * automatically freed on driver detach.  Like all other devres
> >        * resources, guaranteed alignment is unsigned long long.
> >        *
> >        * RETURNS:
> >        * Pointer to allocated memory on success, NULL on failure.
> >        */
> >
> >
> > I wonder if it would be possible to note that the current thread is going
> > through probe() or remove() code path in driver core and scream if we
> > encounter devm* call outside of such path. That will give false
> positives in
> > cases when there is a legitimate mix of automatic and manual resource
> > management (i.e. you rely on automatic cleanup in probe()/remove() but
> need
> > to free/reallocate object somewhere else), but we can create another call
> > for that.
>
> This seems easy to do with Coccinelle, at least with respect to the
> functions in a single file.  I guess it doesn't solve the file operations
> problem, though?
>

No, it does not, but I've seen a few examples with devm (mostly
devm_k*alloc) used in wrong context and causing almost-memory-leaks (i.e.
they had a chance to be eventually cleaned up, but not really).

Thanks.

-- 
Dmitry

[-- Attachment #2: Type: text/html, Size: 3193 bytes --]

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

[Russell King - ARM Linux]

On Tue, Aug 04, 2015 at 10:55:35AM -0700, Dmitry Torokhov wrote:
> I wonder if it would be possible to note that the current thread is going
> through probe() or remove() code path in driver core and scream if we
> encounter devm* call outside of such path. That will give false positives
> in cases when there is a legitimate mix of automatic and manual resource
> management (i.e. you rely on automatic cleanup in probe()/remove() but need
> to free/reallocate object somewhere else), but we can create another call
> for that.

Well, it's not that easy, now that we have the component helper (sorry).

The "easy" bit to implement that idea would be to add a flag to struct
device which indicates whether this device is in an active probe()
function, and use that in the devm* functions to determine whether
to warn about incorrect usage.

However, the component helper messes that up slightly, because it allows
devm usage outside of a device's probe() function, though still in a
properly controlled manner.  (Resources claimed via devm_* in the bind()
call will be released in after the corresponding unbind() call.)  This
is done using the devres groups.

So, maybe instead of "is it inside a probe" a better check should be "is
the devm* resource claim being done in an active devres group or inside
a probe".  Not forgetting that a devres group can be removed, merging
the child resource group with the parent resource group (which are
probably about the same as for resource claiming using devm_*.)

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

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

[Laurent Pinchart]

Hi Daniel,

On Tuesday 04 August 2015 13:56:38 Daniel Vetter wrote:
> On Tue, Aug 4, 2015 at 1:18 PM, Russell King - ARM Linux wrote:
> > A solution to that would be to drop something like a read-write lock into
> > almost all f_op methods, which sounds expensive to me in the general case.
> 
> srcu is what I considered since it would be least intrusive and shifts
> the overall all to the write. The problem of course is that if you do
> that then there will be deadlock gallore - suddenly anything called
> from f->ops can stall code called from ->remove. And looking at how
> regularly we have lockdep splat in the driver unload code just in i915
> that will be really painful.
> 
> But I don't see anything else that would work and which would be
> semantically different from a reader/writer lock. There's an
> additional problem that we need to guarantee that everyone completes
> f->ops in finite time, which is a problem if you have blockings
> ioctls. And that's a deadlock lockdep won't catch (in general at
> least). For i915 that won't be a problem since because of the gpu
> reset all our waiting is done interruptibly and all ioctls can be
> restarted (userspace has to do it, it's part of the drm abi contract).
> But even for drivers who can't do that and might deadlock I think a
> deadlock in ->remove is better than randomly oopsing somewhere later
> on because some f->ops is accessing freed memory.

This seems subsystem-dependent. Looking at V4L2 for instance, we do have 
blocking ioctls, but drivers are expected to cancel all pending operations in 
the remove() handler, which will have the effect of waking up the waiters. It 
should thus be possible for a V4L2 driver to ensure in its remove() handler 
that

1. no new file operation can be called
2. all blocking file operations are woken up

There's however no current provision for ensuring that a non-blocking file 
operation completes before returning from the remove() handler.

A revoke semantics for file operations is tempting, but it might open a big 
can of worms. I wonder whether it wouldn't be possible to implement proper 
life time management in a simpler way that we do today without going for full 
synchronous revoke in remove().

-- 
Regards,

Laurent Pinchart

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

[Daniel Vetter]

On Wed, Aug 5, 2015 at 12:44 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Tuesday 04 August 2015 13:56:38 Daniel Vetter wrote:
>> On Tue, Aug 4, 2015 at 1:18 PM, Russell King - ARM Linux wrote:
>> > A solution to that would be to drop something like a read-write lock into
>> > almost all f_op methods, which sounds expensive to me in the general case.
>>
>> srcu is what I considered since it would be least intrusive and shifts
>> the overall all to the write. The problem of course is that if you do
>> that then there will be deadlock gallore - suddenly anything called
>> from f->ops can stall code called from ->remove. And looking at how
>> regularly we have lockdep splat in the driver unload code just in i915
>> that will be really painful.
>>
>> But I don't see anything else that would work and which would be
>> semantically different from a reader/writer lock. There's an
>> additional problem that we need to guarantee that everyone completes
>> f->ops in finite time, which is a problem if you have blockings
>> ioctls. And that's a deadlock lockdep won't catch (in general at
>> least). For i915 that won't be a problem since because of the gpu
>> reset all our waiting is done interruptibly and all ioctls can be
>> restarted (userspace has to do it, it's part of the drm abi contract).
>> But even for drivers who can't do that and might deadlock I think a
>> deadlock in ->remove is better than randomly oopsing somewhere later
>> on because some f->ops is accessing freed memory.
>
> This seems subsystem-dependent. Looking at V4L2 for instance, we do have
> blocking ioctls, but drivers are expected to cancel all pending operations in
> the remove() handler, which will have the effect of waking up the waiters. It
> should thus be possible for a V4L2 driver to ensure in its remove() handler
> that
>
> 1. no new file operation can be called
> 2. all blocking file operations are woken up
>
> There's however no current provision for ensuring that a non-blocking file
> operation completes before returning from the remove() handler.

Yeah what I meant to say is that revoke won't be a silver bullet, it
still needs some work from the driver to avoid deadlocks. But like I
said I think a deadlock is already an improvement over randomly
crashing, which is what we usually do today.

> A revoke semantics for file operations is tempting, but it might open a big
> can of worms. I wonder whether it wouldn't be possible to implement proper
> life time management in a simpler way that we do today without going for full
> synchronous revoke in remove().

The problem is that the device is gone, so somewhere you need to catch
calls and reject them. And besides trying to do it at the
kernel/userspace level with revoke we could do filters at the
subsystem level (drm tries to do something like that with the unplug
stuff) or even at the device level. Some of this might require big
reworks all over but I think it should all work.

The problem is that the deeper down in the stack we reject stuff for
unplugged devices the more risk there is that we blow up in some
untested error handling code. And not blowing up on unplug is the goal
and that's why I like to reject ops at the top with a generic revoke.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Linus Walleij]

On Fri, Jul 31, 2015 at 5:14 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:

> 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

I've encountered it a few times in review, not in practice, a
relevant part of the question is whether your driver really cannot
live without the bind/unbind attributes in sysfs. I've realized that
I don't use them, and I suspect that for a largeish set of drivers
the developers don't use it.

I've started to think that the .suppress_bind_attrs = true in
struct device_driver is often your friend, simplifying the world.

I've actually thought about trying to set that for *any* GPIO
driver if they enable the sysfs access to GPIOs as these
dangling userspace users is a common problem here, but since
we also want to have hotplug/unplug of these hosts it's maybe
a real bad idea, as these sysfs files are good for testing that.

Yours,
Linus Walleij

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

[Russell King - ARM Linux]

On Mon, Aug 10, 2015 at 09:58:25AM +0200, Linus Walleij wrote:
> I've encountered it a few times in review, not in practice, a
> relevant part of the question is whether your driver really cannot
> live without the bind/unbind attributes in sysfs. I've realized that
> I don't use them, and I suspect that for a largeish set of drivers
> the developers don't use it.

>From the development point of view, it's useful to be able to unbind/rebind
a driver after something has gone wrong as part of the debugging strategy,
especially if the driver is built into the kernel.

> I've actually thought about trying to set that for *any* GPIO
> driver if they enable the sysfs access to GPIOs as these
> dangling userspace users is a common problem here, but since
> we also want to have hotplug/unplug of these hosts it's maybe
> a real bad idea, as these sysfs files are good for testing that.

sysfs itself should be fine - the problem is what you do with sysfs.
I think sysfs's protection comes from kernfs's deactivation, tested by
kernfs_get_active() before passing any operation up.

So, when kobject_del() has been called, which calls sysfs_remove_dir()
and kernfs_remove(), this deactivates the file, and waits (via
kernfs_drain()) for the active users to go away.  Once that returns,
no further calls should occur via sysfs.

When you unexport a GPIO, you do this:

                put_device(dev);
                device_unregister(dev);

device_unregister() calls device_del() which in turn calls kobject_del().
So, by the time device_unregister() returns, you should no longer receive
any calls to your attributes - and this figures, because the sysfs/kernfs
code does not take a reference on the struct device anymore, so once
you drop all other references to the struct device, it's freed, and the
struct device which _would_ have been passed into your dev_attr code
would no longer be valid.

So, I think provided you properly delete kobjects/devices or unregister
the sysfs attributes prior to private data going away, there should not
be any use-after-del cases with sysfs attributes.

Having read the fs/kernfs, fs/sysfs, lib/kobject code this morning, I'm
now left wondering why people are saying that there's a problem here:
it seems there isn't if your driver removes any attributes in the
->remove path which it registered in the ->probe path - either by
deleting the attributes themselves, or by deleting the object they are
attached to.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

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

[Takashi Iwai]

On Mon, 10 Aug 2015 12:23:30 +0200,
Russell King - ARM Linux wrote:
> 
> On Mon, Aug 10, 2015 at 09:58:25AM +0200, Linus Walleij wrote:
> > I've encountered it a few times in review, not in practice, a
> > relevant part of the question is whether your driver really cannot
> > live without the bind/unbind attributes in sysfs. I've realized that
> > I don't use them, and I suspect that for a largeish set of drivers
> > the developers don't use it.
> 
> >From the development point of view, it's useful to be able to unbind/rebind
> a driver after something has gone wrong as part of the debugging strategy,
> especially if the driver is built into the kernel.
> 
> > I've actually thought about trying to set that for *any* GPIO
> > driver if they enable the sysfs access to GPIOs as these
> > dangling userspace users is a common problem here, but since
> > we also want to have hotplug/unplug of these hosts it's maybe
> > a real bad idea, as these sysfs files are good for testing that.
> 
> sysfs itself should be fine - the problem is what you do with sysfs.
> I think sysfs's protection comes from kernfs's deactivation, tested by
> kernfs_get_active() before passing any operation up.
> 
> So, when kobject_del() has been called, which calls sysfs_remove_dir()
> and kernfs_remove(), this deactivates the file, and waits (via
> kernfs_drain()) for the active users to go away.  Once that returns,
> no further calls should occur via sysfs.
> 
> When you unexport a GPIO, you do this:
> 
>                 put_device(dev);
>                 device_unregister(dev);
> 
> device_unregister() calls device_del() which in turn calls kobject_del().
> So, by the time device_unregister() returns, you should no longer receive
> any calls to your attributes - and this figures, because the sysfs/kernfs
> code does not take a reference on the struct device anymore, so once
> you drop all other references to the struct device, it's freed, and the
> struct device which _would_ have been passed into your dev_attr code
> would no longer be valid.
> 
> So, I think provided you properly delete kobjects/devices or unregister
> the sysfs attributes prior to private data going away, there should not
> be any use-after-del cases with sysfs attributes.
> 
> Having read the fs/kernfs, fs/sysfs, lib/kobject code this morning, I'm
> now left wondering why people are saying that there's a problem here:
> it seems there isn't if your driver removes any attributes in the
> ->remove path which it registered in the ->probe path - either by
> deleting the attributes themselves, or by deleting the object they are
> attached to.

I thought that the original problem is about the normal device file
accesses.  So, if we have a drain function for normal device files, it
should work well like sysfs/kernfs, too.

Some subsystems have their own ways to do a similar thing, but a
generic framework wound be nice to have, IMO.


thanks,

Takashi

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

[Daniel Vetter]

On Tue, Aug 11, 2015 at 1:35 PM, Takashi Iwai <tiwai@suse.de> wrote:
> On Mon, 10 Aug 2015 12:23:30 +0200,
> Russell King - ARM Linux wrote:
>>
>> On Mon, Aug 10, 2015 at 09:58:25AM +0200, Linus Walleij wrote:
>> > I've encountered it a few times in review, not in practice, a
>> > relevant part of the question is whether your driver really cannot
>> > live without the bind/unbind attributes in sysfs. I've realized that
>> > I don't use them, and I suspect that for a largeish set of drivers
>> > the developers don't use it.
>>
>> >From the development point of view, it's useful to be able to unbind/rebind
>> a driver after something has gone wrong as part of the debugging strategy,
>> especially if the driver is built into the kernel.
>>
>> > I've actually thought about trying to set that for *any* GPIO
>> > driver if they enable the sysfs access to GPIOs as these
>> > dangling userspace users is a common problem here, but since
>> > we also want to have hotplug/unplug of these hosts it's maybe
>> > a real bad idea, as these sysfs files are good for testing that.
>>
>> sysfs itself should be fine - the problem is what you do with sysfs.
>> I think sysfs's protection comes from kernfs's deactivation, tested by
>> kernfs_get_active() before passing any operation up.
>>
>> So, when kobject_del() has been called, which calls sysfs_remove_dir()
>> and kernfs_remove(), this deactivates the file, and waits (via
>> kernfs_drain()) for the active users to go away.  Once that returns,
>> no further calls should occur via sysfs.
>>
>> When you unexport a GPIO, you do this:
>>
>>                 put_device(dev);
>>                 device_unregister(dev);
>>
>> device_unregister() calls device_del() which in turn calls kobject_del().
>> So, by the time device_unregister() returns, you should no longer receive
>> any calls to your attributes - and this figures, because the sysfs/kernfs
>> code does not take a reference on the struct device anymore, so once
>> you drop all other references to the struct device, it's freed, and the
>> struct device which _would_ have been passed into your dev_attr code
>> would no longer be valid.
>>
>> So, I think provided you properly delete kobjects/devices or unregister
>> the sysfs attributes prior to private data going away, there should not
>> be any use-after-del cases with sysfs attributes.
>>
>> Having read the fs/kernfs, fs/sysfs, lib/kobject code this morning, I'm
>> now left wondering why people are saying that there's a problem here:
>> it seems there isn't if your driver removes any attributes in the
>> ->remove path which it registered in the ->probe path - either by
>> deleting the attributes themselves, or by deleting the object they are
>> attached to.
>
> I thought that the original problem is about the normal device file
> accesses.  So, if we have a drain function for normal device files, it
> should work well like sysfs/kernfs, too.
>
> Some subsystems have their own ways to do a similar thing, but a
> generic framework wound be nice to have, IMO.

I do remember drm drivers blowing up on sysfs reads of connector
status files. Might just be a problem with drm unload sequence being
terminally broken wrt correct ordering, I never looked too much into
it really since the real one is /dev char node access. There's also
other problems in drm driver load/unload which makes fixing this all
more complicated than necessary.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Dmitry Torokhov]

On Fri, Jul 31, 2015 at 8:14 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> 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.

<skip>

Just realized another fun fact: most busses execute
dev_pm_domain_detach() in bus' remove() method, but devm-managed
resources (including interrupts and such) are only released in driver
core, which happens after bus' remove() method returns. So we may end
up shutting off PM domain of a device that is still active and may
generate interrupts, etc.

Fun, huh?

Thanks.

-- 
Dmitry

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

[Julia Lawall]

On Thu, 20 Aug 2015, Dmitry Torokhov wrote:

> On Fri, Jul 31, 2015 at 8:14 AM, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> 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.
>
> <skip>
>
> Just realized another fun fact: most busses execute
> dev_pm_domain_detach() in bus' remove() method, but devm-managed
> resources (including interrupts and such) are only released in driver
> core, which happens after bus' remove() method returns. So we may end
> up shutting off PM domain of a device that is still active and may
> generate interrupts, etc.

Can the PM call be devmified too?

What is there too the "etc"?  Would it be better to just remove the devm
functions related to interrupts?  They seem to cause a lot of subtle
problems.

julia

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

[Dmitry Torokhov]

On Fri, Aug 21, 2015 at 08:07:09AM -0700, Julia Lawall wrote:
> On Thu, 20 Aug 2015, Dmitry Torokhov wrote:
> 
> > On Fri, Jul 31, 2015 at 8:14 AM, Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> 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.
> >
> > <skip>
> >
> > Just realized another fun fact: most busses execute
> > dev_pm_domain_detach() in bus' remove() method, but devm-managed
> > resources (including interrupts and such) are only released in driver
> > core, which happens after bus' remove() method returns. So we may end
> > up shutting off PM domain of a device that is still active and may
> > generate interrupts, etc.
> 
> Can the PM call be devmified too?

I think they have to. I think everything in bus(es) code has to be
devm-ified if we want to allow devm in drivers.

> 
> What is there too the "etc"?  Would it be better to just remove the devm
> functions related to interrupts?  They seem to cause a lot of subtle
> problems.

And regulators, clocks, gpios... Even if we won't have interrupt coming
in someone may submit request to the device (userspace for example) and
we still may try accessing powered off or half-powered-off hardware.

Thanks.

-- 
Dmitry

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 880 bytes --]

On Fri, Aug 21, 2015 at 09:14:09AM -0700, Dmitry Torokhov wrote:
> On Fri, Aug 21, 2015 at 08:07:09AM -0700, Julia Lawall wrote:

> > What is there too the "etc"?  Would it be better to just remove the devm
> > functions related to interrupts?  They seem to cause a lot of subtle
> > problems.

> And regulators, clocks, gpios... Even if we won't have interrupt coming
> in someone may submit request to the device (userspace for example) and
> we still may try accessing powered off or half-powered-off hardware.

For most of those there's no issue with devm - we're just releasing a
reference that allows us to control the device, not doing anything that
affects the state of the device.  We don't have managed functions for
the state changing operations.  Interrupts are different here in that
when we free we are also performing the operation that stops the
interrupt firing.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Dmitry Torokhov]

On Fri, Aug 21, 2015 at 9:58 AM, Mark Brown <broonie@kernel.org> wrote:
> On Fri, Aug 21, 2015 at 09:14:09AM -0700, Dmitry Torokhov wrote:
>> On Fri, Aug 21, 2015 at 08:07:09AM -0700, Julia Lawall wrote:
>
>> > What is there too the "etc"?  Would it be better to just remove the devm
>> > functions related to interrupts?  They seem to cause a lot of subtle
>> > problems.
>
>> And regulators, clocks, gpios... Even if we won't have interrupt coming
>> in someone may submit request to the device (userspace for example) and
>> we still may try accessing powered off or half-powered-off hardware.
>
> For most of those there's no issue with devm - we're just releasing a
> reference that allows us to control the device, not doing anything that
> affects the state of the device.  We don't have managed functions for
> the state changing operations.

That is true (at least for now), but there is a demand for adding
them. In the meantime they are quite often invoked via a custom devm
action.

They are also often wrapped into other objects. For example input
device might turn off regulators/clocks in it's close() method, which
is called as part of input_unregister_device(). If input device is
managed then that state change happens "some time later".

Thanks.

-- 
Dmitry

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1058 bytes --]

On Fri, Aug 21, 2015 at 10:30:17AM -0700, Dmitry Torokhov wrote:
> On Fri, Aug 21, 2015 at 9:58 AM, Mark Brown <broonie@kernel.org> wrote:

> > For most of those there's no issue with devm - we're just releasing a
> > reference that allows us to control the device, not doing anything that
> > affects the state of the device.  We don't have managed functions for
> > the state changing operations.

> That is true (at least for now), but there is a demand for adding
> them. In the meantime they are quite often invoked via a custom devm
> action.

At least for regulator I'm seeing very occasional requests for this but
they're really not very strong.

> They are also often wrapped into other objects. For example input
> device might turn off regulators/clocks in it's close() method, which
> is called as part of input_unregister_device(). If input device is
> managed then that state change happens "some time later".

Right, but my understanding is that the integration with devm with the
object reference counting was intended to do the right thing.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 733 bytes --]

On Fri, Aug 21, 2015 at 10:41:50AM -0700, Mark Brown wrote:
> On Fri, Aug 21, 2015 at 10:30:17AM -0700, Dmitry Torokhov wrote:

> > They are also often wrapped into other objects. For example input
> > device might turn off regulators/clocks in it's close() method, which
> > is called as part of input_unregister_device(). If input device is
> > managed then that state change happens "some time later".

> Right, but my understanding is that the integration with devm with the
> object reference counting was intended to do the right thing.

More specifically: the managed operations are guaranteed to be run in
reverse order so so long as the resources used by the managed input
device are also managed we're supposed to be fine.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Dmitry Torokhov]

On Fri, Aug 21, 2015 at 10:52 AM, Mark Brown <broonie@kernel.org> wrote:
> On Fri, Aug 21, 2015 at 10:41:50AM -0700, Mark Brown wrote:
>> On Fri, Aug 21, 2015 at 10:30:17AM -0700, Dmitry Torokhov wrote:
>
>> > They are also often wrapped into other objects. For example input
>> > device might turn off regulators/clocks in it's close() method, which
>> > is called as part of input_unregister_device(). If input device is
>> > managed then that state change happens "some time later".
>
>> Right, but my understanding is that the integration with devm with the
>> object reference counting was intended to do the right thing.
>
> More specifically: the managed operations are guaranteed to be run in
> reverse order so so long as the resources used by the managed input
> device are also managed we're supposed to be fine.

That only works if _all_ resources are managed. Here we have an
example of non-managed operations (such as detaching device from a
power domain) in the middle of managed ones. And so the ordering gets
broken.

Thanks.

-- 
Dmitry

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 739 bytes --]

On Fri, Aug 21, 2015 at 11:05:19AM -0700, Dmitry Torokhov wrote:
> On Fri, Aug 21, 2015 at 10:52 AM, Mark Brown <broonie@kernel.org> wrote:

> > More specifically: the managed operations are guaranteed to be run in
> > reverse order so so long as the resources used by the managed input
> > device are also managed we're supposed to be fine.

> That only works if _all_ resources are managed. Here we have an
> example of non-managed operations (such as detaching device from a
> power domain) in the middle of managed ones. And so the ordering gets
> broken.

Right, we need to ensure that managed resources don't depend on
resources that aren't managed (either directly or indirectly by being
handled by some callback which is managed).

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Theodore Ts'o]

This thread has meandered pretty far afield from the original topic
proposal, and it's not clear whether there is something that is worth
talking about at the kernel summit.  I don't _think_ there is, but it's
possible I've missed something.  Please speak up if there's something
that you think would be worth having a face-to-face discussion about.

Thanks,

					- Ted
					

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 750 bytes --]

On Mon, Oct 12, 2015 at 02:36:07PM -0400, Theodore Ts'o wrote:

> This thread has meandered pretty far afield from the original topic
> proposal, and it's not clear whether there is something that is worth
> talking about at the kernel summit.  I don't _think_ there is, but it's
> possible I've missed something.  Please speak up if there's something
> that you think would be worth having a face-to-face discussion about.

Having gone and found the context to refresh my memory I do see there
was the discussion about providing an interface for drivers to more
easily manage the lifetime of objects allocated for userspace use (which
devm_ isn't).  I'm not sure if that needs a KS discussion or someone to
step up and try to code something though.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Theodore Ts'o]

On Mon, Oct 12, 2015 at 07:44:54PM +0100, Mark Brown wrote:
> On Mon, Oct 12, 2015 at 02:36:07PM -0400, Theodore Ts'o wrote:
> 
> > This thread has meandered pretty far afield from the original topic
> > proposal, and it's not clear whether there is something that is worth
> > talking about at the kernel summit.  I don't _think_ there is, but it's
> > possible I've missed something.  Please speak up if there's something
> > that you think would be worth having a face-to-face discussion about.
> 
> Having gone and found the context to refresh my memory I do see there
> was the discussion about providing an interface for drivers to more
> easily manage the lifetime of objects allocated for userspace use (which
> devm_ isn't).  I'm not sure if that needs a KS discussion or someone to
> step up and try to code something though.

Yeah, that was my personal take on things; that further discussion
some concrete code to disuss would likely not be productive.  If
someone has time to throw a proof of concept together between now and
the kernel summit, we can always dynamically schedule a slot at that
point.

	    	       	       	    - Ted