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?
