openvpn-devel — OpenVPN developers list

[Openvpn-devel] [S] Change in openvpn[master]: GHA: Dependency updates July 2025

[uddr (C. Review) <ge...@op...>]

Attention is currently required from: flichtenheld, plaisthos.

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

Change subject: GHA: Dependency updates July 2025
......................................................................


Patch Set 3: Code-Review+2


-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1079?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: I6122225cc12c4f299a2a48db24bc7379ac6c5921
Gerrit-Change-Number: 1079
Gerrit-PatchSet: 3
Gerrit-Owner: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-Reviewer: uddr <yur...@gm...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Comment-Date: Tue, 22 Jul 2025 10:20:02 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[Openvpn-devel] [S] Change in openvpn[release/2.6]: GHA: Update dependencies July 2025 (2.6)

[uddr (C. Review) <ge...@op...>]

Attention is currently required from: flichtenheld, plaisthos.

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

Change subject: GHA: Update dependencies July 2025 (2.6)
......................................................................


Patch Set 1: Code-Review+2


-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1080?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: release/2.6
Gerrit-Change-Id: I46177b0614ad8b167a421c50d3cc8e7da4054e42
Gerrit-Change-Number: 1080
Gerrit-PatchSet: 1
Gerrit-Owner: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-Reviewer: uddr <yur...@gm...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Comment-Date: Tue, 22 Jul 2025 10:19:22 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[Openvpn-devel] [L] Change in openvpn[master]: PUSH_UPDATE message sender: enabling the server to send PUSH_UPDATE c...

[cron2 (C. Review) <ge...@op...>]

Attention is currently required from: flichtenheld, mrbff, plaisthos, stipa.

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

Change subject: PUSH_UPDATE message sender: enabling the server to send PUSH_UPDATE control messages
......................................................................


Patch Set 16: Code-Review-1

(13 comments)

Patchset:

PS16: 
lots of string and buffer related nagging, plus some general questioning if we need this in the full breadth...


File doc/management-notes.txt:

http://gerrit.openvpn.net/c/openvpn/+/869/comment/a8274ef9_1e452d98 :
PS16, Line 1075: 
I do question whether this is a reasonable approach, to have "by cn" and "by addr" functions, that add code (... that needs to be tested and maintained) when a management client can just look up the CID itself, and then do `push-update-cid`?

Is this something AS wants?  Or "let's just do all variants, so nobody complains about a missing feature"?


File src/openvpn/manage.c:

http://gerrit.openvpn.net/c/openvpn/+/869/comment/54080c2e_539d2c6e :
PS16, Line 1362:         }
I bet there are more elegant and less repetitive ways to do 4 different `if (status) print(SUCCESS!) else print(ERROR)` blocks...


File src/openvpn/push.h:

http://gerrit.openvpn.net/c/openvpn/+/869/comment/f1e00c73_49d8e6c7 :
PS16, Line 54: 
This does confuse me a bit.  If we have no ENABLE_MANAGEMENT, why would we have UPD_BY_ADDR or UPD_BY_CN?  Is this usable from a plugin interface or a script?


File src/openvpn/push_util.c:

http://gerrit.openvpn.net/c/openvpn/+/869/comment/fb47d1a4_53787e8b :
PS16, Line 66: /* Allocate memory and asseble the final message */
typo


http://gerrit.openvpn.net/c/openvpn/+/869/comment/384c531b_cb1d0b7b :
PS16, Line 82:     }
it might increase readability to juse use `snprintf( ... "%s,%s%s", push_update_cmd, src, continuation? continuation: 0)` here... and get rid of `i`.

Or use `buf_printf()` and get rid of the length and malloc stuff...  (returning `BSTR(&buf)` then).


http://gerrit.openvpn.net/c/openvpn/+/869/comment/e1cd130c_c8e39305 :
PS16, Line 122:             msgs[im] = forge_msg(str, ",push-continuation 2", gc);
I do wonder if this can be done without modifying the string, so getting rid of the `strdup()` requirement...


http://gerrit.openvpn.net/c/openvpn/+/869/comment/5e9e0424_e91519f2 :
PS16, Line 143: /* It actually send the already divided messagge to one single client */
This comment needs a visit from the grammar police ;-)

```
/* send the message(s) prepared to one single client */
```


http://gerrit.openvpn.net/c/openvpn/+/869/comment/648be3fc_6ea876c8 :
PS16, Line 158:         buf_write(&buf, msgs[i], strlen(msgs[i]));
If you want to have the message in a `buf` here, you could have `forge_msg()` just use `buf_printf()` and return the resulting `buf`...?


http://gerrit.openvpn.net/c/openvpn/+/869/comment/caf4cb50_9100e63f :
PS16, Line 165:         /* After sending the control message, we update the options server-side in the client's context */
Can you extend the comment a bit on why this is needed?  I assume (which might or might not be a bad idea) that this is so `ifconfig-push` and `ifconfig-ipv6-push`  can work?  `cipher` etc. is not PUSH_UPDATE-able, and most other options the server should not care about?  But without a bit of more specific explanation, this is very, uh, non-intuitive stuff.


http://gerrit.openvpn.net/c/openvpn/+/869/comment/693c5314_a1a08fa4 :
PS16, Line 204:     if (!message_splitter(gc_strdup(msg, &gc), msgs, &gc, safe_cap))
Please do not call `strdup()` in a function call... yes, this works, but it is hard to read and no gain compared to "just call strdup() at the beginning of `message_splutter()`  (if it turns out to be necessary).


http://gerrit.openvpn.net/c/openvpn/+/869/comment/8aa9f5e8_3e0aac7c :
PS16, Line 239: 
how can we ever end here if ENABLE_MANAGEMENT is not defined?  Except for the unit test, there is no caller remaining...


http://gerrit.openvpn.net/c/openvpn/+/869/comment/ecc7ac6a_56bfc11e :
PS16, Line 305:     RETURN_UPDATE_STATUS(n_sent);
so here we have this nice if/else macro that manage.c could use :-)... but only 3 out of 4 functions use it?  Can we not do the "does this CID exist?" check in the caller (for `management_callback_send_push_update_by_cid()`)?



-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/869?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: Ie82bcc7a8e583de9156b185d71d1a323ed8df3fc
Gerrit-Change-Number: 869
Gerrit-PatchSet: 16
Gerrit-Owner: mrbff <ma...@ma...>
Gerrit-Reviewer: cron2 <ge...@gr...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-Reviewer: stipa <lst...@gm...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Attention: mrbff <ma...@ma...>
Gerrit-Attention: stipa <lst...@gm...>
Gerrit-Comment-Date: Mon, 21 Jul 2025 19:53:36 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[Openvpn-devel] [L] Change in openvpn[master]: PUSH_UPDATE: Added update_option() function.

[cron2 (C. Review) <ge...@op...>]

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

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

Change subject: PUSH_UPDATE: Added update_option() function.
......................................................................


Patch Set 21: Code-Review-1

(5 comments)

Patchset:

PS21: 
Nagging about code duplication...


File src/openvpn/options.c:

http://gerrit.openvpn.net/c/openvpn/+/810/comment/e80b547e_689dbd50 :
PS21, Line 5897:             }
I find this duplication of syntax checking, mmmh, sub-optimal.  We'll check the syntax anyway in `add_option()` later on...

I could see user complaints about "I sent a PUSH_UPDATE with one invalid `route` in it, and all my routes were lost", but honestly, do we really care that much?  (If yes, we could consider moving the syntax check into a small helper, if not, just drop the lines from `rol_check_alloc()` up to this line).


http://gerrit.openvpn.net/c/openvpn/+/810/comment/e1f5b5bd_f34b1d7f :
PS21, Line 5927:             }
same here


http://gerrit.openvpn.net/c/openvpn/+/810/comment/4054ed13_3dac7037 :
PS21, Line 6009: 
same here... lots of code that needs to be in sync with the add_option() checks...


http://gerrit.openvpn.net/c/openvpn/+/810/comment/abc555fa_e0e5856c :
PS21, Line 6045: #endif
I note a distinct lack of syntax checking for `dhcp-option` ;-)



-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/810?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: Ia45c99e6df7b3ad24020c10b8a9b3577984ecdc2
Gerrit-Change-Number: 810
Gerrit-PatchSet: 21
Gerrit-Owner: mrbff <ma...@ma...>
Gerrit-Reviewer: cron2 <ge...@gr...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-Reviewer: stipa <lst...@gm...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Attention: mrbff <ma...@ma...>
Gerrit-Comment-Date: Mon, 21 Jul 2025 19:21:15 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[Openvpn-devel] [L] Change in openvpn[master]: PUSH_UPDATE: Added remove_option() and do_update().

[cron2 (C. Review) <ge...@op...>]

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

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

Change subject: PUSH_UPDATE: Added remove_option() and do_update().
......................................................................


