PATCH against 2.4.2: TTY hangup on PPP channel corrupts kernel memory

PATCH against 2.4.2: TTY hangup on PPP channel corrupts kernel memory

[Kevin Buhr]

Linus:

This one was tough to find.  I think I triggered it maybe four times
over the space of six months and almost chalked it up to an electrical
problem with the modem.

If there's a hangup in the TTY layer on an async PPP channel,
do_tty_hangup shuts down the PPP line discipline, and, in ppp_async.c,
the function ppp_asynctty_close unregisteres the channel.  In
ppp_generic.c, ppp_unregister_channel merrily wakes up the rwait
queue, then proceeds to destroy the channel, freeing the "struct
channel" which contains the "struct ppp_file" that contains the
"wait_queue_head_t rwait".  When the waiting process wakes up, it
removes itself from the wait queue, modifying freed memory.

(In my case, it turns out that the "struct channel" in ppp_generic.c
and "struct shmid_kernel" are both in the size-128 slab cache.  My
desktop pager uses the X SHM extension to snapshot the desktop and
does many SHM allocations per second.  In the occasional case that the
allocation beats the rwaiting process, a modem hangup sends the X
server spinning on a random piece of memory way down in
"valid_swaphandles", and the other CPU locks up in some unrelated
place waiting on the kernel lock.)

A patch against 2.4.2 follows.  I've overloaded the "refcnt" in
"struct ppp_file" to also keep track of rwaiters.  The last refcnt
user destroys the channel and decreases the module use count.  I've
tested this with printks in all the right places, and it seems to fix
the problem correctly.

I don't use any other PPP drivers besides async, but it looks like the
same bug exists (and will be fixed by this patch) in ppp_synctty.c.

The FILEVERSION at the top of ppp_generic may need to be fixed, but 

Thanks.

Kevin <buhr@stat.wisc.edu>

                        *       *       *

--- linux-2.4.2/drivers/net/ppp_generic.c	Sun Mar  4 20:09:19 2001
+++ linux-2.4.2-local/drivers/net/ppp_generic.c	Fri Mar 16 14:50:28 2001
@@ -72,7 +72,7 @@
 	struct sk_buff_head xq;		/* pppd transmit queue */
 	struct sk_buff_head rq;		/* receive queue for pppd */
 	wait_queue_head_t rwait;	/* for poll on reading /dev/ppp */
-	atomic_t	refcnt;		/* # refs (incl /dev/ppp attached) */
+	atomic_t	refcnt;		/* # refs (attaches and rwaiters) */
 	int		hdrlen;		/* space to leave for headers */
 	struct list_head list;		/* link in all_* list */
 	int		index;		/* interface unit / channel number */
@@ -316,6 +316,28 @@
 	return 0;
 }
 
+/*
+ * For ppp_async, a (true) hangup in the TTY layer will shut down the
+ * current line discipline, unregistering the channel, so...
+ *
+ * We must ppp_file_acquire before waiting on rwait to keep the
+ * channel around, then ppp_file_release after waiting to release
+ * resources if we're the last user.
+ */
+#define ppp_file_acquire(pf) atomic_inc(&(pf)->refcnt)
+static void ppp_file_release(struct ppp_file *pf) {
+	if (atomic_dec_and_test(&pf->refcnt)) {
+		switch (pf->kind) {
+		case INTERFACE:
+			ppp_destroy_interface(PF_TO_PPP(pf));
+			break;
+		case CHANNEL:
+			ppp_destroy_channel(PF_TO_CHANNEL(pf));
+			break;
+		}
+	}
+}
+
 static int ppp_release(struct inode *inode, struct file *file)
 {
 	struct ppp_file *pf = (struct ppp_file *) file->private_data;
@@ -323,16 +345,7 @@
 	lock_kernel();
 	if (pf != 0) {
 		file->private_data = 0;
-		if (atomic_dec_and_test(&pf->refcnt)) {
-			switch (pf->kind) {
-			case INTERFACE:
-				ppp_destroy_interface(PF_TO_PPP(pf));
-				break;
-			case CHANNEL:
-				ppp_destroy_channel(PF_TO_CHANNEL(pf));
-				break;
-			}
-		}
+		ppp_file_release(pf);
 	}
 	unlock_kernel();
 	return 0;
@@ -357,6 +370,7 @@
 	if (pf == 0)
 		goto out;		/* not currently attached */
 
+	ppp_file_acquire(pf);
 	add_wait_queue(&pf->rwait, &wait);
 	current->state = TASK_INTERRUPTIBLE;
 	for (;;) {
@@ -376,6 +390,7 @@
 	}
 	current->state = TASK_RUNNING;
 	remove_wait_queue(&pf->rwait, &wait);
+	ppp_file_release(pf);
 
 	if (skb == 0)
 		goto out;
@@ -448,6 +463,7 @@
 
 	if (pf == 0)
 		return 0;
+	ppp_file_acquire(pf);
 	poll_wait(file, &pf->rwait, wait);
 	mask = POLLOUT | POLLWRNORM;
 	if (skb_peek(&pf->rq) != 0)
@@ -457,6 +473,7 @@
 		if (pch->chan == 0)
 			mask |= POLLHUP;
 	}
+	ppp_file_release(pf);
 	return mask;
 }
 
