[PATCH net-next 0/2] l2tp: fix offset/peer_offset conf parameters

[PATCH net-next 0/2] l2tp: fix offset/peer_offset conf parameters

[Lorenzo Bianconi]

This patchset add peer_offset configuration parameter in order to
specify two different values for payload offset on tx/rx side.
Moreover fix missing print session offset info

Hangbin Liu (1):
  l2tp: fix missing print session offset info

Lorenzo Bianconi (1):
  l2tp: add peer_offset parameter

 include/uapi/linux/l2tp.h |  1 +
 net/l2tp/l2tp_core.c      |  3 ++-
 net/l2tp/l2tp_core.h      | 13 ++++++++++---
 net/l2tp/l2tp_debugfs.c   |  8 +++++---
 net/l2tp/l2tp_netlink.c   | 23 ++++++++++++++++++++++-
 5 files changed, 40 insertions(+), 8 deletions(-)

-- 
2.13.6

[PATCH net-next 1/2] l2tp: fix missing print session offset info

[Lorenzo Bianconi]

From: Hangbin Liu <liuhangbin@gmail.com>

Report offset parameter in L2TP_CMD_SESSION_GET command if
it has been configured by userspace

Fixes: 309795f4bec ("l2tp: Add netlink control API for L2TP")
Reported-by: Jianlin Shi <jishi@redhat.com>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
 net/l2tp/l2tp_netlink.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
index a1f24fb2be98..7e9c50125556 100644
--- a/net/l2tp/l2tp_netlink.c
+++ b/net/l2tp/l2tp_netlink.c
@@ -761,6 +761,8 @@ 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->cookie_len &&
 	     nla_put(skb, L2TP_ATTR_COOKIE, session->cookie_len,
 		     &session->cookie[0])) ||
-- 
2.13.6

[PATCH net-next 2/2] l2tp: add peer_offset parameter

[Lorenzo Bianconi]

Introduce peer_offset parameter in order to add the capability
to specify two different values for payload offset on tx/rx side.
If just offset is provided by userspace use it for rx side as well
in order to maintain compatibility with older l2tp versions

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
 include/uapi/linux/l2tp.h |  1 +
 net/l2tp/l2tp_core.c      |  3 ++-
 net/l2tp/l2tp_core.h      | 13 ++++++++++---
 net/l2tp/l2tp_debugfs.c   |  8 +++++---
 net/l2tp/l2tp_netlink.c   | 21 ++++++++++++++++++++-
 5 files changed, 38 insertions(+), 8 deletions(-)

diff --git a/include/uapi/linux/l2tp.h b/include/uapi/linux/l2tp.h
index d84ce5c1c9aa..d6fee55dbded 100644
--- a/include/uapi/linux/l2tp.h
+++ b/include/uapi/linux/l2tp.h
@@ -127,6 +127,7 @@ 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,
 };
 
diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 115918ad8eca..6ff64717da1e 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -792,7 +792,7 @@ void l2tp_recv_common(struct l2tp_session *session, struct sk_buff *skb,
 			ptr += 2 + offset;
 		}
 	} else
-		ptr += session->offset;
+		ptr += session->peer_offset;
 
 	offset = ptr - optr;
 	if (!pskb_may_pull(skb, offset))
@@ -1785,6 +1785,7 @@ struct l2tp_session *l2tp_session_create(int priv_size, struct l2tp_tunnel *tunn
 			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;
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index 9534e16965cc..c6fe7cc42a05 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -59,7 +59,8 @@ struct l2tp_session_cfg {
 	int			debug;		/* bitmask of debug message
 						 * categories */
 	u16			vlan_id;	/* VLAN pseudowire only */
-	u16			offset;		/* offset to payload */
+	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 */
@@ -86,8 +87,14 @@ 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 data */
+	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;
diff --git a/net/l2tp/l2tp_debugfs.c b/net/l2tp/l2tp_debugfs.c
index eb69411bcb47..4cc30b38aba4 100644
--- a/net/l2tp/l2tp_debugfs.c
+++ b/net/l2tp/l2tp_debugfs.c
@@ -180,8 +180,9 @@ 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 l2specific %hu/%hu\n",
-		   session->offset, session->l2specific_type, session->l2specific_len);
+	seq_printf(m, "   offset %hu peer_offset %hu l2specific %hu/%hu\n",
+		   session->offset, session->peer_offset,
+		   session->l2specific_type, session->l2specific_len);
 	if (session->cookie_len) {
 		seq_printf(m, "   cookie %02x%02x%02x%02x",
 			   session->cookie[0], session->cookie[1],
@@ -228,7 +229,8 @@ 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 l2specific TYPE/LEN\n");
+		seq_puts(m, "   offset OFFSET peer_offset OFFSET");
+		seq_puts(m, " 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");
diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
index 7e9c50125556..d7d4d7a7a54d 100644
--- a/net/l2tp/l2tp_netlink.c
+++ b/net/l2tp/l2tp_netlink.c
@@ -547,9 +547,25 @@ static int l2tp_nl_cmd_session_create(struct sk_buff *skb, struct genl_info *inf
 	}
 
 	if (tunnel->version > 2) {
-		if (info->attrs[L2TP_ATTR_OFFSET])
+		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]);
 
@@ -763,6 +779,8 @@ static int l2tp_nl_session_send(struct sk_buff *skb, u32 portid, u32 seq, int fl
 	     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])) ||
@@ -903,6 +921,7 @@ 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, },
-- 
2.13.6

Re: [PATCH net-next 0/2] l2tp: fix offset/peer_offset conf parameters

[David Miller]

From: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
Date: Fri, 22 Dec 2017 15:10:16 +0100

> This patchset add peer_offset configuration parameter in order to
> specify two different values for payload offset on tx/rx side.
> Moreover fix missing print session offset info

Series applied, thank you.

Re: [PATCH net-next 2/2] l2tp: add peer_offset parameter

[Guillaume Nault]

On Fri, Dec 22, 2017 at 03:10:18PM +0100, Lorenzo Bianconi wrote:
> Introduce peer_offset parameter in order to add the capability
> to specify two different values for payload offset on tx/rx side.
> If just offset is provided by userspace use it for rx side as well
> in order to maintain compatibility with older l2tp versions
> 
Sorry for being late on this, I originally missed this patchset and
only noticed it yesterday.

