Hi Guillaume, On Sun, Dec 16, 2018 at 6:29 PM Guillaume Nault <g.na...@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.protse...@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?
