[PATCH] ppp: Move PFC decompression to PPP generic layer

[PATCH] ppp: Move PFC decompression to PPP generic layer

[Sam Protsenko]

Extract "Protocol" field decompression code from transport protocols to
PPP generic layer, where it actually belongs. As a consequence, this
patch fixes incorrect place of PFC decompression in L2TP driver (when
it's not PPPOX_BOUND) and also enables this decompression for other
protocols, like PPPoE.

Protocol field decompression also happens in PPP Multilink Protocol
code and in PPP compression protocols implementations (bsd, deflate,
mppe). It looks like there is no easy way to get rid of that, so it was
decided to leave it as is, but provide those cases with appropriate
comments instead.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 drivers/net/ppp/ppp_async.c   | 14 +++++++-------
 drivers/net/ppp/ppp_generic.c | 21 +++++++++++++++++++--
 drivers/net/ppp/ppp_synctty.c |  9 ++++-----
 drivers/net/ppp/pptp.c        |  5 -----
 net/l2tp/l2tp_ppp.c           |  4 ----
 5 files changed, 30 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ppp/ppp_async.c b/drivers/net/ppp/ppp_async.c
index 288cf099876b..b287bb811875 100644
--- a/drivers/net/ppp/ppp_async.c
+++ b/drivers/net/ppp/ppp_async.c
@@ -770,7 +770,7 @@ process_input_packet(struct asyncppp *ap)
 {
 	struct sk_buff *skb;
 	unsigned char *p;
-	unsigned int len, fcs, proto;
+	unsigned int len, fcs;
 
 	skb = ap->rpkt;
 	if (ap->state & (SC_TOSS | SC_ESCAPE))
@@ -799,14 +799,14 @@ process_input_packet(struct asyncppp *ap)
 			goto err;
 		p = skb_pull(skb, 2);
 	}
-	proto = p[0];
-	if (proto & 1) {
-		/* protocol is compressed */
-		*(u8 *)skb_push(skb, 1) = 0;
-	} else {
+
+	/* If protocol field is not compressed, it can be LCP packet */
+	if (!(p[0] & 0x01)) {
+		unsigned int proto;
+
 		if (skb->len < 2)
 			goto err;
-		proto = (proto << 8) + p[1];
+		proto = (p[0] << 8) + p[1];
 		if (proto == PPP_LCP)
 			async_lcp_peek(ap, p, skb->len, 1);
 	}
diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 500bc0027c1b..10c4c8eec995 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -1965,6 +1965,14 @@ ppp_do_recv(struct ppp *ppp, struct sk_buff *skb, struct channel *pch)
 	ppp_recv_unlock(ppp);
 }
 
+/* Decompress protocol field in PPP header if it's compressed */
+static inline void
+ppp_decompress_proto(struct sk_buff *skb)
+{
+	if (skb->data[0] & 0x01)
+		*(u8 *)skb_push(skb, 1) = 0x00;
+}
+
 void
 ppp_input(struct ppp_channel *chan, struct sk_buff *skb)
 {
@@ -1986,6 +1994,7 @@ ppp_input(struct ppp_channel *chan, struct sk_buff *skb)
 		goto done;
 	}
 
+	ppp_decompress_proto(skb);
 	proto = PPP_PROTO(skb);
 	if (!pch->ppp || proto >= 0xc000 || proto == PPP_CCPFRAG) {
 		/* put it on the channel queue */
@@ -2074,6 +2083,9 @@ ppp_receive_nonmp_frame(struct ppp *ppp, struct sk_buff *skb)
 	if (ppp->flags & SC_MUST_COMP && ppp->rstate & SC_DC_FERROR)
 		goto err;
 
+	/* At this point the "Protocol" field MUST be decompressed, either in
+	 * ppp_decompress_frame() or in ppp_receive_mp_frame().
+	 */
 	proto = PPP_PROTO(skb);
 	switch (proto) {
 	case PPP_VJC_COMP:
@@ -2245,6 +2257,9 @@ ppp_decompress_frame(struct ppp *ppp, struct sk_buff *skb)
 		skb_put(skb, len);
 		skb_pull(skb, 2);	/* pull off the A/C bytes */
 
+		/* Don't call ppp_decompress_proto() here, but rather rely on
+		 * corresponding algo (mppe/bsd/deflate) to decompress it.
+		 */
 	} else {
 		/* Uncompressed frame - pass to decompressor so it
 		   can update its dictionary if necessary. */
@@ -2290,9 +2305,11 @@ ppp_receive_mp_frame(struct ppp *ppp, struct sk_buff *skb, struct channel *pch)
 
 	/*
 	 * Do protocol ID decompression on the first fragment of each packet.
+	 * We can't wait for this to happen in ppp_input(), because
+	 * ppp_receive_nonmp_frame() expects decompressed protocol field.
 	 */
-	if ((PPP_MP_CB(skb)->BEbits & B) && (skb->data[0] & 1))
-		*(u8 *)skb_push(skb, 1) = 0;
+	if (PPP_MP_CB(skb)->BEbits & B)
+		ppp_decompress_proto(skb);
 
 	/*
 	 * Expand sequence number to 32 bits, making it as close
diff --git a/drivers/net/ppp/ppp_synctty.c b/drivers/net/ppp/ppp_synctty.c
index 047f6c68a441..d02ba2494d93 100644
--- a/drivers/net/ppp/ppp_synctty.c
+++ b/drivers/net/ppp/ppp_synctty.c
@@ -709,11 +709,10 @@ ppp_sync_input(struct syncppp *ap, const unsigned char *buf,
 		p = skb_pull(skb, 2);
 	}
 
-	/* decompress protocol field if compressed */
-	if (p[0] & 1) {
-		/* protocol is compressed */
-		*(u8 *)skb_push(skb, 1) = 0;
-	} else if (skb->len < 2)
+	/* PPP packet length should be >= 2 bytes when protocol field is not
+	 * compressed.
+	 */
+	if (!(p[0] & 0x01) && skb->len < 2)
 		goto err;
 
 	/* queue the frame to be processed */
diff --git a/drivers/net/ppp/pptp.c b/drivers/net/ppp/pptp.c
index 67ffe74747a1..8f09edd811e9 100644
--- a/drivers/net/ppp/pptp.c
+++ b/drivers/net/ppp/pptp.c
@@ -325,11 +325,6 @@ static int pptp_rcv_core(struct sock *sk, struct sk_buff *skb)
 			skb_pull(skb, 2);
 		}
 
-		if ((*skb->data) & 1) {
-			/* protocol is compressed */
-			*(u8 *)skb_push(skb, 1) = 0;
-		}
-
 		skb->ip_summed = CHECKSUM_NONE;
 		skb_set_network_header(skb, skb->head-skb->data);
 		ppp_input(&po->chan, skb);
diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index c03c6461f236..04d9946dcdba 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -236,10 +236,6 @@ static void pppol2tp_recv(struct l2tp_session *session, struct sk_buff *skb, int
 	    skb->data[1] == PPP_UI)
 		skb_pull(skb, 2);
 
-	/* Decompress protocol field if PFC is enabled */
-	if ((*skb->data) & 0x1)
-		*(u8 *)skb_push(skb, 1) = 0;
-
 	if (sk->sk_state & PPPOX_BOUND) {
 		struct pppox_sock *po;
 
-- 
2.19.2

Re: [PATCH] ppp: Move PFC decompression to PPP generic layer

[Guillaume Nault]

On Wed, Dec 19, 2018 at 02:08:08AM +0200, Sam Protsenko wrote:
> Extract "Protocol" field decompression code from transport protocols to
> PPP generic layer, where it actually belongs. As a consequence, this
> patch fixes incorrect place of PFC decompression in L2TP driver (when
> it's not PPPOX_BOUND) and also enables this decompression for other
> protocols, like PPPoE.
> 
> Protocol field decompression also happens in PPP Multilink Protocol
> code and in PPP compression protocols implementations (bsd, deflate,
> mppe). It looks like there is no easy way to get rid of that, so it was
> decided to leave it as is, but provide those cases with appropriate
> comments instead.
> 
Yes, ideally we'd make PPP_PROTO() handle compressed protocol so that
we wouldn't need to decompress protocol field in place. But other parts
of the ppp code assume uncompressed protocol field in skb->data, so
let's stick with the current behaviour for now.

> diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> index 500bc0027c1b..10c4c8eec995 100644
> --- a/drivers/net/ppp/ppp_generic.c
> +++ b/drivers/net/ppp/ppp_generic.c
> @@ -1965,6 +1965,14 @@ ppp_do_recv(struct ppp *ppp, struct sk_buff *skb, struct channel *pch)
>  	ppp_recv_unlock(ppp);
>  }
>  
> +/* Decompress protocol field in PPP header if it's compressed */
> +static inline void
> +ppp_decompress_proto(struct sk_buff *skb)
> 
No need for inline keyword in .c files. Also, no need to split line
before function name. I know older function declarations of
ppp_generic.c do this, but let's stick to the general networking stack
style instead.

> +{
> +	if (skb->data[0] & 0x01)
> +		*(u8 *)skb_push(skb, 1) = 0x00;
> +}
> +
This assumes that we have at least 1 byte of head room and 1 byte of
linear data. Although that's what the original code did, I find that's
a bit dangerous. Maybe prefix the function name with '__' and add a
comment in the function description to warn the reader.

>  void
>  ppp_input(struct ppp_channel *chan, struct sk_buff *skb)
>  {
> @@ -1986,6 +1994,7 @@ ppp_input(struct ppp_channel *chan, struct sk_buff *skb)
>  		goto done;
>  	}
>  
> +	ppp_decompress_proto(skb);
> 
The pskb_may_pull(skb, 2) at the beginning of the functions is there
because we're going to read the protocol field. It doesn't make sense to
decompress it after this test (although technically that mostly works).

However, if we move ppp_decompress_proto() before pskb_may_pull(), then
callers must respect the requirements of ppp_decompress_proto().

>  	proto = PPP_PROTO(skb);
>  	if (!pch->ppp || proto >= 0xc000 || proto == PPP_CCPFRAG) {
>  		/* put it on the channel queue */
> @@ -2074,6 +2083,9 @@ ppp_receive_nonmp_frame(struct ppp *ppp, struct sk_buff *skb)
>  	if (ppp->flags & SC_MUST_COMP && ppp->rstate & SC_DC_FERROR)
>  		goto err;
>  
> +	/* At this point the "Protocol" field MUST be decompressed, either in
> +	 * ppp_decompress_frame() or in ppp_receive_mp_frame().
> +	 */
> 
Or ppp_input(). In the general case (no multilink header and no compression)
it's ppp_input() that'd decompress the protocol field.

> @@ -2290,9 +2305,11 @@ ppp_receive_mp_frame(struct ppp *ppp, struct sk_buff *skb, struct channel *pch)
>  
>  	/*
>  	 * Do protocol ID decompression on the first fragment of each packet.
> +	 * We can't wait for this to happen in ppp_input(), because
> +	 * ppp_receive_nonmp_frame() expects decompressed protocol field.
>  	 */
ppp_input() is not going to be called anyway. I think you can omit the
middle line entirely.

Re: [PATCH] ppp: Move PFC decompression to PPP generic layer

[Sam Protsenko]

Hi Guillaume,

On Wed, Dec 19, 2018 at 4:38 PM Guillaume Nault <g.nault@alphalink.fr> wrote:
>
> On Wed, Dec 19, 2018 at 02:08:08AM +0200, Sam Protsenko wrote:
> > Extract "Protocol" field decompression code from transport protocols to
> > PPP generic layer, where it actually belongs. As a consequence, this
> > patch fixes incorrect place of PFC decompression in L2TP driver (when
> > it's not PPPOX_BOUND) and also enables this decompression for other
> > protocols, like PPPoE.
> >
> > Protocol field decompression also happens in PPP Multilink Protocol
> > code and in PPP compression protocols implementations (bsd, deflate,
> > mppe). It looks like there is no easy way to get rid of that, so it was
> > decided to leave it as is, but provide those cases with appropriate
> > comments instead.
> >
> Yes, ideally we'd make PPP_PROTO() handle compressed protocol so that
> we wouldn't need to decompress protocol field in place. But other parts
> of the ppp code assume uncompressed protocol field in skb->data, so
> let's stick with the current behaviour for now.
>
> > diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> > index 500bc0027c1b..10c4c8eec995 100644
> > --- a/drivers/net/ppp/ppp_generic.c
> > +++ b/drivers/net/ppp/ppp_generic.c
> > @@ -1965,6 +1965,14 @@ ppp_do_recv(struct ppp *ppp, struct sk_buff *skb, struct channel *pch)
> >       ppp_recv_unlock(ppp);
> >  }
> >
> > +/* Decompress protocol field in PPP header if it's compressed */
> > +static inline void
> > +ppp_decompress_proto(struct sk_buff *skb)
> >
> No need for inline keyword in .c files. Also, no need to split line
> before function name. I know older function declarations of
> ppp_generic.c do this, but let's stick to the general networking stack
> style instead.
>
> > +{
> > +     if (skb->data[0] & 0x01)
> > +             *(u8 *)skb_push(skb, 1) = 0x00;
> > +}
> > +
> This assumes that we have at least 1 byte of head room and 1 byte of
> linear data. Although that's what the original code did, I find that's
> a bit dangerous. Maybe prefix the function name with '__' and add a
> comment in the function description to warn the reader.
>
> >  void
> >  ppp_input(struct ppp_channel *chan, struct sk_buff *skb)
> >  {
> > @@ -1986,6 +1994,7 @@ ppp_input(struct ppp_channel *chan, struct sk_buff *skb)
> >               goto done;
> >       }
> >
> > +     ppp_decompress_proto(skb);
> >
> The pskb_may_pull(skb, 2) at the beginning of the functions is there
> because we're going to read the protocol field. It doesn't make sense to
> decompress it after this test (although technically that mostly works).
>
> However, if we move ppp_decompress_proto() before pskb_may_pull(), then
> callers must respect the requirements of ppp_decompress_proto().
>
> >       proto = PPP_PROTO(skb);
> >       if (!pch->ppp || proto >= 0xc000 || proto == PPP_CCPFRAG) {
> >               /* put it on the channel queue */
> > @@ -2074,6 +2083,9 @@ ppp_receive_nonmp_frame(struct ppp *ppp, struct sk_buff *skb)
> >       if (ppp->flags & SC_MUST_COMP && ppp->rstate & SC_DC_FERROR)
> >               goto err;
> >
> > +     /* At this point the "Protocol" field MUST be decompressed, either in
> > +      * ppp_decompress_frame() or in ppp_receive_mp_frame().
> > +      */
> >
> Or ppp_input(). In the general case (no multilink header and no compression)
> it's ppp_input() that'd decompress the protocol field.
>
> > @@ -2290,9 +2305,11 @@ ppp_receive_mp_frame(struct ppp *ppp, struct sk_buff *skb, struct channel *pch)
> >
> >       /*
> >        * Do protocol ID decompression on the first fragment of each packet.
> > +      * We can't wait for this to happen in ppp_input(), because
> > +      * ppp_receive_nonmp_frame() expects decompressed protocol field.
> >        */
> ppp_input() is not going to be called anyway. I think you can omit the
> middle line entirely.

Thanks for the detailed review! All your points are well taken.
Already sent fixed v2, please review.