Lorenzo, can you give some context around this new feature?
Quite frankly I can't see the point of it. I've never heard of offsets
in L2TPv3, and for L2TPv2, the offset value is already encoded in the
header.

After a quick review of L2TPv3 and pseudowires RFCs, I still don't see
how adding some padding between the L2TPv3 header and the payload could
constitute a valid frame. Of course, the base feature is already there,
but after a quick test, it looks like the padding bits aren't
initialised and leak memory.

So, unless I missed something, setting offsets in L2TPv3 is
non-compliant, the current implementation is buggy and most likely
unused. I'd really prefer getting the implementation fixed, or even
removed entirely. Extending it to allow asymmetrical offset values
looks wrong to me, unless you have a use case in mind.

Regards,

Guillaume

PS: I also noticed that iproute2 has a "peer_offset" option, but it's a
noop.

Re: [PATCH net-next 2/2] l2tp: add peer_offset parameter

[Lorenzo Bianconi]

On Dec 28, Guillaume Nault wrote:
> On Fri, Dec 22, 2017 at 03:10:18PM +0100, Lorenzo Bianconi wrote:
> > Introduce peer_offset parameter in order to add the capability
> > to specify two different values for payload offset on tx/rx side.
> > If just offset is provided by userspace use it for rx side as well
> > in order to maintain compatibility with older l2tp versions
> > 
> Sorry for being late on this, I originally missed this patchset and
> only noticed it yesterday.
> 
> Lorenzo, can you give some context around this new feature?
> Quite frankly I can't see the point of it. I've never heard of offsets
> in L2TPv3, and for L2TPv2, the offset value is already encoded in the
> header.

Hi Guillaume,

thanks for your feedback.

> 
> After a quick review of L2TPv3 and pseudowires RFCs, I still don't see
> how adding some padding between the L2TPv3 header and the payload could
> constitute a valid frame. Of course, the base feature is already there,
> but after a quick test, it looks like the padding bits aren't
> initialised and leak memory.

Do you mean for L2TPv2 or L2TPv3? For L2TPv3 offset/peer_offset are initialized
in l2tp_nl_cmd_session_create()

> 
> So, unless I missed something, setting offsets in L2TPv3 is
> non-compliant, the current implementation is buggy and most likely
> unused. I'd really prefer getting the implementation fixed, or even
> removed entirely. Extending it to allow asymmetrical offset values
> looks wrong to me, unless you have a use case in mind.
> 
> Regards,
> 
> Guillaume
> 
> PS: I also noticed that iproute2 has a "peer_offset" option, but it's a
> noop.

Setting session data offset is already supported in L2TP kernel module
(and could be already used by userspace applications);
for L2TPv2 there is an optional 16-bit value in the header while for L2TPv3
the offset is configured by userspace.
At the moment the kernel (for L2TPv3) uses offset for both tx and rx side.
Userspace (iproute2) allows to distinguish tx offset (offset) from rx one
(peer_offset) but since the rx part is not handled at the moment
(I fixed peer_offset support in iproute2, I have not sent the patch upstream yet, attached below)
this leads to a misalignment between tunnel endpoints.
You can easily reproduce the issue using this setup (and the below patch for iproute2):

ip l2tp add tunnel local <ip0> remote <ip1> tunnel_id <id0> peer_tunnel_id <id1> udp_sport <p0> udp_dport <p1>
ip l2tp add tunnel local <ip1> remote <ip0> tunnel_id <id1> peer_tunnel_id <id0> udp_sport <p1> udp_dport <p0>

ip l2tp add session name l2tp0 tunnel_id <id0> session_id <s0> peer_session_id <s1> offset 8 peer_offset 16
ip l2tp add session name l2tp0 tunnel_id <id1> session_id <s1> peer_session_id <s0> offset 16 peer_offset 8

commit ee1b976f22fbea530c94a5233ac8c7cd8d643ae9
Author: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
Date:   Thu Dec 21 14:41:39 2017 +0100

    ip: l2tp: add peer_offset netlink callback

diff --git a/include/uapi/linux/l2tp.h b/include/uapi/linux/l2tp.h
index 472e9924..21223df7 100644
--- a/include/uapi/linux/l2tp.h
+++ b/include/uapi/linux/l2tp.h
@@ -127,6 +127,7 @@ 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,
 };
 
diff --git a/ip/ipl2tp.c b/ip/ipl2tp.c
index 7c5ed313..a3220a8b 100644
--- a/ip/ipl2tp.c
+++ b/ip/ipl2tp.c
@@ -176,6 +176,8 @@ static int create_session(struct l2tp_parm *p)
 					  p->reorder_timeout);
 	if (p->offset)
 		addattr16(&req.n, 1024, L2TP_ATTR_OFFSET, p->offset);
+	if (p->peer_offset)
+		addattr16(&req.n, 1024, L2TP_ATTR_PEER_OFFSET, p->peer_offset);
 	if (p->cookie_len)
 		addattr_l(&req.n, 1024, L2TP_ATTR_COOKIE,
 			  p->cookie, p->cookie_len);
@@ -316,6 +318,8 @@ static int get_response(struct nlmsghdr *n, void *arg)
 		p->encap = rta_getattr_u16(attrs[L2TP_ATTR_ENCAP_TYPE]);
 	if (attrs[L2TP_ATTR_OFFSET])
 		p->offset = rta_getattr_u16(attrs[L2TP_ATTR_OFFSET]);
+	if (attrs[L2TP_ATTR_PEER_OFFSET])
+		p->peer_offset = rta_getattr_u16(attrs[L2TP_ATTR_PEER_OFFSET]);
 	if (attrs[L2TP_ATTR_DATA_SEQ])
 		p->data_seq = rta_getattr_u16(attrs[L2TP_ATTR_DATA_SEQ]);
 	if (attrs[L2TP_ATTR_CONN_ID])


Regards,
Lorenzo

Re: [PATCH net-next 2/2] l2tp: add peer_offset parameter

[Guillaume Nault]