@@ -1788,9 +1805,12 @@
 	spin_lock_bh(&all_channels_lock);
 	list_del(&pch->file.list);
 	spin_unlock_bh(&all_channels_lock);
+	/*
+	 * If refcnt goes to zero, there are no attachments *and*
+	 * no rwaiters, so destruction is safe.
+	 */
 	if (atomic_dec_and_test(&pch->file.refcnt))
 		ppp_destroy_channel(pch);
-	MOD_DEC_USE_COUNT;
 }
 
 /*
@@ -1840,9 +1860,11 @@
 
 	mask = POLLOUT | POLLWRNORM;
 	if (pch != 0) {
+		ppp_file_acquire(&pch->file);
 		poll_wait(file, &pch->file.rwait, wait);
 		if (skb_peek(&pch->file.rq) != 0)
 			mask |= POLLIN | POLLRDNORM;
+		ppp_file_release(&pch->file);
 	}
 	return mask;
 }
@@ -2221,6 +2243,7 @@
 	}
 
 	list_add(&ppp->file.list, list->prev);
+	MOD_INC_USE_COUNT;
  out:
 	spin_unlock(&all_ppp_lock);
 	*retp = ret;
@@ -2286,6 +2309,7 @@
 		kfree(ppp);
 
 	spin_unlock(&all_ppp_lock);
+	MOD_DEC_USE_COUNT;
 }
 
 /*
@@ -2407,6 +2431,7 @@
 	skb_queue_purge(&pch->file.xq);
 	skb_queue_purge(&pch->file.rq);
 	kfree(pch);
+	MOD_DEC_USE_COUNT;
 }
 
 static void __exit ppp_cleanup(void)

Re: PATCH against 2.4.2: TTY hangup on PPP channel corrupts kernel memory

[Paul Mackerras]

Kevin Buhr writes:

> If there's a hangup in the TTY layer on an async PPP channel,
> do_tty_hangup shuts down the PPP line discipline, and, in ppp_async.c,
> the function ppp_asynctty_close unregisteres the channel.  In
> ppp_generic.c, ppp_unregister_channel merrily wakes up the rwait
> queue, then proceeds to destroy the channel, freeing the "struct
> channel" which contains the "struct ppp_file" that contains the
> "wait_queue_head_t rwait".  When the waiting process wakes up, it
> removes itself from the wait queue, modifying freed memory.

But the waiting process must have had an instance of /dev/ppp open and
attached to the channel in order to be doing anything with rwait,
within either ppp_file_read or ppp_poll.  The process of attaching to
the channel increases its refcnt, meaning that the channel shouldn't
be destroyed until the instance of /dev/ppp is closed and ppp_release
is called.

Note that pppd will not be blocking inside ppp_file_read since it sets
the file descriptor non-blocking.  Most of the time pppd would be
inside a select, so rwait would be in use by the poll/select code.

I presume that the generic file descriptor code ensures that the file
release function doesn't get called while any task is inside the read
or write function for that file, or while the file descriptor is in
use in a select or poll.  If that assumption is wrong then it would
indeed be possible for the channel to be destroyed while some process
is waiting on rwait.  But in any case it shouldn't be a problem in
practice since it would only be pppd that would have the channel open
and pppd is single-threaded, i.e. it couldn't be closing the file
descriptor while it is blocked inside read or select.

So, to put it in other words, this is the sequence (simplified):

	fd = open("/dev/ppp", O_RDWR);
	ioctl(fd, PPPIOCATTCHAN, &channel_number);
	fcntl(fd, F_SETFL, fcntl(fd, F_GETFL) | O_NONBLOCK);

	select(...);	/* fd_sets including fd */
	read(fd, ...);
	...
	close(fd);

I believe the channel structure is guaranteed to exist from the ioctl
to the close, and all the selects and reads (i.e. all the uses of
rwait) have to happen within that time interval.

> A patch against 2.4.2 follows.  I've overloaded the "refcnt" in
> "struct ppp_file" to also keep track of rwaiters.  The last refcnt
> user destroys the channel and decreases the module use count.  I've
> tested this with printks in all the right places, and it seems to fix
> the problem correctly.

I'm not sure this is the right fix, this sounds to me like the
refcounts are going awry somehow or there is an SMP race that I
haven't considered, and I am concerned that this patch will just cover
over the real problem.  Actually, given that you've seen it 4 times in
6 months it's more likely that it is an SMP race IMHO.

In any case I don't think your patch does the right thing with
ppp_poll, because poll_wait doesn't actually wait, it just adds rwait
to a list of things to watch for wakeups.  In other words, rwait will
be in use from the time poll_wait is called until the time that the
poll/select logic (in fs/select.c) decides that it's time to return to
the user.  So increasing the refcount around just the poll_wait call
won't help much.

Do you have a way to reproduce the problem at will?  Have you seen it
happen on a UP box (i.e. could it be an SMP race)?  How sure are you
that your patch really fixes the problem?

Regards,
Paul.

-- 
Paul Mackerras, Open Source Research Fellow, Linuxcare, Inc.
+61 2 6262 8990 tel, +61 2 6262 8991 fax
paulus@linuxcare.com.au, http://www.linuxcare.com.au/
Linuxcare.  Putting Open Source to work.

Re: PATCH against 2.4.2: TTY hangup on PPP channel corrupts kernel memory

