| Message ID | 20181214175921.6859-1-semen.protsenko@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | l2tp: Add protocol field decompression | expand |
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.
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?
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?
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;
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(+) -- 2.19.1