Message-ID: <72007ca8-3ad4-62db-1b38-1ecefb82cb20@katalix.com>
Date:   Mon, 27 Jan 2020 09:25:30 +0000
From:   James Chapman <jchapman@...alix.com>
To:     Guillaume Nault <gnault@...hat.com>
Cc:     Tom Parkin <tparkin@...alix.com>,
        Ridge Kennedy <ridgek@...iedtelesis.co.nz>,
        netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net] l2tp: Allow duplicate session creation with UDP

On 25/01/2020 11:57, Guillaume Nault wrote:
> On Wed, Jan 22, 2020 at 11:55:35AM +0000, James Chapman wrote:
>> On Tue, 21 Jan 2020 at 16:35, Guillaume Nault <gnault@...hat.com> wrote:
>>> On Mon, Jan 20, 2020 at 03:09:46PM +0000, Tom Parkin wrote:
>>>> I'm struggling with it a bit though.  Wouldn't extending the hash key
>>>> like this get expensive, especially for IPv6 addresses?
>>>>
>>> From what I recall, L2TP performances are already quite low. That's
>>> certainly not a reason for making things worse, but I believe that
>>> computing a 3 or 5 tuple hash should be low overhead in comparison.
>>> But checking with real numbers would be interesting.
>>>
>> In my experience, poor L2TP data performance is usually the result of
>> MTU config issues causing IP fragmentation in the tunnel. L2TPv3
>> ethernet throughput is similar to ethernet bridge throughput in the
>> setups that I know of.
>>
> I said that because I remember I had tested L2TPv3 and VXLAN a few
> years ago and I was surprised by the performance gap. I certainly can't
> remember the details of the setup, but I'd be very surprised if I had
> misconfigured the MTU.


Fair enough. I'd be interested in your observations and ideas regarding
improving performance at some point. But I suggest keep this thread
focused on the session ID scope issue.


>> Like my colleague, Tom, I'm also struggling with this approach.
>>
> I don't pretend that implementing scoped sessions IDs is trivial. It
> just looks like the best way to solve the compatibility problem (IMHO).

>> I can't see how replacing a lookup using a 32-bit hash key with one
>> using a 260-bit or more key (128+128+4 for two IPv[46] addresses and
>> the session ID) isn't going to hurt performance, let alone the
>> per-session memory footprint. In addition, it is changing the scope of
>> the session ID away from what is defined in the RFC.
>>
> I don't see why we'd need to increase the l2tp_session's structure size.
> We can already get the 3/5-tuple from the parent's tunnel socket. And
> there are some low hanging fruits to pick if one wants to reduce L2TP's
> memory footprint.
>
> From a performance point of view, 3/5-tuple matches are quite common
> operations in the networking stack. I don't expect that to be costly
> compared to the rest of the L2TP Rx operations. And we certainly have
> room to streamline the datapath if necessary.


I was assuming the key used for the session ID lookup would be stored
with the session so that we wouldn't have to prepare it for each and
every packet receive.


>> I think Linux shouldn't diverge from the spirit of the L2TPv3 RFC
>> since the RFC is what implementors code against. Ridge's application
>> relies on duplicated L2TPv3 session IDs which are scoped by the UDP
>> 5-tuple address. But equally, there may be existing applications out
>> there which rely on Linux performing L2TPv3 session lookup by session
>> ID alone, as per the RFC. For IP-encap, Linux already does this, but
>> not for UDP. What if we get a request to do so for UDP, for
>> RFC-compliance? It would be straightforward to do as long as the
>> session ID scope isn't restricted by the proposed patch.
>>
> As long as the external behavior conforms to the RFC, I don't see any
> problem. Local applications are still responsible for selecting
> session-IDs. I don't see how they could be confused if the kernel
> accepted more IDs, especially since that was the original behaviour.

But it wouldn't conform with the RFC.

RFC3931 says:

 The Session ID alone provides the necessary context for all further
 packet processing, including the presence, size, and value of the
 Cookie, the type of L2-Specific Sublayer, and the type of payload
 being tunneled.