Patch Set 21: Code-Review-1

(4 comments)

Patchset:

PS21: 
As discussed on IRC, I do have some things I'd like to see changed before merging.

I am taking note of the +2's already given, it's just polishing, comments, readability...


File src/openvpn/init.c:

http://gerrit.openvpn.net/c/openvpn/+/809/comment/20d8d7ae_599aaf68 :
PS21, Line 2833:         if (!is_update && !check_pull_client_ncp(c, found))
does it make sense to suppress this check?  It should never fail, and never modify anything (as we passed on the first run).

As it is, this code is a bit confusing, so we'd need a comment

```
    /* on PUSH_UPDATE, do not run the NCP checks, because they take 5 minutes
     * to complete and we are in a hurry
     */
```

(put the real reasoning there, of course ;-) )

Mmmh, I think I see the reason, OPT_P_NCP is not set on PUSH_UPDATE, so the code assumes "we have no cipher" and does fallback things... so the comment would need to be something like

```
    /* on PUSH_UPDATE, NCP related flags are never updated, and so the code
     * would assume "no cipher pushed = NCP failed" - so, don't call it on
     * updates */
```


File src/openvpn/options.c:

http://gerrit.openvpn.net/c/openvpn/+/809/comment/758d81ff_703882a6 :
PS21, Line 5502: options_server_import(struct options *o,
please do not move them (as for 808).  thanks :-)


http://gerrit.openvpn.net/c/openvpn/+/809/comment/dbc0b2a7_f3d837dd :
PS21, Line 3134:     }
with the extra code, the comment before the function is no longer appropriate.

Also, it's unclear to me why this is needed?  I'm not guessing the reasoning but claiming "this does not look reasonable" - if `redirect-gateway def1` was pushed or updated, we should not remove it again just because it's not METHOD_SERVICE?

If we need it, we definitely need a comment to explain why.



-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/809?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: I507180d7397b6959844a30908010132bc3411067
Gerrit-Change-Number: 809
Gerrit-PatchSet: 21
Gerrit-Owner: mrbff <ma...@ma...>
Gerrit-Reviewer: cron2 <ge...@gr...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-Reviewer: stipa <lst...@gm...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Attention: mrbff <ma...@ma...>
Gerrit-Comment-Date: Mon, 21 Jul 2025 18:25:37 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[Openvpn-devel] [L] Change in openvpn[master]: PUSH_UPDATE: Allow OpenVPN in client mode to receive and handle PUSH ...

[cron2 (C. Review) <ge...@op...>]

Attention is currently required from: cron2, flichtenheld, mrbff, stipa.

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

Change subject: PUSH_UPDATE: Allow OpenVPN in client mode to receive and handle PUSH UPDATE control messages to allow options updating at runtime.
......................................................................


Patch Set 19: Code-Review-1

Copied votes on follow-up patch sets have been updated:
* Code-Review-1 has been copied to patch set 20 (copy condition: "changekind:NO_CHANGE OR changekind:TRIVIAL_REBASE OR is:MIN").

(10 comments)

Patchset:

PS19: 
So, I think this works and is sufficently save and efficient, but I'm and old man and like my lawn orderly... so as discussed on IRC, please adjust a few minor points.


File src/openvpn/options.c:

http://gerrit.openvpn.net/c/openvpn/+/808/comment/849fb9c1_8109d087 :
PS19, Line 5567: apply_push_options(struct options *options,
please do not move that one around in options.c, this makes comparing old/new harder without good benefit, also breaks git blame...


http://gerrit.openvpn.net/c/openvpn/+/808/comment/25512e6f_b2eb183f :
PS19, Line 5654: 
so I would call check_push_update_option_flags() from here, possibly moving the "while isspace(*line) { line++; }" part up here as well.  Then we can keep "apply_pull_filter()" focused on "pull filter", not dealing with a number of extra conditions...  (just "is_update", see there)


File src/openvpn/options_util.c:

http://gerrit.openvpn.net/c/openvpn/+/808/comment/5c5cd074_1394fe2c :
PS19, Line 198:     if (!line || !*line || !**line)
the use of a double pointer irritates me, because we never move the pointer, instead `memmove()` the buffer...

I think we should just do `(*line)++` instead of the `memmove()` below - possibly also getting rid of the `strlen(str)` thing, or `str` altogether :-) - so keep the pointer, call it from `apply_push_options()` and accept a moving `*line` pointer.

Also, I'm not sure if "empty string" should be treated as `false` or just ignored (return true, that is) - the existing environment where `apply_pull_filter()` is used does not care for "empty lines".

A NULL pointer is a programming error that must never happen, so an ASSERT(line) is valid.


http://gerrit.openvpn.net/c/openvpn/+/808/comment/6ffca9a9_893ed859 :
PS19, Line 231: 
If there is whitespace here (`?  route`) we'll fail with `pushed option is not updateable: '  route'`, if I'm not misreading it.

I'd document that "there must not be whitespace between the flag letters and the option" and then we could add an `if (isspace(**line)) { complain; return false; }` here... the documentation is important, the error handling not so much (as it will hit the `return false` anyway)


http://gerrit.openvpn.net/c/openvpn/+/808/comment/bc8b42ab_3cf0ba0c :
PS19, Line 252:             msg(D_PUSH, "Pushed option is not updatable: '%s'", *line);
Maybe add an " Ignoring." to the string, so it's clear "this is informational, but not an error, and this is how we handle it"?  Since the other one has "Restarting"...


http://gerrit.openvpn.net/c/openvpn/+/808/comment/ce24fecc_c0a39fcd :
PS19, Line 270:     if (!o->pull_filter_list && !(is_update))
So, if we call `check_push_update_option_flags()` from outside, we do not need this extra dance, checking o->pull_filter_list twice...


http://gerrit.openvpn.net/c/openvpn/+/808/comment/8974bb49_5a803112 :
PS19, Line 276:     while (isspace(*line))
It makes sense to move that upwards to `apply_push_options()`, before calling anything on the line.  So both all functions called do not need to bother about leading whitespace.


http://gerrit.openvpn.net/c/openvpn/+/808/comment/8689afde_5d9ee877 :
PS19, Line 310:                 return true;
This duplication of the messages and `return true` looks a bit silly :-) - I would suggest to do

```
        /* on PUSH_UPDATE, "reject" and "ignore" filters are treated the same */
        else if ( (f->type == PUF_TYPE_IGNORE || is_update)
                   && strncmp(line, f->pattern, f->size) == 0)
        {
            msg(D_PUSH, "Pushed option removed by filter: '%s'", line);
            return true;
        }
```


File src/openvpn/push_util.c:

http://gerrit.openvpn.net/c/openvpn/+/808/comment/b8dd5363_84e249de :
PS19, Line 23:                                true))
Just for the record - this is the really-old-style OpenVPN formatting, which we no longer use.  That said, I know it's the same as in `process_incoming_push_reply()`, so it makes sense to keep it the same, and then clang-format will change both at once.



-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/808?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: I6ecd4cb47571cc8c20e46de8595c742aeec6064a
Gerrit-Change-Number: 808
Gerrit-PatchSet: 19
Gerrit-Owner: mrbff <ma...@ma...>
Gerrit-Reviewer: cron2 <ge...@gr...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-Reviewer: stipa <lst...@gm...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: cron2 <ge...@gr...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Attention: mrbff <ma...@ma...>
Gerrit-Attention: stipa <lst...@gm...>
Gerrit-Comment-Date: Mon, 21 Jul 2025 18:08:07 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[Openvpn-devel] [S] Change in openvpn[master]: add flag to print addresses in a consistent format during float

[ralf_lici (C. Review) <ge...@op...>]

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

Hello flichtenheld, plaisthos, 

I'd like you to reexamine a change. Please visit

    http://gerrit.openvpn.net/c/openvpn/+/1092?usp=email

