| 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 |
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; > } >
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 {
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.
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 --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; }
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(+)