dco: don't use NetLink to exchange control packets

Using NetLink for control messages did not work out as it did lead to kernel side buffer congestion during heavy client activity. With this patch DCO will redirect control packets directly to the transport socket without altering them, so that userspace can happily process them as usual. NOTE: this is an API breaking change. Up to this commit, the userland requests a kernel module called "ovpn-dco" which does control messages via netlink. From this commit on, OpenVPN requests a kernel module named "ovpn-dco-v2" which brings the kernel change corresponding to this commit. If the system only has "the wrong module" available (either way), OpenVPN will log ... Kernel support for ovpn-dco missing, disabling data channel offload. and proceed without kernel support. Change-Id: Ia1297c3ae9a28b188ed21ad21ae96fff3d02ee4d [lev@openvpn.net: ensure win_dco flag is still exposed] Signed-off-by: Antonio Quartulli <a@unstable.cc> Acked-by: Arne Schwabe <arne@rfc2549.org> Message-Id: <20230309210344.5763-1-a@unstable.cc> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg26384.html Signed-off-by: Gert Doering <gert@greenie.muc.de> (cherry picked from commit ac1d242)
OpenVPN · Mar 13, 2023 · 321b04f · 321b04f
1 parent 5eb94ce
commit 321b04f
Show file tree

Hide file tree

Showing 14 changed files with 35 additions and 296 deletions.
diff --git a/src/openvpn/dco.c b/src/openvpn/dco.c
@@ -485,7 +485,6 @@ dco_p2p_add_new_peer(struct context *c)
     }
 
     c->c2.tls_multi->dco_peer_id = multi->peer_id;
-    c->c2.link_socket->dco_installed = true;
 
     return 0;
 }
@@ -605,17 +604,6 @@ dco_multi_add_new_peer(struct multi_context *m, struct multi_instance *mi)
 
     c->c2.tls_multi->dco_peer_id = peer_id;
 
-    if (c->mode == CM_CHILD_TCP)
-    {
-        multi_tcp_dereference_instance(m->mtcp, mi);
-        if (close(sd))
-        {
-            msg(D_DCO|M_ERRNO, "error closing TCP socket after DCO handover");
-        }
-        c->c2.link_socket->dco_installed = true;
-        c->c2.link_socket->sd = SOCKET_UNDEFINED;
-    }
-
     return 0;
 }
 

diff --git a/src/openvpn/dco.h b/src/openvpn/dco.h
@@ -127,15 +127,6 @@ void close_tun_dco(struct tuntap *tt, openvpn_net_ctx_t *ctx);
  */
 int dco_do_read(dco_context_t *dco);
 
-/**
- * Write data to the DCO communication channel (control packet expected)
- *
- * @param dco       the DCO context
- * @param peer_id   the ID of the peer to send the data to
- * @param buf       the buffer containing the data to send
- */
-int dco_do_write(dco_context_t *dco, int peer_id, struct buffer *buf);
-
 /**
  * Install a DCO in the main event loop
  */
@@ -301,13 +292,6 @@ dco_do_read(dco_context_t *dco)
     return 0;
 }
 
-static inline int
-dco_do_write(dco_context_t *dco, int peer_id, struct buffer *buf)
-{
-    ASSERT(false);
-    return 0;
-}
-
 static inline void
 dco_event_set(dco_context_t *dco, struct event_set *es, void *arg)
 {

diff --git a/src/openvpn/dco_freebsd.c b/src/openvpn/dco_freebsd.c
@@ -142,7 +142,6 @@ open_fd(dco_context_t *dco)
     {
         dco->open = true;
     }
-    dco->dco_packet_in = alloc_buf(PAGE_SIZE);
 
     return dco->fd;
 }
@@ -560,15 +559,6 @@ dco_do_read(dco_context_t *dco)
     return 0;
 }
 
-int
-dco_do_write(dco_context_t *dco, int peer_id, struct buffer *buf)
-{
-    /* Control packets are passed through the socket, so this should never get
-     * called. See should_use_dco_socket(). */
-    ASSERT(0);
-    return -EINVAL;
-}
-
 bool
 dco_available(int msglevel)
 {

diff --git a/src/openvpn/dco_freebsd.h b/src/openvpn/dco_freebsd.h
@@ -51,8 +51,6 @@ typedef struct dco_context {
 
     char ifname[IFNAMSIZ];
 
-    struct buffer dco_packet_in;
-
     int dco_message_type;
     int dco_message_peer_id;
     int dco_del_peer_reason;

diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c
@@ -436,24 +436,6 @@ ovpn_dco_register(dco_context_t *dco)
     {
         msg(M_ERR, "%s: failed to join groups: %d", __func__, ret);
     }
-
-    /* Register for non-data packets that ovpn-dco may receive. They will be
-     * forwarded to userspace
-     */
-    struct nl_msg *nl_msg = ovpn_dco_nlmsg_create(dco, OVPN_CMD_REGISTER_PACKET);
-    if (!nl_msg)
-    {
-        msg(M_ERR, "%s: cannot allocate message to register for control packets",
-            __func__);
-    }
-
-    ret = ovpn_nl_msg_send(dco, nl_msg, NULL, __func__);
-    if (ret)
-    {
-        msg(M_ERR, "%s: failed to register for control packets: %d", __func__,
-            ret);
-    }
-    nlmsg_free(nl_msg);
 }
 
 int
@@ -476,8 +458,6 @@ open_tun_dco(struct tuntap *tt, openvpn_net_ctx_t *ctx, const char *dev)
     }
 
     tt->actual_name = string_alloc(dev, NULL);