to look at the new patch set (#2).


Change subject: add flag to print addresses in a consistent format during float
......................................................................

add flag to print addresses in a consistent format during float

Introduce the MAPF_SHOW_FAMILY flag to prepend the address family to the
address when printing an mroute_addr object, similar to
print_sockaddr_ex(). This ensures that when logging a float operation,
both the old and new addresses are printed in the same format:
$proto:[$family]$address:$port.

Note: when using this flag with an IPv4-mapped IPv6 address, the output
will appear as: [AF_INET6]a.b.c.d

Change-Id: I43cd3d564d8c6ad4e41de5a38130d90cb6778395
Signed-off-by: Ralf Lici <ra...@ma...>
---
M src/openvpn/mroute.c
M src/openvpn/mroute.h
M src/openvpn/multi.c
3 files changed, 11 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/92/1092/2

diff --git a/src/openvpn/mroute.c b/src/openvpn/mroute.c
index a617b33..e3b1e9b 100644
--- a/src/openvpn/mroute.c
+++ b/src/openvpn/mroute.c
@@ -415,6 +415,10 @@
                 {
                     buf_printf(&out, "%s:", proto2ascii(maddr.proto, AF_INET, false));
                 }
+                if (flags & MAPF_SHOW_FAMILY)
+                {
+                    buf_printf(&out, "[AF_INET]");
+                }
                 buf_printf(&out, "%s", print_in_addr_t(ntohl(maddr.v4.addr),
                                                        (flags & MAPF_IA_EMPTY_IF_UNDEF) ? IA_EMPTY_IF_UNDEF : 0, gc));
                 if (maddr.type & MR_WITH_NETBITS)
@@ -442,6 +446,10 @@
                 {
                     buf_printf(&out, "%s:", proto2ascii(maddr.proto, AF_INET6, false));
                 }
+                if (flags & MAPF_SHOW_FAMILY)
+                {
+                    buf_printf(&out, "[AF_INET6]");
+                }
                 if (IN6_IS_ADDR_V4MAPPED( &maddr.v6.addr ) )
                 {
                     buf_printf(&out, "%s", print_in_addr_t(maddr.v4mappedv6.addr,
diff --git a/src/openvpn/mroute.h b/src/openvpn/mroute.h
index c359fd2..4f9fc03 100644
--- a/src/openvpn/mroute.h
+++ b/src/openvpn/mroute.h
@@ -150,6 +150,7 @@
 #define MAPF_SUBNET            (1<<0)
 #define MAPF_IA_EMPTY_IF_UNDEF (1<<1)
 #define MAPF_SHOW_ARP          (1<<2)
+#define MAPF_SHOW_FAMILY       (1<<3)
 const char *mroute_addr_print_ex(const struct mroute_addr *ma,
                                  const unsigned int flags,
                                  struct gc_arena *gc);
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 4696686..69ec2da 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -3274,8 +3274,8 @@
     msg(D_MULTI_MEDIUM, "peer %" PRIu32 " (%s) floated from %s to %s",
         mi->context.c2.tls_multi->peer_id,
         tls_common_name(mi->context.c2.tls_multi, false),
-        mroute_addr_print(&mi->real, &gc),
-        print_link_socket_actual(&m->top.c2.from, &gc));
+        mroute_addr_print_ex(&mi->real, MAPF_SHOW_FAMILY, &gc),
+        mroute_addr_print_ex(&real, MAPF_SHOW_FAMILY, &gc));
 
     /* remove old address from hash table before changing address */
     ASSERT(hash_remove(m->hash, &mi->real));

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

[Openvpn-devel] [XS] Change in openvpn[master]: add flag to print mroute_addr address family

[ralf_lici (C. Review) <ge...@op...>]

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

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

Change subject: add flag to print mroute_addr address family
......................................................................


Patch Set 1:

(1 comment)

Patchset:

PS1: 
> I'm not really sure I understand this patch - nobody is using that new flag yet, so it's a bit of a  […]
ACK



-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1092?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: I43cd3d564d8c6ad4e41de5a38130d90cb6778395
Gerrit-Change-Number: 1092
Gerrit-PatchSet: 1
Gerrit-Owner: ralf_lici <ra...@ma...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: cron2 <ge...@gr...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: cron2 <ge...@gr...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Comment-Date: Fri, 18 Jul 2025 20:55:39 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: cron2 <ge...@gr...>
Gerrit-MessageType: comment

[Openvpn-devel] [XS] Change in openvpn[master]: add flag to print mroute_addr address family

[cron2 (C. Review) <ge...@op...>]

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

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

Change subject: add flag to print mroute_addr address family
......................................................................


Patch Set 1:

(1 comment)

Patchset:

PS1: 
I'm not really sure I understand this patch - nobody is using that new flag yet, so it's a bit of a no-op...?



-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1092?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: I43cd3d564d8c6ad4e41de5a38130d90cb6778395
Gerrit-Change-Number: 1092
Gerrit-PatchSet: 1
Gerrit-Owner: ralf_lici <ra...@ma...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: cron2 <ge...@gr...>
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 20:45:39 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

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

[cron2 (C. Review) <ge...@op...>]

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

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

[cron2 (C. Review) <ge...@op...>]

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] [PATCH applied] Re: improve float collision logging

[Gert D. <ge...@gr...>]

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] [PATCH v1] improve float collision logging

[Gert D. <ge...@gr...>]

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] [XS] Change in openvpn[master]: improve float collision logging

[cron2 (C. Review) <ge...@op...>]

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] [S] Change in openvpn[master]: Multi-socket: Fix assert triggered by stale peer-id reuse

[cron2 (C. Review) <ge...@op...>]

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

Change subject: Multi-socket: Fix assert triggered by stale peer-id reuse
......................................................................

Multi-socket: Fix assert triggered by stale peer-id reuse

Fixed a bug where clients using different transport
protocols (UDP, TCP) could interfere with each other
after a server restart.
The issue occurred when a client reused a previously
assigned peer-id that was now associated with a
different client using a different transport protocol.

For example, a UDP client could send packets with a
peer-id now assigned to a TCP client, which lacks
a valid context->c2.from which is filled by the
recvfrom(), causing an assert to be triggered.

A protocol check has been added to prevent packets
from different protocols from hijacking active
connections.

Github: OpenVPN/openvpn#773

Change-Id: Iecbbcf32c0059f2b16a05333b3794599060d7d6a
Signed-off-by: Gianmarco De Gregori <gia...@ma...>
Acked-by: Gert Doering <ge...@gr...>
Message-Id: <202...@gr...>
URL: https://www.mail-archive.com/ope...@li.../msg32220.html
Signed-off-by: Gert Doering <ge...@gr...>
---
M src/openvpn/mudp.c
1 file changed, 13 insertions(+), 9 deletions(-)




diff --git a/src/openvpn/mudp.c b/src/openvpn/mudp.c
index 93e65e0..ee8446a 100644
--- a/src/openvpn/mudp.c
+++ b/src/openvpn/mudp.c
@@ -216,16 +216,20 @@
 
             if (!peer_id_disabled && (peer_id < m->max_clients) && (m->instances[peer_id]))
             {
-                mi = m->instances[peer_id];
-
-                *floated = !link_socket_actual_match(&mi->context.c2.from, &m->top.c2.from);
-
-                if (*floated)
+                /* Floating on TCP will never be possible, so ensure we only process
+                 * UDP clients */
+                if (m->instances[peer_id]->context.c2.link_sockets[0]->info.proto == sock->info.proto)
                 {
-                    /* reset prefix, since here we are not sure peer is the one it claims to be */
-                    ungenerate_prefix(mi);
-                    msg(D_MULTI_MEDIUM, "Float requested for peer %" PRIu32 " to %s", peer_id,
-                        mroute_addr_print(&real, &gc));
+                    mi = m->instances[peer_id];
+                    *floated = !link_socket_actual_match(&mi->context.c2.from, &m->top.c2.from);
+
+                    if (*floated)
+                    {
+                        /* reset prefix, since here we are not sure peer is the one it claims to be */
+                        ungenerate_prefix(mi);
+                        msg(D_MULTI_MEDIUM, "Float requested for peer %" PRIu32 " to %s", peer_id,
+                            mroute_addr_print(&real, &gc));
+                    }
                 }
             }
         }

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1078?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: Iecbbcf32c0059f2b16a05333b3794599060d7d6a
Gerrit-Change-Number: 1078
Gerrit-PatchSet: 4
Gerrit-Owner: its_Giaan <gia...@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] [S] Change in openvpn[master]: Multi-socket: Fix assert triggered by stale peer-id reuse

[cron2 (C. Review) <ge...@op...>]

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

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


Change subject: Multi-socket: Fix assert triggered by stale peer-id reuse
......................................................................

Multi-socket: Fix assert triggered by stale peer-id reuse

Fixed a bug where clients using different transport
protocols (UDP, TCP) could interfere with each other
after a server restart.
The issue occurred when a client reused a previously
assigned peer-id that was now associated with a
different client using a different transport protocol.

For example, a UDP client could send packets with a
peer-id now assigned to a TCP client, which lacks
a valid context->c2.from which is filled by the
recvfrom(), causing an assert to be triggered.

A protocol check has been added to prevent packets
from different protocols from hijacking active
connections.

Github: OpenVPN/openvpn#773

Change-Id: Iecbbcf32c0059f2b16a05333b3794599060d7d6a
Signed-off-by: Gianmarco De Gregori <gia...@ma...>
Acked-by: Gert Doering <ge...@gr...>
Message-Id: <202...@gr...>
URL: https://www.mail-archive.com/ope...@li.../msg32220.html
Signed-off-by: Gert Doering <ge...@gr...>
---
M src/openvpn/mudp.c
1 file changed, 13 insertions(+), 9 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/78/1078/4

