Skip to content

Commit

Permalink
Initial support for Multipath TCP on recent Linux kernels
Browse files Browse the repository at this point in the history
  • Loading branch information
@obonaventure
obonaventure committed Sep 5, 2022
1 parent a5cf4cf commit a0ca7a0
Show file tree
Hide file tree
Showing 6 changed files with 148 additions and 2 deletions.
46 changes: 46 additions & 0 deletions configure.ac
View file
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,12 @@ AC_ARG_WITH(
[with_openssl_engine="auto"]
)

AC_ARG_WITH(mptcp,
[AS_HELP_STRING([--without-mptcp],[Disable Multipath TCP support])],

This comment has been minimized.

Sign in to view

Copy link
@matttbe

matttbe Sep 5, 2022

It looks like they prefer to use --disable-xxx, instead of --without-xxx.

Please add a space after the comma: mptcp], [Disable

This comment has been minimized.

Sign in to view

Copy link
@matttbe

matttbe Sep 5, 2022

you also need a @<:@default=yes@:>@ at the end
[enable_mptcp=no],

This comment has been minimized.

Sign in to view

Copy link
@matttbe

matttbe Sep 5, 2022

I think you can just keep the line with , (looking at what is done above)
[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 @@ -820,6 +826,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);
}
]]
) ],

This comment has been minimized.

Sign in to view

Copy link
@matttbe

matttbe Sep 5, 2022

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
[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
20 changes: 20 additions & 0 deletions src/openvpn/options.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,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)

This comment has been minimized.

Sign in to view

Copy link
@matttbe

matttbe Sep 5, 2022 •
edited
Loading

(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"

This comment has been minimized.

Sign in to view

Copy link
@matttbe

matttbe Sep 5, 2022

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)

This comment has been minimized.

Sign in to view

Copy link
@matttbe

matttbe Sep 5, 2022

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

(or maybe --proto-mptcp?)

This comment has been minimized.

Sign in to view

Copy link
@matttbe

matttbe Sep 5, 2022

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

This comment has been minimized.

Sign in to view

Copy link
@matttbe

matttbe Sep 5, 2022

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 @@ -903,6 +906,11 @@ init_options(struct options *o, const bool init_gc)
}
#endif /* _WIN32 */
o->allow_recursive_routing = false;

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

}

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

This comment has been minimized.

Sign in to view

Copy link
@matttbe

matttbe Sep 5, 2022

(mptcp?)
{
VERIFY_PERMISSION(OPT_P_GENERAL);

This comment has been minimized.

Sign in to view

Copy link
@matttbe

matttbe Sep 5, 2022

(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");

This comment has been minimized.

Sign in to view

Copy link
@matttbe

matttbe Sep 5, 2022

when looking above, it looks like you don't need such messages, e.g.

    else if (streq(p[0], "tls-server") && !p[1])
    {
        VERIFY_PERMISSION(OPT_P_GENERAL);
        options->tls_server = true;
    }
    else if (streq(p[0], "tls-client") && !p[1])
    {
        VERIFY_PERMISSION(OPT_P_GENERAL);
        options->tls_client = true;
    }

Just:

else if (streq(p[0], "mptcp"), && !p[1])
goto err;
}
options->enable_multipath = true;
}
#endif
else
{
int i;
Expand Down
3 changes: 3 additions & 0 deletions src/openvpn/options.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -448,6 +448,9 @@ struct options
#define SF_NO_PUSH_ROUTE_GATEWAY (1<<2)
unsigned int server_flags;

#ifdef ENABLE_MPTCP
bool enable_multipath;

This comment has been minimized.

Sign in to view

Copy link
@matttbe

matttbe Sep 5, 2022

I would recommend to use enable_mptcp to be consistent with the rest. Also multipath is too generic
#endif

This comment has been minimized.

Sign in to view

Copy link
@matttbe

matttbe Sep 5, 2022

(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
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,14 @@

#include "memdbg.h"


#if defined(TARGET_LINUX) && defined(ENABLE_MPTCP)

This comment has been minimized.

Sign in to view

Copy link
@matttbe

matttbe Sep 5, 2022

I don't think you need this: you can define IPPROTO_MPTCP even if you don't need it, we can avoid some if conditions
#ifndef IPPROTO_MPTCP

This comment has been minimized.

Sign in to view

Copy link
@matttbe

matttbe Sep 5, 2022

can you not do that in a header file once?
#define IPPROTO_MPTCP 262
#endif
#endif


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

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

/* connect to port share server */
#if defined(TARGET_LINUX) && defined(ENABLE_MPTCP)

This comment has been minimized.

Sign in to view

Copy link
@matttbe

matttbe Sep 5, 2022

here you only check if the binary supports the option but not if the client asks to use MPTCP
if ((sd_server = socket(PF_INET, SOCK_STREAM, IPPROTO_MPTCP)) < 0)

This comment has been minimized.

Sign in to view

Copy link
@matttbe

matttbe Sep 5, 2022

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)
#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
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,12 @@

#include "memdbg.h"

#if defined(TARGET_LINUX) && defined(ENABLE_MPTCP)

This comment has been minimized.

Sign in to view

Copy link
@matttbe

matttbe Sep 5, 2022 •
edited
Loading

(see a previous comment: probably best to move that in a .h file → socket.h? + remove the #if defined(...) line)
#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)

This comment has been minimized.

Sign in to view

Copy link
@matttbe

matttbe Sep 5, 2022

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;

This comment has been minimized.

Sign in to view

Copy link
@matttbe

matttbe Sep 5, 2022

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)

This comment has been minimized.

Sign in to view

Copy link
@matttbe

matttbe Sep 5, 2022

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;
        }

