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

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

On Fri, Dec 29, 2017 at 06:53:56PM +0000, James Chapman wrote:
> On 28/12/17 19:45, Guillaume Nault wrote:
> > 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.
> 
Yes, and AFAIK, no RFC has ever defined an L2TPv3 sublayer using offsets.

> 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.
>
Makes sense, however L2TP_ATTR_OFFSET seems to be a noop for L2TPv2 in
the current implementation.

> 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.
> 
Agreed for Rx side. I also agree on the theory for Tx, but in the current
implementation, l2tp_build_l2tpv2_header() doesn't take the session's
"offset" field into account. So, unless I've missed something,
L2TP_ATTR_OFFSET is already a noop for L2TPv2.

Not sure if it's worth handling this feature of L2TPv2. The Linux
implementation has been there for so long, and nobody ever complained
that there was no way to define an offset on Tx.