-    uint8_t *dcobuf = malloc(65536);
-    buf_set_write(&tt->dco.dco_packet_in, dcobuf, 65536);
     tt->dco.dco_message_peer_id = -1;
 
     ovpn_dco_register(&tt->dco);
@@ -492,7 +472,6 @@ close_tun_dco(struct tuntap *tt, openvpn_net_ctx_t *ctx)
 
     net_iface_del(ctx, tt->actual_name);
     ovpn_dco_uninit_netlink(&tt->dco);
-    free(tt->dco.dco_packet_in.data);
 }
 
 int
@@ -823,51 +802,6 @@ ovpn_handle_msg(struct nl_msg *msg, void *arg)
             break;
         }
 
-        case OVPN_CMD_PACKET:
-        {
-            if (!attrs[OVPN_ATTR_PACKET])
-            {
-                msg(D_DCO, "ovpn-dco: no packet in OVPN_CMD_PACKET message");
-                return NL_SKIP;
-            }
-            struct nlattr *pkt_attrs[OVPN_PACKET_ATTR_MAX + 1];
-
-            if (nla_parse_nested(pkt_attrs, OVPN_PACKET_ATTR_MAX,
-                                 attrs[OVPN_ATTR_PACKET], NULL))
-            {
-                msg(D_DCO, "received bogus cmd packet data from ovpn-dco");
-                return NL_SKIP;
-            }
-            if (!pkt_attrs[OVPN_PACKET_ATTR_PEER_ID])
-            {
-                msg(D_DCO, "ovpn-dco: Received OVPN_CMD_PACKET message without peer id");
-                return NL_SKIP;
-            }
-            if (!pkt_attrs[OVPN_PACKET_ATTR_PACKET])
-            {
-                msg(D_DCO, "ovpn-dco: Received OVPN_CMD_PACKET message without packet");
-                return NL_SKIP;
-            }
-
-            unsigned int peerid = nla_get_u32(pkt_attrs[OVPN_PACKET_ATTR_PEER_ID]);
-
-            uint8_t *data = nla_data(pkt_attrs[OVPN_PACKET_ATTR_PACKET]);
-            int len = nla_len(pkt_attrs[OVPN_PACKET_ATTR_PACKET]);
-
-            msg(D_DCO_DEBUG, "ovpn-dco: received OVPN_PACKET_ATTR_PACKET, ifindex: %d peer-id: %d, len %d",
-                ifindex, peerid, len);
-            if (BLEN(&dco->dco_packet_in) > 0)
-            {
-                msg(D_DCO, "DCO packet buffer still full?!");
-                return NL_SKIP;
-            }
-            buf_init(&dco->dco_packet_in, 0);
-            buf_write(&dco->dco_packet_in, data, len);
-            dco->dco_message_peer_id = peerid;
-            dco->dco_message_type = OVPN_CMD_PACKET;
-            break;
-        }
-
         default:
             msg(D_DCO, "ovpn-dco: received unknown command: %d", gnlh->cmd);
             dco->dco_message_type = 0;
@@ -886,41 +820,6 @@ dco_do_read(dco_context_t *dco)
     return ovpn_nl_recvmsgs(dco, __func__);
 }
 
-int
-dco_do_write(dco_context_t *dco, int peer_id, struct buffer *buf)
-{
-    packet_size_type len = BLEN(buf);
-    dmsg(D_STREAM_DEBUG, "DCO: WRITE %d offset=%d", (int)len, buf->offset);
-
-    msg(D_DCO_DEBUG, "%s: peer-id %d, len=%d", __func__, peer_id, len);
-
-    struct nl_msg *nl_msg = ovpn_dco_nlmsg_create(dco, OVPN_CMD_PACKET);
-
-    if (!nl_msg)
-    {
-        return -ENOMEM;
-    }
-
-    struct nlattr *attr = nla_nest_start(nl_msg, OVPN_ATTR_PACKET);
-    int ret = -EMSGSIZE;
-    NLA_PUT_U32(nl_msg, OVPN_PACKET_ATTR_PEER_ID, peer_id);
-    NLA_PUT(nl_msg, OVPN_PACKET_ATTR_PACKET, len, BSTR(buf));
-    nla_nest_end(nl_msg, attr);
-
-    ret = ovpn_nl_msg_send(dco, nl_msg, NULL, __func__);
-    if (ret)
-    {
-        goto nla_put_failure;
-    }
-
-    /* return the length of the written data in case of success */
-    ret = len;
-
-nla_put_failure:
-    nlmsg_free(nl_msg);
-    return ret;
-}
-
 int
 dco_get_peer_stats_multi(dco_context_t *dco, struct multi_context *m)
 {

diff --git a/src/openvpn/dco_linux.h b/src/openvpn/dco_linux.h
@@ -48,8 +48,6 @@ typedef struct
 
     unsigned int ifindex;
 
-    struct buffer dco_packet_in;
-
     int dco_message_type;
     int dco_message_peer_id;
     int dco_del_peer_reason;

diff --git a/src/openvpn/dco_win.c b/src/openvpn/dco_win.c
@@ -393,14 +393,6 @@ dco_do_read(dco_context_t *dco)
     return 0;
 }
 
