> 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
