Thanks for all the test reports on the ppp patch. For the most part it's working
OK, but I did get one report of an error in the PPPIOCSNPMODE ioctl code, and
there was one copy_xx_user that I forget to fix. I've made some changes that
should fix that problem.
I've also made a few additional changes to set pointers to NULL after freeing a
structure, and to make the more serious error messages not depend on the
SC_DEBUG flag. The particular messages look unlikely, but if it's serios enough
to be tagged KERN_ERR, I think it should get logged.
Please continue testing and report any problems.
Regards,
Bill
--------------43578C18B690E38278FCD280
Content-Type: text/plain; charset=us-ascii; name="ppp_81-patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline; filename="ppp_81-patch"
--- linux-2.1.81/drivers/net/ppp.c.old Tue Dec 30 16:08:58 1997
+++ linux-2.1.81/drivers/net/ppp.c Sun Jan 25 10:44:43 1998
@@ -7,7 +7,7 @@
* Dynamic PPP devices by Jim Freeman <jfree@caldera.com>.
* ppp_tty_receive ``noisy-raise-bug'' fixed by Ove Ewerlid <ewerlid@syscon.uu.se>
*
- * ==FILEVERSION 971205==
+ * ==FILEVERSION 980123==
*
* NOTE TO MAINTAINERS:
* If you modify this file at all, please set the number above to the
@@ -568,9 +568,7 @@
*/
if (new_wbuf == NULL || new_tbuf == NULL ||
new_rbuf == NULL || new_cbuf == NULL) {
- if (ppp->flags & SC_DEBUG)
- printk (KERN_ERR
- "ppp: failed to allocate new buffers\n");
+ printk (KERN_ERR "ppp: failed to allocate new buffers\n");
ppp_free_buf (new_wbuf);
ppp_free_buf (new_tbuf);
@@ -770,8 +768,7 @@
* There should not be an existing table for this slot.
*/
if (ppp) {
- if (ppp->flags & SC_DEBUG)
- printk (KERN_ERR
+ printk (KERN_ERR
"ppp_tty_open: gack! tty already associated to %s!\n",
ppp->magic == PPP_MAGIC ? ppp2dev(ppp)->name
: "unknown");
@@ -793,8 +790,7 @@
} else {
ppp = ppp_alloc();
if (ppp == NULL) {
- if (ppp->flags & SC_DEBUG)
- printk (KERN_ERR "ppp_alloc failed\n");
+ printk (KERN_ERR "ppp_alloc failed\n");
return -ENFILE;
}
/*
@@ -808,9 +804,8 @@
*/
ppp->slcomp = slhc_init (16, 16);
if (ppp->slcomp == NULL) {
- if (ppp->flags & SC_DEBUG)
- printk (KERN_ERR "ppp_tty_open: "
- "no space for compression buffers!\n");
+ printk (KERN_ERR "ppp_tty_open: "
+ "no space for compression buffers!\n");
ppp_release (ppp);
return -ENOMEM;
}
@@ -826,9 +821,8 @@
*/
ppp->ubuf = ppp_alloc_buf (RBUFSIZE, BUFFER_TYPE_TTY_RD);
if (ppp->ubuf == NULL) {
- if (ppp->flags & SC_DEBUG)
- printk (KERN_ERR "ppp_tty_open: "
- "no space for user receive buffer\n");
+ printk (KERN_ERR "ppp_tty_open: "
+ "no space for user receive buffer\n");
ppp_release (ppp);
return -ENOMEM;
}
@@ -1300,9 +1294,7 @@
*/
new_data = kmalloc (ppp->mru + PPP_HDRLEN, GFP_ATOMIC);
if (new_data == NULL) {
- if (ppp->flags & SC_DEBUG)
- printk (KERN_ERR
- "ppp_doframe: no memory\n");
+ printk (KERN_ERR "ppp_doframe: no memory\n");
new_count = DECOMP_ERROR;
} else {
new_count = (*ppp->sc_rcomp->decompress)
@@ -1321,8 +1313,7 @@
case DECOMP_FATALERROR:
ppp->flags |= SC_DC_FERROR;
- if (ppp->flags & SC_DEBUG)
- printk(KERN_ERR "ppp: fatal decomp error\n");
+ printk(KERN_ERR "ppp: fatal decomp error\n");
break;
}
/*
@@ -2069,15 +2060,16 @@
struct ppp *ppp = tty2ppp (tty);
__u8 *new_data;
int error;
+ struct wait_queue wait = {current, NULL};
/*
* Verify the pointers.
*/
+ error = -EIO;
if (!ppp)
- return -EIO;
-
+ goto out;
if (ppp->magic != PPP_MAGIC)
- return -EIO;
+ goto out;
CHECK_PPP (-ENXIO);
/*
@@ -2096,43 +2088,48 @@
*/
new_data = kmalloc (count, GFP_KERNEL);
if (new_data == NULL) {
- if (ppp->flags & SC_DEBUG)
- printk (KERN_ERR
- "ppp_tty_write: no memory\n");
+ printk (KERN_ERR "ppp_tty_write: no memory\n");
return 0;
}
/*
* Retrieve the user's buffer
*/
- error = copy_from_user(new_data, data, count);
- if (error) {
- kfree (new_data);
- return error;
- }
+ error = -EFAULT;
+ if (copy_from_user(new_data, data, count))
+ goto out_free;
/*
- * lock this PPP unit so we will be the only writer;
- * sleep if necessary
+ * Lock this PPP unit so we will be the only writer,
+ * sleeping if necessary.
+ *
+ * Note that we add our task to the wait queue before
+ * attempting to lock, as the lock flag may be cleared
+ * from an interrupt.
*/
- while (lock_buffer (ppp->tbuf) != 0) {
+ add_wait_queue(&ppp->write_wait, &wait);
+ while (1) {
+ error = 0;
current->timeout = 0;
-#if 0
- if (ppp->flags & SC_DEBUG)
- printk (KERN_DEBUG "ppp_tty_write: sleeping\n");
-#endif
- interruptible_sleep_on (&ppp->write_wait);
+ current->state = TASK_INTERRUPTIBLE;
+ if (lock_buffer(ppp->tbuf) == 0)
+ break;
+ schedule();
+ error = -EINVAL;
ppp = tty2ppp (tty);
- if (!ppp || ppp->magic != PPP_MAGIC || !ppp->inuse
- || tty != ppp->tty) {
- kfree (new_data);
- return 0;
- }
-
- if (signal_pending(current)) {
- kfree (new_data);
- return -EINTR;
+ if (!ppp || ppp->magic != PPP_MAGIC ||
+ !ppp->inuse || tty != ppp->tty) {
+ printk("ppp_tty_write: %p invalid after wait!\n", ppp);
+ break;
}
+ error = -EINTR;
+ if (signal_pending(current))
+ break;
}
+ current->state = TASK_RUNNING;
+ remove_wait_queue(&ppp->write_wait, &wait);
+ if (error)
+ goto out_free;
+
/*
* Change the LQR frame
*/
@@ -2152,9 +2149,12 @@
} else {
ppp_dev_xmit_frame (ppp, ppp->tbuf, new_data, count);
}
+ error = count;
+out_free:
kfree (new_data);
- return count;
+out:
+ return error;
}
/*
@@ -2165,31 +2165,30 @@
ppp_set_compression (struct ppp *ppp, struct ppp_option_data *odp)
{
struct compressor *cp;
- struct ppp_option_data data;
- int error;
- int nb;
+ int error, nb;
+ unsigned long flags;
__u8 *ptr;
__u8 ccp_option[CCP_MAX_OPTION_LENGTH];
- unsigned long flags;
+ struct ppp_option_data data;
/*
* Fetch the compression parameters
*/
- error = copy_from_user(&data, odp, sizeof (data));
- if (error != 0)
- return error;
+ error = -EFAULT;
+ if (copy_from_user(&data, odp, sizeof (data)))
+ goto out;
nb = data.length;
ptr = data.ptr;
if ((__u32) nb >= (__u32)CCP_MAX_OPTION_LENGTH)
nb = CCP_MAX_OPTION_LENGTH;
- error = copy_from_user(ccp_option, ptr, nb);
- if (error != 0)
- return error;
+ if (copy_from_user(ccp_option, ptr, nb))
+ goto out;
+ error = -EINVAL;
if (ccp_option[1] < 2) /* preliminary check on the length byte */
- return (-EINVAL);
+ goto out;
save_flags(flags);
cli();
@@ -2206,53 +2205,52 @@
}
#endif /* CONFIG_KERNELD */
- if (cp != (struct compressor *) 0) {
- /*
- * Found a handler for the protocol - try to allocate
- * a compressor or decompressor.
- */
- error = 0;
- if (data.transmit) {
- if (ppp->sc_xc_state != NULL)
- (*ppp->sc_xcomp->comp_free)(ppp->sc_xc_state);
+ if (cp == NULL)
+ goto out_no_comp;
+ /*
+ * Found a handler for the protocol - try to allocate
+ * a compressor or decompressor.
+ */
+ error = 0;
+ if (data.transmit) {
+ if (ppp->sc_xc_state != NULL)
+ (*ppp->sc_xcomp->comp_free)(ppp->sc_xc_state);
+ ppp->sc_xc_state = NULL;
- ppp->sc_xcomp = cp;
- ppp->sc_xc_state = cp->comp_alloc(ccp_option, nb);
+ ppp->sc_xcomp = cp;
+ ppp->sc_xc_state = cp->comp_alloc(ccp_option, nb);
+ if (ppp->sc_xc_state == NULL) {
+ printk(KERN_WARNING "%s: comp_alloc failed\n",
+ ppp->name);
+ error = -ENOBUFS;
+ } else if (ppp->flags & SC_DEBUG)
+ printk(KERN_DEBUG "%s: comp_alloc -> %p\n",
+ ppp->name, ppp->sc_xc_state);
+ } else {
+ if (ppp->sc_rc_state != NULL)
+ (*ppp->sc_rcomp->decomp_free)(ppp->sc_rc_state);
+ ppp->sc_rc_state = NULL;
- if (ppp->sc_xc_state == NULL) {
- if (ppp->flags & SC_DEBUG)
- printk(KERN_DEBUG "%s: comp_alloc failed\n",
- ppp->name);
- error = -ENOBUFS;
- } else {
- if (ppp->flags & SC_DEBUG)
- printk(KERN_DEBUG "%s: comp_alloc -> %p\n",
- ppp->name, ppp->sc_xc_state);
- }
- } else {
- if (ppp->sc_rc_state != NULL)
- (*ppp->sc_rcomp->decomp_free)(ppp->sc_rc_state);
- ppp->sc_rcomp = cp;
- ppp->sc_rc_state = cp->decomp_alloc(ccp_option, nb);
- if (ppp->sc_rc_state == NULL) {
- if (ppp->flags & SC_DEBUG)
- printk(KERN_DEBUG "%s: decomp_alloc failed\n",
- ppp->name);
- error = -ENOBUFS;
- } else {
- if (ppp->flags & SC_DEBUG)
- printk(KERN_DEBUG "%s: decomp_alloc -> %p\n",
- ppp->name, ppp->sc_rc_state);
- }
- }
- return (error);
+ ppp->sc_rcomp = cp;
+ ppp->sc_rc_state = cp->decomp_alloc(ccp_option, nb);
+ if (ppp->sc_rc_state == NULL) {
+ printk(KERN_WARNING "%s: decomp_alloc failed\n",
+ ppp->name);
+ error = -ENOBUFS;
+ } else if (ppp->flags & SC_DEBUG)
+ printk(KERN_DEBUG "%s: decomp_alloc -> %p\n",
+ ppp->name, ppp->sc_rc_state);
}
+out:
+ return error;
+out_no_comp:
+ error = -EINVAL; /* no handler found */
if (ppp->flags & SC_DEBUG)
printk(KERN_DEBUG "%s: no compressor for [%x %x %x], %x\n",
- ppp->name, ccp_option[0], ccp_option[1],
- ccp_option[2], nb);
- return (-EINVAL); /* no handler found */
+ ppp->name, ccp_option[0], ccp_option[1],
+ ccp_option[2], nb);
+ goto out;
}
/*
@@ -2409,16 +2407,20 @@
/* change absolute times to relative times. */
cur_ddinfo.xmit_idle = (jiffies - ppp->last_xmit) / HZ;
cur_ddinfo.recv_idle = (jiffies - ppp->last_recv) / HZ;
- error = copy_to_user((void *) param3, &cur_ddinfo,
- sizeof (cur_ddinfo));
+ error = -EFAULT;
+ if (!copy_to_user((void *) param3, &cur_ddinfo,
+ sizeof (cur_ddinfo)))
+ error = 0;
}
break;
/*
* Retrieve the extended async map
*/
case PPPIOCGXASYNCMAP:
- error = copy_to_user((void *) param3, ppp->xmit_async_map,
- sizeof (ppp->xmit_async_map));
+ error = -EFAULT;
+ if (!copy_to_user((void *) param3, ppp->xmit_async_map,
+ sizeof (ppp->xmit_async_map)))
+ error = 0;
break;
/*
* Set the async extended map
@@ -2427,9 +2429,9 @@
{
__u32 temp_tbl[8];
- error = copy_from_user(temp_tbl, (void *) param3,
- sizeof (temp_tbl));
- if (error != 0)
+ error = -EFAULT;
+ if (copy_from_user(temp_tbl, (void *) param3,
+ sizeof (temp_tbl)))
break;
temp_tbl[1] = 0x00000000;
@@ -2461,16 +2463,15 @@
temp_i = (temp_i & 255) + 1;
if (ppp->flags & SC_DEBUG)
printk (KERN_INFO
- "ppp_tty_ioctl: set maxcid to %d\n",
- temp_i);
+ "ppp_tty_ioctl: set maxcid to %d\n", temp_i);
if (ppp->slcomp != NULL)
slhc_free (ppp->slcomp);
- ppp->slcomp = slhc_init (16, temp_i);
+ ppp->slcomp = NULL;
+ ppp->slcomp = slhc_init (16, temp_i);
if (ppp->slcomp == NULL) {
- if (ppp->flags & SC_DEBUG)
- printk (KERN_ERR
- "ppp: no space for compression buffers!\n");
+ printk (KERN_ERR "ppp_tty_ioctl: "
+ "no space for compression buffers!\n");
ppp_release (ppp);
error = -ENOMEM;
}
@@ -2486,39 +2487,35 @@
{
struct npioctl npi;
- error = copy_from_user(&npi, (void *) param3,
- sizeof (npi));
- if (error != 0)
+ error = -EFAULT;
+ if (copy_from_user(&npi, (void *) param3, sizeof(npi)))
break;
- switch (npi.protocol) {
- case PPP_IP:
- npi.protocol = NP_IP;
- break;
- default:
+ if (npi.protocol != PPP_IP) {
if (ppp->flags & SC_DEBUG)
printk(KERN_DEBUG "pppioc[gs]npmode: "
- "invalid proto %d\n", npi.protocol);
+ "invalid protocol %d\n",
+ npi.protocol);
error = -EINVAL;
- }
-
- if (error != 0)
break;
+ }
+ npi.protocol = NP_IP;
if (param2 == PPPIOCGNPMODE) {
npi.mode = ppp->sc_npmode[npi.protocol];
-
- error = copy_to_user((void *) param3, &npi,
- sizeof (npi));
- break;
+ if (copy_to_user((void *) param3, &npi,
+ sizeof (npi)))
+ break;
}
ppp->sc_npmode[npi.protocol] = npi.mode;
if (ppp->flags & SC_DEBUG)
printk(KERN_DEBUG "ppp: set np %d to %d\n",
npi.protocol, npi.mode);
+ /* N.B. Why is the busy flag cleared here? */
ppp2dev(ppp)->tbusy = 0;
mark_bh(NET_BH);
+ error = 0;
}
break;
/*
@@ -2542,11 +2539,8 @@
*/
default:
if (ppp->flags & SC_DEBUG)
- printk (KERN_ERR
- "ppp_tty_ioctl: invalid ioctl: %x, addr %lx\n",
- param2,
- param3);
-
+ printk (KERN_WARNING "ppp_tty_ioctl: "
+ "invalid ioctl=%x, addr=%lx\n", param2, param3);
error = -ENOIOCTLCMD;
break;
}
@@ -2606,8 +2600,7 @@
struct ppp *ppp = dev2ppp (dev);
if (ppp2tty (ppp) == NULL) {
- if (ppp->flags & SC_DEBUG)
- printk (KERN_ERR
+ printk (KERN_ERR
"ppp: %s not connected to a TTY! can't go open!\n",
dev->name);
return -ENXIO;
@@ -2659,8 +2652,9 @@
/*
* Move the version data
*/
- error = copy_to_user(result, szVersion, len);
-
+ error = -EFAULT;
+ if (!copy_to_user(result, szVersion, len))
+ error = 0;
return error;
}
@@ -2695,8 +2689,9 @@
result = (struct ppp_stats *) ifr->ifr_ifru.ifru_data;
- error = copy_to_user(result, &temp, sizeof (temp));
-
+ error = -EFAULT;
+ if (!copy_to_user(result, &temp, sizeof (temp)))
+ error = 0;
return error;
}
@@ -2727,8 +2722,9 @@
*/
result = (struct ppp_comp_stats *) ifr->ifr_ifru.ifru_data;
- error = copy_to_user(result, &temp, sizeof (temp));
-
+ error = -EFAULT;
+ if (!copy_to_user(result, &temp, sizeof (temp)))
+ error = 0;
return error;
}
--------------43578C18B690E38278FCD280--