diff --git a/src/openvpn/mudp.c b/src/openvpn/mudp.c
index 93e65e0..ee8446a 100644
--- a/src/openvpn/mudp.c
+++ b/src/openvpn/mudp.c
@@ -216,16 +216,20 @@
 
             if (!peer_id_disabled && (peer_id < m->max_clients) && (m->instances[peer_id]))
             {
-                mi = m->instances[peer_id];
-
-                *floated = !link_socket_actual_match(&mi->context.c2.from, &m->top.c2.from);
-
-                if (*floated)
+                /* Floating on TCP will never be possible, so ensure we only process
+                 * UDP clients */
+                if (m->instances[peer_id]->context.c2.link_sockets[0]->info.proto == sock->info.proto)
                 {
-                    /* reset prefix, since here we are not sure peer is the one it claims to be */
-                    ungenerate_prefix(mi);
-                    msg(D_MULTI_MEDIUM, "Float requested for peer %" PRIu32 " to %s", peer_id,
-                        mroute_addr_print(&real, &gc));
+                    mi = m->instances[peer_id];
+                    *floated = !link_socket_actual_match(&mi->context.c2.from, &m->top.c2.from);
+
+                    if (*floated)
+                    {
+                        /* reset prefix, since here we are not sure peer is the one it claims to be */
+                        ungenerate_prefix(mi);
+                        msg(D_MULTI_MEDIUM, "Float requested for peer %" PRIu32 " to %s", peer_id,
+                            mroute_addr_print(&real, &gc));
+                    }
                 }
             }
         }

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1078?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: Iecbbcf32c0059f2b16a05333b3794599060d7d6a
Gerrit-Change-Number: 1078
Gerrit-PatchSet: 4
Gerrit-Owner: its_Giaan <gia...@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

[Openvpn-devel] [PATCH applied] Re: Multi-socket: Fix assert triggered by stale peer-id reuse

[Gert D. <ge...@gr...>]

So, while I could not trigger the original ASSERT() (GH issue #773) I
was able to trigger "server misbehaviour" (TCP client with peer-id #0
being kicked out when a leftover UDP client with (old) peer-id #0 sent
data packets).  With the patch, these are gone.

The explanation makes sense - when checking for float, just do not look
at TCP instances at all.  Those can not float, might not have all
data fields filled in, and bring no relevant info for a floating UDP 
client.  So when seen with "git show -w", it's just one extra if()
to verify "only compare with UDP instances" (= 'same proto', as this
function is only called for UDP).

Your patch has been applied to the master branch.

commit fd93e4ad8245e1fd9530a6c1f89cb66c047f3abe
Author: Gianmarco De Gregori
Date:   Fri Jul 18 20:55:53 2025 +0200

     Multi-socket: Fix assert triggered by stale peer-id reuse

     Signed-off-by: Gianmarco De Gregori <gia...@ma...>
     Acked-by: Gert Doering <ge...@gr...>
     Message-Id: <202...@gr...>
     URL: https://www.mail-archive.com/ope...@li.../msg32220.html
     Signed-off-by: Gert Doering <ge...@gr...>


--
kind regards,

Gert Doering

[Openvpn-devel] [PATCH v3] Multi-socket: Fix assert triggered by stale peer-id reuse

[Gert D. <ge...@gr...>]

From: Gianmarco De Gregori <gia...@ma...>

Fixed a bug where clients using different transport
protocols (UDP, TCP) could interfere with each other
after a server restart.
The issue occurred when a client reused a previously
assigned peer-id that was now associated with a
different client using a different transport protocol.

For example, a UDP client could send packets with a
peer-id now assigned to a TCP client, which lacks
a valid context->c2.from which is filled by the
recvfrom(), causing an assert to be triggered.

A protocol check has been added to prevent packets
from different protocols from hijacking active
connections.

Github: #773

Change-Id: Iecbbcf32c0059f2b16a05333b3794599060d7d6a
Signed-off-by: Gianmarco De Gregori <gia...@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/+/1078
This mail reflects revision 3 of this Change.

Acked-by according to Gerrit (reflected above):
Gert Doering <ge...@gr...>

        
diff --git a/src/openvpn/mudp.c b/src/openvpn/mudp.c
index 93e65e0..ee8446a 100644
--- a/src/openvpn/mudp.c
+++ b/src/openvpn/mudp.c
@@ -216,16 +216,20 @@
 
             if (!peer_id_disabled && (peer_id < m->max_clients) && (m->instances[peer_id]))
             {
-                mi = m->instances[peer_id];
-
-                *floated = !link_socket_actual_match(&mi->context.c2.from, &m->top.c2.from);
-
-                if (*floated)
+                /* Floating on TCP will never be possible, so ensure we only process
+                 * UDP clients */
+                if (m->instances[peer_id]->context.c2.link_sockets[0]->info.proto == sock->info.proto)
                 {
-                    /* reset prefix, since here we are not sure peer is the one it claims to be */
-                    ungenerate_prefix(mi);
-                    msg(D_MULTI_MEDIUM, "Float requested for peer %" PRIu32 " to %s", peer_id,
-                        mroute_addr_print(&real, &gc));
+                    mi = m->instances[peer_id];
+                    *floated = !link_socket_actual_match(&mi->context.c2.from, &m->top.c2.from);
+
+                    if (*floated)
+                    {
+                        /* reset prefix, since here we are not sure peer is the one it claims to be */
+                        ungenerate_prefix(mi);
+                        msg(D_MULTI_MEDIUM, "Float requested for peer %" PRIu32 " to %s", peer_id,
+                            mroute_addr_print(&real, &gc));
+                    }
                 }
             }
         }

[Openvpn-devel] [S] Change in openvpn[master]: Multi-socket: Fix assert triggered by stale peer-id reuse

[cron2 (C. Review) <ge...@op...>]

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

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

Change subject: Multi-socket: Fix assert triggered by stale peer-id reuse
......................................................................


Patch Set 3: Code-Review+2

(1 comment)

Patchset:

PS3: 
Interesting.  I can not trigger the ASSERT() with "master" with the sequence described

- start multisocket master
- connect UDP client on peer id 0
- connect TCP client on peer id 1
- restart the server
- TCP client reconnects, now peer id 0
- UDP client "sends packets with peer id 0"

it does some other weird things here, namely killing the TCP #0 instance on each UDP packet from #0 - but this is also fixed with this patch (it will just ignore udp #0 packets).



-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1078?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: Iecbbcf32c0059f2b16a05333b3794599060d7d6a
Gerrit-Change-Number: 1078
Gerrit-PatchSet: 3
Gerrit-Owner: its_Giaan <gia...@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: its_Giaan <gia...@ma...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Comment-Date: Fri, 18 Jul 2025 18:55:36 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[Openvpn-devel] [S] Change in openvpn[master]: Fix broken DHCP options

[stipa (C. Review) <ge...@op...>]

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

to review the following change.


Change subject: Fix broken DHCP options
......................................................................

Fix broken DHCP options

Commit

  2dfc4f ("dns: deal with --dhcp-options when --dns is active")

broke support for --dhcp-options. It removed the setting of the
DHCP_OPTIONS_DHCP_OPTIONAL flag for some DHCP options. This flag is
required for those options to be applied correctly, as it is used
when building the DHCP options string that is passed to the TAP driver.

This commit fixes the issue by restoring the setting of this flag.

GitHub: #791

Change-Id: I0d75efcceb826d06e74abd003d5377468ff9fe3b
Signed-off-by: Lev Stipakov <le...@op...>
---
M src/openvpn/options.c
1 file changed, 13 insertions(+), 0 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/90/1090/1

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 0662b49..4ba9243 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -8310,9 +8310,12 @@
 #endif
         VERIFY_PERMISSION(OPT_P_DHCPDNS);
 
+        bool dhcp_optional = false;
+
         if ((streq(p[1], "DOMAIN") || streq(p[1], "ADAPTER_DOMAIN_SUFFIX")) && p[2] && !p[3])
         {
             dhcp->domain = p[2];
+            dhcp_optional = true;
         }
         else if (streq(p[1], "DOMAIN-SEARCH") && p[2] && !p[3])
         {
@@ -8325,6 +8328,7 @@
                 msg(msglevel, "--dhcp-option %s: maximum of %d search entries can be specified",
                     p[1], N_SEARCH_LIST_LEN);
             }
+            dhcp_optional = true;
         }
         else if ((streq(p[1], "DNS") || streq(p[1], "DNS6")) && p[2] && !p[3]
                  && (!strstr(p[2], ":") || ipv6_addr_safe(p[2])))
@@ -8336,6 +8340,7 @@
             else
             {
                 dhcp_option_address_parse("DNS", p[2], dhcp->dns, &dhcp->dns_len, msglevel);
+                dhcp_optional = true;
             }
         }
 #if defined(_WIN32) || defined(TARGET_ANDROID)
