diff mbox series

[2/2] l2tp: Add Protocol field compression

Message ID 20181214211242.9721-2-semen.protsenko@linaro.org
State New
Headers show
Series [1/2] l2tp: Bring back ->flags to struct pppol2tp_session | expand

Commit Message

Sam Protsenko Dec. 14, 2018, 9:12 p.m. UTC 
When Protocol Field Compression (PFC) is enabled, the "Protocol" field
in PPP packet should be transmitted without leading 0x00. See section
6.5 in RFC 1661 for details. Let's compress protocol field if needed,
the same way it's done in drivers/net/ppp/pptp.c.

To actually enable PFC, one should issue corresponding ioctl to L2TP
driver from user-space, like this:

    ioctl(fd, PPPIOCGFLAGS, &flags);
    flags |= SC_COMP_PROT;
    ioctl(fd, PPPIOCSFLAGS, &flags);

It can be done e.g. from pppol2tp plugin (pppd), when pcomp option was
negotiated with peer.

Of course, we don't compress Protocol field when sending LCP packets. As
stated in RFC 1661, section 6.5:

    The Protocol field is never compressed when sending any LCP
    packet.  This rule guarantees unambiguous recognition of LCP
    packets.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>

---
 net/l2tp/l2tp_ppp.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

-- 
2.19.1

Comments

Guillaume Nault Dec. 16, 2018, 4:30 p.m. UTC | #1 
On Fri, Dec 14, 2018 at 11:12:42PM +0200, Sam Protsenko wrote:
> When Protocol Field Compression (PFC) is enabled, the "Protocol" field

> in PPP packet should be transmitted without leading 0x00. See section

> 6.5 in RFC 1661 for details. Let's compress protocol field if needed,

> the same way it's done in drivers/net/ppp/pptp.c.

> 

> To actually enable PFC, one should issue corresponding ioctl to L2TP

> driver from user-space, like this:

> 

>     ioctl(fd, PPPIOCGFLAGS, &flags);

>     flags |= SC_COMP_PROT;

>     ioctl(fd, PPPIOCSFLAGS, &flags);

> 

> It can be done e.g. from pppol2tp plugin (pppd), when pcomp option was

> negotiated with peer.

> 

> Of course, we don't compress Protocol field when sending LCP packets. As

> stated in RFC 1661, section 6.5:

> 

>     The Protocol field is never compressed when sending any LCP

>     packet.  This rule guarantees unambiguous recognition of LCP

>     packets.

> 

Again, I'm sorry, but I must oppose this change. Although I'm lacking
time to keep sanitising L2TP, at least I'd like to avoid making the
situation worse.

L2TP's uapi is already messy enough. Please don't add non-L2TP features
there.

Activating PFC should be done on PPP file descriptors, not no L2TP
sockets. We certainly don't want L2TP to snoop on PPP data, much less
modify them.
Sam Protsenko Dec. 16, 2018, 6:46 p.m. UTC | #2 
Hi Guillaume,

On Sun, Dec 16, 2018 at 6:30 PM Guillaume Nault <g.nault@alphalink.fr> wrote:
>

> On Fri, Dec 14, 2018 at 11:12:42PM +0200, Sam Protsenko wrote:

> > When Protocol Field Compression (PFC) is enabled, the "Protocol" field

> > in PPP packet should be transmitted without leading 0x00. See section

> > 6.5 in RFC 1661 for details. Let's compress protocol field if needed,

> > the same way it's done in drivers/net/ppp/pptp.c.

> >

> > To actually enable PFC, one should issue corresponding ioctl to L2TP

> > driver from user-space, like this:

> >

> >     ioctl(fd, PPPIOCGFLAGS, &flags);

> >     flags |= SC_COMP_PROT;

> >     ioctl(fd, PPPIOCSFLAGS, &flags);

> >

> > It can be done e.g. from pppol2tp plugin (pppd), when pcomp option was

> > negotiated with peer.

> >

> > Of course, we don't compress Protocol field when sending LCP packets. As

> > stated in RFC 1661, section 6.5:

> >

> >     The Protocol field is never compressed when sending any LCP

> >     packet.  This rule guarantees unambiguous recognition of LCP

> >     packets.

> >

> Again, I'm sorry, but I must oppose this change. Although I'm lacking

> time to keep sanitising L2TP, at least I'd like to avoid making the

