Message-ID: <27e0a7c8-b95b-04eb-d808-bcae67e417b1@katalix.com>
Date:   Fri, 29 Dec 2017 18:53:56 +0000
From:   James Chapman <jchapman@...alix.com>
To:     Guillaume Nault <g.nault@...halink.fr>,
        Lorenzo Bianconi <lorenzo.bianconi@...hat.com>
Cc:     davem@...emloft.net, netdev@...r.kernel.org, liuhangbin@...il.com
Subject: Re: [PATCH net-next 2/2] l2tp: add peer_offset parameter

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?

Powered by blists - more mailing lists