@@ -8359,6 +8364,7 @@
         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])
         {
@@ -8390,6 +8396,13 @@
 #else /* if defined(_WIN32) || defined(TARGET_ANDROID) */
         setenv_foreign_option(options, (const char **)p, 3, es);
 #endif /* if defined(_WIN32) || defined(TARGET_ANDROID) */
+
+        if (dhcp_optional)
+        {
+#if defined(_WIN32) || defined(TARGET_ANDROID)
+            o->dhcp_options |= DHCP_OPTIONS_DHCP_OPTIONAL;
+#endif
+        }
     }
 #ifdef _WIN32
     else if (streq(p[0], "show-adapters") && !p[1])

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1090?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: I0d75efcceb826d06e74abd003d5377468ff9fe3b
Gerrit-Change-Number: 1090
Gerrit-PatchSet: 1
Gerrit-Owner: stipa <lst...@gm...>
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] [M] Change in openvpn[master]: dco: Add support for float notifications

[cron2 (C. Review) <ge...@op...>]

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

Change subject: dco: Add support for float notifications
......................................................................

dco: Add support for float notifications

When a peer changes its UDP endpoint, the DCO module emits a
notification to userpace. The message is parsed and the relevant
information are extracted in order to process the floating operation.

Note that we preserve IPv4-mapped IPv6 addresses in userspace when
receiving a pure IPv4 address from the module, otherwise openvpn
wouldn't be able to retrieve the multi_instance using the transport
address hash table lookup.

It may happen that a netlink notification gets lost, causing us to skip
a float step. If the peer then floats back to its previous address,
userspace closes the only valid instance while trying to process the
float, leading to a segfault. To prevent this, we ignore float attempts
to an address already taken by a peer with the same peer ID.

Change-Id: I33e9272b4196c7634db2fb33a75ae4261660867f
Signed-off-by: Ralf Lici <ra...@ma...>
Acked-by: Gert Doering <ge...@gr...>
Acked-by: Antonio Quartulli <an...@ma...>
Acked-by: Lev Stipakov <lst...@gm...>
Message-Id: <202...@gr...>
URL: https://www.mail-archive.com/ope...@li.../msg32210.html
Signed-off-by: Gert Doering <ge...@gr...>
---
M src/openvpn/dco_linux.c
M src/openvpn/dco_linux.h
M src/openvpn/dco_win.c
M src/openvpn/dco_win.h
M src/openvpn/forward.c
M src/openvpn/forward.h
M src/openvpn/multi.c
M src/openvpn/multi.h
M src/openvpn/ovpn_dco_linux.h
M src/openvpn/ovpn_dco_win.h
10 files changed, 159 insertions(+), 2 deletions(-)




diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c
index 22a445a..f04ebfe 100644
--- a/src/openvpn/dco_linux.c
+++ b/src/openvpn/dco_linux.c
@@ -768,6 +768,44 @@
     return ret;
 }
 
+static bool
+ovpn_parse_float_addr(struct nlattr **attrs, struct sockaddr *out)
+{
+    if (!attrs[OVPN_A_PEER_REMOTE_PORT])
+    {
+        msg(D_DCO, "ovpn-dco: no remote port in PEER_FLOAT_NTF message");
+        return false;
+    }
+
+    if (attrs[OVPN_A_PEER_REMOTE_IPV4])
+    {
+        struct sockaddr_in *addr4 = (struct sockaddr_in *)out;
+        CLEAR(*addr4);
+        addr4->sin_family = AF_INET;
+        addr4->sin_port = nla_get_u16(attrs[OVPN_A_PEER_REMOTE_PORT]);
+        addr4->sin_addr.s_addr = nla_get_u32(attrs[OVPN_A_PEER_REMOTE_IPV4]);
+        return true;
+    }
+    else if (attrs[OVPN_A_PEER_REMOTE_IPV6]
+             && nla_len(attrs[OVPN_A_PEER_REMOTE_IPV6]) == sizeof(struct in6_addr))
+    {
+        struct sockaddr_in6 *addr6 = (struct sockaddr_in6 *)out;
+        CLEAR(*addr6);
+        addr6->sin6_family = AF_INET6;
+        addr6->sin6_port = nla_get_u16(attrs[OVPN_A_PEER_REMOTE_PORT]);
+        memcpy(&addr6->sin6_addr, nla_data(attrs[OVPN_A_PEER_REMOTE_IPV6]),
+               sizeof(addr6->sin6_addr));
+        if (attrs[OVPN_A_PEER_REMOTE_IPV6_SCOPE_ID])
+        {
+            addr6->sin6_scope_id = nla_get_u32(attrs[OVPN_A_PEER_REMOTE_IPV6_SCOPE_ID]);
+        }
+        return true;
+    }
+
+    msg(D_DCO, "ovpn-dco: no valid remote IP address in PEER_FLOAT_NTF message");
+    return false;
+}
+
 /* This function parses any netlink message sent by ovpn-dco to userspace */
 static int
 ovpn_handle_msg(struct nl_msg *msg, void *arg)
@@ -856,6 +894,45 @@
             break;
         }
 
+        case OVPN_CMD_PEER_FLOAT_NTF:
+        {
+            if (!attrs[OVPN_A_PEER])
+            {
+                msg(D_DCO, "ovpn-dco: no peer in PEER_FLOAT_NTF message");
+                return NL_STOP;
+            }
+
+            struct nlattr *fp_attrs[OVPN_A_PEER_MAX + 1];
+            if (nla_parse_nested(fp_attrs, OVPN_A_PEER_MAX, attrs[OVPN_A_PEER],
+                                 NULL))
+            {
+                msg(D_DCO, "ovpn-dco: can't parse peer in PEER_FLOAT_NTF messsage");
+                return NL_STOP;
+            }
+
+            if (!fp_attrs[OVPN_A_PEER_ID])
+            {
+                msg(D_DCO, "ovpn-dco: no peer-id in PEER_FLOAT_NTF message");
+                return NL_STOP;
+            }
+            uint32_t peerid = nla_get_u32(fp_attrs[OVPN_A_PEER_ID]);
+
+            if (!ovpn_parse_float_addr(fp_attrs, (struct sockaddr *)&dco->dco_float_peer_ss))
+            {
+                return NL_STOP;
+            }
+
+            struct gc_arena gc = gc_new();
+            msg(D_DCO_DEBUG,
+                "ovpn-dco: received CMD_PEER_FLOAT_NTF, ifindex: %u, peer-id %u, address: %s",
+                ifindex, peerid, print_sockaddr((struct sockaddr *)&dco->dco_float_peer_ss, &gc));
+            dco->dco_message_peer_id = (int)peerid;
+            dco->dco_message_type = OVPN_CMD_PEER_FLOAT_NTF;
+
+            gc_free(&gc);
+            break;
+        }
+
         case OVPN_CMD_KEY_SWAP_NTF:
         {
             if (!attrs[OVPN_A_KEYCONF])
diff --git a/src/openvpn/dco_linux.h b/src/openvpn/dco_linux.h
index 4e441ec..676b8cd 100644
--- a/src/openvpn/dco_linux.h
+++ b/src/openvpn/dco_linux.h
@@ -34,6 +34,7 @@
 /* Defines to avoid mismatching with other platforms */
 #define OVPN_CMD_DEL_PEER OVPN_CMD_PEER_DEL_NTF
 #define OVPN_CMD_SWAP_KEYS OVPN_CMD_KEY_SWAP_NTF
+#define OVPN_CMD_FLOAT_PEER OVPN_CMD_PEER_FLOAT_NTF
 
 typedef enum ovpn_key_slot dco_key_slot_t;
 typedef enum ovpn_cipher_alg dco_cipher_t;
@@ -75,6 +76,7 @@
     int dco_message_peer_id;
     int dco_message_key_id;
     int dco_del_peer_reason;
+    struct sockaddr_storage dco_float_peer_ss;
     uint64_t dco_read_bytes;
     uint64_t dco_write_bytes;
 } dco_context_t;
diff --git a/src/openvpn/dco_win.c b/src/openvpn/dco_win.c
index 2a13658..83db739 100644
--- a/src/openvpn/dco_win.c
+++ b/src/openvpn/dco_win.c
@@ -663,6 +663,7 @@
         dco->dco_message_peer_id = dco->notif_buf.PeerId;
         dco->dco_message_type = dco->notif_buf.Cmd;
         dco->dco_del_peer_reason = dco->notif_buf.DelPeerReason;
+        dco->dco_float_peer_ss = dco->notif_buf.FloatAddress;
     }
     else
     {
diff --git a/src/openvpn/dco_win.h b/src/openvpn/dco_win.h
index 4513f3f..b9d93fa 100644
--- a/src/openvpn/dco_win.h
+++ b/src/openvpn/dco_win.h
@@ -52,6 +52,7 @@
     int dco_message_peer_id;
     int dco_message_type;
     int dco_del_peer_reason;
+    struct sockaddr_storage dco_float_peer_ss;
 
     uint64_t dco_read_bytes;
     uint64_t dco_write_bytes;
diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index a4f260a..dfc6708 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -1243,6 +1243,41 @@
     perf_pop();
 }
 