On Thu, Dec 28, 2017 at 07:23:48PM +0100, Lorenzo Bianconi wrote:
> On Dec 28, Guillaume Nault wrote:
> > After a quick review of L2TPv3 and pseudowires RFCs, I still don't see
> > how adding some padding between the L2TPv3 header and the payload could
> > constitute a valid frame. Of course, the base feature is already there,
> > but after a quick test, it looks like the padding bits aren't
> > initialised and leak memory.
> 
> Do you mean for L2TPv2 or L2TPv3? For L2TPv3 offset/peer_offset are initialized
> in l2tp_nl_cmd_session_create()
>
That's the offsets stored in the l2tp_session_cfg structure. But I was
talking about the xmit path: l2tp_build_l2tpv3_header() doesn't
initialise the padding between the header and the payload. So when
someone activates this option, then every transmitted packet leaks
memory on the wire.

> Setting session data offset is already supported in L2TP kernel module
> (and could be already used by userspace applications);
> for L2TPv2 there is an optional 16-bit value in the header while for L2TPv3
> the offset is configured by userspace.
> At the moment the kernel (for L2TPv3) uses offset for both tx and rx side.
> Userspace (iproute2) allows to distinguish tx offset (offset) from rx one
> (peer_offset) but since the rx part is not handled at the moment
> (I fixed peer_offset support in iproute2, I have not sent the patch upstream yet, attached below)
> this leads to a misalignment between tunnel endpoints.
> You can easily reproduce the issue using this setup (and the below patch for iproute2):
> 
> ip l2tp add tunnel local <ip0> remote <ip1> tunnel_id <id0> peer_tunnel_id <id1> udp_sport <p0> udp_dport <p1>
> ip l2tp add tunnel local <ip1> remote <ip0> tunnel_id <id1> peer_tunnel_id <id0> udp_sport <p1> udp_dport <p0>
> 
> ip l2tp add session name l2tp0 tunnel_id <id0> session_id <s0> peer_session_id <s1> offset 8 peer_offset 16
> ip l2tp add session name l2tp0 tunnel_id <id1> session_id <s1> peer_session_id <s0> offset 16 peer_offset 8
> 
Yes, I'm well aware of that. And thanks for having worked on a full
solution including iproute2. But does one really need to set
asymetrical offset values? It doesn't look wrong to require setting the
same value on both sides. Other options need this, like "l2spec_type".

Here we have an option that:
  * creates invalid packets (AFAIK),
  * is buggy and leaks memory on the network,
  * doesn't seem to have any use case (even the manpage
    says "This is hardly ever used").

So I'm sorry, but I don't see the point in expanding this option to
allow even stranger setups. If there's a use case, then fine.
Otherwise, let's just acknowledge that the "peer_offset" option of
iproute2 is a noop (and maybe remove it from the manpage).

And the kernel "offset" option needs to be fixed. Actually, I wouldn't
mind if it was converted to be a noop, or even rejected. L2TP already
has its share of unused features that complicate the code and hamper
evolution and bug fixing. As I said earlier, if it's buggy, unused and
can't even produce valid packets, then why bothering with it?

But that's just my point of view. James, do you have an opinion on
this?

Re: [PATCH net-next 2/2] l2tp: add peer_offset parameter

[James Chapman]

Sorry for only just seeing this (vacation).

On 28/12/17 19:45, Guillaume Nault wrote:
> On Thu, Dec 28, 2017 at 07:23:48PM +0100, Lorenzo Bianconi wrote:
>> On Dec 28, Guillaume Nault wrote:
>>> After a quick review of L2TPv3 and pseudowires RFCs, I still don't see
>>> how adding some padding between the L2TPv3 header and the payload could
>>> constitute a valid frame. Of course, the base feature is already there,
>>> but after a quick test, it looks like the padding bits aren't
>>> initialised and leak memory.
>> Do you mean for L2TPv2 or L2TPv3? For L2TPv3 offset/peer_offset are initialized
>> in l2tp_nl_cmd_session_create()
>>
> That's the offsets stored in the l2tp_session_cfg structure. But I was
> talking about the xmit path: l2tp_build_l2tpv3_header() doesn't
> initialise the padding between the header and the payload. So when
> someone activates this option, then every transmitted packet leaks
> memory on the wire.
>
>> Setting session data offset is already supported in L2TP kernel module
>> (and could be already used by userspace applications);
>> for L2TPv2 there is an optional 16-bit value in the header while for L2TPv3
>> the offset is configured by userspace.
>> At the moment the kernel (for L2TPv3) uses offset for both tx and rx side.
>> Userspace (iproute2) allows to distinguish tx offset (offset) from rx one
>> (peer_offset) but since the rx part is not handled at the moment
>> (I fixed peer_offset support in iproute2, I have not sent the patch upstream yet, attached below)
>> this leads to a misalignment between tunnel endpoints.
>> You can easily reproduce the issue using this setup (and the below patch for iproute2):
>>
>> ip l2tp add tunnel local <ip0> remote <ip1> tunnel_id <id0> peer_tunnel_id <id1> udp_sport <p0> udp_dport <p1>
>> ip l2tp add tunnel local <ip1> remote <ip0> tunnel_id <id1> peer_tunnel_id <id0> udp_sport <p1> udp_dport <p0>
>>
>> ip l2tp add session name l2tp0 tunnel_id <id0> session_id <s0> peer_session_id <s1> offset 8 peer_offset 16
>> ip l2tp add session name l2tp0 tunnel_id <id1> session_id <s1> peer_session_id <s0> offset 16 peer_offset 8
>>
> Yes, I'm well aware of that. And thanks for having worked on a full
> solution including iproute2. But does one really need to set
> asymetrical offset values? It doesn't look wrong to require setting the
> same value on both sides. Other options need this, like "l2spec_type".
>
> Here we have an option that:
>    * creates invalid packets (AFAIK),
>    * is buggy and leaks memory on the network,
>    * doesn't seem to have any use case (even the manpage
>      says "This is hardly ever used").
>
> So I'm sorry, but I don't see the point in expanding this option to
> allow even stranger setups. If there's a use case, then fine.
> Otherwise, let's just acknowledge that the "peer_offset" option of
> iproute2 is a noop (and maybe remove it from the manpage).
>
> And the kernel "offset" option needs to be fixed. Actually, I wouldn't
> mind if it was converted to be a noop, or even rejected. L2TP already
> has its share of unused features that complicate the code and hamper
> evolution and bug fixing. As I said earlier, if it's buggy, unused and
> can't even produce valid packets, then why bothering with it?
>
> But that's just my point of view. James, do you have an opinion on
> this?

I agree, Guillaume.

The L2TPv3 protocol RFC dropped the configurable offset of L2TPv2 - 
instead, the Layer-2-Specific-Sublayer is supposed to handle any 
transport-specific data alignment requirements. I think a configurable 
offset has found its way into iproute2 l2tp commands by mistake, perhaps 
because the netlink API defines an attribute for it, but which was only 
intended for use with L2TPv2. For L2TPv2, we only configure the offset 
for transmitted packets. In received packets, the offset (if present) is 
obtained from the L2TPv2 header in each received packet. There is no 
need to add a peer-offset netlink attribute to set the offset expected 
in received packets.

Lorenzo, is this being added to fix interoperability with another L2TPv3 
implementation? If so, can you share more details?

Re: [PATCH net-next 2/2] l2tp: add peer_offset parameter

[Lorenzo Bianconi]

> Sorry for only just seeing this (vacation).
>
>
> On 28/12/17 19:45, Guillaume Nault wrote:
>>
>> On Thu, Dec 28, 2017 at 07:23:48PM +0100, Lorenzo Bianconi wrote:
>>>
>>> On Dec 28, Guillaume Nault wrote:
>>>>
>>>> After a quick review of L2TPv3 and pseudowires RFCs, I still don't see
>>>> how adding some padding between the L2TPv3 header and the payload could
>>>> constitute a valid frame. Of course, the base feature is already there,
>>>> but after a quick test, it looks like the padding bits aren't
>>>> initialised and leak memory.
>>>
>>> Do you mean for L2TPv2 or L2TPv3? For L2TPv3 offset/peer_offset are
>>> initialized
>>> in l2tp_nl_cmd_session_create()
>>>
>> That's the offsets stored in the l2tp_session_cfg structure. But I was
>> talking about the xmit path: l2tp_build_l2tpv3_header() doesn't
>> initialise the padding between the header and the payload. So when
>> someone activates this option, then every transmitted packet leaks
>> memory on the wire.
>>
>>> Setting session data offset is already supported in L2TP kernel module
>>> (and could be already used by userspace applications);
>>> for L2TPv2 there is an optional 16-bit value in the header while for
>>> L2TPv3
>>> the offset is configured by userspace.
>>> At the moment the kernel (for L2TPv3) uses offset for both tx and rx
>>> side.
>>> Userspace (iproute2) allows to distinguish tx offset (offset) from rx one
>>> (peer_offset) but since the rx part is not handled at the moment
>>> (I fixed peer_offset support in iproute2, I have not sent the patch
>>> upstream yet, attached below)
>>> this leads to a misalignment between tunnel endpoints.
>>> You can easily reproduce the issue using this setup (and the below patch
>>> for iproute2):
>>>
>>> ip l2tp add tunnel local <ip0> remote <ip1> tunnel_id <id0>
>>> peer_tunnel_id <id1> udp_sport <p0> udp_dport <p1>
>>> ip l2tp add tunnel local <ip1> remote <ip0> tunnel_id <id1>
>>> peer_tunnel_id <id0> udp_sport <p1> udp_dport <p0>
>>>
>>> ip l2tp add session name l2tp0 tunnel_id <id0> session_id <s0>
>>> peer_session_id <s1> offset 8 peer_offset 16
>>> ip l2tp add session name l2tp0 tunnel_id <id1> session_id <s1>
>>> peer_session_id <s0> offset 16 peer_offset 8
>>>
>> Yes, I'm well aware of that. And thanks for having worked on a full
>> solution including iproute2. But does one really need to set
>> asymetrical offset values? It doesn't look wrong to require setting the
>> same value on both sides. Other options need this, like "l2spec_type".
>>
>> Here we have an option that:
>>    * creates invalid packets (AFAIK),
>>    * is buggy and leaks memory on the network,
>>    * doesn't seem to have any use case (even the manpage
>>      says "This is hardly ever used").
>>
>> So I'm sorry, but I don't see the point in expanding this option to
>> allow even stranger setups. If there's a use case, then fine.
>> Otherwise, let's just acknowledge that the "peer_offset" option of
>> iproute2 is a noop (and maybe remove it from the manpage).
>>
>> And the kernel "offset" option needs to be fixed. Actually, I wouldn't
>> mind if it was converted to be a noop, or even rejected. L2TP already
>> has its share of unused features that complicate the code and hamper
>> evolution and bug fixing. As I said earlier, if it's buggy, unused and
>> can't even produce valid packets, then why bothering with it?
>>
>> But that's just my point of view. James, do you have an opinion on
>> this?
>
>
> I agree, Guillaume.
>
> The L2TPv3 protocol RFC dropped the configurable offset of L2TPv2 - instead,
> the Layer-2-Specific-Sublayer is supposed to handle any transport-specific
> data alignment requirements. I think a configurable offset has found its way
> into iproute2 l2tp commands by mistake, perhaps because the netlink API
> defines an attribute for it, but which was only intended for use with
> L2TPv2. For L2TPv2, we only configure the offset for transmitted packets. In
> received packets, the offset (if present) is obtained from the L2TPv2 header
> in each received packet. There is no need to add a peer-offset netlink
> attribute to set the offset expected in received packets.
>
> Lorenzo, is this being added to fix interoperability with another L2TPv3
> implementation? If so, can you share more details?
>

Hi James,

I introduced peer_offset parameter to fix a specific setup where
tunnel endpoints
running L2TPv3 would use different values for tx offset (since in
iproute2 there is no
restriction on it), not to fix a given an interoperability issue.
I introduced this feature since:
 - offset has been added for long time to L2TPv3 implementation
   (commit f7faffa3ff8ef6ae712ef16312b8a2aa7a1c95fe and
   commit 309795f4bec2d69cd507a631f82065c2198a0825) and I wanted to
preserve UABI
 - have the same degree of freedom for offset parameter we have in
L2TPv2 and fix the issue
   described above

Now what we can do I guess is:
- as suggested by Guillaume drop completely the offset support without removing
  netlink attribute in order to not break UABI
- fix offset support initializing properly padding bits

I think at the moment we can skip the option to impose to have the
same offset value
for both tx and rx side.