This comment has been minimized.

Sign in to view

Copy link
@matttbe

matttbe Sep 5, 2022

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

This comment has been minimized.

Sign in to view

Copy link
@matttbe

matttbe Sep 7, 2022

@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);

This comment has been minimized.

Sign in to view

Copy link
@matttbe

matttbe Sep 5, 2022

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 !");

This comment has been minimized.

Sign in to view

Copy link
@matttbe

matttbe Sep 5, 2022

(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);
}
}
else
#endif
sock->sd = create_socket_tcp(addr);
}
else
Expand Down Expand Up @@ -1849,7 +1903,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;

This comment has been minimized.

Sign in to view

Copy link
@matttbe

matttbe Sep 5, 2022

strange this modification here (but maybe Github doesn't tell me if you remove/left some spaces?)

#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 @@ -2208,7 +2266,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);

This comment has been minimized.

Sign in to view

Copy link
@matttbe

matttbe Sep 5, 2022

please undo this
}

}
Expand Down
7 changes: 7 additions & 0 deletions src/openvpn/socket.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,9 @@ struct link_socket_info
bool bind_ipv6_only;
int mtu_changed; /* Set to true when mtu value is changed */
bool dco_installed;
#if defined(TARGET_LINUX) && defined(ENABLE_MPTCP)

This comment has been minimized.

Sign in to view

Copy link
@matttbe

matttbe Sep 5, 2022 •
edited
Loading

I would avoid the ifdef here, no? It is probably fine to keep it declared and avoid ifdef elsewhere in the code, just make sure it is never set to true when this is not supported
bool multipath;

This comment has been minimized.

Sign in to view

Copy link
@matttbe

matttbe Sep 5, 2022

same here: probably best with mptcp, no?
#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

7 comments on commit a0ca7a0

@matttbe
Copy link

@matttbe matttbe commented on a0ca7a0 Sep 5, 2022

Choose a reason for hiding this comment

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

@obonaventure : regarding the commit message, it is often very important to document a maximum of things there: why you are doing that, what is MPTCP, why you did it like that, questions you had, possible alternatives, etc. Without this useful description, patches might simply not be reviewed if it is the reviewer that has to guess all of that.

Also, regarding the other commits in the project, it might be useful to add a prefix (option:, socket:?)

And a Signed-off-by tag.

@matttbe
Copy link

@matttbe matttbe commented on a0ca7a0 Sep 5, 2022

Choose a reason for hiding this comment

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

also, I don't know if they have unit tests, changelog and doc to update as well

@arinc9
Copy link

@arinc9 arinc9 commented on a0ca7a0 Jun 19, 2024

Choose a reason for hiding this comment

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

@matttbe @obonaventure what's the status of this effort? MPTCP support in OpenVPN would be extremely useful.

@matttbe
Copy link

Choose a reason for hiding this comment

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

@arinc9 I agree that it would be useful, but it looks like we need someone to continue the work from where it was here.
Do you think you could apply the review, rebase on top of the latest version of OpenVPN, run the tests, and suggest that to the OpenVPN devs?

@arinc9
Copy link

@arinc9 arinc9 commented on a0ca7a0 Jun 19, 2024

Choose a reason for hiding this comment

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

I'll see what I can do.

@arinc9
Copy link

@arinc9 arinc9 commented on a0ca7a0 Jun 19, 2024

Choose a reason for hiding this comment

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

I've rebased this patch to the master branch and replicated your review here arinc9#1. I will start addressing them.

@matttbe
Copy link

Choose a reason for hiding this comment

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

I've rebased this patch to the master branch and replicated your review here arinc9#1. I will start addressing them.

Thanks!

Do not hesitate to ping me if you need a review! (I don't know the OpenVPN project, and I don't remember what I said here in review, but I can see what I can do).

Also, don't miss these comments from here above:

regarding the commit message, it is often very important to document a maximum of things there: why you are doing that, what is MPTCP, why you did it like that, questions you had, possible alternatives, etc. Without this useful description, patches might simply not be reviewed if it is the reviewer that has to guess all of that.

Also, regarding the other commits in the project, it might be useful to add a prefix (option:, socket:?)

And a Signed-off-by tag.

also, I don't know if they have unit tests, changelog and doc to update as well

For the commit message, don't hesitate to get some inspiration from recent changes adding native MPTCP support in apps, e.g. curl/curl@ab6d544 and systemd/systemd@3f69070

Please sign in to comment.