add module reference to struct tty_driver
Subsystems (2)
RFC 1/1 add module reference to struct tty_driver
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.
drivers/char/tty_io.c +7 -0
@@ -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); }
include/linux/tty_driver.h +1 -0
@@ -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 */
Quoted 8 lines · Greg KH
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.
Signature · 2 lines
Russell King ([email protected]) The developer of ARM Linux http://www.arm.linux.org.uk/personal/aboutme.html
Quoted 10 lines · Russell King
Quoted 8 lines · Greg KH
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.
Quoted 8 lines
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
Russell,
Any comments on this one
http://marc.theaimsgroup.com/?l=linux-kernel&m=104155741713906&w=2
??
Max
Quoted 8 lines · Greg KH
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?).
Signature · 2 lines
Russell King ([email protected]) The developer of ARM Linux http://www.arm.linux.org.uk/personal/aboutme.html
Quoted 15 lines · Russell King
Quoted 8 lines · Greg KH
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.
Quoted 4 lines
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.
Quoted 6 lines
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?
Quoted 2 lines
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" :(
Quoted 3 lines
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
Quoted 4 lines · Greg KH
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.
Signature · 2 lines
Russell King ([email protected]) The developer of ARM Linux http://www.arm.linux.org.uk/personal/aboutme.html
Quoted 7 lines
Quoted 4 lines
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.
Quoted 10 lines · John Bradford
Quoted 7 lines
Quoted 4 lines
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)
Signature · 2 lines
Russell King ([email protected]) The developer of ARM Linux http://www.arm.linux.org.uk/personal/aboutme.html
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
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" :(
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
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?
Quoted 5 lines
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.
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
Signature · 1 line
~Randy
Quoted 8 lines · Russell King
Quoted 4 lines · Greg KH
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
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
Quoted 10 lines · Russell King
Quoted 4 lines · Greg KH
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
Quoted 9 lines · Russell King
Quoted 4 lines · Greg KH
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