From mboxrd@z Thu Jan 1 00:00:00 1970 From: James Chapman Subject: Re: [PATCH net-next 2/2] l2tp: add peer_offset parameter Date: Tue, 2 Jan 2018 20:08:07 +0000 Message-ID: References: <20171228145340.GA1292@alphalink.fr> <20171228182347.GA8732@localhost.localdomain> <20171228194556.GA1256@alphalink.fr> <27e0a7c8-b95b-04eb-d808-bcae67e417b1@katalix.com> <20180102175048.GA1402@alphalink.fr> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: Lorenzo Bianconi , davem@davemloft.net, netdev@vger.kernel.org, liuhangbin@gmail.com To: Guillaume Nault Return-path: Received: from mail.katalix.com ([82.103.140.233]:34049 "EHLO mail.katalix.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750726AbeABUUA (ORCPT ); Tue, 2 Jan 2018 15:20:00 -0500 In-Reply-To: <20180102175048.GA1402@alphalink.fr> Content-Language: en-GB Sender: netdev-owner@vger.kernel.org List-ID: On 02/01/18 17:50, Guillaume Nault wrote: > 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. You're right. My bad. > 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. I agree.