[Kevin Buhr]

Paul Mackerras <paulus@linuxcare.com.au> writes:
> 
> But the waiting process must have had an instance of /dev/ppp open and
> attached to the channel in order to be doing anything with rwait,
> within either ppp_file_read or ppp_poll.  The process of attaching to
> the channel increases its refcnt, meaning that the channel shouldn't
> be destroyed until the instance of /dev/ppp is closed and ppp_release
> is called.

Ugh...  I duplicated the hangs and verified my patch fixed them under
kernel 2.4.0 with "pppd" 2.3.11; and I also verified that kernel 2.4.2
with my patch and "pppd" 2.4.0f correctly deferred channel destruction
on modem hangup.  However, I forget to double-check that the hangs
were actually still possible with 2.4.2 and "pppd" 2.4.0f.

I didn't realize my specific hang was a peculiarity of the older
attachment style.  The channel created by pushing the PPP line
discipline onto a TTY was connected to a unit with a PPPIOCATTACH
ioctl on the TTY---this didn't really "attach" the channel; it still
had a refcnt of only one.  Through the old compatibility interface, it
was possible to call ppp_asynctty_read -> ppp_channel_read -> ppp_read
on the channel's "struct ppp_file" and wait on the channel's "rwait".
If the modem hung up, "do_tty_hangup" would call "ppp_asynctty_close"
(with a reader still in "ppp_asynctty_read") and the "struct channel"
would be freed in "ppp_unregister_channel".

I think your analysis of how things presently are with 2.4.2 and a
modern "pppd" is correct...

Since the new "pppd" uses an explicit PPPIOCATTCHAN / PPPIOCCONNECT
sequence, the refcnt gets bumped to 2 and stays there while the
channel is attached.  So, this specific hang isn't a problem anymore
for "ppp_async.c".  It's still a problem with "ppp_synctty.c", though
(when used with "pppd" 2.3.11, say).  Is the compatibility stuff in
there slated for removal, too?

> I presume that the generic file descriptor code ensures that the file
> release function doesn't get called while any task is inside the read
> or write function for that file, or while the file descriptor is in
> use in a select or poll.

It's not the file "release" function that's the problem.  It's the
line discipline's "close" call.  On a real hardware hangup, The kernel
thread "eventd" calls "do_tty_hangup" which grabs the big kernel lock
and calls ppp_asynctty_close.  I believe any line discipline or
"/dev/ppp" file function that sleeps (and so gives up its big kernel
lock) with refcnt == 1 is susceptible to having the "struct channel"
pulled out from under it.

In particular, the comment above "ppp_asynctty_close" is misleading.
It's true that the TTY layer won't call any further line discipline
entries while the "close" is executing; however, there may be
processes already sleeping in line discipline functions called before
the hangup.  For example, "ppp_asynctty_close" could be called while
we sleep in the "get_user" in "ppp_channel_ioctl" (called from
"ppp_asynctty_ioctl").  Therefore, calling "PPPIOCATTACH" on an
unattached PPP-disciplined TTY could, in unlikely circumstances
(argument swapped out), lead to a crash.

In fact, I think the only remaining PPP locking problem in
"ppp_async.c" still in 2.4.2 has to do with those PPPIOCATTACH/DETACH
ioctls for the TTY.  They seem to be the only way someone can
reference the "struct channel" of an unattached channel.

I assume PPPIOCATTACH (on the TTY) is deprecated in favor of
PPPIOCATTCHAN / PPPIOCCONNECT (on the "/dev/ppp" handle).  Can we
eliminate "ppp_channel_ioctl" from "ppp_async.c" entirely, as in the
patch below?  We're requiring people to upgrade to "pppd" 2.4.0
anyway, and it has no need for these calls.  This would give me a warm,
fuzzy feeling.

Kevin <buhr@stat.wisc.edu>

                        *       *       *

--- linux-2.4.2/drivers/net/ppp_async.c	Sun Mar  4 20:09:19 2001
+++ linux-2.4.2-local/drivers/net/ppp_async.c	Sat Mar 17 20:11:45 2001
@@ -244,11 +244,6 @@
 		err = 0;
 		break;
 
-	case PPPIOCATTACH:
-	case PPPIOCDETACH:
-		err = ppp_channel_ioctl(&ap->chan, cmd, arg);
-		break;
-
 	default:
 		err = -ENOIOCTLCMD;
 	}

Re: PATCH against 2.4.2: TTY hangup on PPP channel corrupts kernel memory

[Paul Mackerras]

Kevin Buhr writes:

> I didn't realize my specific hang was a peculiarity of the older
> attachment style.  The channel created by pushing the PPP line

I didn't realize you were talking about linux 2.4.0 and pppd 2.3.11.

> discipline onto a TTY was connected to a unit with a PPPIOCATTACH
> ioctl on the TTY---this didn't really "attach" the channel; it still
> had a refcnt of only one.  Through the old compatibility interface, it
> was possible to call ppp_asynctty_read -> ppp_channel_read -> ppp_read
> on the channel's "struct ppp_file" and wait on the channel's "rwait".
> If the modem hung up, "do_tty_hangup" would call "ppp_asynctty_close"
> (with a reader still in "ppp_asynctty_read") and the "struct channel"
> would be freed in "ppp_unregister_channel".

