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

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

[Ridge Kennedy]

In the past it was possible to create multiple L2TPv3 sessions with the
same session id as long as the sessions belonged to different tunnels.
The resulting sessions had issues when used with IP encapsulated tunnels,
but worked fine with UDP encapsulated ones. Some applications began to
rely on this behaviour to avoid having to negotiate unique session ids.

Some time ago a change was made to require session ids to be unique across
all tunnels, breaking the applications making use of this "feature".

This change relaxes the duplicate session id check to allow duplicates
if both of the colliding sessions belong to UDP encapsulated tunnels.

Fixes: dbdbc73b4478 ("l2tp: fix duplicate session creation")
Signed-off-by: Ridge Kennedy <ridge.kennedy@alliedtelesis.co.nz>
---
 net/l2tp/l2tp_core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index f82ea12bac37..0cc86227c618 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -323,7 +323,9 @@ int l2tp_session_register(struct l2tp_session *session,
 		spin_lock_bh(&pn->l2tp_session_hlist_lock);
 
 		hlist_for_each_entry(session_walk, g_head, global_hlist)
-			if (session_walk->session_id == session->session_id) {
+			if (session_walk->session_id == session->session_id &&
+			    (session_walk->tunnel->encap == L2TP_ENCAPTYPE_IP ||
+			     tunnel->encap == L2TP_ENCAPTYPE_IP)) {
 				err = -EEXIST;
 				goto err_tlock_pnlock;
 			}
-- 
2.24.0

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

[Tom Parkin]

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

On  Thu, Jan 16, 2020 at 11:34:47 +1300, Ridge Kennedy wrote:
> In the past it was possible to create multiple L2TPv3 sessions with the
> same session id as long as the sessions belonged to different tunnels.
> The resulting sessions had issues when used with IP encapsulated tunnels,
> but worked fine with UDP encapsulated ones. Some applications began to
> rely on this behaviour to avoid having to negotiate unique session ids.
> 
> Some time ago a change was made to require session ids to be unique across
> all tunnels, breaking the applications making use of this "feature".
> 
> This change relaxes the duplicate session id check to allow duplicates
> if both of the colliding sessions belong to UDP encapsulated tunnels.

I appreciate what you're saying with respect to buggy applications,
however I think the existing kernel code is consistent with RFC 3931,
which makes session IDs unique for a given LCCE.

Given how the L2TP data packet headers work for L2TPv3, I'm assuming
that sessions in UDP-encapsulated tunnels work even if their session
IDs clash because the tunnel sockets are using distinct UDP ports
which will effectively separate the data traffic into the "correct"
tunnel.  Obviously the same thing doesn't apply for IP-encap.

However, there's nothing to prevent user space from using the same UDP
port for multiple tunnels, at which point this relaxation of the RFC
rules would break down again.

Since UDP-encap can also be broken in the face of duplicated L2TPv3
session IDs, I think its better that the kernel continue to enforce
the RFC.

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

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

[Guillaume Nault]

On Thu, Jan 16, 2020 at 12:31:43PM +0000, Tom Parkin wrote:
> On  Thu, Jan 16, 2020 at 11:34:47 +1300, Ridge Kennedy wrote:
> > In the past it was possible to create multiple L2TPv3 sessions with the
> > same session id as long as the sessions belonged to different tunnels.
> > The resulting sessions had issues when used with IP encapsulated tunnels,
> > but worked fine with UDP encapsulated ones. Some applications began to
> > rely on this behaviour to avoid having to negotiate unique session ids.
> > 
> > Some time ago a change was made to require session ids to be unique across
> > all tunnels, breaking the applications making use of this "feature".
> > 
> > This change relaxes the duplicate session id check to allow duplicates
> > if both of the colliding sessions belong to UDP encapsulated tunnels.
> 
> I appreciate what you're saying with respect to buggy applications,
> however I think the existing kernel code is consistent with RFC 3931,
> which makes session IDs unique for a given LCCE.
> 
> Given how the L2TP data packet headers work for L2TPv3, I'm assuming
> that sessions in UDP-encapsulated tunnels work even if their session
> IDs clash because the tunnel sockets are using distinct UDP ports
> which will effectively separate the data traffic into the "correct"
> tunnel.  Obviously the same thing doesn't apply for IP-encap.
> 
> However, there's nothing to prevent user space from using the same UDP
> port for multiple tunnels, at which point this relaxation of the RFC
> rules would break down again.
> 
Multiplexing L2TP tunnels on top of non-connected UDP sockets might be
a nice optimisation for someone using many tunnels (like hundred of
thouthands), but I'm afraid the rest of the L2TP code is not ready to
handle such load anyway. And the current implementation only allows
one tunnel for each UDP socket anyway.

> Since UDP-encap can also be broken in the face of duplicated L2TPv3
> session IDs, I think its better that the kernel continue to enforce
> the RFC.
How is UDP-encap broken with duplicate session IDs (as long as a UDP
socket can only one have one tunnel associated with it and that no
duplicate session IDs are allowed inside the same tunnel)?

It all boils down to what's the scope of a session ID. For me it has
always been the parent tunnel. But if that's in contradiction with
RFC 3931, I'd be happy to know.

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

[Tom Parkin]

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

On  Thu, Jan 16, 2020 at 20:28:27 +0100, Guillaume Nault wrote:
> On Thu, Jan 16, 2020 at 12:31:43PM +0000, Tom Parkin wrote:
> > However, there's nothing to prevent user space from using the same UDP
> > port for multiple tunnels, at which point this relaxation of the RFC
> > rules would break down again.
> > 
> Multiplexing L2TP tunnels on top of non-connected UDP sockets might be
> a nice optimisation for someone using many tunnels (like hundred of
> thouthands), but I'm afraid the rest of the L2TP code is not ready to
> handle such load anyway. And the current implementation only allows
> one tunnel for each UDP socket anyway.

TBH I was thinking more of the case where multiple sockets are bound and
connected to the same address/port (SO_REUSEADDR).  There's still a
1:1 mapping of tunnel:socket, but it's possible to have packets for tunnel
A arrive on tunnel B's socket and vice versa.

It's a bit of a corner case, I grant you.

> > Since UDP-encap can also be broken in the face of duplicated L2TPv3
> > session IDs, I think its better that the kernel continue to enforce
> > the RFC.
> How is UDP-encap broken with duplicate session IDs (as long as a UDP
> socket can only one have one tunnel associated with it and that no
> duplicate session IDs are allowed inside the same tunnel)?
> 
> It all boils down to what's the scope of a session ID. For me it has
> always been the parent tunnel. But if that's in contradiction with
> RFC 3931, I'd be happy to know.

For RFC 2661 the session ID is scoped to the tunnel.  Section 3.1
says:

  "Session ID indicates the identifier for a session within a tunnel."

Control and data packets share the same header which includes both the
tunnel and session ID with 16 bits allocated to each.  So it's always
possible to tell from the data packet header which tunnel the session is
associated with.

RFC 3931 changed the scheme.  Control packets now include a 32-bit
"Control Connection ID" (analogous to the Tunnel ID).  Data packets
have a session header specific to the packet-switching network in use:
the RFC describes schemes for both IP and UDP, both of which employ a
32-bit session ID.  Section 4.1 says:

  "The Session ID alone provides the necessary context for all further
  packet processing"

Since neither UDP nor IP encapsulated data packets include the control
connection ID, the session ID must be unique to the LCCE to allow
identification of the session.

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

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

[Guillaume Nault]

On Thu, Jan 16, 2020 at 09:05:01PM +0000, Tom Parkin wrote:
> On  Thu, Jan 16, 2020 at 20:28:27 +0100, Guillaume Nault wrote:
> > On Thu, Jan 16, 2020 at 12:31:43PM +0000, Tom Parkin wrote:
> > > However, there's nothing to prevent user space from using the same UDP
> > > port for multiple tunnels, at which point this relaxation of the RFC
> > > rules would break down again.
> > > 
> > Multiplexing L2TP tunnels on top of non-connected UDP sockets might be
> > a nice optimisation for someone using many tunnels (like hundred of
> > thouthands), but I'm afraid the rest of the L2TP code is not ready to
> > handle such load anyway. And the current implementation only allows
> > one tunnel for each UDP socket anyway.
> 
> TBH I was thinking more of the case where multiple sockets are bound and
> connected to the same address/port (SO_REUSEADDR).  There's still a
> 1:1 mapping of tunnel:socket, but it's possible to have packets for tunnel
> A arrive on tunnel B's socket and vice versa.
> 
> It's a bit of a corner case, I grant you.
> 
Creating several sockets to handle the same tunnel (as identified by
its 5-tuple) may be doable with SO_REUSEPORT, with an ebpf program to
direct the packet to the right socket depending on the session ID. The
session ID would be local to the so_reuseport group. So I guess that
even this kind of setup can be achieved with non-global session IDs (I
haven't tried though, so I might have missed something).

> > > Since UDP-encap can also be broken in the face of duplicated L2TPv3
> > > session IDs, I think its better that the kernel continue to enforce
> > > the RFC.
> > How is UDP-encap broken with duplicate session IDs (as long as a UDP
> > socket can only one have one tunnel associated with it and that no
> > duplicate session IDs are allowed inside the same tunnel)?
> > 
> > It all boils down to what's the scope of a session ID. For me it has
> > always been the parent tunnel. But if that's in contradiction with
> > RFC 3931, I'd be happy to know.
> 
> For RFC 2661 the session ID is scoped to the tunnel.  Section 3.1
> says:
> 
>   "Session ID indicates the identifier for a session within a tunnel."
> 
> Control and data packets share the same header which includes both the
> tunnel and session ID with 16 bits allocated to each.  So it's always
> possible to tell from the data packet header which tunnel the session is
> associated with.
> 
> RFC 3931 changed the scheme.  Control packets now include a 32-bit
> "Control Connection ID" (analogous to the Tunnel ID).  Data packets
> have a session header specific to the packet-switching network in use:
> the RFC describes schemes for both IP and UDP, both of which employ a
> 32-bit session ID.  Section 4.1 says:
> 
>   "The Session ID alone provides the necessary context for all further
>   packet processing"
> 
> Since neither UDP nor IP encapsulated data packets include the control
> connection ID, the session ID must be unique to the LCCE to allow
> identification of the session.

Well my understanding was that the tunnel was implicitely given by the
UDP and IP headers. I don't think that multiplexing tunnels over the
same UDP connection made any sense with L2TPv2, and the kernel never
supported it natively (it might be possible with SO_REUSEPORT). Given
that the tunnel ID field was redundant with the lower headers, it made
sense to me that L2TPv3 dropped it (note that the kernel ignores the
L2TPv2 tunnel ID field on Rx). At least that was my understanding.

But as your quote says, the session ID _alone_ should provide all the
L2TP context. So I guess the spirit of the RFC is that there's a single
global namespace for session IDs. Now, practically speaking, I don't
see how scoped session IDs makes us incompatible, unless we consider
that a given session can be shared between several remote hosts (the
cross-talk case in my other email). Also, sharing a session over
several hosts would mean that L2TPv3 sessions aren't point-to-point,
which the control plane doesn't seem to take into account.

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

[Tom Parkin]

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

On  Fri, Jan 17, 2020 at 14:43:27 +0100, Guillaume Nault wrote:
> On Thu, Jan 16, 2020 at 09:05:01PM +0000, Tom Parkin wrote:
> > On  Thu, Jan 16, 2020 at 20:28:27 +0100, Guillaume Nault wrote:
> > > How is UDP-encap broken with duplicate session IDs (as long as a UDP
> > > socket can only one have one tunnel associated with it and that no
> > > duplicate session IDs are allowed inside the same tunnel)?
> > > 
> > > It all boils down to what's the scope of a session ID. For me it has
> > > always been the parent tunnel. But if that's in contradiction with
> > > RFC 3931, I'd be happy to know.
> > 
> > For RFC 2661 the session ID is scoped to the tunnel.  Section 3.1
> > says:
> > 
> >   "Session ID indicates the identifier for a session within a tunnel."
> > 
> > Control and data packets share the same header which includes both the
> > tunnel and session ID with 16 bits allocated to each.  So it's always
> > possible to tell from the data packet header which tunnel the session is
> > associated with.
> > 
> > RFC 3931 changed the scheme.  Control packets now include a 32-bit
> > "Control Connection ID" (analogous to the Tunnel ID).  Data packets
> > have a session header specific to the packet-switching network in use:
> > the RFC describes schemes for both IP and UDP, both of which employ a
> > 32-bit session ID.  Section 4.1 says:
> > 
> >   "The Session ID alone provides the necessary context for all further
> >   packet processing"
> > 
> > Since neither UDP nor IP encapsulated data packets include the control
> > connection ID, the session ID must be unique to the LCCE to allow
> > identification of the session.
> 
> Well my understanding was that the tunnel was implicitely given by the
> UDP and IP headers. I don't think that multiplexing tunnels over the
> same UDP connection made any sense with L2TPv2, and the kernel never
> supported it natively (it might be possible with SO_REUSEPORT). Given
> that the tunnel ID field was redundant with the lower headers, it made
> sense to me that L2TPv3 dropped it (note that the kernel ignores the
> L2TPv2 tunnel ID field on Rx). At least that was my understanding.
> 
> But as your quote says, the session ID _alone_ should provide all the
> L2TP context. So I guess the spirit of the RFC is that there's a single
> global namespace for session IDs. Now, practically speaking, I don't
> see how scoped session IDs makes us incompatible, unless we consider
> that a given session can be shared between several remote hosts (the
> cross-talk case in my other email). Also, sharing a session over
> several hosts would mean that L2TPv3 sessions aren't point-to-point,
> which the control plane doesn't seem to take into account.

I think from your other emails in this thread that we're maybe in
agreement already.

But just in case, I wanted to clarify that the session ID namespace
is for a given LCCE (LAC or LNS in L2TPv2 parlance) per RFC 3931
section 4.1 -- it's not truly "global".

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

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

[Guillaume Nault]

On Fri, Jan 17, 2020 at 06:59:31PM +0000, Tom Parkin wrote:
> On  Fri, Jan 17, 2020 at 14:43:27 +0100, Guillaume Nault wrote:
> > On Thu, Jan 16, 2020 at 09:05:01PM +0000, Tom Parkin wrote:
> > > On  Thu, Jan 16, 2020 at 20:28:27 +0100, Guillaume Nault wrote:
> > > > How is UDP-encap broken with duplicate session IDs (as long as a UDP
> > > > socket can only one have one tunnel associated with it and that no
> > > > duplicate session IDs are allowed inside the same tunnel)?
> > > > 
> > > > It all boils down to what's the scope of a session ID. For me it has
> > > > always been the parent tunnel. But if that's in contradiction with
> > > > RFC 3931, I'd be happy to know.
> > > 
> > > For RFC 2661 the session ID is scoped to the tunnel.  Section 3.1
> > > says:
> > > 
> > >   "Session ID indicates the identifier for a session within a tunnel."
> > > 
> > > Control and data packets share the same header which includes both the
> > > tunnel and session ID with 16 bits allocated to each.  So it's always
> > > possible to tell from the data packet header which tunnel the session is
> > > associated with.
> > > 
> > > RFC 3931 changed the scheme.  Control packets now include a 32-bit
> > > "Control Connection ID" (analogous to the Tunnel ID).  Data packets
> > > have a session header specific to the packet-switching network in use:
> > > the RFC describes schemes for both IP and UDP, both of which employ a
> > > 32-bit session ID.  Section 4.1 says:
> > > 
> > >   "The Session ID alone provides the necessary context for all further
> > >   packet processing"
> > > 
> > > Since neither UDP nor IP encapsulated data packets include the control
> > > connection ID, the session ID must be unique to the LCCE to allow
> > > identification of the session.
> > 
> > Well my understanding was that the tunnel was implicitely given by the
> > UDP and IP headers. I don't think that multiplexing tunnels over the
> > same UDP connection made any sense with L2TPv2, and the kernel never
> > supported it natively (it might be possible with SO_REUSEPORT). Given
> > that the tunnel ID field was redundant with the lower headers, it made
> > sense to me that L2TPv3 dropped it (note that the kernel ignores the
> > L2TPv2 tunnel ID field on Rx). At least that was my understanding.
> > 
> > But as your quote says, the session ID _alone_ should provide all the
> > L2TP context. So I guess the spirit of the RFC is that there's a single
> > global namespace for session IDs. Now, practically speaking, I don't
> > see how scoped session IDs makes us incompatible, unless we consider
> > that a given session can be shared between several remote hosts (the
> > cross-talk case in my other email). Also, sharing a session over
> > several hosts would mean that L2TPv3 sessions aren't point-to-point,
> > which the control plane doesn't seem to take into account.
> 
> I think from your other emails in this thread that we're maybe in
> agreement already.
> 
> But just in case, I wanted to clarify that the session ID namespace
> is for a given LCCE (LAC or LNS in L2TPv2 parlance) per RFC 3931
> section 4.1 -- it's not truly "global".
> 
I meant global to a given host (LCCE or LAC/LNS), which for Linux
actually means global to a network namespace. I probably should have
been more precise in my previous emails, but everytime I talked about
"global" session IDs, I meant "global to the network namespace", and
when I talked about "scoped" session IDs, I meant that the ID was only
valid in the context of the UDP or L2TP_IP socket.

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

[Guillaume Nault]

On Thu, Jan 16, 2020 at 11:34:47AM +1300, Ridge Kennedy wrote:
> In the past it was possible to create multiple L2TPv3 sessions with the
> same session id as long as the sessions belonged to different tunnels.
> The resulting sessions had issues when used with IP encapsulated tunnels,
> but worked fine with UDP encapsulated ones. Some applications began to
> rely on this behaviour to avoid having to negotiate unique session ids.
> 
> Some time ago a change was made to require session ids to be unique across
> all tunnels, breaking the applications making use of this "feature".
> 
> This change relaxes the duplicate session id check to allow duplicates
> if both of the colliding sessions belong to UDP encapsulated tunnels.
> 
> Fixes: dbdbc73b4478 ("l2tp: fix duplicate session creation")
> Signed-off-by: Ridge Kennedy <ridge.kennedy@alliedtelesis.co.nz>
> ---
>  net/l2tp/l2tp_core.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> index f82ea12bac37..0cc86227c618 100644
> --- a/net/l2tp/l2tp_core.c
> +++ b/net/l2tp/l2tp_core.c
> @@ -323,7 +323,9 @@ int l2tp_session_register(struct l2tp_session *session,
>  		spin_lock_bh(&pn->l2tp_session_hlist_lock);
>  
>  		hlist_for_each_entry(session_walk, g_head, global_hlist)
> -			if (session_walk->session_id == session->session_id) {
> +			if (session_walk->session_id == session->session_id &&
> +			    (session_walk->tunnel->encap == L2TP_ENCAPTYPE_IP ||
> +			     tunnel->encap == L2TP_ENCAPTYPE_IP)) {
>  				err = -EEXIST;
>  				goto err_tlock_pnlock;
>  			}
Well, I think we have a more fundamental problem here. By adding
L2TPoUDP sessions to the global list, we allow cross-talks with L2TPoIP
sessions. That is, if we have an L2TPv3 session X running over UDP and
we receive an L2TP_IP packet targetted at session ID X, then
l2tp_session_get() will return the L2TP_UDP session to l2tp_ip_recv().

I guess l2tp_session_get() should be dropped and l2tp_ip_recv() should
look up the session in the context of its socket, like in the UDP case.

But for the moment, what about just not adding L2TP_UDP sessions to the
global list? That should fix both your problem and the L2TP_UDP/L2TP_IP
cross-talks.

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index f82ea12bac37..f70fea8d093d 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -316,7 +316,7 @@ int l2tp_session_register(struct l2tp_session *session,
 			goto err_tlock;
 		}
 
-	if (tunnel->version == L2TP_HDR_VER_3) {
+	if (tunnel->encap == L2TP_ENCAPTYPE_IP) {
 		pn = l2tp_pernet(tunnel->l2tp_net);
 		g_head = l2tp_session_id_hash_2(pn, session->session_id);
 
@@ -1587,8 +1587,8 @@ void __l2tp_session_unhash(struct l2tp_session *session)
 		hlist_del_init(&session->hlist);
 		write_unlock_bh(&tunnel->hlist_lock);
 
-		/* For L2TPv3 we have a per-net hash: remove from there, too */
-		if (tunnel->version != L2TP_HDR_VER_2) {
+		/* For IP encap we have a per-net hash: remove from there, too */
+		if (tunnel->encap == L2TP_ENCAPTYPE_IP) {
 			struct l2tp_net *pn = l2tp_pernet(tunnel->l2tp_net);
 			spin_lock_bh(&pn->l2tp_session_hlist_lock);
 			hlist_del_init_rcu(&session->global_hlist);

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

[Tom Parkin]

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

On  Thu, Jan 16, 2020 at 13:38:54 +0100, Guillaume Nault wrote:
> Well, I think we have a more fundamental problem here. By adding
> L2TPoUDP sessions to the global list, we allow cross-talks with L2TPoIP
> sessions. That is, if we have an L2TPv3 session X running over UDP and
> we receive an L2TP_IP packet targetted at session ID X, then
> l2tp_session_get() will return the L2TP_UDP session to l2tp_ip_recv().
> 
> I guess l2tp_session_get() should be dropped and l2tp_ip_recv() should
> look up the session in the context of its socket, like in the UDP case.
> 
> But for the moment, what about just not adding L2TP_UDP sessions to the
> global list? That should fix both your problem and the L2TP_UDP/L2TP_IP
> cross-talks.
> 
> diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> index f82ea12bac37..f70fea8d093d 100644
> --- a/net/l2tp/l2tp_core.c
> +++ b/net/l2tp/l2tp_core.c
> @@ -316,7 +316,7 @@ int l2tp_session_register(struct l2tp_session *session,
>  			goto err_tlock;
>  		}
>  
> -	if (tunnel->version == L2TP_HDR_VER_3) {
> +	if (tunnel->encap == L2TP_ENCAPTYPE_IP) {
>  		pn = l2tp_pernet(tunnel->l2tp_net);
>  		g_head = l2tp_session_id_hash_2(pn, session->session_id);
>  
> @@ -1587,8 +1587,8 @@ void __l2tp_session_unhash(struct l2tp_session *session)
>  		hlist_del_init(&session->hlist);
>  		write_unlock_bh(&tunnel->hlist_lock);
>  
> -		/* For L2TPv3 we have a per-net hash: remove from there, too */
> -		if (tunnel->version != L2TP_HDR_VER_2) {
> +		/* For IP encap we have a per-net hash: remove from there, too */
> +		if (tunnel->encap == L2TP_ENCAPTYPE_IP) {
>  			struct l2tp_net *pn = l2tp_pernet(tunnel->l2tp_net);
>  			spin_lock_bh(&pn->l2tp_session_hlist_lock);
>  			hlist_del_init_rcu(&session->global_hlist);
>

I agree with you about the possibility for cross-talk, and I would
welcome l2tp_ip/ip6 doing more validation.  But I don't think we should
ditch the global list.

As per the RFC, for L2TPv3 the session ID should be a unique
identifier for the LCCE.  So it's reasonable that the kernel should
enforce that when registering sessions.

When you're referring to cross-talk, I wonder whether you have in mind
normal operation or malicious intent?  I suppose it would be possible
for someone to craft session data packets in order to disrupt a
session.

For normal operation, you just need to get the wrong packet on the
wrong socket to run into trouble of course.  In such a situation
having a unique session ID for v3 helps you to determine that
something has gone wrong, which is what the UDP encap recv path does
if the session data packet's session ID isn't found in the context of
the socket that receives it.

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

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

[Guillaume Nault]

On Thu, Jan 16, 2020 at 01:12:24PM +0000, Tom Parkin wrote:
> On  Thu, Jan 16, 2020 at 13:38:54 +0100, Guillaume Nault wrote:
> > Well, I think we have a more fundamental problem here. By adding
> > L2TPoUDP sessions to the global list, we allow cross-talks with L2TPoIP
> > sessions. That is, if we have an L2TPv3 session X running over UDP and
> > we receive an L2TP_IP packet targetted at session ID X, then
> > l2tp_session_get() will return the L2TP_UDP session to l2tp_ip_recv().
> > 
> > I guess l2tp_session_get() should be dropped and l2tp_ip_recv() should
> > look up the session in the context of its socket, like in the UDP case.
> > 
> > But for the moment, what about just not adding L2TP_UDP sessions to the
> > global list? That should fix both your problem and the L2TP_UDP/L2TP_IP
> > cross-talks.
> > 
> > diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> > index f82ea12bac37..f70fea8d093d 100644
> > --- a/net/l2tp/l2tp_core.c
> > +++ b/net/l2tp/l2tp_core.c
> > @@ -316,7 +316,7 @@ int l2tp_session_register(struct l2tp_session *session,
> >  			goto err_tlock;
> >  		}
> >  
> > -	if (tunnel->version == L2TP_HDR_VER_3) {
> > +	if (tunnel->encap == L2TP_ENCAPTYPE_IP) {
> >  		pn = l2tp_pernet(tunnel->l2tp_net);
> >  		g_head = l2tp_session_id_hash_2(pn, session->session_id);
> >  
> > @@ -1587,8 +1587,8 @@ void __l2tp_session_unhash(struct l2tp_session *session)
> >  		hlist_del_init(&session->hlist);
> >  		write_unlock_bh(&tunnel->hlist_lock);
> >  
> > -		/* For L2TPv3 we have a per-net hash: remove from there, too */
> > -		if (tunnel->version != L2TP_HDR_VER_2) {
> > +		/* For IP encap we have a per-net hash: remove from there, too */
> > +		if (tunnel->encap == L2TP_ENCAPTYPE_IP) {
> >  			struct l2tp_net *pn = l2tp_pernet(tunnel->l2tp_net);
> >  			spin_lock_bh(&pn->l2tp_session_hlist_lock);
> >  			hlist_del_init_rcu(&session->global_hlist);
> >
> 
> I agree with you about the possibility for cross-talk, and I would
> welcome l2tp_ip/ip6 doing more validation.  But I don't think we should
> ditch the global list.
> 
> As per the RFC, for L2TPv3 the session ID should be a unique
> identifier for the LCCE.  So it's reasonable that the kernel should
> enforce that when registering sessions.
> 
I had never thought that the session ID could have global significance
in L2TPv3, but maybe that's because my experience is mostly about
L2TPv2. I haven't read RFC 3931 in detail, but I can't see how
restricting the scope of sessions to their parent tunnel would conflict
with the RFC.

> When you're referring to cross-talk, I wonder whether you have in mind
> normal operation or malicious intent?  I suppose it would be possible
> for someone to craft session data packets in order to disrupt a
> session.
> 
What makes me uneasy is that, as soon as the l2tp_ip or l2tp_ip6 module
is loaded, a peer can reach whatever L2TPv3 session exists on the host
just by sending an L2TP_IP packet to it.
I don't know how practical it is to exploit this fact, but it looks
like it's asking for trouble.

> For normal operation, you just need to get the wrong packet on the
> wrong socket to run into trouble of course.  In such a situation
> having a unique session ID for v3 helps you to determine that
> something has gone wrong, which is what the UDP encap recv path does
> if the session data packet's session ID isn't found in the context of
> the socket that receives it.
Unique global session IDs might help troubleshooting, but they also
break some use cases, as reported by Ridge. At some point, we'll have
to make a choice, or even add a knob if necessary.

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

[Tom Parkin]

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

On  Thu, Jan 16, 2020 at 20:05:56 +0100, Guillaume Nault wrote:
> On Thu, Jan 16, 2020 at 01:12:24PM +0000, Tom Parkin wrote:
> > I agree with you about the possibility for cross-talk, and I would
> > welcome l2tp_ip/ip6 doing more validation.  But I don't think we should
> > ditch the global list.
> > 
> > As per the RFC, for L2TPv3 the session ID should be a unique
> > identifier for the LCCE.  So it's reasonable that the kernel should
> > enforce that when registering sessions.
> > 
> I had never thought that the session ID could have global significance
> in L2TPv3, but maybe that's because my experience is mostly about
> L2TPv2. I haven't read RFC 3931 in detail, but I can't see how
> restricting the scope of sessions to their parent tunnel would conflict
> with the RFC.

Sorry Guillaume, I responded to your other mail without reading this
one.

I gave more detail in my other response: it boils down to how RFC 3931
changes the use of IDs in the L2TP header.  Data packets for IP or UDP
only contain the 32-bit session ID, and hence this must be unique to
the LCCE to allow the destination session to be successfully
identified.

> > When you're referring to cross-talk, I wonder whether you have in mind
> > normal operation or malicious intent?  I suppose it would be possible
> > for someone to craft session data packets in order to disrupt a
> > session.
> > 
> What makes me uneasy is that, as soon as the l2tp_ip or l2tp_ip6 module
> is loaded, a peer can reach whatever L2TPv3 session exists on the host
> just by sending an L2TP_IP packet to it.
> I don't know how practical it is to exploit this fact, but it looks
> like it's asking for trouble.

Yes, I agree, although practically it's only a slightly easier
"exploit" than L2TP/UDP offers.

The UDP case requires a rogue packet to be delivered to the correct
socket AND have a session ID matching that of one in the associated
tunnel.

It's a slightly more arduous scenario to engineer than the existing
L2TPv3/IP case, but only a little.

I also don't know how practical this would be to leverage to cause
problems.

> > For normal operation, you just need to get the wrong packet on the
> > wrong socket to run into trouble of course.  In such a situation
> > having a unique session ID for v3 helps you to determine that
> > something has gone wrong, which is what the UDP encap recv path does
> > if the session data packet's session ID isn't found in the context of
> > the socket that receives it.
> Unique global session IDs might help troubleshooting, but they also
> break some use cases, as reported by Ridge. At some point, we'll have
> to make a choice, or even add a knob if necessary.

I suspect we need to reach agreement on what RFC 3931 implies before
making headway on what the kernel should ideally do.

There is perhaps room for pragmatism given that the kernel
used to be more permissive prior to dbdbc73b4478, and we weren't
inundated with reports of session ID clashes.

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

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

[Ridge Kennedy]

On Thu, 16 Jan 2020, Tom Parkin wrote:

> On  Thu, Jan 16, 2020 at 20:05:56 +0100, Guillaume Nault wrote:
>> On Thu, Jan 16, 2020 at 01:12:24PM +0000, Tom Parkin wrote:
>>> I agree with you about the possibility for cross-talk, and I would
>>> welcome l2tp_ip/ip6 doing more validation.  But I don't think we should
>>> ditch the global list.
>>>
>>> As per the RFC, for L2TPv3 the session ID should be a unique
>>> identifier for the LCCE.  So it's reasonable that the kernel should
>>> enforce that when registering sessions.
>>>
>> I had never thought that the session ID could have global significance
>> in L2TPv3, but maybe that's because my experience is mostly about
>> L2TPv2. I haven't read RFC 3931 in detail, but I can't see how
>> restricting the scope of sessions to their parent tunnel would conflict
>> with the RFC.
>
> Sorry Guillaume, I responded to your other mail without reading this
> one.
>
> I gave more detail in my other response: it boils down to how RFC 3931
> changes the use of IDs in the L2TP header.  Data packets for IP or UDP
> only contain the 32-bit session ID, and hence this must be unique to
> the LCCE to allow the destination session to be successfully
> identified.
>
>>> When you're referring to cross-talk, I wonder whether you have in mind
>>> normal operation or malicious intent?  I suppose it would be possible
>>> for someone to craft session data packets in order to disrupt a
>>> session.
>>>
>> What makes me uneasy is that, as soon as the l2tp_ip or l2tp_ip6 module
>> is loaded, a peer can reach whatever L2TPv3 session exists on the host
>> just by sending an L2TP_IP packet to it.
>> I don't know how practical it is to exploit this fact, but it looks
>> like it's asking for trouble.
>
> Yes, I agree, although practically it's only a slightly easier
> "exploit" than L2TP/UDP offers.
>
> The UDP case requires a rogue packet to be delivered to the correct
> socket AND have a session ID matching that of one in the associated
> tunnel.
>
> It's a slightly more arduous scenario to engineer than the existing
> L2TPv3/IP case, but only a little.
>
> I also don't know how practical this would be to leverage to cause
> problems.
>
>>> For normal operation, you just need to get the wrong packet on the
>>> wrong socket to run into trouble of course.  In such a situation
>>> having a unique session ID for v3 helps you to determine that
>>> something has gone wrong, which is what the UDP encap recv path does
>>> if the session data packet's session ID isn't found in the context of
>>> the socket that receives it.
>> Unique global session IDs might help troubleshooting, but they also
>> break some use cases, as reported by Ridge. At some point, we'll have
>> to make a choice, or even add a knob if necessary.
>
> I suspect we need to reach agreement on what RFC 3931 implies before
> making headway on what the kernel should ideally do.
>
> There is perhaps room for pragmatism given that the kernel
> used to be more permissive prior to dbdbc73b4478, and we weren't
> inundated with reports of session ID clashes.
>

A knob (module_param?) to enable the permissive behaviour would certainly
work for me.

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

[Tom Parkin]

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

On  Fri, Jan 17, 2020 at 10:50:55 +1300, Ridge Kennedy wrote:
> On Thu, 16 Jan 2020, Tom Parkin wrote:
> 
> > On  Thu, Jan 16, 2020 at 20:05:56 +0100, Guillaume Nault wrote:
> > > On Thu, Jan 16, 2020 at 01:12:24PM +0000, Tom Parkin wrote:
> > > > I agree with you about the possibility for cross-talk, and I would
> > > > welcome l2tp_ip/ip6 doing more validation.  But I don't think we should
> > > > ditch the global list.
> > > > 
> > > > As per the RFC, for L2TPv3 the session ID should be a unique
> > > > identifier for the LCCE.  So it's reasonable that the kernel should
> > > > enforce that when registering sessions.
> > > > 
> > > I had never thought that the session ID could have global significance
> > > in L2TPv3, but maybe that's because my experience is mostly about
> > > L2TPv2. I haven't read RFC 3931 in detail, but I can't see how
> > > restricting the scope of sessions to their parent tunnel would conflict
> > > with the RFC.
> > 
> > Sorry Guillaume, I responded to your other mail without reading this
> > one.
> > 
> > I gave more detail in my other response: it boils down to how RFC 3931
> > changes the use of IDs in the L2TP header.  Data packets for IP or UDP
> > only contain the 32-bit session ID, and hence this must be unique to
> > the LCCE to allow the destination session to be successfully
> > identified.
> > 
> > > > When you're referring to cross-talk, I wonder whether you have in mind
> > > > normal operation or malicious intent?  I suppose it would be possible
> > > > for someone to craft session data packets in order to disrupt a
> > > > session.
> > > > 
> > > What makes me uneasy is that, as soon as the l2tp_ip or l2tp_ip6 module
> > > is loaded, a peer can reach whatever L2TPv3 session exists on the host
> > > just by sending an L2TP_IP packet to it.
> > > I don't know how practical it is to exploit this fact, but it looks
> > > like it's asking for trouble.
> > 
> > Yes, I agree, although practically it's only a slightly easier
> > "exploit" than L2TP/UDP offers.
> > 
> > The UDP case requires a rogue packet to be delivered to the correct
> > socket AND have a session ID matching that of one in the associated
> > tunnel.
> > 
> > It's a slightly more arduous scenario to engineer than the existing
> > L2TPv3/IP case, but only a little.
> > 
> > I also don't know how practical this would be to leverage to cause
> > problems.
> > 
> > > > For normal operation, you just need to get the wrong packet on the
> > > > wrong socket to run into trouble of course.  In such a situation
> > > > having a unique session ID for v3 helps you to determine that
> > > > something has gone wrong, which is what the UDP encap recv path does
> > > > if the session data packet's session ID isn't found in the context of
> > > > the socket that receives it.
> > > Unique global session IDs might help troubleshooting, but they also
> > > break some use cases, as reported by Ridge. At some point, we'll have
> > > to make a choice, or even add a knob if necessary.
> > 
> > I suspect we need to reach agreement on what RFC 3931 implies before
> > making headway on what the kernel should ideally do.
> > 
> > There is perhaps room for pragmatism given that the kernel
> > used to be more permissive prior to dbdbc73b4478, and we weren't
> > inundated with reports of session ID clashes.
> > 
> 
> A knob (module_param?) to enable the permissive behaviour would certainly
> work for me.

I think a knob might be the worst of both worlds.  It'd be more to test,
and more to document.  I think explaining to a user when they'd want
to use the knob might be quite involved.  So personally I'd sooner
either make the change or not.

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.

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.

 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.

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

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

[Guillaume Nault]

On Fri, Jan 17, 2020 at 01:18:49PM +0000, Tom Parkin wrote:
> On  Fri, Jan 17, 2020 at 10:50:55 +1300, Ridge Kennedy wrote:
> > On Thu, 16 Jan 2020, Tom Parkin wrote:
> > 
> > > On  Thu, Jan 16, 2020 at 20:05:56 +0100, Guillaume Nault wrote:
> > > > On Thu, Jan 16, 2020 at 01:12:24PM +0000, Tom Parkin wrote:
> > > > > I agree with you about the possibility for cross-talk, and I would
> > > > > welcome l2tp_ip/ip6 doing more validation.  But I don't think we should
> > > > > ditch the global list.
> > > > > 
> > > > > As per the RFC, for L2TPv3 the session ID should be a unique
> > > > > identifier for the LCCE.  So it's reasonable that the kernel should
> > > > > enforce that when registering sessions.
> > > > > 
> > > > I had never thought that the session ID could have global significance
> > > > in L2TPv3, but maybe that's because my experience is mostly about
> > > > L2TPv2. I haven't read RFC 3931 in detail, but I can't see how
> > > > restricting the scope of sessions to their parent tunnel would conflict
> > > > with the RFC.
> > > 
> > > Sorry Guillaume, I responded to your other mail without reading this
> > > one.
> > > 
> > > I gave more detail in my other response: it boils down to how RFC 3931
> > > changes the use of IDs in the L2TP header.  Data packets for IP or UDP
> > > only contain the 32-bit session ID, and hence this must be unique to
> > > the LCCE to allow the destination session to be successfully
> > > identified.
> > > 
> > > > > When you're referring to cross-talk, I wonder whether you have in mind
> > > > > normal operation or malicious intent?  I suppose it would be possible
> > > > > for someone to craft session data packets in order to disrupt a
> > > > > session.
> > > > > 
> > > > What makes me uneasy is that, as soon as the l2tp_ip or l2tp_ip6 module
> > > > is loaded, a peer can reach whatever L2TPv3 session exists on the host
> > > > just by sending an L2TP_IP packet to it.
> > > > I don't know how practical it is to exploit this fact, but it looks
> > > > like it's asking for trouble.
> > > 
> > > Yes, I agree, although practically it's only a slightly easier
> > > "exploit" than L2TP/UDP offers.
> > > 
> > > The UDP case requires a rogue packet to be delivered to the correct
> > > socket AND have a session ID matching that of one in the associated
> > > tunnel.
> > > 
> > > It's a slightly more arduous scenario to engineer than the existing
> > > L2TPv3/IP case, but only a little.
> > > 
> > > I also don't know how practical this would be to leverage to cause
> > > problems.
> > > 
> > > > > For normal operation, you just need to get the wrong packet on the
> > > > > wrong socket to run into trouble of course.  In such a situation
> > > > > having a unique session ID for v3 helps you to determine that
> > > > > something has gone wrong, which is what the UDP encap recv path does
> > > > > if the session data packet's session ID isn't found in the context of
> > > > > the socket that receives it.
> > > > Unique global session IDs might help troubleshooting, but they also
> > > > break some use cases, as reported by Ridge. At some point, we'll have
> > > > to make a choice, or even add a knob if necessary.
> > > 
> > > I suspect we need to reach agreement on what RFC 3931 implies before
> > > making headway on what the kernel should ideally do.
> > > 
> > > There is perhaps room for pragmatism given that the kernel
> > > used to be more permissive prior to dbdbc73b4478, and we weren't
> > > inundated with reports of session ID clashes.
> > > 
> > 
> > A knob (module_param?) to enable the permissive behaviour would certainly
> > work for me.
> 
> I think a knob might be the worst of both worlds.  It'd be more to test,
> and more to document.  I think explaining to a user when they'd want
> to use the knob might be quite involved.  So personally I'd sooner
> either make the change or not.
> 
Yes, I'd also prefer to not have a knob, if possible.

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

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

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

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

[Tom Parkin]

[-- 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 --]

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

[Guillaume Nault]

On Fri, Jan 17, 2020 at 07:19:39PM +0000, Tom Parkin wrote:
> 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.
> 
Well, the situation is the same with UDP, when the peer always uses
source port 1701, which is a pretty common case as you noted
previously.
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.

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

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

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

[Tom Parkin]

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

On  Sat, Jan 18, 2020 at 20:13:36 +0100, Guillaume Nault wrote:
> On Fri, Jan 17, 2020 at 07:19:39PM +0000, Tom Parkin wrote:
> > 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.
> > 
> Well, the situation is the same with UDP, when the peer always uses
> source port 1701, which is a pretty common case as you noted
> previously.

Yes, it's the same situation as the UDP case; it's just much easier to
hit with IP encapsulation.

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

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

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.

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

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

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

[Guillaume Nault]

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.

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

[James Chapman]

On Tue, 21 Jan 2020 at 16:35, Guillaume Nault <gnault@redhat.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?

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

[Guillaume Nault]

On Wed, Jan 22, 2020 at 11:55:35AM +0000, James Chapman wrote:
> On Tue, 21 Jan 2020 at 16:35, Guillaume Nault <gnault@redhat.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.

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

[James Chapman]

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

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

[Guillaume Nault]

On Mon, Jan 27, 2020 at 09:25:30AM +0000, James Chapman wrote:
> On 25/01/2020 11:57, Guillaume Nault wrote:
> > On Wed, Jan 22, 2020 at 11:55:35AM +0000, James Chapman wrote:
> >> 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.
> 
I had started working on the data path more than a year ago, but never
got far enough to submit anything. I might revive this work if I find
enough time. But yes, sure, let's focus on the sessions IDs for now.

> >> 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 don't think that we could store the hash in the session structure.
The tunnel socket could be rebound or reconnected, thus changing the
5/3-tuple from under us.

My idea was to lookup the hash bucket using only the session ID, then
select the session from this bucket by checking both the session ID and
the 5/3-tuple.

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

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

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

> >> 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".
> 
I'm uncomfortable with this. 3 years is not that long, it's the typical
long term support time for community kernels (not even mentioning
"enterprise" distributions). Also, we have a report showing that the
current behaviour broke some use cases, while we never had any problem
reported for the ancient behaviour (which had been in place for 7
years). And finally, rejecting duplicate IDs, won't make the session ID
space global. As I pointed out earlier, L2TPoUDP sessions are still
going to be scoped in practice, because that's how lookup is done
currently. So I don't see what would be the benefit of artificially
limitting the sessions IDs accepted by the kernel (but I agree that
L2TPoIP session IDs have to remain unique in the network namespace).

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

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

[James Chapman]

On 29/01/2020 11:44, Guillaume Nault wrote:
> On Mon, Jan 27, 2020 at 09:25:30AM +0000, James Chapman wrote:
>> On 25/01/2020 11:57, Guillaume Nault wrote:
>>> On Wed, Jan 22, 2020 at 11:55:35AM +0000, James Chapman wrote:
>>>> 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.
>>
> I had started working on the data path more than a year ago, but never
> got far enough to submit anything. I might revive this work if I find
> enough time. But yes, sure, let's focus on the sessions IDs for now.
>
>>>> 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 don't think that we could store the hash in the session structure.
> The tunnel socket could be rebound or reconnected, thus changing the
> 5/3-tuple from under us.
>
> My idea was to lookup the hash bucket using only the session ID, then
> select the session from this bucket by checking both the session ID and
> the 5/3-tuple.

Ah I see. I'm more comfortable with this now.


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


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


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

L2TP_ATTR_SCOPE would be a good way for the app to select which
behaviour it prefers.


>>>> 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".
>>
> I'm uncomfortable with this. 3 years is not that long, it's the typical
> long term support time for community kernels (not even mentioning
> "enterprise" distributions). Also, we have a report showing that the
> current behaviour broke some use cases, while we never had any problem
> reported for the ancient behaviour (which had been in place for 7
> years). 
This is the policy decision. I see pros and cons both ways. But perhaps
it's ok as long as the session ID can be treated as unscoped as a config
option. See later.

> And finally, rejecting duplicate IDs, won't make the session ID
> space global. As I pointed out earlier, L2TPoUDP sessions are still
> going to be scoped in practice, because that's how lookup is done
> currently. So I don't see what would be the benefit of artificially
> limitting the sessions IDs accepted by the kernel (but I agree that
> L2TPoIP session IDs have to remain unique in the network namespace).

I'd like UDP and IP to eventually work the same way.


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

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

[Guillaume Nault]

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?

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

[James Chapman]

On 30/01/2020 22:34, 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?

It might use some netfilter or BPF code to change the IPs and redirect
outbound packets. TBH, it's a hypothetical use case and probably easier
to implement using scoped session IDs. :-)


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

Yes, that would be the safest approach.


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

Yes.


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

Yes, it sounds good to me.

Your proposed approach of using only the session ID to do the session
lookup but then optionally using the 3/5-tuple to scope it resolves my
concerns.

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

[Guillaume Nault]

On Fri, Jan 31, 2020 at 08:12:01AM +0000, James Chapman wrote:
> On 30/01/2020 22:34, Guillaume Nault wrote:
> > 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?
> 
> Yes, it sounds good to me.
> 
> Your proposed approach of using only the session ID to do the session
> lookup but then optionally using the 3/5-tuple to scope it resolves my
> concerns.
> 
Great! I'll ask Ridge to repost his patch then.
Thanks a lot.

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

[Tom Parkin]

[-- 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 --]

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

[Guillaume Nault]

On Fri, Jan 31, 2020 at 09:55:54AM +0000, Tom Parkin wrote:
> On  Thu, Jan 30, 2020 at 23:34:40 +0100, Guillaume Nault wrote:
> > 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.
> 
Nice to see that we're all on the same page.
Thanks!

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

[Guillaume Nault]

On Thu, Jan 16, 2020 at 09:23:32PM +0000, Tom Parkin wrote:
> On  Thu, Jan 16, 2020 at 20:05:56 +0100, Guillaume Nault wrote:
> > On Thu, Jan 16, 2020 at 01:12:24PM +0000, Tom Parkin wrote:
> > > I agree with you about the possibility for cross-talk, and I would
> > > welcome l2tp_ip/ip6 doing more validation.  But I don't think we should
> > > ditch the global list.
> > > 
> > > As per the RFC, for L2TPv3 the session ID should be a unique
> > > identifier for the LCCE.  So it's reasonable that the kernel should
> > > enforce that when registering sessions.
> > > 
> > I had never thought that the session ID could have global significance
> > in L2TPv3, but maybe that's because my experience is mostly about
> > L2TPv2. I haven't read RFC 3931 in detail, but I can't see how
> > restricting the scope of sessions to their parent tunnel would conflict
> > with the RFC.
> 
> Sorry Guillaume, I responded to your other mail without reading this
> one.
> 
> I gave more detail in my other response: it boils down to how RFC 3931
> changes the use of IDs in the L2TP header.  Data packets for IP or UDP
> only contain the 32-bit session ID, and hence this must be unique to
> the LCCE to allow the destination session to be successfully
> identified.
> 
> > > When you're referring to cross-talk, I wonder whether you have in mind
> > > normal operation or malicious intent?  I suppose it would be possible
> > > for someone to craft session data packets in order to disrupt a
> > > session.
> > > 
> > What makes me uneasy is that, as soon as the l2tp_ip or l2tp_ip6 module
> > is loaded, a peer can reach whatever L2TPv3 session exists on the host
> > just by sending an L2TP_IP packet to it.
> > I don't know how practical it is to exploit this fact, but it looks
> > like it's asking for trouble.
> 
> Yes, I agree, although practically it's only a slightly easier
> "exploit" than L2TP/UDP offers.
> 
> The UDP case requires a rogue packet to be delivered to the correct
> socket AND have a session ID matching that of one in the associated
> tunnel.
> 
> It's a slightly more arduous scenario to engineer than the existing
> L2TPv3/IP case, but only a little.
> 
In the UDP case, we have a socket connected to the peer (or at least
bound to a local address). That is, some local setup is needed. With
l2tp_ip, we don't even need to have an open socket. Everything is
triggered remotely. And here, "remotely" means "any host on any IP
network the LCCE is connected", because the destination address can
be any address assigned to the LCCE, even if it's not configured to
handle any kind of L2TP. But well, after thinking more about our L2TPv3
discussiong, I guess that's how the designer's of the protocol wanted.

> I also don't know how practical this would be to leverage to cause
> problems.
> 
> > > For normal operation, you just need to get the wrong packet on the
> > > wrong socket to run into trouble of course.  In such a situation
> > > having a unique session ID for v3 helps you to determine that
> > > something has gone wrong, which is what the UDP encap recv path does
> > > if the session data packet's session ID isn't found in the context of
> > > the socket that receives it.
> > Unique global session IDs might help troubleshooting, but they also
> > break some use cases, as reported by Ridge. At some point, we'll have
> > to make a choice, or even add a knob if necessary.
> 
> I suspect we need to reach agreement on what RFC 3931 implies before
> making headway on what the kernel should ideally do.
> 
> There is perhaps room for pragmatism given that the kernel
> used to be more permissive prior to dbdbc73b4478, and we weren't
> inundated with reports of session ID clashes.
> 
To summarise, my understanding is that global session IDs would follow
the spirit of RFC 3931 and would allow establishing multiple L2TPv3
connections (tunnels) over the same 5-tuple (or 3-tuple for IP encap).
Per socket session IDs don't, but would allow fixing Ridge's case.

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

[Tom Parkin]

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

On  Fri, Jan 17, 2020 at 17:36:27 +0100, Guillaume Nault wrote:
> On Thu, Jan 16, 2020 at 09:23:32PM +0000, Tom Parkin wrote:
> > On  Thu, Jan 16, 2020 at 20:05:56 +0100, Guillaume Nault wrote:
> > > What makes me uneasy is that, as soon as the l2tp_ip or l2tp_ip6 module
> > > is loaded, a peer can reach whatever L2TPv3 session exists on the host
> > > just by sending an L2TP_IP packet to it.
> > > I don't know how practical it is to exploit this fact, but it looks
> > > like it's asking for trouble.
> > 
> > Yes, I agree, although practically it's only a slightly easier
> > "exploit" than L2TP/UDP offers.
> > 
> > The UDP case requires a rogue packet to be delivered to the correct
> > socket AND have a session ID matching that of one in the associated
> > tunnel.
> > 
> > It's a slightly more arduous scenario to engineer than the existing
> > L2TPv3/IP case, but only a little.
> > 
> In the UDP case, we have a socket connected to the peer (or at least
> bound to a local address). That is, some local setup is needed. With
> l2tp_ip, we don't even need to have an open socket. Everything is
> triggered remotely. And here, "remotely" means "any host on any IP
> network the LCCE is connected", because the destination address can
> be any address assigned to the LCCE, even if it's not configured to
> handle any kind of L2TP. But well, after thinking more about our L2TPv3
> discussiong, I guess that's how the designer's of the protocol wanted.

I note that RFC 3931 provides for a cookie in the data packet header
to mitigate against data packet spoofing (section 8.2).

More generally I've not tried to see what could be done to spoof
session data packets, so I can't really comment further.  It would be
interesting to try spoofing packets and see if the kernel code could
do more to limit any potential problems.

> > > > For normal operation, you just need to get the wrong packet on the
> > > > wrong socket to run into trouble of course.  In such a situation
> > > > having a unique session ID for v3 helps you to determine that
> > > > something has gone wrong, which is what the UDP encap recv path does
> > > > if the session data packet's session ID isn't found in the context of
> > > > the socket that receives it.
> > > Unique global session IDs might help troubleshooting, but they also
> > > break some use cases, as reported by Ridge. At some point, we'll have
> > > to make a choice, or even add a knob if necessary.
> > 
> > I suspect we need to reach agreement on what RFC 3931 implies before
> > making headway on what the kernel should ideally do.
> > 
> > There is perhaps room for pragmatism given that the kernel
> > used to be more permissive prior to dbdbc73b4478, and we weren't
> > inundated with reports of session ID clashes.
> > 
> To summarise, my understanding is that global session IDs would follow
> the spirit of RFC 3931 and would allow establishing multiple L2TPv3
> connections (tunnels) over the same 5-tuple (or 3-tuple for IP encap).
> Per socket session IDs don't, but would allow fixing Ridge's case.

I'm not 100% certain what "per socket session IDs" means here.  Could
you clarify?

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

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

[Guillaume Nault]

On Fri, Jan 17, 2020 at 07:29:12PM +0000, Tom Parkin wrote:
> On  Fri, Jan 17, 2020 at 17:36:27 +0100, Guillaume Nault wrote:
> > On Thu, Jan 16, 2020 at 09:23:32PM +0000, Tom Parkin wrote:
> > > On  Thu, Jan 16, 2020 at 20:05:56 +0100, Guillaume Nault wrote:
> > > > What makes me uneasy is that, as soon as the l2tp_ip or l2tp_ip6 module
> > > > is loaded, a peer can reach whatever L2TPv3 session exists on the host
> > > > just by sending an L2TP_IP packet to it.
> > > > I don't know how practical it is to exploit this fact, but it looks
> > > > like it's asking for trouble.
> > > 
> > > Yes, I agree, although practically it's only a slightly easier
> > > "exploit" than L2TP/UDP offers.
> > > 
> > > The UDP case requires a rogue packet to be delivered to the correct
> > > socket AND have a session ID matching that of one in the associated
> > > tunnel.
> > > 
> > > It's a slightly more arduous scenario to engineer than the existing
> > > L2TPv3/IP case, but only a little.
> > > 
> > In the UDP case, we have a socket connected to the peer (or at least
> > bound to a local address). That is, some local setup is needed. With
> > l2tp_ip, we don't even need to have an open socket. Everything is
> > triggered remotely. And here, "remotely" means "any host on any IP
> > network the LCCE is connected", because the destination address can
> > be any address assigned to the LCCE, even if it's not configured to
> > handle any kind of L2TP. But well, after thinking more about our L2TPv3
> > discussiong, I guess that's how the designer's of the protocol wanted.
> 
> I note that RFC 3931 provides for a cookie in the data packet header
> to mitigate against data packet spoofing (section 8.2).
>
Indeed, I forgot about the L2TPv3 cookie.

> More generally I've not tried to see what could be done to spoof
> session data packets, so I can't really comment further.  It would be
> interesting to try spoofing packets and see if the kernel code could
> do more to limit any potential problems.
> 
> > > > > For normal operation, you just need to get the wrong packet on the
> > > > > wrong socket to run into trouble of course.  In such a situation
> > > > > having a unique session ID for v3 helps you to determine that
> > > > > something has gone wrong, which is what the UDP encap recv path does
> > > > > if the session data packet's session ID isn't found in the context of
> > > > > the socket that receives it.
> > > > Unique global session IDs might help troubleshooting, but they also
> > > > break some use cases, as reported by Ridge. At some point, we'll have
> > > > to make a choice, or even add a knob if necessary.
> > > 
> > > I suspect we need to reach agreement on what RFC 3931 implies before
> > > making headway on what the kernel should ideally do.
> > > 
> > > There is perhaps room for pragmatism given that the kernel
> > > used to be more permissive prior to dbdbc73b4478, and we weren't
> > > inundated with reports of session ID clashes.
> > > 
> > To summarise, my understanding is that global session IDs would follow
> > the spirit of RFC 3931 and would allow establishing multiple L2TPv3
> > connections (tunnels) over the same 5-tuple (or 3-tuple for IP encap).
> > Per socket session IDs don't, but would allow fixing Ridge's case.
> 
> I'm not 100% certain what "per socket session IDs" means here.  Could
> you clarify?
> 
By "per socket session IDs", I mean that the session IDs have to be
interpreted in the context of their parent tunnel socket (the current
l2tp_udp_recv_core() approach). That's opposed to "global session IDs"
which have netns-wide significance (the current l2tp_ip_recv()
approach).

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

[Tom Parkin]

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

On  Sat, Jan 18, 2020 at 18:52:24 +0100, Guillaume Nault wrote:
> On Fri, Jan 17, 2020 at 07:29:12PM +0000, Tom Parkin wrote:
> > On  Fri, Jan 17, 2020 at 17:36:27 +0100, Guillaume Nault wrote:
> > > To summarise, my understanding is that global session IDs would follow
> > > the spirit of RFC 3931 and would allow establishing multiple L2TPv3
> > > connections (tunnels) over the same 5-tuple (or 3-tuple for IP encap).
> > > Per socket session IDs don't, but would allow fixing Ridge's case.
> > 
> > I'm not 100% certain what "per socket session IDs" means here.  Could
> > you clarify?
> > 
> By "per socket session IDs", I mean that the session IDs have to be
> interpreted in the context of their parent tunnel socket (the current
> l2tp_udp_recv_core() approach). That's opposed to "global session IDs"
> which have netns-wide significance (the current l2tp_ip_recv()
> approach).
>

OK, thanks for confirming.

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

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

[Ridge Kennedy]

On Thu, 16 Jan 2020, Guillaume Nault wrote:

> On Thu, Jan 16, 2020 at 11:34:47AM +1300, Ridge Kennedy wrote:
>> In the past it was possible to create multiple L2TPv3 sessions with the
>> same session id as long as the sessions belonged to different tunnels.
>> The resulting sessions had issues when used with IP encapsulated tunnels,
>> but worked fine with UDP encapsulated ones. Some applications began to
>> rely on this behaviour to avoid having to negotiate unique session ids.
>>
>> Some time ago a change was made to require session ids to be unique across
>> all tunnels, breaking the applications making use of this "feature".
>>
>> This change relaxes the duplicate session id check to allow duplicates
>> if both of the colliding sessions belong to UDP encapsulated tunnels.
>>
>> Fixes: dbdbc73b4478 ("l2tp: fix duplicate session creation")
>> Signed-off-by: Ridge Kennedy <ridge.kennedy@alliedtelesis.co.nz>
>> ---
>>  net/l2tp/l2tp_core.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
>> index f82ea12bac37..0cc86227c618 100644
>> --- a/net/l2tp/l2tp_core.c
>> +++ b/net/l2tp/l2tp_core.c
>> @@ -323,7 +323,9 @@ int l2tp_session_register(struct l2tp_session *session,
>>  		spin_lock_bh(&pn->l2tp_session_hlist_lock);
>>
>>  		hlist_for_each_entry(session_walk, g_head, global_hlist)
>> -			if (session_walk->session_id == session->session_id) {
>> +			if (session_walk->session_id == session->session_id &&
>> +			    (session_walk->tunnel->encap == L2TP_ENCAPTYPE_IP ||
>> +			     tunnel->encap == L2TP_ENCAPTYPE_IP)) {
>>  				err = -EEXIST;
>>  				goto err_tlock_pnlock;
>>  			}
> Well, I think we have a more fundamental problem here. By adding
> L2TPoUDP sessions to the global list, we allow cross-talks with L2TPoIP
> sessions. That is, if we have an L2TPv3 session X running over UDP and
> we receive an L2TP_IP packet targetted at session ID X, then
> l2tp_session_get() will return the L2TP_UDP session to l2tp_ip_recv().
>
> I guess l2tp_session_get() should be dropped and l2tp_ip_recv() should
> look up the session in the context of its socket, like in the UDP case.
>
> But for the moment, what about just not adding L2TP_UDP sessions to the
> global list? That should fix both your problem and the L2TP_UDP/L2TP_IP
> cross-talks.

I did consider not adding the L2TP_UDP sessions to the global list, but 
that change looked a little more involved as the netlink interface was 
also making use of the list to lookup sessions by ifname. At a minimum
it looks like this would require rework of l2tp_session_get_by_ifname().

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

[Guillaume Nault]

On Fri, Jan 17, 2020 at 10:26:09AM +1300, Ridge Kennedy wrote:
> On Thu, 16 Jan 2020, Guillaume Nault wrote:
> > On Thu, Jan 16, 2020 at 11:34:47AM +1300, Ridge Kennedy wrote:
> > > diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> > > index f82ea12bac37..0cc86227c618 100644
> > > --- a/net/l2tp/l2tp_core.c
> > > +++ b/net/l2tp/l2tp_core.c
> > > @@ -323,7 +323,9 @@ int l2tp_session_register(struct l2tp_session *session,
> > >  		spin_lock_bh(&pn->l2tp_session_hlist_lock);
> > > 
> > >  		hlist_for_each_entry(session_walk, g_head, global_hlist)
> > > -			if (session_walk->session_id == session->session_id) {
> > > +			if (session_walk->session_id == session->session_id &&
> > > +			    (session_walk->tunnel->encap == L2TP_ENCAPTYPE_IP ||
> > > +			     tunnel->encap == L2TP_ENCAPTYPE_IP)) {
> > >  				err = -EEXIST;
> > >  				goto err_tlock_pnlock;
> > >  			}
> > Well, I think we have a more fundamental problem here. By adding
> > L2TPoUDP sessions to the global list, we allow cross-talks with L2TPoIP
> > sessions. That is, if we have an L2TPv3 session X running over UDP and
> > we receive an L2TP_IP packet targetted at session ID X, then
> > l2tp_session_get() will return the L2TP_UDP session to l2tp_ip_recv().
> > 
> > I guess l2tp_session_get() should be dropped and l2tp_ip_recv() should
> > look up the session in the context of its socket, like in the UDP case.
> > 
> > But for the moment, what about just not adding L2TP_UDP sessions to the
> > global list? That should fix both your problem and the L2TP_UDP/L2TP_IP
> > cross-talks.
> 
> I did consider not adding the L2TP_UDP sessions to the global list, but that
> change looked a little more involved as the netlink interface was also
> making use of the list to lookup sessions by ifname. At a minimum
> it looks like this would require rework of l2tp_session_get_by_ifname().
> 
Yes, you're right. Now that we all agree on this fix, can you please
repost your patch?

BTW, I wouldn't mind a small comment on top of the conditional to
explain that IP encap assumes globally unique session IDs while UDP
doesn't.

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

[Ridge Kennedy]

On Fri, 31 Jan 2020, Guillaume Nault wrote:

> On Fri, Jan 17, 2020 at 10:26:09AM +1300, Ridge Kennedy wrote:
>> On Thu, 16 Jan 2020, Guillaume Nault wrote:
>>> On Thu, Jan 16, 2020 at 11:34:47AM +1300, Ridge Kennedy wrote:
>>>> diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
>>>> index f82ea12bac37..0cc86227c618 100644
>>>> --- a/net/l2tp/l2tp_core.c
>>>> +++ b/net/l2tp/l2tp_core.c
>>>> @@ -323,7 +323,9 @@ int l2tp_session_register(struct l2tp_session *session,
>>>>  		spin_lock_bh(&pn->l2tp_session_hlist_lock);
>>>>
>>>>  		hlist_for_each_entry(session_walk, g_head, global_hlist)
>>>> -			if (session_walk->session_id == session->session_id) {
>>>> +			if (session_walk->session_id == session->session_id &&
>>>> +			    (session_walk->tunnel->encap == L2TP_ENCAPTYPE_IP ||
>>>> +			     tunnel->encap == L2TP_ENCAPTYPE_IP)) {
>>>>  				err = -EEXIST;
>>>>  				goto err_tlock_pnlock;
>>>>  			}
>>> Well, I think we have a more fundamental problem here. By adding
>>> L2TPoUDP sessions to the global list, we allow cross-talks with L2TPoIP
>>> sessions. That is, if we have an L2TPv3 session X running over UDP and
>>> we receive an L2TP_IP packet targetted at session ID X, then
>>> l2tp_session_get() will return the L2TP_UDP session to l2tp_ip_recv().
>>>
>>> I guess l2tp_session_get() should be dropped and l2tp_ip_recv() should
>>> look up the session in the context of its socket, like in the UDP case.
>>>
>>> But for the moment, what about just not adding L2TP_UDP sessions to the
>>> global list? That should fix both your problem and the L2TP_UDP/L2TP_IP
>>> cross-talks.
>>
>> I did consider not adding the L2TP_UDP sessions to the global list, but that
>> change looked a little more involved as the netlink interface was also
>> making use of the list to lookup sessions by ifname. At a minimum
>> it looks like this would require rework of l2tp_session_get_by_ifname().
>>
> Yes, you're right. Now that we all agree on this fix, can you please
> repost your patch?
>
> BTW, I wouldn't mind a small comment on top of the conditional to
> explain that IP encap assumes globally unique session IDs while UDP
> doesn't.
>
Thanks all, I have reposted the patch with added comment.