Merge branch 'l2tp-remove-configurable-offset-parameters'

James Chapman says:

====================
l2tp: remove configurable offset parameters

This patch series removes all code to support a configurable offset in
transmitted l2tp packets. Code to handle this is incomplete and buggy
and has been this way for years. If anyone tried to configure an
offset, it would be ignored for L2TPv2 tunnels, or for L2TPv3 tunnels,
could result in L2TPv3 packets being transmitted which are not
compliant with L2TPv3 RFC3931. This patch series removes the support
for configurable offsets.

No known userspace l2tp daemon configures an offset. However,
iproute2's "ip l2tp" command has an offset parameter and if set, the
value is passed to the kernel. This is the most likely use case where
offsets might be configured, e.g.

   ip l2tp add tunnel local 1.1.1.1 remote 1.1.1.2 tunnel_id 1 \
       peer_tunnel_id 2 encap ip
   ip l2tp add session name l2tp0 tunnel_id 1 session_id 1 \
       peer_session_id 2 offset 8

The above would result in packets being transmitted to 1.1.1.2 with 8
bytes padding between the L2TPv3 header and the payload. The peer
would need to be configured with the same offset value. However, the
packets are not compliant with the L2TPv3 RFC, hence I think it's
unlikely that offset is being used. With this patch series applied,
the offset would not be configured. The peer would need to be modified to
remove its offset setting too.

iproute2 should be modified to remove or ignore the ip l2tp offset
parameter.

This issue was discovered when reviewing a patch series from
lorenzo.bianconi@redhat.com which adds another netlink attribute to
configure the expected offset in received L2TPv3 packets. This change
is reverted by this series because offsets do not exist in L2TPv3
packets. These commits are:

  commit f15bc54e ("l2tp: add peer_offset parameter")
  commit 820da535 ("l2tp: fix missing print session offset info")

In more detail:

The L2TPv2 protocol supports a variable offset from the L2TPv2 header
to the payload to give the sender implementation some flexibility for
data alignment when adding L2TP headers on to payloads. The offset
value is indicated by an optional field in the L2TP header.  Our L2TP
implementation already detects the presence of the optional offset in
received packets and skips those bytes when parsing packets. All
transmitted L2TPv2 packets are always transmitted with no offset.

L2TPv3 has no optional offset field in the L2TPv3 packet
header. Instead, L2TPv3 defines optional fields in a "Layer-2 Specific
Sublayer". At the time when the original L2TP code was written, there
was talk at IETF of offset being implemented in a new Layer-2 Specific
Sublayer. A L2TP_ATTR_OFFSET netlink attribute was added so that this
offset could be configured and the intention was to allow it to be
also used to set the tx offset for L2TPv2. However, no L2TPv3 offset
was ever specified and the L2TP_ATTR_OFFSET parameter was forgotten
about.

Setting L2TP_ATTR_OFFSET results in L2TPv3 packets being transmitted
with the specified number of bytes padding between L2TPv3 header and
payload. This is not compliant with L2TPv3 RFC3931. So this change
removes the configurable offset altogether while retaining
L2TP_ATTR_OFFSET in the API for backwards compatibility. If
L2TP_ATTR_OFFSET is given, its value is now silently ignored.
====================
Reviewed-by: Guillaume Nault <g.nault@alphalink.fr>
Tested-by: Guillaume Nault <g.nault@alphalink.fr>
Signed-off-by: David S. Miller <davem@davemloft.net>

include/uapi/linux/l2tp.h

