[RFC] add module reference to struct tty_driver

[RFC] add module reference to struct tty_driver

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

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

Re: [RFC] add module reference to struct tty_driver

[Russell King]

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

Re: [RFC] add module reference to struct tty_driver

[Greg KH]

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

Re: [RFC] add module reference to struct tty_driver

[Max Krasnyansky]

Russell,

Any comments on this one
	http://marc.theaimsgroup.com/?l=linux-kernel&m=104155741713906&w=2

??

Max

Re: [RFC] add module reference to struct tty_driver

[Russell King]

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

Re: [RFC] add module reference to struct tty_driver

[Greg KH]

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

Re: [RFC] add module reference to struct tty_driver

[Henrique Gobbi]

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

Re: [RFC] add module reference to struct tty_driver

[Russell King]

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

Re: [RFC] add module reference to struct tty_driver

[John Bradford]

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

Re: [RFC] add module reference to struct tty_driver

[Russell King]

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

Re: [RFC] add module reference to struct tty_driver

[John Bradford]

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

Re: [RFC] add module reference to struct tty_driver

[Kai Germaschewski]

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

Re: [RFC] add module reference to struct tty_driver

[Randy.Dunlap]

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

Re: [RFC] add module reference to struct tty_driver

[Roman Zippel]

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