[PATCH] l2tp: Add protocol field decompression

[PATCH] l2tp: Add protocol field decompression

[Sam Protsenko]

When Protocol Field Compression (PFC) is enabled, the "Protocol" field
in PPP packet will be received without leading 0x00. See section 6.5 in
RFC 1661 for details. So let's decompress protocol field if needed, the
same way it's done in drivers/net/ppp/pptp.c.

In case when "nopcomp" pppd option is not enabled, PFC (pcomp) can be
negotiated during LCP handshake, and L2TP driver in kernel will receive
PPP packets with compressed Protocol field, which in turn leads to next
error:

    Protocol Rejected (unsupported protocol 0x2145)

because instead of Protocol=0x0021 in PPP packet there will be
Protocol=0x21. This patch unwraps it back to 0x0021, which fixes the
issue.

Sending the compressed Protocol field will be implemented in subsequent
patch, this one is self-sufficient.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 net/l2tp/l2tp_ppp.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index 04d9946dcdba..c03c6461f236 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -236,6 +236,10 @@ 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.1

Re: [PATCH] l2tp: Add protocol field decompression

[David Miller]

From: Sam Protsenko <semen.protsenko@linaro.org>
Date: Fri, 14 Dec 2018 19:59:21 +0200

> When Protocol Field Compression (PFC) is enabled, the "Protocol" field
> in PPP packet will be received without leading 0x00. See section 6.5 in
> RFC 1661 for details. So let's decompress protocol field if needed, the
> same way it's done in drivers/net/ppp/pptp.c.
> 
> In case when "nopcomp" pppd option is not enabled, PFC (pcomp) can be
> negotiated during LCP handshake, and L2TP driver in kernel will receive
> PPP packets with compressed Protocol field, which in turn leads to next
> error:
> 
>     Protocol Rejected (unsupported protocol 0x2145)
> 
> because instead of Protocol=0x0021 in PPP packet there will be
> Protocol=0x21. This patch unwraps it back to 0x0021, which fixes the
> issue.
> 
> Sending the compressed Protocol field will be implemented in subsequent
> patch, this one is self-sufficient.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>

Applied, thanks.

Re: [PATCH] l2tp: Add protocol field decompression

[Guillaume Nault]

On Fri, Dec 14, 2018 at 07:59:21PM +0200, Sam Protsenko wrote:
> When Protocol Field Compression (PFC) is enabled, the "Protocol" field
> in PPP packet will be received without leading 0x00. See section 6.5 in
> RFC 1661 for details. So let's decompress protocol field if needed, the
> same way it's done in drivers/net/ppp/pptp.c.
> 
> In case when "nopcomp" pppd option is not enabled, PFC (pcomp) can be
> negotiated during LCP handshake, and L2TP driver in kernel will receive
> PPP packets with compressed Protocol field, which in turn leads to next
> error:
> 
>     Protocol Rejected (unsupported protocol 0x2145)
> 
> because instead of Protocol=0x0021 in PPP packet there will be
> Protocol=0x21. This patch unwraps it back to 0x0021, which fixes the
> issue.
> 
> Sending the compressed Protocol field will be implemented in subsequent
> patch, this one is self-sufficient.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>  net/l2tp/l2tp_ppp.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
> index 04d9946dcdba..c03c6461f236 100644
> --- a/net/l2tp/l2tp_ppp.c
> +++ b/net/l2tp/l2tp_ppp.c
> @@ -236,6 +236,10 @@ 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;
> +
Sorry, but PFC is PPP stuff. It's absolutely independent from the
transport layer. Therefore, it belongs to ppp_generic.c and L2TP has
nothing do with it.

I know some other transports got it wrong, but let's stop
re-implementing PPP features in every lower layer. Teaching ppp_input()
about PFC is all that is needed to get the feature work correctly on
all transports. As a bonus, it'd bring PFC support to PPPoE and would
let us drop the duplicated code from pptp.c, ppp_async.c and
ppp_synctty.c.