Regards,
Lorenzo

Re: [PATCH net-next 2/2] l2tp: add peer_offset parameter

[Guillaume Nault]

On Fri, Dec 29, 2017 at 06:53:56PM +0000, James Chapman wrote:
> On 28/12/17 19:45, Guillaume Nault wrote:
> > Here we have an option that:
> >    * creates invalid packets (AFAIK),
> >    * is buggy and leaks memory on the network,
> >    * doesn't seem to have any use case (even the manpage
> >      says "This is hardly ever used").
> > 
> > So I'm sorry, but I don't see the point in expanding this option to
> > allow even stranger setups. If there's a use case, then fine.
> > Otherwise, let's just acknowledge that the "peer_offset" option of
> > iproute2 is a noop (and maybe remove it from the manpage).
> > 
> > And the kernel "offset" option needs to be fixed. Actually, I wouldn't
> > mind if it was converted to be a noop, or even rejected. L2TP already
> > has its share of unused features that complicate the code and hamper
> > evolution and bug fixing. As I said earlier, if it's buggy, unused and
> > can't even produce valid packets, then why bothering with it?
> > 
> > But that's just my point of view. James, do you have an opinion on
> > this?
> 
> I agree, Guillaume.
> 
> The L2TPv3 protocol RFC dropped the configurable offset of L2TPv2 - instead,
> the Layer-2-Specific-Sublayer is supposed to handle any transport-specific
> data alignment requirements.
> 
Yes, and AFAIK, no RFC has ever defined an L2TPv3 sublayer using offsets.

> I think a configurable offset has found its way
> into iproute2 l2tp commands by mistake, perhaps because the netlink API
> defines an attribute for it, but which was only intended for use with
> L2TPv2.
>
Makes sense, however L2TP_ATTR_OFFSET seems to be a noop for L2TPv2 in
the current implementation.

> For L2TPv2, we only configure the offset for transmitted packets. In
> received packets, the offset (if present) is obtained from the L2TPv2 header
> in each received packet. There is no need to add a peer-offset netlink
> attribute to set the offset expected in received packets.
> 
Agreed for Rx side. I also agree on the theory for Tx, but in the current
implementation, l2tp_build_l2tpv2_header() doesn't take the session's
"offset" field into account. So, unless I've missed something,
L2TP_ATTR_OFFSET is already a noop for L2TPv2.

Not sure if it's worth handling this feature of L2TPv2. The Linux
implementation has been there for so long, and nobody ever complained
that there was no way to define an offset on Tx.

Re: [PATCH net-next 2/2] l2tp: add peer_offset parameter

[Guillaume Nault]

> > Lorenzo, is this being added to fix interoperability with another L2TPv3
> > implementation? If so, can you share more details?
> >
> 
> Hi James,
> 
> I introduced peer_offset parameter to fix a specific setup where
> tunnel endpoints
> running L2TPv3 would use different values for tx offset (since in
> iproute2 there is no
> restriction on it), not to fix a given an interoperability issue.
> 
Yes, but was it just to test iproute2's peer_offset option? Or is there
a plan to use it for real?

> I introduced this feature since:
>  - offset has been added for long time to L2TPv3 implementation
>    (commit f7faffa3ff8ef6ae712ef16312b8a2aa7a1c95fe and
>    commit 309795f4bec2d69cd507a631f82065c2198a0825) and I wanted to
> preserve UABI
>  - have the same degree of freedom for offset parameter we have in
> L2TPv2 and fix the issue
>    described above
> 
AFAIU, the current L2TPv2 implementation never sets the offset field
and nobody ever realised.

> Now what we can do I guess is:
> - as suggested by Guillaume drop completely the offset support without removing
>   netlink attribute in order to not break UABI
> - fix offset support initializing properly padding bits
> 
I'd go for the first one. I just wonder if that looks acceptable to
David an James.

Re: [PATCH net-next 2/2] l2tp: add peer_offset parameter

[Lorenzo Bianconi]

>> > Lorenzo, is this being added to fix interoperability with another L2TPv3
>> > implementation? If so, can you share more details?
>> >
>>
>> Hi James,
>>
>> I introduced peer_offset parameter to fix a specific setup where
>> tunnel endpoints
>> running L2TPv3 would use different values for tx offset (since in
>> iproute2 there is no
>> restriction on it), not to fix a given an interoperability issue.
>>
> Yes, but was it just to test iproute2's peer_offset option? Or is there
> a plan to use it for real?
>
>> I introduced this feature since:
>>  - offset has been added for long time to L2TPv3 implementation
>>    (commit f7faffa3ff8ef6ae712ef16312b8a2aa7a1c95fe and
>>    commit 309795f4bec2d69cd507a631f82065c2198a0825) and I wanted to
>> preserve UABI
>>  - have the same degree of freedom for offset parameter we have in
>> L2TPv2 and fix the issue
>>    described above
>>
> AFAIU, the current L2TPv2 implementation never sets the offset field
> and nobody ever realised.
>

Perhaps I am little bit polarized on UABI issue, but I was rethinking
about it and maybe removing offset parameter would lead to an
interoperability issue for device running L2TPv3 since offset
parameter is there and it is not a nope.
Please consider this setup:
- 2 endpoint running L2TPv3, the first running net-next and the second
running 4.14
- both endpoint are configured using iproute2 in this way:

  - ip l2tp add tunnel local <ip0> remote <ip1> tunnel_id <id0>
peer_tunnel_id <id1> udp_sport <p0> udp_dport <p1>
  - ip l2tp add tunnel local <ip1> remote <ip0> tunnel_id <id1>
peer_tunnel_id <id0> udp_sport <p1> udp_dport <p0>
  - ip l2tp add session name l2tp0 tunnel_id <id0> session_id <s0>
peer_session_id <s1> offset 8
  - ip l2tp add session name l2tp0 tunnel_id <id1> session_id <s1>
peer_session_id <s0> offset 8

Can we assume offset is never used for L2TPv3?

Regards,
Lorenzo

>> Now what we can do I guess is:
>> - as suggested by Guillaume drop completely the offset support without removing
>>   netlink attribute in order to not break UABI
>> - fix offset support initializing properly padding bits
>>
> I'd go for the first one. I just wonder if that looks acceptable to
> David an James.