@@ -94,7 +94,7 @@ enum {
 	L2TP_ATTR_NONE,			/* no data */
 	L2TP_ATTR_PW_TYPE,		/* u16, enum l2tp_pwtype */
 	L2TP_ATTR_ENCAP_TYPE,		/* u16, enum l2tp_encap_type */
-	L2TP_ATTR_OFFSET,		/* u16 */
+	L2TP_ATTR_OFFSET,		/* u16 (not used) */
 	L2TP_ATTR_DATA_SEQ,		/* u16 */
 	L2TP_ATTR_L2SPEC_TYPE,		/* u8, enum l2tp_l2spec_type */
 	L2TP_ATTR_L2SPEC_LEN,		/* u8, enum l2tp_l2spec_type */
@@ -127,7 +127,6 @@ enum {
 	L2TP_ATTR_UDP_ZERO_CSUM6_TX,	/* flag */
 	L2TP_ATTR_UDP_ZERO_CSUM6_RX,	/* flag */
 	L2TP_ATTR_PAD,
-	L2TP_ATTR_PEER_OFFSET,		/* u16 */
 	__L2TP_ATTR_MAX,
 };
 

net/l2tp/l2tp_core.c

@@ -780,10 +780,8 @@ void l2tp_recv_common(struct l2tp_session *session, struct sk_buff *skb,
 		}
 	}
 
-	/* Session data offset is handled differently for L2TPv2 and
-	 * L2TPv3. For L2TPv2, there is an optional 16-bit value in
-	 * the header. For L2TPv3, the offset is negotiated using AVPs
-	 * in the session setup control protocol.
+	/* Session data offset is defined only for L2TPv2 and is
+	 * indicated by an optional 16-bit value in the header.
 	 */
 	if (tunnel->version == L2TP_HDR_VER_2) {
 		/* If offset bit set, skip it. */
@@ -791,8 +789,7 @@ void l2tp_recv_common(struct l2tp_session *session, struct sk_buff *skb,
 			offset = ntohs(*(__be16 *)ptr);
 			ptr += 2 + offset;
 		}
-	} else
-		ptr += session->peer_offset;
+	}
 
 	offset = ptr - optr;
 	if (!pskb_may_pull(skb, offset))
@@ -1068,8 +1065,6 @@ static int l2tp_build_l2tpv3_header(struct l2tp_session *session, void *buf)
 		}
 		bufp += session->l2specific_len;
 	}
-	if (session->offset)
-		bufp += session->offset;
 
 	return bufp - optr;
 }
@@ -1734,7 +1729,7 @@ void l2tp_session_set_header_len(struct l2tp_session *session, int version)
 		if (session->send_seq)
 			session->hdr_len += 4;
 	} else {
-		session->hdr_len = 4 + session->cookie_len + session->l2specific_len + session->offset;
+		session->hdr_len = 4 + session->cookie_len + session->l2specific_len;
 		if (session->tunnel->encap == L2TP_ENCAPTYPE_UDP)
 			session->hdr_len += 4;
 	}
