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

[Lorenzo Bianconi <lorenzo.bianconi@redhat.com>]

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