[Openvpn-devel] [S] Change in openvpn[master]: Fix float support in P2P topology

[Openvpn-devel] [S] Change in openvpn[master]: Fix float support in P2P topology

[ralf_lici (Code Review)]

[-- Attachment #1: Type: text/plain, Size: 3144 bytes --]

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

to review the following change.


Change subject: Fix float support in P2P topology
......................................................................

Fix float support in P2P topology

Fix the handling of floating operations in P2P topology, where new UDP
endpoints were previously ignored. When floating occurs, this update
processes the new endpoints and updates the address if the `--float`
option is specified or `--remote` is omitted.

Since the same code path is used for clients in MP topology, this change
also enables processing of server floating operations from the client
perspective.

Change-Id: I806757a8c6f9a589665624f176391b5f7b87f581
Signed-off-by: Ralf Lici <ralf@...2726...>
---
M src/openvpn/forward.c
1 file changed, 23 insertions(+), 2 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/44/844/1

diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 2c72001..5feffba 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -1227,12 +1227,33 @@
 {
     perf_push(PERF_PROC_IN_LINK);

+    struct gc_arena gc = gc_new();
+    bool floated = false;
     struct link_socket_info *lsi = &sock->info;
     const uint8_t *orig_buf = c->c2.buf.data;
+    const struct link_socket_actual *incoming = &c->c2.from;
+    struct link_socket_actual *remote = c->c2.to_link_addr;
+    const sa_family_t family = incoming->dest.addr.sa.sa_family;

-    process_incoming_link_part1(c, lsi, false);
-    process_incoming_link_part2(c, lsi, orig_buf);
+    if (remote && (family == AF_INET || family == AF_INET6))
+    {
+        floated = !link_socket_actual_match(incoming, remote);
+    }

+    if (process_incoming_link_part1(c, lsi, floated))
+    {
+        if (floated && c->c2.buf.len > 0)
+        {
+            msg(D_LOW, "peer floated from %s to %s",
+                print_link_socket_actual(remote, &gc),
+                print_link_socket_actual(incoming, &gc));
+            link_socket_set_outgoing_addr(lsi, &c->c2.from, NULL, c->c2.es);
+            tls_update_remote_addr(c->c2.tls_multi, incoming);
+        }
+        process_incoming_link_part2(c, lsi, orig_buf);
+    }
+
+    gc_free(&gc);
     perf_pop();
 }


--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/844?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: I806757a8c6f9a589665624f176391b5f7b87f581
Gerrit-Change-Number: 844
Gerrit-PatchSet: 1
Gerrit-Owner: ralf_lici <ralf@...2726...>
Gerrit-Reviewer: flichtenheld <frank@...2641...>
Gerrit-Reviewer: plaisthos <arne-openvpn@...1227...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-Attention: plaisthos <arne-openvpn@...1227...>
Gerrit-Attention: flichtenheld <frank@...2641...>
Gerrit-MessageType: newchange

[-- Attachment #2: Type: text/html, Size: 5676 bytes --]

[Openvpn-devel] [S] Change in openvpn[master]: Fix float support in P2P topology

[cron2 (Code Review)]

[-- Attachment #1: Type: text/plain, Size: 2273 bytes --]

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

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

Change subject: Fix float support in P2P topology
......................................................................


Patch Set 1:

(4 comments)

Patchset:

PS1:
This looks like it should work (I do have a few nags)


File src/openvpn/forward.c:

http://gerrit.openvpn.net/c/openvpn/+/844/comment/54eabdc3_8b8edf10 :
PS1, Line 1238:     if (remote && (family == AF_INET || family == AF_INET6))
I'm not sure what *that* does?  When can we ever have a `remote` and an family that is neither `AF_INET` nor `AF_INET6`?  (If we're waiting for incoming data it can be `AF_UNSPEC`, but then we have no `remote`, no?)


http://gerrit.openvpn.net/c/openvpn/+/844/comment/c48925bd_4bc7c300 :
PS1, Line 1245:         if (floated && c->c2.buf.len > 0)
the GC seems only used inside here, and since this is a performance relevant function ("hot path", executed for each packet) we should not doing unneccessary work -> `gc_new(), gc_free()` can be inside the `if (floated)`.


http://gerrit.openvpn.net/c/openvpn/+/844/comment/21341812_f18c537c :
PS1, Line 1253:         process_incoming_link_part2(c, lsi, orig_buf);
should this be *inside* the `if()`?  In the old code `_part2()` is unconditionally called always



--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/844?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: I806757a8c6f9a589665624f176391b5f7b87f581
Gerrit-Change-Number: 844
Gerrit-PatchSet: 1
Gerrit-Owner: ralf_lici <ralf@...2726...>
Gerrit-Reviewer: flichtenheld <frank@...2641...>
Gerrit-Reviewer: plaisthos <arne-openvpn@...1227...>
Gerrit-CC: cron2 <gert@...1296...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-Attention: plaisthos <arne-openvpn@...1227...>
Gerrit-Attention: flichtenheld <frank@...2641...>
Gerrit-Attention: ralf_lici <ralf@...2726...>
Gerrit-Comment-Date: Fri, 20 Dec 2024 21:56:39 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[-- Attachment #2: Type: text/html, Size: 4217 bytes --]

[Openvpn-devel] [S] Change in openvpn[master]: Fix float support in P2P topology

[ralf_lici (Code Review)]

[-- Attachment #1: Type: text/plain, Size: 2916 bytes --]

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

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

Change subject: Fix float support in P2P topology
......................................................................


Patch Set 1:

(4 comments)

File src/openvpn/forward.c:

http://gerrit.openvpn.net/c/openvpn/+/844/comment/a2f4224f_180af603 :
PS1, Line 1238:     if (remote && (family == AF_INET || family == AF_INET6))
> I'm not sure what *that* does?  When can we ever have a `remote` and an family that is neither `AF_I […]
Acknowledged


http://gerrit.openvpn.net/c/openvpn/+/844/comment/ed3d6af5_b5a33468 :
PS1, Line 1245:         if (floated && c->c2.buf.len > 0)
> the GC seems only used inside here, and since this is a performance relevant function ("hot path", e […]
Definitely, I think I forgot to reorder this when I removed the other debugging prints I used for testing purposes. Acknowledged.


http://gerrit.openvpn.net/c/openvpn/+/844/comment/34c401d6_00f96d38 :
PS1, Line 1251: tls_update_remote_addr
Tested the non-tls p2p configuration (`--secret`) and realized that this line obviously segfaults. Sorry for this, I will fix it on the next patch.


http://gerrit.openvpn.net/c/openvpn/+/844/comment/ee561cd1_89c6d5cc :
PS1, Line 1253:         process_incoming_link_part2(c, lsi, orig_buf);
> should this be *inside* the `if()`?  In the old code `_part2()` is unconditionally called always
AFAICT the output of `_part1()` is correlated with `c->c2.buf.len` such that `buf.len` is always <= 0 if `_part1()` returns false (though the reverse isn't always true). Since `_part2()` processes a packet only if `buf.len > 0`, it seems safe to call `_part2()` only if `_part1()` returns true. However, as the current implementation works and I’m not entirely certain, I suggest leaving it as is unless there’s a strong reason to switch to conditional execution. What do you think?



--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/844?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: I806757a8c6f9a589665624f176391b5f7b87f581
Gerrit-Change-Number: 844
Gerrit-PatchSet: 1
Gerrit-Owner: ralf_lici <ralf@...2726...>
Gerrit-Reviewer: flichtenheld <frank@...2641...>
Gerrit-Reviewer: plaisthos <arne-openvpn@...1227...>
Gerrit-CC: cron2 <gert@...1296...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-Attention: plaisthos <arne-openvpn@...1227...>
Gerrit-Attention: cron2 <gert@...1296...>
Gerrit-Attention: flichtenheld <frank@...2641...>
Gerrit-Comment-Date: Sat, 21 Dec 2024 15:23:08 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: cron2 <gert@...1296...>
Gerrit-MessageType: comment

[-- Attachment #2: Type: text/html, Size: 5099 bytes --]

[Openvpn-devel] [S] Change in openvpn[master]: Fix float support in P2P topology

[ralf_lici (Code Review)]

[-- Attachment #1: Type: text/plain, Size: 3079 bytes --]

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

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


Change subject: Fix float support in P2P topology
......................................................................

Fix float support in P2P topology

Fix the handling of floating operations in P2P topology, where new UDP
endpoints were previously ignored. When floating occurs, this update
processes the new endpoints and updates the address if the `--float`
option is specified or `--remote` is omitted.

Since the same code path is used for clients in MP topology, this change
also enables processing of server floating operations from the client
perspective.

Change-Id: I806757a8c6f9a589665624f176391b5f7b87f581
Signed-off-by: Ralf Lici <ralf@...2726...>
---
M src/openvpn/forward.c
1 file changed, 25 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/44/844/2

diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 2c72001..0aa5b6a 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -1227,10 +1227,34 @@
 {
     perf_push(PERF_PROC_IN_LINK);

+    bool floated = false;
     struct link_socket_info *lsi = &sock->info;
     const uint8_t *orig_buf = c->c2.buf.data;
+    const struct link_socket_actual *incoming = &c->c2.from;
+    struct link_socket_actual *remote = c->c2.to_link_addr;
 
-    process_incoming_link_part1(c, lsi, false);
+    if (remote)
+    {
+        floated = !link_socket_actual_match(incoming, remote);
+    }
+
+    process_incoming_link_part1(c, lsi, floated);
+    if (floated && c->c2.buf.len > 0)
+    {
+        struct gc_arena gc = gc_new();
+
+        msg(D_LOW, "peer floated from %s to %s",
+            print_link_socket_actual(remote, &gc),
+            print_link_socket_actual(incoming, &gc));
+
+        link_socket_set_outgoing_addr(lsi, &c->c2.from, NULL, c->c2.es);
+        if (TLS_MODE(c))
+        {
+            tls_update_remote_addr(c->c2.tls_multi, incoming);
+        }
+
+        gc_free(&gc);
+    }
     process_incoming_link_part2(c, lsi, orig_buf);

     perf_pop();

--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/844?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: I806757a8c6f9a589665624f176391b5f7b87f581
Gerrit-Change-Number: 844
Gerrit-PatchSet: 2
Gerrit-Owner: ralf_lici <ralf@...2726...>
Gerrit-Reviewer: flichtenheld <frank@...2641...>
Gerrit-Reviewer: plaisthos <arne-openvpn@...1227...>
Gerrit-CC: cron2 <gert@...1296...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-Attention: plaisthos <arne-openvpn@...1227...>
Gerrit-Attention: cron2 <gert@...1296...>
Gerrit-Attention: flichtenheld <frank@...2641...>
Gerrit-MessageType: newpatchset

[-- Attachment #2: Type: text/html, Size: 5689 bytes --]

[Openvpn-devel] [S] Change in openvpn[master]: Fix float support in P2P topology

[ralf_lici (Code Review)]

[-- Attachment #1: Type: text/plain, Size: 1559 bytes --]

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

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

Change subject: Fix float support in P2P topology
......................................................................


Patch Set 2:

(1 comment)

File src/openvpn/forward.c:

http://gerrit.openvpn.net/c/openvpn/+/844/comment/fd7d307f_f86255fe :
PS1, Line 1253:         process_incoming_link_part2(c, lsi, orig_buf);
> AFAICT the output of `_part1()` is correlated with `c->c2.buf.len` such that `buf. […]
Done



--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/844?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: I806757a8c6f9a589665624f176391b5f7b87f581
Gerrit-Change-Number: 844
Gerrit-PatchSet: 2
Gerrit-Owner: ralf_lici <ralf@...2726...>
Gerrit-Reviewer: flichtenheld <frank@...2641...>
Gerrit-Reviewer: plaisthos <arne-openvpn@...1227...>
Gerrit-CC: cron2 <gert@...1296...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-Attention: plaisthos <arne-openvpn@...1227...>
Gerrit-Attention: cron2 <gert@...1296...>
Gerrit-Attention: flichtenheld <frank@...2641...>
Gerrit-Comment-Date: Tue, 07 Jan 2025 09:53:32 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: cron2 <gert@...1296...>
Comment-In-Reply-To: ralf_lici <ralf@...2726...>
Gerrit-MessageType: comment

[-- Attachment #2: Type: text/html, Size: 2914 bytes --]

[Openvpn-devel] [S] Change in openvpn[master]: Fix float support in P2P topology

[ralf_lici (Code Review)]

[-- Attachment #1: Type: text/plain, Size: 3129 bytes --]

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

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


Change subject: Fix float support in P2P topology
......................................................................

Fix float support in P2P topology

Fix the handling of floating operations in P2P topology, where new UDP
endpoints were previously ignored. When floating occurs, this update
processes the new endpoints and updates the address if the `--float`
option is specified or `--remote` is omitted.

Since the same code path is used for clients in MP topology, this change
also enables processing of server floating operations from the client
perspective.

Change-Id: I806757a8c6f9a589665624f176391b5f7b87f581
Signed-off-by: Ralf Lici <ralf@...2726...>
---
M src/openvpn/forward.c
1 file changed, 25 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/44/844/3

diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 2c72001..76df59d5 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -1227,10 +1227,34 @@
 {
     perf_push(PERF_PROC_IN_LINK);

+    bool floated = false;
     struct link_socket_info *lsi = &sock->info;
     const uint8_t *orig_buf = c->c2.buf.data;
+    const struct link_socket_actual *incoming = &c->c2.from;
+    struct link_socket_actual *remote = c->c2.to_link_addr;
 
-    process_incoming_link_part1(c, lsi, false);
+    if (remote && incoming->dest.addr.sa.sa_family != AF_UNSPEC)
+    {
+        floated = !link_socket_actual_match(incoming, remote);
+    }
+
+    process_incoming_link_part1(c, lsi, floated);
+    if (floated && c->c2.buf.len > 0)
+    {
+        struct gc_arena gc = gc_new();
+
+        msg(D_LOW, "peer floated from %s to %s",
+            print_link_socket_actual(remote, &gc),
+            print_link_socket_actual(incoming, &gc));
+
+        link_socket_set_outgoing_addr(lsi, &c->c2.from, NULL, c->c2.es);
+        if (TLS_MODE(c))
+        {
+            tls_update_remote_addr(c->c2.tls_multi, incoming);
+        }
+
+        gc_free(&gc);
+    }
     process_incoming_link_part2(c, lsi, orig_buf);

     perf_pop();

--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/844?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: I806757a8c6f9a589665624f176391b5f7b87f581
Gerrit-Change-Number: 844
Gerrit-PatchSet: 3
Gerrit-Owner: ralf_lici <ralf@...2726...>
Gerrit-Reviewer: flichtenheld <frank@...2641...>
Gerrit-Reviewer: plaisthos <arne-openvpn@...1227...>
Gerrit-CC: cron2 <gert@...1296...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-Attention: plaisthos <arne-openvpn@...1227...>
Gerrit-Attention: cron2 <gert@...1296...>
Gerrit-Attention: flichtenheld <frank@...2641...>
Gerrit-MessageType: newpatchset

[-- Attachment #2: Type: text/html, Size: 5750 bytes --]