[PATCH 1/2] l2tp: Bring back ->flags to struct pppol2tp_session

[PATCH 1/2] l2tp: Bring back ->flags to struct pppol2tp_session

[Sam Protsenko]

Flags field will be used in further commits (e.g. for keeping
SC_COMP_PROT), so let's bring those back. This commit effectively
reverts commit 1998b5ed9c9b ("l2tp: drop ->flags from struct
pppol2tp_session"), with some cosmetic changes.

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

diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index c03c6461f236..145435977b21 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -125,6 +125,7 @@ struct pppol2tp_session {
 						 * PPPoX socket */
 	struct sock		*__sk;		/* Copy of .sk, for cleanup */
 	struct rcu_head		rcu;		/* For asynchronous release */
+	int			flags;		/* accessed by PPPIOCGFLAGS */
 };
 
 static int pppol2tp_xmit(struct ppp_channel *chan, struct sk_buff *skb);
@@ -1074,44 +1075,60 @@ static int pppol2tp_ioctl(struct socket *sock, unsigned int cmd,
 {
 	struct pppol2tp_ioc_stats stats;
 	struct l2tp_session *session;
+	struct pppol2tp_session *ps;
 	int val;
+	bool is_tunnel;
+
+	session = sock->sk->sk_user_data;
+	if (!session)
+		return -ENOTCONN;
+
+	ps = l2tp_session_priv(session);
+	is_tunnel = !session->session_id && !session->peer_session_id;
 
 	switch (cmd) {
 	case PPPIOCGMRU:
-	case PPPIOCGFLAGS:
-		session = sock->sk->sk_user_data;
-		if (!session)
-			return -ENOTCONN;
-
 		/* Not defined for tunnels */
-		if (!session->session_id && !session->peer_session_id)
+		if (is_tunnel)
 			return -ENOSYS;
 
 		if (put_user(0, (int __user *)arg))
 			return -EFAULT;
 		break;
+	case PPPIOCGFLAGS:
+		/* Not defined for tunnels */
+		if (is_tunnel)
+			return -ENOSYS;
 
-	case PPPIOCSMRU:
-	case PPPIOCSFLAGS:
-		session = sock->sk->sk_user_data;
-		if (!session)
-			return -ENOTCONN;
+		if (put_user(ps->flags, (int __user *)arg))
+			return -EFAULT;
 
+		l2tp_info(session, L2TP_MSG_CONTROL, "%s: get flags=%d\n",
+			  session->name, ps->flags);
+		break;
+	case PPPIOCSMRU:
 		/* Not defined for tunnels */
-		if (!session->session_id && !session->peer_session_id)
+		if (is_tunnel)
 			return -ENOSYS;
 
 		if (get_user(val, (int __user *)arg))
 			return -EFAULT;
 		break;
+	case PPPIOCSFLAGS:
+		/* Not defined for tunnels */
+		if (is_tunnel)
+			return -ENOSYS;
 
-	case PPPIOCGL2TPSTATS:
-		session = sock->sk->sk_user_data;
-		if (!session)
-			return -ENOTCONN;
+		if (get_user(val, (int __user *)arg))
+			return -EFAULT;
 
+		ps->flags = val;
+		l2tp_info(session, L2TP_MSG_CONTROL, "%s: set flags=%d\n",
+			  session->name, ps->flags);
+		break;
+	case PPPIOCGL2TPSTATS:
 		/* Session 0 represents the parent tunnel */
-		if (!session->session_id && !session->peer_session_id) {
+		if (is_tunnel) {
 			u32 session_id;
 			int err;
 
@@ -1136,7 +1153,6 @@ static int pppol2tp_ioctl(struct socket *sock, unsigned int cmd,
 		if (copy_to_user((void __user *)arg, &stats, sizeof(stats)))
 			return -EFAULT;
 		break;
-
 	default:
 		return -ENOIOCTLCMD;
 	}
-- 
2.19.1

[PATCH 2/2] l2tp: Add Protocol field compression

[Sam Protsenko]

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(+)

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;
-- 
2.19.1

Re: [PATCH 2/2] l2tp: Add Protocol field compression

[Guillaume Nault]

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.

Re: [PATCH 2/2] l2tp: Add Protocol field compression

[Sam Protsenko]

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.

Re: [PATCH 2/2] l2tp: Add Protocol field compression

[Guillaume Nault]

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.