> situation worse.

>

> L2TP's uapi is already messy enough. Please don't add non-L2TP features

> there.

>

> Activating PFC should be done on PPP file descriptors, not no L2TP

> sockets. We certainly don't want L2TP to snoop on PPP data, much less

> modify them.


Makes sense. I thought about this, too, just found that it's done that
way in PPTP code and decided not to be too smart about this. Let me
try and re-work this one. Will send v2 soon.

Thanks.
Guillaume Nault Dec. 16, 2018, 7:33 p.m. UTC | #3 
On Sun, Dec 16, 2018 at 08:46:19PM +0200, Sam Protsenko wrote:
> Hi Guillaume,

> 

> On Sun, Dec 16, 2018 at 6:30 PM Guillaume Nault <g.nault@alphalink.fr> wrote:

> >

> > On Fri, Dec 14, 2018 at 11:12:42PM +0200, Sam Protsenko wrote:

> > > When Protocol Field Compression (PFC) is enabled, the "Protocol" field

> > > in PPP packet should be transmitted without leading 0x00. See section

> > > 6.5 in RFC 1661 for details. Let's compress protocol field if needed,

> > > the same way it's done in drivers/net/ppp/pptp.c.

> > >

> > > To actually enable PFC, one should issue corresponding ioctl to L2TP

> > > driver from user-space, like this:

> > >

> > >     ioctl(fd, PPPIOCGFLAGS, &flags);

> > >     flags |= SC_COMP_PROT;

> > >     ioctl(fd, PPPIOCSFLAGS, &flags);

> > >

> > > It can be done e.g. from pppol2tp plugin (pppd), when pcomp option was

> > > negotiated with peer.

> > >

> > > Of course, we don't compress Protocol field when sending LCP packets. As

> > > stated in RFC 1661, section 6.5:

> > >

> > >     The Protocol field is never compressed when sending any LCP

> > >     packet.  This rule guarantees unambiguous recognition of LCP

> > >     packets.

> > >

> > Again, I'm sorry, but I must oppose this change. Although I'm lacking

> > time to keep sanitising L2TP, at least I'd like to avoid making the

> > situation worse.

> >

> > L2TP's uapi is already messy enough. Please don't add non-L2TP features

> > there.

> >

> > Activating PFC should be done on PPP file descriptors, not no L2TP

> > sockets. We certainly don't want L2TP to snoop on PPP data, much less

> > modify them.

> 

> Makes sense. I thought about this, too, just found that it's done that

> way in PPTP code and decided not to be too smart about this. Let me

> try and re-work this one. Will send v2 soon.

> 

While at it, be sure to target net-next and to post a cover letter if
you have more than one patch in the series.

Also, the code will have to be compatible with those layers that
already implement PFC using their own API (pptp.c, ppp_async.c, etc.).
I haven't looked at MP-PPP for a while, but multi-link might have to be
taken into account too.

But, for now, fixing the reception part is more important, IMO.
diff mbox series

Patch

diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index 145435977b21..7860b219af07 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -363,7 +363,10 @@  static int pppol2tp_xmit(struct ppp_channel *chan, struct sk_buff *skb)
 	struct sock *sk = (struct sock *) chan->private;
 	struct l2tp_session *session;
 	struct l2tp_tunnel *tunnel;
+	struct pppol2tp_session *ps;
 	int uhlen, headroom;
+	unsigned char *data;
+	bool is_lcp;
 
 	if (sock_flag(sk, SOCK_DEAD) || !(sk->sk_state & PPPOX_CONNECTED))
 		goto abort;
@@ -384,6 +387,15 @@  static int pppol2tp_xmit(struct ppp_channel *chan, struct sk_buff *skb)
 	if (skb_cow_head(skb, headroom))
 		goto abort_put_sess;
 
+	ps = l2tp_session_priv(session);
+	data = skb->data;
+	is_lcp = ((data[0] << 8) + data[1]) == PPP_LCP &&
+		data[2] >= 1 && data[2] <= 7;
+
+	/* Compress protocol field if PFC is enabled */
+	if ((ps->flags & SC_COMP_PROT) && data[0] == 0x00 && !is_lcp)
+		__skb_pull(skb, 1);
+
 	/* Setup PPP header */
 	__skb_push(skb, 2);
 	skb->data[0] = PPP_ALLSTATIONS;