and also

 The data message format for tunneling data packets may be utilized
 with or without the L2TP control channel, either via manual
 configuration or via other signaling methods to pre-configure or
 distribute L2TP session information.

>> I'm not aware
>> that such an application exists, but my point is that the RFC is a key
>> document that implementors use when implementing applications so
>> diverging from it seems more likely to result in unforseen problems
>> later.
>>
> I would have to read the RFC with scoped session IDs in mind, but, as
> far as I can see, the only things that global session IDs allow which
> can't be done with scoped session IDs are:
>   * Accepting L2TPoIP sessions to receive L2TPoUDP packets and
>     vice-versa.
>   * Accepting L2TPv3 packets from peers we're not connected to.
>
> I don't find any of these to be desirable. Although Tom convinced me
> that global session IDs are in the spirit of the RFC, I still don't
> think that restricting their scope goes against it in any practical
> way. The L2TPv3 control plane requires a two way communication, which
> means that the session is bound to a given 3/5-tuple for control
> messages. Why would the data plane behave differently?


The Cable Labs / DOCSIS DEPI protocol is a good example. It is based on
L2TPv3 and uses the L2TPv3 data plane. It treats the session ID as
unscoped and not associated with a given tunnel.


> I agree that it looks saner (and simpler) for a control plane to never
> assign the same session ID to sessions running over different tunnels,
> even if they have different 3/5-tuples. But that's the user space
> control plane implementation's responsability to select unique session
> IDs in this case. The fact that the kernel uses scoped or global IDs is
> irrelevant. For unmanaged tunnels, the administrator has complete
> control over the local and remote session IDs and is free to assign
> them globally if it wants to, even if the kernel would have accepted
> reusing session IDs.


I disagree. Using scoped session IDs may break applications that assume
RFC behaviour. I mentioned one example where session IDs are used
unscoped above.


>> It's unfortunate that Linux previously unintentionally allowed L2TPv3
>> session ID clashes and an application is in the field that relies on
>> this behaviour. However, the change that fixed this (and introduced
>> the reported regression) was back in March 2017 and the problem is
>> only coming to light now. Of the options we have available, a knob to
>> re-enable the old behaviour may be the best compromise after all.
>>
>> Could we ask Ridge to submit a new version of his patch which includes
>> a knob to enable it?
>>
> But what would be the default behaviour? If it's "use global IDs", then
> we'll keep breaking applications that used to work with older kernels.
> Ridge would know how to revert to the ancient behaviour, but other
> users would probably never know about the knob. And if we set the
> default behaviour to "allow duplicate IDs for L2TPv3oUDP", then the
> knob doesn't need to be implemented as part of Ridge's fix. It can
> always be added later, if we ever decide to unify session lookups
> accross L2TPoUDP and L2TPoIP and that extending the session hash key
> proves not to be a practical solution.


The default would be the current behaviour: "global IDs". We'll be
breaking applications that assume scoped session IDs, yes. But I think
the number of these applications will be minimal given the RFC is clear
that session IDs are unscoped and the kernel has worked this way for
almost 3 years.

I think it's important that the kernel continues to treat the L2TPv3
session ID as "global".

However, there might be an alternative solution to fix this for Ridge's
use case that doesn't involve adding 3/5-tuple session ID lookups in the
receive path or adding a control knob...

My understanding is that Ridge's application uses unmanaged tunnels
(like "ip l2tp" does). These use kernel sockets. The netlink tunnel
create request does not indicate a valid tunnel socket fd. So we could
use scoped session IDs for unmanaged UDP tunnels only. If Ridge's patch
were tweaked to allow scoped IDs only for UDP unmanaged tunnels (adding
a test for tunnel->fd < 0), managed tunnels would continue to work as
they do now and any application that uses unmanaged tunnels would get
scoped session IDs. No control knob or 3/5-tuple session ID lookups
required.


> Sorry for replying late. It's been a busy week.


No problem at all.

Powered by blists - more mailing lists