dco: backport immediate message processing to 2.6#945
dco: backport immediate message processing to 2.6#945alloc33 wants to merge 1 commit intoOpenVPN:release/2.6from
Conversation
Backport of commit 7791f53 from master. Currently, reading and processing of incoming DCO messages are decoupled: notifications are read, parsed, and the relevant information is stored in fields of dco_context_t for later processing. This approach is problematic on Linux, since libnl does not allow reading a single netlink message at a time, which can result in loss of information when multiple notifications are available. This change adopts a read -> parse -> process paradigm. On Linux, processing is now invoked directly from within the parsing callback. On Linux, a DEL_PEER notification from the kernel triggers a GET_PEER request from userspace, which can lead to errors when multiple simultaneous DEL_PEER notifications are received. To avoid this, introduce a lock that prevents requesting stats while still busy parsing other messages. Note: The 2.6 backport requires additional changes not present in the master commit because the multi context linkage infrastructure differs: - Added multi pointer to struct context (openvpn.h) - Set context linkages in init.c, mtcp.c, mudp.c - Added dco_linux.h changes for context pointer Reported-by: Stefan Baranoff <stefan.baranoff@trinitycyber.com> Github: OpenVPN#900 Github: OpenVPN#918 Github: OpenVPN#931 Github: fixes OpenVPN#919 (backport of commit 7791f53) Signed-off-by: Nikolai Shelekhov <nickshv13@icloud.com>
|
Thanks for the PR. It got lost in the preparations for 2.7.0 and 2.7.1 release. We discussed this today, and try to get it done "soon", so it can be released as 2.6.20. |
|
This looks good to me. I tested it with 50 clients all connecting and disconnecting simultaneously, and it showed no issues. The only suggestion I have is to add a debug print (like we have in v2.7) when the lock is taken and we cannot request stats, so the logs are easier to interpret. |
|
So, I have taken the patch from here, tried to apply it to release/2.6 "as of 246bc0e", and it fails with @ralflici can you help me figure out which commits I need to cherrypick to 2.6? What did you have in your tree when you tested this? |
|
I actually used the PR tree for testing and did not try applying the patches on top of v2.6. Technically, the PR includes the needed change in the DCO context, but only for Linux, while that part in From master, I can see that the commits adding the Looking more carefully, this line might also be incorrect, because it accesses a field that is defined only for Linux and is outside any So I think this PR should also be tested on the other platforms, and on a build with DCO disabled. |
…791f53 Change ovpn_dco_init() to take a reference to struct context, add a backlink from the platform DCO contexts to the owning openvpn context, and store the top multi context in struct context for server mode. This prepares the tree for the follow-up backend changes from commit 7791f53 ("dco: process messages immediately after read") where Linux and FreeBSD process DCO notifications directly from their backend read paths. It is part of a reworked backport of PR #945 originally proposed by Nikolai Shelekhov <nickshv13@icloud.com>. The original commits in master are commit a699681 Author: Antonio Quartulli <antonio@mandelbit.com> Date: Wed Jul 23 15:39:11 2025 +0200 dco: only pass struct context to init function commit 7f5a6de Author: Antonio Quartulli <antonio@mandelbit.com> Date: Wed Jul 23 08:10:25 2025 +0200 multi: store multi_context address inside top instance Change-Id: I974e10ec91a0b63f52387f1406ce1b49145eb0be Signed-off-by: Ralf Lici <ralf@mandelbit.com> Acked-by: Gert Doering <gert@greenie.muc.de> Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1631 Message-Id: <20260420165923.14226-1-gert@greenie.muc.de> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg36691.html Signed-off-by: Gert Doering <gert@greenie.muc.de>
Backport the immediate DCO message processing model from commit 7791f53 ("dco: process messages immediately after read"). Change the core DCO API from dco_do_read() to dco_read_and_process(), and make the backend read paths own the parsing and immediate handling of kernel notifications before handing the resulting state to process_incoming_dco() or multi_process_incoming_dco(). On Linux, install a permanent netlink callback and process GET_PEER and DEL_PEER messages as they are received, instead of switching callbacks for each read and deferring handling through shared DCO message state. Also add a small guard to avoid requesting peer stats while libnl is still parsing a batch of notifications. On FreeBSD, move notification handling into dco_read_and_process() and update peer statistics directly from the backend instead of storing temporary byte counters in dco_context_t. This commit is part of a reworked backport of PR #945 originally proposed by Nikolai Shelekhov <nickshv13@icloud.com>. Github: #900 Github: #918 Github: #931 Github: fixes #919 Github: closes #945 Change-Id: I92c5f74a27b40fede204f714b042a6cc80b3703e Signed-off-by: Ralf Lici <ralf@mandelbit.com> Acked-by: Gert Doering <gert@greenie.muc.de> Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1632 Message-Id: <20260421084906.5720-1-gert@greenie.muc.de> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg36707.html Signed-off-by: Gert Doering <gert@greenie.muc.de>
|
So, @ralflici split the work into two - infrastructure ("introduce commit adece45 commit e78a8af commit 876a8cf ... so we should have properly-functioning DCO event handling for Linux and FreeBSD in the release/2.6 tree now, without breaking compilation on FreeBSD or Windows. Thanks, @alloc33 for your help in actually making it here ;-) - I will close this PR now (as there is no more code to be merged) and leave open the issues until we have confirmation that things work correctly now. 2.6.20 with the fixes is supposed to arrive in a few hours. |
Backport of commit 7791f53 from master.
Currently, reading and processing of incoming DCO messages are decoupled: notifications are read, parsed, and the relevant information is stored in fields of dco_context_t for later processing. This approach is problematic on Linux, since libnl does not allow reading a single netlink message at a time, which can result in loss of information when multiple notifications are available.
This change adopts a read -> parse -> process paradigm. On Linux, processing is now invoked directly from within the parsing callback.
On Linux, a DEL_PEER notification from the kernel triggers a GET_PEER request from userspace, which can lead to errors when multiple simultaneous DEL_PEER notifications are received. To avoid this, introduce a lock that prevents requesting stats while still busy parsing other messages.
Note: The 2.6 backport requires additional changes not present in the master commit because the multi context linkage infrastructure differs:
Reported-by: Stefan Baranoff stefan.baranoff@trinitycyber.com
Github: #900
Github: #918
Github: #931
Github: fixes #919
(backport of commit 7791f53)
Thank you for your contribution
You are welcome to open PR, but they are used for discussion only. All
patches must eventually go to the openvpn-devel mailing list for review:
Please send your patch using git-send-email. For example to send your latest commit to the list:
For details, see these Wiki articles: