Re: RFC: [1/2] PPP MPPE module

[Matt Domsch <Matt_Domsch@dell.com>]

[-- Attachment #1: Type: text/plain, Size: 4910 bytes --]

On Fri, Jun 18, 2004 at 05:15:58PM +0100, Christoph Hellwig wrote:
> Last time I talked to Paul on MPPE he didn't like those hacks in
> the ppp core.

Adding the linux-ppp list to the discussion.

Rather than have code in ppp_generic.c testing for CI_MPPE and doing
code conditionally (which I agree was ugly), how about adding two new
fields to struct compressor:

+	/* Extra skb space needed by the compressor algorithm */
+	unsigned int comp_skb_extra_space;
+	/* Extra skb space needed by the decompressor algorithm */
+	unsigned int decomp_skb_extra_space;

which the compressor modules can fill in if needed?  Presently,
bsd_comp.c and ppp_deflate.c don't need these, so they will be filled
with zeros automatically at struct compressor instantiation.

This hunk may still be contriversial though.  It adds a negative
return value to the compress() method, which is only (presently) used
by ppp_mppe (bsd_comp.c and ppp_deflate.c always return 0 if they
couldn't compress), to indicate the frame should be dropped.

-		} else {
+                } else if (len == 0) {
 			/* didn't compress, or CCP not up yet */
 			kfree_skb(new_skb);
+                } else {
+                        /*
+                         * (len < 0)
+                         * MPPE requires that we do not send unencrypted
+                         * frames.  The compressor will return -1 if we
+                         * should drop the frame.  We cannot simply test
+                         * the compress_proto because MPPE and MPPC share
+                         * the same number.
+                         */
+                        printk(KERN_ERR "ppp: compressor dropped pkt\n");
+                        kfree_skb(new_skb);
+                        goto drop;

Thoughts?

Patch below against 2.6.7-bk ppp_generic.c and ppp-comp.h to add such,
for comment only.

Thanks,
Matt

-- 
Matt Domsch
Sr. Software Engineer, Lead Engineer
Dell Linux Solutions linux.dell.com & www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com

===== drivers/net/ppp_generic.c 1.45 vs edited =====
--- 1.45/drivers/net/ppp_generic.c	2004-04-09 18:21:06 -05:00
+++ edited/drivers/net/ppp_generic.c	2004-06-21 10:56:34 -05:00
@@ -1066,8 +1066,9 @@
 	/* try to do packet compression */
 	if ((ppp->xstate & SC_COMP_RUN) && ppp->xc_state != 0
 	    && proto != PPP_LCP && proto != PPP_CCP) {
-		new_skb = alloc_skb(ppp->dev->mtu + ppp->dev->hard_header_len,
-				    GFP_ATOMIC);
+                int new_skb_size = ppp->dev->mtu + ppp->xcomp->comp_skb_extra_space + ppp->dev->hard_header_len;
+                int compressor_skb_size = ppp->dev->mtu + ppp->xcomp->comp_skb_extra_space + PPP_HDRLEN;
+                new_skb = alloc_skb(new_skb_size, GFP_ATOMIC);
 		if (new_skb == 0) {
 			printk(KERN_ERR "PPP: no memory (comp pkt)\n");
 			goto drop;
@@ -1079,15 +1080,27 @@
 		/* compressor still expects A/C bytes in hdr */
 		len = ppp->xcomp->compress(ppp->xc_state, skb->data - 2,
 					   new_skb->data, skb->len + 2,
-					   ppp->dev->mtu + PPP_HDRLEN);
+                                           compressor_skb_size);
 		if (len > 0 && (ppp->flags & SC_CCP_UP)) {
 			kfree_skb(skb);
 			skb = new_skb;
 			skb_put(skb, len);
 			skb_pull(skb, 2);	/* pull off A/C bytes */
-		} else {
+                } else if (len == 0) {
 			/* didn't compress, or CCP not up yet */
 			kfree_skb(new_skb);
+                } else {
+                        /*
+                         * (len < 0)
+                         * MPPE requires that we do not send unencrypted
+                         * frames.  The compressor will return -1 if we
+                         * should drop the frame.  We cannot simply test
+                         * the compress_proto because MPPE and MPPC share
+                         * the same number.
+                         */
+                        printk(KERN_ERR "ppp: compressor dropped pkt\n");
+                        kfree_skb(new_skb);
+                        goto drop;
 		}
 	}
 
@@ -1596,7 +1609,7 @@
 		goto err;
 
 	if (proto == PPP_COMP) {
-		ns = dev_alloc_skb(ppp->mru + PPP_HDRLEN);
+		ns = dev_alloc_skb(ppp->mru + ppp->rcomp->decomp_skb_extra_space + PPP_HDRLEN);
 		if (ns == 0) {
 			printk(KERN_ERR "ppp_decompress_frame: no memory\n");
 			goto err;


===== include/linux/ppp-comp.h 1.4 vs edited =====
--- 1.4/include/linux/ppp-comp.h	2003-08-07 18:57:19 -05:00
+++ edited/include/linux/ppp-comp.h	2004-06-21 10:59:44 -05:00
@@ -111,6 +111,10 @@
 
 	/* Used in locking compressor modules */
 	struct module *owner;
+	/* Extra skb space needed by the compressor algorithm */
+	unsigned int comp_skb_extra_space;
+	/* Extra skb space needed by the decompressor algorithm */
+	unsigned int decomp_skb_extra_space;
 };
 
 /*

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]