From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dmitry Torokhov Subject: Re: [RFC] Input: Remove unsafe device module references Date: Tue, 1 Nov 2011 11:05:19 -0700 Message-ID: <20111101180519.GB15216@core.coreip.homeip.net> References: <1320162100-13494-1-git-send-email-dh.herrmann@googlemail.com> <20111101170156.GA8925@suse.de> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Received: from mail-gy0-f174.google.com ([209.85.160.174]:33633 "EHLO mail-gy0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932259Ab1KASFe (ORCPT ); Tue, 1 Nov 2011 14:05:34 -0400 Content-Disposition: inline In-Reply-To: Sender: linux-input-owner@vger.kernel.org List-Id: linux-input@vger.kernel.org To: David Herrmann Cc: Greg KH , linux-input@vger.kernel.org, linux-kernel@vger.kernel.org On Tue, Nov 01, 2011 at 06:52:11PM +0100, David Herrmann wrote: > Hi Greg >=20 > 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 sti= ll alive when > >> this function returns. Therefore, module_put() will return to inva= lid 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, "st= ruct device" > >> doesn't provide an owner field so there is no way for us to let th= e 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 d= efinitely > >> unsafe so this is marked as RFC. Currently, the device_attributes = probably > >> already keep a reference to our module so applying this patch woul= d 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 (https://lkml.org/lkml/2011/10/29/75) s= o I now try to > >> show this with an example here. > > > > It died due to me traveling, sorry, I'll respond to them now. >=20 > No problem. This is why I've resent this with an example. >=20 > > I fail to see what the real problem you are trying to solve here is= =2E =A0Is > > there something with the way the kernel works today that you are ha= ving > > problems with? =A0What is driving this? >=20 > I am working on converting the hci stack to properly use sysfs APIs + > struct device. And my problem simply is the following: >=20 > @@ -1417,8 +1417,6 @@ static void input_dev_release(struct device *de= vice) > input_mt_destroy_slots(dev); > kfree(dev->absinfo); > kfree(dev); > - > - module_put(THIS_MODULE); > } >=20 > 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. >=20 > But, if I can be sure that this doesn't release the last reference, > why take this reference at all? >=20 > 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. >=20 > 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? (Bu= t > then I wonder how does it know what module I am?) >=20 > 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 stil= l > breaks without this patch applied. >=20 >=20 >=20 > For instance see my example: >=20 > 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-re= fcnt > 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 proble= m much much easier to hit. >=20 > 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? --=20 Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-input" = in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html