+void
+extract_dco_float_peer_addr(const sa_family_t socket_family,
+                            struct openvpn_sockaddr *out_osaddr,
+                            const struct sockaddr *float_sa)
+{
+    if (float_sa->sa_family == AF_INET)
+    {
+        struct sockaddr_in *float4 = (struct sockaddr_in *)float_sa;
+        /* DCO treats IPv4-mapped IPv6 addresses as pure IPv4. However, on a
+         * dual-stack socket, we need to preserve the mapping otherwise openvpn
+         * will not be able to find the peer by its transport address.
+         */
+        if (socket_family == AF_INET6)
+        {
+            out_osaddr->addr.in6.sin6_family = AF_INET6;
+            out_osaddr->addr.in6.sin6_port = float4->sin_port;
+
+            memset(&out_osaddr->addr.in6.sin6_addr.s6_addr, 0, 10);
+            out_osaddr->addr.in6.sin6_addr.s6_addr[10] = 0xff;
+            out_osaddr->addr.in6.sin6_addr.s6_addr[11] = 0xff;
+            memcpy(&out_osaddr->addr.in6.sin6_addr.s6_addr[12],
+                   &float4->sin_addr.s_addr, sizeof(in_addr_t));
+        }
+        else
+        {
+            memcpy(&out_osaddr->addr.in4, float4, sizeof(struct sockaddr_in));
+        }
+    }
+    else
+    {
+        struct sockaddr_in6 *float6 = (struct sockaddr_in6 *)float_sa;
+        memcpy(&out_osaddr->addr.in6, float6, sizeof(struct sockaddr_in6));
+    }
+}
+
 static void
 process_incoming_dco(struct context *c)
 {
diff --git a/src/openvpn/forward.h b/src/openvpn/forward.h
index 318691f..2818fd1 100644
--- a/src/openvpn/forward.h
+++ b/src/openvpn/forward.h
@@ -196,6 +196,21 @@
 void process_incoming_link_part2(struct context *c, struct link_socket_info *lsi, const uint8_t *orig_buf);
 
 /**
+ * Transfers \c float_sa data extracted from an incoming DCO
+ * PEER_FLOAT_NTF to \c out_osaddr for later processing.
+ *
+ * @param socket_family - The address family of the socket
+ * @param out_osaddr - openvpn_sockaddr struct that will be filled the new
+ *      address data
+ * @param float_sa - The sockaddr struct containing the data received from the
+ *      DCO notification
+ */
+void
+extract_dco_float_peer_addr(sa_family_t socket_family,
+                            struct openvpn_sockaddr *out_osaddr,
+                            const struct sockaddr *float_sa);
+
+/**
  * Write a packet to the external network interface.
  * @ingroup external_multiplexer
  *
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index a760e07..ead3dd0 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -3251,6 +3251,18 @@
             goto done;
         }
 
+        /* It doesn't make sense to let a peer float to the address it already
+         * has, so we disallow it. This can happen if a DCO netlink notification
+         * gets lost and we miss a floating step.
+         */
+        if (m1->peer_id == m2->peer_id)
+        {
+            msg(M_WARN, "disallowing peer %" PRIu32 " (%s) from floating to "
+                "its own address (%s)",
+                m1->peer_id, tls_common_name(mi->context.c2.tls_multi, false),
+                mroute_addr_print(&mi->real, &gc));
+            goto done;
+        }
         msg(D_MULTI_MEDIUM, "closing instance %s", multi_instance_string(ex_mi, false, &gc));
         multi_close_instance(m, ex_mi, false);
     }
@@ -3384,6 +3396,17 @@
         {
             process_incoming_del_peer(m, mi, dco);
         }
+#if defined(TARGET_LINUX) || defined(TARGET_WIN32)
+        else if (dco->dco_message_type == OVPN_CMD_FLOAT_PEER)
+        {
+            ASSERT(mi->context.c2.link_sockets[0]);
+            extract_dco_float_peer_addr(mi->context.c2.link_sockets[0]->info.af,
+                                        &m->top.c2.from.dest,
+                                        (struct sockaddr *)&dco->dco_float_peer_ss);
+            multi_process_float(m, mi, mi->context.c2.link_sockets[0]);
+            CLEAR(dco->dco_float_peer_ss);
+        }
+#endif /* if defined(TARGET_LINUX) || defined(TARGET_WIN32) */
         else if (dco->dco_message_type == OVPN_CMD_SWAP_KEYS)
         {
             tls_session_soft_reset(mi->context.c2.tls_multi);
diff --git a/src/openvpn/multi.h b/src/openvpn/multi.h
index 40f7519..fe9e847 100644
--- a/src/openvpn/multi.h
+++ b/src/openvpn/multi.h
@@ -322,7 +322,7 @@
 /**
  * Process an incoming DCO message (from kernel space).
  *
- * @param m            - The single \c multi_context structur.e
+ * @param m            - The single \c multi_context structure.
  *
  * @return
  *  - True, if the message was received correctly.
diff --git a/src/openvpn/ovpn_dco_linux.h b/src/openvpn/ovpn_dco_linux.h
index 680d152..b3c9ff0 100644
--- a/src/openvpn/ovpn_dco_linux.h
+++ b/src/openvpn/ovpn_dco_linux.h
@@ -99,6 +99,7 @@
 	OVPN_CMD_KEY_SWAP,
 	OVPN_CMD_KEY_SWAP_NTF,
 	OVPN_CMD_KEY_DEL,
+	OVPN_CMD_PEER_FLOAT_NTF,
 
 	__OVPN_CMD_MAX,
 	OVPN_CMD_MAX = (__OVPN_CMD_MAX - 1)
diff --git a/src/openvpn/ovpn_dco_win.h b/src/openvpn/ovpn_dco_win.h
index 865bb38..dd6b7ce 100644
--- a/src/openvpn/ovpn_dco_win.h
+++ b/src/openvpn/ovpn_dco_win.h
@@ -149,7 +149,8 @@
 
 typedef enum {
     OVPN_CMD_DEL_PEER,
-    OVPN_CMD_SWAP_KEYS
+    OVPN_CMD_SWAP_KEYS,
+    OVPN_CMD_FLOAT_PEER
 } OVPN_NOTIFY_CMD;
 
 typedef enum {
@@ -164,6 +165,7 @@
     OVPN_NOTIFY_CMD Cmd;
     int PeerId;
     OVPN_DEL_PEER_REASON DelPeerReason;
+    struct sockaddr_storage FloatAddress;
 } OVPN_NOTIFY_EVENT, * POVPN_NOTIFY_EVENT;
 
 typedef struct _OVPN_MP_DEL_PEER {

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

[Openvpn-devel] [M] Change in openvpn[master]: dco: Add support for float notifications

[cron2 (C. Review) <ge...@op...>]

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

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


Change subject: dco: Add support for float notifications
......................................................................

dco: Add support for float notifications

When a peer changes its UDP endpoint, the DCO module emits a
notification to userpace. The message is parsed and the relevant
information are extracted in order to process the floating operation.

Note that we preserve IPv4-mapped IPv6 addresses in userspace when
receiving a pure IPv4 address from the module, otherwise openvpn
wouldn't be able to retrieve the multi_instance using the transport
address hash table lookup.

It may happen that a netlink notification gets lost, causing us to skip
a float step. If the peer then floats back to its previous address,
userspace closes the only valid instance while trying to process the
float, leading to a segfault. To prevent this, we ignore float attempts
to an address already taken by a peer with the same peer ID.

Change-Id: I33e9272b4196c7634db2fb33a75ae4261660867f
Signed-off-by: Ralf Lici <ra...@ma...>
Acked-by: Gert Doering <ge...@gr...>
Acked-by: Antonio Quartulli <an...@ma...>
Acked-by: Lev Stipakov <lst...@gm...>
Message-Id: <202...@gr...>
URL: https://www.mail-archive.com/ope...@li.../msg32210.html
Signed-off-by: Gert Doering <ge...@gr...>
---
M src/openvpn/dco_linux.c
M src/openvpn/dco_linux.h
M src/openvpn/dco_win.c
M src/openvpn/dco_win.h
M src/openvpn/forward.c
M src/openvpn/forward.h
M src/openvpn/multi.c
M src/openvpn/multi.h
M src/openvpn/ovpn_dco_linux.h
M src/openvpn/ovpn_dco_win.h
10 files changed, 159 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/84/1084/7

diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c
index 22a445a..f04ebfe 100644
--- a/src/openvpn/dco_linux.c
+++ b/src/openvpn/dco_linux.c
@@ -768,6 +768,44 @@
     return ret;
 }
 
+static bool
+ovpn_parse_float_addr(struct nlattr **attrs, struct sockaddr *out)
+{
+    if (!attrs[OVPN_A_PEER_REMOTE_PORT])
+    {
+        msg(D_DCO, "ovpn-dco: no remote port in PEER_FLOAT_NTF message");
+        return false;
+    }
+
+    if (attrs[OVPN_A_PEER_REMOTE_IPV4])
+    {
+        struct sockaddr_in *addr4 = (struct sockaddr_in *)out;
+        CLEAR(*addr4);
+        addr4->sin_family = AF_INET;
+        addr4->sin_port = nla_get_u16(attrs[OVPN_A_PEER_REMOTE_PORT]);
+        addr4->sin_addr.s_addr = nla_get_u32(attrs[OVPN_A_PEER_REMOTE_IPV4]);
+        return true;
+    }
+    else if (attrs[OVPN_A_PEER_REMOTE_IPV6]
+             && nla_len(attrs[OVPN_A_PEER_REMOTE_IPV6]) == sizeof(struct in6_addr))
+    {
+        struct sockaddr_in6 *addr6 = (struct sockaddr_in6 *)out;
+        CLEAR(*addr6);
+        addr6->sin6_family = AF_INET6;
+        addr6->sin6_port = nla_get_u16(attrs[OVPN_A_PEER_REMOTE_PORT]);
+        memcpy(&addr6->sin6_addr, nla_data(attrs[OVPN_A_PEER_REMOTE_IPV6]),
+               sizeof(addr6->sin6_addr));
+        if (attrs[OVPN_A_PEER_REMOTE_IPV6_SCOPE_ID])
+        {
+            addr6->sin6_scope_id = nla_get_u32(attrs[OVPN_A_PEER_REMOTE_IPV6_SCOPE_ID]);
+        }
+        return true;
+    }
+
+    msg(D_DCO, "ovpn-dco: no valid remote IP address in PEER_FLOAT_NTF message");
+    return false;
+}
+
 /* This function parses any netlink message sent by ovpn-dco to userspace */
 static int
 ovpn_handle_msg(struct nl_msg *msg, void *arg)
@@ -856,6 +894,45 @@
             break;
         }
 
