Re: [RFC] Input: Remove unsafe device module references

On Tue, Nov 1, 2011 at 7:05 PM, Dmitry Torokhov
[off-list ref] wrote:

On Tue, Nov 01, 2011 at 06:52:11PM +0100, David Herrmann wrote:

quoted

Hi Greg

On Tue, Nov 1, 2011 at 6:01 PM, Greg KH [off-list ref] wrote:

quoted

On Tue, Nov 01, 2011 at 04:41:40PM +0100, David Herrmann wrote:

quoted

Hi Dmitry and Greg

It doesn't make sense to take a reference to our own module. When we call
module_put(THIS_MODULE) we cannot make sure that our module is still alive when
this function returns. Therefore, module_put() will return to invalid memory and
our input_dev_release() function is no longer available.

It would be interesting if Greg could elaborate what else we could do to replace
this module-refcount as it is definitely needed here. However, "struct device"
doesn't provide an owner field so there is no way for us to let the device core
keep a reference to our module.

For a bus module, yes, this is needed, so don't remove these calls, it's
wrong to do so.

quoted

I have no clue what to do here but the current implementation is definitely
unsafe so this is marked as RFC. Currently, the device_attributes probably
already keep a reference to our module so applying this patch would probably not
break anything, however, this does not look like something we can trust on.

Yes it is, why do you think it isn't?

quoted

My bug-thread kind of died (https://lkml.org/lkml/2011/10/29/75) so I now try to
show this with an example here.

It died due to me traveling, sorry, I'll respond to them now.

No problem. This is why I've resent this with an example.

quoted

I fail to see what the real problem you are trying to solve here is.  Is
there something with the way the kernel works today that you are having
problems with?  What is driving this?

I am working on converting the hci stack to properly use sysfs APIs +
struct device. And my problem simply is the following:

@@ -1417,8 +1417,6 @@ static void input_dev_release(struct device *device)

       input_mt_destroy_slots(dev);
       kfree(dev->absinfo);
       kfree(dev);
-
-       module_put(THIS_MODULE);
 }

If this module_put(THIS_MODULE) is needed as you said, then I can be
sure that this call does not release the last module-reference, can I?
Otherwise, this call may return to invalid memory.

But, if I can be sure that this doesn't release the last reference,
why take this reference at all?

The only reason I can think of is, that some other code calls
__get_module() after I called it, and it calls put_module() after I
call it. In all other cases, taking/releasing this reference is
needless as we can trust that our caller protects us.

In other words, which code does this module_get/put() protect? It
cannot protect input_dev_release() because module_put(THIS_MODULE) is
called *inside* input_dev_release(). I need some way to protect the
input_dev_release() callback-code outside of this callback.
Or can I go sure that the caller of the input_dev_callback() takes a
reference to my module before calling this and releases it after? (But
then I wonder how does it know what module I am?)

If this is the recommended way to protect the device_release
callbacks, I will just copy it into hci_dev, but currently I really
don't get why these are needed.
If you can tell me an example why the input-core breaks if this patch
is applied, I can probably better explain to you, why I think it still
breaks without this patch applied.



For instance see my example:

1)
input-core-module is loaded
2)
input-core-module creates a new input device and increases
module-refcnt inside input_allocate_device()
4)
another subsystem grabs the "struct device" and increases its refcnt
(for any reason...)
5)
input-core-module destroys the input-device but it still stays alive
until the other subsystem releases its refcnt of the "struct device".
6)
input-core-module is unloaded
This doesn't succeed as the still living input-device has a module-refcnt
7)
the other subsystem releases its refcnt of the input-device
8)
The input-device is destroyed and its _release_ function is called
The release function destroys the input-device *and* frees the last
module-refcnt. Then *boom*, the release function cannot return as it
is no longer available as described above.

The analysis is right except that this condition is very unlikely to
trigger. By removing __module_get/module_put you are making this problem
much much easier to hit.

I was aware of that, this is why I marked it as RFC and included some
comment, that another locking is needed. I don't know any trivial way,
though.

quoted

My solution: Some parent subsystem of us must take and release this
module-refcnt instead of us, so this bug doesn't occur.
Or: We simply wait for all these input_devices to be released before
exiting input_exit().

How would you know that all input devices are released in the sense
that all threads left (as in exited) all code in input.c completely?

I rethought this and all ideas (like completion_t or waitqueues) I
have will trigger the same race-condition as with the module_put()
thing we currently use.
So I am left with the parent-subsystem to take care of the module
refcount. Or just accepting this race-condition.

--
Dmitry

David