That's one of the main reasons why I removed the compatibility
stuff. :)

> I think your analysis of how things presently are with 2.4.2 and a
> modern "pppd" is correct...
> 
> Since the new "pppd" uses an explicit PPPIOCATTCHAN / PPPIOCCONNECT
> sequence, the refcnt gets bumped to 2 and stays there while the
> channel is attached.  So, this specific hang isn't a problem anymore
> for "ppp_async.c".  It's still a problem with "ppp_synctty.c", though
> (when used with "pppd" 2.3.11, say).  Is the compatibility stuff in
> there slated for removal, too?

Yep, and we should take out the stuff in ppp_generic.c that was called
by the compatibility stuff in the channels, too.

> In particular, the comment above "ppp_asynctty_close" is misleading.
> It's true that the TTY layer won't call any further line discipline
> entries while the "close" is executing; however, there may be
> processes already sleeping in line discipline functions called before
> the hangup.  For example, "ppp_asynctty_close" could be called while
> we sleep in the "get_user" in "ppp_channel_ioctl" (called from
> "ppp_asynctty_ioctl").  Therefore, calling "PPPIOCATTACH" on an
> unattached PPP-disciplined TTY could, in unlikely circumstances
> (argument swapped out), lead to a crash.

Yuck.  I don't see that we can protect against this without having
some sort of lock in the tty structure, though.  We can't protect the
existence of the channel structure with a lock inside that structure.
Ideally the necessary protection would be provided at the tty level.

> I assume PPPIOCATTACH (on the TTY) is deprecated in favor of
> PPPIOCATTCHAN / PPPIOCCONNECT (on the "/dev/ppp" handle).  Can we
> eliminate "ppp_channel_ioctl" from "ppp_async.c" entirely, as in the
> patch below?  We're requiring people to upgrade to "pppd" 2.4.0
> anyway, and it has no need for these calls.  This would give me a warm,
> fuzzy feeling.

Sure, that would be fine.  I'll make up a patch and send it to Linus.

Paul.

Re: PATCH against 2.4.2: TTY hangup on PPP channel corrupts kernel memory

[Kevin Buhr]

Paul Mackerras <paulus@samba.org> writes:
> 
> I didn't realize you were talking about linux 2.4.0 and pppd 2.3.11.

That was my stupid oversight.  I carefully tested and retested the
patch under 2.4.0-test5, then ported it to 2.4.2 and sent it off with
only a cursory check using the new pppd [smack forehead here].

> > In particular, the comment above "ppp_asynctty_close" is misleading.
> > It's true that the TTY layer won't call any further line discipline
> > entries while the "close" is executing; however, there may be
> > processes already sleeping in line discipline functions called before
> > the hangup.  For example, "ppp_asynctty_close" could be called while
> > we sleep in the "get_user" in "ppp_channel_ioctl" (called from
> > "ppp_asynctty_ioctl").  Therefore, calling "PPPIOCATTACH" on an
> > unattached PPP-disciplined TTY could, in unlikely circumstances
> > (argument swapped out), lead to a crash.
> 
> Yuck.  I don't see that we can protect against this without having
> some sort of lock in the tty structure, though.  We can't protect the
> existence of the channel structure with a lock inside that structure.
> Ideally the necessary protection would be provided at the tty level.

Well, the closer I look at line discipline locking the less I think I
understand it.  I can't even see what prevents an "ldisc.close"
function from being called when an "ldisc.open" is sleeping on a
memory allocation.

Can someone help me understand?

When changing line disciplines, "sys_ioctl" gets the big kernel lock
for us, and the "tty_set_ldisc" function doesn't get any additional
locks.  It just calls the line discipline "open" function.

Suppose, at this point, the modem hangs up.  From a hardware
interrupt, "tty_hangup" is called which schedule_tasks the tq_hangup
routine, "do_tty_hangup".

Now, suppose the line discipline "open" function doesn't do any
special locking and has a harmless-looking "kmalloc" that isn't
GPF_ATOMIC.  It falls asleep and gives up the big kernel lock!!

Now, the eventd kernel thread wakes up and runs "do_tty_hangup".
"do_tty_hangup" has no trouble getting the big kernel lock and running
the "flush_buffer", "write_wakeup", and "close" line discipline
function on the half-initialized line discipline all with no further
locking.  In a naive implementation, "close" would start freeing the
same kernel structures that "open" hasn't had a chance to allocate!
And, now, "open" is free to wake up and try allocating structures for
a line discipline that has already been shutdown from the TTY.

Does this mean that all line discipline implementations must use a
spinlock around critical code in "open", "close", and every other line
discipline function?  It looks like they must, and it looks like most
don't right now.

Maybe I'm just overlooking something obvious.

> >                                                            Can we
> > eliminate "ppp_channel_ioctl" from "ppp_async.c" entirely, as in the
> > patch below?  We're requiring people to upgrade to "pppd" 2.4.0
> > anyway, and it has no need for these calls.  This would give me a warm,
> > fuzzy feeling.
> 
> Sure, that would be fine.  I'll make up a patch and send it to Linus.

Thank you.

Kevin <buhr@stat.wisc.edu>

Re: PATCH against 2.4.2: TTY hangup on PPP channel corrupts kernel memory

[Andrew Morton]

