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
42 changes: 7 additions & 35 deletions configure.ac
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,10 +308,11 @@ AC_ARG_WITH(
[with_openssl_engine="auto"]
)

AC_ARG_WITH(mptcp,
[AS_HELP_STRING([--without-mptcp],[Disable Multipath TCP support])],
[enable_mptcp=no],
[enable_mptcp=yes]
AC_ARG_WITH(
Copy link

Choose a reason for hiding this comment

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

see my other comment: AC_ARG_ENABLE I suppose

[mptcp],
[AS_HELP_STRING([--disable-mptcp], [Disable Multipath TCP support @<:@default=yes@:>@])],
,
[enable_mptcp="yes"]
Copy link

Choose a reason for hiding this comment

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

see my other comment, here it should only be enabled by default on Linux

)

AC_ARG_VAR([PLUGINDIR], [Path of plug-in directory @<:@default=LIBDIR/openvpn/plugins@:>@])
Expand Down Expand Up @@ -887,40 +888,11 @@ esac


dnl
dnl Checking Multipath TCP support on Linux
dnl Enable 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);
}
]]
) ],
[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]) ],
)
])
AC_DEFINE(ENABLE_MPTCP, 1, [Enable checking Multipath TCP support])
Copy link

Choose a reason for hiding this comment

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

see my other comment, it should not be done here, but above, in the AC_ARG

;;
esac

Expand Down
19 changes: 7 additions & 12 deletions src/openvpn/options.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,8 +135,8 @@ 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)
"--multipath : Enable Multipath TCP on the TCP connections.\n"
#ifdef ENABLE_MPTCP
"--mptcp : Enable Multipath TCP on the TCP connections.\n"
#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"
Expand Down Expand Up @@ -909,8 +909,8 @@ init_options(struct options *o, const bool init_gc)
o->tuntap_options.disable_dco = true;
#endif /* ENABLE_DCO */

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

Expand Down Expand Up @@ -9519,16 +9519,11 @@ add_option(struct options *options,
goto err;
}
}
#if defined(TARGET_LINUX) && defined(ENABLE_MPTCP)
else if (streq(p[0], "multipath"))
#ifdef ENABLE_MPTCP
else if (streq(p[0], "mptcp"), && !p[1])
{
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");
goto err;
}
options->enable_multipath = true;
options->enable_mptcp = true;
}
#endif
else
Expand Down
2 changes: 1 addition & 1 deletion src/openvpn/options.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,7 +461,7 @@ struct options
unsigned int server_flags;

#ifdef ENABLE_MPTCP
bool enable_multipath;
bool enable_mptcp;
#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;

Expand Down
10 changes: 3 additions & 7 deletions src/openvpn/ps.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,9 @@
#include "memdbg.h"


#if defined(TARGET_LINUX) && defined(ENABLE_MPTCP)
#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 */
Expand Down Expand Up @@ -433,11 +431,9 @@ proxy_entry_new(struct proxy_connection **list,
struct proxy_connection *cp;

/* connect to port share server */
#if defined(TARGET_LINUX) && defined(ENABLE_MPTCP)
if ((sd_server = socket(PF_INET, SOCK_STREAM, IPPROTO_MPTCP)) < 0)
#else
if ((sd_server = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP)) < 0)
#endif
/* TODO: provide access to the options structure */
Copy link

Choose a reason for hiding this comment

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

Sorry, I don't think I can help with that. Maybe this option has to be duplicated in a different structure here (similar to sock->info.mptcp = o->enable_mptcp; for socket.c), or passed by the caller.

But before doing that, do you know when this function is used? Is it used when connecting to OpenVPN server via an HTTP proxy? Maybe MPTCP support for this can be added later to that, no?

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

#include "memdbg.h"

#if defined(TARGET_LINUX) && defined(ENABLE_MPTCP)
#ifndef IPPROTO_MPTCP
#define IPPROTO_MPTCP 262
#endif
#endif

/*
* Convert sockflags/getaddr_flags into getaddr_flags
*/
Expand Down Expand Up @@ -1165,15 +1159,15 @@ 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)
if(sock->info.mptcp)
{
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 !");
msg(M_NONFATAL, "Can't resolve MPTCP socket, fallback to TCP!");
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 )

}
}
Expand Down Expand Up @@ -1904,11 +1898,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;

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

#ifdef ENABLE_DEBUG
sock->gremlin = o->gremlin;
#endif
Expand Down Expand Up @@ -2268,7 +2262,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);
}

}
Expand Down
8 changes: 5 additions & 3 deletions src/openvpn/socket.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,10 @@
*/
#define RESOLV_RETRY_INFINITE 1000000000

#ifndef IPPROTO_MPTCP
#define IPPROTO_MPTCP 262
#endif

/*
* packet_size_type is used to communicate packet size
* over the wire when stream oriented protocols are
Expand Down Expand Up @@ -121,9 +125,7 @@ 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)
bool multipath;
#endif
bool mptcp;
};

/*
Expand Down