* [RFC] Input: Remove unsafe device module references @ 2011-11-01 15:41 David Herrmann 2011-11-01 17:01 ` Greg KH 0 siblings, 1 reply; 12+ messages in thread From: David Herrmann @ 2011-11-01 15:41 UTC (permalink / raw) To: linux-input; +Cc: dmitry.torokhov, gregkh, linux-kernel, David Herrmann 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. 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. 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. Regards David Signed-off-by: David Herrmann <dh.herrmann@googlemail.com> --- drivers/input/input.c | 4 ---- 1 files changed, 0 insertions(+), 4 deletions(-) diff --git a/drivers/input/input.c b/drivers/input/input.c index da38d97..f691502 100644 --- a/drivers/input/input.c +++ b/drivers/input/input.c @@ -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); } /* @@ -1657,8 +1655,6 @@ struct input_dev *input_allocate_device(void) spin_lock_init(&dev->event_lock); INIT_LIST_HEAD(&dev->h_list); INIT_LIST_HEAD(&dev->node); - - __module_get(THIS_MODULE); } return dev; -- 1.7.7.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [RFC] Input: Remove unsafe device module references 2011-11-01 15:41 [RFC] Input: Remove unsafe device module references David Herrmann @ 2011-11-01 17:01 ` Greg KH 2011-11-01 17:52 ` Dmitry Torokhov 2011-11-01 17:52 ` David Herrmann 0 siblings, 2 replies; 12+ messages in thread From: Greg KH @ 2011-11-01 17:01 UTC (permalink / raw) To: David Herrmann; +Cc: linux-input, dmitry.torokhov, linux-kernel 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 (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. 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? greg k-h ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC] Input: Remove unsafe device module references 2011-11-01 17:01 ` Greg KH @ 2011-11-01 17:52 ` Dmitry Torokhov 2011-11-01 17:58 ` Greg KH 2011-11-01 17:52 ` David Herrmann 1 sibling, 1 reply; 12+ messages in thread From: Dmitry Torokhov @ 2011-11-01 17:52 UTC (permalink / raw) To: Greg KH; +Cc: David Herrmann, linux-input, linux-kernel On Tue, Nov 01, 2011 at 10:01:56AM -0700, 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. Strictly speaking, David is right, there is a race condition here. However since we do module_put() as very last operation of input_dev_release() it is extremely hard to trigger this race. Until we have a better way of pinning the bus (or class) implementation in memory we should keep __module_get/module_put in input core. -- Dmitry ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC] Input: Remove unsafe device module references 2011-11-01 17:52 ` Dmitry Torokhov @ 2011-11-01 17:58 ` Greg KH 0 siblings, 0 replies; 12+ messages in thread From: Greg KH @ 2011-11-01 17:58 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: David Herrmann, linux-input, linux-kernel On Tue, Nov 01, 2011 at 10:52:10AM -0700, Dmitry Torokhov wrote: > On Tue, Nov 01, 2011 at 10:01:56AM -0700, 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. > > Strictly speaking, David is right, there is a race condition here. > However since we do module_put() as very last operation of > input_dev_release() it is extremely hard to trigger this race. > > Until we have a better way of pinning the bus (or class) implementation > in memory we should keep __module_get/module_put in input core. I agree, that's fine for a bus to do, as long as you are aware of how it is working. greg k-h ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC] Input: Remove unsafe device module references 2011-11-01 17:01 ` Greg KH 2011-11-01 17:52 ` Dmitry Torokhov @ 2011-11-01 17:52 ` David Herrmann 2011-11-01 18:00 ` Greg KH 2011-11-01 18:05 ` Dmitry Torokhov 1 sibling, 2 replies; 12+ messages in thread From: David Herrmann @ 2011-11-01 17:52 UTC (permalink / raw) To: Greg KH; +Cc: linux-input, dmitry.torokhov, linux-kernel Hi Greg On Tue, Nov 1, 2011 at 6:01 PM, Greg KH <gregkh@suse.de> 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 (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. > 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. 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(). > greg k-h > Regards David -- 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC] Input: Remove unsafe device module references 2011-11-01 17:52 ` David Herrmann @ 2011-11-01 18:00 ` Greg KH 2011-11-01 18:09 ` David Herrmann 2011-11-01 18:05 ` Dmitry Torokhov 1 sibling, 1 reply; 12+ messages in thread From: Greg KH @ 2011-11-01 18:00 UTC (permalink / raw) To: David Herrmann; +Cc: linux-input, dmitry.torokhov, linux-kernel On Tue, Nov 01, 2011 at 06:52:11PM +0100, David Herrmann wrote: > My solution: Some parent subsystem of us must take and release this > module-refcnt instead of us, so this bug doesn't occur. Yes, that is the ultimate solution for something like this. But, in reality, we don't care about module unloading races as there are plenty of other issues involved there where things can go bad, so we just try the best we can :) thanks, greg k-h ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC] Input: Remove unsafe device module references 2011-11-01 18:00 ` Greg KH @ 2011-11-01 18:09 ` David Herrmann 2011-11-01 18:18 ` Dmitry Torokhov 0 siblings, 1 reply; 12+ messages in thread From: David Herrmann @ 2011-11-01 18:09 UTC (permalink / raw) To: Greg KH; +Cc: linux-input, dmitry.torokhov, linux-kernel On Tue, Nov 1, 2011 at 7:00 PM, Greg KH <gregkh@suse.de> wrote: > On Tue, Nov 01, 2011 at 06:52:11PM +0100, David Herrmann wrote: >> My solution: Some parent subsystem of us must take and release this >> module-refcnt instead of us, so this bug doesn't occur. > > Yes, that is the ultimate solution for something like this. > > But, in reality, we don't care about module unloading races as there are > plenty of other issues involved there where things can go bad, so we > just try the best we can :) Ah, I am kind of relieved that I got this right. I almost started thinking I am insane.. ;) So your answer is that this is so unlikely that it won't be fixed? I am fine with that, even though I wonder why stuff like "struct file_operations" include "owner" fields to protect callbacks but "struct device_type" does *not* include any protection of it's "release" callback. This is why I thought calling module_get/put() inside the driver-core would be just consistent with other subsystems. But if this race is fine, I will simply copy it. > thanks, > > greg k-h Sorry for the confusion and thanks for your answers. Regards David ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC] Input: Remove unsafe device module references 2011-11-01 18:09 ` David Herrmann @ 2011-11-01 18:18 ` Dmitry Torokhov 2011-11-02 13:45 ` David Herrmann 0 siblings, 1 reply; 12+ messages in thread From: Dmitry Torokhov @ 2011-11-01 18:18 UTC (permalink / raw) To: David Herrmann; +Cc: Greg KH, linux-input, linux-kernel On Tue, Nov 01, 2011 at 07:09:27PM +0100, David Herrmann wrote: > On Tue, Nov 1, 2011 at 7:00 PM, Greg KH <gregkh@suse.de> wrote: > > On Tue, Nov 01, 2011 at 06:52:11PM +0100, David Herrmann wrote: > >> My solution: Some parent subsystem of us must take and release this > >> module-refcnt instead of us, so this bug doesn't occur. > > > > Yes, that is the ultimate solution for something like this. > > > > But, in reality, we don't care about module unloading races as there are > > plenty of other issues involved there where things can go bad, so we > > just try the best we can :) > > Ah, I am kind of relieved that I got this right. I almost started > thinking I am insane.. ;) > > So your answer is that this is so unlikely that it won't be fixed? I > am fine with that, even though I wonder why stuff like "struct > file_operations" include "owner" fields to protect callbacks but > "struct device_type" does *not* include any protection of it's > "release" callback. I think adding owner to device_type might not be a bad idea at all... Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC] Input: Remove unsafe device module references 2011-11-01 18:18 ` Dmitry Torokhov @ 2011-11-02 13:45 ` David Herrmann 2011-11-02 14:43 ` Greg KH 0 siblings, 1 reply; 12+ messages in thread From: David Herrmann @ 2011-11-02 13:45 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: Greg KH, linux-input, linux-kernel On Tue, Nov 1, 2011 at 7:18 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > On Tue, Nov 01, 2011 at 07:09:27PM +0100, David Herrmann wrote: >> On Tue, Nov 1, 2011 at 7:00 PM, Greg KH <gregkh@suse.de> wrote: >> > On Tue, Nov 01, 2011 at 06:52:11PM +0100, David Herrmann wrote: >> >> My solution: Some parent subsystem of us must take and release this >> >> module-refcnt instead of us, so this bug doesn't occur. >> > >> > Yes, that is the ultimate solution for something like this. >> > >> > But, in reality, we don't care about module unloading races as there are >> > plenty of other issues involved there where things can go bad, so we >> > just try the best we can :) >> >> Ah, I am kind of relieved that I got this right. I almost started >> thinking I am insane.. ;) >> >> So your answer is that this is so unlikely that it won't be fixed? I >> am fine with that, even though I wonder why stuff like "struct >> file_operations" include "owner" fields to protect callbacks but >> "struct device_type" does *not* include any protection of it's >> "release" callback. > > I think adding owner to device_type might not be a bad idea at all... Exactly. But Greg does not seem to be very amused by that idea :-/ > Thanks. > > -- > Dmitry Cheers David ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC] Input: Remove unsafe device module references 2011-11-02 13:45 ` David Herrmann @ 2011-11-02 14:43 ` Greg KH 0 siblings, 0 replies; 12+ messages in thread From: Greg KH @ 2011-11-02 14:43 UTC (permalink / raw) To: David Herrmann; +Cc: Dmitry Torokhov, linux-input, linux-kernel On Wed, Nov 02, 2011 at 02:45:58PM +0100, David Herrmann wrote: > On Tue, Nov 1, 2011 at 7:18 PM, Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > On Tue, Nov 01, 2011 at 07:09:27PM +0100, David Herrmann wrote: > >> On Tue, Nov 1, 2011 at 7:00 PM, Greg KH <gregkh@suse.de> wrote: > >> > On Tue, Nov 01, 2011 at 06:52:11PM +0100, David Herrmann wrote: > >> >> My solution: Some parent subsystem of us must take and release this > >> >> module-refcnt instead of us, so this bug doesn't occur. > >> > > >> > Yes, that is the ultimate solution for something like this. > >> > > >> > But, in reality, we don't care about module unloading races as there are > >> > plenty of other issues involved there where things can go bad, so we > >> > just try the best we can :) > >> > >> Ah, I am kind of relieved that I got this right. I almost started > >> thinking I am insane.. ;) > >> > >> So your answer is that this is so unlikely that it won't be fixed? I > >> am fine with that, even though I wonder why stuff like "struct > >> file_operations" include "owner" fields to protect callbacks but > >> "struct device_type" does *not* include any protection of it's > >> "release" callback. > > > > I think adding owner to device_type might not be a bad idea at all... > > Exactly. But Greg does not seem to be very amused by that idea :-/ Actually that might work, but again, is it worth it? Patches, as always, are gladly accepted, if you think this would resolve the issue. thanks, greg k-h ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC] Input: Remove unsafe device module references 2011-11-01 17:52 ` David Herrmann 2011-11-01 18:00 ` Greg KH @ 2011-11-01 18:05 ` Dmitry Torokhov 2011-11-01 18:16 ` David Herrmann 1 sibling, 1 reply; 12+ messages in thread From: Dmitry Torokhov @ 2011-11-01 18:05 UTC (permalink / raw) To: David Herrmann; +Cc: Greg KH, linux-input, linux-kernel 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 <gregkh@suse.de> 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 (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. > > > 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. > > 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? -- 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC] Input: Remove unsafe device module references 2011-11-01 18:05 ` Dmitry Torokhov @ 2011-11-01 18:16 ` David Herrmann 0 siblings, 0 replies; 12+ messages in thread From: David Herrmann @ 2011-11-01 18:16 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: Greg KH, linux-input, linux-kernel On Tue, Nov 1, 2011 at 7:05 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> 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 <gregkh@suse.de> 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 (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. >> >> > 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. >> >> 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2011-11-02 14:43 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-11-01 15:41 [RFC] Input: Remove unsafe device module references David Herrmann 2011-11-01 17:01 ` Greg KH 2011-11-01 17:52 ` Dmitry Torokhov 2011-11-01 17:58 ` Greg KH 2011-11-01 17:52 ` David Herrmann 2011-11-01 18:00 ` Greg KH 2011-11-01 18:09 ` David Herrmann 2011-11-01 18:18 ` Dmitry Torokhov 2011-11-02 13:45 ` David Herrmann 2011-11-02 14:43 ` Greg KH 2011-11-01 18:05 ` Dmitry Torokhov 2011-11-01 18:16 ` David Herrmann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).