Kevin Buhr wrote:
> 
> ...
> When changing line disciplines, "sys_ioctl" gets the big kernel lock
> for us, and the "tty_set_ldisc" function doesn't get any additional
> locks.  It just calls the line discipline "open" function.
> 
> Suppose, at this point, the modem hangs up.  From a hardware
> interrupt, "tty_hangup" is called which schedule_tasks the tq_hangup
> routine, "do_tty_hangup".
> 
> Now, suppose the line discipline "open" function doesn't do any
> special locking and has a harmless-looking "kmalloc" that isn't
> GPF_ATOMIC.  It falls asleep and gives up the big kernel lock!!
> 
> Now, the eventd kernel thread wakes up and runs "do_tty_hangup".
> "do_tty_hangup" has no trouble getting the big kernel lock and running
> the "flush_buffer", "write_wakeup", and "close" line discipline
> function on the half-initialized line discipline all with no further
> locking.  In a naive implementation, "close" would start freeing the
> same kernel structures that "open" hasn't had a chance to allocate!
> And, now, "open" is free to wake up and try allocating structures for
> a line discipline that has already been shutdown from the TTY.

Your analysis is correct.  It's a bug.

Furthermore, n_hdlc_tty_open() (for example) can sleep prior to
incrementing the module refcount, which means the module can be
unloaded while it's running.  I cut a patch ages ago which fixes
this one for both ttys and ldiscs.  I never got around to sending
it to anyone.

> Does this mean that all line discipline implementations must use a
> spinlock around critical code in "open", "close", and every other line
> discipline function?  It looks like they must, and it looks like most
> don't right now.
> 

Seems a semaphore would be adequate.  Adding locking to the
tty code can be tricky, because it likes to copy big structures
around by value, rather than by reference.  You can end up
accidentally overwriting your locks.  I'm not sure why the
code was done this way.

Here's the old tty+ldisc module safety patch.  I've added the
ldisc locking.  Does it look to you like it'll fix the race
you identify?  Note that only one ldisc (ppp_async) and one
tty driver (serial) have been edited to actually use the new
module handling. It's trivial to change the other ldiscs and
tty drivers.

I think the scenario you describe could happen with the 
tty_struct.driver handling, as well as with tty_struct.ldisc.

This patch isn't ready to roll yet, BTW.  It needs a few hours
staring, thinking and testing.  Mainly checking whether all
scenarios are covered.  Hacking the tty layer is not 
exactly a walk in the park....


--- linux-2.4.3-pre6/include/linux/tty_driver.h	Tue Jan 30 18:24:56 2001
+++ linux-akpm/include/linux/tty_driver.h	Fri Mar 23 21:20:36 2001
@@ -117,6 +117,14 @@
 
 #include <linux/fs.h>
 
+#ifdef CONFIG_MODULES
+#include <linux/module.h>
+#define SET_TTY_OWNER(driver)	\
+	do { (driver)->owner = THIS_MODULE; } while (0)
+#define SET_LDISC_OWNER(ldisc)	\
+	do { (ldisc)->owner = THIS_MODULE; } while (0)
+#endif
+
 struct tty_driver {
 	int	magic;		/* magic number for this structure */
 	const char	*driver_name;
@@ -176,6 +184,9 @@
 	 */
 	struct tty_driver *next;
 	struct tty_driver *prev;
+#ifdef CONFIG_MODULES
+	struct module *owner;
+#endif
 };
 
 /* tty driver magic number */
--- linux-2.4.3-pre6/include/linux/tty_ldisc.h	Tue Jan 30 18:24:56 2001
+++ linux-akpm/include/linux/tty_ldisc.h	Fri Mar 23 21:20:36 2001
@@ -100,6 +100,8 @@
 #include <linux/fs.h>
 #include <linux/wait.h>
 
+struct module;
+
 struct tty_ldisc {
 	int	magic;
 	char	*name;
@@ -129,6 +131,9 @@
 			       char *fp, int count);
 	int	(*receive_room)(struct tty_struct *);
 	void	(*write_wakeup)(struct tty_struct *);
+#ifdef CONFIG_MODULES
+	struct module *owner;
+#endif
 };
 
 #define TTY_LDISC_MAGIC	0x5403
--- linux-2.4.3-pre6/include/linux/tty.h	Tue Jan 30 18:24:56 2001
+++ linux-akpm/include/linux/tty.h	Fri Mar 23 21:29:32 2001
@@ -306,6 +306,7 @@
 	unsigned int canon_column;
 	struct semaphore atomic_read;
 	struct semaphore atomic_write;
+	struct semaphore ldisc_sem;	/* Lock this while we're manipulating ldisc */
 	spinlock_t read_lock;
 };
 
