Message-ID: <20200125115702.GB4023@p271.fit.wifi.vutbr.cz>
Date:   Sat, 25 Jan 2020 12:57:02 +0100
From:   Guillaume Nault <gnault@...hat.com>
To:     James Chapman <jchapman@...alix.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 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.

> 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 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.

> 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?

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.

> 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.

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

Powered by blists - more mailing lists