+        case OVPN_CMD_PEER_FLOAT_NTF:
+        {
+            if (!attrs[OVPN_A_PEER])
+            {
+                msg(D_DCO, "ovpn-dco: no peer in PEER_FLOAT_NTF message");
+                return NL_STOP;
+            }
+
+            struct nlattr *fp_attrs[OVPN_A_PEER_MAX + 1];
+            if (nla_parse_nested(fp_attrs, OVPN_A_PEER_MAX, attrs[OVPN_A_PEER],
+                                 NULL))
+            {
+                msg(D_DCO, "ovpn-dco: can't parse peer in PEER_FLOAT_NTF messsage");
+                return NL_STOP;
+            }
+
+            if (!fp_attrs[OVPN_A_PEER_ID])
+            {
+                msg(D_DCO, "ovpn-dco: no peer-id in PEER_FLOAT_NTF message");
+                return NL_STOP;
+            }
+            uint32_t peerid = nla_get_u32(fp_attrs[OVPN_A_PEER_ID]);
+
+            if (!ovpn_parse_float_addr(fp_attrs, (struct sockaddr *)&dco->dco_float_peer_ss))
+            {
+                return NL_STOP;
+            }
+
+            struct gc_arena gc = gc_new();
+            msg(D_DCO_DEBUG,
+                "ovpn-dco: received CMD_PEER_FLOAT_NTF, ifindex: %u, peer-id %u, address: %s",
+                ifindex, peerid, print_sockaddr((struct sockaddr *)&dco->dco_float_peer_ss, &gc));
+            dco->dco_message_peer_id = (int)peerid;
+            dco->dco_message_type = OVPN_CMD_PEER_FLOAT_NTF;
+
+            gc_free(&gc);
+            break;
+        }
+
         case OVPN_CMD_KEY_SWAP_NTF:
         {
             if (!attrs[OVPN_A_KEYCONF])
diff --git a/src/openvpn/dco_linux.h b/src/openvpn/dco_linux.h
index 4e441ec..676b8cd 100644
--- a/src/openvpn/dco_linux.h
+++ b/src/openvpn/dco_linux.h
@@ -34,6 +34,7 @@
 /* Defines to avoid mismatching with other platforms */
 #define OVPN_CMD_DEL_PEER OVPN_CMD_PEER_DEL_NTF
 #define OVPN_CMD_SWAP_KEYS OVPN_CMD_KEY_SWAP_NTF
+#define OVPN_CMD_FLOAT_PEER OVPN_CMD_PEER_FLOAT_NTF
 
 typedef enum ovpn_key_slot dco_key_slot_t;
 typedef enum ovpn_cipher_alg dco_cipher_t;
@@ -75,6 +76,7 @@
     int dco_message_peer_id;
     int dco_message_key_id;
     int dco_del_peer_reason;
+    struct sockaddr_storage dco_float_peer_ss;
     uint64_t dco_read_bytes;
     uint64_t dco_write_bytes;
 } dco_context_t;
diff --git a/src/openvpn/dco_win.c b/src/openvpn/dco_win.c
index 2a13658..83db739 100644
--- a/src/openvpn/dco_win.c
+++ b/src/openvpn/dco_win.c
@@ -663,6 +663,7 @@
         dco->dco_message_peer_id = dco->notif_buf.PeerId;
         dco->dco_message_type = dco->notif_buf.Cmd;
         dco->dco_del_peer_reason = dco->notif_buf.DelPeerReason;
+        dco->dco_float_peer_ss = dco->notif_buf.FloatAddress;
     }
     else
     {
diff --git a/src/openvpn/dco_win.h b/src/openvpn/dco_win.h
index 4513f3f..b9d93fa 100644
--- a/src/openvpn/dco_win.h
+++ b/src/openvpn/dco_win.h
@@ -52,6 +52,7 @@
     int dco_message_peer_id;
     int dco_message_type;
     int dco_del_peer_reason;
+    struct sockaddr_storage dco_float_peer_ss;
 
     uint64_t dco_read_bytes;
     uint64_t dco_write_bytes;
diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index a4f260a..dfc6708 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -1243,6 +1243,41 @@
     perf_pop();
 }
 
+void
+extract_dco_float_peer_addr(const sa_family_t socket_family,
+                            struct openvpn_sockaddr *out_osaddr,
+                            const struct sockaddr *float_sa)
+{
+    if (float_sa->sa_family == AF_INET)
+    {
+        struct sockaddr_in *float4 = (struct sockaddr_in *)float_sa;
+        /* DCO treats IPv4-mapped IPv6 addresses as pure IPv4. However, on a
+         * dual-stack socket, we need to preserve the mapping otherwise openvpn
+         * will not be able to find the peer by its transport address.
+         */
+        if (socket_family == AF_INET6)
+        {
+            out_osaddr->addr.in6.sin6_family = AF_INET6;
+            out_osaddr->addr.in6.sin6_port = float4->sin_port;
+
+            memset(&out_osaddr->addr.in6.sin6_addr.s6_addr, 0, 10);
+            out_osaddr->addr.in6.sin6_addr.s6_addr[10] = 0xff;
+            out_osaddr->addr.in6.sin6_addr.s6_addr[11] = 0xff;
+            memcpy(&out_osaddr->addr.in6.sin6_addr.s6_addr[12],
+                   &float4->sin_addr.s_addr, sizeof(in_addr_t));
+        }
+        else
+        {
+            memcpy(&out_osaddr->addr.in4, float4, sizeof(struct sockaddr_in));
+        }
+    }
+    else
+    {
+        struct sockaddr_in6 *float6 = (struct sockaddr_in6 *)float_sa;
+        memcpy(&out_osaddr->addr.in6, float6, sizeof(struct sockaddr_in6));
+    }
+}
+
 static void
 process_incoming_dco(struct context *c)
 {
diff --git a/src/openvpn/forward.h b/src/openvpn/forward.h
index 318691f..2818fd1 100644
--- a/src/openvpn/forward.h
+++ b/src/openvpn/forward.h
@@ -196,6 +196,21 @@
 void process_incoming_link_part2(struct context *c, struct link_socket_info *lsi, const uint8_t *orig_buf);
 
 /**
+ * Transfers \c float_sa data extracted from an incoming DCO
+ * PEER_FLOAT_NTF to \c out_osaddr for later processing.
+ *
+ * @param socket_family - The address family of the socket
+ * @param out_osaddr - openvpn_sockaddr struct that will be filled the new
+ *      address data
+ * @param float_sa - The sockaddr struct containing the data received from the
+ *      DCO notification
+ */
+void
+extract_dco_float_peer_addr(sa_family_t socket_family,
+                            struct openvpn_sockaddr *out_osaddr,
+                            const struct sockaddr *float_sa);
+
+/**
  * Write a packet to the external network interface.
  * @ingroup external_multiplexer
  *
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index a760e07..ead3dd0 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -3251,6 +3251,18 @@
             goto done;
         }
 
+        /* It doesn't make sense to let a peer float to the address it already
+         * has, so we disallow it. This can happen if a DCO netlink notification
+         * gets lost and we miss a floating step.
+         */
+        if (m1->peer_id == m2->peer_id)
+        {
+            msg(M_WARN, "disallowing peer %" PRIu32 " (%s) from floating to "
+                "its own address (%s)",
+                m1->peer_id, tls_common_name(mi->context.c2.tls_multi, false),
+                mroute_addr_print(&mi->real, &gc));
+            goto done;
+        }
         msg(D_MULTI_MEDIUM, "closing instance %s", multi_instance_string(ex_mi, false, &gc));
         multi_close_instance(m, ex_mi, false);
     }
