[lkml]   [2011]   [Nov]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [RFC] Input: Remove unsafe device module references
    On Tue, Nov 1, 2011 at 7:05 PM, Dmitry Torokhov
    <> wrote:
    > On Tue, Nov 01, 2011 at 06:52:11PM +0100, David Herrmann wrote:
    >> Hi Greg
    >> On Tue, Nov 1, 2011 at 6:01 PM, Greg KH <> wrote:
    >> > On Tue, Nov 01, 2011 at 04:41:40PM +0100, David Herrmann wrote:
    >> >> 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.
    >> >
    >> >> 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?
    >> >
    >> >> My bug-thread kind of died ( 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.
    >> > 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,

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

    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to
    More majordomo info at
    Please read the FAQ at

     \ /
      Last update: 2011-11-01 19:19    [W:0.032 / U:5.828 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site