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)
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
This archive was generated by hypermail 2b29 : Fri Mar 23 2001 - 21:00:09 EST