@@ -3384,6 +3396,17 @@
         {
             process_incoming_del_peer(m, mi, dco);
         }
+#if defined(TARGET_LINUX) || defined(TARGET_WIN32)
+        else if (dco->dco_message_type == OVPN_CMD_FLOAT_PEER)
+        {
+            ASSERT(mi->context.c2.link_sockets[0]);
+            extract_dco_float_peer_addr(mi->context.c2.link_sockets[0]->info.af,
+                                        &m->top.c2.from.dest,
+                                        (struct sockaddr *)&dco->dco_float_peer_ss);
+            multi_process_float(m, mi, mi->context.c2.link_sockets[0]);
+            CLEAR(dco->dco_float_peer_ss);
+        }
+#endif /* if defined(TARGET_LINUX) || defined(TARGET_WIN32) */
         else if (dco->dco_message_type == OVPN_CMD_SWAP_KEYS)
         {
             tls_session_soft_reset(mi->context.c2.tls_multi);
diff --git a/src/openvpn/multi.h b/src/openvpn/multi.h
index 40f7519..fe9e847 100644
--- a/src/openvpn/multi.h
+++ b/src/openvpn/multi.h
@@ -322,7 +322,7 @@
 /**
  * Process an incoming DCO message (from kernel space).
  *
- * @param m            - The single \c multi_context structur.e
+ * @param m            - The single \c multi_context structure.
  *
  * @return
  *  - True, if the message was received correctly.
diff --git a/src/openvpn/ovpn_dco_linux.h b/src/openvpn/ovpn_dco_linux.h
index 680d152..b3c9ff0 100644
--- a/src/openvpn/ovpn_dco_linux.h
+++ b/src/openvpn/ovpn_dco_linux.h
@@ -99,6 +99,7 @@
 	OVPN_CMD_KEY_SWAP,
 	OVPN_CMD_KEY_SWAP_NTF,
 	OVPN_CMD_KEY_DEL,
+	OVPN_CMD_PEER_FLOAT_NTF,
 
 	__OVPN_CMD_MAX,
 	OVPN_CMD_MAX = (__OVPN_CMD_MAX - 1)
diff --git a/src/openvpn/ovpn_dco_win.h b/src/openvpn/ovpn_dco_win.h
index 865bb38..dd6b7ce 100644
--- a/src/openvpn/ovpn_dco_win.h
+++ b/src/openvpn/ovpn_dco_win.h
@@ -149,7 +149,8 @@
 
 typedef enum {
     OVPN_CMD_DEL_PEER,
-    OVPN_CMD_SWAP_KEYS
+    OVPN_CMD_SWAP_KEYS,
+    OVPN_CMD_FLOAT_PEER
 } OVPN_NOTIFY_CMD;
 
 typedef enum {
@@ -164,6 +165,7 @@
     OVPN_NOTIFY_CMD Cmd;
     int PeerId;
     OVPN_DEL_PEER_REASON DelPeerReason;
+    struct sockaddr_storage FloatAddress;
 } OVPN_NOTIFY_EVENT, * POVPN_NOTIFY_EVENT;
 
 typedef struct _OVPN_MP_DEL_PEER {

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

[Openvpn-devel] [PATCH applied] Re: dco: Add support for float notifications

[Gert D. <ge...@gr...>]

So, this was quite a bit of work from the already-+2'ed v3 :-)

The patch looks larger than it actually is, because it affects the
two low level DCO modules implemented plus the "midlayer" having to
update the peer address.  Each individual change is fairly straightforward.
I have stared at the code for quite a bit and everything looks reasonable
and works (now).

v3 had problems where it would apply the wrong logic to "does this v4
address need to be wrapped into a v6mapped v6 address?", so OpenVPN would
end up trying to send v6 packets on a v4-only socket (no good), and also
there is a race condition in the DCO message callback leading to lost
DCO notification if the timing is just right (they are read by the
"give me counters!" callback, which gets confused and just throws the
notification away) - leading to "TLS packets get sent to the old address"
and (worse) "on floating *back*, OpenVPN kills the current instance and
then SIGSEGVs".  The DCO notification race will be fixed by Antonio
ASAP - it's annoying, but with the fixes in v6, we just log a message
that makes clear what must be happened, ignore the second notification,
and do not crash.  This is really outside *this* patch to fix.


I tested this against Linux / float-notification-6.16.0-rc3-cb06c90
(Backport to ubuntu 20.04), with a Mac laptop on Wifi and LAN, plugging
and unplugging the LAN cable while an OpenVPN UDP session was active
and pings going on in the tunnel.  This nicely floats back and forth
(and fast, usually 0 or 1 packets lost).  Server had --tls-reneg 60,
which made it quickly obvious if userland's idea of "what IP address
does the client have?" was wrong (thanks to Ralf for that suggestion).

What I did not test (because it does not really happen in practice and
I couldn't get my current middleboxes to do that) is "client floats from
v4 to v6, on a dual-stacked socket" - this should work, but it needs
NAT64/NAT46 set up in a particular way which I gave up on.  If you 
are very bored, test this, and show me the float messages :-)

(In practice our client decides on 1 proto and 1 server IP, and sticks
to that, even if it *could* decide to just go to a different server IP
"because it can!" - we might add that one day, like "continuously test
latency to all server IPs and use the best path" or so, but that's more
a 2.8 feature...)

FTR, there is a v3 from Lev on the patch, which is for testing the
Windows side.  I have recorded that as well, so we have lots of ACKs
here :-)

Your patch has been applied to the master branch.

commit cb8a0f6f5741d102b667d98370ab4d553503d0b5
Author: Ralf Lici
Date:   Fri Jul 18 14:22:24 2025 +0200

     dco: Add support for float notifications

     Signed-off-by: Ralf Lici <ra...@ma...>
     Acked-by: Gert Doering <ge...@gr...>
     Acked-by: Antonio Quartulli <an...@ma...>
     Acked-by: Lev Stipakov <lst...@gm...>
     Message-Id: <202...@gr...>
     URL: https://www.mail-archive.com/ope...@li.../msg32210.html
     Signed-off-by: Gert Doering <ge...@gr...>


--
kind regards,

Gert Doering

Re: [Openvpn-devel] [PATCH ovpn] net: ovpn: use correct array size to parse nested attributes in ovpn_nl_key_swap_doit

[Antonio Q. <a...@un...>]

On 14/07/2025 16:20, Sabrina Dubroca wrote:
> In ovpn_nl_key_swap_doit, the attributes array used to parse the
> OVPN_A_KEYCONF uses OVPN_A_PEER_MAX instead of
> OVPN_A_KEYCONF_MAX. Note that this does not cause any bug, since
> currently OVPN_A_KEYCONF_MAX < OVPN_A_PEER_MAX.
> 
> Fixes: 203e2bf55990 ("ovpn: implement key add/get/del/swap via netlink")
> Signed-off-by: Sabrina Dubroca <sd...@qu...>

Nice catch, thanks a lot!
As you suggested I fixed the reverse christmas-tree while applying this 
patch.

It's now queued for sending to net-next.

Regards,

-- 
Antonio Quartulli

[Openvpn-devel] [M] Change in openvpn[master]: dco: Add support for float notifications

[cron2 (C. Review) <ge...@op...>]

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

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

Change subject: dco: Add support for float notifications
......................................................................


Patch Set 6: Code-Review+2


-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1084?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: I33e9272b4196c7634db2fb33a75ae4261660867f
Gerrit-Change-Number: 1084
Gerrit-PatchSet: 6
Gerrit-Owner: ralf_lici <ra...@ma...>
Gerrit-Reviewer: cron2 <ge...@gr...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: ordex <an...@ma...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-Reviewer: stipa <lst...@gm...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Attention: ordex <an...@ma...>
Gerrit-Attention: ralf_lici <ra...@ma...>
Gerrit-Attention: stipa <lst...@gm...>
Gerrit-Comment-Date: Fri, 18 Jul 2025 12:22:11 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment