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

[Tom Parkin <tparkin@katalix.com>]

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

On  Thu, Jan 30, 2020 at 23:34:40 +0100, Guillaume Nault wrote:
> On Thu, Jan 30, 2020 at 10:28:23AM +0000, James Chapman wrote:
> > On 29/01/2020 11:44, Guillaume Nault wrote:
> > > Since userspace is in charge of selecting the session ID, I still can't
> > > see how having the kernel accept duplicate IDs goes against the RFC.
> > > The kernel doesn't assign duplicate IDs on its own. Userspace has full
> > > control on the IDs and can implement whatever constraint when assigning
> > > session IDs (even the DOCSIS DEPI way of partioning the session ID
> > > space).
> > Perhaps another example might help.
> > 
> > Suppose there's an L2TPv3 app out there today that creates two tunnels
> > to a peer, one of which is used as a hot-standby backup in case the main
> > tunnel fails. This system uses separate network interfaces for the
> > tunnels, e.g. a router using a mobile network as a backup. If the main
> > tunnel fails, it switches traffic of sessions immediately into the
> > second tunnel. Userspace is deliberately using the same session IDs in
> > both tunnels in this case. This would work today for IP-encap, but not
> > for UDP. However, if the kernel treats session IDs as scoped by 3-tuple,
> > the application would break. The app would need to be modified to add
> > each session ID into both tunnels to work again.
> > 
> That's an interesting use case. I can imagine how this works on Rx, but
> how can packets be transmitted on the new tunnel? The session will
> still send packets through the original tunnel with the original
> 3-tuple, and there's no way to reassign a session to a new tunnel. We
> could probably rebind/reconnect the tunnel socket, but then why
> creating the second tunnel in the kernel?
> 
> > >>> 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.
> > >>
> > > Fair enough. Then we could add a L2TP_ATTR_SCOPE netlink attribute to
> > > sessions. A global scope would reject the session ID if another session
> > > already exists with this ID in the same network namespace. Sessions with
> > > global scope would be looked up solely based on their ID. A non-global
> > > scope would allow a session ID to be duplicated as long as the 3/5-tuple
> > > is different and no session uses this ID with global scope.
> > >
> > >>> 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.
> > >>
> > > I'm sorry, but I still don't understand how could that break any
> > > existing application.
> > 
> > Does my example of the hot-standby backup tunnel help?
> > 
> Yes, even though I'm not sure how it precisely translate in terms of
> userspace/kernel interraction. But anyway, with L2TP_ATTR_SCOPE, we'd
> have the possibility to keep session ID unscoped for l2tp_ip by
> default. That should be enough to keep any such scenario working
> without any modification.
> 
> > > For L2TPoUDP, session IDs are always looked up in the context of the
> > > UDP socket. So even though the kernel has stopped accepting duplicate
> > > IDs, the session IDs remain scoped in practice. And with the
> > > application being responsible for assigning IDs, I don't see how making
> > > the kernel less restrictive could break any existing implementation.
> > > Again, userspace remains in full control for session ID assignment
> > > policy.
> > >
> > > Then we have L2TPoIP, which does the opposite, always looks up sessions
> > > globally and depends on session IDs being unique in the network
> > > namespace. But Ridge's patch does not change that. Also, by adding the
> > > L2TP_ATTR_SCOPE attribute (as defined above), we could keep this
> > > behaviour (L2TPoIP session could have global scope by default).
> > 
> > I'm looking at this with an end goal of having the UDP rx path later
> > modified to work the same way as IP-encap currently does. I know Linux
> > has never worked this way in the L2TPv3 UDP path and no-one has
> > requested that it does yet, but I think it would improve the
> > implementation if UDP and IP encap behaved similarly.
> > 
> Yes, unifying UDP and IP encap would be really nice.
> 
> > L2TP_ATTR_SCOPE would be a good way for the app to select which
> > behaviour it prefers.
> > 
> Yes. But do we agree that it's also a way to keep the existing
> behaviour: unscoped for IP, scoped to the 5-tuple for UDP? That is, IP
> and UDP encap would use a different default value when user space
> doesn't request a specific behaviour.
> 
> > >> 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.
> > >>
> > > Well, I'd prefer to not introduce another subtle behaviour change. What
> > > does rejecting duplicate IDs bring us if the lookup is still done in
> > > the context of the socket? If the point is to have RFC compliance, then
> > > we'd also need to modify the lookup functions.
> > > 
> > I agree, it's not ideal. Rejecting duplicate IDs for UDP will allow the
> > UDP rx path to be modified later to work the same way as IP. So my idea
> > was to allow for that change to be made later but only for managed
> > tunnels (sockets created by userspace). My worry with the original patch
> > is that it suggests that session IDs for UDP are always scoped by the
> > tunnel so tweaking it to apply only for unmanaged tunnels was a way of
> > showing this.
> > 
> > However, you've convinced me now that scoping the session ID by
> > 3/5-tuple could work. As long as there's a mechanism that lets
> > applications choose whether the 3/5-tuple is ignored in the rx path, I'm
> > ok with it.
> > 
> Do we agree that, with L2TP_ATTR_SCOPE being a long-term solution, we
> shouldn't need to reject duplicate session IDs for UDP tunnels?
> 
> To summarise my idea:
> 
>   * Short term plan:
>     Integrate a variant of Ridge's patch, as it's simple, can easily be
>     backported to -stable and doesn't prevent the future use of global
>     session IDs (as those will be specified with L2TP_ATTR_SCOPE).
> 
>   * Long term plan:
>     Implement L2TP_ATTR_SCOPE, a session attribute defining if the
>     session ID is global or scoped to the X-tuple (3-tuple for IP,
>     5-tuple for UDP).
>     Original behaviour would be respected to avoid breaking existing
>     applications. So, by default, IP encapsulation would use global
>     scope and UDP encapsulation would use 5-tuple scope.
> 
> Does that look like a good way forward?

FWIW, this sounds reasonable to me too.

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