Re: [Openvpn-devel] [PATCH 1/5] src/openvpn/init.c: handle strdup failures
On 08/07/2024 23:53, Илья Шипицин wrote: пн, 8 июл. 2024 г. в 23:47, Antonio Quartulli : Hi, On 08/07/2024 23:44, Илья Шипицин wrote: +msg( M_FATAL, "Failed allocate memory saved_pid_file_name"); patchset looks great, but (!!) there should be no space after the opening parenthesis.. I just followed code style already present in repo $ grep -r 'msg( M_FATAL' . grep: ./.git/objects/pack/pack-d09f3eabac42224b1dc52ffb4f53d9af23bc5b7d.pack: binary file matches ./src/openvpn/auth_token.c:msg( M_FATAL, "Failed to get enough randomness for " ./src/openvpn/tun.c:msg( M_FATAL, "init_tun: problem converting IPv6 ifconfig addresses %s and %s to binary", ifconfig_ipv6_local_parm, ifconfig_ipv6_remote_parm ); ./src/openvpn/tun.c:msg( M_FATAL, "cannot find unused tap device" ); ./src/openvpn/tun.c:msg( M_FATAL, "TAP device name must be '--dev tap'" ); Unfortunately those are unlucky leftovers that haven't been fixed yet: $ grep -r 'msg(M_FATAL' . |wc -l 286 $ grep -r 'msg( M_FATAL' . |wc -l 4 also, uncrustify GHA jobs agreed that it is no formatting violation doubly unfortunate as I think uncrustify can't force this or it gets confused somehow :( if those spaces makes you unhappy, I'm fine if you fix them during commit apply I don't think Gert like to change code lines while committing a patch (We can wait for him to say something) However, I can also offer myself to send a patch with a full cleanup afterwards. Cheers, -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 1/5] src/openvpn/init.c: handle strdup failures
Hi, On 08/07/2024 23:44, Илья Шипицин wrote: +msg( M_FATAL, "Failed allocate memory saved_pid_file_name"); patchset looks great, but (!!) there should be no space after the opening parenthesis.. I just followed code style already present in repo $ grep -r 'msg( M_FATAL' . grep: ./.git/objects/pack/pack-d09f3eabac42224b1dc52ffb4f53d9af23bc5b7d.pack: binary file matches ./src/openvpn/auth_token.c:msg( M_FATAL, "Failed to get enough randomness for " ./src/openvpn/tun.c:msg( M_FATAL, "init_tun: problem converting IPv6 ifconfig addresses %s and %s to binary", ifconfig_ipv6_local_parm, ifconfig_ipv6_remote_parm ); ./src/openvpn/tun.c:msg( M_FATAL, "cannot find unused tap device" ); ./src/openvpn/tun.c:msg( M_FATAL, "TAP device name must be '--dev tap'" ); Unfortunately those are unlucky leftovers that haven't been fixed yet: $ grep -r 'msg(M_FATAL' . |wc -l 286 $ grep -r 'msg( M_FATAL' . |wc -l 4 also, uncrustify GHA jobs agreed that it is no formatting violation doubly unfortunate as I think uncrustify can't force this or it gets confused somehow :( Cheers, -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 1/5] src/openvpn/init.c: handle strdup failures
Hi, On 08/07/2024 23:08, Ilia Shipitsin wrote: Signed-off-by: Ilia Shipitsin --- src/openvpn/init.c | 4 1 file changed, 4 insertions(+) diff --git a/src/openvpn/init.c b/src/openvpn/init.c index a49e5639..59205ba6 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -4967,6 +4967,10 @@ write_pid_file(const char *filename, const char *chroot_dir) if (!chroot_dir) { saved_pid_file_name = strdup(filename); +if (!saved_pid_file_name) +{ +msg( M_FATAL, "Failed allocate memory saved_pid_file_name"); patchset looks great, but (!!) there should be no space after the opening parenthesis.. Cheers, +} } } } -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2] Document that auth-user-pass may be inlined
Hi, On 20/02/2024 18:52, selva.n...@gmail.com wrote: From: Selva Nair Commits 7d48d31b, 39619b7f added support for inlining username and, optionally, password. Add a description of its usage in the man page. Github: resolves OpenVPN/openvpn#370 Change-Id: I7a1765661f7676eeba8016024080fd1026220ced Signed-off-by: Selva Nair Acked-by: Antonio Quartulli --- v2: Add '--' prefix when referring to auth-user-pass and mention related github issue doc/man-sections/client-options.rst | 11 +++ doc/man-sections/inline-files.rst | 2 +- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/doc/man-sections/client-options.rst b/doc/man-sections/client-options.rst index b92b1a46..b75fe5bd 100644 --- a/doc/man-sections/client-options.rst +++ b/doc/man-sections/client-options.rst @@ -73,6 +73,17 @@ configuration. If ``up`` is omitted, username/password will be prompted from the console. + This option can also be inlined + :: + + +username +[password] + + + where password is optional, and will be prompted from the console if + missing. + The server configuration must specify an ``--auth-user-pass-verify`` script to verify the username/password provided by the client. diff --git a/doc/man-sections/inline-files.rst b/doc/man-sections/inline-files.rst index f46301e8..4dba73c9 100644 --- a/doc/man-sections/inline-files.rst +++ b/doc/man-sections/inline-files.rst @@ -5,7 +5,7 @@ OpenVPN allows including files in the main configuration for the ``--ca``, ``--cert``, ``--dh``, ``--extra-certs``, ``--key``, ``--pkcs12``, ``--crl-verify``, ``--http-proxy-user-pass``, ``--tls-auth``, ``--auth-gen-token-secret``, ``--peer-fingerprint``, ``--tls-crypt``, -``--tls-crypt-v2`` and ``--verify-hash`` options. +``--tls-crypt-v2``, ``--verify-hash`` and ``--auth-user-pass`` options. Each inline file started by the line and ended by the line -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Document that auth-user-pass may be inlined
Hi, On 19/02/2024 20:28, selva.n...@gmail.com wrote: From: Selva Nair Commits 7d48d31b, 39619b7f added support for inlining username and, optionally, password. Add a description of its usage in the man page. Change-Id: I7a1765661f7676eeba8016024080fd1026220ced Signed-off-by: Selva Nair Acked-by: Antonio Quartulli --- Does this have to go through gerrit? doc/man-sections/client-options.rst | 11 +++ doc/man-sections/inline-files.rst | 2 +- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/doc/man-sections/client-options.rst b/doc/man-sections/client-options.rst index b92b1a46..b75fe5bd 100644 --- a/doc/man-sections/client-options.rst +++ b/doc/man-sections/client-options.rst @@ -73,6 +73,17 @@ configuration. If ``up`` is omitted, username/password will be prompted from the console. + This option can also be inlined + :: + + +username +[password] + + + where password is optional, and will be prompted from the console if + missing. + The server configuration must specify an ``--auth-user-pass-verify`` script to verify the username/password provided by the client. diff --git a/doc/man-sections/inline-files.rst b/doc/man-sections/inline-files.rst index f46301e8..ad02c855 100644 --- a/doc/man-sections/inline-files.rst +++ b/doc/man-sections/inline-files.rst @@ -5,7 +5,7 @@ OpenVPN allows including files in the main configuration for the ``--ca``, ``--cert``, ``--dh``, ``--extra-certs``, ``--key``, ``--pkcs12``, ``--crl-verify``, ``--http-proxy-user-pass``, ``--tls-auth``, ``--auth-gen-token-secret``, ``--peer-fingerprint``, ``--tls-crypt``, -``--tls-crypt-v2`` and ``--verify-hash`` options. +``--tls-crypt-v2``, ``--verify-hash`` and ``auth-user-pass`` options. Each inline file started by the line and ended by the line -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [S] Change in openvpn[master]: Minor fix to process_ip_header
Hi, On 19/02/2024 14:12, Gert Doering wrote: Maybe that would be a more reasonable approach here... get rid of the umbrella if(), and check individual bits inside. It seems to be a micro-optimization "skip this branch if we have no single feature active", while at least MSSFIX is active by default. Technically we still retain the "speed up" when all features are disabled (i.e. remove this and this from the config and gain some Mbps) But I do agree that the code complexity is not worth the gain, for a very narrow corner case. I vote for letting the umbrella if() go. Cheers, -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [S] Change in openvpn[master]: Minor fix to process_ip_header
Hi, On 16/02/2024 15:00, Antonio Quartulli wrote: Hi, On 15/02/2024 17:17, Gert Doering wrote: Hi, On Thu, Feb 15, 2024 at 03:59:02PM +, its_Giaan (Code Review) wrote: if (buf->len > 0) { - /* - * The --passtos and --mssfix options require - * us to examine the IPv4 header. - */ - - if (flags & (PIP_MSSFIX -#if PASSTOS_CAPABILITY - | PIPV4_PASSTOS -#endif - | PIPV4_CLIENT_NAT - )) + if (flags & PIP_OPT_MASK) NAK, as this is not the same thing. PIP_OPT_MASK will also match on the IPv6 flags, which are not something we need to test for here (= if only an IPv6 flag is active, why should we enter this branch?). We need to enter for either v4 or v6 flags, no? The check on whether the packet is v4 or v6 happens *inside* this if block. Am I wrong? I double checked the code once again and I think there is a bug in this check. Right now we check for some flags (say A and B) before entering the if-block. Once inside we take action on flag A, B, but also C, D and E. Now, if C, D and E are set, but A and B are not, we will never enter the if-block and execute the related code. Among this we have: * IPv6 flags (they are checked inside, but not on the outer guard) * PIPV4_EXTRACT_DHCP_ROUTER * PIPV6_IMCP_NOHOST_CLIENT * PIPV6_IMCP_NOHOST_SERVER (we also got an interesting typ0 in the last two). This said, I do believe this patch fixes these issues in one go as the new PIP_OPT_MASK will match all the flags mentioned above. It'd be interesting to test any behaviour expected to be triggered by PIPV4_EXTRACT_DHCP_ROUTER to confirm the theory. Cheers, -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [S] Change in openvpn[master]: Minor fix to process_ip_header
Hi, On 15/02/2024 17:17, Gert Doering wrote: Hi, On Thu, Feb 15, 2024 at 03:59:02PM +, its_Giaan (Code Review) wrote: if (buf->len > 0) { -/* - * The --passtos and --mssfix options require - * us to examine the IPv4 header. - */ - -if (flags & (PIP_MSSFIX -#if PASSTOS_CAPABILITY - | PIPV4_PASSTOS -#endif - | PIPV4_CLIENT_NAT - )) +if (flags & PIP_OPT_MASK) NAK, as this is not the same thing. PIP_OPT_MASK will also match on the IPv6 flags, which are not something we need to test for here (= if only an IPv6 flag is active, why should we enter this branch?). We need to enter for either v4 or v6 flags, no? The check on whether the packet is v4 or v6 happens *inside* this if block. Am I wrong? Cheers, -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [ovpn-dco] Can ovpn-dco use all cpu cores?
Hi, On 29/01/2024 05:25, Tony He wrote: Hi Antonio, I'm using ovpn-dco which is backported to v4.14 based on your latest code. My topology is: LAN PC -- openwrt router running openvpn server -- WAN PC running openvpn client Router is with two mips64 cores. I use the iperf3 to test speed between LAN PC and WAN PC. The result is sometime the performance is good(~280Mbps) while sometimes is bad(~140Mbps). When the performance is bad, one of two CPUs is 100% idle. When the performance is good, two CPUs are busy. However, I don't see the issue when ipsec is tested in the same test env. Two cores are always used for ipsec. So , can ovpn-dco use all cpu cores to get max performance? ovpn-dco uses more than core in order to perform different operations, but more parallelism on traffic processing can definitely be implemented (patches are welcome ;)). Now it's hard to tell if what you are seeing is the result of this implementation detail or something else, especially because in some cases you get higher throughput. Cheers, -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [Openvpn-users] OpenVPN and outside clients
Hi, On 03/01/2024 09:14, Peter Davis wrote: Hello, I changed the IP address in the client configuration file, but I can't connect to the server. I got the following error: Wed Jan 3 10:32:32 2024 TLS Error: TLS key negotiation failed to occur within 60 seconds (check your network connectivity) Wed Jan 3 10:32:32 2024 TLS Error: TLS handshake failed I checked the OpenVPN log file too, but there was nothing there: # cat /var/log/openvpn/openvpn.log This is a clear indication that your connection attempt is not reaching the VPN server at all. This is a problem with whatever is in the middle, i.e. gateway/firewall. * Have you properly configured the port forwarding on the gateway/firewall? * Have you allowed the right ports on the gateway/firewall? * Is there any firewall on the VPN server which may be preventing connections from outside the LAN? Note: this is unrelated to OpenVPN, but just a generic network configuration issue. Regards, -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v3] Add missing check for nl_socket_alloc failure
Hi, On 21/11/2023 18:06, Arne Schwabe wrote: This can happen if the memory alloc fails. Patch V2: add goto error Patch V3: return -ENOMEM instead of going to error Change-Id: Iee66caa794d267ac5f8bee584633352893047171 Signed-off-by: Arne Schwabe Acked-by: Antonio Quartulli --- src/openvpn/dco_linux.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c index b033f8543..3c91606b7 100644 --- a/src/openvpn/dco_linux.c +++ b/src/openvpn/dco_linux.c @@ -81,6 +81,12 @@ resolve_ovpn_netlink_id(int msglevel) int ret; struct nl_sock *nl_sock = nl_socket_alloc(); +if (!nl_sock) +{ +msg(msglevel, "Allocating net link socket failed"); +return -ENOMEM; +} + ret = genl_connect(nl_sock); if (ret) { -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] OpenVPN3 thread safety
Hi, On 20/11/2023 10:11, Savely Krasovsky wrote: Thanks for the reply! After few days I have found a major bug: a close and free client before stop() successfully executes. After fixing this I didn't get any segfaults anymore. Program survives very extensive tests. But after your reply I am in question again... Sure, openvpn3 has single thread after Connect(), but Go still could call Stop() or any other method from another thread. And still I didn't get any issues at the moment. Should I rewrite it anyway? Calling from different threads is not an issue. Invoking multiple methods at the same time is. If you call openvpn3 code from multiple threads, it may be better for you to implement your own locking system "outside" of the library which makes sure no more than one caller is running openvpn3 code at a given moment. Cheers, Пн, 20.11.2023 в 9:55 Arne Schwabe писал(а): Am 12.11.2023 um 14:16 schrieb Savely Krasovsky: Hello! I am trying to use OpenVPN3 with Golang SWIG binding. It works pretty nice, but I have random segmentation faults without obvious reason. My current guess is that Golang calls OpenVPN3 from various threads and library is not ready for that sometime. Am I right? Documentation doesn't say a lot about threading. If this is a case, I have two possible solutions: 1. Use OPENVPN_ENABLE_BIGMUTEX build flag (I am not sure I need that). 2. Move all calls to OpenVPN3 into single goroutine and use runtime.LockOSThread(). Could you please clarify, should I get segfaults in that case or not and the problems is unrelated. Yes. The client is singlethreaded and expects to be called only from a single thread. You can have multiple clients in multiple threads but all our own software uses just a single thread for the client itself. Arne -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] man: extend description for "dhcp-option DNS" on Windows
Hi, On 05/10/2023 21:03, Selva Nair wrote: [cut] Thanks fr the feedback on the context of the patch. That said, I'm not convinced this is the right way to document this. IMO, using "--dhcp-option DNS n.n.n" to set a DNS server not reachable through the tunnel is a misuse of that option on any platform. Even on "non-windows" one shouldn't do that as it's very reasonable for a script that handles it to set it on the interface, instead of globally, if the platform permits it. For example, using systemd-resolved on Linux (though I'm not totally sure how exactly interface-specific DNS works in this case). So, if at all, why not state that the DNS server specified here should be reachable through the tunnel irrespective of the platform? Well I simply want to document the current behaviour (we spent a few hours debugging this situation because the documentation wasn't clear enough). On Linux at the moment you can pass any DNS and, like you said, the user script will do something with it. On Windows, this is not the case and I simply want to note this down. If you think we should accept only VPN-reachable IPs (which may make sense, although some people do crazy things and may not like it), then we should change the code in order to reject anything that cannot be routed through the VPN. But so far OpenVPN has always left the responsibility to the user/admin to make sure that all pieces will fit together. So instead of forcing any semantic, I think we should simply document what the code does. Cheers, Regards, Selva -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] man: extend description for "dhcp-option DNS" on Windows
From: Antonio Quartulli Add an important detail about the DNS configured via this option to be an "interface-specific" DNS. This detail is important when troubleshooting DNS issues since this logic will bypass the routing table. Signed-off-by: Antonio Quartulli --- doc/man-sections/vpn-network-options.rst | 6 ++ 1 file changed, 6 insertions(+) diff --git a/doc/man-sections/vpn-network-options.rst b/doc/man-sections/vpn-network-options.rst index b7f33acd..17c7d4a2 100644 --- a/doc/man-sections/vpn-network-options.rst +++ b/doc/man-sections/vpn-network-options.rst @@ -158,6 +158,12 @@ routing. Set primary domain name server IPv4 or IPv6 address. Repeat this option to set secondary DNS server addresses. +On Windows this option sets an "interface-specific" DNS, meaning +that the system will try to reach it via the VPN interface, no +matter what the routing table says. If the wanted DNS is not +expected to be reached via VPN, please don't use this option, but +rather set the DNS manually (or via DHCP). + Note: DNS IPv6 servers are currently set using netsh (the existing DHCP code can only do IPv4 DHCP, and that protocol only permits IPv4 addresses anywhere). The option will be put into the -- 2.41.0 ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] configure: disable engines if OPENSSL_NO_ENGINE is defined
Hi, On 03/09/2023 18:55, orbea wrote: Here is a patch that preserves the version check and adds a second check for OPENSSL_NO_ENGINE which seems to also be useful for BoringSSL. I prefer this version, but I'll let other chime in. On top of that I think we may need a proper v2 PATCH having no extra body, as I am not sure your message can be properly parsed by git-am. Cheers, From d6700ec0f5af2522bb4eb136d3760f5b1445c9d1 Mon Sep 17 00:00:00 2001 From: orbea Date: Sat, 2 Sep 2023 23:06:22 -0700 Subject: [PATCH] configure: disable engines if OPENSSL_NO_ENGINE is defined Starting with LibreSSL 3.8.1 the engines have been removed which causes the OpenVPN build to fail. This can be solved during configure by checking if OPENSSL_NO_ENGINE is defined in opensslconf.h. --- configure.ac | 8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index 2f65cbd5..1adfb9d4 100644 --- a/configure.ac +++ b/configure.ac @@ -927,11 +927,17 @@ if test "${with_crypto_library}" = "openssl"; then [AC_LANG_PROGRAM( [[ #include + #include ]], [[ /* Version encoding: MNNFFPPS - see opensslv.h for details */ #if OPENSSL_VERSION_NUMBER >= 0x3000L - #error Engine supported disabled by default in OpenSSL 3.0+ + #error Engine support disabled by default in OpenSSL 3.0+ + #endif + + /* BoringSSL and LibreSSL >= 3.8.1 removed engine support */ + #ifdef OPENSSL_NO_ENGINE + #error Engine support disabled by default in openssl/opensslconf.h #endif ]] )], -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] configure: disable engines if OPENSSL_NO_ENGINE is defined
Hi, On 03/09/2023 16:29, or...@riseup.net wrote: From: orbea Starting with LibreSSL 3.8.1 the engines have been removed which causes the OpenVPN build to fail. This can be solved during configure by checking if OPENSSL_NO_ENGINE is defined in opensslconf.h. --- configure.ac | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index 2f65cbd5..b5a835dc 100644 --- a/configure.ac +++ b/configure.ac @@ -926,11 +926,12 @@ if test "${with_crypto_library}" = "openssl"; then AC_COMPILE_IFELSE( [AC_LANG_PROGRAM( [[ + #include #include ]], [[ /* Version encoding: MNNFFPPS - see opensslv.h for details */ - #if OPENSSL_VERSION_NUMBER >= 0x3000L + #if OPENSSL_VERSION_NUMBER >= 0x3000L || defined(OPENSSL_NO_ENGINE) #error Engine supported disabled by default in OpenSSL 3.0+ Maybe the message should be changed now? Or we could have an entirely different message for this case? Cheers, #endif ]] -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Implement server_poll_timeout for socks
out, struct signal_info *sig_info) { char buf[270]; size_t len; -if (!socks_handshake(p, sd, &sig_info->signal_received)) +if (!socks_handshake(p, sd, server_poll_timeout, &sig_info->signal_received)) { goto error; } @@ -494,7 +496,7 @@ establish_socks_proxy_passthru(struct socks_proxy_info *p, /* receive reply from Socks proxy and discard */ -if (!recv_socks_reply(sd, NULL, &sig_info->signal_received)) +if (!recv_socks_reply(sd, NULL, server_poll_timeout, &sig_info->signal_received)) { goto error; } @@ -512,9 +514,10 @@ establish_socks_proxy_udpassoc(struct socks_proxy_info *p, socket_descriptor_t ctrl_sd, /* already open to proxy */ socket_descriptor_t udp_sd, struct openvpn_sockaddr *relay_addr, + struct event_timeout *server_poll_timeout, struct signal_info *sig_info) { -if (!socks_handshake(p, ctrl_sd, &sig_info->signal_received)) +if (!socks_handshake(p, ctrl_sd, server_poll_timeout, &sig_info->signal_received)) { goto error; } @@ -535,7 +538,7 @@ establish_socks_proxy_udpassoc(struct socks_proxy_info *p, /* receive reply from Socks proxy */ CLEAR(*relay_addr); -if (!recv_socks_reply(ctrl_sd, relay_addr, &sig_info->signal_received)) +if (!recv_socks_reply(ctrl_sd, relay_addr, server_poll_timeout, &sig_info->signal_received)) { goto error; } diff --git a/src/openvpn/socks.h b/src/openvpn/socks.h index 3a89245b..a7094f06 100644 --- a/src/openvpn/socks.h +++ b/src/openvpn/socks.h @@ -52,12 +52,14 @@ void establish_socks_proxy_passthru(struct socks_proxy_info *p, socket_descriptor_t sd, /* already open to proxy */ const char *host,/* openvpn server remote */ const char *servname, /* openvpn server port */ +struct event_timeout *server_poll_timeout, struct signal_info *sig_info); void establish_socks_proxy_udpassoc(struct socks_proxy_info *p, socket_descriptor_t ctrl_sd, /* already open to proxy */ socket_descriptor_t udp_sd, struct openvpn_sockaddr *relay_addr, + struct event_timeout *server_poll_timeout, struct signal_info *sig_info); void socks_process_incoming_udp(struct buffer *buf, -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] dco: fix crash when --multihome is used with --proto tcp
Although it's a combination of options that is not really useful, when specifying --multihome along with --proto tcp and DCO is enabled, OpenVPN will crash while attempting to access c2.link_socket_actual (NULL for the TCP case) in order to retrieve the local address (in function dco_multi_get_localaddr()) Prevent crash by running this code only if proto is UDP. The same check is already performed in socket.c/h for the non-DCO case. Fixes: https://github.com/OpenVPN/openvpn/issues/390 Change-Id: I61adc26ce2ff737e020c3d980902a46758cb23e5 Signed-off-by: Antonio Quartulli --- src/openvpn/dco.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/openvpn/dco.c b/src/openvpn/dco.c index 7c7eaac4..cd3e0ad3 100644 --- a/src/openvpn/dco.c +++ b/src/openvpn/dco.c @@ -509,7 +509,7 @@ dco_multi_get_localaddr(struct multi_context *m, struct multi_instance *mi, #if ENABLE_IP_PKTINFO struct context *c = &mi->context; -if (!(c->options.sockflags & SF_USE_IP_PKTINFO)) +if (!proto_is_udp(c->c2.link_socket->info.proto) || !(c->options.sockflags & SF_USE_IP_PKTINFO)) { return false; } -- 2.41.0 ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] configure.ac: fix typ0 in LIBCAPNG_CFALGS
Reported-by: Matt Whitlock Change-Id: Ic473fbc447741e54a9aac83c70bc4e6d87d91080 Signed-off-by: Antonio Quartulli --- configure.ac | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index 5ab1d0df..2f65cbd5 100644 --- a/configure.ac +++ b/configure.ac @@ -869,7 +869,7 @@ case "$host" in ) AC_CHECK_HEADER([sys/prctl.h],,[AC_MSG_ERROR([sys/prctl.h not found!])]) - CFLAGS="${CFLAGS} ${LIBCAPNG_CFALGS}" + CFLAGS="${CFLAGS} ${LIBCAPNG_CFLAGS}" LIBS="${LIBS} ${LIBCAPNG_LIBS}" AC_DEFINE(HAVE_LIBCAPNG, 1, [Enable libcap-ng support]) ;; -- 2.41.0 ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] dco-linux: fix counter print format
Hi, On 26/06/2023 15:09, Sergey Korolev via Openvpn-devel wrote: Avoid compilation warnings on 32 bit platforms. dco_linux.c: In function 'dco_update_peer_stat': dco_linux.c:830:26: error: format '%lu' expects argument of type 'long unsigned int', but argument 4 has type 'counter_type' {aka 'long long unsigned int'} [-Werror=format=] 830 | msg(D_DCO_DEBUG, "%s / dco_read_bytes: %lu", __func__, | ^~ 831 | c2->dco_read_bytes); | ~~ | | | counter_type {aka long long unsigned int} Signed-off-by: Sergey Korolev Thanks for catching this! Acked-by: Antonio Quartulli -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Fix use-after-free with EVP_CIPHER_free
Hi, On 01/06/2023 11:57, Arne Schwabe wrote: In many scenerios the context will still have a reference to the cipher, so scenerios -> scenarios this use-after-free does not explode but it is still wrong. Good catch - glad we're so lucky :-) Change-Id: I59002d6613eaef36d5a47b20b56073e399cfa1df Signed-off-by: Arne Schwabe --- src/openvpn/crypto_openssl.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c index c2ac80b74..8fe56fc78 100644 --- a/src/openvpn/crypto_openssl.c +++ b/src/openvpn/crypto_openssl.c @@ -839,11 +839,12 @@ cipher_ctx_init(EVP_CIPHER_CTX *ctx, const uint8_t *key, crypto_msg(M_FATAL, "EVP cipher init #2"); } -EVP_CIPHER_free(kt); /* make sure we used a big enough key */ ASSERT(EVP_CIPHER_CTX_key_length(ctx) <= EVP_CIPHER_key_length(kt)); +EVP_CIPHER_free(kt); } + This is not required - please remove it before merging. int cipher_ctx_iv_length(const EVP_CIPHER_CTX *ctx) { Acked-by: Antonio Quartulli -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] src/openvpn/dco_freebsd.c: handle malloc failure
Hi, On 17/05/2023 23:10, Kristof Provost wrote: On 17 May 2023, at 17:06, Илья Шипицин wrote: ср, 17 мая 2023 г. в 23:04, Kristof Provost : On 17 May 2023, at 16:58, Илья Шипицин wrote: ср, 17 мая 2023 г. в 22:43, Kristof Provost : On 17 May 2023, at 16:01, Ilya Shipitsin wrote: malloc was not checked against NULL, I was able to get core dump in case of failure Signed-off-by: Ilya Shipitsin --- src/openvpn/dco_freebsd.c | 5 + 1 file changed, 5 insertions(+) diff --git a/src/openvpn/dco_freebsd.c b/src/openvpn/dco_freebsd.c index abeb..adbd1120 100644 --- a/src/openvpn/dco_freebsd.c +++ b/src/openvpn/dco_freebsd.c @@ -594,6 +594,11 @@ dco_available(int msglevel) } buf = malloc(ifcr.ifcr_total * IFNAMSIZ); +if (buf == NULL) +{ I’d ‘goto out;’ instead, because that’s how we handle other errors in this function. (free(NULL) is guaranteed to be safe, so we can just do that.) on "goto out" we'll end with "return available;" ‘available’ defaults to ‘false’, so that seems fine to me. looks fragile :) I do not see benefits of such an approach. But it will work indeed The reasoning is that it keeps the error handing consistent. If we ever extend the function to e.g. open another file descriptor we’d only have to change the error handling in one location. We’re not adding new ways for the function to fail either. The next potential error is the ioctl() call, where we do exactly the same thing (i.e. goto out) on error, so it already relies on available being set to false. I agree on this style too. Should we require more clean up work in the future, we can just add extra lines right after the out label (with the appropriate checks, of course), thus keeping error handling in one place. Cheers, Best regards, Kristof ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel -- Antonio Quartulli -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] src/openvpn/dco_freebsd.c: handle malloc failure
Hi, On 17/05/2023 22:01, Ilya Shipitsin wrote: malloc was not checked against NULL, I was able to get core dump in case of failure Signed-off-by: Ilya Shipitsin --- src/openvpn/dco_freebsd.c | 5 + 1 file changed, 5 insertions(+) diff --git a/src/openvpn/dco_freebsd.c b/src/openvpn/dco_freebsd.c index abeb..adbd1120 100644 --- a/src/openvpn/dco_freebsd.c +++ b/src/openvpn/dco_freebsd.c @@ -594,6 +594,11 @@ dco_available(int msglevel) } buf = malloc(ifcr.ifcr_total * IFNAMSIZ); +if (buf == NULL) as style guideline, we wanted to go for if (!A) rather than if (A == NULL). Although I am not sure if the whole codebase was cleaned up yet or not. Cheers, +{ +close(fd); +return false; +} ifcr.ifcr_count = ifcr.ifcr_total; ifcr.ifcr_buffer = buf; -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] DCO: fix memory leak in dco_get_peer_stats_multi for Linux
Hi, On 15/05/2023 16:21, Frank Lichtenheld wrote: Leaks a small amount of memory every 15s. Signed-off-by: Frank Lichtenheld wonderful catch, Frank! Acked-by: Antonio Quartulli --- src/openvpn/dco_linux.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c index 796e6f25..2bfdf980 100644 --- a/src/openvpn/dco_linux.c +++ b/src/openvpn/dco_linux.c @@ -925,7 +925,10 @@ dco_get_peer_stats_multi(dco_context_t *dco, struct multi_context *m) nlmsg_hdr(nl_msg)->nlmsg_flags |= NLM_F_DUMP; -return ovpn_nl_msg_send(dco, nl_msg, dco_parse_peer_multi, m, __func__); +int ret = ovpn_nl_msg_send(dco, nl_msg, dco_parse_peer_multi, m, __func__); + +nlmsg_free(nl_msg); +return ret; } static int -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] DCO: support key rotation notifications
Hi, On 14/04/2023 11:42, Kristof Provost via Openvpn-devel wrote: From: Kristof Provost Allow the kernel driver to notify us that it's time to renegotiate keys. The intent is to avoid IV re-use after 2^32 packets. This is a first draft intended for discussion. The accompanying kernel change for FreeBSD can be found in https://reviews.freebsd.org/D39570 Signed-off-by: Kristof Provost This looks good to me and I think it's reasonable to use the CMD_SWAP_KEYS as notification for userspace to actually trigger a key rotation. Acked-by: Antonio Quartulli Linux and Windows part is now missing. Cheers, --- src/openvpn/dco_freebsd.c | 4 src/openvpn/dco_freebsd.h | 1 + src/openvpn/forward.c | 32 +--- src/openvpn/multi.c| 4 src/openvpn/ovpn_dco_freebsd.h | 1 + src/openvpn/ssl.c | 6 ++ src/openvpn/ssl.h | 3 +++ 7 files changed, 40 insertions(+), 11 deletions(-) diff --git a/src/openvpn/dco_freebsd.c b/src/openvpn/dco_freebsd.c index a334d5d2..abeb 100644 --- a/src/openvpn/dco_freebsd.c +++ b/src/openvpn/dco_freebsd.c @@ -550,6 +550,10 @@ dco_do_read(dco_context_t *dco) dco->dco_message_type = OVPN_CMD_DEL_PEER; break; +case OVPN_NOTIF_ROTATE_KEY: +dco->dco_message_type = OVPN_CMD_SWAP_KEYS; +break; + default: msg(M_WARN, "Unknown kernel notification %d", type); break; diff --git a/src/openvpn/dco_freebsd.h b/src/openvpn/dco_freebsd.h index a07f9b69..e1a054e0 100644 --- a/src/openvpn/dco_freebsd.h +++ b/src/openvpn/dco_freebsd.h @@ -35,6 +35,7 @@ typedef enum ovpn_key_cipher dco_cipher_t; enum ovpn_message_type_t { OVPN_CMD_DEL_PEER, OVPN_CMD_PACKET, +OVPN_CMD_SWAP_KEYS, }; enum ovpn_del_reason_t { diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c index b3e0ba5d..d50eb457 100644 --- a/src/openvpn/forward.c +++ b/src/openvpn/forward.c @@ -1232,20 +1232,30 @@ process_incoming_dco(struct context *c) return; } -if (dco->dco_message_type != OVPN_CMD_DEL_PEER) +switch (dco->dco_message_type) { -msg(D_DCO_DEBUG, "%s: received message of type %u - ignoring", __func__, -dco->dco_message_type); -return; -} +case OVPN_CMD_DEL_PEER: +if (dco->dco_del_peer_reason == OVPN_DEL_PEER_REASON_EXPIRED) +{ +msg(D_DCO_DEBUG, "%s: received peer expired notification of for peer-id " +"%d", __func__, dco->dco_message_peer_id); +trigger_ping_timeout_signal(c); +return; +} +break; -if (dco->dco_del_peer_reason == OVPN_DEL_PEER_REASON_EXPIRED) -{ -msg(D_DCO_DEBUG, "%s: received peer expired notification of for peer-id " -"%d", __func__, dco->dco_message_peer_id); -trigger_ping_timeout_signal(c); -return; +case OVPN_CMD_SWAP_KEYS: +msg(D_DCO_DEBUG, "%s: received key rotation notification for peer-id %d", +__func__, dco->dco_message_peer_id); +tls_session_soft_reset(c->c2.tls_multi); +break; + +default: +msg(D_DCO_DEBUG, "%s: received message of type %u - ignoring", __func__, +dco->dco_message_type); +return; } + #endif /* if defined(ENABLE_DCO) && (defined(TARGET_LINUX) || defined(TARGET_FREEBSD)) */ } diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index 5444e752..6fb9cff2 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -3284,6 +3284,10 @@ multi_process_incoming_dco(struct multi_context *m) { process_incoming_del_peer(m, mi, dco); } +else if (dco->dco_message_type == OVPN_CMD_SWAP_KEYS) +{ +tls_session_soft_reset(mi->context.c2.tls_multi); +} } else { diff --git a/src/openvpn/ovpn_dco_freebsd.h b/src/openvpn/ovpn_dco_freebsd.h index fec33835..53f94dfd 100644 --- a/src/openvpn/ovpn_dco_freebsd.h +++ b/src/openvpn/ovpn_dco_freebsd.h @@ -36,6 +36,7 @@ enum ovpn_notif_type { OVPN_NOTIF_DEL_PEER, +OVPN_NOTIF_ROTATE_KEY, }; enum ovpn_del_reason { diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index 60aaee8d..26e86c8d 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -1918,6 +1918,12 @@ key_state_soft_reset(struct tls_session *session) ks->remote_addr = ks_lame->remote_addr; } +void +tls_session_soft_reset(struct tls_multi *tls_multi) +{ + key_state_soft_reset(&tls_multi->session[TM_ACTIVE]); +} + /* * Read/write strings from/to a struct buffer with a u1
Re: [Openvpn-devel] Compiling DCO module on Oracle Linux 8, against UEK kernel
Hi, On 02/05/2023 23:18, Shawn Asmussen wrote: Certainly. Here is the contents of the redhat-release, oracle-release, and os-release files. It's also worth noting that the UEK kernel is optional on OEL, so theoretically an OEL system can be running either the RHEL kernel (A recompile of the RHEL source code, just like CentOS 8 used to be before the change), or Oracle's own UEK kernel. In my experience, most people using Oracle tend to stick with the Oracle UEK kernel, though. Therefore, an OEL user could either need the RHEL configuration when compiling the DCO module, or not, depending on which kernel they were compiling against. I see that the build system of the backports project looks into the kernel config for some RHEL specific macros. This might be more sensible as we abstract away from the distro name/version and we rather focus on what kernel is available on the system. I'll add it to my todo list, but patches are welcome :-) Cheers, -[~:#]- cat /etc/os-release NAME="Oracle Linux Server" VERSION="8.7" ID="ol" ID_LIKE="fedora" VARIANT="Server" VARIANT_ID="server" VERSION_ID="8.7" PLATFORM_ID="platform:el8" PRETTY_NAME="Oracle Linux Server 8.7" ANSI_COLOR="0;31" CPE_NAME="cpe:/o:oracle:linux:8:7:server" HOME_URL="https://linux.oracle.com/ <https://linux.oracle.com/>" BUG_REPORT_URL="https://bugzilla.oracle.com/ <https://bugzilla.oracle.com/>" ORACLE_BUGZILLA_PRODUCT="Oracle Linux 8" ORACLE_BUGZILLA_PRODUCT_VERSION=8.7 ORACLE_SUPPORT_PRODUCT="Oracle Linux" ORACLE_SUPPORT_PRODUCT_VERSION=8.7 -[~:#]- cat /etc/oracle-release Oracle Linux Server release 8.7 -[~:#]- cat /etc/redhat-release Red Hat Enterprise Linux release 8.7 (Ootpa) On Tue, May 2, 2023 at 2:06 PM Antonio Quartulli <mailto:a...@unstable.cc>> wrote: Hi, On 02/05/2023 20:38, Shawn Asmussen wrote: > Today I began to setup a test platform in our organization for openvpn > DCO. We are running Oracle's OEL 8. The make of the dco module fails, > and I suspect that it is because the Makefile detects EL8, and tries to > compile using a configuration that is specific to RHEL's custom kernel > source code. However, Oracle Linux, by default uses > their own custom kernel called the UEK (Unbreakable Enterprise Kernel). > Commenting out the EL8 check in the Makefile results in the module > compiling, but I still have to configure the rest of the pieces of the > test before I can verify that the compiled module is functional. Unfortunately this is a bit of a mess with distros claiming to have a certain kernel, but then supporting features that do not belong to it. The RHEL workaround is just a way to deal with it in the Red Hat world. Somebody else on GitHub already reported about this logic not working for CentOS. Can you please paste the content of your /etc/redhat-release ? Regards, > > > -[~/dco-test/ovpn-dco:$]- make > /buildhome/oracle8/build/dco-test/ovpn-dco/gen-compat-autoconf.sh > /buildhome/oracle8/build/dco-test/ovpn-dco/compat-autoconf.h > make -C /lib/modules/5.4.17-2136.318.7.1.el8uek.x86_64/build > M=/buildhome/oracle8/build/dco-test/ovpn-dco > PWD=/buildhome/oracle8/build/dco-test/ovpn-dco REVISION=0.2.20230426 > CONFIG_OVPN_DCO_V2=m INSTALL_MOD_DIR=updates/ modules > make[1]: Entering directory > '/usr/src/kernels/5.4.17-2136.318.7.1.el8uek.x86_64' > CC [M] > /buildhome/oracle8/build/dco-test/ovpn-dco/drivers/net/ovpn-dco/main.o > /buildhome/oracle8/build/dco-test/ovpn-dco/drivers/net/ovpn-dco/main.c:113:28: error: ‘dev_get_tstats64’ undeclared here (not in a function); did you mean ‘dev_get_stats’? > .ndo_get_stats64 = dev_get_tstats64, > ^~~~ > dev_get_stats > make[3]: *** [scripts/Makefile.build:268: > /buildhome/oracle8/build/dco-test/ovpn-dco/drivers/net/ovpn-dco/main.o] > Error 1 > make[2]: *** [scripts/Makefile.build:503: > /buildhome/oracle8/build/dco-test/ovpn-dco/drivers/net/ovpn-dco] Error 2 > make[1]: *** [Makefile:1798: /buildhome/oracle8/build/dco-test/ovpn-dco] > Error 2 > make[1]: Leaving directory > '/usr/src/kernels/5.4.17-2136.318.7.1.el8uek.x86_64' > make: *** [Makefile:59: all] Error 2 > > > ___ > Openvpn-devel mailing list > Openvpn-devel@lists.sourceforge.net <mailto:Openvpn-devel@lists.sourceforge.net> > htt
Re: [Openvpn-devel] Compiling DCO module on Oracle Linux 8, against UEK kernel
Hi, On 02/05/2023 20:38, Shawn Asmussen wrote: Today I began to setup a test platform in our organization for openvpn DCO. We are running Oracle's OEL 8. The make of the dco module fails, and I suspect that it is because the Makefile detects EL8, and tries to compile using a configuration that is specific to RHEL's custom kernel source code. However, Oracle Linux, by default uses their own custom kernel called the UEK (Unbreakable Enterprise Kernel). Commenting out the EL8 check in the Makefile results in the module compiling, but I still have to configure the rest of the pieces of the test before I can verify that the compiled module is functional. Unfortunately this is a bit of a mess with distros claiming to have a certain kernel, but then supporting features that do not belong to it. The RHEL workaround is just a way to deal with it in the Red Hat world. Somebody else on GitHub already reported about this logic not working for CentOS. Can you please paste the content of your /etc/redhat-release ? Regards, -[~/dco-test/ovpn-dco:$]- make /buildhome/oracle8/build/dco-test/ovpn-dco/gen-compat-autoconf.sh /buildhome/oracle8/build/dco-test/ovpn-dco/compat-autoconf.h make -C /lib/modules/5.4.17-2136.318.7.1.el8uek.x86_64/build M=/buildhome/oracle8/build/dco-test/ovpn-dco PWD=/buildhome/oracle8/build/dco-test/ovpn-dco REVISION=0.2.20230426 CONFIG_OVPN_DCO_V2=m INSTALL_MOD_DIR=updates/ modules make[1]: Entering directory '/usr/src/kernels/5.4.17-2136.318.7.1.el8uek.x86_64' CC [M] /buildhome/oracle8/build/dco-test/ovpn-dco/drivers/net/ovpn-dco/main.o /buildhome/oracle8/build/dco-test/ovpn-dco/drivers/net/ovpn-dco/main.c:113:28: error: ‘dev_get_tstats64’ undeclared here (not in a function); did you mean ‘dev_get_stats’? .ndo_get_stats64 = dev_get_tstats64, ^~~~ dev_get_stats make[3]: *** [scripts/Makefile.build:268: /buildhome/oracle8/build/dco-test/ovpn-dco/drivers/net/ovpn-dco/main.o] Error 1 make[2]: *** [scripts/Makefile.build:503: /buildhome/oracle8/build/dco-test/ovpn-dco/drivers/net/ovpn-dco] Error 2 make[1]: *** [Makefile:1798: /buildhome/oracle8/build/dco-test/ovpn-dco] Error 2 make[1]: Leaving directory '/usr/src/kernels/5.4.17-2136.318.7.1.el8uek.x86_64' make: *** [Makefile:59: all] Error 2 ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2] Add missing check for nl_socket_alloc failure
Hi, On 25/04/2023 12:20, Arne Schwabe wrote: This can happen if the memory alloc fails. Patch V2: add goto error Change-Id: Iee66caa794d267ac5f8bee584633352893047171 Signed-off-by: Arne Schwabe This patch seems to be a duplicate? I also commented the "old v2". Cheers, --- src/openvpn/dco_linux.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c index 41540c0f8..95fe94848 100644 --- a/src/openvpn/dco_linux.c +++ b/src/openvpn/dco_linux.c @@ -83,6 +83,13 @@ resolve_ovpn_netlink_id(int msglevel) int ret; struct nl_sock *nl_sock = nl_socket_alloc(); +if (!nl_sock) +{ +msg(msglevel, "Allocating net link socket failed"); +ret = -1; +goto err_sock; +} + ret = genl_connect(nl_sock); if (ret) { -- Antonio Quartulli -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2] Add missing check for nl_socket_alloc failure
On 29/03/2023 14:46, Arne Schwabe wrote: This can happen if the memory alloc fails. Patch V2: add goto error Change-Id: Iee66caa794d267ac5f8bee584633352893047171 Signed-off-by: Arne Schwabe --- src/openvpn/dco_linux.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c index 41540c0f8..95fe94848 100644 --- a/src/openvpn/dco_linux.c +++ b/src/openvpn/dco_linux.c @@ -83,6 +83,13 @@ resolve_ovpn_netlink_id(int msglevel) int ret; struct nl_sock *nl_sock = nl_socket_alloc(); +if (!nl_sock) +{ +msg(msglevel, "Allocating net link socket failed"); +ret = -1; Please use -ENOMEM here - it is always better to return an actual reason rather than just "failed". +goto err_sock; There is no need to jump to cleanup. You can just return -1 here and save one line. (this is what we do in other functions of this file) Cheers, -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] buffer: use memcpy in buf_catrunc
Hi, On 28/03/2023 20:51, Matthias Andree wrote: Am 28.03.23 um 17:12 schrieb Frank Lichtenheld: Since we use strlen() to determine the length and then check it ourselves, there is really no point in using strncpy. But the compiler might complain that we use the output of strlen() for the length of strncpy which is usually a sign for bugs: error: ‘strncpy’ specified bound depends on the length of the source argument [-Werror=stringop-overflow=] Warning was at least triggered for mingw-gcc version 10-win32 20220113. Signed-off-by: Frank Lichtenheld --- src/openvpn/buffer.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/openvpn/buffer.c b/src/openvpn/buffer.c index d099795b..de40d690 100644 --- a/src/openvpn/buffer.c +++ b/src/openvpn/buffer.c @@ -317,9 +317,9 @@ buf_catrunc(struct buffer *buf, const char *str) if (buf_forward_capacity(buf) <= 1) { int len = (int) strlen(str) + 1; - if (len < buf_forward_capacity_total(buf)) + if ((len > 0) && (len < buf_forward_capacity_total(buf))) I hope you don't mind but there are a few things I would like to bring to attention. 1. The first is clamping and wrapping around: strlen(someptr) returns size_t, and this can be >= INT_MAX. Add +1 and this wraps to INT_MIN. While you now trap this with the new "len > 0", there does not seem to be any error handling, in that the function silently skips its actual function. Is this what such a function should do? Or should it be "complain an give up"? { - strncpynt((char *)(buf->data + buf->capacity - len), str, len); + memcpy((void *)(buf->data + buf->capacity - len), (void *)str, (size_t)len); 2. This opens the door for another compiler warning, which is that you are casting away the const-ness of "str". It is const char * when passed in, but you cast it to void * (non-const). The 2nd argument of memcpy would need to be cast to (const void *) to avoid that. memcpy is declared (..., const void *restrict src, ...), so that's safe. 3. However, strncpynt would set buf->data[buf->capacity] = '\0', which is now missing. I thin this is not needed in this case, because str [0..strlen(str) + 1] includes the \0 which will be copied by memcpy. No? So: NAK, please revise on #3. Cheers, Matthias ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] buffer: use memcpy in buf_catrunc
Hi, On 28/03/2023 17:12, Frank Lichtenheld wrote: Since we use strlen() to determine the length and then check it ourselves, there is really no point in using strncpy. But the compiler might complain that we use the output of strlen() for the length of strncpy which is usually a sign for bugs: error: ‘strncpy’ specified bound depends on the length of the source argument [-Werror=stringop-overflow=] Warning was at least triggered for mingw-gcc version 10-win32 20220113. Signed-off-by: Frank Lichtenheld --- src/openvpn/buffer.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/openvpn/buffer.c b/src/openvpn/buffer.c index d099795b..de40d690 100644 --- a/src/openvpn/buffer.c +++ b/src/openvpn/buffer.c @@ -317,9 +317,9 @@ buf_catrunc(struct buffer *buf, const char *str) if (buf_forward_capacity(buf) <= 1) { int len = (int) strlen(str) + 1; -if (len < buf_forward_capacity_total(buf)) +if ((len > 0) && (len < buf_forward_capacity_total(buf))) len cannot be negative, but I guess you want to check that it is actually not 0. Just a style thing I guess? (Because memcpy can happily deal with len=0.) { -strncpynt((char *)(buf->data + buf->capacity - len), str, len); +memcpy((void *)(buf->data + buf->capacity - len), (void *)str, (size_t)len); I'd throw away the casts to 'void *'. They are not really needed as you can assign everything to 'void *'. Cheers, } } } -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Bug-fix: segfault in dco_get_peer_stats()
Hi, On 27/03/2023 22:42, Selva Nair wrote: But this may not be the time and place for a refactor. As dco is young, I think it can mature in 2.7.. I agree on all fronts. Let's get this patch in with a minimal fix that deals with what we have. We can make this better in master. Acked-by: Antonio Quartulli -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Bug-fix: segfault in dco_get_peer_stats()
Hi, On 27/03/2023 19:12, selva.n...@gmail.com wrote: From: Selva Nair We persist peer-stats when restarting, but an early restart before open_tun results in a segfault in dco_get_peer_stats(). To reproduce, trigger a TLS handshake error due to lack of common protocols, for example. Fix by checking that tuntap is defined before dereferencing it. Signed-off-by: Selva Nair Nice catch! Thanks a lot! --- I'm not entirely sure this is the right place to fix this. Or is it the caller at fault exercising dco_get_peer_stats() when tuntap is not set? Indeed that was my assumption. Does somebody have the stacktrace? I wanted to check where this call is coming from exactly. Imho it'd be reasonable if the caller could check if we have a device before invoking any DCO API. Alla other DCO API just assume that everything was properly initialized before being invoked, therefore it'd be nice to keep the same assumption here. (better would even be if this gets_stats function would not require to fiddle with a struct context at allbut that's material for master) Cheers, src/openvpn/dco_linux.c | 5 + 1 file changed, 5 insertions(+) diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c index 317f9dc0..41540c0f 100644 --- a/src/openvpn/dco_linux.c +++ b/src/openvpn/dco_linux.c @@ -975,6 +975,11 @@ dco_get_peer_stats(struct context *c) uint32_t peer_id = c->c2.tls_multi->dco_peer_id; msg(D_DCO_DEBUG, "%s: peer-id %d", __func__, peer_id); +if (!c->c1.tuntap) +{ +return 0; +} + dco_context_t *dco = &c->c1.tuntap->dco; struct nl_msg *nl_msg = ovpn_dco_nlmsg_create(dco, OVPN_CMD_GET_PEER); struct nlattr *attr = nla_nest_start(nl_msg, OVPN_ATTR_GET_PEER); -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v4] dco-linux: implement dco_get_peer_stats{, multi} API
Spot on and sorry for forgetting to mentioning it: You need ovpn-dco at this commit: commit 726fdfe0fa21aa4e87c5a60294ea0365ce7b6809 (HEAD -> master, origin/master) Author: Antonio Quartulli Date: Mon Mar 20 23:50:52 2023 +0100 ovpn-dco: store and report transport rx/tx stats as well Signed-off-by: Antonio Quartulli Which comes right after the one you are using. It is not a breaking change (as you see there is no real *failure*, just missing information from the kernel side). Regards, On 23/03/2023 13:03, Gert Doering wrote: Hi, On Wed, Mar 22, 2023 at 08:27:57PM +0100, Antonio Quartulli wrote: With this API it is possible to retrieve the stats for a specific peer or for all peers and then update the userspace counters with the value reported by DCO. Change-Id: Ia3990b86b1be7ca844fb1674b39ce0d60528ccff Signed-off-by: Antonio Quartulli This *looks* very reasonable, but only half of it works for me. ... tun-udp-p2mp[492453]: OpenVPN 2.7_git [git:vw/master/5a8fb55ac8cf4019] x86_64-pc-linux-gnu [SSL (mbed TLS)] [LZO] [LZ4] [EPOLL] [MH/PKTINFO] [AEAD] [DCO] built on Mar 23 2023 tun-udp-p2mp[492453]: DCO version: 0.2.20230313 ... tun-udp-p2mp[492454]: dco_get_peer_stats_multi tun-udp-p2mp[492454]: dco_parse_peer_multi: parsing message... tun-udp-p2mp[492454]: dco_update_peer_stat: no link RX bytes provided in reply for peer 0 tun-udp-p2mp[492454]: dco_update_peer_stat: no link TX bytes provided in reply for peer 0 tun-udp-p2mp[492454]: dco_update_peer_stat / tun_read_bytes: 240 tun-udp-p2mp[492454]: dco_update_peer_stat / tun_write_bytes: 96 tun-udp-p2mp[492454]: dco_parse_peer_multi: parsing message... tun-udp-p2mp[492454]: dco_update_peer_stat: no link RX bytes provided in reply for peer 1 tun-udp-p2mp[492454]: dco_update_peer_stat: no link TX bytes provided in reply for peer 1 tun-udp-p2mp[492454]: dco_update_peer_stat / tun_read_bytes: 776 tun-udp-p2mp[492454]: dco_update_peer_stat / tun_write_bytes: 80 So neither for p2p tls nor for --server setups, I am receiving "link bytes", only tun*bytes. I have not checked the kernel side if I *should* receive anything, but this looks incomplete - either the kernel side is not yet ready (not a showstopper) or there is a desync in the tags used, which might or might not prevent merging... Antonio, any ideas? gert -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v4] dco-freebsd: use m->instances[] instead of m->hash
Hi, On 23/03/2023 09:03, Gert Doering wrote: From: Antonio Quartulli When retrieving the multi_instance of a specific peer, there is no need to peform a linear search across the whole m->hash list. We can directly access the needed object via m->instances[peer-id] in constant time (and just one line of code). Adapt the dco-freebsd code to do so. v4: use "peerid" everywhere as that's what FreeBSD does, change message text Cc: Kristof Provost Change-Id: I8d8af6f872146604a9710edf443db65df48ac3cb Signed-off-by: Antonio Quartulli --- src/openvpn/dco_freebsd.c | 22 ++ 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/src/openvpn/dco_freebsd.c b/src/openvpn/dco_freebsd.c index 225b3cf8..a334d5d2 100644 --- a/src/openvpn/dco_freebsd.c +++ b/src/openvpn/dco_freebsd.c @@ -674,27 +674,17 @@ dco_event_set(dco_context_t *dco, struct event_set *es, void *arg) static void dco_update_peer_stat(struct multi_context *m, uint32_t peerid, const nvlist_t *nvl) { -struct hash_element *he; -struct hash_iterator hi; -hash_iterator_init(m->hash, &hi); - -while ((he = hash_iterator_next(&hi))) +if (peerid >= m->max_clients || !m->instances[peerid]) { -struct multi_instance *mi = (struct multi_instance *) he->value; - -if (mi->context.c2.tls_multi->peer_id != peerid) -{ -continue; -} - -mi->context.c2.dco_read_bytes = nvlist_get_number(nvl, "in"); -mi->context.c2.dco_write_bytes = nvlist_get_number(nvl, "out"); - +msg(M_WARN, "dco_update_peer_stat: invalid peer ID %d returned by kernel", peerid); I'd prefer to use __func__ here, but it is noted that dco_freebsd.c seldomly uses this approach. For this reason I think hardcoding the function name is fine for now. Later on we can do a full cleanup. return; } -msg(M_INFO, "Peer %d returned by kernel, but not found locally", peerid); +struct multi_instance *mi = m->instances[peerid]; + +mi->context.c2.dco_read_bytes = nvlist_get_number(nvl, "in"); +mi->context.c2.dco_write_bytes = nvlist_get_number(nvl, "out"); } int This patch is basically my v3 with the peerid fixed and the message changed. I can't ACK since this patch bears my own signature, but I am fine with the applied modification. Cheers, -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH v3] dco-freebsd: use m->instances[] instead of m->hash
When retrieving the multi_instance of a specific peer, there is no need to peform a linear search across the whole m->hash list. We can directly access the needed object via m->instances[peer-id] in constant time (and just one line of code). Adapt the dco-freebsd code to do so. Cc: Kristof Provost Change-Id: I8d8af6f872146604a9710edf443db65df48ac3cb Signed-off-by: Antonio Quartulli --- NOTE: not tested because I have no FreeBSD environment Changes from v1: * added boundary check on peer-id Changes from v2: * use one check only instead of two --- src/openvpn/dco_freebsd.c | 23 +++ 1 file changed, 7 insertions(+), 16 deletions(-) diff --git a/src/openvpn/dco_freebsd.c b/src/openvpn/dco_freebsd.c index 225b3cf8..54ec16ff 100644 --- a/src/openvpn/dco_freebsd.c +++ b/src/openvpn/dco_freebsd.c @@ -674,27 +674,18 @@ dco_event_set(dco_context_t *dco, struct event_set *es, void *arg) static void dco_update_peer_stat(struct multi_context *m, uint32_t peerid, const nvlist_t *nvl) { -struct hash_element *he; -struct hash_iterator hi; -hash_iterator_init(m->hash, &hi); - -while ((he = hash_iterator_next(&hi))) +if (peer_id >= m->max_clients || !m->instances[peer_id]) { -struct multi_instance *mi = (struct multi_instance *) he->value; - -if (mi->context.c2.tls_multi->peer_id != peerid) -{ -continue; -} - -mi->context.c2.dco_read_bytes = nvlist_get_number(nvl, "in"); -mi->context.c2.dco_write_bytes = nvlist_get_number(nvl, "out"); - +msg(M_WARN, "Cannot store DCO stats for peer %u", +peerid); return; } -msg(M_INFO, "Peer %d returned by kernel, but not found locally", peerid); +struct multi_instance *mi = m->instances[peer_id]; + +mi->context.c2.dco_read_bytes = nvlist_get_number(nvl, "in"); +mi->context.c2.dco_write_bytes = nvlist_get_number(nvl, "out"); } int -- 2.39.2 ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH v4] dco-linux: implement dco_get_peer_stats{, multi} API
With this API it is possible to retrieve the stats for a specific peer or for all peers and then update the userspace counters with the value reported by DCO. Change-Id: Ia3990b86b1be7ca844fb1674b39ce0d60528ccff Signed-off-by: Antonio Quartulli --- Changes from v1: * use m->instances[] instead of iterating over m->hash Changes from v2: * added boundary check on peer-id Changes from v3: * use one check only instead of two --- src/openvpn/dco_linux.c | 183 --- src/openvpn/ovpn_dco_linux.h | 14 ++- 2 files changed, 179 insertions(+), 18 deletions(-) diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c index 47961849..317f9dc0 100644 --- a/src/openvpn/dco_linux.c +++ b/src/openvpn/dco_linux.c @@ -41,6 +41,7 @@ #include "tun.h" #include "ssl.h" #include "fdmisc.h" +#include "multi.h" #include "ssl_verify.h" #include "ovpn_dco_linux.h" @@ -168,16 +169,17 @@ ovpn_nl_recvmsgs(dco_context_t *dco, const char *prefix) * @param dco The dco context to use * @param nl_msgthe message to use * @param cbAn optional callback if the caller expects an answer + * @param cb_argAn optional param to pass to the callback * @param prefixA prefix to report in the error message to give the user context * @return status of sending the message */ static int ovpn_nl_msg_send(dco_context_t *dco, struct nl_msg *nl_msg, ovpn_nl_cb cb, - const char *prefix) + void *cb_arg, const char *prefix) { dco->status = 1; -nl_cb_set(dco->nl_cb, NL_CB_VALID, NL_CB_CUSTOM, cb, dco); +nl_cb_set(dco->nl_cb, NL_CB_VALID, NL_CB_CUSTOM, cb, cb_arg); nl_send_auto(dco->nl_sock, nl_msg); while (dco->status == 1) @@ -268,7 +270,7 @@ dco_new_peer(dco_context_t *dco, unsigned int peerid, int sd, } nla_nest_end(nl_msg, attr); -ret = ovpn_nl_msg_send(dco, nl_msg, NULL, __func__); +ret = ovpn_nl_msg_send(dco, nl_msg, NULL, NULL, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -489,7 +491,7 @@ dco_swap_keys(dco_context_t *dco, unsigned int peerid) NLA_PUT_U32(nl_msg, OVPN_SWAP_KEYS_ATTR_PEER_ID, peerid); nla_nest_end(nl_msg, attr); -ret = ovpn_nl_msg_send(dco, nl_msg, NULL, __func__); +ret = ovpn_nl_msg_send(dco, nl_msg, NULL, NULL, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -513,7 +515,7 @@ dco_del_peer(dco_context_t *dco, unsigned int peerid) NLA_PUT_U32(nl_msg, OVPN_DEL_PEER_ATTR_PEER_ID, peerid); nla_nest_end(nl_msg, attr); -ret = ovpn_nl_msg_send(dco, nl_msg, NULL, __func__); +ret = ovpn_nl_msg_send(dco, nl_msg, NULL, NULL, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -539,7 +541,7 @@ dco_del_key(dco_context_t *dco, unsigned int peerid, NLA_PUT_U8(nl_msg, OVPN_DEL_KEY_ATTR_KEY_SLOT, slot); nla_nest_end(nl_msg, attr); -ret = ovpn_nl_msg_send(dco, nl_msg, NULL, __func__); +ret = ovpn_nl_msg_send(dco, nl_msg, NULL, NULL, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -596,7 +598,7 @@ dco_new_key(dco_context_t *dco, unsigned int peerid, int keyid, nla_nest_end(nl_msg, attr); -ret = ovpn_nl_msg_send(dco, nl_msg, NULL, __func__); +ret = ovpn_nl_msg_send(dco, nl_msg, NULL, NULL, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -625,7 +627,7 @@ dco_set_peer(dco_context_t *dco, unsigned int peerid, keepalive_timeout); nla_nest_end(nl_msg, attr); -ret = ovpn_nl_msg_send(dco, nl_msg, NULL, __func__); +ret = ovpn_nl_msg_send(dco, nl_msg, NULL, NULL, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -706,7 +708,7 @@ ovpn_get_mcast_id(dco_context_t *dco) int ret = -EMSGSIZE; NLA_PUT_STRING(nl_msg, CTRL_ATTR_FAMILY_NAME, OVPN_NL_NAME); -ret = ovpn_nl_msg_send(dco, nl_msg, mcast_family_handler, __func__); +ret = ovpn_nl_msg_send(dco, nl_msg, mcast_family_handler, dco, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -819,18 +821,173 @@ dco_do_read(dco_context_t *dco) return ovpn_nl_recvmsgs(dco, __func__); } +static void +dco_update_peer_stat(struct context_2 *c2, struct nlattr *tb[], uint32_t id) +{ +if (tb[OVPN_GET_PEER_RESP_ATTR_LINK_RX_BYTES]) +{ +c2->dco_read_bytes = nla_get_u64(tb[OVPN_GET_PEER_RESP_ATTR_LINK_RX_BYTES]); +msg(D_DCO_DEBUG, "%s / dco_read_bytes: %lu", __func__, +c2->dco_read_bytes); +} +else +{ +msg(M_WARN, "%s: no link RX bytes provided in reply for peer %u", +__func__, id); +} + +if (tb[OVPN_GET_PEER_RESP_ATTR_LINK_TX_BYTES]) +{ +c2->dco_write_bytes = nla_get_u64(tb[OVPN_GET_PEER_RESP_ATTR_LINK_TX_BYTES]); +msg(D_DCO_DEBUG, "%s / dco_write_bytes: %lu", __func__, +c2->dco_write_bytes); +} +else +{
[Openvpn-devel] [PATCH v3] dco-linux: implement dco_get_peer_stats{, multi} API
With this API it is possible to retrieve the stats for a specific peer or for all peers and then update the userspace counters with the value reported by DCO. Change-Id: Ia3990b86b1be7ca844fb1674b39ce0d60528ccff Signed-off-by: Antonio Quartulli --- Changes from v1: * use m->instances[] instead of iterating over m->hash Changes from v2: * added boundary check on peer-id --- src/openvpn/dco_linux.c | 190 --- src/openvpn/ovpn_dco_linux.h | 14 ++- 2 files changed, 186 insertions(+), 18 deletions(-) diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c index 47961849..8e7b4eab 100644 --- a/src/openvpn/dco_linux.c +++ b/src/openvpn/dco_linux.c @@ -41,6 +41,7 @@ #include "tun.h" #include "ssl.h" #include "fdmisc.h" +#include "multi.h" #include "ssl_verify.h" #include "ovpn_dco_linux.h" @@ -168,16 +169,17 @@ ovpn_nl_recvmsgs(dco_context_t *dco, const char *prefix) * @param dco The dco context to use * @param nl_msgthe message to use * @param cbAn optional callback if the caller expects an answer + * @param cb_argAn optional param to pass to the callback * @param prefixA prefix to report in the error message to give the user context * @return status of sending the message */ static int ovpn_nl_msg_send(dco_context_t *dco, struct nl_msg *nl_msg, ovpn_nl_cb cb, - const char *prefix) + void *cb_arg, const char *prefix) { dco->status = 1; -nl_cb_set(dco->nl_cb, NL_CB_VALID, NL_CB_CUSTOM, cb, dco); +nl_cb_set(dco->nl_cb, NL_CB_VALID, NL_CB_CUSTOM, cb, cb_arg); nl_send_auto(dco->nl_sock, nl_msg); while (dco->status == 1) @@ -268,7 +270,7 @@ dco_new_peer(dco_context_t *dco, unsigned int peerid, int sd, } nla_nest_end(nl_msg, attr); -ret = ovpn_nl_msg_send(dco, nl_msg, NULL, __func__); +ret = ovpn_nl_msg_send(dco, nl_msg, NULL, NULL, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -489,7 +491,7 @@ dco_swap_keys(dco_context_t *dco, unsigned int peerid) NLA_PUT_U32(nl_msg, OVPN_SWAP_KEYS_ATTR_PEER_ID, peerid); nla_nest_end(nl_msg, attr); -ret = ovpn_nl_msg_send(dco, nl_msg, NULL, __func__); +ret = ovpn_nl_msg_send(dco, nl_msg, NULL, NULL, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -513,7 +515,7 @@ dco_del_peer(dco_context_t *dco, unsigned int peerid) NLA_PUT_U32(nl_msg, OVPN_DEL_PEER_ATTR_PEER_ID, peerid); nla_nest_end(nl_msg, attr); -ret = ovpn_nl_msg_send(dco, nl_msg, NULL, __func__); +ret = ovpn_nl_msg_send(dco, nl_msg, NULL, NULL, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -539,7 +541,7 @@ dco_del_key(dco_context_t *dco, unsigned int peerid, NLA_PUT_U8(nl_msg, OVPN_DEL_KEY_ATTR_KEY_SLOT, slot); nla_nest_end(nl_msg, attr); -ret = ovpn_nl_msg_send(dco, nl_msg, NULL, __func__); +ret = ovpn_nl_msg_send(dco, nl_msg, NULL, NULL, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -596,7 +598,7 @@ dco_new_key(dco_context_t *dco, unsigned int peerid, int keyid, nla_nest_end(nl_msg, attr); -ret = ovpn_nl_msg_send(dco, nl_msg, NULL, __func__); +ret = ovpn_nl_msg_send(dco, nl_msg, NULL, NULL, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -625,7 +627,7 @@ dco_set_peer(dco_context_t *dco, unsigned int peerid, keepalive_timeout); nla_nest_end(nl_msg, attr); -ret = ovpn_nl_msg_send(dco, nl_msg, NULL, __func__); +ret = ovpn_nl_msg_send(dco, nl_msg, NULL, NULL, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -706,7 +708,7 @@ ovpn_get_mcast_id(dco_context_t *dco) int ret = -EMSGSIZE; NLA_PUT_STRING(nl_msg, CTRL_ATTR_FAMILY_NAME, OVPN_NL_NAME); -ret = ovpn_nl_msg_send(dco, nl_msg, mcast_family_handler, __func__); +ret = ovpn_nl_msg_send(dco, nl_msg, mcast_family_handler, dco, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -819,18 +821,180 @@ dco_do_read(dco_context_t *dco) return ovpn_nl_recvmsgs(dco, __func__); } +static void +dco_update_peer_stat(struct context_2 *c2, struct nlattr *tb[], uint32_t id) +{ +if (tb[OVPN_GET_PEER_RESP_ATTR_LINK_RX_BYTES]) +{ +c2->dco_read_bytes = nla_get_u64(tb[OVPN_GET_PEER_RESP_ATTR_LINK_RX_BYTES]); +msg(D_DCO_DEBUG, "%s / dco_read_bytes: %lu", __func__, +c2->dco_read_bytes); +} +else +{ +msg(M_WARN, "%s: no link RX bytes provided in reply for peer %u", +__func__, id); +} + +if (tb[OVPN_GET_PEER_RESP_ATTR_LINK_TX_BYTES]) +{ +c2->dco_write_bytes = nla_get_u64(tb[OVPN_GET_PEER_RESP_ATTR_LINK_TX_BYTES]); +msg(D_DCO_DEBUG, "%s / dco_write_bytes: %lu", __func__, +c2->dco_write_bytes); +} +else +{ +msg(M_WARN, "%s: no link TX bytes provided i
[Openvpn-devel] [PATCH v2] dco-freebsd: use m->instances[] instead of m->hash
When retrieving the multi_instance of a specific peer, there is no need to peform a linear search across the whole m->hash list. We can directly access the needed object via m->instances[peer-id] in constant time (and just one line of code). Adapt the dco-freebsd code to do so. Cc: Kristof Provost Change-Id: I8d8af6f872146604a9710edf443db65df48ac3cb Signed-off-by: Antonio Quartulli --- NOTE: not tested because I have no FreeBSD environment Changes from v1: * added boundary check on peer-id --- src/openvpn/dco_freebsd.c | 27 --- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/src/openvpn/dco_freebsd.c b/src/openvpn/dco_freebsd.c index 225b3cf8..ec93eb61 100644 --- a/src/openvpn/dco_freebsd.c +++ b/src/openvpn/dco_freebsd.c @@ -674,27 +674,24 @@ dco_event_set(dco_context_t *dco, struct event_set *es, void *arg) static void dco_update_peer_stat(struct multi_context *m, uint32_t peerid, const nvlist_t *nvl) { -struct hash_element *he; -struct hash_iterator hi; +struct multi_instance *mi; -hash_iterator_init(m->hash, &hi); - -while ((he = hash_iterator_next(&hi))) +if (peer_id >= m->max_clients) { -struct multi_instance *mi = (struct multi_instance *) he->value; - -if (mi->context.c2.tls_multi->peer_id != peerid) -{ -continue; -} - -mi->context.c2.dco_read_bytes = nvlist_get_number(nvl, "in"); -mi->context.c2.dco_write_bytes = nvlist_get_number(nvl, "out"); +msg(M_WARN, "Cannot retrieve stats for peer %u. ID is invalid (out of bound)!", +peerid); +return; +} +mi = m->instances[peer_id]; +if (!mi) +{ +msg(M_INFO, "Peer %u returned by kernel, but not found locally", peerid); return; } -msg(M_INFO, "Peer %d returned by kernel, but not found locally", peerid); +mi->context.c2.dco_read_bytes = nvlist_get_number(nvl, "in"); +mi->context.c2.dco_write_bytes = nvlist_get_number(nvl, "out"); } int -- 2.39.2 ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] dco_freebsd: use m->instances[] instead of m->hash
Hi, On 22/03/2023 08:14, Gert Doering wrote: Hi, On Wed, Mar 22, 2023 at 12:10:03AM +0100, Antonio Quartulli wrote: +struct multi_instance *mi = m->instances[peer_id]; +if (!mi) { This (and undoubtedly the same code in dco_linux.c) is trusting the kernel to never return peer_id values that are outside the array boundaries. very good catch. Is this what we want? no. we should not trust an external source. Will send v2 for this and v3 for dco-linux I'd strongly prefer to have a check like this here if ((peer_id < m->max_clients) && (m->instances[peer_id])) { ... } (which is what we do in multi_process_incoming_dco(), for example) Note: in p2p mode, peer-id is something random, usually much bigger than max_clients - now this *should* never be called in p2p mode, but I still do not have a good feeling without the bounds check. gert -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] dco_freebsd: use m->instances[] instead of m->hash
Hi, On 22/03/2023 00:10, Antonio Quartulli wrote: When retrieving the multi_instance of a specific peer, there is no need to peform a linear search across the whole m->hash list. We can directly access the needed object via m->instances[peer-id] in constant time (and just one line of code). Adapt the dco-freebsd code to do so. Cc: Kristof Provost If the patch is suitable for merging, please change the above to k...@freebsd.org before moving on. Cheers, Change-Id: I8d8af6f872146604a9710edf443db65df48ac3cb Signed-off-by: Antonio Quartulli --- NOTE: not tested because I have no FreeBSD environment and I can't find how to kick off the buildbot --- src/openvpn/dco_freebsd.c | 22 +- 1 file changed, 5 insertions(+), 17 deletions(-) diff --git a/src/openvpn/dco_freebsd.c b/src/openvpn/dco_freebsd.c index 225b3cf8..ae8c1380 100644 --- a/src/openvpn/dco_freebsd.c +++ b/src/openvpn/dco_freebsd.c @@ -674,27 +674,15 @@ dco_event_set(dco_context_t *dco, struct event_set *es, void *arg) static void dco_update_peer_stat(struct multi_context *m, uint32_t peerid, const nvlist_t *nvl) { -struct hash_element *he; -struct hash_iterator hi; - -hash_iterator_init(m->hash, &hi); - -while ((he = hash_iterator_next(&hi))) +struct multi_instance *mi = m->instances[peer_id]; +if (!mi) { -struct multi_instance *mi = (struct multi_instance *) he->value; - -if (mi->context.c2.tls_multi->peer_id != peerid) -{ -continue; -} - -mi->context.c2.dco_read_bytes = nvlist_get_number(nvl, "in"); -mi->context.c2.dco_write_bytes = nvlist_get_number(nvl, "out"); - +msg(M_INFO, "Peer %d returned by kernel, but not found locally", peerid); return; } -msg(M_INFO, "Peer %d returned by kernel, but not found locally", peerid); +mi->context.c2.dco_read_bytes = nvlist_get_number(nvl, "in"); + mi->context.c2.dco_write_bytes = nvlist_get_number(nvl, "out"); } int -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] dco_freebsd: use m->instances[] instead of m->hash
When retrieving the multi_instance of a specific peer, there is no need to peform a linear search across the whole m->hash list. We can directly access the needed object via m->instances[peer-id] in constant time (and just one line of code). Adapt the dco-freebsd code to do so. Cc: Kristof Provost Change-Id: I8d8af6f872146604a9710edf443db65df48ac3cb Signed-off-by: Antonio Quartulli --- NOTE: not tested because I have no FreeBSD environment and I can't find how to kick off the buildbot --- src/openvpn/dco_freebsd.c | 22 +- 1 file changed, 5 insertions(+), 17 deletions(-) diff --git a/src/openvpn/dco_freebsd.c b/src/openvpn/dco_freebsd.c index 225b3cf8..ae8c1380 100644 --- a/src/openvpn/dco_freebsd.c +++ b/src/openvpn/dco_freebsd.c @@ -674,27 +674,15 @@ dco_event_set(dco_context_t *dco, struct event_set *es, void *arg) static void dco_update_peer_stat(struct multi_context *m, uint32_t peerid, const nvlist_t *nvl) { -struct hash_element *he; -struct hash_iterator hi; - -hash_iterator_init(m->hash, &hi); - -while ((he = hash_iterator_next(&hi))) +struct multi_instance *mi = m->instances[peer_id]; +if (!mi) { -struct multi_instance *mi = (struct multi_instance *) he->value; - -if (mi->context.c2.tls_multi->peer_id != peerid) -{ -continue; -} - -mi->context.c2.dco_read_bytes = nvlist_get_number(nvl, "in"); -mi->context.c2.dco_write_bytes = nvlist_get_number(nvl, "out"); - +msg(M_INFO, "Peer %d returned by kernel, but not found locally", peerid); return; } -msg(M_INFO, "Peer %d returned by kernel, but not found locally", peerid); +mi->context.c2.dco_read_bytes = nvlist_get_number(nvl, "in"); +mi->context.c2.dco_write_bytes = nvlist_get_number(nvl, "out"); } int -- 2.39.2 ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH v2] dco-linux: implement dco_get_peer_stats{, multi} API
With this API it is possible to retrieve the stats for a specific peer or for all peers and then update the userspace counters with the value reported by DCO. Change-Id: Ia3990b86b1be7ca844fb1674b39ce0d60528ccff Signed-off-by: Antonio Quartulli --- Changes from v1: * use m->instances[] instead of iterating over m->hash --- src/openvpn/dco_linux.c | 183 --- src/openvpn/ovpn_dco_linux.h | 14 ++- 2 files changed, 179 insertions(+), 18 deletions(-) diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c index 47961849..4bbe7e22 100644 --- a/src/openvpn/dco_linux.c +++ b/src/openvpn/dco_linux.c @@ -41,6 +41,7 @@ #include "tun.h" #include "ssl.h" #include "fdmisc.h" +#include "multi.h" #include "ssl_verify.h" #include "ovpn_dco_linux.h" @@ -168,16 +169,17 @@ ovpn_nl_recvmsgs(dco_context_t *dco, const char *prefix) * @param dco The dco context to use * @param nl_msgthe message to use * @param cbAn optional callback if the caller expects an answer + * @param cb_argAn optional param to pass to the callback * @param prefixA prefix to report in the error message to give the user context * @return status of sending the message */ static int ovpn_nl_msg_send(dco_context_t *dco, struct nl_msg *nl_msg, ovpn_nl_cb cb, - const char *prefix) + void *cb_arg, const char *prefix) { dco->status = 1; -nl_cb_set(dco->nl_cb, NL_CB_VALID, NL_CB_CUSTOM, cb, dco); +nl_cb_set(dco->nl_cb, NL_CB_VALID, NL_CB_CUSTOM, cb, cb_arg); nl_send_auto(dco->nl_sock, nl_msg); while (dco->status == 1) @@ -268,7 +270,7 @@ dco_new_peer(dco_context_t *dco, unsigned int peerid, int sd, } nla_nest_end(nl_msg, attr); -ret = ovpn_nl_msg_send(dco, nl_msg, NULL, __func__); +ret = ovpn_nl_msg_send(dco, nl_msg, NULL, NULL, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -489,7 +491,7 @@ dco_swap_keys(dco_context_t *dco, unsigned int peerid) NLA_PUT_U32(nl_msg, OVPN_SWAP_KEYS_ATTR_PEER_ID, peerid); nla_nest_end(nl_msg, attr); -ret = ovpn_nl_msg_send(dco, nl_msg, NULL, __func__); +ret = ovpn_nl_msg_send(dco, nl_msg, NULL, NULL, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -513,7 +515,7 @@ dco_del_peer(dco_context_t *dco, unsigned int peerid) NLA_PUT_U32(nl_msg, OVPN_DEL_PEER_ATTR_PEER_ID, peerid); nla_nest_end(nl_msg, attr); -ret = ovpn_nl_msg_send(dco, nl_msg, NULL, __func__); +ret = ovpn_nl_msg_send(dco, nl_msg, NULL, NULL, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -539,7 +541,7 @@ dco_del_key(dco_context_t *dco, unsigned int peerid, NLA_PUT_U8(nl_msg, OVPN_DEL_KEY_ATTR_KEY_SLOT, slot); nla_nest_end(nl_msg, attr); -ret = ovpn_nl_msg_send(dco, nl_msg, NULL, __func__); +ret = ovpn_nl_msg_send(dco, nl_msg, NULL, NULL, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -596,7 +598,7 @@ dco_new_key(dco_context_t *dco, unsigned int peerid, int keyid, nla_nest_end(nl_msg, attr); -ret = ovpn_nl_msg_send(dco, nl_msg, NULL, __func__); +ret = ovpn_nl_msg_send(dco, nl_msg, NULL, NULL, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -625,7 +627,7 @@ dco_set_peer(dco_context_t *dco, unsigned int peerid, keepalive_timeout); nla_nest_end(nl_msg, attr); -ret = ovpn_nl_msg_send(dco, nl_msg, NULL, __func__); +ret = ovpn_nl_msg_send(dco, nl_msg, NULL, NULL, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -706,7 +708,7 @@ ovpn_get_mcast_id(dco_context_t *dco) int ret = -EMSGSIZE; NLA_PUT_STRING(nl_msg, CTRL_ATTR_FAMILY_NAME, OVPN_NL_NAME); -ret = ovpn_nl_msg_send(dco, nl_msg, mcast_family_handler, __func__); +ret = ovpn_nl_msg_send(dco, nl_msg, mcast_family_handler, dco, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -819,18 +821,173 @@ dco_do_read(dco_context_t *dco) return ovpn_nl_recvmsgs(dco, __func__); } +static void +dco_update_peer_stat(struct context_2 *c2, struct nlattr *tb[], uint32_t id) +{ +if (tb[OVPN_GET_PEER_RESP_ATTR_LINK_RX_BYTES]) +{ +c2->dco_read_bytes = nla_get_u64(tb[OVPN_GET_PEER_RESP_ATTR_LINK_RX_BYTES]); +msg(D_DCO_DEBUG, "%s / dco_read_bytes: %lu", __func__, +c2->dco_read_bytes); +} +else +{ +msg(M_WARN, "%s: no link RX bytes provided in reply for peer %u", +__func__, id); +} + +if (tb[OVPN_GET_PEER_RESP_ATTR_LINK_TX_BYTES]) +{ +c2->dco_write_bytes = nla_get_u64(tb[OVPN_GET_PEER_RESP_ATTR_LINK_TX_BYTES]); +msg(D_DCO_DEBUG, "%s / dco_write_bytes: %lu", __func__, +c2->dco_write_bytes); +} +else +{ +msg(M_WARN, "%s: no link TX bytes provided in reply for peer %u", +
[Openvpn-devel] [PATCH] multi: don't call DCO APIs if DCO is disabled
The agreement with the DCO submodule is that no API should be called if DCO is actually disabled. For this reason, every invocation must happen only after having checked that dco_enabled() returns true. Add missing checks before invoking dco_get_peer_stats_multi() Reported-by: Lev Stipakov Signed-off-by: Antonio Quartulli --- src/openvpn/multi.c | 10 -- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index 53c17b3a..1f0a9c01 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -549,7 +549,10 @@ multi_del_iroutes(struct multi_context *m, static void setenv_stats(struct multi_context *m, struct context *c) { -dco_get_peer_stats_multi(&m->top.c1.tuntap->dco, m); +if (dco_enabled(&m->top.options)) +{ +dco_get_peer_stats_multi(&m->top.c1.tuntap->dco, m); +} setenv_counter(c->c2.es, "bytes_received", c->c2.link_read_bytes + c->c2.dco_read_bytes); setenv_counter(c->c2.es, "bytes_sent", c->c2.link_write_bytes + c->c2.dco_write_bytes); @@ -849,7 +852,10 @@ multi_print_status(struct multi_context *m, struct status_output *so, const int status_reset(so); -dco_get_peer_stats_multi(&m->top.c1.tuntap->dco, m); +if (dco_enabled(&m->top.options)) +{ +dco_get_peer_stats_multi(&m->top.c1.tuntap->dco, m); +} if (version == 1) { -- 2.39.2 ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] dco-linux: implement dco_get_peer_stats{, multi} API
With this API it is possible to retrieve the stats for a specific peer or for all peers and then update the userspace counters with the value reported by DCO. Change-Id: Ia3990b86b1be7ca844fb1674b39ce0d60528ccff Signed-off-by: Antonio Quartulli --- Pleas, use the latest ovpn-dco master branch! src/openvpn/dco_linux.c | 194 --- src/openvpn/ovpn_dco_linux.h | 14 ++- 2 files changed, 190 insertions(+), 18 deletions(-) diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c index 47961849..1f18fa81 100644 --- a/src/openvpn/dco_linux.c +++ b/src/openvpn/dco_linux.c @@ -41,6 +41,7 @@ #include "tun.h" #include "ssl.h" #include "fdmisc.h" +#include "multi.h" #include "ssl_verify.h" #include "ovpn_dco_linux.h" @@ -168,16 +169,17 @@ ovpn_nl_recvmsgs(dco_context_t *dco, const char *prefix) * @param dco The dco context to use * @param nl_msgthe message to use * @param cbAn optional callback if the caller expects an answer + * @param cb_argAn optional param to pass to the callback * @param prefixA prefix to report in the error message to give the user context * @return status of sending the message */ static int ovpn_nl_msg_send(dco_context_t *dco, struct nl_msg *nl_msg, ovpn_nl_cb cb, - const char *prefix) + void *cb_arg, const char *prefix) { dco->status = 1; -nl_cb_set(dco->nl_cb, NL_CB_VALID, NL_CB_CUSTOM, cb, dco); +nl_cb_set(dco->nl_cb, NL_CB_VALID, NL_CB_CUSTOM, cb, cb_arg); nl_send_auto(dco->nl_sock, nl_msg); while (dco->status == 1) @@ -268,7 +270,7 @@ dco_new_peer(dco_context_t *dco, unsigned int peerid, int sd, } nla_nest_end(nl_msg, attr); -ret = ovpn_nl_msg_send(dco, nl_msg, NULL, __func__); +ret = ovpn_nl_msg_send(dco, nl_msg, NULL, NULL, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -489,7 +491,7 @@ dco_swap_keys(dco_context_t *dco, unsigned int peerid) NLA_PUT_U32(nl_msg, OVPN_SWAP_KEYS_ATTR_PEER_ID, peerid); nla_nest_end(nl_msg, attr); -ret = ovpn_nl_msg_send(dco, nl_msg, NULL, __func__); +ret = ovpn_nl_msg_send(dco, nl_msg, NULL, NULL, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -513,7 +515,7 @@ dco_del_peer(dco_context_t *dco, unsigned int peerid) NLA_PUT_U32(nl_msg, OVPN_DEL_PEER_ATTR_PEER_ID, peerid); nla_nest_end(nl_msg, attr); -ret = ovpn_nl_msg_send(dco, nl_msg, NULL, __func__); +ret = ovpn_nl_msg_send(dco, nl_msg, NULL, NULL, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -539,7 +541,7 @@ dco_del_key(dco_context_t *dco, unsigned int peerid, NLA_PUT_U8(nl_msg, OVPN_DEL_KEY_ATTR_KEY_SLOT, slot); nla_nest_end(nl_msg, attr); -ret = ovpn_nl_msg_send(dco, nl_msg, NULL, __func__); +ret = ovpn_nl_msg_send(dco, nl_msg, NULL, NULL, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -596,7 +598,7 @@ dco_new_key(dco_context_t *dco, unsigned int peerid, int keyid, nla_nest_end(nl_msg, attr); -ret = ovpn_nl_msg_send(dco, nl_msg, NULL, __func__); +ret = ovpn_nl_msg_send(dco, nl_msg, NULL, NULL, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -625,7 +627,7 @@ dco_set_peer(dco_context_t *dco, unsigned int peerid, keepalive_timeout); nla_nest_end(nl_msg, attr); -ret = ovpn_nl_msg_send(dco, nl_msg, NULL, __func__); +ret = ovpn_nl_msg_send(dco, nl_msg, NULL, NULL, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -706,7 +708,7 @@ ovpn_get_mcast_id(dco_context_t *dco) int ret = -EMSGSIZE; NLA_PUT_STRING(nl_msg, CTRL_ATTR_FAMILY_NAME, OVPN_NL_NAME); -ret = ovpn_nl_msg_send(dco, nl_msg, mcast_family_handler, __func__); +ret = ovpn_nl_msg_send(dco, nl_msg, mcast_family_handler, dco, __func__); nla_put_failure: nlmsg_free(nl_msg); @@ -819,18 +821,184 @@ dco_do_read(dco_context_t *dco) return ovpn_nl_recvmsgs(dco, __func__); } +static void +dco_update_peer_stat(struct context_2 *c2, struct nlattr *tb[], uint32_t id) +{ +if (tb[OVPN_GET_PEER_RESP_ATTR_LINK_RX_BYTES]) +{ +c2->dco_read_bytes = nla_get_u64(tb[OVPN_GET_PEER_RESP_ATTR_LINK_RX_BYTES]); +msg(D_DCO_DEBUG, "%s / dco_read_bytes: %lu", __func__, +c2->dco_read_bytes); +} +else +{ +msg(M_WARN, "%s: no link RX bytes provided in reply for peer %u", +__func__, id); +} + +if (tb[OVPN_GET_PEER_RESP_ATTR_LINK_TX_BYTES]) +{ +c2->dco_write_bytes = nla_get_u64(tb[OVPN_GET_PEER_RESP_ATTR_LINK_TX_BYTES]); +msg(D_DCO_DEBUG, "%s / dco_write_bytes: %lu", __func__, +c2->dco_write_bytes); +} +else +{ +msg(M_WARN, "%s: no link TX bytes provided in reply for peer %u", +__func__, id); +} + +if (tb[OVPN_GET_PEER_RES
[Openvpn-devel] [PATCH] dco-linux: remove M_ERRNO flag when printing netlink error message
Netlink has its own error space and reports errors via the return value of its functions. For this reason remove the M_ERRNO flag when printing its errors. At the moment we get something like this: netlink reports error (-7): Invalid input data or parameter: Interrupted system call (errno=4) where the errno=4 (and its human readable representation) is a leftover from the previous recv() interrupted by a signal and it is totally unrelated to this netlink failure. Signed-off-by: Antonio Quartulli --- src/openvpn/dco_linux.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c index 98b2aae3..0e479beb 100644 --- a/src/openvpn/dco_linux.c +++ b/src/openvpn/dco_linux.c @@ -154,7 +154,7 @@ ovpn_nl_recvmsgs(dco_context_t *dco, const char *prefix) default: if (ret) { -msg(M_NONFATAL|M_ERRNO, "%s: netlink reports error (%d): %s", prefix, ret, nl_geterror(-ret)); +msg(M_NONFATAL, "%s: netlink reports error (%d): %s", prefix, ret, nl_geterror(-ret)); } break; } -- 2.39.2 ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v3] dco: print version to log if available
Ignore the last message - it was meant for another patch *shrug* On 09/03/2023 16:02, Antonio Quartulli wrote: This is being discussed on Gerrit at: https://gerrit.openvpn.net/c/openvpn/+/28 On 09/03/2023 14:14, Antonio Quartulli wrote: In order to provide better support in case of troubleshooting issues, it's important to know what exact DCO version is loaded on the user system. Therefore print the DCO version during bootup. For Windows and FreeBSD we currently implement a placeholder printing 'v0'. This should be improved with a follow-up patch. For Linux we directly fetch the module version from /sys and print something like: DCO version: 0.1.20230206-15-g580608ec7c59 Change-Id: Ie1f6fa5d12a473d353d84fd119c2430b638e8bcd Signed-off-by: Antonio Quartulli --- Changes from v1: * beautify usage of buf with some helpers * don't apply -1 to buffer capacity as fgets will already read one byte less than provided length and will add a null terminator there Changes from v2: * fix style of type returned by dco_version_string() (uncrustify doesn't work anymore on my box..due to incompatibler config) --- src/openvpn/dco.h | 15 +++ src/openvpn/dco_freebsd.c | 6 ++ src/openvpn/dco_linux.c | 27 +++ src/openvpn/dco_win.c | 6 ++ src/openvpn/openvpn.c | 2 ++ src/openvpn/options.c | 11 +++ src/openvpn/options.h | 2 ++ 7 files changed, 69 insertions(+) diff --git a/src/openvpn/dco.h b/src/openvpn/dco.h index 2fe671bf..96d95c21 100644 --- a/src/openvpn/dco.h +++ b/src/openvpn/dco.h @@ -58,6 +58,15 @@ struct tuntap; */ bool dco_available(int msglevel); + +/** + * Return a human readable string representing the DCO version + * + * @param gc the garbage collector to use for any dynamic allocation + * @return a pointer to the string (allocated via gc) containing the string + */ +const char *dco_version_string(struct gc_arena *gc); + /** * Check whether the options struct has any option that is not supported by * our current dco implementation. If so print a warning at warning level @@ -250,6 +259,12 @@ dco_available(int msglevel) return false; } +static inline const char * +dco_version_string(struct gc_arena *gc) +{ + return "not-compiled"; +} + static inline bool dco_check_option(int msglevel, const struct options *o) { diff --git a/src/openvpn/dco_freebsd.c b/src/openvpn/dco_freebsd.c index 92de5f04..cbd2ce20 100644 --- a/src/openvpn/dco_freebsd.c +++ b/src/openvpn/dco_freebsd.c @@ -614,6 +614,12 @@ out: return available; } +const char * +dco_version_string(struct gc_arena *gc) +{ + return "v0"; +} + void dco_event_set(dco_context_t *dco, struct event_set *es, void *arg) { diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c index 308abfc4..9f3531d5 100644 --- a/src/openvpn/dco_linux.c +++ b/src/openvpn/dco_linux.c @@ -841,9 +841,36 @@ dco_available(int msglevel) "Note: Kernel support for ovpn-dco missing, disabling data channel offload."); return false; } + return true; } +const char * +dco_version_string(struct gc_arena *gc) +{ + struct buffer out = alloc_buf_gc(256, gc); + FILE *fp = fopen("/sys/module/ovpn_dco_v2/version", "r"); + if (!fp) + { + return "N/A"; + } + + if (!fgets(BSTR(&out), BCAP(&out), fp)) + { + return "ERR"; + } + + /* remove potential newline at the end of the string */ + char *str = BSTR(&out); + char *nl = strchr(str, '\n'); + if (nl) + { + *nl = '\0'; + } + + return BSTR(&out); +} + void dco_event_set(dco_context_t *dco, struct event_set *es, void *arg) { diff --git a/src/openvpn/dco_win.c b/src/openvpn/dco_win.c index a805c2a0..26463b38 100644 --- a/src/openvpn/dco_win.c +++ b/src/openvpn/dco_win.c @@ -385,6 +385,12 @@ dco_available(int msglevel) return false; } +const char * +dco_version_string(struct gc_arena *gc) +{ + return "v0"; +} + int dco_do_read(dco_context_t *dco) { diff --git a/src/openvpn/openvpn.c b/src/openvpn/openvpn.c index cba58276..1aaddcdf 100644 --- a/src/openvpn/openvpn.c +++ b/src/openvpn/openvpn.c @@ -262,6 +262,8 @@ openvpn_main(int argc, char *argv[]) #endif show_library_versions(M_INFO); + show_dco_version(M_INFO); + /* misc stuff */ pre_setup(&c.options); diff --git a/src/openvpn/options.c b/src/openvpn/options.c index ce7bd0a7..8f0e2194 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -4816,6 +4816,16 @@ show_windows_version(const unsigned int flags) } #endif +void +show_dco_version(const unsigned int flags) +{ +#ifdef ENABLE_DCO + struct gc_arena gc = gc_new(); + msg(flags, "DCO version: %s", dco_version_string(&a
[Openvpn-devel] [PATCH v2] dco: don't use NetLink to exchange control packets
Using NetLink for control messages did not work out as it did lead to kernel side buffer congestion during heavy client activity. With this patch DCO will redirect control packets directly to the transport socket without altering them, so that userspace can happily process them as usual. Change-Id: Ia1297c3ae9a28b188ed21ad21ae96fff3d02ee4d [l...@openvpn.net: ensure win_dco flag is still exposed] Signed-off-by: Antonio Quartulli --- Changes from v1: * improved comments * improved commit message This patch was also reviewed and approved on gerrit at: https://gerrit.openvpn.net/c/openvpn/+/28 src/openvpn/dco.c| 12 - src/openvpn/dco.h| 16 -- src/openvpn/dco_freebsd.c| 10 src/openvpn/dco_freebsd.h| 2 - src/openvpn/dco_linux.c | 101 --- src/openvpn/dco_linux.h | 2 - src/openvpn/dco_win.c| 8 --- src/openvpn/forward.c| 63 +++--- src/openvpn/init.c | 3 +- src/openvpn/mtcp.c | 22 +--- src/openvpn/multi.c | 39 +- src/openvpn/ovpn_dco_linux.h | 16 +- src/openvpn/socket.c | 8 +-- src/openvpn/socket.h | 29 ++ 14 files changed, 35 insertions(+), 296 deletions(-) diff --git a/src/openvpn/dco.c b/src/openvpn/dco.c index b53332a8..308578b4 100644 --- a/src/openvpn/dco.c +++ b/src/openvpn/dco.c @@ -485,7 +485,6 @@ dco_p2p_add_new_peer(struct context *c) } c->c2.tls_multi->dco_peer_id = multi->peer_id; -c->c2.link_socket->dco_installed = true; return 0; } @@ -605,17 +604,6 @@ dco_multi_add_new_peer(struct multi_context *m, struct multi_instance *mi) c->c2.tls_multi->dco_peer_id = peer_id; -if (c->mode == CM_CHILD_TCP) -{ -multi_tcp_dereference_instance(m->mtcp, mi); -if (close(sd)) -{ -msg(D_DCO|M_ERRNO, "error closing TCP socket after DCO handover"); -} -c->c2.link_socket->dco_installed = true; -c->c2.link_socket->sd = SOCKET_UNDEFINED; -} - return 0; } diff --git a/src/openvpn/dco.h b/src/openvpn/dco.h index 18a9d78b..2fe671bf 100644 --- a/src/openvpn/dco.h +++ b/src/openvpn/dco.h @@ -127,15 +127,6 @@ void close_tun_dco(struct tuntap *tt, openvpn_net_ctx_t *ctx); */ int dco_do_read(dco_context_t *dco); -/** - * Write data to the DCO communication channel (control packet expected) - * - * @param dco the DCO context - * @param peer_id the ID of the peer to send the data to - * @param buf the buffer containing the data to send - */ -int dco_do_write(dco_context_t *dco, int peer_id, struct buffer *buf); - /** * Install a DCO in the main event loop */ @@ -301,13 +292,6 @@ dco_do_read(dco_context_t *dco) return 0; } -static inline int -dco_do_write(dco_context_t *dco, int peer_id, struct buffer *buf) -{ -ASSERT(false); -return 0; -} - static inline void dco_event_set(dco_context_t *dco, struct event_set *es, void *arg) { diff --git a/src/openvpn/dco_freebsd.c b/src/openvpn/dco_freebsd.c index cd4083c4..92de5f04 100644 --- a/src/openvpn/dco_freebsd.c +++ b/src/openvpn/dco_freebsd.c @@ -142,7 +142,6 @@ open_fd(dco_context_t *dco) { dco->open = true; } -dco->dco_packet_in = alloc_buf(PAGE_SIZE); return dco->fd; } @@ -560,15 +559,6 @@ dco_do_read(dco_context_t *dco) return 0; } -int -dco_do_write(dco_context_t *dco, int peer_id, struct buffer *buf) -{ -/* Control packets are passed through the socket, so this should never get - * called. See should_use_dco_socket(). */ -ASSERT(0); -return -EINVAL; -} - bool dco_available(int msglevel) { diff --git a/src/openvpn/dco_freebsd.h b/src/openvpn/dco_freebsd.h index 970beca0..a07f9b69 100644 --- a/src/openvpn/dco_freebsd.h +++ b/src/openvpn/dco_freebsd.h @@ -51,8 +51,6 @@ typedef struct dco_context { char ifname[IFNAMSIZ]; -struct buffer dco_packet_in; - int dco_message_type; int dco_message_peer_id; int dco_del_peer_reason; diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c index c84f9cfe..308abfc4 100644 --- a/src/openvpn/dco_linux.c +++ b/src/openvpn/dco_linux.c @@ -434,24 +434,6 @@ ovpn_dco_register(dco_context_t *dco) { msg(M_ERR, "%s: failed to join groups: %d", __func__, ret); } - -/* Register for non-data packets that ovpn-dco may receive. They will be - * forwarded to userspace - */ -struct nl_msg *nl_msg = ovpn_dco_nlmsg_create(dco, OVPN_CMD_REGISTER_PACKET); -if (!nl_msg) -{ -msg(M_ERR, "%s: cannot allocate message to register for control packets", -__func__); -} - -ret = ovpn_nl_msg_send(dco, nl_msg, NULL, __func__); -if (ret) -{ -msg(M_ERR, "%s: failed to register for control packets: %d", __func__, -ret); -
Re: [Openvpn-devel] [PATCH v3] dco: print version to log if available
This is being discussed on Gerrit at: https://gerrit.openvpn.net/c/openvpn/+/28 On 09/03/2023 14:14, Antonio Quartulli wrote: In order to provide better support in case of troubleshooting issues, it's important to know what exact DCO version is loaded on the user system. Therefore print the DCO version during bootup. For Windows and FreeBSD we currently implement a placeholder printing 'v0'. This should be improved with a follow-up patch. For Linux we directly fetch the module version from /sys and print something like: DCO version: 0.1.20230206-15-g580608ec7c59 Change-Id: Ie1f6fa5d12a473d353d84fd119c2430b638e8bcd Signed-off-by: Antonio Quartulli --- Changes from v1: * beautify usage of buf with some helpers * don't apply -1 to buffer capacity as fgets will already read one byte less than provided length and will add a null terminator there Changes from v2: * fix style of type returned by dco_version_string() (uncrustify doesn't work anymore on my box..due to incompatibler config) --- src/openvpn/dco.h | 15 +++ src/openvpn/dco_freebsd.c | 6 ++ src/openvpn/dco_linux.c | 27 +++ src/openvpn/dco_win.c | 6 ++ src/openvpn/openvpn.c | 2 ++ src/openvpn/options.c | 11 +++ src/openvpn/options.h | 2 ++ 7 files changed, 69 insertions(+) diff --git a/src/openvpn/dco.h b/src/openvpn/dco.h index 2fe671bf..96d95c21 100644 --- a/src/openvpn/dco.h +++ b/src/openvpn/dco.h @@ -58,6 +58,15 @@ struct tuntap; */ bool dco_available(int msglevel); + +/** + * Return a human readable string representing the DCO version + * + * @param gcthe garbage collector to use for any dynamic allocation + * @return a pointer to the string (allocated via gc) containing the string + */ +const char *dco_version_string(struct gc_arena *gc); + /** * Check whether the options struct has any option that is not supported by * our current dco implementation. If so print a warning at warning level @@ -250,6 +259,12 @@ dco_available(int msglevel) return false; } +static inline const char * +dco_version_string(struct gc_arena *gc) +{ +return "not-compiled"; +} + static inline bool dco_check_option(int msglevel, const struct options *o) { diff --git a/src/openvpn/dco_freebsd.c b/src/openvpn/dco_freebsd.c index 92de5f04..cbd2ce20 100644 --- a/src/openvpn/dco_freebsd.c +++ b/src/openvpn/dco_freebsd.c @@ -614,6 +614,12 @@ out: return available; } +const char * +dco_version_string(struct gc_arena *gc) +{ +return "v0"; +} + void dco_event_set(dco_context_t *dco, struct event_set *es, void *arg) { diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c index 308abfc4..9f3531d5 100644 --- a/src/openvpn/dco_linux.c +++ b/src/openvpn/dco_linux.c @@ -841,9 +841,36 @@ dco_available(int msglevel) "Note: Kernel support for ovpn-dco missing, disabling data channel offload."); return false; } + return true; } +const char * +dco_version_string(struct gc_arena *gc) +{ +struct buffer out = alloc_buf_gc(256, gc); +FILE *fp = fopen("/sys/module/ovpn_dco_v2/version", "r"); +if (!fp) +{ +return "N/A"; +} + +if (!fgets(BSTR(&out), BCAP(&out), fp)) +{ +return "ERR"; +} + +/* remove potential newline at the end of the string */ +char *str = BSTR(&out); +char *nl = strchr(str, '\n'); +if (nl) +{ +*nl = '\0'; +} + +return BSTR(&out); +} + void dco_event_set(dco_context_t *dco, struct event_set *es, void *arg) { diff --git a/src/openvpn/dco_win.c b/src/openvpn/dco_win.c index a805c2a0..26463b38 100644 --- a/src/openvpn/dco_win.c +++ b/src/openvpn/dco_win.c @@ -385,6 +385,12 @@ dco_available(int msglevel) return false; } +const char * +dco_version_string(struct gc_arena *gc) +{ +return "v0"; +} + int dco_do_read(dco_context_t *dco) { diff --git a/src/openvpn/openvpn.c b/src/openvpn/openvpn.c index cba58276..1aaddcdf 100644 --- a/src/openvpn/openvpn.c +++ b/src/openvpn/openvpn.c @@ -262,6 +262,8 @@ openvpn_main(int argc, char *argv[]) #endif show_library_versions(M_INFO); +show_dco_version(M_INFO); + /* misc stuff */ pre_setup(&c.options); diff --git a/src/openvpn/options.c b/src/openvpn/options.c index ce7bd0a7..8f0e2194 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -4816,6 +4816,16 @@ show_windows_version(const unsigned int flags) } #endif +void +show_dco_version(const unsigned int flags) +{ +#ifdef ENABLE_DCO +struct gc_arena gc = gc_new(); +msg(flags, "DCO version: %s", dco_version_string(&gc)); +gc_free(&gc); +#endif +} + void show_library_versions(const unsig
[Openvpn-devel] [PATCH v3] dco: print version to log if available
In order to provide better support in case of troubleshooting issues, it's important to know what exact DCO version is loaded on the user system. Therefore print the DCO version during bootup. For Windows and FreeBSD we currently implement a placeholder printing 'v0'. This should be improved with a follow-up patch. For Linux we directly fetch the module version from /sys and print something like: DCO version: 0.1.20230206-15-g580608ec7c59 Change-Id: Ie1f6fa5d12a473d353d84fd119c2430b638e8bcd Signed-off-by: Antonio Quartulli --- Changes from v1: * beautify usage of buf with some helpers * don't apply -1 to buffer capacity as fgets will already read one byte less than provided length and will add a null terminator there Changes from v2: * fix style of type returned by dco_version_string() (uncrustify doesn't work anymore on my box..due to incompatibler config) --- src/openvpn/dco.h | 15 +++ src/openvpn/dco_freebsd.c | 6 ++ src/openvpn/dco_linux.c | 27 +++ src/openvpn/dco_win.c | 6 ++ src/openvpn/openvpn.c | 2 ++ src/openvpn/options.c | 11 +++ src/openvpn/options.h | 2 ++ 7 files changed, 69 insertions(+) diff --git a/src/openvpn/dco.h b/src/openvpn/dco.h index 2fe671bf..96d95c21 100644 --- a/src/openvpn/dco.h +++ b/src/openvpn/dco.h @@ -58,6 +58,15 @@ struct tuntap; */ bool dco_available(int msglevel); + +/** + * Return a human readable string representing the DCO version + * + * @param gcthe garbage collector to use for any dynamic allocation + * @return a pointer to the string (allocated via gc) containing the string + */ +const char *dco_version_string(struct gc_arena *gc); + /** * Check whether the options struct has any option that is not supported by * our current dco implementation. If so print a warning at warning level @@ -250,6 +259,12 @@ dco_available(int msglevel) return false; } +static inline const char * +dco_version_string(struct gc_arena *gc) +{ +return "not-compiled"; +} + static inline bool dco_check_option(int msglevel, const struct options *o) { diff --git a/src/openvpn/dco_freebsd.c b/src/openvpn/dco_freebsd.c index 92de5f04..cbd2ce20 100644 --- a/src/openvpn/dco_freebsd.c +++ b/src/openvpn/dco_freebsd.c @@ -614,6 +614,12 @@ out: return available; } +const char * +dco_version_string(struct gc_arena *gc) +{ +return "v0"; +} + void dco_event_set(dco_context_t *dco, struct event_set *es, void *arg) { diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c index 308abfc4..9f3531d5 100644 --- a/src/openvpn/dco_linux.c +++ b/src/openvpn/dco_linux.c @@ -841,9 +841,36 @@ dco_available(int msglevel) "Note: Kernel support for ovpn-dco missing, disabling data channel offload."); return false; } + return true; } +const char * +dco_version_string(struct gc_arena *gc) +{ +struct buffer out = alloc_buf_gc(256, gc); +FILE *fp = fopen("/sys/module/ovpn_dco_v2/version", "r"); +if (!fp) +{ +return "N/A"; +} + +if (!fgets(BSTR(&out), BCAP(&out), fp)) +{ +return "ERR"; +} + +/* remove potential newline at the end of the string */ +char *str = BSTR(&out); +char *nl = strchr(str, '\n'); +if (nl) +{ +*nl = '\0'; +} + +return BSTR(&out); +} + void dco_event_set(dco_context_t *dco, struct event_set *es, void *arg) { diff --git a/src/openvpn/dco_win.c b/src/openvpn/dco_win.c index a805c2a0..26463b38 100644 --- a/src/openvpn/dco_win.c +++ b/src/openvpn/dco_win.c @@ -385,6 +385,12 @@ dco_available(int msglevel) return false; } +const char * +dco_version_string(struct gc_arena *gc) +{ +return "v0"; +} + int dco_do_read(dco_context_t *dco) { diff --git a/src/openvpn/openvpn.c b/src/openvpn/openvpn.c index cba58276..1aaddcdf 100644 --- a/src/openvpn/openvpn.c +++ b/src/openvpn/openvpn.c @@ -262,6 +262,8 @@ openvpn_main(int argc, char *argv[]) #endif show_library_versions(M_INFO); +show_dco_version(M_INFO); + /* misc stuff */ pre_setup(&c.options); diff --git a/src/openvpn/options.c b/src/openvpn/options.c index ce7bd0a7..8f0e2194 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -4816,6 +4816,16 @@ show_windows_version(const unsigned int flags) } #endif +void +show_dco_version(const unsigned int flags) +{ +#ifdef ENABLE_DCO +struct gc_arena gc = gc_new(); +msg(flags, "DCO version: %s", dco_version_string(&gc)); +gc_free(&gc); +#endif +} + void show_library_versions(const unsigned int flags) { @@ -4839,6 +4849,7 @@ usage_version(void) #ifdef _WIN32 show_windows_version( M_INFO|M_NOPREFIX ); #endif +show_dco_version(M_INFO | M_NOPREFIX); msg(M_INFO|M_NOP
Re: [Openvpn-devel] [PATCH] Ensure n = 2 is set in key2 structer in tls_crypt_v2_unwrap_client_key
Hi, On 09/03/2023 13:00, Arne Schwabe wrote: The ASSERT in xor_key2 assumes that all methods that load a key2 struct correctly set n=2. However, tls_crypt_v2_unwrap_client_key loads a key without setting n = 2, trigerring the assert. trigerring -> triggering Closes and reported in https://github.com/OpenVPN/openvpn/issues/272 Change-Id: Iaeb163d83b95818e0b26faf9d25e7737dc8ecb23 Signed-off-by: Arne Schwabe I can easily reproduce the issue ad verify that indeed the patch fixes it. By checking the code I can see that we set 'n' whenever we initialize the keys member. However, this was not happening in tls_crypt_v2_unwrap_client_key() thus leading to the assert being triggered. Acked-by: Antonio Quartulli --- src/openvpn/tls_crypt.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/openvpn/tls_crypt.c b/src/openvpn/tls_crypt.c index 8882d5de0..4f22f8af7 100644 --- a/src/openvpn/tls_crypt.c +++ b/src/openvpn/tls_crypt.c @@ -533,6 +533,7 @@ tls_crypt_v2_unwrap_client_key(struct key2 *client_key, struct buffer *metadata, } memcpy(&client_key->keys, BPTR(&plaintext), sizeof(client_key->keys)); ASSERT(buf_advance(&plaintext, sizeof(client_key->keys))); +client_key->n = 2; if (!buf_copy(metadata, &plaintext)) { -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH v2] dco: print version to log if available
In order to provide better support in case of troubleshooting issues, it's important to know what exact DCO version is loaded on the user system. Therefore print the DCO version during bootup. For Windows and FreeBSD we currently implement a placeholder printing 'v0'. This should be improved with a follow-up patch. For Linux we directly fetch the module version from /sys and print something like: DCO version: 0.1.20230206-15-g580608ec7c59 Change-Id: Ie1f6fa5d12a473d353d84fd119c2430b638e8bcd Signed-off-by: Antonio Quartulli --- Changes from v1: * beautify usage of buf with some helpers * don't apply -1 to buffer capacity as fgets will already read one byte less than provided length and will add a null terminator there --- src/openvpn/dco.h | 15 +++ src/openvpn/dco_freebsd.c | 6 ++ src/openvpn/dco_linux.c | 27 +++ src/openvpn/dco_win.c | 6 ++ src/openvpn/openvpn.c | 2 ++ src/openvpn/options.c | 11 +++ src/openvpn/options.h | 2 ++ 7 files changed, 69 insertions(+) diff --git a/src/openvpn/dco.h b/src/openvpn/dco.h index 2fe671bf..96d95c21 100644 --- a/src/openvpn/dco.h +++ b/src/openvpn/dco.h @@ -58,6 +58,15 @@ struct tuntap; */ bool dco_available(int msglevel); + +/** + * Return a human readable string representing the DCO version + * + * @param gcthe garbage collector to use for any dynamic allocation + * @return a pointer to the string (allocated via gc) containing the string + */ +const char *dco_version_string(struct gc_arena *gc); + /** * Check whether the options struct has any option that is not supported by * our current dco implementation. If so print a warning at warning level @@ -250,6 +259,12 @@ dco_available(int msglevel) return false; } +static inline const char * +dco_version_string(struct gc_arena *gc) +{ +return "not-compiled"; +} + static inline bool dco_check_option(int msglevel, const struct options *o) { diff --git a/src/openvpn/dco_freebsd.c b/src/openvpn/dco_freebsd.c index 92de5f04..cbd2ce20 100644 --- a/src/openvpn/dco_freebsd.c +++ b/src/openvpn/dco_freebsd.c @@ -614,6 +614,12 @@ out: return available; } +const char * +dco_version_string(struct gc_arena *gc) +{ +return "v0"; +} + void dco_event_set(dco_context_t *dco, struct event_set *es, void *arg) { diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c index 308abfc4..00d32d05 100644 --- a/src/openvpn/dco_linux.c +++ b/src/openvpn/dco_linux.c @@ -841,9 +841,36 @@ dco_available(int msglevel) "Note: Kernel support for ovpn-dco missing, disabling data channel offload."); return false; } + return true; } +const char +*dco_version_string(struct gc_arena *gc) +{ +struct buffer out = alloc_buf_gc(256, gc); +FILE *fp = fopen("/sys/module/ovpn_dco_v2/version", "r"); +if (!fp) +{ +return "N/A"; +} + +if (!fgets(BSTR(&out), BCAP(&out), fp)) +{ +return "ERR"; +} + +/* remove potential newline at the end of the string */ +char *str = BSTR(&out); +char *nl = strchr(str, '\n'); +if (nl) +{ +*nl = '\0'; +} + +return BSTR(&out); +} + void dco_event_set(dco_context_t *dco, struct event_set *es, void *arg) { diff --git a/src/openvpn/dco_win.c b/src/openvpn/dco_win.c index a805c2a0..26463b38 100644 --- a/src/openvpn/dco_win.c +++ b/src/openvpn/dco_win.c @@ -385,6 +385,12 @@ dco_available(int msglevel) return false; } +const char * +dco_version_string(struct gc_arena *gc) +{ +return "v0"; +} + int dco_do_read(dco_context_t *dco) { diff --git a/src/openvpn/openvpn.c b/src/openvpn/openvpn.c index cba58276..1aaddcdf 100644 --- a/src/openvpn/openvpn.c +++ b/src/openvpn/openvpn.c @@ -262,6 +262,8 @@ openvpn_main(int argc, char *argv[]) #endif show_library_versions(M_INFO); +show_dco_version(M_INFO); + /* misc stuff */ pre_setup(&c.options); diff --git a/src/openvpn/options.c b/src/openvpn/options.c index ce7bd0a7..8f0e2194 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -4816,6 +4816,16 @@ show_windows_version(const unsigned int flags) } #endif +void +show_dco_version(const unsigned int flags) +{ +#ifdef ENABLE_DCO +struct gc_arena gc = gc_new(); +msg(flags, "DCO version: %s", dco_version_string(&gc)); +gc_free(&gc); +#endif +} + void show_library_versions(const unsigned int flags) { @@ -4839,6 +4849,7 @@ usage_version(void) #ifdef _WIN32 show_windows_version( M_INFO|M_NOPREFIX ); #endif +show_dco_version(M_INFO | M_NOPREFIX); msg(M_INFO|M_NOPREFIX, "Originally developed by James Yonan"); msg(M_INFO|M_NOPREFIX, "Copyright (C) 2002-2023 OpenVPN Inc "); #ifndef ENABLE
Re: [Openvpn-devel] [PATCH] dco: print FreeBSD version
Hi, On 09/03/2023 13:13, Kristof Provost via Openvpn-devel wrote: This should use BSTR(data) instead. I copied Antonio’s code here, but that is better, so I’ll fix that too. dang! with one email Arne spoiled two patches! Cheers, -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] dco: print version to log if available
Hi, On 09/03/2023 10:03, Kristof Provost wrote: On 9 Mar 2023, at 9:57, Antonio Quartulli wrote: On 09/03/2023 09:36, Kristof Provost wrote: On 9 Mar 2023, at 1:52, Antonio Quartulli wrote: In order to provide better support in case of troubleshooting issues, it's important to know what exact DCO version is loaded on the user system. Therefore print the DCO version during bootup. For Windows and FreeBSD we currently implement a placeholder printing 'v0'. This should be improved with a follow-up patch. FreeBSD is a bit special in this regard, because the DCO driver is part of the base operating system and not an add-on as in Linux and Windows. Arguably the thing to do for FreeBSD is to log the OS version. Isn't it possible to install DCO "out-of-tree" (as we can do on Linux)? With some effort it could be done, but any version (in the source tree) before DCO was actually imported isn’t going to work. I made a few (small) tweaks to the network stack to make things work. Realistically this isn’t something people will do, and if they do decide to the usual warranty applies (i.e. they get to keep all of the pieces it breaks into). Yeah, same problem on Linux. This is why we have to ship a compatibility layer in order to compile DCO on older kernels. On Linux, though, compiling a new module on an older kernel is pretty common, this is why we have put some effort there too. However, what you said will also apply to linux in the future: we will also need to report the kernel version, *if* DCO is compiled in (to be implemented). This said, reporting the kernel/os version is absolutely appropriate if that's what identifies the DCO version. There’s no API in FreeBSD’s DCO to identify its version either, which is another reason to just use the OS version. yup, makes sense! Is that something you could implement in dco_freebsd.c? Sure. I’ll send a patch later today. Thanks! Cheers, Best regards, Kristof -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] dco: print version to log if available
Hi, On 09/03/2023 09:36, Kristof Provost wrote: On 9 Mar 2023, at 1:52, Antonio Quartulli wrote: In order to provide better support in case of troubleshooting issues, it's important to know what exact DCO version is loaded on the user system. Therefore print the DCO version during bootup. For Windows and FreeBSD we currently implement a placeholder printing 'v0'. This should be improved with a follow-up patch. FreeBSD is a bit special in this regard, because the DCO driver is part of the base operating system and not an add-on as in Linux and Windows. Arguably the thing to do for FreeBSD is to log the OS version. Isn't it possible to install DCO "out-of-tree" (as we can do on Linux)? However, what you said will also apply to linux in the future: we will also need to report the kernel version, *if* DCO is compiled in (to be implemented). This said, reporting the kernel/os version is absolutely appropriate if that's what identifies the DCO version. Is that something you could implement in dco_freebsd.c? Cheers, Kristof -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] dco: print version to log if available
In order to provide better support in case of troubleshooting issues, it's important to know what exact DCO version is loaded on the user system. Therefore print the DCO version during bootup. For Windows and FreeBSD we currently implement a placeholder printing 'v0'. This should be improved with a follow-up patch. For Linux we directly fetch the module version from /sys and print something like: DCO version: 0.1.20230206-15-g580608ec7c59 Cc: Lev Stipakov Cc: Kristof Provost Change-Id: Ie1f6fa5d12a473d353d84fd119c2430b638e8bcd Signed-off-by: Antonio Quartulli --- src/openvpn/dco.h | 15 +++ src/openvpn/dco_freebsd.c | 6 ++ src/openvpn/dco_linux.c | 30 ++ src/openvpn/dco_win.c | 6 ++ src/openvpn/openvpn.c | 2 ++ src/openvpn/options.c | 11 +++ src/openvpn/options.h | 2 ++ 7 files changed, 72 insertions(+) diff --git a/src/openvpn/dco.h b/src/openvpn/dco.h index 2fe671bf..96d95c21 100644 --- a/src/openvpn/dco.h +++ b/src/openvpn/dco.h @@ -58,6 +58,15 @@ struct tuntap; */ bool dco_available(int msglevel); + +/** + * Return a human readable string representing the DCO version + * + * @param gcthe garbage collector to use for any dynamic allocation + * @return a pointer to the string (allocated via gc) containing the string + */ +const char *dco_version_string(struct gc_arena *gc); + /** * Check whether the options struct has any option that is not supported by * our current dco implementation. If so print a warning at warning level @@ -250,6 +259,12 @@ dco_available(int msglevel) return false; } +static inline const char * +dco_version_string(struct gc_arena *gc) +{ +return "not-compiled"; +} + static inline bool dco_check_option(int msglevel, const struct options *o) { diff --git a/src/openvpn/dco_freebsd.c b/src/openvpn/dco_freebsd.c index 92de5f04..cbd2ce20 100644 --- a/src/openvpn/dco_freebsd.c +++ b/src/openvpn/dco_freebsd.c @@ -614,6 +614,12 @@ out: return available; } +const char * +dco_version_string(struct gc_arena *gc) +{ +return "v0"; +} + void dco_event_set(dco_context_t *dco, struct event_set *es, void *arg) { diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c index 308abfc4..c9cd9df1 100644 --- a/src/openvpn/dco_linux.c +++ b/src/openvpn/dco_linux.c @@ -841,9 +841,39 @@ dco_available(int msglevel) "Note: Kernel support for ovpn-dco missing, disabling data channel offload."); return false; } + return true; } +const char +*dco_version_string(struct gc_arena *gc) +{ +struct buffer out = alloc_buf_gc(256, gc); +FILE *fp = fopen("/sys/module/ovpn_dco_v2/version", "r"); +if (!fp) +{ +buf_printf(&out, "N/A"); +goto out; +} + +if (!fgets((char *)buf_bptr(&out), out.capacity - 1, fp)) +{ +buf_printf(&out, "ERR"); +goto out; +} + +/* remove potential newline at the end of the string */ +char *str = (char *)buf_bptr(&out); +char *nl = strchr(str, '\n'); +if (nl) +{ +*nl = '\0'; +} + +out: +return (char *)out.data; +} + void dco_event_set(dco_context_t *dco, struct event_set *es, void *arg) { diff --git a/src/openvpn/dco_win.c b/src/openvpn/dco_win.c index a805c2a0..26463b38 100644 --- a/src/openvpn/dco_win.c +++ b/src/openvpn/dco_win.c @@ -385,6 +385,12 @@ dco_available(int msglevel) return false; } +const char * +dco_version_string(struct gc_arena *gc) +{ +return "v0"; +} + int dco_do_read(dco_context_t *dco) { diff --git a/src/openvpn/openvpn.c b/src/openvpn/openvpn.c index cba58276..1aaddcdf 100644 --- a/src/openvpn/openvpn.c +++ b/src/openvpn/openvpn.c @@ -262,6 +262,8 @@ openvpn_main(int argc, char *argv[]) #endif show_library_versions(M_INFO); +show_dco_version(M_INFO); + /* misc stuff */ pre_setup(&c.options); diff --git a/src/openvpn/options.c b/src/openvpn/options.c index ce7bd0a7..8f0e2194 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -4816,6 +4816,16 @@ show_windows_version(const unsigned int flags) } #endif +void +show_dco_version(const unsigned int flags) +{ +#ifdef ENABLE_DCO +struct gc_arena gc = gc_new(); +msg(flags, "DCO version: %s", dco_version_string(&gc)); +gc_free(&gc); +#endif +} + void show_library_versions(const unsigned int flags) { @@ -4839,6 +4849,7 @@ usage_version(void) #ifdef _WIN32 show_windows_version( M_INFO|M_NOPREFIX ); #endif +show_dco_version(M_INFO | M_NOPREFIX); msg(M_INFO|M_NOPREFIX, "Originally developed by James Yonan"); msg(M_INFO|M_NOPREFIX, "Copyright (C) 2002-2023 OpenVPN Inc "); #ifndef ENABLE_SMALL diff --git a/src/openvpn/options.h b/src/openvpn/options
[Openvpn-devel] [PATCH] dco: don't use NetLink to exchange control packets
Using NetLink has proved to be overkill and performance critical. The amount of control traffic can also easily overrun the NetLink buffer when a server has enough clients connected. Stop using NetLink to send/receive control packets and just use the transport socket as if DCO was not there at all. Under the hood DCO will redirect control packets to the transport socket without altering them, so that userspace can happily process them as usual. Change-Id: Ia1297c3ae9a28b188ed21ad21ae96fff3d02ee4d [l...@openvpn.net: ensure win_dco flag is still exposed] Signed-off-by: Antonio Quartulli --- src/openvpn/dco.c| 12 - src/openvpn/dco.h| 16 -- src/openvpn/dco_freebsd.c| 10 src/openvpn/dco_freebsd.h| 2 - src/openvpn/dco_linux.c | 101 --- src/openvpn/dco_linux.h | 2 - src/openvpn/dco_win.c| 8 --- src/openvpn/forward.c| 63 +++--- src/openvpn/init.c | 3 +- src/openvpn/mtcp.c | 22 +--- src/openvpn/multi.c | 39 +- src/openvpn/ovpn_dco_linux.h | 16 +- src/openvpn/socket.c | 8 +-- src/openvpn/socket.h | 22 14 files changed, 29 insertions(+), 295 deletions(-) diff --git a/src/openvpn/dco.c b/src/openvpn/dco.c index b53332a8..308578b4 100644 --- a/src/openvpn/dco.c +++ b/src/openvpn/dco.c @@ -485,7 +485,6 @@ dco_p2p_add_new_peer(struct context *c) } c->c2.tls_multi->dco_peer_id = multi->peer_id; -c->c2.link_socket->dco_installed = true; return 0; } @@ -605,17 +604,6 @@ dco_multi_add_new_peer(struct multi_context *m, struct multi_instance *mi) c->c2.tls_multi->dco_peer_id = peer_id; -if (c->mode == CM_CHILD_TCP) -{ -multi_tcp_dereference_instance(m->mtcp, mi); -if (close(sd)) -{ -msg(D_DCO|M_ERRNO, "error closing TCP socket after DCO handover"); -} -c->c2.link_socket->dco_installed = true; -c->c2.link_socket->sd = SOCKET_UNDEFINED; -} - return 0; } diff --git a/src/openvpn/dco.h b/src/openvpn/dco.h index 18a9d78b..2fe671bf 100644 --- a/src/openvpn/dco.h +++ b/src/openvpn/dco.h @@ -127,15 +127,6 @@ void close_tun_dco(struct tuntap *tt, openvpn_net_ctx_t *ctx); */ int dco_do_read(dco_context_t *dco); -/** - * Write data to the DCO communication channel (control packet expected) - * - * @param dco the DCO context - * @param peer_id the ID of the peer to send the data to - * @param buf the buffer containing the data to send - */ -int dco_do_write(dco_context_t *dco, int peer_id, struct buffer *buf); - /** * Install a DCO in the main event loop */ @@ -301,13 +292,6 @@ dco_do_read(dco_context_t *dco) return 0; } -static inline int -dco_do_write(dco_context_t *dco, int peer_id, struct buffer *buf) -{ -ASSERT(false); -return 0; -} - static inline void dco_event_set(dco_context_t *dco, struct event_set *es, void *arg) { diff --git a/src/openvpn/dco_freebsd.c b/src/openvpn/dco_freebsd.c index cd4083c4..92de5f04 100644 --- a/src/openvpn/dco_freebsd.c +++ b/src/openvpn/dco_freebsd.c @@ -142,7 +142,6 @@ open_fd(dco_context_t *dco) { dco->open = true; } -dco->dco_packet_in = alloc_buf(PAGE_SIZE); return dco->fd; } @@ -560,15 +559,6 @@ dco_do_read(dco_context_t *dco) return 0; } -int -dco_do_write(dco_context_t *dco, int peer_id, struct buffer *buf) -{ -/* Control packets are passed through the socket, so this should never get - * called. See should_use_dco_socket(). */ -ASSERT(0); -return -EINVAL; -} - bool dco_available(int msglevel) { diff --git a/src/openvpn/dco_freebsd.h b/src/openvpn/dco_freebsd.h index 970beca0..a07f9b69 100644 --- a/src/openvpn/dco_freebsd.h +++ b/src/openvpn/dco_freebsd.h @@ -51,8 +51,6 @@ typedef struct dco_context { char ifname[IFNAMSIZ]; -struct buffer dco_packet_in; - int dco_message_type; int dco_message_peer_id; int dco_del_peer_reason; diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c index c84f9cfe..308abfc4 100644 --- a/src/openvpn/dco_linux.c +++ b/src/openvpn/dco_linux.c @@ -434,24 +434,6 @@ ovpn_dco_register(dco_context_t *dco) { msg(M_ERR, "%s: failed to join groups: %d", __func__, ret); } - -/* Register for non-data packets that ovpn-dco may receive. They will be - * forwarded to userspace - */ -struct nl_msg *nl_msg = ovpn_dco_nlmsg_create(dco, OVPN_CMD_REGISTER_PACKET); -if (!nl_msg) -{ -msg(M_ERR, "%s: cannot allocate message to register for control packets", -__func__); -} - -ret = ovpn_nl_msg_send(dco, nl_msg, NULL, __func__); -if (ret) -{ -msg(M_ERR, "%s: failed to register for control packets: %d", __func__, -ret); -
Re: [Openvpn-devel] [PATCH] Set netlink socket to be non-blocking
Hi, On 08/03/2023 16:19, Arne Schwabe wrote: Even though we use select/poll to explicitly query when the nextlink nextlink -> netlink socket is ready for read, sometimes we end up reading from the socket when it is not ready to read and then the process hangs for several seoneds (20-30s). Avoid this situation by setting the socket to be non-blocking, so we get a status in this case that allows us to continue. Change-Id: I35447c23a9350176007df5455bf9451021e9856d Signed-off-by: Arne Schwabe Well spotted! Acked-by: Antonio Quartulli --- src/openvpn/dco_linux.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c index 1a6566aad..6f1b999bb 100644 --- a/src/openvpn/dco_linux.c +++ b/src/openvpn/dco_linux.c @@ -359,7 +359,9 @@ ovpn_dco_init_netlink(dco_context_t *dco) nl_geterror(ret)); } +/* set close on exec and non-block on the netlink socket */ set_cloexec(nl_socket_get_fd(dco->nl_sock)); +set_nonblock(nl_socket_get_fd(dco->nl_sock)); dco->nl_cb = nl_cb_alloc(NL_CB_DEFAULT); if (!dco->nl_cb) -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH v2] Avoid warning about missing braces when initialising key struct
Signed-off-by: Antonio Quartulli --- As concluded on IRC, this version does what we want it to do. We also quickly tested with some sample program to make sure we weren't making this up. src/openvpn/tls_crypt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/openvpn/tls_crypt.c b/src/openvpn/tls_crypt.c index 81098355..3b68d186 100644 --- a/src/openvpn/tls_crypt.c +++ b/src/openvpn/tls_crypt.c @@ -348,7 +348,7 @@ tls_crypt_v2_init_client_key(struct key_ctx_bi *key, struct key2 *original_key, msg(M_FATAL, "ERROR: invalid tls-crypt-v2 client key format"); } -struct key2 key2 = { .n = 2, .keys = { 0 } }; +struct key2 key2 = { .n = 2 }; if (!buf_read(&client_key, &key2.keys, sizeof(key2.keys))) { msg(M_FATAL, "ERROR: not enough data in tls-crypt-v2 client key"); -- 2.39.2 ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Avoid warning about missing braces when initialising key struct
Hi, On 08/03/2023 00:57, Arne Schwabe wrote: This avoids the warning from gcc about initialising the key2 struct Change-Id: Ia73d24923b1efd99263f33ce13d90e04b59bd980 Signed-off-by: Arne Schwabe --- src/openvpn/tls_crypt.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/openvpn/tls_crypt.c b/src/openvpn/tls_crypt.c index 81098355e..e84d979e9 100644 --- a/src/openvpn/tls_crypt.c +++ b/src/openvpn/tls_crypt.c @@ -348,7 +348,8 @@ tls_crypt_v2_init_client_key(struct key_ctx_bi *key, struct key2 *original_key, msg(M_FATAL, "ERROR: invalid tls-crypt-v2 client key format"); } -struct key2 key2 = { .n = 2, .keys = { 0 } }; +struct key2 key2 = { .n = 2, .keys = {{{ 0 }}} }; why not just struct key2 key2 = { .n = 2 } ? Cheers, + if (!buf_read(&client_key, &key2.keys, sizeof(key2.keys))) { msg(M_FATAL, "ERROR: not enough data in tls-crypt-v2 client key"); -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] dco: define OVPN_DEL_PEER_REASON_TRANSPORT_DISCONNECT on FreeBSD
Hi, On 03/03/2023 12:27, Antonio Quartulli wrote: Hi, On 03/03/2023 12:05, Kristof Provost via Openvpn-devel wrote: From: Kristof Provost FreeBSD's if_ovpn will never emit this as a peer deletion reason (because it doesn't support TCP), but this allows us to align the defines between Linux and FreeBSD, and remove a Linux-specific case from process_incoming_del_peer(). SoB missing Sorry for being very condensed, however the patch looks good and gets my ACK: Acked-by: Antonio Quartulli I normally put my ACK under the SoB, so when I couldn't find it, my brain just threw that exception :-P Cheers, --- src/openvpn/dco_freebsd.h | 1 + src/openvpn/multi.c | 3 --- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/src/openvpn/dco_freebsd.h b/src/openvpn/dco_freebsd.h index 2e35f3ac..970beca0 100644 --- a/src/openvpn/dco_freebsd.h +++ b/src/openvpn/dco_freebsd.h @@ -41,6 +41,7 @@ enum ovpn_del_reason_t { OVPN_DEL_PEER_REASON_EXPIRED, OVPN_DEL_PEER_REASON_TRANSPORT_ERROR, OVPN_DEL_PEER_REASON_USERSPACE, + OVPN_DEL_PEER_REASON_TRANSPORT_DISCONNECT, }; typedef struct dco_context { diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index f2559016..99123c39 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -3244,12 +3244,9 @@ process_incoming_del_peer(struct multi_context *m, struct multi_instance *mi, reason = "ovpn-dco: transport error"; break; -#ifdef TARGET_LINUX - /* FIXME: this is linux-only today and breaks FreeBSD compilation */ case OVPN_DEL_PEER_REASON_TRANSPORT_DISCONNECT: reason = "ovpn-dco: transport disconnected"; break; -#endif case OVPN_DEL_PEER_REASON_USERSPACE: /* We assume that is ourselves. Unfortunately, sometimes these -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] dco: define OVPN_DEL_PEER_REASON_TRANSPORT_DISCONNECT on FreeBSD
Hi, On 03/03/2023 12:05, Kristof Provost via Openvpn-devel wrote: From: Kristof Provost FreeBSD's if_ovpn will never emit this as a peer deletion reason (because it doesn't support TCP), but this allows us to align the defines between Linux and FreeBSD, and remove a Linux-specific case from process_incoming_del_peer(). SoB missing --- src/openvpn/dco_freebsd.h | 1 + src/openvpn/multi.c | 3 --- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/src/openvpn/dco_freebsd.h b/src/openvpn/dco_freebsd.h index 2e35f3ac..970beca0 100644 --- a/src/openvpn/dco_freebsd.h +++ b/src/openvpn/dco_freebsd.h @@ -41,6 +41,7 @@ enum ovpn_del_reason_t { OVPN_DEL_PEER_REASON_EXPIRED, OVPN_DEL_PEER_REASON_TRANSPORT_ERROR, OVPN_DEL_PEER_REASON_USERSPACE, +OVPN_DEL_PEER_REASON_TRANSPORT_DISCONNECT, }; typedef struct dco_context { diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index f2559016..99123c39 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -3244,12 +3244,9 @@ process_incoming_del_peer(struct multi_context *m, struct multi_instance *mi, reason = "ovpn-dco: transport error"; break; -#ifdef TARGET_LINUX -/* FIXME: this is linux-only today and breaks FreeBSD compilation */ case OVPN_DEL_PEER_REASON_TRANSPORT_DISCONNECT: reason = "ovpn-dco: transport disconnected"; break; -#endif case OVPN_DEL_PEER_REASON_USERSPACE: /* We assume that is ourselves. Unfortunately, sometimes these -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] Update issue templates
With this change we extend the text exposed to people opening a bug in the OpenVPN project. Hopefully they will read and immediately understand that GH is not the right place to report ossues about commercial products. Change-Id: Idd039612698a6b08f9544450885d1a5f77fd95c6 Signed-off-by: Antonio Quartulli --- .github/ISSUE_TEMPLATE/bug_report.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/ISSUE_TEMPLATE/bug_report.md b/.github/ISSUE_TEMPLATE/bug_report.md index 6cbd3ba4..3bf6f233 100644 --- a/.github/ISSUE_TEMPLATE/bug_report.md +++ b/.github/ISSUE_TEMPLATE/bug_report.md @@ -7,6 +7,9 @@ assignees: '' --- +**IMPORTANT NOTE** +Bugs about OpenVPN Access Server, OpenVPN Connect or any other product by OpenVPN Inc. should be directly reported to OpenVPN Inc. at https://support.openvpn.net + **Describe the bug** A clear and concise description of what the bug is. -- 2.39.2 ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Avoid management log loop with verb >= 6
Hi, On 17/02/2023 13:21, Lev Stipakov wrote: From: Lev Stipakov This log message is printed within check_tls(), which is called by pre_select(), which is called on every iteration of event loop. When management is attached (and doesn't use own event loop), this message sets management state to "wait write", which arms event loop. When on the next iteration iowait returns with "management write event is set", we call pre_select() and print that message again, causing the loop. Fix by simply removing this log message. Signed-off-by: Lev Stipakov As discussed on IRC, removing this message is a good thing as it serves no real purpose. On top of that this message is causing this log infinite recursion, therefore it should just go. OTOH we may still have *some* recursion due to other messages printed by this function. However, this messages will print only once, therefore they won't cause the recursion to continue indefinitely. Acked-by: Antonio Quartulli --- src/openvpn/dco.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/openvpn/dco.c b/src/openvpn/dco.c index 3087a0df..b53332a8 100644 --- a/src/openvpn/dco.c +++ b/src/openvpn/dco.c @@ -133,8 +133,6 @@ dco_get_secondary_key(struct tls_multi *multi, const struct key_state *primary) bool dco_update_keys(dco_context_t *dco, struct tls_multi *multi) { -msg(D_DCO_DEBUG, "%s: peer_id=%d", __func__, multi->dco_peer_id); - /* this function checks if keys have to be swapped or erased, therefore it * can't do much if we don't have any key installed */ -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2] Disabe DCO if proxy is set via management
Hi, On 20/02/2023 10:06, Lev Stipakov wrote: From: Lev Stipakov DCO doesn't support proxy and we already disable DCO is proxy is set in profile. Signed-off-by: Lev Stipakov Acked-by: Antonio Quartulli --- v2: use dco_enabled() helper function src/openvpn/init.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/src/openvpn/init.c b/src/openvpn/init.c index b500d354..622239f6 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -223,6 +223,12 @@ management_callback_proxy_cmd(void *arg, const char **p) } else if (p[2] && p[3]) { +if (dco_enabled(&c->options)) +{ +msg(M_INFO, "Proxy set via management, disabling Data Channel Offload."); +c->options.tuntap_options.disable_dco = true; +} + if (streq(p[1], "HTTP")) { struct http_proxy_options *ho; -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Improve format specifier for socket handle in Windows
Hi, On 10/02/2023 20:54, Selva Nair wrote: I also discussed this with Lev and, despite this being different from what we do in the *nix world (where decimal representations make sense for file descriptors), it seems to be the right hting to do on Windows when using HANDLEs (shrug). Windows kernel handles are very much "like" fds though the handle table also covers processes, threads, semaphores etc. They are not pointers, are generally smallish numbers or ~0 (== -1). But not as small as fds in the Unix/Linux world --- mainly because several objects share the name space and they have to be multiples of 4 for some arcane reason. The problem here was not printing as "decimal" but as "unsigned decimal". Use of the latter would have caused the same issue on Linux too. Using hex is a compromise here as handles are defined as "unsigned ints" of the same size as pointers. So PRIdPTR somehow looks improper though I would have chosen that. Given your explanation then it sounds like we should truly use PRIdPTR. Wouldn't you agree Lev? Cheers, Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Improve format specifier for socket handle in Windows
Hi, On 10/02/2023 14:31, Lev Stipakov wrote: From: Lev Stipakov Socket is a handle on Windows, which is usually logged in hex. Also an interesting value is INVALID_SOCKET, which is ~0. PRIuPTR prints decimals, and for INVALID_SOCKET it prints something like 2023-02-10 14:45:21 us=906000 write to TUN/TAP : Jrjestelmkutsulle annettu data-alue on liian pieni. (fd=18446744073709551615,code=122) PRIxPTR prints hex, and INVALID_SOCKET looks a bit nicer: 2023-02-10 15:17:11 us=828000 write to TUN/TAP : Jrjestelmkutsulle annettu data-alue on liian pieni. (fd=,code=122) Reported-by: Selva Nair Signed-off-by: Lev Stipakov I also discussed this with Lev and, despite this being different from what we do in the *nix world (where decimal representations make sense for file descriptors), it seems to be the right hting to do on Windows when using HANDLEs (shrug). Acked-by: Antonio Quartulli -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v3] configure: enable DCO by default on FreeBSD/Linux
Hi, On 07/02/2023 14:20, Frank Lichtenheld wrote: Automatically disabled when - iproute2 is enabled (Don't want to force people specifying --disable-dco explicitely) - libnv is missing on FreeBSD (FreeBSD version too old anyway) Will still error out if libnl-genl is missing on Linux to make people aware of the new dependency. Signed-off-by: Frank Lichtenheld Acked-by: Antonio Quartulli -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v3 release/2.6] Allow certain DHCP options to be used without DHCP server
Hi, On 07/02/2023 15:54, Lev Stipakov wrote: From: Lev Stipakov Followin DHCP options: DOMAIN, ADAPTER_DOMAIN_SUFFIX, DNS, WINS don't require DHCP server in order to be used. This change allows those options to be used with dco and wintun drivers. If an option specified which requires DHCP server and tap-windows6 driver is not used, print a clear error message instead of obscure reference to --ip-win32. Reported-by: Marek Zarychta Signed-off-by: Lev Stipakov Code makes sense and does what it says. Acked-by: Antonio Quartulli However, please not that I did not test this code path explicitly as my testing capabilities on Windows are pretty limited. Cheers, --- v3: replace SHOW_INT with SHOW_UNSIGNED v2: replace enum with defines, which are more suitable as bit flags src/openvpn/options.c | 39 +++ src/openvpn/tun.h | 6 +- 2 files changed, 28 insertions(+), 17 deletions(-) diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 6ae3faf8..8cbffc5c 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -1290,7 +1290,7 @@ show_tuntap_options(const struct tuntap_options *o) SHOW_INT(dhcp_masq_offset); SHOW_INT(dhcp_lease_time); SHOW_INT(tap_sleep); -SHOW_BOOL(dhcp_options); +SHOW_UNSIGNED(dhcp_options); SHOW_BOOL(dhcp_renew); SHOW_BOOL(dhcp_pre_release); SHOW_STR(domain); @@ -2478,12 +2478,20 @@ options_postprocess_verify_ce(const struct options *options, msg(M_USAGE, "On Windows, --ip-win32 doesn't make sense unless --ifconfig is also used"); } -if (options->tuntap_options.dhcp_options -&& options->windows_driver != WINDOWS_DRIVER_WINTUN -&& options->tuntap_options.ip_win32_type != IPW32_SET_DHCP_MASQ -&& options->tuntap_options.ip_win32_type != IPW32_SET_ADAPTIVE) +if (options->tuntap_options.dhcp_options & DHCP_OPTIONS_DHCP_REQUIRED) { -msg(M_USAGE, "--dhcp-option requires --ip-win32 dynamic or adaptive"); +const char *prefix = "Some dhcp-options require DHCP server"; +if (options->windows_driver != WINDOWS_DRIVER_TAP_WINDOWS6) +{ +msg(M_USAGE, "%s, which is not supported by selected %s driver", +prefix, print_windows_driver(options->windows_driver)); +} +else if (options->tuntap_options.ip_win32_type != IPW32_SET_DHCP_MASQ + && options->tuntap_options.ip_win32_type != IPW32_SET_ADAPTIVE) +{ +msg(M_USAGE, "%s, which requires --ip-win32 dynamic or adaptive", +prefix); +} } if (options->windows_driver == WINDOWS_DRIVER_WINTUN && dev != DEV_TYPE_TUN) @@ -8083,16 +8091,17 @@ add_option(struct options *options, { struct tuntap_options *o = &options->tuntap_options; VERIFY_PERMISSION(OPT_P_DHCPDNS); -bool ipv6dns = false; if ((streq(p[1], "DOMAIN") || streq(p[1], "ADAPTER_DOMAIN_SUFFIX")) && p[2] && !p[3]) { o->domain = p[2]; +o->dhcp_options |= DHCP_OPTIONS_DHCP_OPTIONAL; } else if (streq(p[1], "NBS") && p[2] && !p[3]) { o->netbios_scope = p[2]; +o->dhcp_options |= DHCP_OPTIONS_DHCP_REQUIRED; } else if (streq(p[1], "NBT") && p[2] && !p[3]) { @@ -8104,31 +8113,35 @@ add_option(struct options *options, goto err; } o->netbios_node_type = t; +o->dhcp_options |= DHCP_OPTIONS_DHCP_REQUIRED; } else if ((streq(p[1], "DNS") || streq(p[1], "DNS6")) && p[2] && !p[3] && (!strstr(p[2], ":") || ipv6_addr_safe(p[2]))) { if (strstr(p[2], ":")) { -ipv6dns = true; dhcp_option_dns6_parse(p[2], o->dns6, &o->dns6_len, msglevel); } else { dhcp_option_address_parse("DNS", p[2], o->dns, &o->dns_len, msglevel); +o->dhcp_options |= DHCP_OPTIONS_DHCP_OPTIONAL; } } else if (streq(p[1], "WINS") && p[2] && !p[3]) { dhcp_option_address_parse("WINS", p[2], o->wins, &o->wins_len, msglevel); +o->dhcp_options |= DHCP_OPTIONS_DHCP_OPTIONAL; } else if (streq(p[1], "NTP") && p[2] && !p[3]) { dhcp_option_address_parse("NTP", p[2], o-&g
Re: [Openvpn-devel] [PATCH v2 release/2.6] Allow certain DHCP options to be used without DHCP server
Hi, On 07/02/2023 14:59, Lev Stipakov wrote: This definition seems to be unused by the way :) I am not sure if "0x0003" is better than "3" (we have only two flags). Something like 0011b would be clearer, but we don't have SHOW_BINARY. Personally I find the hex representation better when you want to print a bitfield. The fact we are using hex kinda tells me already that it's not the value "3" that we care about. And later is may become "10". Imho it just gets more confusing. Cheers, ti 7. helmik. 2023 klo 15.36 Antonio Quartulli (a...@unstable.cc) kirjoitti: Hi, On 07/02/2023 10:42, Lev Stipakov wrote: From: Lev Stipakov Followin DHCP options: DOMAIN, ADAPTER_DOMAIN_SUFFIX, DNS, WINS don't require DHCP server in order to be used. This change allows those options to be used with dco and wintun drivers. If an option specified which requires DHCP server and tap-windows6 driver is not used, print a clear error message instead of obscure reference to --ip-win32. This fixes https://github.com/OpenVPN/openvpn/issues/239 Reported-by: Marek Zarychta Signed-off-by: Lev Stipakov --- v2: replace enum with defines, which are more suitable as bit flags src/openvpn/options.c | 39 +++ src/openvpn/tun.h | 6 +- 2 files changed, 28 insertions(+), 17 deletions(-) diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 6ae3faf8..9c05217c 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -1290,7 +1290,7 @@ show_tuntap_options(const struct tuntap_options *o) SHOW_INT(dhcp_masq_offset); SHOW_INT(dhcp_lease_time); SHOW_INT(tap_sleep); -SHOW_BOOL(dhcp_options); +SHOW_INT(dhcp_options); wouldn't SHOW_UNSIGNED be more appropriate since this is a bitfield? This is its definition: options.c:987:#define SHOW_UNSIGNED(var) SHOW_PARM(var, o->var, "0x%08x") Cheers, SHOW_BOOL(dhcp_renew); SHOW_BOOL(dhcp_pre_release); SHOW_STR(domain); @@ -2478,12 +2478,20 @@ options_postprocess_verify_ce(const struct options *options, msg(M_USAGE, "On Windows, --ip-win32 doesn't make sense unless --ifconfig is also used"); } -if (options->tuntap_options.dhcp_options -&& options->windows_driver != WINDOWS_DRIVER_WINTUN -&& options->tuntap_options.ip_win32_type != IPW32_SET_DHCP_MASQ -&& options->tuntap_options.ip_win32_type != IPW32_SET_ADAPTIVE) +if (options->tuntap_options.dhcp_options & DHCP_OPTIONS_DHCP_REQUIRED) { -msg(M_USAGE, "--dhcp-option requires --ip-win32 dynamic or adaptive"); +const char *prefix = "Some dhcp-options require DHCP server"; +if (options->windows_driver != WINDOWS_DRIVER_TAP_WINDOWS6) +{ +msg(M_USAGE, "%s, which is not supported by selected %s driver", +prefix, print_windows_driver(options->windows_driver)); +} +else if (options->tuntap_options.ip_win32_type != IPW32_SET_DHCP_MASQ + && options->tuntap_options.ip_win32_type != IPW32_SET_ADAPTIVE) +{ +msg(M_USAGE, "%s, which requires --ip-win32 dynamic or adaptive", +prefix); +} } if (options->windows_driver == WINDOWS_DRIVER_WINTUN && dev != DEV_TYPE_TUN) @@ -8083,16 +8091,17 @@ add_option(struct options *options, { struct tuntap_options *o = &options->tuntap_options; VERIFY_PERMISSION(OPT_P_DHCPDNS); -bool ipv6dns = false; if ((streq(p[1], "DOMAIN") || streq(p[1], "ADAPTER_DOMAIN_SUFFIX")) && p[2] && !p[3]) { o->domain = p[2]; +o->dhcp_options |= DHCP_OPTIONS_DHCP_OPTIONAL; } else if (streq(p[1], "NBS") && p[2] && !p[3]) { o->netbios_scope = p[2]; +o->dhcp_options |= DHCP_OPTIONS_DHCP_REQUIRED; } else if (streq(p[1], "NBT") && p[2] && !p[3]) { @@ -8104,31 +8113,35 @@ add_option(struct options *options, goto err; } o->netbios_node_type = t; +o->dhcp_options |= DHCP_OPTIONS_DHCP_REQUIRED; } else if ((streq(p[1], "DNS") || streq(p[1], "DNS6")) && p[2] && !p[3] && (!strstr(p[2], ":") || ipv6_addr_safe(p[2]))) { if (strstr(p[2], ":")) { -ipv6dns = true; dhcp_option_dns6_parse(p[2], o->dns6, &o->dns6_len, msglevel); }
Re: [Openvpn-devel] [PATCH v2 release/2.6] Allow certain DHCP options to be used without DHCP server
tp, &o->ntp_len, msglevel); +o->dhcp_options |= DHCP_OPTIONS_DHCP_REQUIRED; } else if (streq(p[1], "NBDD") && p[2] && !p[3]) { dhcp_option_address_parse("NBDD", p[2], o->nbdd, &o->nbdd_len, msglevel); +o->dhcp_options |= DHCP_OPTIONS_DHCP_REQUIRED; } else if (streq(p[1], "DOMAIN-SEARCH") && p[2] && !p[3]) { @@ -8141,10 +8154,12 @@ add_option(struct options *options, msg(msglevel, "--dhcp-option %s: maximum of %d search entries can be specified", p[1], N_SEARCH_LIST_LEN); } +o->dhcp_options |= DHCP_OPTIONS_DHCP_REQUIRED; } else if (streq(p[1], "DISABLE-NBT") && !p[2]) { o->disable_nbt = 1; +o->dhcp_options |= DHCP_OPTIONS_DHCP_REQUIRED; } #if defined(TARGET_ANDROID) else if (streq(p[1], "PROXY_HTTP") && p[3] && !p[4]) @@ -8158,14 +8173,6 @@ add_option(struct options *options, msg(msglevel, "--dhcp-option: unknown option type '%s' or missing or unknown parameter", p[1]); goto err; } - -/* flag that we have options to give to the TAP driver's DHCPv4 server - * - skipped for "DNS6", as that's not a DHCPv4 option - */ -if (!ipv6dns) -{ -o->dhcp_options = true; -} } #endif /* if defined(_WIN32) || defined(TARGET_ANDROID) */ #ifdef _WIN32 diff --git a/src/openvpn/tun.h b/src/openvpn/tun.h index 3b0a0d24..e19e1a2e 100644 --- a/src/openvpn/tun.h +++ b/src/openvpn/tun.h @@ -62,6 +62,10 @@ enum windows_driver_type { #define IPW32_SET_ADAPTIVE_DELAY_WINDOW 300 #define IPW32_SET_ADAPTIVE_TRY_NETSH20 +/* bit flags for DHCP options */ +#define DHCP_OPTIONS_DHCP_OPTIONAL (1<<0) +#define DHCP_OPTIONS_DHCP_REQUIRED (1<<1) + struct tuntap_options { /* --ip-win32 options */ bool ip_win32_defined; @@ -90,7 +94,7 @@ struct tuntap_options { /* --dhcp-option options */ -bool dhcp_options; +int dhcp_options; const char *domain; /* DOMAIN (15) */ -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] dco_linux: update license for ovpn_dco_linux.h
The linux userspace API header has acquired the MIT license (check the ovpn-dco repository for the related change), therefore we simply bring this change in our local copy to ensure compliancy. Signed-off-by: Antonio Quartulli --- src/openvpn/ovpn_dco_linux.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/openvpn/ovpn_dco_linux.h b/src/openvpn/ovpn_dco_linux.h index f9a3b827..96395886 100644 --- a/src/openvpn/ovpn_dco_linux.h +++ b/src/openvpn/ovpn_dco_linux.h @@ -1,4 +1,4 @@ -/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ +/* SPDX-License-Identifier: (GPL-2.0-only WITH Linux-syscall-note) OR MIT */ /* * OpenVPN data channel accelerator * -- 2.39.1 ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Workaround: make ovpn-dco more reliable
Hi, On 12/01/2023 17:37, Arne Schwabe wrote: This workaround avoids the kernel trigger ENOBUFS when the kernel internal queue is overrun with events of disconnectingh clients or similar. This is a workaround until we come up with a more permanent solution. Signed-off-by: Arne Schwabe After further discussion and deeper testing, we concluded that it is possible to generate so much netlink traffic that we can easily fill the buffers and start losing message or get desync'd with kernelspace. The long term solution is improving ovpn-dco (kernel module) to reduce such traffic, however, for now it makes sense to extend the userspace buffer in order to decrease the likelihood of filling it up during normal operations. Therefore this patch gets my ACK: Acked-by: Antonio Quartulli [please add spaces around the '*' operator] --- src/openvpn/dco_linux.c | 5 + 1 file changed, 5 insertions(+) diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c index 5524cdbcd..c84f9cfe1 100644 --- a/src/openvpn/dco_linux.c +++ b/src/openvpn/dco_linux.c @@ -375,6 +375,11 @@ ovpn_dco_init_netlink(dco_context_t *dco) * wrong sequence numbers (NLE_SEQ_MISMATCH), so disable libnl's sequence * number check */ nl_socket_disable_seq_check(dco->nl_sock); + +/* nl library sets the buffer size to 32k/32k by default which is sometimes + * overrun with very fast connecting/disconnecting clients. + * TODO: fix this in a better and more reliable way */ +ASSERT(!nl_socket_set_buffer_size(dco->nl_sock, 1024*1024, 1024*1024)); } bool -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Fix OVPN_DEL_PEER_REASON_TRANSPORT_DISCONNECT breakage on FreeBSD+DCO
Hi, On 13/01/2023 09:07, Gert Doering wrote: commit 67c4eebdae introduces a new peer disconnect reason (transport disconnected, aka "TCP session closed") which breaks compilation on FreeBSD - OVPN_DEL_PEER_REASON_TRANSPORT_DISCONNECT not part of the enum in freebsd_dco.h, and no kernel support for TCP anyway. This patch is an intermediate bandaid, making the offending code in multi.c "linux only" while a better solution is discussed. Signed-off-by: Gert Doering Hi, as we are discussing in a nother thread and on IRC, we need to come up with a better solution that is flexible enough to prevent this from happening in the future. There are some floating ideas. Anyway, the discussion will continue in the other thread. Regarding this patch: Acked-by: Antonio Quartulli Maybe we need a fbsd14 buildbot? Cheers, -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] dco: print proper message in case of transport disconnection
Hi, On 13/01/2023 09:44, Gert Doering wrote: Hi, On Fri, Jan 13, 2023 at 09:37:49AM +0100, Antonio Quartulli wrote: On 13/01/2023 09:32, Kristof Provost wrote: I???m not sure how we???d cope with supporting building on older releases though. Not a worry just yet, because FreeBSD main is the only version with DCO support in it, but it???ll be a problem sooner or later. OpenVPN ships its own "recent"| version of the if_ovpn.h file, no? So as long as the file contains all the enums it will compile just fine. Yes, these defines are mirrored in our dco_freebsd.h - so I could just add it there, but we need to be careful that we never have conflicting enums in OpenVPN dco_freebsd.h vs. kernel if_ovpn.h. Or, as Antonio explained to me how it works on linux uapi headers "enums are only ever appended to, never ever reordered, nothing is ever deleted". I'm open for anything that lets me ship rc2+patch as FreeBSD port :-) As I was saying on IRC, IMHO we took this path of using the same names for enums on linux and freebsd because it was convenient. But here we are hitting a shortcoming of this approach. If we assume that enums are not supposed to be the same across platforms (the latter may have different logic, or whatever), we could implement a conversion function that takes a platform specific enum as input and returns a userspace specific enum. Platform independent code will then always only deal with userspace enums and won't care about what is truly written in the platform-specific headers. This way we draw a clear boundary between "what kernel on platform X supports" and "what userspace openvpn can deal with". And situations like this should never happen again. This conversion function would be implemented in the platform specific code (i.e. dco_linux.c) Does it make sense? -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] dco: print proper message in case of transport disconnection
Hi, On 13/01/2023 09:32, Kristof Provost wrote: I’m not sure how we’d cope with supporting building on older releases though. Not a worry just yet, because FreeBSD main is the only version with DCO support in it, but it’ll be a problem sooner or later. OpenVPN ships its own "recent"| version of the if_ovpn.h file, no? So as long as the file contains all the enums it will compile just fine. Cheers, -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Repair special-casing of EEXIST for Linux/SITNL route install
Hi, for the netlink/sitnl bits: this makes sense to me. I agree with Selva that the v6 variant could benefit from the same treatment. However, this patch can also be hacked on its own Acked-by: Antonio Quartulli On 11/01/2023 17:08, Gert Doering wrote: The code in sitnl_route_set() used to treat "route can not be installed because it already exists" (EEXIST) as "not an error". This is arguably a reasonable approach, but needs to handled higher up - if the low level add_route() function say "no error", we will try to remove that route later on in delete_route(), possibly removing someone else's "already existing" route then. So: - remove special case in sitnl_route_set() - do not pass NLM_F_REPLACE flag to sitnl_route_set() call - this would cause netlink to just replace existing routes, never return EEXIST (see "man netlink(7)") - add detailed return code handling to route_add(), assign "2" on "-EEXIST" (and log appropriate message). (Note: sitnl_route_set() is a common function for sitnl route add and delete, but EEXIST can not happen on delete - so this change has no impact for the "delete" case) Signed-off-by: Gert Doering --- src/openvpn/networking_sitnl.c | 6 +- src/openvpn/route.c| 10 -- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/openvpn/networking_sitnl.c b/src/openvpn/networking_sitnl.c index fe124616..92f30044 100644 --- a/src/openvpn/networking_sitnl.c +++ b/src/openvpn/networking_sitnl.c @@ -944,10 +944,6 @@ sitnl_route_set(int cmd, uint32_t flags, int ifindex, sa_family_t af_family, } ret = sitnl_send(&req.n, 0, 0, NULL, NULL); -if (ret == -EEXIST) -{ -ret = 0; -} err: return ret; } @@ -1177,7 +1173,7 @@ sitnl_route_add(const char *iface, sa_family_t af_family, const void *dst, scope = RT_SCOPE_LINK; } -return sitnl_route_set(RTM_NEWROUTE, NLM_F_CREATE | NLM_F_REPLACE, ifindex, +return sitnl_route_set(RTM_NEWROUTE, NLM_F_CREATE, ifindex, af_family, dst, prefixlen, gw, table, metric, scope, RTPROT_BOOT, RTN_UNICAST); } diff --git a/src/openvpn/route.c b/src/openvpn/route.c index 99f948ba..2db127bb 100644 --- a/src/openvpn/route.c +++ b/src/openvpn/route.c @@ -1594,8 +1594,14 @@ add_route(struct route_ipv4 *r, } status = 1; -if (net_route_v4_add(ctx, &r->network, netmask_to_netbits2(r->netmask), - &r->gateway, iface, 0, metric) < 0) +int ret = net_route_v4_add(ctx, &r->network, netmask_to_netbits2(r->netmask), + &r->gateway, iface, 0, metric); +if (ret == -EEXIST) +{ +msg(D_ROUTE, "NOTE: Linux route add command failed because route exists"); +status = 2; +} + else if (ret < 0) { msg(M_WARN, "ERROR: Linux route add command failed"); status = 0; -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] dco: send SIGUSR1 upon ping timeout
When a peer is removed with reason "ping expire", we should kill the instance with SIGUSR1 and not SIGTERM Cc: Arne Schwabe Signed-off-by: Antonio Quartulli -- Arne, I am not 100% sure why but it seems for ping-restart we always use SIGUSR1, right? but the DCO handling code was apparently using SIGTERM. What do you think? --- src/openvpn/multi.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index 99123c39..10efffec 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -3234,10 +3234,12 @@ process_incoming_del_peer(struct multi_context *m, struct multi_instance *mi, dco_context_t *dco) { const char *reason = "ovpn-dco: unknown reason"; +int signal = SIGTERM; switch (dco->dco_del_peer_reason) { case OVPN_DEL_PEER_REASON_EXPIRED: reason = "ovpn-dco: ping expired"; +signal = SIGUSR1; break; case OVPN_DEL_PEER_REASON_TRANSPORT_ERROR: @@ -3270,7 +3272,7 @@ process_incoming_del_peer(struct multi_context *m, struct multi_instance *mi, mi->context.sig->signal_text = reason; mi->context.c2.dco_read_bytes = dco->dco_read_bytes; mi->context.c2.dco_write_bytes = dco->dco_write_bytes; -multi_signal_instance(m, mi, SIGTERM); +multi_signal_instance(m, mi, signal); } bool -- 2.38.2 ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] dco: print proper message in case of transport disconnection
Signed-off-by: Antonio Quartulli --- --no-verify is required upon commit due to changes in ovpn_dco_linux.h Little logging improvement for https://github.com/OpenVPN/ovpn-dco/issues/9 --- src/openvpn/multi.c | 4 src/openvpn/ovpn_dco_linux.h | 5 +++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index 77dcaa60..99123c39 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -3244,6 +3244,10 @@ process_incoming_del_peer(struct multi_context *m, struct multi_instance *mi, reason = "ovpn-dco: transport error"; break; +case OVPN_DEL_PEER_REASON_TRANSPORT_DISCONNECT: +reason = "ovpn-dco: transport disconnected"; +break; + case OVPN_DEL_PEER_REASON_USERSPACE: /* We assume that is ourselves. Unfortunately, sometimes these * events happen with enough delay that they can have an order of diff --git a/src/openvpn/ovpn_dco_linux.h b/src/openvpn/ovpn_dco_linux.h index beca1beb..f9a3b827 100644 --- a/src/openvpn/ovpn_dco_linux.h +++ b/src/openvpn/ovpn_dco_linux.h @@ -1,8 +1,8 @@ -/* SPDX-License-Identifier: GPL-2.0-only WITH Linux-syscall-note */ +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ /* * OpenVPN data channel accelerator * - * Copyright (C) 2019-2021 OpenVPN, Inc. + * Copyright (C) 2019-2022 OpenVPN, Inc. * * Author:James Yonan * Antonio Quartulli @@ -85,6 +85,7 @@ enum ovpn_del_peer_reason { OVPN_DEL_PEER_REASON_USERSPACE, OVPN_DEL_PEER_REASON_EXPIRED, OVPN_DEL_PEER_REASON_TRANSPORT_ERROR, + OVPN_DEL_PEER_REASON_TRANSPORT_DISCONNECT, __OVPN_DEL_PEER_REASON_AFTER_LAST }; -- 2.38.2 ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Reduce logspam about 'dco_update_keys: peer_id=-1' in p2p server mode
Hi, On 09/01/2023 21:00, Gert Doering wrote: p2p --tls-server with no active client/peer logs once per second "dco_update_keys: peer_id=-1" which does exactly nothing, except fill the disk. So skip the call to dco_update_keys() if peer_id == -1. Signed-off-by: Gert Doering --- src/openvpn/forward.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c index ae0512fc..2ba8b0fa 100644 --- a/src/openvpn/forward.c +++ b/src/openvpn/forward.c @@ -151,6 +151,12 @@ check_dco_key_status(struct context *c) return; } +/* no active peer (p2p tls-server mode) */ +if (c->c2.tls_multi->dco_peer_id == -1 ) Please remove the space after -1 (not sure why uncrustify hasn't caught it). +{ +return; +} + if (!dco_update_keys(&c->c1.tuntap->dco, c->c2.tls_multi)) { /* Something bad happened. Kill the connection to Rest looks good. Thanks! Acked-by: Antonio Quartulli However, as discussed on IRC: *why* are we running the check_tls code is the peer has gone away and we have switched the peer-id to -1? This is the real question. -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH 3/3] dco: improve comment about hidden debug message
While at it also improve the debug message itself to be more self-explanatory. Signed-off-by: Antonio Quartulli --- src/openvpn/multi.c | 13 + 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index b10a6d8d..8facc66f 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -3296,12 +3296,17 @@ multi_process_incoming_dco(struct multi_context *m) if (dco->dco_message_type == OVPN_CMD_DEL_PEER && dco->dco_del_peer_reason == OVPN_DEL_PEER_REASON_USERSPACE) { -/* we get notified after we kill the peer ourselves and probably - * have already forgotten about it. This is expected */ +/* we receive OVPN_CMD_DEL_PEER message with reason USERSPACE + * after we kill the peer ourselves. This peer may have already + * been deleted, so we end up here. + * In this case, print the following debug message with DCO_DEBUG + * level only to avoid polluting the standard DCO level with this + * harmless event. + */ msglevel = D_DCO_DEBUG; } -msg(msglevel, "Received packet for peer-id unknown to OpenVPN: %d, " -"type %d, reason %d", peer_id, dco->dco_message_type, +msg(msglevel, "Received DCO message for unknown peer-id: %d, " +"type %d, del_peer_reason %d", peer_id, dco->dco_message_type, dco->dco_del_peer_reason); /* Also clear the buffer if this was incoming packet for a dropped peer */ buf_init(&dco->dco_packet_in, 0); -- 2.38.2 ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH 2/3] dco: bail out when no peer-specific message is delivered
multi_process_incoming_dco() is currently partly processing messages that were actually discarded. This results in a bogus message being printed: "Received packet for peer-id unknown to OpenVPN: -1, type 0, reason 2" Change the flow so that we bail out immediately when we know that no message was truly delivered by DCO. Currently this can be verified by chacking that the peed_is is greater than -1. Signed-off-by: Antonio Quartulli --- src/openvpn/multi.c | 10 +- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index 27676de5..b10a6d8d 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -3270,7 +3270,15 @@ multi_process_incoming_dco(struct multi_context *m) int peer_id = dco->dco_message_peer_id; -if ((peer_id >= 0) && (peer_id < m->max_clients) && (m->instances[peer_id])) +/* no peer-specific message delivered -> nothing to process. + * bail out right away + */ +if (peer_id < 0) +{ +return ret > 0; +} + +if ((peer_id < m->max_clients) && (m->instances[peer_id])) { mi = m->instances[peer_id]; if (dco->dco_message_type == OVPN_CMD_PACKET) -- 2.38.2 ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH 1/3] dco: properly re-initialize dco_del_peer_reason
After processing a message, all fields of the dco object should be re-initialized so that future processings are not affected by stale values. This includes dco_del_peer_reason. Since its values can start at 0, re-initialize it with -1. Signed-off-by: Antonio Quartulli --- src/openvpn/multi.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index 3658e1d5..27676de5 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -3301,6 +3301,7 @@ multi_process_incoming_dco(struct multi_context *m) dco->dco_message_type = 0; dco->dco_message_peer_id = -1; +dco->dco_del_peer_reason = -1; dco->dco_read_bytes = 0; dco->dco_write_bytes = 0; return ret > 0; -- 2.38.2 ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Introduce dco_get_peer_stats API and Windows implementation
Hi, On 15/12/2022 13:55, Gert Doering wrote: Hi, On Thu, Dec 15, 2022 at 07:49:53AM -0500, Selva Nair wrote: Possibly we can merge the current patch for openvpn.exe and then improve it later in bug-fix releases of 2.6? This was my thinking as well - so the patch is in, and provides at least some statistics, and room for improvements :-) Yeah, this was the best course of action in my opinion as well. Cheers, -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Introduce dco_get_peer_stats API and Windows implementation
Hi, On 15/12/2022 10:31, Lev Stipakov wrote: Hi, In non-dco use, the stats as persisted by the management interface are not reset throughout the lifetime of the process. With dco, what the driver provides is "Peer Stats" which is reset in OvpnPeerNew() (linux appears to do the same). A quick option > would be to move the zero-ing to OvpnEvtFileCleanup() which, I guess, would work at least when persist-tun is in use. Yeah, without persist-tun client closes the handle, which triggers OvpnEvtFileCleanup(), which would reset stats. Or in a new callback that gets called on tun-open. But then it wont be strictly "Peer Stats". Yeah that might be an option, to have RESET_STATS ioctl. Also I wonder if the driver could remember, say, the last pid, and then reset stats on NEW_PEER if pid != last pid. Since pids are recycled, we might "leak" stats in unfortunate cases, but that's okay I guess? Sorry for jumping in the discussion a bit late. IMHO the stats belong to the Peer object. Let's not forget that, apart from windows, DCO supports multi-peer mode, therefore we have to consider that case as well. Due to the above, I'd "reset" the stats when a new peer object is instantiated. Basically when a new peer object is instantiated it should come with empty stats, as expected. If the desired behvaiour in userspace is to keep the stats persistent across a number of events, it should simply be userspace to keep those stats. This way we don't mix up the logic of counting the bytes per peer, and keeping a general picture of the VPN process. Cheers, -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 1/3] Improve debug logging of DCO swap key message and Linux dco_new_peer
On 13/12/2022 23:54, Arne Schwabe wrote: Signed-off-by: Arne Schwabe --- src/openvpn/dco.c | 18 ++ src/openvpn/dco_linux.c | 10 -- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/src/openvpn/dco.c b/src/openvpn/dco.c index feb38cd02..2396bcbf0 100644 --- a/src/openvpn/dco.c +++ b/src/openvpn/dco.c @@ -55,8 +55,8 @@ dco_install_key(struct tls_multi *multi, struct key_state *ks, const char *ciphername) { -msg(D_DCO_DEBUG, "%s: peer_id=%d keyid=%d", __func__, multi->dco_peer_id, -ks->key_id); +msg(D_DCO_DEBUG, "%s: peer_id=%d keyid=%d, currently installed %d", I'd make this "num. installed" because "currently installed" makes me think as if what follows is the ID of something that is installed. While we should make it explicit that this is the number of installed keys. +__func__, multi->dco_peer_id, ks->key_id, multi->dco_keys_installed); /* Install a key in the PRIMARY slot only when no other key exist. * From that moment on, any new key will be installed in the SECONDARY @@ -181,8 +181,18 @@ dco_update_keys(dco_context_t *dco, struct tls_multi *multi) */ if (primary->dco_status == DCO_INSTALLED_SECONDARY) { -msg(D_DCO_DEBUG, "Swapping primary and secondary keys, now: id1=%d id2=%d", -primary->key_id, secondary ? secondary->key_id : -1); +if (secondary) +{ +msg(D_DCO_DEBUG, "Swapping primary and secondary keys to " +"primary-id=%d secondary-id=%d", +primary->key_id, secondary->key_id); +} +else +{ +msg(D_DCO_DEBUG, "Swapping primary and secondary keys to" +"primary-id=%d secondary-id=(to be deleted)", +primary->key_id); +} int ret = dco_swap_keys(dco, multi->dco_peer_id); if (ret < 0) diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c index 109358205..fbd940c28 100644 --- a/src/openvpn/dco_linux.c +++ b/src/openvpn/dco_linux.c @@ -216,9 +216,15 @@ dco_new_peer(dco_context_t *dco, unsigned int peerid, int sd, struct sockaddr *localaddr, struct sockaddr *remoteaddr, struct in_addr *remote_in4, struct in6_addr *remote_in6) { -msg(D_DCO_DEBUG, "%s: peer-id %d, fd %d", __func__, peerid, sd); - struct gc_arena gc = gc_new(); +const char *remotestr = "[undefined]"; +if (remoteaddr) +{ +remotestr = print_sockaddr(remoteaddr, &gc); +} +msg(D_DCO_DEBUG, "%s: peer-id %d, fd %d, remote addr: %s", __func__, +peerid, sd, remotestr); + struct nl_msg *nl_msg = ovpn_dco_nlmsg_create(dco, OVPN_CMD_NEW_PEER); struct nlattr *attr = nla_nest_start(nl_msg, OVPN_ATTR_NEW_PEER); int ret = -EMSGSIZE; The rest makes sense: Acked-by: Antonio Quartulli -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 3/3] Set DCO_NOT_INSTALLED also for keys not in the get_key_scan range
Hi, On 13/12/2022 23:54, Arne Schwabe wrote: We have 6 key slots but normally only consider 3 of them to be active/valid keys. Especially the secondary key of TM_LAME_DUCK can in rare corner cases have a key that is still installed in the kernel. While this should not cause any issues since I do not see way for this key to become active ever again, it is better to keep the state correctly. Signed-off-by: Arne Schwabe I am fine with this because it is just "safer". But I truly dislike that we have to "Defend ourselves" from conditions which we don't even know when they may happen Still, this is a rant for another patch/cleanup. This patch makes sense Acked-by: Antonio Quartulli --- src/openvpn/dco.c | 14 +- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/openvpn/dco.c b/src/openvpn/dco.c index 36bfbf10a..20196fe5d 100644 --- a/src/openvpn/dco.c +++ b/src/openvpn/dco.c @@ -221,13 +221,17 @@ dco_update_keys(dco_context_t *dco, struct tls_multi *multi) multi->dco_keys_installed = 1; } -/* all keys that are not installed are set to NOT installed */ -for (int i = 0; i < KEY_SCAN_SIZE; ++i) +/* all keys that are not installed are set to NOT installed. Include also + * keys that might even be considered as active keys to be sure*/ +for (int i = 0; i < TM_SIZE; ++i) { -struct key_state *ks = get_key_scan(multi, i); -if (ks != primary && ks != secondary) +for (int j = 0; j < KS_SIZE; j++) { -ks->dco_status = DCO_NOT_INSTALLED; +struct key_state *ks = &multi->session[i].key[j]; +if (ks != primary && ks != secondary) +{ +ks->dco_status = DCO_NOT_INSTALLED; + } } } return true; -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 2/3] Trigger a USR1 if dco_update_keys fails
Hi, On 13/12/2022 23:54, Arne Schwabe wrote: When dco_update_keys fails, we are in some weird state that we are unlikely to recover since what userspace and kernel space think of the keys is very likely to not in sync anymore. So abandon the connection if this happens. Signed-off-by: Arne Schwabe This makes sense to me. We didn't do that earlier because we weren't sure about what to doing this case, but issuing USR1 and bailing out is actually sensible. Acked-by: Antonio Quartulli --- src/openvpn/dco.c | 15 --- src/openvpn/dco.h | 9 ++--- src/openvpn/forward.c | 7 ++- 3 files changed, 20 insertions(+), 11 deletions(-) diff --git a/src/openvpn/dco.c b/src/openvpn/dco.c index 2396bcbf0..36bfbf10a 100644 --- a/src/openvpn/dco.c +++ b/src/openvpn/dco.c @@ -130,7 +130,7 @@ dco_get_secondary_key(struct tls_multi *multi, const struct key_state *primary) return NULL; } -void +bool dco_update_keys(dco_context_t *dco, struct tls_multi *multi) { msg(D_DCO_DEBUG, "%s: peer_id=%d", __func__, multi->dco_peer_id); @@ -140,7 +140,7 @@ dco_update_keys(dco_context_t *dco, struct tls_multi *multi) */ if (multi->dco_keys_installed == 0) { -return; +return true; } struct key_state *primary = tls_select_encryption_key(multi); @@ -155,18 +155,18 @@ dco_update_keys(dco_context_t *dco, struct tls_multi *multi) if (ret < 0) { msg(D_DCO, "Cannot delete primary key during wipe: %s (%d)", strerror(-ret), ret); -return; +return false; } ret = dco_del_key(dco, multi->dco_peer_id, OVPN_KEY_SLOT_SECONDARY); if (ret < 0) { msg(D_DCO, "Cannot delete secondary key during wipe: %s (%d)", strerror(-ret), ret); -return; +return false; } multi->dco_keys_installed = 0; -return; +return true; } /* if we have a primary key, it must have been installed already (keys @@ -198,7 +198,7 @@ dco_update_keys(dco_context_t *dco, struct tls_multi *multi) if (ret < 0) { msg(D_DCO, "Cannot swap keys: %s (%d)", strerror(-ret), ret); -return; +return false; } primary->dco_status = DCO_INSTALLED_PRIMARY; @@ -216,7 +216,7 @@ dco_update_keys(dco_context_t *dco, struct tls_multi *multi) if (ret < 0) { msg(D_DCO, "Cannot delete secondary key: %s (%d)", strerror(-ret), ret); -return; +return false; } multi->dco_keys_installed = 1; } @@ -230,6 +230,7 @@ dco_update_keys(dco_context_t *dco, struct tls_multi *multi) ks->dco_status = DCO_NOT_INSTALLED; } } +return true; } static bool diff --git a/src/openvpn/dco.h b/src/openvpn/dco.h index e051db068..7e1febaa3 100644 --- a/src/openvpn/dco.h +++ b/src/openvpn/dco.h @@ -164,9 +164,11 @@ int init_key_dco_bi(struct tls_multi *multi, struct key_state *ks, * * @param dco DCO device context * @param multi TLS multi instance + * + * @return returns false if an error occurred that is not + * recoverable and should reset the connection */ -void dco_update_keys(dco_context_t *dco, struct tls_multi *multi); - +bool dco_update_keys(dco_context_t *dco, struct tls_multi *multi); /** * Install a new peer in DCO - to be called by a CLIENT (or P2P) instance * @@ -304,10 +306,11 @@ init_key_dco_bi(struct tls_multi *multi, struct key_state *ks, return 0; } -static inline void +static inline bool dco_update_keys(dco_context_t *dco, struct tls_multi *multi) { ASSERT(false); +return false; } static inline int diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c index 5cd7eaa6e..8c1e49a34 100644 --- a/src/openvpn/forward.c +++ b/src/openvpn/forward.c @@ -151,7 +151,12 @@ check_dco_key_status(struct context *c) return; } -dco_update_keys(&c->c1.tuntap->dco, c->c2.tls_multi); +if (!dco_update_keys(&c->c1.tuntap->dco, c->c2.tls_multi)) +{ +/* Something bad happened. Kill the connection to + * be able to recover. */ +register_signal(c, SIGUSR1, "dco update keys error"); +} } /* -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 1/4] Read DCO traffic stats from the kernel
Hi, On 05/12/2022 17:41, Kristof Provost via Openvpn-devel wrote: From: Kristof Provost When DCO is active userspace doesn't see all of the traffic, so when we access these stats we must update them. Retrieve kernel statistics every time we access the link_(read|write)_bytes values. Introduce a dco_(read|write)_bytes so that we don't clobber the existing statistics, which still count control packets, sent or received directly through the socket. Signed-off-by: Kristof Provost Acked-by: Antonio Quartulli --- src/openvpn/dco.h | 8 src/openvpn/dco_freebsd.c | 78 ++ src/openvpn/dco_linux.c| 7 +++ src/openvpn/dco_win.c | 7 +++ src/openvpn/multi.c| 30 +++-- src/openvpn/openvpn.h | 2 + src/openvpn/ovpn_dco_freebsd.h | 1 + 7 files changed, 120 insertions(+), 13 deletions(-) diff --git a/src/openvpn/dco.h b/src/openvpn/dco.h index e051db06..e5d89358 100644 --- a/src/openvpn/dco.h +++ b/src/openvpn/dco.h @@ -225,6 +225,14 @@ void dco_install_iroute(struct multi_context *m, struct multi_instance *mi, */ void dco_delete_iroutes(struct multi_context *m, struct multi_instance *mi); +/** + * Update traffic statistics for all peers + * + * @param dco DCO device context + * @param mthe server context + **/ +int dco_get_peer_stats(dco_context_t *dco, struct multi_context *m); + /** * Retrieve the list of ciphers supported by the current platform * diff --git a/src/openvpn/dco_freebsd.c b/src/openvpn/dco_freebsd.c index a52ac8c1..5b352859 100644 --- a/src/openvpn/dco_freebsd.c +++ b/src/openvpn/dco_freebsd.c @@ -37,6 +37,7 @@ #include "dco.h" #include "tun.h" #include "crypto.h" +#include "multi.h" #include "ssl_common.h" static nvlist_t * @@ -641,6 +642,83 @@ dco_event_set(dco_context_t *dco, struct event_set *es, void *arg) nvlist_destroy(nvl); } +static void +dco_update_peer_stat(struct multi_context *m, uint32_t peerid, const nvlist_t *nvl) +{ +struct hash_element *he; +struct hash_iterator hi; + +hash_iterator_init(m->hash, &hi); + +while ((he = hash_iterator_next(&hi))) +{ +struct multi_instance *mi = (struct multi_instance *) he->value; + +if (mi->context.c2.tls_multi->peer_id != peerid) +continue; + +mi->context.c2.dco_read_bytes = nvlist_get_number(nvl, "in"); +mi->context.c2.dco_write_bytes = nvlist_get_number(nvl, "out"); + +return; +} + +msg(M_INFO, "Peer %d returned by kernel, but not found locally", peerid); +} + +int +dco_get_peer_stats(dco_context_t *dco, struct multi_context *m) +{ + +struct ifdrv drv; +uint8_t buf[4096]; +nvlist_t *nvl; +const nvlist_t *const *nvpeers; +size_t npeers; +int ret; + +if (!dco || !dco->open) +{ +return 0; +} + +CLEAR(drv); +snprintf(drv.ifd_name, IFNAMSIZ, "%s", dco->ifname); +drv.ifd_cmd = OVPN_GET_PEER_STATS; +drv.ifd_len = sizeof(buf); +drv.ifd_data = buf; + +ret = ioctl(dco->fd, SIOCGDRVSPEC, &drv); +if (ret) +{ +msg(M_WARN | M_ERRNO, "Failed to get peer stats"); +return -EINVAL; +} + +nvl = nvlist_unpack(buf, drv.ifd_len, 0); +if (! nvl) +{ +msg(M_WARN, "Failed to unpack nvlist"); +return -EINVAL; +} + +if (! nvlist_exists_nvlist_array(nvl, "peers")) { +/* no peers */ +return 0; +} + +nvpeers = nvlist_get_nvlist_array(nvl, "peers", &npeers); +for (size_t i = 0; i < npeers; i++) +{ +const nvlist_t *peer = nvpeers[i]; +uint32_t peerid = nvlist_get_number(peer, "peerid"); + +dco_update_peer_stat(m, peerid, nvlist_get_nvlist(peer, "bytes")); +} + +return 0; +} + const char * dco_get_supported_ciphers() { diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c index 10935820..0306cec3 100644 --- a/src/openvpn/dco_linux.c +++ b/src/openvpn/dco_linux.c @@ -911,6 +911,13 @@ nla_put_failure: return ret; } +int +dco_get_peer_stats(dco_context_t *dco, struct multi_context *m) +{ +/* Not implemented. */ +return 0; +} + bool dco_available(int msglevel) { diff --git a/src/openvpn/dco_win.c b/src/openvpn/dco_win.c index 48a1755a..68ec931c 100644 --- a/src/openvpn/dco_win.c +++ b/src/openvpn/dco_win.c @@ -399,6 +399,13 @@ dco_do_write(dco_context_t *dco, int peer_id, struct buffer *buf) return 0; } +int +dco_get_peer_stats(dco_context_t *dco, struct multi_context *m) +{ +/* Not implemented. */ +return 0; +} + void dco_event_set(dco_context_t *dco, struct event_set *es, void *arg) { diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index 0a23c2bc..38
Re: [Openvpn-devel] [PATCH 1/4] Read DCO traffic stats from the kernel
Hi, On 13/12/2022 05:46, Gert Doering wrote: Hi, On Mon, Dec 12, 2022 at 09:53:36PM +0100, Antonio Quartulli wrote: On 05/12/2022 17:41, Kristof Provost via Openvpn-devel wrote: [cut] +int +dco_get_peer_stats(dco_context_t *dco, struct multi_context *m) +{ + +struct ifdrv drv; +uint8_t buf[4096]; +nvlist_t *nvl; +const nvlist_t *const *nvpeers; +size_t npeers; +int ret; + +if (!dco || !dco->open) +{ +return 0; +} + +CLEAR(drv); +snprintf(drv.ifd_name, IFNAMSIZ, "%s", dco->ifname); +drv.ifd_cmd = OVPN_GET_PEER_STATS; What speaks against having a simple GET_PEER here which returns the full peer object with all possible attributes? This way, if we want to retrieve another attribute in the future, this attribute will already be delivered by the same API, without the need to implement a new command each time. One counter-argument would be "the statistics are polled more often than 'all information about a peer object', so it should be less costly" It's a netlink/ioctl API: it's already costly and out of any fast-path, therefore I don't think this argument really plays an important role here. What is more important is designing an API that can last long, without the need for extra cluttering when we decide to implement something new. Ideally a GET_PEER command is pretty standard and can also be used for any kind of state inspection (i.e. even for 'debugging', although it's not the primary usage) (also, when do we ever poll 'all attributes'? Since OpenVPN *sets* these, we should know, except for the counters...) Like I said above, being this an API imho it is important to implement it in a way that can serve a reasonably broad set of use cases. (OpenVPN is the only user now, but may not be the case later) Cheers, -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 1/4] Read DCO traffic stats from the kernel
Hi, On 05/12/2022 17:41, Kristof Provost via Openvpn-devel wrote: [cut] + +int +dco_get_peer_stats(dco_context_t *dco, struct multi_context *m) +{ + +struct ifdrv drv; +uint8_t buf[4096]; +nvlist_t *nvl; +const nvlist_t *const *nvpeers; +size_t npeers; +int ret; + +if (!dco || !dco->open) +{ +return 0; +} + +CLEAR(drv); +snprintf(drv.ifd_name, IFNAMSIZ, "%s", dco->ifname); +drv.ifd_cmd = OVPN_GET_PEER_STATS; What speaks against having a simple GET_PEER here which returns the full peer object with all possible attributes? This way, if we want to retrieve another attribute in the future, this attribute will already be delivered by the same API, without the need to implement a new command each time. Cheers, -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH applied] Re: Disable DCO when TLS mode is not used
Hi, On 12/12/2022 09:32, Gert Doering wrote: Acked-by: Gert Doering Yeah, thanks :-) (tested on the "p2p --secret" server, still does the right thing. Have no "no secrets at all" setup, but from stare-at-code I see no reason why this wouldn't work as well) I know I am late to the party - but still wanted to give my virtual ACK Acked-by: Antonio Quartulli Thanks for cleaning after my half baked fix! Cheers, -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] disable DCO if --secret is specified
P2P mode with pre-shared key is deprecated, unsecure and should NOT be used. This said we still carry it around for a bit and we have to make sure it does not fights with DCO. Disable DCO at all when --secret is specified. Signed-off-by: Antonio Quartulli --- src/openvpn/dco.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/src/openvpn/dco.c b/src/openvpn/dco.c index d599dd7e..1cd698bf 100644 --- a/src/openvpn/dco.c +++ b/src/openvpn/dco.c @@ -274,6 +274,12 @@ dco_check_startup_option(int msglevel, const struct options *o) return false; } +if (o->shared_secret_file) +{ +msg(msglevel, "--secret is set. Disabling data channel offload"); +return false; +} + if (dev_type_enum(o->dev, o->dev_type) != DEV_TYPE_TUN) { msg(msglevel, "Note: dev-type not tun, disabling data channel offload."); -- 2.37.4 ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2 1/3] Move dco_installed from sock->info to sock->info.lsa.actual
Hi, On 26/10/2022 18:45, Arne Schwabe wrote: For tcp this makes no difference as the remote address of the socket never changes. For udp this allows OpenVPN to differentiate if a reconnecting client is using the same address as before or from a different one. This allow sending via the normal userspace socket in that case. Patch v2: fix windows code path Signed-off-by: Arne Schwabe I couldn't test this patch extensively, but it looks good to me. This basically completes what I discussed with Arne some weeks ago. Acked-by: Antonio Quartulli However, please ensure to get this patch through the test rig before merging. Regards, --- src/openvpn/dco.c | 7 --- src/openvpn/dco_linux.c | 2 +- src/openvpn/forward.c | 8 src/openvpn/init.c | 2 +- src/openvpn/mtcp.c | 6 +++--- src/openvpn/socket.c| 8 src/openvpn/socket.h| 12 ++-- 7 files changed, 23 insertions(+), 22 deletions(-) diff --git a/src/openvpn/dco.c b/src/openvpn/dco.c index a76cdd0cd..1f402bfc2 100644 --- a/src/openvpn/dco.c +++ b/src/openvpn/dco.c @@ -448,7 +448,7 @@ dco_p2p_add_new_peer(struct context *c) } c->c2.tls_multi->dco_peer_added = true; -c->c2.link_socket->info.dco_installed = true; +c->c2.link_socket->info.lsa->actual.dco_installed = true; return 0; } @@ -522,7 +522,7 @@ dco_multi_add_new_peer(struct multi_context *m, struct multi_instance *mi) { struct context *c = &mi->context; -int peer_id = mi->context.c2.tls_multi->peer_id; +int peer_id = c->c2.tls_multi->peer_id; struct sockaddr *remoteaddr, *localaddr = NULL; struct sockaddr_storage local = { 0 }; int sd = c->c2.link_socket->sd; @@ -531,6 +531,7 @@ dco_multi_add_new_peer(struct multi_context *m, struct multi_instance *mi) { /* the remote address will be inferred from the TCP socket endpoint */ remoteaddr = NULL; +c->c2.link_socket->info.lsa->actual.dco_installed = true; } else { @@ -575,7 +576,7 @@ dco_multi_add_new_peer(struct multi_context *m, struct multi_instance *mi) { msg(D_DCO|M_ERRNO, "error closing TCP socket after DCO handover"); } -c->c2.link_socket->info.dco_installed = true; +c->c2.link_socket->info.lsa->actual.dco_installed = true; c->c2.link_socket->sd = SOCKET_UNDEFINED; } diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c index 98e10507b..109358205 100644 --- a/src/openvpn/dco_linux.c +++ b/src/openvpn/dco_linux.c @@ -285,7 +285,7 @@ ovpn_nl_cb_finish(struct nl_msg (*msg) __attribute__ ((unused)), void *arg) * * We pass the error code to the user by means of a variable pointed by *arg * (supplied by the user when setting this callback) and we parse the kernel - * reply to see if it contains a human readable error. If found, it is printed. + * reply to see if it contains a human-readable error. If found, it is printed. */ static int ovpn_nl_cb_error(struct sockaddr_nl (*nla) __attribute__ ((unused)), diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c index 20d9a7598..622be8411 100644 --- a/src/openvpn/forward.c +++ b/src/openvpn/forward.c @@ -1643,13 +1643,13 @@ process_ip_header(struct context *c, unsigned int flags, struct buffer *buf) * standard Overlapped I/O. * * Hide that complexity (...especially if more platforms show up - * in future...) in a small inline function. + * in the future...) in a small inline function. */ static inline bool -should_use_dco_socket(struct link_socket *sock) +should_use_dco_socket(struct link_socket_actual *actual) { #if defined(TARGET_LINUX) || defined(TARGET_FREEBSD) -return sock->info.dco_installed; +return actual->dco_installed; #else return false; #endif @@ -1728,7 +1728,7 @@ process_outgoing_link(struct context *c) socks_preprocess_outgoing_link(c, &to_addr, &size_delta); /* Send packet */ -if (should_use_dco_socket(c->c2.link_socket)) +if (should_use_dco_socket(c->c2.to_link_addr)) { size = dco_do_write(&c->c1.tuntap->dco, c->c2.tls_multi->peer_id, diff --git a/src/openvpn/init.c b/src/openvpn/init.c index fe554ffd5..ac8a31055 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -3644,7 +3644,7 @@ do_close_link_socket(struct context *c) * closed in do_close_tun(). Set it to UNDEFINED so * we won't use WinSock API to close it. */ if (tuntap_is_dco_win(c->c1.tuntap) && c->c2.link_socket -&& c->c2.link_socket->info.dco_installed) +&& c->c2.link_socket->info.lsa->actual.dco_installed) { c-
Re: [Openvpn-devel] [PATCH] Fix logic error in checking early negotiation support check
Hi, On 16/11/2022 01:54, Arne Schwabe wrote: Without the == it is enough if any of the bits EARLY_NEG_START is set (0xf0), we want them all to be set. If EARLY_NEG_START were a flag/single bit, you would be right. Ouch, I indeed assumed it was 1 bit only.. Cheers, -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Fix logic error in checking early negotiation support check
Hi, On 15/11/2022 13:29, Arne Schwabe wrote: We want to check if EARLY_NEG_START is set and reserve the other bits for future expansions. Right now we also check if all reserved bits are zero. oops. Signed-off-by: Arne Schwabe --- src/openvpn/mudp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/openvpn/mudp.c b/src/openvpn/mudp.c index 7c6fc816e..bdf35a8ba 100644 --- a/src/openvpn/mudp.c +++ b/src/openvpn/mudp.c @@ -92,7 +92,7 @@ do_pre_decrypt_check(struct multi_context *m, ASSERT(packet_id_read(&pin, &tmp, true)); /* The most significant byte is 0x0f if early negotiation is supported */ -bool early_neg_support = (pin.id & EARLY_NEG_MASK) == EARLY_NEG_START; +bool early_neg_support = ((pin.id & EARLY_NEG_MASK) & EARLY_NEG_START) == EARLY_NEG_START; The "== EARLY_NEG_START" is not needed. On top of that, my brain parses the expression easier without that part, because the question is "after filtering using the mask, is EARLY_NEG_START set?". The "==" imho adds logic which is not needed (both at the code and at the brain level). Cheers, /* All clients that support early negotiation and tls-crypt are assumed * to also support resending the WKc in the 2nd packet */ -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2] Improve documentation for --dev and --dev-node.
Hi, On 08/11/2022 14:45, Gert Doering wrote: During the research for commit a5cf4cfb77f745 it turned out that OpenVPN's behaviour regarding "--dev arbitrary-name" is very platform-specific and not very well documented. The referenced commit fixed DCO behaviour to be in line with non-DCO linux behaviour, this commit catches up on the documentation. v2: disambiguate Linux ("all drivers") and FreeBSD ("only DCO"), add comment about --dev-type being necessary for devices not starting with tun* or tap* Signed-off-by: Gert Doering I think the proposed text is much better than before. After ironing out some details in v2 it got even clearer. I see no reason why it should not be merged in this form. Should somebody not feel it's 100% clear, a follow-up patch can always be sent to improve the text even more. Acked-by: Antonio Quartulli --- doc/man-sections/vpn-network-options.rst | 38 +++- 1 file changed, 31 insertions(+), 7 deletions(-) diff --git a/doc/man-sections/vpn-network-options.rst b/doc/man-sections/vpn-network-options.rst index 5b2f8470..72cf9064 100644 --- a/doc/man-sections/vpn-network-options.rst +++ b/doc/man-sections/vpn-network-options.rst @@ -69,15 +69,34 @@ routing. dev tap4 dev ovpn - When the device name starts with :code:`tun` or :code:`tap`, the device - type is extracted automatically. Otherwise the ``--dev-type`` option - needs to be added as well. + What happens if the device name is not :code:`tun` or :code:`tap` is + platform dependent. + + On most platforms, :code:`tunN` (e.g. tun2, tun30) and :code:`tapN` + (e.g. tap3) will create a numbered tun/tap interface with the number + specified - this is useful if multiple OpenVPN instances are active, + and the instance-to-device mapping needs to be known. Some platforms + do not support "numbered tap", so trying ``--dev tap3`` will fail. + + Arbitrary device names (e.g. ``--dev tun-home``) will only work on + FreeBSD (with the DCO kernel driver for ``tun`` devices) and Linux + (for both ``tun`` and ``tap`` devices, DCO and tun/tap driver). + + If such a device name starts with ``tun`` or ``tap`` (e.g. ``tun-home``), + OpenVPN will choose the right device type automatically. Otherwise the + desired device type needs to be specified with ``--dev-type tun`` or + ``--dev-type tap``. + + On Windows, only the names :code:`tun` and :code:`tap` are supported. + Selection among multiple installed drivers or driver instances is done + with ``--dev-node`` and ``--windows-driver``. --dev-node node - Explicitly set the device node rather than using :code:`/dev/net/tun`, - :code:`/dev/tun`, :code:`/dev/tap`, etc. If OpenVPN cannot figure out - whether ``node`` is a TUN or TAP device based on the name, you should - also specify ``--dev-type tun`` or ``--dev-type tap``. + This is a highly system dependent option to influence tun/tap driver + selection. + + On Linux, tun/tap devices are created by accessing :code:`/dev/net/tun`, + and this device name can be changed using ``--dev-node ...``. Under Mac OS X this option can be used to specify the default tun implementation. Using ``--dev-node utun`` forces usage of the native @@ -93,6 +112,11 @@ routing. both the network connections control panel name and the GUID for each TAP-Win32 adapter. + On other platforms, ``--dev-node node`` will influence the naming of the + created tun/tap device, if supported on that platform. If OpenVPN cannot + figure out whether ``node`` is a TUN or TAP device based on the name, + you should also specify ``--dev-type tun`` or ``--dev-type tap``. + --dev-type device-type Which device type are we using? ``device-type`` should be :code:`tun` (OSI Layer 3) or :code:`tap` (OSI Layer 2). Use this option only if -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH v2] close_tun: print interface type consistently in message
When closing the tunnel interface we know if we were using DCO or not. for this reason we can customize the closing message and make it consistent with the opening one. Signed-off-by: Antonio Quartulli --- Changes from v1: * use termary if instead of full blown if block --- src/openvpn/init.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/openvpn/init.c b/src/openvpn/init.c index c48048a1..01d6dab5 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -1946,7 +1946,9 @@ do_open_tun(struct context *c) static void do_close_tun_simple(struct context *c) { -msg(D_CLOSE, "Closing TUN/TAP interface"); +msg(D_CLOSE, "Closing %s interface", +dco_enabled(&c->options) ? "DCO": "TUN/TAP"); + if (c->c1.tuntap) { if (!c->options.ifconfig_noexec) -- 2.37.4 ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2] ovpn-dco: fix kernel 6.1 compile issue in ovpn_peer_create
Hi, On 22/10/2022 10:38, John Thomson wrote: Linux 5.19 replaces netif_tx_napi_add, but maintains a definition to the new function in: 58caed3dacb4 ("netdev: reshuffle netif_napi_add() APIs to allow dropping weigh") [0] Linux 6.1 removes netif_tx_napi_add in c3f760ef1287 ("net: remove netif_tx_napi_add()") [1] [0]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=58caed3dacb4354a25a1aa8d2febc3e9648ba1f4 [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c3f760ef128789252e7c4f10d3c1721422dceba9 Signed-off-by: John Thomson Thanks a lot for this! Your patch has been applied to the master branch with a few minor style fixes. --- v2: Do not throw ifdefs into the middle of the code compile tested only: openwrt toolchain aarch64: kernel v6.1rc1, kernel 5.10 Out of curiosity: are you using the openvpn-dev feed for openwqrt? (https://github.com/OpenVPN/openvpn-dev-openwrt) Cheers, -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] ovpn-dco: fix kernel 6.1 compile issue in ovpn_peer_create
Hi John, Thanks a lot for your contribution! However, we can't have #ifs like that in the middle of the code, as this code is expected to go upstream (and #ifs on the kernel version are not allowed). The code should always work with the latest kernel version, while any magic compatibility trick should be hidden in compat.h (i.e. implement netif_napi_add_tx() so that it calls netif_tx_napi_add(..., NAPI_POLL_WEIGHT)). Cheers, On 22/10/2022 08:16, John Thomson wrote: Linux 5.19 replaces netif_tx_napi_add, but maintains a definition to the new function in: 58caed3dacb4 ("netdev: reshuffle netif_napi_add() APIs to allow dropping weigh") [0] Linux 6.1 removes netif_tx_napi_add in c3f760ef1287 ("net: remove netif_tx_napi_add()") [1] [0]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=58caed3dacb4354a25a1aa8d2febc3e9648ba1f4 [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c3f760ef128789252e7c4f10d3c1721422dceba9 Signed-off-by: John Thomson --- compile test only, on kernel 6.1rc1 --- drivers/net/ovpn-dco/peer.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/net/ovpn-dco/peer.c b/drivers/net/ovpn-dco/peer.c index 5838286..c460976 100644 --- a/drivers/net/ovpn-dco/peer.c +++ b/drivers/net/ovpn-dco/peer.c @@ -88,8 +88,12 @@ static struct ovpn_peer *ovpn_peer_create(struct ovpn_struct *ovpn, u32 id) } /* configure and start NAPI */ +#if(LINUX_VERSION_CODE < KERNEL_VERSION(5,19,0)) netif_tx_napi_add(ovpn->dev, &peer->napi, ovpn_napi_poll, NAPI_POLL_WEIGHT); +#else + netif_napi_add_tx(ovpn->dev, &peer->napi, ovpn_napi_poll); +#endif napi_enable(&peer->napi); dev_hold(ovpn->dev); -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel