Message-ID: <CAJ0CqmWc_-L1mXqBEDJ=UWsbJ9b-Xeug2q4MVwM_EcmTsOZyeA@mail.gmail.com>
Date:   Fri, 29 Dec 2017 23:21:39 +0100
From:   Lorenzo Bianconi <lorenzo.bianconi@...hat.com>
To:     James Chapman <jchapman@...alix.com>
Cc:     Guillaume Nault <g.nault@...halink.fr>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        Hangbin Liu <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?
>

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

Powered by blists - more mailing lists