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

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

From: Hangbin Liu <redacted>

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 <redacted>
Signed-off-by: Hangbin Liu <redacted>
Signed-off-by: Lorenzo Bianconi <redacted>
---
 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

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 <redacted>
---
 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

From: Lorenzo Bianconi <redacted>
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.

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.

On Dec 28, Guillaume Nault wrote:

On Fri, Dec 22, 2017 at 03:10:18PM +0100, Lorenzo Bianconi wrote:

quoted

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 [off-list ref]
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

On Thu, Dec 28, 2017 at 07:23:48PM +0100, Lorenzo Bianconi wrote:

On Dec 28, Guillaume Nault wrote:

quoted

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?

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:

quoted

On Dec 28, Guillaume Nault wrote:

quoted

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.

quoted

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?

Sorry for only just seeing this (vacation).


On 28/12/17 19:45, Guillaume Nault wrote:

quoted

On Thu, Dec 28, 2017 at 07:23:48PM +0100, Lorenzo Bianconi wrote:

quoted

On Dec 28, Guillaume Nault wrote:

quoted

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.

quoted

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

On Fri, Dec 29, 2017 at 06:53:56PM +0000, James Chapman wrote:

On 28/12/17 19:45, Guillaume Nault wrote:

quoted

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.

quoted

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.

quoted

quoted

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?

quoted

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

quoted

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.

On 02/01/18 17:50, Guillaume Nault wrote:

On Fri, Dec 29, 2017 at 06:53:56PM +0000, James Chapman wrote:

quoted

On 28/12/17 19:45, Guillaume Nault wrote:

quoted

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.

quoted

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.

quoted

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.

On 02/01/18 18:05, Guillaume Nault wrote:

quoted

quoted

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?

quoted

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.

quoted

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.

On 02/01/18 19:28, Lorenzo Bianconi wrote:

quoted

quoted

quoted

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?

quoted

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

quoted

quoted

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.

On 02/01/18 20:08, James Chapman wrote:

On 02/01/18 18:05, Guillaume Nault wrote:

quoted

quoted

quoted

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?

quoted

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.

quoted

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

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.

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.

On Tue, Jan 02, 2018 at 08:28:03PM +0100, Lorenzo Bianconi wrote:

quoted

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

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?

On Wed, Jan 03, 2018 at 04:06:28PM +0100, Lorenzo Bianconi wrote:

quoted

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.

quoted

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.

quoted

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

Yes, certainly.

quoted

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