Re: [PATCH net-next 2/2] l2tp: add peer_offset parameter

[Guillaume Nault <g.nault@alphalink.fr>]

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?