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