--- linux-2.4.3-pre6/drivers/char/tty_io.c	Sun Feb 25 17:37:04 2001
+++ linux-akpm/drivers/char/tty_io.c	Fri Mar 23 21:39:11 2001
@@ -182,6 +182,67 @@
 }
 
 /*
+ * Lock a driver's module into core and increment its low-level refcount.
+ * Return 0 on success.  If we return non-zero then the driver module isn't there
+ * any more and action needs to be taken by the caller
+ */
+static int tty_dev_hold(struct tty_driver *driver)
+{
+#ifdef CONFIG_MODULES
+	if (driver->owner) {
+		if (try_inc_mod_count(driver->owner) == 0)
+			return -ENODEV;	/* Module is deleted */
+	}
+#endif
+	(*driver->refcount)++;
+	return 0;
+}
+
+/*
+ * Release the driver and (possibly) its module
+ */
+static void tty_dev_put(struct tty_driver *driver)
+{
+#ifdef CONFIG_MODULES
+	if (driver->owner)
+		__MOD_DEC_USE_COUNT(driver->owner);
+#endif
+	(*driver->refcount)--;
+}
+
+/*
+ * Lock an ldisc's module into core.  Return non-zero if the
+ * containing module is unusable - remedial action is needed
+ * by the caller
+ */
+static int tty_ldisc_hold(struct tty_ldisc *ldisc)
+{
+#ifdef CONFIG_MODULES
+	if (try_inc_mod_count(ldisc->owner) == 0)
+		return -ENODEV;	/* Module is deleted */
+	return 0;
+#endif
+}
+
+/*
+ * Release an ldisc and (possibly) its module
+ */
+static void tty_ldisc_put(struct tty_ldisc *ldisc)
+{
+#ifdef CONFIG_MODULES
+	if (ldisc->owner) {
+		int uc;
+		__MOD_DEC_USE_COUNT(ldisc->owner);
+		uc = atomic_read(&(ldisc->owner)->uc.usecount);
+		if (uc < 0) {
+			printk("tty_ldisc_put: count=%d.  This is not good\n",
+				atomic_read(&(ldisc->owner)->uc.usecount));
+		}
+	}
+#endif
+}
+
+/*
  * This routine returns the name of tty.
  */
 static char *
@@ -270,12 +331,15 @@
 /* Set the discipline of a tty line. */
 static int tty_set_ldisc(struct tty_struct *tty, int ldisc)
 {
-	int	retval = 0;
+	int	retval;
 	struct	tty_ldisc o_ldisc;
 	char buf[64];
 
-	if ((ldisc < N_TTY) || (ldisc >= NR_LDISCS))
-		return -EINVAL;
+	down(&tty->ldisc_sem);	/* We're changing the ldisc.  Get exclusive access */
+	if ((ldisc < N_TTY) || (ldisc >= NR_LDISCS)) {
+		retval = -EINVAL;
+		goto out;
+	}
 	/* Eduardo Blanco <ejbs@cs.cs.com.uy> */
 	/* Cyrus Durgin <cider@speakeasy.org> */
 	if (!(ldiscs[ldisc].flags & LDISC_FLAG_DEFINED)) {
@@ -283,11 +347,14 @@
 		sprintf(modname, "tty-ldisc-%d", ldisc);
 		request_module (modname);
 	}
-	if (!(ldiscs[ldisc].flags & LDISC_FLAG_DEFINED))
-		return -EINVAL;
-
-	if (tty->ldisc.num == ldisc)
-		return 0;	/* We are already in the desired discipline */
+	if (!(ldiscs[ldisc].flags & LDISC_FLAG_DEFINED)) {
+		retval = -EINVAL;
+		goto out;
+	}
+	if (tty->ldisc.num == ldisc) {
+		retval = 0;	/* We are already in the desired discipline */
+		goto out;
+	}
 	o_ldisc = tty->ldisc;
 
 	tty_wait_until_sent(tty, 0);
@@ -296,15 +363,24 @@
 	if (tty->ldisc.close)
 		(tty->ldisc.close)(tty);
 
+	/* Don't do tty_ldisc_put(&o_ldisc) - we may need it later */
+
 	/* Now set up the new line discipline. */
 	tty->ldisc = ldiscs[ldisc];
 	tty->termios->c_line = ldisc;
-	if (tty->ldisc.open)
+
+	retval = tty_ldisc_hold(&tty->ldisc);
+
+	if (retval == 0 && tty->ldisc.open)
 		retval = (tty->ldisc.open)(tty);
+
 	if (retval < 0) {
+		tty_ldisc_put(&tty->ldisc);
+		tty_ldisc_hold(&o_ldisc);
 		tty->ldisc = o_ldisc;
 		tty->termios->c_line = tty->ldisc.num;
 		if (tty->ldisc.open && (tty->ldisc.open(tty) < 0)) {
+			tty_ldisc_put(&tty->ldisc);
 			tty->ldisc = ldiscs[N_TTY];
 			tty->termios->c_line = N_TTY;
 			if (tty->ldisc.open) {
@@ -317,8 +393,11 @@
 			}
 		}
 	}
+	tty_ldisc_put(&o_ldisc);	/* Do it now */
 	if (tty->ldisc.num != o_ldisc.num && tty->driver.set_ldisc)
 		tty->driver.set_ldisc(tty);
+out:
+	up(&tty->ldisc_sem);
 	return retval;
 }
 
@@ -460,8 +539,9 @@
 		filp->f_op = &hung_up_tty_fops;
 	}
 	file_list_unlock();
-	
-	/* FIXME! What are the locking issues here? This may me overdoing things.. */
+
+	/* Pin down tty->ldisc to avoid racing with tty_set_ldisc */
+	down(&tty->ldisc_sem);
 	{
 		unsigned long flags;
 
@@ -488,6 +568,7 @@
 	if (tty->ldisc.num != ldiscs[N_TTY].num) {
 		if (tty->ldisc.close)
 			(tty->ldisc.close)(tty);
+		tty_ldisc_put(&tty->ldisc);
 		tty->ldisc = ldiscs[N_TTY];
 		tty->termios->c_line = N_TTY;
 		if (tty->ldisc.open) {
@@ -497,6 +578,7 @@
 				       -i);
 		}
 	}
