diff mbox series

[Openvpn-devel,2/3] dco: bail out when no peer-specific message is delivered

Message ID 20230103202330.1835-2-a@unstable.cc
State Accepted
Headers
Series [Openvpn-devel,1/3] dco: properly re-initialize dco_del_peer_reason |

Commit Message

Antonio Quartulli Jan. 3, 2023, 8:23 p.m. UTC 
  multi_process_incoming_dco() is currently partly processing
messages that were actually discarded. This results in a bogus
message being printed:

"Received packet for peer-id unknown to OpenVPN: -1, type 0, reason 2"

Change the flow so that we bail out immediately when we know that no
message was truly delivered by DCO.
Currently this can be verified by chacking that the peed_is is greater
than -1.

Signed-off-by: Antonio Quartulli <a@unstable.cc>
---
 src/openvpn/multi.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Comments

Arne Schwabe Jan. 6, 2023, 3:04 p.m. UTC | #1 
Am 03.01.23 um 21:23 schrieb Antonio Quartulli:
> multi_process_incoming_dco() is currently partly processing
> messages that were actually discarded. This results in a bogus
> message being printed:
> 
> "Received packet for peer-id unknown to OpenVPN: -1, type 0, reason 2"
> 
> Change the flow so that we bail out immediately when we know that no
> message was truly delivered by DCO.
> Currently this can be verified by chacking that the peed_is is greater
> than -1.

I think we should somewhere properly document that peer-id = -1 has this 
meaning.

Acked-By: Arne Schwabe <arne@rfc2549.org>

Arne
Gert Doering Jan. 7, 2023, 5:53 p.m. UTC | #2 
I've done a bit of commit message grammar ("chacking that the peed_is")
and actually tested this one.  Without the check, my DCO test server
has quite a lot of these...

Jan  7 07:24:06 ubuntu2004 tun-udp-p2mp-topology-subnet[2441339]: Received packet for peer-id unknown to OpenVPN: -1, type 0, reason 2
Jan  7 07:24:06 ubuntu2004 tun-tcp-p2mp[2441317]: Received packet for peer-id unknown to OpenVPN: -1, type 0, reason 1

with the patch these are gone.  Very welcome de-noisification of my logs :-)


We do still have a bit of "logging redundancy" here... for every client
connect to instance A, p2p instance B logs 3 lines (on D_DCO_DEBUG)

Jan  7 18:35:35 ubuntu2004 tun-udp-p2p-tls-sha256[2463894]: dco_do_read
Jan  7 18:35:35 ubuntu2004 tun-udp-p2p-tls-sha256[2463894]: ovpn-dco: ignoring message (type=3) for foreign ifindex 34074
Jan  7 18:35:35 ubuntu2004 tun-udp-p2p-tls-sha256[2463894]: process_incoming_dco: received message of type 0 - ignoring

.. this is coming from forward.c::process_incoming_dco(), which I assume
is only called in the p2p case, and the first message is coming from
ovpn_handle_msg() (which is not using the __func__ paradigm... why?) - so
if we can assert that "there is no message, there is nothing to see
(and nothing to log!)" in the multi_process_incoming_dco() case - maybe
we can apply this (unwritten) function contract here as well...


Stare-at-code also agrees that if the function contract says
"if (peer_id < 0) there is no valid packet, ever", early exit is a
good way out - see my note about un-initializing the dco-> fields at 
the *end* of the function for 1/3, though - in this case, no un-init...
and also Arne's comment about "is this documented anywhere?")


Your patch has been applied to the master and release/2.6 branch.

commit 388e032019ec3674b8294c856039b96fe35e5f32 (master)
commit b0dee39c353ae9479fd19e66ae07cb336d57eef8 (release/2.6)
Author: Antonio Quartulli
Date:   Tue Jan 3 21:23:29 2023 +0100

     dco: bail out when no peer-specific message is delivered

     Signed-off-by: Antonio Quartulli <a@unstable.cc>
     Acked-by: Arne Schwabe <arne@rfc2549.org>
     Message-Id: <20230103202330.1835-2-a@unstable.cc>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg25882.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 27676de5..b10a6d8d 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -3270,7 +3270,15 @@  multi_process_incoming_dco(struct multi_context *m)
 
     int peer_id = dco->dco_message_peer_id;
 
-    if ((peer_id >= 0) && (peer_id < m->max_clients) && (m->instances[peer_id]))
+    /* no peer-specific message delivered -> nothing to process.
+     * bail out right away
+     */
+    if (peer_id < 0)
+    {
+        return ret > 0;
+    }
+
+    if ((peer_id < m->max_clients) && (m->instances[peer_id]))
     {
         mi = m->instances[peer_id];
         if (dco->dco_message_type == OVPN_CMD_PACKET)