Thread: [Openvpn-devel] [XS] Change in openvpn[master]: improve float collision logging

Robust and flexible VPN network tunnelling

Brought to you by: dazo, djpig, ericcrist, jimyonan, mattock

openvpn-devel

[Openvpn-devel] [XS] Change in openvpn[master]: improve float collision logging

From: ralf_lici (C. Review) <ge...@op...> - 2025-07-18 15:08:30

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/+/1091?usp=email

to review the following change.


Change subject: improve float collision logging
......................................................................

improve float collision logging

Extend the log message printed when an instance floats to an address
already taken by another instance with the same certificate. The updated
message now includes the instance being closed, the reason it's being
closed, and the new instance taking over that address.

Change-Id: I217cfb319b85fd75a88f7d4d50c374d28771df28
Signed-off-by: Ralf Lici <ra...@ma...>
---
M src/openvpn/multi.c
1 file changed, 5 insertions(+), 1 deletion(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/91/1091/1

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index ead3dd0..4696686 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -3263,7 +3263,11 @@
                 mroute_addr_print(&mi->real, &gc));
             goto done;
         }
-        msg(D_MULTI_MEDIUM, "closing instance %s", multi_instance_string(ex_mi, false, &gc));
+
+        msg(D_MULTI_LOW, "closing instance %s due to float collision with %s "
+            "using the same certificate",
+            multi_instance_string(ex_mi, false, &gc),
+            multi_instance_string(mi, false, &gc));
         multi_close_instance(m, ex_mi, false);
     }
 

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1091?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: I217cfb319b85fd75a88f7d4d50c374d28771df28
Gerrit-Change-Number: 1091
Gerrit-PatchSet: 1
Gerrit-Owner: ralf_lici <ra...@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

[Openvpn-devel] [XS] Change in openvpn[master]: improve float collision logging

From: cron2 (C. Review) <ge...@op...> - 2025-07-18 19:17:02

Attention is currently required from: flichtenheld, plaisthos, ralf_lici.

cron2 has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/1091?usp=email )

Change subject: improve float collision logging
......................................................................


Patch Set 1: Code-Review+2

(1 comment)

Patchset:

PS1: 
works!

Disabled the "goto done" part from the "same peer-id" check, and played with the LAN cable :-) - resulting in

```
2025-07-18 21:14:25 us=793140 cron2-freebsd-tc-amd64/udp6:193.149.48.173:49409 peer-id=0 disallowing peer 0 (cron2-freebsd-tc-amd64) from floating to its own address (udp6:193.149.48.173:49409)
2025-07-18 21:14:25 us=793175 cron2-freebsd-tc-amd64/udp6:193.149.48.173:49409 peer-id=0 closing instance cron2-freebsd-tc-amd64/udp6:193.149.48.173:49409 peer-id=0 due to float collision with cron2-freebsd-tc-amd64/udp6:193.149.48.173:49409 peer-id=0 using the same certificate
```

(and then, of course, it SIGSEGVs, but that's expected - so the extra logging makes this a much clearer picture.  Could not reproduce with two different clients, but I don't think this will add extra testing coverage)



-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1091?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: I217cfb319b85fd75a88f7d4d50c374d28771df28
Gerrit-Change-Number: 1091
Gerrit-PatchSet: 1
Gerrit-Owner: ralf_lici <ra...@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: ralf_lici <ra...@ma...>
Gerrit-Comment-Date: Fri, 18 Jul 2025 19:16:48 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[Openvpn-devel] [PATCH v1] improve float collision logging

From: Gert D. <ge...@gr...> - 2025-07-18 19:17:17

From: Ralf Lici <ra...@ma...>

Extend the log message printed when an instance floats to an address
already taken by another instance with the same certificate. The updated
message now includes the instance being closed, the reason it's being
closed, and the new instance taking over that address.

Change-Id: I217cfb319b85fd75a88f7d4d50c374d28771df28
Signed-off-by: Ralf Lici <ra...@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/+/1091
This mail reflects revision 1 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 ead3dd0..4696686 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -3263,7 +3263,11 @@
                 mroute_addr_print(&mi->real, &gc));
             goto done;
         }
-        msg(D_MULTI_MEDIUM, "closing instance %s", multi_instance_string(ex_mi, false, &gc));
+
+        msg(D_MULTI_LOW, "closing instance %s due to float collision with %s "
+            "using the same certificate",
+            multi_instance_string(ex_mi, false, &gc),
+            multi_instance_string(mi, false, &gc));
         multi_close_instance(m, ex_mi, false);
     }

[Openvpn-devel] [PATCH applied] Re: improve float collision logging

From: Gert D. <ge...@gr...> - 2025-07-18 19:21:37