+	up(&tty->ldisc_sem);
 	
 	read_lock(&tasklist_lock);
  	for_each_task(p) {
@@ -901,7 +983,8 @@
 			*o_ltp_loc = o_ltp;
 		o_tty->termios = *o_tp_loc;
 		o_tty->termios_locked = *o_ltp_loc;
-		(*driver->other->refcount)++;
+		if (tty_dev_hold(driver->other) != 0)
+			goto free_mem_out;
 		if (driver->subtype == PTY_TYPE_MASTER)
 			o_tty->count++;
 
@@ -910,6 +993,9 @@
 		o_tty->link = tty;
 	}
 
+	if (tty_dev_hold(driver) != 0)
+		goto free_mem_out;
+
 	/* 
 	 * All structures have been allocated, so now we install them.
 	 * Failures after this point use release_mem to clean up, so 
@@ -923,7 +1009,6 @@
 		*ltp_loc = ltp;
 	tty->termios = *tp_loc;
 	tty->termios_locked = *ltp_loc;
-	(*driver->refcount)++;
 	tty->count++;
 
 	/* 
@@ -1020,7 +1105,7 @@
 			kfree(tp);
 		}
 		o_tty->magic = 0;
-		(*o_tty->driver.refcount)--;
+		tty_dev_put(&o_tty->driver);
 		free_tty_struct(o_tty);
 	}
 
@@ -1031,7 +1116,7 @@
 		kfree(tp);
 	}
 	tty->magic = 0;
-	(*tty->driver.refcount)--;
+	tty_dev_put(&tty->driver);
 	free_tty_struct(tty);
 }
 
@@ -1248,11 +1333,13 @@
 	 */
 	if (tty->ldisc.close)
 		(tty->ldisc.close)(tty);
+	tty_ldisc_put(&tty->ldisc);
 	tty->ldisc = ldiscs[N_TTY];
 	tty->termios->c_line = N_TTY;
 	if (o_tty) {
 		if (o_tty->ldisc.close)
 			(o_tty->ldisc.close)(o_tty);
+		tty_ldisc_put(&o_tty->ldisc);
 		o_tty->ldisc = ldiscs[N_TTY];
 	}
 	
@@ -1971,6 +2058,7 @@
 	tty->tq_hangup.data = tty;
 	sema_init(&tty->atomic_read, 1);
 	sema_init(&tty->atomic_write, 1);
+	sema_init(&tty->ldisc_sem, 1);
 	spin_lock_init(&tty->read_lock);
 	INIT_LIST_HEAD(&tty->tty_files);
 }
--- linux-2.4.3-pre6/drivers/char/serial.c	Thu Mar 22 18:52:47 2001
+++ linux-akpm/drivers/char/serial.c	Fri Mar 23 21:20:36 2001
@@ -209,6 +209,14 @@
 #include <linux/sysrq.h>
 #endif
 
+#ifdef SET_TTY_OWNER
+#define TTY_MOD_INC do {} while (0)
+#define TTY_MOD_DEC do {} while (0)
+#else
+#define TTY_MOD_INC	MOD_INC_USE_COUNT
+#define TTY_MOD_DEC	MOD_DEC_USE_COUNT
+#endif
+
 /*
  * All of the compatibilty code so we can compile serial.c against
  * older kernels is hidden in serial_compat.h
@@ -2760,7 +2768,7 @@
 	
 	if (tty_hung_up_p(filp)) {
 		DBG_CNT("before DEC-hung");
-		MOD_DEC_USE_COUNT;
+		TTY_MOD_DEC;
 		restore_flags(flags);
 		return;
 	}
@@ -2787,7 +2795,7 @@
 	}
 	if (state->count) {
 		DBG_CNT("before DEC-2");
-		MOD_DEC_USE_COUNT;
+		TTY_MOD_DEC;
 		restore_flags(flags);
 		return;
 	}
@@ -2843,7 +2851,7 @@
 	info->flags &= ~(ASYNC_NORMAL_ACTIVE|ASYNC_CALLOUT_ACTIVE|
 			 ASYNC_CLOSING);
 	wake_up_interruptible(&info->close_wait);
-	MOD_DEC_USE_COUNT;
+	TTY_MOD_DEC;
 }
 
 /*
@@ -3126,21 +3134,21 @@
 	int 			retval, line;
 	unsigned long		page;
 
-	MOD_INC_USE_COUNT;
+	TTY_MOD_INC;
 	line = MINOR(tty->device) - tty->driver.minor_start;
 	if ((line < 0) || (line >= NR_PORTS)) {
-		MOD_DEC_USE_COUNT;
+		TTY_MOD_DEC;
 		return -ENODEV;
 	}
 	retval = get_async_struct(line, &info);
 	if (retval) {
-		MOD_DEC_USE_COUNT;
+		TTY_MOD_DEC;
 		return retval;
 	}
 	tty->driver_data = info;
 	info->tty = tty;
 	if (serial_paranoia_check(info, tty->device, "rs_open")) {
-		MOD_DEC_USE_COUNT;		
+		TTY_MOD_DEC;		
 		return -ENODEV;
 	}
 
@@ -3155,7 +3163,7 @@
 	if (!tmp_buf) {
 		page = get_zeroed_page(GFP_KERNEL);
 		if (!page) {
-			MOD_DEC_USE_COUNT;
+			TTY_MOD_DEC;
 			return -ENOMEM;
 		}
 		if (tmp_buf)
@@ -3171,7 +3179,7 @@
 	    (info->flags & ASYNC_CLOSING)) {
 		if (info->flags & ASYNC_CLOSING)
 			interruptible_sleep_on(&info->close_wait);
-		MOD_DEC_USE_COUNT;
+		TTY_MOD_DEC;
 #ifdef SERIAL_DO_RESTART
 		return ((info->flags & ASYNC_HUP_NOTIFY) ?
 			-EAGAIN : -ERESTARTSYS);
@@ -3185,7 +3193,7 @@
 	 */
 	retval = startup(info);
 	if (retval) {
-		MOD_DEC_USE_COUNT;
+		TTY_MOD_DEC;
 		return retval;
 	}
 
