* [RFC] add module reference to struct tty_driver @ 2003-01-13 5:47 Greg KH 2003-01-13 9:27 ` Russell King 2003-01-14 20:07 ` Russell King 0 siblings, 2 replies; 14+ messages in thread From: Greg KH @ 2003-01-13 5:47 UTC (permalink / raw) To: linux-kernel In digging into the tty layer locking, I noticed that the tty layer doesn't handle module reference counting for any tty drivers. Well, I've known this for a long time, just finally got around to fixing it :) Here's a patch against 2.5.56 that should fix this issue (works for me...) Comments? If no one objects, I'll send it on to Linus, and add support for this to a number of tty drivers that commonly get built as modules. thanks, greg k-h # TTY: add module reference counting for tty drivers. diff -Nru a/drivers/char/tty_io.c b/drivers/char/tty_io.c --- a/drivers/char/tty_io.c Sun Jan 12 21:46:53 2003 +++ b/drivers/char/tty_io.c Sun Jan 12 21:46:53 2003 @@ -833,6 +833,11 @@ * and locked termios may be retained.) */ + if (!try_module_get(driver->owner)) { + retval = -ENODEV; + goto end_init; + } + o_tty = NULL; tp = o_tp = NULL; ltp = o_ltp = NULL; @@ -991,6 +996,7 @@ free_tty_struct(tty); fail_no_mem: + module_put(driver->owner); retval = -ENOMEM; goto end_init; @@ -1033,6 +1039,7 @@ tty->magic = 0; (*tty->driver.refcount)--; list_del(&tty->tty_files); + module_put(tty->driver.owner); free_tty_struct(tty); } diff -Nru a/include/linux/tty_driver.h b/include/linux/tty_driver.h --- a/include/linux/tty_driver.h Sun Jan 12 21:46:53 2003 +++ b/include/linux/tty_driver.h Sun Jan 12 21:46:53 2003 @@ -120,6 +120,7 @@ struct tty_driver { int magic; /* magic number for this structure */ + struct module *owner; const char *driver_name; const char *name; int name_base; /* offset of printed name */ ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC] add module reference to struct tty_driver 2003-01-13 5:47 [RFC] add module reference to struct tty_driver Greg KH @ 2003-01-13 9:27 ` Russell King 2003-01-13 17:12 ` Greg KH 2003-01-13 19:51 ` Max Krasnyansky 2003-01-14 20:07 ` Russell King 1 sibling, 2 replies; 14+ messages in thread From: Russell King @ 2003-01-13 9:27 UTC (permalink / raw) To: Greg KH; +Cc: linux-kernel On Sun, Jan 12, 2003 at 09:47:09PM -0800, Greg KH wrote: > In digging into the tty layer locking, I noticed that the tty layer > doesn't handle module reference counting for any tty drivers. Well, I've > known this for a long time, just finally got around to fixing it :) > Here's a patch against 2.5.56 that should fix this issue (works for > me...) > > Comments? If no one objects, I'll send it on to Linus, and add support > for this to a number of tty drivers that commonly get built as modules. I'd just ask whether you considered what happens when: 1. two people open the same tty 2. the tty is hung up 3. both people close the tty (this isn't an indication that the patch is wrong, I'm just interested to know.) I'll test its behaviour later today - the current rules for incrementing/ decrementing the tty driver module use counts are rediculous at present, and this patch would solve it nicely. -- Russell King (rmk@arm.linux.org.uk) The developer of ARM Linux http://www.arm.linux.org.uk/personal/aboutme.html ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC] add module reference to struct tty_driver 2003-01-13 9:27 ` Russell King @ 2003-01-13 17:12 ` Greg KH 2003-01-13 19:51 ` Max Krasnyansky 1 sibling, 0 replies; 14+ messages in thread From: Greg KH @ 2003-01-13 17:12 UTC (permalink / raw) To: linux-kernel On Mon, Jan 13, 2003 at 09:27:34AM +0000, Russell King wrote: > On Sun, Jan 12, 2003 at 09:47:09PM -0800, Greg KH wrote: > > In digging into the tty layer locking, I noticed that the tty layer > > doesn't handle module reference counting for any tty drivers. Well, I've > > known this for a long time, just finally got around to fixing it :) > > Here's a patch against 2.5.56 that should fix this issue (works for > > me...) > > > > Comments? If no one objects, I'll send it on to Linus, and add support > > for this to a number of tty drivers that commonly get built as modules. > > I'd just ask whether you considered what happens when: > > 1. two people open the same tty > 2. the tty is hung up > 3. both people close the tty > > (this isn't an indication that the patch is wrong, I'm just interested > to know.) It "should" work with the above situation, as we only decrement the module count when the tty device structure that is bound to a driver is freed, and increment it when it is created. So if those functions work properly with regards to memory management, the module reference counting should also work. Hm, well I hope so at least :) Let me know if your tests show up any problems. thanks, gregk -h ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC] add module reference to struct tty_driver 2003-01-13 9:27 ` Russell King 2003-01-13 17:12 ` Greg KH @ 2003-01-13 19:51 ` Max Krasnyansky 1 sibling, 0 replies; 14+ messages in thread From: Max Krasnyansky @ 2003-01-13 19:51 UTC (permalink / raw) To: Russell King, Greg KH; +Cc: linux-kernel Russell, Any comments on this one http://marc.theaimsgroup.com/?l=linux-kernel&m=104155741713906&w=2 ?? Max ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC] add module reference to struct tty_driver 2003-01-13 5:47 [RFC] add module reference to struct tty_driver Greg KH 2003-01-13 9:27 ` Russell King @ 2003-01-14 20:07 ` Russell King 2003-01-14 22:08 ` Greg KH 1 sibling, 1 reply; 14+ messages in thread From: Russell King @ 2003-01-14 20:07 UTC (permalink / raw) To: Greg KH; +Cc: linux-kernel On Sun, Jan 12, 2003 at 09:47:09PM -0800, Greg KH wrote: > In digging into the tty layer locking, I noticed that the tty layer > doesn't handle module reference counting for any tty drivers. Well, I've > known this for a long time, just finally got around to fixing it :) > Here's a patch against 2.5.56 that should fix this issue (works for > me...) > > Comments? If no one objects, I'll send it on to Linus, and add support > for this to a number of tty drivers that commonly get built as modules. Firstly, I've proven my original suspicions about tty hangup wrong. However, I'm concerned that we don't have sufficient locking present (even in 2.4) to ensure that unloading tty driver modules is safe by any means. The first point where we obtain a driver structure is under the BKL in tty_io.c:init_dev(), which calls get_tty_driver(). get_tty_driver() searches a list of drivers for the relevant entry. There are no locks here. Now, consider tty_unregister_driver(). This is normally called from a tty driver modules cleanup function. Also note that there are no locks here. Also consider tty_register_driver() and note, again, that there are no locks here. Checking kernel/module.c, the BKL isn't held when calling the modules init and cleanup functions. So, all in all, we have a nice SMP race between loading tty driver modules, unloading tty driver modules, and getting reference counts on driver modules. Since tty_register_driver() and tty_unregister_driver() are both called from process context, the fix can be a semaphore. However, note carefully that any semaphore that can sleep in the open path will drop the BKL and therefore could cause other races (wrt driver->refcount?). -- Russell King (rmk@arm.linux.org.uk) The developer of ARM Linux http://www.arm.linux.org.uk/personal/aboutme.html ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC] add module reference to struct tty_driver 2003-01-14 20:07 ` Russell King @ 2003-01-14 22:08 ` Greg KH 2003-01-15 10:00 ` Russell King 0 siblings, 1 reply; 14+ messages in thread From: Greg KH @ 2003-01-14 22:08 UTC (permalink / raw) To: linux-kernel On Tue, Jan 14, 2003 at 08:07:19PM +0000, Russell King wrote: > On Sun, Jan 12, 2003 at 09:47:09PM -0800, Greg KH wrote: > > In digging into the tty layer locking, I noticed that the tty layer > > doesn't handle module reference counting for any tty drivers. Well, I've > > known this for a long time, just finally got around to fixing it :) > > Here's a patch against 2.5.56 that should fix this issue (works for > > me...) > > > > Comments? If no one objects, I'll send it on to Linus, and add support > > for this to a number of tty drivers that commonly get built as modules. > > Firstly, I've proven my original suspicions about tty hangup wrong. > However, I'm concerned that we don't have sufficient locking present > (even in 2.4) to ensure that unloading tty driver modules is safe by > any means. > > The first point where we obtain a driver structure is under the > BKL in tty_io.c:init_dev(), which calls get_tty_driver(). > get_tty_driver() searches a list of drivers for the relevant > entry. There are no locks here. init_dev() is only called from tty_open() which is called under the BKL. > Now, consider tty_unregister_driver(). This is normally called from > a tty driver modules cleanup function. Also note that there are no > locks here. > > Also consider tty_register_driver() and note, again, that there are > no locks here. Ok, there should be some kind of lock of the tty_drivers list, I agree. But that doesn't pertain to this module reference counting patch, right? > Checking kernel/module.c, the BKL isn't held when calling the modules > init and cleanup functions. Woah! Hm, this is going to cause lots of problems in drivers that have been assuming that the BKL is grabbed during module unload, and during open(). Hm, time to just fallback on the argument, "module unloading is unsafe" :( > So, all in all, we have a nice SMP race between loading tty driver > modules, unloading tty driver modules, and getting reference counts > on driver modules. Yeah, you're right. But this isn't the only subsystem that now has this race :( I still think the original patch will help, and it will remove all of the MOD_INC usages in tty drivers, which is a step in the right direction. So I'll send this to Linus, and we'll move on to the next locking mess in here... thanks, greg k-h ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC] add module reference to struct tty_driver 2003-01-14 22:08 ` Greg KH @ 2003-01-15 10:00 ` Russell King 2003-01-15 9:09 ` Henrique Gobbi ` (3 more replies) 0 siblings, 4 replies; 14+ messages in thread From: Russell King @ 2003-01-15 10:00 UTC (permalink / raw) To: Greg KH; +Cc: linux-kernel On Tue, Jan 14, 2003 at 02:08:59PM -0800, Greg KH wrote: > Woah! Hm, this is going to cause lots of problems in drivers that have > been assuming that the BKL is grabbed during module unload, and during > open(). Hm, time to just fallback on the argument, "module unloading is > unsafe" :( Note that its the same in 2.4 as well. iirc, the BKL was removed from module loading/unloading sometime in the 2.3 timeline. -- Russell King (rmk@arm.linux.org.uk) The developer of ARM Linux http://www.arm.linux.org.uk/personal/aboutme.html ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC] add module reference to struct tty_driver 2003-01-15 10:00 ` Russell King @ 2003-01-15 9:09 ` Henrique Gobbi 2003-01-15 10:30 ` John Bradford ` (2 subsequent siblings) 3 siblings, 0 replies; 14+ messages in thread From: Henrique Gobbi @ 2003-01-15 9:09 UTC (permalink / raw) To: linux-kernel Hi !!! Noob question !! Could anyone contribute to my poor kernel knowledge base and explain me what is BKL you're all talking about ? thank you Henrique Russell King wrote: > On Tue, Jan 14, 2003 at 02:08:59PM -0800, Greg KH wrote: > >>Woah! Hm, this is going to cause lots of problems in drivers that have >>been assuming that the BKL is grabbed during module unload, and during >>open(). Hm, time to just fallback on the argument, "module unloading is >>unsafe" :( > > > Note that its the same in 2.4 as well. iirc, the BKL was removed from > module loading/unloading sometime in the 2.3 timeline ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC] add module reference to struct tty_driver 2003-01-15 10:00 ` Russell King 2003-01-15 9:09 ` Henrique Gobbi @ 2003-01-15 10:30 ` John Bradford 2003-01-15 10:40 ` Russell King 2003-01-15 15:26 ` Kai Germaschewski 2003-01-15 16:31 ` Roman Zippel 3 siblings, 1 reply; 14+ messages in thread From: John Bradford @ 2003-01-15 10:30 UTC (permalink / raw) To: Russell King; +Cc: linux-kernel, greg > > Woah! Hm, this is going to cause lots of problems in drivers that have > > been assuming that the BKL is grabbed during module unload, and during > > open(). Hm, time to just fallback on the argument, "module unloading is > > unsafe" :( > > Note that its the same in 2.4 as well. iirc, the BKL was removed from > module loading/unloading sometime in the 2.3 timeline. Surely no recent code should be making that assumption anyway - the BKL is being removed all over the place. John. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC] add module reference to struct tty_driver 2003-01-15 10:30 ` John Bradford @ 2003-01-15 10:40 ` Russell King 2003-01-15 11:12 ` John Bradford 2003-01-15 16:03 ` Randy.Dunlap 0 siblings, 2 replies; 14+ messages in thread From: Russell King @ 2003-01-15 10:40 UTC (permalink / raw) To: John Bradford; +Cc: linux-kernel, greg On Wed, Jan 15, 2003 at 10:30:03AM +0000, John Bradford wrote: > > > Woah! Hm, this is going to cause lots of problems in drivers that have > > > been assuming that the BKL is grabbed during module unload, and during > > > open(). Hm, time to just fallback on the argument, "module unloading is > > > unsafe" :( > > > > Note that its the same in 2.4 as well. iirc, the BKL was removed from > > module loading/unloading sometime in the 2.3 timeline. > > Surely no recent code should be making that assumption anyway - the > BKL is being removed all over the place. The TTY layer isn't "recent code", its "very old code", and (IMO) removing the BKL from the TTY layer is a far from trivial matter. I believe at this point in the 2.5 cycle, we should not be looking to remove the BKL. We should be looking to fix the problems we know about. That basically means: - module refcounting - interrupt races - any other races (eg, tty_register_driver / tty_unregister_driver) -- Russell King (rmk@arm.linux.org.uk) The developer of ARM Linux http://www.arm.linux.org.uk/personal/aboutme.html ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC] add module reference to struct tty_driver 2003-01-15 10:40 ` Russell King @ 2003-01-15 11:12 ` John Bradford 2003-01-15 16:03 ` Randy.Dunlap 1 sibling, 0 replies; 14+ messages in thread From: John Bradford @ 2003-01-15 11:12 UTC (permalink / raw) To: Russell King; +Cc: linux-kernel, greg > > > > Woah! Hm, this is going to cause lots of problems in drivers that have > > > > been assuming that the BKL is grabbed during module unload, and during > > > > open(). Hm, time to just fallback on the argument, "module > > > > unloading is unsafe" :( > > > > > > Note that its the same in 2.4 as well. iirc, the BKL was removed from > > > module loading/unloading sometime in the 2.3 timeline. > > > > Surely no recent code should be making that assumption anyway - the > > BKL is being removed all over the place. > > The TTY layer isn't "recent code", its "very old code" Oh, I know, I was just thinking aloud and not making much sense, ( :-) ) - what I really meant was is anybody writing new code without the near-future BKL changes in mind, and if so, shouldn't it be avoided now to save work in 2.7? I.E. is the BKL officially depreciated? > and (IMO) removing the BKL from the TTY layer is a far from trivial > matter. > > I believe at this point in the 2.5 cycle, we should not be looking > to remove the BKL. I think there were hints in another thread of a TTY layer overhaul in 2.7. John. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC] add module reference to struct tty_driver 2003-01-15 10:40 ` Russell King 2003-01-15 11:12 ` John Bradford @ 2003-01-15 16:03 ` Randy.Dunlap 1 sibling, 0 replies; 14+ messages in thread From: Randy.Dunlap @ 2003-01-15 16:03 UTC (permalink / raw) To: Russell King; +Cc: John Bradford, linux-kernel, greg On Wed, 15 Jan 2003, Russell King wrote: | On Wed, Jan 15, 2003 at 10:30:03AM +0000, John Bradford wrote: | > > > Woah! Hm, this is going to cause lots of problems in drivers that have | > > > been assuming that the BKL is grabbed during module unload, and during | > > > open(). Hm, time to just fallback on the argument, "module unloading is | > > > unsafe" :( | > > | > > Note that its the same in 2.4 as well. iirc, the BKL was removed from | > > module loading/unloading sometime in the 2.3 timeline. | > | > Surely no recent code should be making that assumption anyway - the | > BKL is being removed all over the place. | | The TTY layer isn't "recent code", its "very old code", and (IMO) | removing the BKL from the TTY layer is a far from trivial matter. | | I believe at this point in the 2.5 cycle, we should not be looking | to remove the BKL. We should be looking to fix the problems we know | about. That basically means: | | - module refcounting | - interrupt races | - any other races (eg, tty_register_driver / tty_unregister_driver) such as in http://bugme.osdl.org/show_bug.cgi?id=54 -- ~Randy ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC] add module reference to struct tty_driver 2003-01-15 10:00 ` Russell King 2003-01-15 9:09 ` Henrique Gobbi 2003-01-15 10:30 ` John Bradford @ 2003-01-15 15:26 ` Kai Germaschewski 2003-01-15 16:31 ` Roman Zippel 3 siblings, 0 replies; 14+ messages in thread From: Kai Germaschewski @ 2003-01-15 15:26 UTC (permalink / raw) To: Russell King; +Cc: Greg KH, linux-kernel On Wed, 15 Jan 2003, Russell King wrote: > On Tue, Jan 14, 2003 at 02:08:59PM -0800, Greg KH wrote: > > Woah! Hm, this is going to cause lots of problems in drivers that have > > been assuming that the BKL is grabbed during module unload, and during > > open(). Hm, time to just fallback on the argument, "module unloading is > > unsafe" :( > > Note that its the same in 2.4 as well. iirc, the BKL was removed from > module loading/unloading sometime in the 2.3 timeline. You didn't look at linux-2.4/kernel/module.c lately, did you? ;) --Kai ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC] add module reference to struct tty_driver 2003-01-15 10:00 ` Russell King ` (2 preceding siblings ...) 2003-01-15 15:26 ` Kai Germaschewski @ 2003-01-15 16:31 ` Roman Zippel 3 siblings, 0 replies; 14+ messages in thread From: Roman Zippel @ 2003-01-15 16:31 UTC (permalink / raw) To: Russell King; +Cc: Greg KH, linux-kernel Russell King wrote: > > On Tue, Jan 14, 2003 at 02:08:59PM -0800, Greg KH wrote: > > Woah! Hm, this is going to cause lots of problems in drivers that have > > been assuming that the BKL is grabbed during module unload, and during > > open(). Hm, time to just fallback on the argument, "module unloading is > > unsafe" :( > > Note that its the same in 2.4 as well. iirc, the BKL was removed from > module loading/unloading sometime in the 2.3 timeline. Um, my copy of 2.4 module.c still has lock_kernel()/unlock_kernel() all over it and I'm quite sure that didn't change until 2.5.47. bye, Roman ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2003-01-15 17:47 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2003-01-13 5:47 [RFC] add module reference to struct tty_driver Greg KH 2003-01-13 9:27 ` Russell King 2003-01-13 17:12 ` Greg KH 2003-01-13 19:51 ` Max Krasnyansky 2003-01-14 20:07 ` Russell King 2003-01-14 22:08 ` Greg KH 2003-01-15 10:00 ` Russell King 2003-01-15 9:09 ` Henrique Gobbi 2003-01-15 10:30 ` John Bradford 2003-01-15 10:40 ` Russell King 2003-01-15 11:12 ` John Bradford 2003-01-15 16:03 ` Randy.Dunlap 2003-01-15 15:26 ` Kai Germaschewski 2003-01-15 16:31 ` Roman Zippel
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).