Re: [PATCH net-next 2/2] l2tp: add peer_offset parameter

[James Chapman]

On 02/01/18 17:50, Guillaume Nault wrote:
> On Fri, Dec 29, 2017 at 06:53:56PM +0000, James Chapman wrote:
>> On 28/12/17 19:45, Guillaume Nault wrote:
>>> Here we have an option that:
>>>     * creates invalid packets (AFAIK),
>>>     * is buggy and leaks memory on the network,
>>>     * doesn't seem to have any use case (even the manpage
>>>       says "This is hardly ever used").
>>>
>>> So I'm sorry, but I don't see the point in expanding this option to
>>> allow even stranger setups. If there's a use case, then fine.
>>> Otherwise, let's just acknowledge that the "peer_offset" option of
>>> iproute2 is a noop (and maybe remove it from the manpage).
>>>
>>> And the kernel "offset" option needs to be fixed. Actually, I wouldn't
>>> mind if it was converted to be a noop, or even rejected. L2TP already
>>> has its share of unused features that complicate the code and hamper
>>> evolution and bug fixing. As I said earlier, if it's buggy, unused and
>>> can't even produce valid packets, then why bothering with it?
>>>
>>> But that's just my point of view. James, do you have an opinion on
>>> this?
>> I agree, Guillaume.
>>
>> The L2TPv3 protocol RFC dropped the configurable offset of L2TPv2 - instead,
>> the Layer-2-Specific-Sublayer is supposed to handle any transport-specific
>> data alignment requirements.
>>
> Yes, and AFAIK, no RFC has ever defined an L2TPv3 sublayer using offsets.
>
>> I think a configurable offset has found its way
>> into iproute2 l2tp commands by mistake, perhaps because the netlink API
>> defines an attribute for it, but which was only intended for use with
>> L2TPv2.
>>
> Makes sense, however L2TP_ATTR_OFFSET seems to be a noop for L2TPv2 in
> the current implementation.
>
>> For L2TPv2, we only configure the offset for transmitted packets. In
>> received packets, the offset (if present) is obtained from the L2TPv2 header
>> in each received packet. There is no need to add a peer-offset netlink
>> attribute to set the offset expected in received packets.
>>
> Agreed for Rx side. I also agree on the theory for Tx, but in the current
> implementation, l2tp_build_l2tpv2_header() doesn't take the session's
> "offset" field into account. So, unless I've missed something,
> L2TP_ATTR_OFFSET is already a noop for L2TPv2.

You're right. My bad.

> Not sure if it's worth handling this feature of L2TPv2. The Linux
> implementation has been there for so long, and nobody ever complained
> that there was no way to define an offset on Tx.

I agree.

Re: [PATCH net-next 2/2] l2tp: add peer_offset parameter

[James Chapman]

On 02/01/18 18:05, Guillaume Nault wrote:
>>> Lorenzo, is this being added to fix interoperability with another L2TPv3
>>> implementation? If so, can you share more details?
>>>
>> Hi James,
>>
>> I introduced peer_offset parameter to fix a specific setup where
>> tunnel endpoints
>> running L2TPv3 would use different values for tx offset (since in
>> iproute2 there is no
>> restriction on it), not to fix a given an interoperability issue.
>>
> Yes, but was it just to test iproute2's peer_offset option? Or is there
> a plan to use it for real?
>
>> I introduced this feature since:
>>   - offset has been added for long time to L2TPv3 implementation
>>     (commit f7faffa3ff8ef6ae712ef16312b8a2aa7a1c95fe and
>>     commit 309795f4bec2d69cd507a631f82065c2198a0825) and I wanted to
>> preserve UABI
>>   - have the same degree of freedom for offset parameter we have in
>> L2TPv2 and fix the issue
>>     described above
>>
> AFAIU, the current L2TPv2 implementation never sets the offset field
> and nobody ever realised.
>
>> Now what we can do I guess is:
>> - as suggested by Guillaume drop completely the offset support without removing
>>    netlink attribute in order to not break UABI
>> - fix offset support initializing properly padding bits
>>
> I'd go for the first one. I just wonder if that looks acceptable to
> David an James.

I think the first one too. Also update iproute2 to remove or hide the 
offset and peer_offset parameters.

Re: [PATCH net-next 2/2] l2tp: add peer_offset parameter

[James Chapman]

On 02/01/18 19:28, Lorenzo Bianconi wrote:
>>>> Lorenzo, is this being added to fix interoperability with another L2TPv3
>>>> implementation? If so, can you share more details?
>>>>
>>> Hi James,
>>>
>>> I introduced peer_offset parameter to fix a specific setup where
>>> tunnel endpoints
>>> running L2TPv3 would use different values for tx offset (since in
>>> iproute2 there is no
>>> restriction on it), not to fix a given an interoperability issue.
>>>
>> Yes, but was it just to test iproute2's peer_offset option? Or is there
>> a plan to use it for real?
>>
>>> I introduced this feature since:
>>>   - offset has been added for long time to L2TPv3 implementation
>>>     (commit f7faffa3ff8ef6ae712ef16312b8a2aa7a1c95fe and
>>>     commit 309795f4bec2d69cd507a631f82065c2198a0825) and I wanted to
>>> preserve UABI
>>>   - have the same degree of freedom for offset parameter we have in
>>> L2TPv2 and fix the issue
>>>     described above
>>>
>> AFAIU, the current L2TPv2 implementation never sets the offset field
>> and nobody ever realised.
>>
> Perhaps I am little bit polarized on UABI issue, but I was rethinking
> about it and maybe removing offset parameter would lead to an
> interoperability issue for device running L2TPv3 since offset
> parameter is there and it is not a nope.
> Please consider this setup:
> - 2 endpoint running L2TPv3, the first running net-next and the second
> running 4.14
> - both endpoint are configured using iproute2 in this way:
>
>    - ip l2tp add tunnel local <ip0> remote <ip1> tunnel_id <id0>
> peer_tunnel_id <id1> udp_sport <p0> udp_dport <p1>
>    - ip l2tp add tunnel local <ip1> remote <ip0> tunnel_id <id1>
> peer_tunnel_id <id0> udp_sport <p1> udp_dport <p0>
>    - ip l2tp add session name l2tp0 tunnel_id <id0> session_id <s0>
> peer_session_id <s1> offset 8
>    - ip l2tp add session name l2tp0 tunnel_id <id1> session_id <s1>
> peer_session_id <s0> offset 8
>
> Can we assume offset is never used for L2TPv3?
If offset is ever set non-zero, packets transmitted on the wire are not 
compliant with the L2TPv3 RFC. So if someone is using a non-zero offset 
in their config, it might be a good thing that it stops working with a 
kernel update.