As an example of the problems caused by mixing the PPP and L2TP
layers, consider the case of L2TP sockets that aren't PPPOX_BOUND.
They're supposed to receive the original PPP frames. Now with your
patch if the protocol was compressed, the L2TP socket receives a fake
header. User space didn't ask for that and has no way to figure out
what was the original packet. To make things worse, that behaviour now
changes depending on the kernel version.

If you all agree, can we please revert this patch and properly
implement PFC in ppp_generic.c?

Re: [PATCH] l2tp: Add protocol field decompression

[Sam Protsenko]

Hi Guillaume,

On Sun, Dec 16, 2018 at 6:29 PM Guillaume Nault <g.nault@alphalink.fr> wrote:
>
> On Fri, Dec 14, 2018 at 07:59:21PM +0200, Sam Protsenko wrote:
> > When Protocol Field Compression (PFC) is enabled, the "Protocol" field
> > in PPP packet will be received without leading 0x00. See section 6.5 in
> > RFC 1661 for details. So let's decompress protocol field if needed, the
> > same way it's done in drivers/net/ppp/pptp.c.
> >
> > In case when "nopcomp" pppd option is not enabled, PFC (pcomp) can be
> > negotiated during LCP handshake, and L2TP driver in kernel will receive
> > PPP packets with compressed Protocol field, which in turn leads to next
> > error:
> >
> >     Protocol Rejected (unsupported protocol 0x2145)
> >
> > because instead of Protocol=0x0021 in PPP packet there will be
> > Protocol=0x21. This patch unwraps it back to 0x0021, which fixes the
> > issue.
> >
> > Sending the compressed Protocol field will be implemented in subsequent
> > patch, this one is self-sufficient.
> >
> > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> > ---
> >  net/l2tp/l2tp_ppp.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
> > index 04d9946dcdba..c03c6461f236 100644
> > --- a/net/l2tp/l2tp_ppp.c
> > +++ b/net/l2tp/l2tp_ppp.c
> > @@ -236,6 +236,10 @@ 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;
> > +
> Sorry, but PFC is PPP stuff. It's absolutely independent from the
> transport layer. Therefore, it belongs to ppp_generic.c and L2TP has
> nothing do with it.
>
> I know some other transports got it wrong, but let's stop
> re-implementing PPP features in every lower layer. Teaching ppp_input()
> about PFC is all that is needed to get the feature work correctly on
> all transports. As a bonus, it'd bring PFC support to PPPoE and would
> let us drop the duplicated code from pptp.c, ppp_async.c and
> ppp_synctty.c.
>
> As an example of the problems caused by mixing the PPP and L2TP
> layers, consider the case of L2TP sockets that aren't PPPOX_BOUND.
> They're supposed to receive the original PPP frames. Now with your
> patch if the protocol was compressed, the L2TP socket receives a fake
> header. User space didn't ask for that and has no way to figure out
> what was the original packet. To make things worse, that behaviour now
> changes depending on the kernel version.
>
> If you all agree, can we please revert this patch and properly
> implement PFC in ppp_generic.c?

How about instead of reverting I will try to provide the patch
extracting duplicated PFC code (from L2TP, PPTP, etc) to
ppp_generic.c? This one fixes actual problem, and if we are going to
add it to ppp_generic anyway, why not do it in one patch? I'm willing
to do this work in next few days. Hope there is no rush with revert?

Re: [PATCH] l2tp: Add protocol field decompression

[Guillaume Nault]

On Sun, Dec 16, 2018 at 08:36:42PM +0200, Sam Protsenko wrote:
> Hi Guillaume,
> 
> On Sun, Dec 16, 2018 at 6:29 PM Guillaume Nault <g.nault@alphalink.fr> wrote:
> >
> > If you all agree, can we please revert this patch and properly
> > implement PFC in ppp_generic.c?
> 
> How about instead of reverting I will try to provide the patch
> extracting duplicated PFC code (from L2TP, PPTP, etc) to
> ppp_generic.c? This one fixes actual problem, and if we are going to
> add it to ppp_generic anyway, why not do it in one patch? I'm willing
> to do this work in next few days. Hope there is no rush with revert?
> 
Hi Sam,

Yes, no problem. Thanks for taking care of that.
But please try to get the code ready before net-next gets closed.

Guillaume