Menu
▾
▴
openvpn-devel
|
From: ordex (C. Review) <ge...@op...> - 2025-07-26 00:01:34
|
Attention is currently required from: flichtenheld, plaisthos.
Hello plaisthos, flichtenheld,
I'd like you to do a code review.
Please visit
http://gerrit.openvpn.net/c/openvpn/+/1116?usp=email
to review the following change.
Change subject: dco: drop client prefix after DCO PEER_FLOAT notification
......................................................................
dco: drop client prefix after DCO PEER_FLOAT notification
multi_process_float() will possibly float a client and
also generate and set its new prefix.
However, after processing the PEER_FLOAT_NTF message, we
were not clearing the prefix, thus effectivly making
the generated prefix permanent until the next set_prefix()
call.
Clear the prefix right after calling multi_process_float()
to avoid printing messages with the wrong prefix.
Github: closes OpenVPN/openvpn#799
Change-Id: I1ad5df0f6785ffe9becd9f83329a9335d1a36f24
Signed-off-by: Antonio Quartulli <an...@ma...>
---
M src/openvpn/multi.c
1 file changed, 4 insertions(+), 0 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/16/1116/1
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index d23aee7..32331d7 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -3417,6 +3417,10 @@
&m->top.c2.from.dest,
(struct sockaddr *)&dco->dco_float_peer_ss);
multi_process_float(m, mi, mi->context.c2.link_sockets[0]);
+ /* multi_process_float() generated and set a new peer prefix, but we
+ * don't to keep it at this point.
+ */
+ clear_prefix();
CLEAR(dco->dco_float_peer_ss);
}
#endif /* if defined(TARGET_LINUX) || defined(TARGET_WIN32) */
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1116?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I1ad5df0f6785ffe9becd9f83329a9335d1a36f24
Gerrit-Change-Number: 1116
Gerrit-PatchSet: 1
Gerrit-Owner: ordex <an...@ma...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-MessageType: newchange
|
|
From: cron2 (C. Review) <ge...@op...> - 2025-07-27 10:24:21
|
Attention is currently required from: flichtenheld, ordex, plaisthos. cron2 has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/1116?usp=email ) Change subject: dco: drop client prefix after DCO PEER_FLOAT notification ...................................................................... Patch Set 2: Code-Review+2 (1 comment) Patchset: PS2: This does fix the problem observed. I'm not 100% sure it is the best place to reset the prefix (maybe we want to do it for incoming DCO messages instead?), but let's fix the obvious issue first. -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1116?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I1ad5df0f6785ffe9becd9f83329a9335d1a36f24 Gerrit-Change-Number: 1116 Gerrit-PatchSet: 2 Gerrit-Owner: ordex <an...@ma...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: ordex <an...@ma...> Gerrit-Comment-Date: Sun, 27 Jul 2025 10:24:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment |
|
From: Gert D. <ge...@gr...> - 2025-07-27 10:24:28
|
From: Antonio Quartulli <an...@ma...> multi_process_float() will possibly float a client and also generate and set its new prefix. However, after processing the PEER_FLOAT_NTF message, we were not clearing the prefix, thus effectivly making the generated prefix permanent until the next set_prefix() call. Clear the prefix right after calling multi_process_float() to avoid printing messages with the wrong prefix. Github: closes OpenVPN/openvpn#799 Change-Id: I1ad5df0f6785ffe9becd9f83329a9335d1a36f24 Signed-off-by: Antonio Quartulli <an...@ma...> Acked-by: Gert Doering <ge...@gr...> --- This change was reviewed on Gerrit and approved by at least one developer. I request to merge it to master. Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1116 This mail reflects revision 2 of this Change. Acked-by according to Gerrit (reflected above): Gert Doering <ge...@gr...> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index c90ed5b..44210cb 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -3417,6 +3417,10 @@ &m->top.c2.from.dest, (struct sockaddr *)&dco->dco_float_peer_ss); multi_process_float(m, mi, mi->context.c2.link_sockets[0]); + /* multi_process_float() generated and set a new peer prefix, but we + * don't to keep it at this point. + */ + clear_prefix(); CLEAR(dco->dco_float_peer_ss); } #endif /* if defined(TARGET_LINUX) || defined(TARGET_WIN32) */ |
|
From: cron2 (C. Review) <ge...@op...> - 2025-07-27 10:39:11
|
Attention is currently required from: flichtenheld, ordex, plaisthos. cron2 has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/1116?usp=email ) Change subject: dco: drop client prefix after DCO PEER_FLOAT notification ...................................................................... Patch Set 2: Code-Review-1 (1 comment) Patchset: PS2: Actually it is just fixing one particular symptom, but we can get there in other paths as well - here's a reconnect that happened to trigger the "query stats now!" timer ``` Jul 27 12:14:55 ubuntu2004 tun-udp-p2mp[297683]: freebsd-74-amd64/udp6:194.97.140.3:62210 peer-id=2 sitnl_send: checking for received messages Jul 27 12:14:55 ubuntu2004 tun-udp-p2mp[297683]: freebsd-74-amd64/udp6:194.97.140.3:62210 peer-id=2 sitnl_send: rtnl: received 36 bytes Jul 27 12:14:55 ubuntu2004 tun-udp-p2mp[297683]: freebsd-74-amd64/udp6:194.97.140.3:62210 peer-id=2 net_route_v6_del: fd00:abcd:220:201::/64 via fd00:abcd:220:2::1006 dev tun1 table 0 metric 100 Jul 27 12:14:55 ubuntu2004 tun-udp-p2mp[297683]: freebsd-74-amd64/udp6:194.97.140.3:62210 peer-id=2 sitnl_send: checking for received messages Jul 27 12:14:55 ubuntu2004 tun-udp-p2mp[297683]: freebsd-74-amd64/udp6:194.97.140.3:62210 peer-id=2 sitnl_send: rtnl: received 36 bytes Jul 27 12:14:55 ubuntu2004 tun-udp-p2mp[297683]: freebsd-74-amd64/udp6:194.97.140.3:62210 peer-id=2 net_route_v6_del: fd00:abcd:220:200::74/128 via fd00:abcd:220:2::1006 dev tun1 table 0 metric 100 Jul 27 12:14:55 ubuntu2004 tun-udp-p2mp[297683]: freebsd-74-amd64/udp6:194.97.140.3:62210 peer-id=2 sitnl_send: checking for received messages Jul 27 12:14:55 ubuntu2004 tun-udp-p2mp[297683]: freebsd-74-amd64/udp6:194.97.140.3:62210 peer-id=2 sitnl_send: rtnl: received 36 bytes Jul 27 12:14:55 ubuntu2004 tun-udp-p2mp[297683]: freebsd-74-amd64/udp6:194.97.140.3:62210 peer-id=2 dco_get_peer: peer-id -1 Jul 27 12:14:55 ubuntu2004 tun-udp-p2mp[297683]: freebsd-74-amd64/udp6:194.97.140.3:62210 peer-id=2 ovpn-dco: received netlink message type=31 cmd=3 flags=0x0002 Jul 27 12:14:55 ubuntu2004 tun-udp-p2mp[297683]: freebsd-74-amd64/udp6:194.97.140.3:62210 peer-id=2 ovpn_handle_peer: parsing message for peer 0... Jul 27 12:14:55 ubuntu2004 tun-udp-p2mp[297683]: freebsd-74-amd64/udp6:194.97.140.3:62210 peer-id=2 ovpn_handle_peer: received data for a non-existing peer 0 Jul 27 12:14:55 ubuntu2004 tun-udp-p2mp[297683]: freebsd-74-amd64/udp6:194.97.140.3:62210 peer-id=2 ovpn-dco: received netlink message type=31 cmd=3 flags=0x0002 Jul 27 12:14:55 ubuntu2004 tun-udp-p2mp[297683]: freebsd-74-amd64/udp6:194.97.140.3:62210 peer-id=2 ovpn_handle_peer: parsing message for peer 1... Jul 27 12:14:55 ubuntu2004 tun-udp-p2mp[297683]: freebsd-74-amd64/udp6:194.97.140.3:62210 peer-id=2 dco_update_peer_stat / dco_read_bytes(1): 1064936 Jul 27 12:14:55 ubuntu2004 tun-udp-p2mp[297683]: freebsd-74-amd64/udp6:194.97.140.3:62210 peer-id=2 dco_update_peer_stat / dco_write_bytes(1): 1064360 Jul 27 12:14:55 ubuntu2004 tun-udp-p2mp[297683]: freebsd-74-amd64/udp6:194.97.140.3:62210 peer-id=2 dco_update_peer_stat / tun_read_bytes(1): 1040000 Jul 27 12:14:55 ubuntu2004 tun-udp-p2mp[297683]: freebsd-74-amd64/udp6:194.97.140.3:62210 peer-id=2 dco_update_peer_stat / tun_write_bytes(1): 1040000 ``` no float involved, but still we have a "peer-id=2" prefix for "peer 1" counters. So I would suggest to move the `clear_prefix()` call into the DCO functions that deal with incoming messages, most of which will not deal with "the client that is currently listed in the prefix". Maybe `ovpn_handle_msg()` (which would need the quivalent patch for FreeBSD and Windows)? Or something generic in `dco.c` that deals with DCO events? -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1116?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I1ad5df0f6785ffe9becd9f83329a9335d1a36f24 Gerrit-Change-Number: 1116 Gerrit-PatchSet: 2 Gerrit-Owner: ordex <an...@ma...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: ordex <an...@ma...> Gerrit-Comment-Date: Sun, 27 Jul 2025 10:39:01 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment |
|
From: cron2 (C. Review) <ge...@op...> - 2025-07-27 11:02:45
|
Attention is currently required from: flichtenheld, ordex, plaisthos. cron2 has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/1116?usp=email ) Change subject: dco: drop client prefix after DCO PEER_FLOAT notification ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: Here's another one... counter timer triggering while an outgoing TLS renegotiation is in progress ``` Jul 27 12:33:36 ubuntu2004 tun-udp-p2mp[298589]: udp6:[2001:608:0:814::fb00:14]:33827 Control Channel: TLSv1.3, cipher TLSv1.3 TLS_AES_256_GCM_SHA384, peer certificate: 2048 bits RSA, signature: RSA-SHA256, peer temporary key: 253 bits X25519, peer signing digest/type: SHA256 RSASSA-PSS Jul 27 12:33:36 ubuntu2004 kernel: [443346.370968] tun1: del peer 1 Jul 27 12:33:36 ubuntu2004 kernel: [443346.370974] tun1: deleting peer with id 1, reason 1 Jul 27 12:33:36 ubuntu2004 tun-udp-p2mp[298589]: udp6:[2001:608:0:814::fb00:14]:33827 [freebsd-14-amd64] Peer Connection Initiated with [AF_INET6]2001:608:0:814::fb00:14:33827 Jul 27 12:33:36 ubuntu2004 tun-udp-p2mp[298589]: freebsd-14-amd64/udp6:[2001:608:0:814::fb00:14]:33827 peer-id=2 dco_get_peer: peer-id -1 Jul 27 12:33:36 ubuntu2004 tun-udp-p2mp[298589]: freebsd-14-amd64/udp6:[2001:608:0:814::fb00:14]:33827 peer-id=2 ovpn-dco: received netlink message type=31 cmd=3 flags=0x0002 Jul 27 12:33:36 ubuntu2004 tun-udp-p2mp[298589]: freebsd-14-amd64/udp6:[2001:608:0:814::fb00:14]:33827 peer-id=2 ovpn_handle_peer: parsing message for peer 0... Jul 27 12:33:36 ubuntu2004 tun-udp-p2mp[298589]: freebsd-14-amd64/udp6:[2001:608:0:814::fb00:14]:33827 peer-id=2 dco_update_peer_stat / dco_read_bytes(0): 440 Jul 27 12:33:36 ubuntu2004 tun-udp-p2mp[298589]: freebsd-14-amd64/udp6:[2001:608:0:814::fb00:14]:33827 peer-id=2 dco_update_peer_stat / dco_write_bytes(0): 480 ``` in this case resetting the prefix would mess up prefix logging for the TLS handshake, so it's not the right approach anyway. Digging through error.c I found something half-forgotten... ``` /* set up client prefix */ if (flags & M_NOIPREFIX) { prefix = NULL; } else { prefix = msg_get_prefix(); } ``` so I think the *right* approach is to use `msg(...|M_NOIPREFIX, ...)` for everything that is not normally related to a particular MI instance - like, most of the DCO events. Magic -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1116?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I1ad5df0f6785ffe9becd9f83329a9335d1a36f24 Gerrit-Change-Number: 1116 Gerrit-PatchSet: 2 Gerrit-Owner: ordex <an...@ma...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: ordex <an...@ma...> Gerrit-Comment-Date: Sun, 27 Jul 2025 11:02:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment |
|
From: ordex (C. Review) <ge...@op...> - 2025-07-28 11:38:04
|
Attention is currently required from: cron2, flichtenheld, plaisthos. ordex has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/1116?usp=email ) Change subject: dco: drop client prefix after DCO PEER_FLOAT notification ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: > Actually it is just fixing one particular symptom, but we can get there in other paths as well - her […] These are 2 different problems: 1) when we float we set an unexpected prefix and we stick to it. 2) when we parse notifications we may use the prefix coming from another instance. This patch is trying to fix problem 1, which is similar but different from problem 2. -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1116?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I1ad5df0f6785ffe9becd9f83329a9335d1a36f24 Gerrit-Change-Number: 1116 Gerrit-PatchSet: 2 Gerrit-Owner: ordex <an...@ma...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: cron2 <ge...@gr...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Comment-Date: Mon, 28 Jul 2025 11:37:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: cron2 <ge...@gr...> Gerrit-MessageType: comment |
|
From: ordex (C. Review) <ge...@op...> - 2025-07-28 11:39:50
|
Attention is currently required from: cron2, flichtenheld, plaisthos. ordex has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/1116?usp=email ) Change subject: dco: drop client prefix after DCO PEER_FLOAT notification ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: > These are 2 different problems: […] After fixing problem 1) we should heck what remains of problem 2, because we execute DCO commands always in the context of a known peer. We can't truly jump from peer X to peer Y. Also when doing a PEER_GET with id=-1 (dump all peers), we should not have any prefix set. If we had one imho it was a leftover from the float (problem 1) -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1116?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I1ad5df0f6785ffe9becd9f83329a9335d1a36f24 Gerrit-Change-Number: 1116 Gerrit-PatchSet: 2 Gerrit-Owner: ordex <an...@ma...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: cron2 <ge...@gr...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Comment-Date: Mon, 28 Jul 2025 11:39:36 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: cron2 <ge...@gr...> Comment-In-Reply-To: ordex <an...@ma...> Gerrit-MessageType: comment |
×
Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.
Thanks for helping keep SourceForge clean.
X