Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial support for Multipath TCP on recent Linux kernels #1

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Initial support for Multipath TCP on recent Linux kernels #1

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
daff3af
Initial support for Multipath TCP on recent Linux kernels
arinc9 Jun 19, 2024
669fef3
Address certain suggestions of Matthieu's
arinc9 Jun 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions configure.ac
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,12 @@ AC_ARG_WITH(
[with_openssl_engine="auto"]
)

AC_ARG_WITH(mptcp,
[AS_HELP_STRING([--without-mptcp],[Disable Multipath TCP support])],
arinc9 marked this conversation as resolved.
Show resolved Hide resolved
[enable_mptcp=no],
arinc9 marked this conversation as resolved.
Show resolved Hide resolved
[enable_mptcp=yes]
)

AC_ARG_VAR([PLUGINDIR], [Path of plug-in directory @<:@default=LIBDIR/openvpn/plugins@:>@])
if test -n "${PLUGINDIR}"; then
plugindir="${PLUGINDIR}"
Expand Down Expand Up @@ -880,6 +886,46 @@ case "$host" in
esac


dnl
dnl Checking Multipath TCP support on Linux
dnl
case "$host" in
*-*-linux*)
AC_MSG_CHECKING([Multipath TCP support ])
AS_IF([test "x$enable_mptcp" != xno],
[AC_RUN_IFELSE( [AC_LANG_PROGRAM([[
#include <stdio.h>
#include <stdlib.h>
#include <sys/types.h>
#include <sys/socket.h>
#ifndef IPPROTO_MPTCP
#define IPPROTO_MPTCP 262
#endif
int x=0;
]],
[[
int s= socket(AF_INET, SOCK_STREAM, IPPROTO_MPTCP);
if(s!=-1)
{
close(s);
return(0);
}
else
{
return(-1);
}
]]
) ],
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is needed: often these days, the binaries are compiled on a build host, probably with a different kernel that the one end-users will have.

In other words, I don't think you should check if the kernel support this at build time. It has to be done later.

What might be useful is to only define ENABLE_MPTCP on Linux target. Then you don't need to have #if defined(TARGET_LINUX) && defined(ENABLE_MPTCP) in the code, just ifdef ENABLE_MPTCP

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matttbe sorry I've got no experience with autoconf. Do you mean something like this:

dnl
dnl Enable checking Multipath TCP support on Linux
dnl
case "$host" in
	*-*-linux*)
		AC_DEFINE(ENABLE_MPTCP, 1, [Enable checking Multipath TCP support])
	;;
esac

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you should do that there, but in the default option with AC_ARG (BTW, it should be an ENABLE, not a WITH), something like that (not tested):

AC_ARG_ENABLE(
	[mptcp],
	[AS_HELP_STRING([--disable-mptcp], [disable ultipath TCP support @<:@default=yes@:>@ on Linux only])],
	,
	[
		case "$host" in
			*-*-linux*) enable_mptcp="yes";;
			*) enable_mptcp="no" ;;
		esac
	]
)

[AC_DEFINE([ENABLE_MPTCP], [1],
[AC_MSG_RESULT([Multipath TCP is enabled on this system])] )],
[ AC_MSG_RESULT([Multipath TCP is not enabled. On Linux, you need a kernel >= 5.15 and ensure that sysctl.net.mptcp_enabled is set to 1]) ],
)
])
;;
esac



if test "${with_crypto_library}" = "openssl"; then
AC_ARG_VAR([OPENSSL_CFLAGS], [C compiler flags for OpenSSL])
AC_ARG_VAR([OPENSSL_LIBS], [linker flags for OpenSSL])
Expand Down
19 changes: 19 additions & 0 deletions src/openvpn/options.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,9 @@ static const char usage_message[] =
" udp6, tcp6-server, tcp6-client\n"
"--proto-force p : only consider protocol p in list of connection profiles.\n"
" p = udp or tcp\n"
#if defined(TARGET_LINUX) && defined(ENABLE_MPTCP)
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(see my comment above: you don't need TARGET_LINUX, same below at a few places)

"--multipath : Enable Multipath TCP on the TCP connections.\n"
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why placing it here? Just to be next to the --proto option? (it would make sense but just to know, might be interesting to add this in the commit message)

(same as below: I would recommend to use --mptcp to avoid confusions)

(or maybe --proto-mptcp?)

or modify the behaviour of --proto? but then you will need to allow mptcp{,4,6}{,-client,server}, maybe not what we want?

#endif
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you need to edit a man page, documentation or something like that, no?

"--connect-retry n [m] : For client, number of seconds to wait between\n"
" connection retries (default=%d). On repeated retries\n"
" the wait time is exponentially increased to a maximum of m\n"
Expand Down Expand Up @@ -905,6 +908,10 @@ init_options(struct options *o, const bool init_gc)
#ifndef ENABLE_DCO
o->tuntap_options.disable_dco = true;
#endif /* ENABLE_DCO */

#if defined(TARGET_LINUX) && defined(ENABLE_MPTCP)
o->enable_multipath = false;
#endif
}