@@ -1784,8 +1779,6 @@ struct l2tp_session *l2tp_session_create(int priv_size, struct l2tp_tunnel *tunn
 			session->recv_seq = cfg->recv_seq;
 			session->lns_mode = cfg->lns_mode;
 			session->reorder_timeout = cfg->reorder_timeout;
-			session->offset = cfg->offset;
-			session->peer_offset = cfg->peer_offset;
 			session->l2specific_type = cfg->l2specific_type;
 			session->l2specific_len = cfg->l2specific_len;
 			session->cookie_len = cfg->cookie_len;

net/l2tp/l2tp_core.h

@@ -59,8 +59,6 @@ struct l2tp_session_cfg {
 	int			debug;		/* bitmask of debug message
 						 * categories */
 	u16			vlan_id;	/* VLAN pseudowire only */
-	u16			offset;		/* offset to tx payload */
-	u16			peer_offset;	/* offset to rx payload */
 	u16			l2specific_len;	/* Layer 2 specific length */
 	u16			l2specific_type; /* Layer 2 specific type */
 	u8			cookie[8];	/* optional cookie */
@@ -87,14 +85,6 @@ struct l2tp_session {
 	int			cookie_len;
 	u8			peer_cookie[8];
 	int			peer_cookie_len;
-	u16			offset;		/* offset from end of L2TP
-						 * header to beginning of
-						 * tx data
-						 */
-	u16			peer_offset;	/* offset from end of L2TP
-						 * header to beginning of
-						 * rx data
-						 */
 	u16			l2specific_len;
 	u16			l2specific_type;
 	u16			hdr_len;

net/l2tp/l2tp_debugfs.c

@@ -180,8 +180,7 @@ static void l2tp_dfs_seq_session_show(struct seq_file *m, void *v)
 		   session->lns_mode ? "LNS" : "LAC",
 		   session->debug,
 		   jiffies_to_msecs(session->reorder_timeout));
-	seq_printf(m, "   offset %hu peer_offset %hu l2specific %hu/%hu\n",
-		   session->offset, session->peer_offset,
+	seq_printf(m, "   offset 0 l2specific %hu/%hu\n",
 		   session->l2specific_type, session->l2specific_len);
 	if (session->cookie_len) {
 		seq_printf(m, "   cookie %02x%02x%02x%02x",
@@ -229,8 +228,7 @@ static int l2tp_dfs_seq_show(struct seq_file *m, void *v)
 		seq_puts(m, " debug tx-pkts/bytes/errs rx-pkts/bytes/errs\n");
 		seq_puts(m, "  SESSION ID, peer ID, PWTYPE\n");
 		seq_puts(m, "   refcnt cnt\n");
-		seq_puts(m, "   offset OFFSET peer_offset OFFSET");
-		seq_puts(m, " l2specific TYPE/LEN\n");
+		seq_puts(m, "   offset OFFSET l2specific TYPE/LEN\n");
 		seq_puts(m, "   [ cookie ]\n");
 		seq_puts(m, "   [ peer cookie ]\n");
 		seq_puts(m, "   config mtu/mru/rcvseq/sendseq/dataseq/lns debug reorderto\n");

net/l2tp/l2tp_netlink.c

@@ -547,25 +547,6 @@ static int l2tp_nl_cmd_session_create(struct sk_buff *skb, struct genl_info *inf
 	}
 
 	if (tunnel->version > 2) {
-		if (info->attrs[L2TP_ATTR_PEER_OFFSET]) {
-			struct nlattr *peer_offset;
-
-			peer_offset = info->attrs[L2TP_ATTR_PEER_OFFSET];
-			cfg.peer_offset = nla_get_u16(peer_offset);
-		}
-
-		if (info->attrs[L2TP_ATTR_OFFSET]) {
-			cfg.offset = nla_get_u16(info->attrs[L2TP_ATTR_OFFSET]);
-
-			/* in order to maintain compatibility with older
-			 * versions where offset was used for both tx and
-			 * rx side, update rx side with offset if peer_offset
-			 * is not provided by userspace
-			 */
-			if (!info->attrs[L2TP_ATTR_PEER_OFFSET])
-				cfg.peer_offset = cfg.offset;
-		}
-
 		if (info->attrs[L2TP_ATTR_DATA_SEQ])
 			cfg.data_seq = nla_get_u8(info->attrs[L2TP_ATTR_DATA_SEQ]);
 
@@ -777,10 +758,6 @@ static int l2tp_nl_session_send(struct sk_buff *skb, u32 portid, u32 seq, int fl
 
 	if ((session->ifname[0] &&
 	     nla_put_string(skb, L2TP_ATTR_IFNAME, session->ifname)) ||
-	    (session->offset &&
-	     nla_put_u16(skb, L2TP_ATTR_OFFSET, session->offset)) ||
-	    (session->peer_offset &&
-	     nla_put_u16(skb, L2TP_ATTR_PEER_OFFSET, session->peer_offset)) ||
 	    (session->cookie_len &&
 	     nla_put(skb, L2TP_ATTR_COOKIE, session->cookie_len,
 		     &session->cookie[0])) ||
@@ -921,7 +898,6 @@ static const struct nla_policy l2tp_nl_policy[L2TP_ATTR_MAX + 1] = {
 	[L2TP_ATTR_PW_TYPE]		= { .type = NLA_U16, },
 	[L2TP_ATTR_ENCAP_TYPE]		= { .type = NLA_U16, },
 	[L2TP_ATTR_OFFSET]		= { .type = NLA_U16, },
-	[L2TP_ATTR_PEER_OFFSET]		= { .type = NLA_U16, },
 	[L2TP_ATTR_DATA_SEQ]		= { .type = NLA_U8, },
 	[L2TP_ATTR_L2SPEC_TYPE]		= { .type = NLA_U8, },
 	[L2TP_ATTR_L2SPEC_LEN]		= { .type = NLA_U8, },