Just a small commit but would have saved us a few hours staring at logs
today - due to a different bug, the floating code would try to "float to
itself", find "that ip+port is already in use", kill 'that' instance
(itself), and then crash on "where did my instance go?"...  and we just
got an ominous "closing instance" with no reason.

Verified that triggering the (already fixed) bug with the new message
would produce a nicer and more helpful message.  Good :-)

Your patch has been applied to the master branch.

commit 495e02e675a1c64ac902428ac82f6b1bfd35ed25
Author: Ralf Lici
Date:   Fri Jul 18 21:16:56 2025 +0200

     improve float collision logging

     Signed-off-by: Ralf Lici <ra...@ma...>
     Acked-by: Gert Doering <ge...@gr...>
     Message-Id: <202...@gr...>
     URL: https://www.mail-archive.com/ope...@li.../msg32226.html
     Signed-off-by: Gert Doering <ge...@gr...>


--
kind regards,

Gert Doering

[Openvpn-devel] [XS] Change in openvpn[master]: improve float collision logging

From: cron2 (C. Review) <ge...@op...> - 2025-07-18 19:21:51

cron2 has submitted this change. ( http://gerrit.openvpn.net/c/openvpn/+/1091?usp=email )

Change subject: improve float collision logging
......................................................................

improve float collision logging

Extend the log message printed when an instance floats to an address
already taken by another instance with the same certificate. The updated
message now includes the instance being closed, the reason it's being
closed, and the new instance taking over that address.

Change-Id: I217cfb319b85fd75a88f7d4d50c374d28771df28
Signed-off-by: Ralf Lici <ra...@ma...>
Acked-by: Gert Doering <ge...@gr...>
Message-Id: <202...@gr...>
URL: https://www.mail-archive.com/ope...@li.../msg32226.html
Signed-off-by: Gert Doering <ge...@gr...>
---
M src/openvpn/multi.c
1 file changed, 5 insertions(+), 1 deletion(-)




diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index ead3dd0..4696686 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -3263,7 +3263,11 @@
                 mroute_addr_print(&mi->real, &gc));
             goto done;
         }
-        msg(D_MULTI_MEDIUM, "closing instance %s", multi_instance_string(ex_mi, false, &gc));
+
+        msg(D_MULTI_LOW, "closing instance %s due to float collision with %s "
+            "using the same certificate",
+            multi_instance_string(ex_mi, false, &gc),
+            multi_instance_string(mi, false, &gc));
         multi_close_instance(m, ex_mi, false);
     }
 

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1091?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: I217cfb319b85fd75a88f7d4d50c374d28771df28
Gerrit-Change-Number: 1091
Gerrit-PatchSet: 2
Gerrit-Owner: ralf_lici <ra...@ma...>
Gerrit-Reviewer: cron2 <ge...@gr...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-MessageType: merged

[Openvpn-devel] [XS] Change in openvpn[master]: improve float collision logging

From: cron2 (C. Review) <ge...@op...> - 2025-07-18 19:21:55

cron2 has uploaded a new patch set (#2) to the change originally created by ralf_lici. ( http://gerrit.openvpn.net/c/openvpn/+/1091?usp=email )

The following approvals got outdated and were removed:
Code-Review+2 by cron2


Change subject: improve float collision logging
......................................................................

improve float collision logging

Extend the log message printed when an instance floats to an address
already taken by another instance with the same certificate. The updated
message now includes the instance being closed, the reason it's being
closed, and the new instance taking over that address.

Change-Id: I217cfb319b85fd75a88f7d4d50c374d28771df28
Signed-off-by: Ralf Lici <ra...@ma...>
Acked-by: Gert Doering <ge...@gr...>
Message-Id: <202...@gr...>
URL: https://www.mail-archive.com/ope...@li.../msg32226.html
Signed-off-by: Gert Doering <ge...@gr...>
---
M src/openvpn/multi.c
1 file changed, 5 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/91/1091/2

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index ead3dd0..4696686 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -3263,7 +3263,11 @@
                 mroute_addr_print(&mi->real, &gc));
             goto done;
         }
-        msg(D_MULTI_MEDIUM, "closing instance %s", multi_instance_string(ex_mi, false, &gc));
+
+        msg(D_MULTI_LOW, "closing instance %s due to float collision with %s "
+            "using the same certificate",
+            multi_instance_string(ex_mi, false, &gc),
+            multi_instance_string(mi, false, &gc));
         multi_close_instance(m, ex_mi, false);
     }
 

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1091?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: I217cfb319b85fd75a88f7d4d50c374d28771df28
Gerrit-Change-Number: 1091
Gerrit-PatchSet: 2
Gerrit-Owner: ralf_lici <ra...@ma...>
Gerrit-Reviewer: cron2 <ge...@gr...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-MessageType: newpatchset