> Regards,
> Lorenzo
>
>>> Now what we can do I guess is:
>>> - as suggested by Guillaume drop completely the offset support without removing
>>>    netlink attribute in order to not break UABI
>>> - fix offset support initializing properly padding bits
>>>
>> I'd go for the first one. I just wonder if that looks acceptable to
>> David an James.

Re: [PATCH net-next 2/2] l2tp: add peer_offset parameter

[James Chapman]

On 02/01/18 20:08, James Chapman wrote:
> On 02/01/18 18:05, Guillaume Nault wrote:
>>>> Lorenzo, is this being added to fix interoperability with another 
>>>> L2TPv3
>>>> implementation? If so, can you share more details?
>>>>
>>> Hi James,
>>>
>>> I introduced peer_offset parameter to fix a specific setup where
>>> tunnel endpoints
>>> running L2TPv3 would use different values for tx offset (since in
>>> iproute2 there is no
>>> restriction on it), not to fix a given an interoperability issue.
>>>
>> Yes, but was it just to test iproute2's peer_offset option? Or is there
>> a plan to use it for real?
>>
>>> I introduced this feature since:
>>>   - offset has been added for long time to L2TPv3 implementation
>>>     (commit f7faffa3ff8ef6ae712ef16312b8a2aa7a1c95fe and
>>>     commit 309795f4bec2d69cd507a631f82065c2198a0825) and I wanted to
>>> preserve UABI
>>>   - have the same degree of freedom for offset parameter we have in
>>> L2TPv2 and fix the issue
>>>     described above
>>>
>> AFAIU, the current L2TPv2 implementation never sets the offset field
>> and nobody ever realised.
>>
>>> Now what we can do I guess is:
>>> - as suggested by Guillaume drop completely the offset support 
>>> without removing
>>>    netlink attribute in order to not break UABI
>>> - fix offset support initializing properly padding bits
>>>
>> I'd go for the first one. I just wonder if that looks acceptable to
>> David an James.
>
> I think the first one too. Also update iproute2 to remove or hide the 
> offset and peer_offset parameters.
>
>
I just realised the peer_offset attribute changes are already applied in 
net-next. (I missed these when they were submitted just before 
Christmas.) Should these commits be reverted? We probably don't want 
v4.15 to get an additional l2tp peer_offset attribute if we are going to 
remove it and the rest of the code supporting configurable offset 
attributes in the next release.

81487bf Merge branch 'l2tp-next'
f15bc54 l2tp: add peer_offset parameter
820da53 l2tp: fix missing print session offset info

Re: [PATCH net-next 2/2] l2tp: add peer_offset parameter

[Guillaume Nault]

On Tue, Jan 02, 2018 at 08:28:03PM +0100, Lorenzo Bianconi wrote:
> Perhaps I am little bit polarized on UABI issue, but I was rethinking
> about it and maybe removing offset parameter would lead to an
> interoperability issue for device running L2TPv3 since offset
> parameter is there and it is not a nope.
> Please consider this setup:
> - 2 endpoint running L2TPv3, the first running net-next and the second
> running 4.14
> - both endpoint are configured using iproute2 in this way:
> 
>   - ip l2tp add tunnel local <ip0> remote <ip1> tunnel_id <id0>
> peer_tunnel_id <id1> udp_sport <p0> udp_dport <p1>
>   - ip l2tp add tunnel local <ip1> remote <ip0> tunnel_id <id1>
> peer_tunnel_id <id0> udp_sport <p1> udp_dport <p0>
>   - ip l2tp add session name l2tp0 tunnel_id <id0> session_id <s0>
> peer_session_id <s1> offset 8
>   - ip l2tp add session name l2tp0 tunnel_id <id1> session_id <s1>
> peer_session_id <s0> offset 8
> 
> Can we assume offset is never used for L2TPv3?
>
That's what I think. You're right worrying about ABI issues. And I
wouldn't dare proposing such a removal if I had doubts about breaking a
user setup.

Considering the lack of use cases and the absence of interoperability
of this feature, I hardly can imagine it being used.
But it's not only that: the feature has been buggy for years without
anyone noticing. And this bug wasn't difficult to spot (one just needs
to look at an L2TPv3 header in a network packet dump).

It's really the combination of these three issues (buggy, no use case
and not producing valid L2TPv3 frames) that makes me propose a removal.

Re: [PATCH net-next 2/2] l2tp: add peer_offset parameter

[Guillaume Nault]

On Tue, Jan 02, 2018 at 08:59:44PM +0000, James Chapman wrote:
> I just realised the peer_offset attribute changes are already applied in
> net-next. (I missed these when they were submitted just before Christmas.)
> Should these commits be reverted? We probably don't want v4.15 to get an
> additional l2tp peer_offset attribute if we are going to remove it and the
> rest of the code supporting configurable offset attributes in the next
> release.
> 
Yes, I agree for a revert. I'm sorry for Lorenzo's work but I'd rather
not expand the user API in this direction.

Re: [PATCH net-next 2/2] l2tp: add peer_offset parameter

[Lorenzo Bianconi]