-int
-dco_do_write(dco_context_t *dco, int peer_id, struct buffer *buf)
-{
-    /* no-op on windows */
-    ASSERT(0);
-    return 0;
-}
-
 int
 dco_get_peer_stats_multi(dco_context_t *dco, struct multi_context *m)
 {

diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
@@ -1191,7 +1191,6 @@ static void
 process_incoming_dco(struct context *c)
 {
 #if defined(ENABLE_DCO) && (defined(TARGET_LINUX) || defined(TARGET_FREEBSD))
-    struct link_socket_info *lsi = get_link_socket_info(c);
     dco_context_t *dco = &c->c1.tuntap->dco;
 
     dco_do_read(dco);
@@ -1204,35 +1203,23 @@ process_incoming_dco(struct context *c)
         msg(D_DCO_DEBUG, "%s: received message for mismatching peer-id %d, "
             "expected %d", __func__, dco->dco_message_peer_id,
             c->c2.tls_multi->dco_peer_id);
-        /* ensure we also drop a message if there is one in the buffer */
-        buf_init(&dco->dco_packet_in, 0);
         return;
     }
 
-    if ((dco->dco_message_type == OVPN_CMD_DEL_PEER)
-        && (dco->dco_del_peer_reason == OVPN_DEL_PEER_REASON_EXPIRED))
+    if (dco->dco_message_type != OVPN_CMD_DEL_PEER)
     {
-        msg(D_DCO_DEBUG, "%s: received peer expired notification of for peer-id "
-            "%d", __func__, dco->dco_message_peer_id);
-        trigger_ping_timeout_signal(c);
+        msg(D_DCO_DEBUG, "%s: received message of type %u - ignoring", __func__,
+            dco->dco_message_type);
         return;
     }
 
-    if (dco->dco_message_type != OVPN_CMD_PACKET)
+    if (dco->dco_del_peer_reason == OVPN_DEL_PEER_REASON_EXPIRED)
     {
-        msg(D_DCO_DEBUG, "%s: received message of type %u - ignoring", __func__,
-            dco->dco_message_type);
+        msg(D_DCO_DEBUG, "%s: received peer expired notification of for peer-id "
+            "%d", __func__, dco->dco_message_peer_id);
+        trigger_ping_timeout_signal(c);
         return;
     }
-
-    struct buffer orig_buff = c->c2.buf;
-    c->c2.buf = dco->dco_packet_in;
-    c->c2.from = lsi->lsa->actual;
-
-    process_incoming_link(c);
-
-    c->c2.buf = orig_buff;
-    buf_init(&dco->dco_packet_in, 0);
 #endif /* if defined(ENABLE_DCO) && (defined(TARGET_LINUX) || defined(TARGET_FREEBSD)) */
 }
 
@@ -1686,30 +1673,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
  */
@@ -1783,17 +1746,7 @@ process_outgoing_link(struct context *c)
                 socks_preprocess_outgoing_link(c, &to_addr, &size_delta);
 
                 /* Send packet */
-                if (should_use_dco_socket(c->c2.link_socket))
-                {
-                    size = dco_do_write(&c->c1.tuntap->dco,
-                                        c->c2.tls_multi->dco_peer_id,
-                                        &c->c2.to_link);
-                }
-                else
-                {
-                    size = link_socket_write(c->c2.link_socket, &c->c2.to_link,
-                                             to_addr);
-                }
+                size = link_socket_write(c->c2.link_socket, &c->c2.to_link, to_addr);
 
                 /* Undo effect of prepend */
                 link_socket_write_post_size_adjust(&size, size_delta, &c->c2.to_link);

diff --git a/src/openvpn/init.c b/src/openvpn/init.c
@@ -3914,8 +3914,7 @@ do_close_link_socket(struct context *c)
     /* in dco-win case, link socket is a tun handle which is
      * closed in do_close_tun(). Set it to UNDEFINED so
      * we won't use WinSock API to close it. */
-    if (tuntap_is_dco_win(c->c1.tuntap) && c->c2.link_socket
-        && c->c2.link_socket->dco_installed)
+    if (tuntap_is_dco_win(c->c1.tuntap) && c->c2.link_socket)
     {
         c->c2.link_socket->sd = SOCKET_UNDEFINED;
     }