diff mbox series

l2tp: Support several sockets with same IP/port quadruple

Message ID 20240502231418.2933925-1-samuel.thibault@ens-lyon.org (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series l2tp: Support several sockets with same IP/port quadruple | expand

Checks

Context Check Description
netdev/series_format warning Single patches do not need cover letters; Target tree name not specified in the subject
netdev/tree_selection success Guessed tree name to be net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 926 this patch: 926
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 1 maintainers not CCed: willemb@google.com
netdev/build_clang success Errors and warnings before: 937 this patch: 937
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 937 this patch: 937
netdev/checkpatch warning WARNING: line length of 82 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-05-03--15-00 (tests: 1001)

Commit Message

Samuel Thibault May 2, 2024, 11:14 p.m. UTC
Some l2tp providers will use 1701 as origin port and open several
tunnels for the same origin and target. On the Linux side, this
may mean opening several sockets, but then trafic will go to only
one of them, losing the trafic for the tunnel of the other socket
(or leaving it up to userland, consuming a lot of cpu%).

This can also happen when the l2tp provider uses a cluster, and
load-balancing happens to migrate from one origin IP to another one,
for which a socket was already established. Managing reassigning
tunnels from one socket to another would be very hairy for userland.

Lastly, as documented in l2tpconfig(1), as client it may be necessary
to use 1701 as origin port for odd firewalls reasons, which could
prevent from establishing several tunnels to a l2tp server, for the
same reason: trafic would get only on one of the two sockets.

With the V2 protocol it is however easy to route trafic to the proper
tunnel, by looking up the tunnel number in the network namespace. This
fixes the three cases altogether.

Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
---
 net/l2tp/l2tp_core.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

Comments

James Chapman May 3, 2024, 11:36 a.m. UTC | #1
On 03/05/2024 00:14, Samuel Thibault wrote:
> Some l2tp providers will use 1701 as origin port and open several
> tunnels for the same origin and target. On the Linux side, this
> may mean opening several sockets, but then trafic will go to only
> one of them, losing the trafic for the tunnel of the other socket
> (or leaving it up to userland, consuming a lot of cpu%).
>
> This can also happen when the l2tp provider uses a cluster, and
> load-balancing happens to migrate from one origin IP to another one,
> for which a socket was already established. Managing reassigning
> tunnels from one socket to another would be very hairy for userland.
>
> Lastly, as documented in l2tpconfig(1), as client it may be necessary
> to use 1701 as origin port for odd firewalls reasons, which could
> prevent from establishing several tunnels to a l2tp server, for the
> same reason: trafic would get only on one of the two sockets.
>
> With the V2 protocol it is however easy to route trafic to the proper
> tunnel, by looking up the tunnel number in the network namespace. This
> fixes the three cases altogether.

Hi Samuel,

Thanks for working on this.

I'm currently working on changes that address this for both L2TPv2 and 
L2TPv3 which will avoid separate tunnel and session lookups in the 
datapath. However, my changes aren't ready yet; I hope to post them in a 
week or so.

Please find comments on your patch inline below.

> Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> ---
>   net/l2tp/l2tp_core.c | 21 +++++++++++++++++++++
>   1 file changed, 21 insertions(+)
>
> diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> index 8d21ff25f160..128f1146c135 100644
> --- a/net/l2tp/l2tp_core.c
> +++ b/net/l2tp/l2tp_core.c
> @@ -794,6 +794,7 @@ static void l2tp_session_queue_purge(struct l2tp_session *session)
>   static int l2tp_udp_recv_core(struct l2tp_tunnel *tunnel, struct sk_buff *skb)
>   {
>   	struct l2tp_session *session = NULL;
> +	struct l2tp_tunnel *orig_tunnel = tunnel;
>   	unsigned char *ptr, *optr;
>   	u16 hdrflags;
>   	u32 tunnel_id, session_id;
> @@ -845,6 +846,20 @@ static int l2tp_udp_recv_core(struct l2tp_tunnel *tunnel, struct sk_buff *skb)
>   		/* Extract tunnel and session ID */
>   		tunnel_id = ntohs(*(__be16 *)ptr);
>   		ptr += 2;
> +
> +		if (tunnel_id != tunnel->tunnel_id && tunnel->l2tp_net) {
Can tunnel->l2tp_net be NULL?
> +			/* We are receiving trafic for another tunnel, probably
> +			 * because we have several tunnels between the same
> +			 * IP/port quadruple, look it up.
> +			 */
> +			struct l2tp_tunnel *alt_tunnel;
> +
> +			alt_tunnel = l2tp_tunnel_get(tunnel->l2tp_net, tunnel_id);
This misses a check that alt_tunnel's protocol version matches the 
header. Move the existing header version check to after this fragment?
> +			if (!alt_tunnel)
> +				goto pass;
> +			tunnel = alt_tunnel;
> +		}
> +
>   		session_id = ntohs(*(__be16 *)ptr);
>   		ptr += 2;
>   	} else {
> @@ -875,6 +890,9 @@ static int l2tp_udp_recv_core(struct l2tp_tunnel *tunnel, struct sk_buff *skb)
>   	l2tp_recv_common(session, skb, ptr, optr, hdrflags, length);
>   	l2tp_session_dec_refcount(session);
>   
> +	if (tunnel != orig_tunnel)
> +		l2tp_tunnel_dec_refcount(tunnel);
> +
>   	return 0;
>   
>   invalid:
> @@ -884,6 +902,9 @@ static int l2tp_udp_recv_core(struct l2tp_tunnel *tunnel, struct sk_buff *skb)
>   	/* Put UDP header back */
>   	__skb_push(skb, sizeof(struct udphdr));
>   
> +	if (tunnel != orig_tunnel)
> +		l2tp_tunnel_dec_refcount(tunnel);
> +
>   	return 1;
>   }
>
Samuel Thibault May 6, 2024, 9:44 p.m. UTC | #2
Hello,

James Chapman, le ven. 03 mai 2024 12:36:14 +0100, a ecrit:
> > @@ -845,6 +846,20 @@ static int l2tp_udp_recv_core(struct l2tp_tunnel *tunnel, struct sk_buff *skb)
> >   		/* Extract tunnel and session ID */
> >   		tunnel_id = ntohs(*(__be16 *)ptr);
> >   		ptr += 2;
> > +
> > +		if (tunnel_id != tunnel->tunnel_id && tunnel->l2tp_net) {
> Can tunnel->l2tp_net be NULL?

l2tp_tunnel_sock_create's comment says

 * Since we don't want these sockets to keep a namespace alive by
 * themselves, we drop the socket's namespace refcount after creation.
 * These sockets are freed when the namespace exits using the pernet
 * exit hook.

and l2tp_tunnel_create does not set l2tp_net, only l2tp_tunnel_register
does, so I assumed it might be NULL and preferred to stay on
the safe side. But it's l2tp_tunnel_register which adds it to
pn->l2tp_tunnel_idr, so AIUI it indeed cannot be NULL since we got it
from pn->l2tp_tunnel_idr, we can probably drop the test indeed.

> > +			/* We are receiving trafic for another tunnel, probably
> > +			 * because we have several tunnels between the same
> > +			 * IP/port quadruple, look it up.
> > +			 */
> > +			struct l2tp_tunnel *alt_tunnel;
> > +
> > +			alt_tunnel = l2tp_tunnel_get(tunnel->l2tp_net, tunnel_id);
> This misses a check that alt_tunnel's protocol version matches the header.
> Move the existing header version check to after this fragment?

We need to check the version before getting the tunnel id, which we need
to look up the struct l2tp_tunnel :)

I'll add another version check.

Samuel

> > +			if (!alt_tunnel)
> > +				goto pass;
> > +			tunnel = alt_tunnel;
> > +		}
> > +
> >   		session_id = ntohs(*(__be16 *)ptr);
> >   		ptr += 2;
> >   	} else {
James Chapman May 7, 2024, 8:06 a.m. UTC | #3
Hi Samuel,

On 06/05/2024 22:44, Samuel Thibault wrote:
> Hello,
>
> James Chapman, le ven. 03 mai 2024 12:36:14 +0100, a ecrit:
>
>>> +			/* We are receiving trafic for another tunnel, probably
>>> +			 * because we have several tunnels between the same
>>> +			 * IP/port quadruple, look it up.
>>> +			 */
>>> +			struct l2tp_tunnel *alt_tunnel;
>>> +
>>> +			alt_tunnel = l2tp_tunnel_get(tunnel->l2tp_net, tunnel_id);
>> This misses a check that alt_tunnel's protocol version matches the header.
>> Move the existing header version check to after this fragment?
> We need to check the version before getting the tunnel id, which we need
> to look up the struct l2tp_tunnel :)
I was referring to the following code fragment which is before your change:

 >    version = hdrflags & L2TP_HDR_VER_MASK;
 >    if (version != tunnel->version) {
 >        pr_debug_ratelimited("%s: recv protocol version mismatch: got 
%d expected %d\n",
 >                     tunnel->name, version, tunnel->version);
 >        goto invalid;
 >    }

The tunnel->version check should now be done after the tunnel pointer is 
possibly modified by your code.

Also, if the tunnel pointer from sk_user_data isn't trusted due to 
5-tuple aliasing, l2tp_udp_recv_core should compare with the local 
'version' variable, not tunnel->version, when parsing the L2TP IDs e.g.:

 >    if (version == L2TP_HDR_VER_2) {
 >        /* If length is present, skip it */

otherwise, L2TPv2 socket aliasing will still not work properly if one or 
more L2TPv3 sockets also alias L2TPv2 sockets, even if there is no 
L2TPv3 traffic.
Samuel Thibault May 7, 2024, 10:29 a.m. UTC | #4
James Chapman, le mar. 07 mai 2024 09:06:35 +0100, a ecrit:
> otherwise, L2TPv2 socket aliasing will still not work properly if one or
> more L2TPv3 sockets also alias L2TPv2 sockets, even if there is no L2TPv3
> traffic.

Ah, I assumed this would not happen (a given l2tp source would usually
either speak v2 or v3), but we can rework the checks to support it if it
can be useful to somebody, indeed.

Samuel
diff mbox series

Patch

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 8d21ff25f160..128f1146c135 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -794,6 +794,7 @@  static void l2tp_session_queue_purge(struct l2tp_session *session)
 static int l2tp_udp_recv_core(struct l2tp_tunnel *tunnel, struct sk_buff *skb)
 {
 	struct l2tp_session *session = NULL;
+	struct l2tp_tunnel *orig_tunnel = tunnel;
 	unsigned char *ptr, *optr;
 	u16 hdrflags;
 	u32 tunnel_id, session_id;
@@ -845,6 +846,20 @@  static int l2tp_udp_recv_core(struct l2tp_tunnel *tunnel, struct sk_buff *skb)
 		/* Extract tunnel and session ID */
 		tunnel_id = ntohs(*(__be16 *)ptr);
 		ptr += 2;
+
+		if (tunnel_id != tunnel->tunnel_id && tunnel->l2tp_net) {
+			/* We are receiving trafic for another tunnel, probably
+			 * because we have several tunnels between the same
+			 * IP/port quadruple, look it up.
+			 */
+			struct l2tp_tunnel *alt_tunnel;
+
+			alt_tunnel = l2tp_tunnel_get(tunnel->l2tp_net, tunnel_id);
+			if (!alt_tunnel)
+				goto pass;
+			tunnel = alt_tunnel;
+		}
+
 		session_id = ntohs(*(__be16 *)ptr);
 		ptr += 2;
 	} else {
@@ -875,6 +890,9 @@  static int l2tp_udp_recv_core(struct l2tp_tunnel *tunnel, struct sk_buff *skb)
 	l2tp_recv_common(session, skb, ptr, optr, hdrflags, length);
 	l2tp_session_dec_refcount(session);
 
+	if (tunnel != orig_tunnel)
+		l2tp_tunnel_dec_refcount(tunnel);
+
 	return 0;
 
 invalid:
@@ -884,6 +902,9 @@  static int l2tp_udp_recv_core(struct l2tp_tunnel *tunnel, struct sk_buff *skb)
 	/* Put UDP header back */
 	__skb_push(skb, sizeof(struct udphdr));
 
+	if (tunnel != orig_tunnel)
+		l2tp_tunnel_dec_refcount(tunnel);
+
 	return 1;
 }