> On Tue, Jan 02, 2018 at 08:28:03PM +0100, Lorenzo Bianconi wrote:
>> Perhaps I am little bit polarized on UABI issue, but I was rethinking
>> about it and maybe removing offset parameter would lead to an
>> interoperability issue for device running L2TPv3 since offset
>> parameter is there and it is not a nope.
>> Please consider this setup:
>> - 2 endpoint running L2TPv3, the first running net-next and the second
>> running 4.14
>> - both endpoint are configured using iproute2 in this way:
>>
>>   - ip l2tp add tunnel local <ip0> remote <ip1> tunnel_id <id0>
>> peer_tunnel_id <id1> udp_sport <p0> udp_dport <p1>
>>   - ip l2tp add tunnel local <ip1> remote <ip0> tunnel_id <id1>
>> peer_tunnel_id <id0> udp_sport <p1> udp_dport <p0>
>>   - ip l2tp add session name l2tp0 tunnel_id <id0> session_id <s0>
>> peer_session_id <s1> offset 8
>>   - ip l2tp add session name l2tp0 tunnel_id <id1> session_id <s1>
>> peer_session_id <s0> offset 8
>>
>> Can we assume offset is never used for L2TPv3?
>>
> That's what I think. You're right worrying about ABI issues. And I
> wouldn't dare proposing such a removal if I had doubts about breaking a
> user setup.
>
> Considering the lack of use cases and the absence of interoperability
> of this feature, I hardly can imagine it being used.
> But it's not only that: the feature has been buggy for years without
> anyone noticing. And this bug wasn't difficult to spot (one just needs
> to look at an L2TPv3 header in a network packet dump).
>
> It's really the combination of these three issues (buggy, no use case
> and not producing valid L2TPv3 frames) that makes me propose a removal.

Hi Guillaume, James,

I agree to remove offset parameter in this case. What about (as
already suggested by James) to take into account possible alignment
issues with previous version of L2TPv3 protocol using 'L2 specific
sublayer'?
I guess, on the kernel side (we will need to patch iproute2 on
userspace side), we need just to properly initialized the 'l2specific'
field to 0 since otherwise we will have the same memleak issue there
if assume we can have l2specific_len != {0,4}.
Moreover does it worth to add some sanity checks in netlink code to
enforce the relation between l2specific_len and l2specific_type? At
the moment there are no guarantee that if l2specific_type is set to
L2TP_L2SPECTYPE_DEFAULT, l2specific_len will be grater or equal than
4.

Regards,
Lorenzo

Re: [PATCH net-next 2/2] l2tp: add peer_offset parameter

[Guillaume Nault]

On Wed, Jan 03, 2018 at 04:06:28PM +0100, Lorenzo Bianconi wrote:
> I agree to remove offset parameter in this case. What about (as
> already suggested by James) to take into account possible alignment
> issues with previous version of L2TPv3 protocol using 'L2 specific
> sublayer'?
> 
I think James was refering to the general architecture of L2TPv3, where
such issues should be handled by pseudo-wire specific headers. I don't
think he was talking about implementing arbitrary padding using an
L2 specific sublayer. None of the standardised headers allow arbitrary
padding. And implementing our own would make us imcompatible with any
other implementation.

> I guess, on the kernel side (we will need to patch iproute2 on
> userspace side), we need just to properly initialized the 'l2specific'
> field to 0 since otherwise we will have the same memleak issue there
> if assume we can have l2specific_len != {0,4}.
> 
That would produce the same frame format as what the 'offset' option
was supposed to produce (if it did properly initialise its padding
bits). That is, we'd have an arbitrary number of padding bits inserted
between the l2-specific header and the l2 frame (L2TP's payload). These
frames are invalid, and doing that brings us nothing compared to
keeping the offset option.

> Moreover does it worth to add some sanity checks in netlink code to
> enforce the relation between l2specific_len and l2specific_type?
> 
Yes, certainly.

> At
> the moment there are no guarantee that if l2specific_type is set to
> L2TP_L2SPECTYPE_DEFAULT, l2specific_len will be grater or equal than
> 4.
> 
Thanks for pointing this out. That needs to be fixed. We don't support
anything but the default l2spec layer (or no l2spec layer at all). So
we should only accept L2TP_L2SPECTYPE_NONE and L2TP_L2SPECTYPE_DEFAULT.

And we shouldn't need to use l2specific_len at all. The default l2spec
header is always four bytes long, so we don't need to rely on a user
supplied value. Looking at the code, it seems that invalid usage of
L2TP_ATTR_L2SPEC_LEN allows leaking memory on the network or sending
corrupted frames (depending on if its value is too small or too big).

Do you want to take care of this?

Re: [PATCH net-next 2/2] l2tp: add peer_offset parameter

[Lorenzo Bianconi]

> On Wed, Jan 03, 2018 at 04:06:28PM +0100, Lorenzo Bianconi wrote:
>> I agree to remove offset parameter in this case. What about (as
>> already suggested by James) to take into account possible alignment
>> issues with previous version of L2TPv3 protocol using 'L2 specific
>> sublayer'?
>>
> I think James was refering to the general architecture of L2TPv3, where
> such issues should be handled by pseudo-wire specific headers. I don't
> think he was talking about implementing arbitrary padding using an
> L2 specific sublayer. None of the standardised headers allow arbitrary
> padding. And implementing our own would make us imcompatible with any
> other implementation.
>
>> I guess, on the kernel side (we will need to patch iproute2 on
>> userspace side), we need just to properly initialized the 'l2specific'
>> field to 0 since otherwise we will have the same memleak issue there
>> if assume we can have l2specific_len != {0,4}.
>>
> That would produce the same frame format as what the 'offset' option
> was supposed to produce (if it did properly initialise its padding
> bits). That is, we'd have an arbitrary number of padding bits inserted
> between the l2-specific header and the l2 frame (L2TP's payload). These
> frames are invalid, and doing that brings us nothing compared to
> keeping the offset option.
>
>> Moreover does it worth to add some sanity checks in netlink code to
>> enforce the relation between l2specific_len and l2specific_type?
>>
> Yes, certainly.
>
>> At
>> the moment there are no guarantee that if l2specific_type is set to
>> L2TP_L2SPECTYPE_DEFAULT, l2specific_len will be grater or equal than
>> 4.
>>
> Thanks for pointing this out. That needs to be fixed. We don't support
> anything but the default l2spec layer (or no l2spec layer at all). So
> we should only accept L2TP_L2SPECTYPE_NONE and L2TP_L2SPECTYPE_DEFAULT.
>
> And we shouldn't need to use l2specific_len at all. The default l2spec
> header is always four bytes long, so we don't need to rely on a user
> supplied value. Looking at the code, it seems that invalid usage of
> L2TP_ATTR_L2SPEC_LEN allows leaking memory on the network or sending
> corrupted frames (depending on if its value is too small or too big).
>
> Do you want to take care of this?

Ack, I am working on it.
Regards,

Lorenzo