diff mbox series

[Openvpn-devel,v2,1/4] Ensure we do not promote a TA_TIMEOUT to a TA_READ event with dco

Message ID 20221227022404.3468137-1-arne@rfc2549.org
State Rejected
Headers show
Series [Openvpn-devel,v2,1/4] Ensure we do not promote a TA_TIMEOUT to a TA_READ event with dco | expand

Commit Message

Arne Schwabe Dec. 27, 2022, 2:24 a.m. UTC 
with dco sometimes we end up promoting a timeout event to read event.
For the residual read, this problem is probably not solvable without
changing the kernel DCO API (ie. passing our residual on new_peer to
let the kernel handle assembling the next packet.)

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/forward.c | 24 ------------------------
 src/openvpn/forward.h | 30 ++++++++++++++++++++++++++++++
 src/openvpn/mtcp.c    | 12 +++++++++++-
 3 files changed, 41 insertions(+), 25 deletions(-)

Comments

Gert Doering Dec. 27, 2022, 3:52 p.m. UTC | #1 
Hi,

On Tue, Dec 27, 2022 at 03:24:01AM +0100, Arne Schwabe wrote:
> with dco sometimes we end up promoting a timeout event to read event.
> For the residual read, this problem is probably not solvable without
> changing the kernel DCO API (ie. passing our residual on new_peer to
> let the kernel handle assembling the next packet.)
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>

So after a long and extensive round of testing (this one as it is,
and 1/4+2/4 against the gremlins) I can say that it does not break
anything, and fixes the "too many TCP connects will cause a fd=-1
recv() and subsequent crash".

After some more discussion on IRC neither Arne and I seem to really
fully understand the residual stuff, so not merging this yet, more
study is needed...

gert
diff mbox series

Patch

diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index af4ed05da..61caf1146 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -1676,30 +1676,6 @@  process_ip_header(struct context *c, unsigned int flags, struct buffer *buf)
     }
 }
 
-/*
- * Linux DCO implementations pass the socket to the kernel and
- * disallow usage of it from userland for TCP, so (control) packets
- * sent and received by OpenVPN need to go through the DCO interface.
- *
- * Windows DCO needs control packets to be sent via the normal
- * standard Overlapped I/O.
- *
- * FreeBSD DCO allows control packets to pass through the socket in both
- * directions.
- *
- * Hide that complexity (...especially if more platforms show up
- * in the future...) in a small inline function.
- */
-static inline bool
-should_use_dco_socket(struct link_socket *ls)
-{
-#if defined(TARGET_LINUX)
-    return ls->dco_installed && proto_is_tcp(ls->info.proto);
-#else
-    return false;
-#endif
-}
-
 /*
  * Input: c->c2.to_link
  */
diff --git a/src/openvpn/forward.h b/src/openvpn/forward.h
index bd2d96010..5cddb5995 100644
--- a/src/openvpn/forward.h
+++ b/src/openvpn/forward.h
@@ -424,4 +424,34 @@  connection_established(struct context *c)
     }
 }
 
+
+/**
+ * @param ls the link_socket the decision should be made for
+ * @return if we should use the dco kernel api or normal socket APIs for
+ *         write/send
+ *
+ *
+ * Linux DCO implementations pass the socket to the kernel and
+ * disallow usage of it from userland for TCP, so (control) packets
+ * sent and received by OpenVPN need to go through the DCO interface.
+ *
+ * Windows DCO needs control packets to be sent via the normal
+ * standard Overlapped I/O.
+ *
+ * FreeBSD DCO allows control packets to pass through the socket in both
+ * directions.
+ *
+ * Hide that complexity (...especially if more platforms show up
+ * in the future...) in a small inline function.
+ */
+static inline bool
+should_use_dco_socket(const struct link_socket *ls)
+{
+#if defined(TARGET_LINUX)
+    return ls->dco_installed && proto_is_tcp(ls->info.proto);
+#else
+    return false;
+#endif
+}
+
 #endif /* FORWARD_H */
diff --git a/src/openvpn/mtcp.c b/src/openvpn/mtcp.c
index ac06ddc64..519630544 100644
--- a/src/openvpn/mtcp.c
+++ b/src/openvpn/mtcp.c
@@ -407,7 +407,7 @@  multi_tcp_wait_lite(struct multi_context *m, struct multi_instance *mi, const in
         /* If we got a socket that has been handed over to the kernel
          * we must not call the normal socket function to figure out
          * if it is readable or writable */
-        /* Assert that we only have the DCO exptected flags */
+        /* Assert that we only have the DCO expected flags */
         ASSERT(action & (TA_SOCKET_READ | TA_SOCKET_WRITE));
 
         /* We are always ready! */
@@ -586,6 +586,16 @@  multi_tcp_post(struct multi_context *m, struct multi_instance *mi, const int act
         case MTP_NONE:
             if (mi && socket_read_residual(c->c2.link_socket))
             {
+                if (should_use_dco_socket(c->c2.link_socket))
+                {
+                    struct gc_arena gc = gc_new();
+                    msg(M_INFO, "ovpn-dco installed socket with residual read "
+                        "len=%d, mi=%s. This connection will probably"
+                        " break.",  BLEN(&c->c2.link_socket->stream_buf.residual),
+                        multi_instance_string(mi, false, &gc));
+                    gc_free(&gc);
+                    break;
+                }
                 newaction = TA_SOCKET_READ_RESIDUAL;
             }
             else