void
Expand Down Expand Up @@ -9512,6 +9519,18 @@ add_option(struct options *options,
goto err;
}
}
#if defined(TARGET_LINUX) && defined(ENABLE_MPTCP)
else if (streq(p[0], "multipath"))
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(mptcp?)

{
VERIFY_PERMISSION(OPT_P_GENERAL);
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I don't know what's the best value for this but I guess you looked at the other ones)

if (p[1])
{
msg(msglevel, "--multipath does not accept any parameters");
arinc9 marked this conversation as resolved.
Show resolved Hide resolved
goto err;
}
options->enable_multipath = true;
}
#endif
else
{
int i;
Expand Down
3 changes: 3 additions & 0 deletions src/openvpn/options.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -460,6 +460,9 @@ struct options
#define SF_NO_PUSH_ROUTE_GATEWAY (1<<2)
unsigned int server_flags;

#ifdef ENABLE_MPTCP
bool enable_multipath;
arinc9 marked this conversation as resolved.
Show resolved Hide resolved
#endif
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(personally, I would have recommended not to surround this option with the ifdef and limit them only where needed (when parsing option and when creating the socket) but I see there are plenty of ifdef above for other options so I guess it is good like that.

bool server_bridge_proxy_dhcp;

bool server_bridge_defined;
Expand Down
12 changes: 12 additions & 0 deletions src/openvpn/ps.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,14 @@

#include "memdbg.h"


#if defined(TARGET_LINUX) && defined(ENABLE_MPTCP)
arinc9 marked this conversation as resolved.
Show resolved Hide resolved
#ifndef IPPROTO_MPTCP
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you not do that in a header file once?

#define IPPROTO_MPTCP 262
#endif
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you can remove that as it is now defined in socket.h

#endif


struct port_share *port_share = NULL; /* GLOBAL */

/* size of i/o buffers */
Expand Down Expand Up @@ -425,7 +433,11 @@ proxy_entry_new(struct proxy_connection **list,
struct proxy_connection *cp;

/* connect to port share server */
#if defined(TARGET_LINUX) && defined(ENABLE_MPTCP)
arinc9 marked this conversation as resolved.
Show resolved Hide resolved
if ((sd_server = socket(PF_INET, SOCK_STREAM, IPPROTO_MPTCP)) < 0)
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here you might want to avoid duplicating everything and do something like:

if ((sd_server = socket(PF_INET, SOCK_STREAM,
                        options->enable_mptcp ? IPPROTO_MPTCP, IPPROTO_TCP)) < 0)

Copy link
Owner Author

@arinc9 arinc9 Jun 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matttbe I don't know OpenVPN at all so I'm trying to figure out how I would pass the options structure to proxy_entry_new() where this code runs.

#else
if ((sd_server = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP)) < 0)
#endif
{
msg(M_WARN|M_ERRNO, "PORT SHARE PROXY: cannot create socket");
return false;
Expand Down
62 changes: 60 additions & 2 deletions src/openvpn/socket.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,12 @@

#include "memdbg.h"

#if defined(TARGET_LINUX) && defined(ENABLE_MPTCP)
arinc9 marked this conversation as resolved.
Show resolved Hide resolved
#ifndef IPPROTO_MPTCP
#define IPPROTO_MPTCP 262
#endif
#endif

/*
* Convert sockflags/getaddr_flags into getaddr_flags
*/
Expand Down Expand Up @@ -1082,6 +1088,39 @@ create_socket_udp(struct addrinfo *addrinfo, const unsigned int flags)
return sd;
}

#if defined(TARGET_LINUX) && defined(ENABLE_MPTCP)
socket_descriptor_t
create_socket_mptcp(struct addrinfo *addrinfo)
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not modifying create_socket_tcp instead of duplicating the code?

{
socket_descriptor_t sd;

ASSERT(addrinfo);
ASSERT(addrinfo->ai_socktype == SOCK_STREAM);
addrinfo->ai_protocol = IPPROTO_MPTCP;
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't check where this is used but I don't think you should set IPPROTO_MPTCP in addrinfo: probably best to only use this proto when calling socket().

if ((sd = socket(addrinfo->ai_family, addrinfo->ai_socktype, addrinfo->ai_protocol)) < 0)
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it might be interesting (in a second commit?) to implement a fallback: create a socket with IPPROTO_MPTCP and if it fails with a certain errno (EPROTONOSUPPORT), use a goto to recreate the socket without MPTCP (proto = addrinfo->ai_protocol), e.g.

    int proto;

    (...)

    proto = options->enable_mptcp ? IPPROTO_MPTCP, addrinfo->ai_protocol;

socket:
    if ((sd = socket(addrinfo->ai_family, addrinfo->ai_socktype, proto)) < 0)
    {
        /* Fallback to TCP */
        if (proto == IPPROTO_MPTCP && errno == EPROTONOSUPPORT)
        {
            msg(M_WARN, "TCP: MPTCP is not supported by the kernel, fallback to TCP");

            proto = addrinfo->ai_protocol;
            goto socket;
        }

(I didn't try the code and I guess you need to use something else than options->enable_mptcp)

@obonaventure maybe we should not check the errno: if net.mptcp.enabled is disabled (or other restrictions via SELinux and others), the errno will be different but still a fallback is possible.

We could check for other errno's but probably easier to just print a warning and retry in TCP.

    int proto;

    (...)

    proto = options->enable_mptcp ? IPPROTO_MPTCP, addrinfo->ai_protocol;

socket:
    if ((sd = socket(addrinfo->ai_family, addrinfo->ai_socktype, proto)) < 0)
    {
        /* Fallback to TCP */
        if (proto == IPPROTO_MPTCP)
        {
            msg(M_WARN, "TCP: MPTCP is not supported by the kernel, fallback to TCP");

            proto = addrinfo->ai_protocol;
            goto socket;
        }

{
msg(M_ERR, "Cannot create MPTCP socket");
}

{
int on = 1;
if (setsockopt(sd, SOL_SOCKET, SO_REUSEADDR,
(void *) &on, sizeof(on)) < 0)
{
msg(M_ERR, "TCP: Cannot setsockopt SO_REUSEADDR on TCP socket");
}
}

/* set socket file descriptor to not pass across execs, so that
* scripts don't have access to it */
set_cloexec(sd);

return sd;
}

#endif


static void
bind_local(struct link_socket *sock, const sa_family_t ai_family)
{
Expand Down Expand Up @@ -1125,6 +1164,21 @@ create_socket(struct link_socket *sock, struct addrinfo *addr)
}
else if (addr->ai_protocol == IPPROTO_TCP || addr->ai_socktype == SOCK_STREAM)
{
#if defined(TARGET_LINUX) && defined(ENABLE_MPTCP)
if(sock->info.multipath)
{
sock->sd = create_socket_mptcp(addr);
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see below, probably best to modify create_socket_tcp and modify the signature

// Multipath TCP could fail because it is not enabled on this host
// Try regular TCP
if(sock->sd == -1)
{

msg(M_NONFATAL, "Can't resolve MPTCP socket, fallback to TCP !");
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(just saw this after my previous comment. Good idea to fallback)

(PS: in English, you don't add a space before !)

sock->sd = create_socket_tcp(addr);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the socket is created when starting OpenVPN, right? (I mean: I guess the client will not request the server to create an MPTCP socket, right?)

In this case, it means that the person who is launching OpenVPN has explicitly requested to use MPTCP: then better not to fallback in case of error, but fail, no? If yes, then it might be better to adapt create_socket_tcp() to support MPTCP, e.g. by adding a new parameter, or by setting addr->ai_protocol = IPPROTO_MPTCP is MPTCP has been requested (but that might have other consequences if addr is used later on, to be confirmed)

In my ideal world, MPTCP would be enabled by default, and used only if the client requested it. But I don't know if OpenVPN devs will accept that.

(while at it, but not directly related to ↑, you might need to do something else for DCO, because they don't support MPTCP yet: OpenVPN/ovpn-dco#60 )

}
}
else
#endif
sock->sd = create_socket_tcp(addr);
}
else
Expand Down Expand Up @@ -1850,7 +1904,11 @@ link_socket_init_phase1(struct context *c, int mode)
sock->bind_local = o->ce.bind_local;
sock->resolve_retry_seconds = o->resolve_retry_seconds;
sock->mtu_discover_type = o->ce.mtu_discover_type;

arinc9 marked this conversation as resolved.
Show resolved Hide resolved

#if defined(TARGET_LINUX) && defined(ENABLE_MPTCP)
sock->info.multipath = o->enable_multipath;
#endif

#ifdef ENABLE_DEBUG
sock->gremlin = o->gremlin;
#endif
Expand Down Expand Up @@ -2210,7 +2268,7 @@ link_socket_init_phase2(struct context *c)
else
#endif
{
create_socket(sock, sock->info.lsa->current_remote);
create_socket(sock, sock->info.lsa->current_remote);
arinc9 marked this conversation as resolved.
Show resolved Hide resolved
}

}
Expand Down
7 changes: 7 additions & 0 deletions src/openvpn/socket.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,9 @@ struct link_socket_info
sa_family_t af; /* Address family like AF_INET, AF_INET6 or AF_UNSPEC*/
bool bind_ipv6_only;
int mtu_changed; /* Set to true when mtu value is changed */
#if defined(TARGET_LINUX) && defined(ENABLE_MPTCP)
arinc9 marked this conversation as resolved.
Show resolved Hide resolved
bool multipath;
arinc9 marked this conversation as resolved.
Show resolved Hide resolved
#endif
};

/*
Expand Down Expand Up @@ -469,6 +472,10 @@ bool ipv6_addr_safe(const char *ipv6_text_addr);

socket_descriptor_t create_socket_tcp(struct addrinfo *);

#ifdef ENABLE_MPTCP
socket_descriptor_t create_socket_mptcp(struct addrinfo *);
#endif

socket_descriptor_t socket_do_accept(socket_descriptor_t sd,
struct link_socket_actual *act,
const bool nowait);
Expand Down