@@ -3195,7 +3203,7 @@
 		printk("rs_open returning after block_til_ready with %d\n",
 		       retval);
 #endif
-		MOD_DEC_USE_COUNT;
+		TTY_MOD_DEC;
 		return retval;
 	}
 
@@ -5254,7 +5262,9 @@
 	serial_driver.wait_until_sent = rs_wait_until_sent;
 	serial_driver.read_proc = rs_read_proc;
 #endif
-	
+#ifdef SET_TTY_OWNER
+	SET_TTY_OWNER(&serial_driver);
+#endif
 	/*
 	 * The callout device is just like normal device except for
 	 * major number and the subtype code.
@@ -5495,12 +5505,16 @@
 	del_timer_sync(&serial_timer);
 	save_flags(flags); cli();
         remove_bh(SERIAL_BH);
-	if ((e1 = tty_unregister_driver(&serial_driver)))
+	if ((e1 = tty_unregister_driver(&serial_driver))) {
 		printk("serial: failed to unregister serial driver (%d)\n",
 		       e1);
-	if ((e2 = tty_unregister_driver(&callout_driver)))
+		BUG();
+	}
+	if ((e2 = tty_unregister_driver(&callout_driver))) {
 		printk("serial: failed to unregister callout driver (%d)\n", 
 		       e2);
+		BUG();
+	}
 	restore_flags(flags);
 
 	for (i = 0; i < NR_PORTS; i++) {
--- linux-2.4.3-pre6/drivers/net/ppp_async.c	Sun Feb 25 17:37:06 2001
+++ linux-akpm/drivers/net/ppp_async.c	Fri Mar 23 21:20:36 2001
@@ -113,7 +113,6 @@
 	struct asyncppp *ap;
 	int err;
 
-	MOD_INC_USE_COUNT;
 	err = -ENOMEM;
 	ap = kmalloc(sizeof(*ap), GFP_KERNEL);
 	if (ap == 0)
@@ -146,7 +145,6 @@
  out_free:
 	kfree(ap);
  out:
-	MOD_DEC_USE_COUNT;
 	return err;
 }
 
@@ -171,7 +169,6 @@
 	if (ap->tpkt != 0)
 		kfree_skb(ap->tpkt);
 	kfree(ap);
-	MOD_DEC_USE_COUNT;
 }
 
 /*
@@ -310,6 +307,9 @@
 	receive_room: ppp_asynctty_room,
 	receive_buf: ppp_asynctty_receive,
 	write_wakeup: ppp_asynctty_wakeup,
+#ifdef CONFIG_MODULES
+	owner: THIS_MODULE,
+#endif
 };
 
 int

Re: PATCH against 2.4.2: TTY hangup on PPP channel corrupts kernel memory

[Paul Fulghum]

From: "Andrew Morton" <andrewm@uow.edu.au>

> Your analysis is correct.  It's a bug.
> 
> Furthermore, n_hdlc_tty_open() (for example) can sleep prior to
> incrementing the module refcount, which means the module can be
> unloaded while it's running.  I cut a patch ages ago which fixes
> this one for both ttys and ldiscs.  I never got around to sending
> it to anyone.
> 
> > Does this mean that all line discipline implementations must use a
> > spinlock around critical code in "open", "close", and every other line
> > discipline function?  It looks like they must, and it looks like most
> > don't right now.

I have experienced essentially the same problem:
A line discipline can be switched while a user mode program is blocked
inside of a line discipline call.

In my case the call was ioctl() (select) which went through the ldisc
(n_hdlc) and was being serviced by (and blocked in) the tty layer. 

Two processes had the underlying serial device open. One process
restored the ldisc to N_TTY, exited, and the script that started
the process unloaded the ldisc driver (which had
a zero ref count as a result of being switched out).
When the select call of the other process tried to return
(to the n_hdlc ldisc), the code was already unloaded and an
oops occurred.

I was not too worried about this because it was caused by
a series of wrong (buggy) moves by the user mode processes.

But it goes back to the problem of allowing the ldisc to
change when there are existing calls blocked in (or through)
the ldisc. 

Paul Fulghum paulkf@microgate.com
Microgate Corporation www.microgate.com