Date:   Wed, 22 Jan 2020 11:55:35 +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 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:
> > On  Sat, Jan 18, 2020 at 20:13:36 +0100, Guillaume Nault wrote:
> > > I've never seen that as a problem in practice since establishing more
> > > than one tunnel between two LCCE or LAC/LNS doesn't bring any
> > > advantage.
> >
> > I think the practical use depends a bit on context -- it might be
> > useful to e.g. segregate sessions with different QoS or security
> > requirements into different tunnels in order to make userspace
> > configuration management easier.
> >
> That could be useful for L2TPv2. But that's not going to be more
> limitted for L2TPv3 as the tunnel ID isn't visible on the wire.
>
> > > > Since we don't want to arbitrarily limit IP-encap tunnels to on per
> > > > pair of peers, it's not practical to stash tunnel context with the
> > > > socket in the IP-encap data path.
> > > >
> > > Even though l2tp_ip doesn't lookup the session in the context of the
> > > socket, it is limitted to one tunnel for a pair of peers, because it
> > > doesn't support SO_REUSEADDR and SO_REUSEPORT.
> >
> > This isn't the case.  It is indeed possible to create multiple IP-encap
> > tunnels between the same IP addresses.
> >
> > l2tp_ip takes tunnel ID into account in struct sockaddr_l2tpip when
> > binding and connecting sockets.
> >
> Yes, sorry. I didn't give this enough thinking and mixed the UDP and IP
> transport constraints.
>
> > I think if l2tp_ip were to support SO_REUSEADDR, it would be in the
> > context of struct sockaddr_l2tpip.  In which case reusing the address
> > wouldn't really make any sense.
> >
> Yes, I think we can just forget about it.
>
> > > Thinking more about the original issue, I think we could restrict the
> > > scope of session IDs to the 3-tuple (for IP encap) or 5-tuple (for UDP
> > > encap) of its parent tunnel. We could do that by adding the IP addresses,
> > > protocol and ports to the hash key in the netns session hash-table.
> > > This way:
> > >  * Sessions would be only accessible from the peer with whom we
> > >    established the tunnel.
> > >  * We could use multiple sockets bound and connected to the same
> > >    address pair, and lookup the right session no matter on which
> > >    socket L2TP messages are received.
> > >  * We would solve Ridge's problem because we could reuse session IDs
> > >    as long as the 3 or 5-tuple of the parent tunnel is different.
> > >
> > > That would be something for net-next though. For -net, we could get
> > > something like Ridge's patch, which is simpler, since we've never
> > > supported multiple tunnels per session anyway.
> >
> > Yes, I think this would be possible.  I've been thinking of similar
> > schemes.
> >
> > 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.

Like my colleague, Tom, I'm also struggling with this approach. 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 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. 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.

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?

Powered by blists - more mailing lists