Re: [PATCH net] l2tp: Allow duplicate session creation with UDP

[Tom Parkin <tparkin@katalix.com>]

[-- Attachment #1: Type: text/plain, Size: 3855 bytes --]

On  Fri, Jan 17, 2020 at 15:25:58 +0100, Guillaume Nault wrote:
> On Fri, Jan 17, 2020 at 01:18:49PM +0000, Tom Parkin wrote:
> > More generally, for v3 having the session ID be unique to the LCCE is
> > required to make IP-encap work at all.  We can't reliably obtain the
> > tunnel context from the socket because we've only got a 3-tuple
> > address to direct an incoming frame to a given socket; and the L2TPv3
> > IP-encap data packet header only contains the session ID, so that's
> > literally all there is to work with.
> > 
> I don't see how that differs from the UDP case. We should still be able
> to get the corresponding socket and lookup the session ID in that
> context. Or did I miss something? Sure, that means that the socket is
> the tunnel, but is there anything wrong with that?

It doesn't fundamentally differ from the UDP case.

The issue is that if you're stashing tunnel context with the socket
(as UDP currently does), then you're relying on the kernel's ability
to deliver packets for a given tunnel on that tunnel's socket.

In the UDP case this is normally easily done, assuming each UDP tunnel
socket has a unique 5-tuple address.  So if peers allow the use of
ports other than port 1701, it's normally not an issue.

However, if you do get a 5-tuple clash, then packets may start
arriving on the "wrong" socket.  In general this is a corner case
assuming peers allow ports other than 1701 to be used, and so we don't
see it terribly often.

Contrast this with IP-encap.  Because we don't have ports, the 5-tuple
address now becomes a 3-tuple address.  Suddenly it's quite easy to
get a clash: two IP-encap tunnels between the same two peers would do
it.

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.

> > If we relax the restriction for UDP-encap then it fixes your (Ridge's)
> > use case; but it does impose some restrictions:
> > 
> >  1. The l2tp subsystem has an existing bug for UDP encap where
> >  SO_REUSEADDR is used, as I've mentioned.  Where the 5-tuple address of
> >  two sockets clashes, frames may be directed to either socket.  So
> >  determining the tunnel context from the socket isn't valid in this
> >  situation.
> > 
> >  For L2TPv2 we could fix this by looking the tunnel context up using
> >  the tunnel ID in the header.
> > 
> >  For L2TPv3 there is no tunnel ID in the header.  If we allow
> >  duplicated session IDs for L2TPv3/UDP, there's no way to fix the
> >  problem.
> > 
> >  This sounds like a bit of a corner case, although its surprising how
> >  many implementations expect all traffic over port 1701, making
> >  5-tuple clashes more likely.
> > 
> Hum, I think I understand your scenario better. I just wonder why one
> would establish several tunnels over the same UDP or IP connection (and
> I've also been surprised by all those implementations forcing 1701 as
> source port).
>

Indeed, it's not ideal :-(

> >  2. Part of the rationale for L2TPv3's approach to IDs is that it
> >  allows the data plane to potentially be more efficient since a
> >  session can be identified by session ID alone.
> >  
> >  The kernel hasn't really exploited that fact fully (UDP encap
> >  still uses the socket to get the tunnel context), but if we make
> >  this change we'll be restricting the optimisations we might make
> >  in the future.
> > 
> > Ultimately it comes down to a judgement call.  Being unable to fix
> > the SO_REUSEADDR bug would be the biggest practical headache I
> > think.
> And it would be good to have a consistent behaviour between IP and UDP
> encapsulation. If one does a global session lookup, the other should
> too.

That would also be my preference.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]