tunnel: refactor tunnel verdict handling

Observed:

STARTTLS creates 2 pseudo packets which are tied to a real packet.
TPR (tunnel packet ref) counter increased to 2.

Pseudo 1: goes through 'verdict', increments 'ready to verdict' to 1.
Packet pool return code frees this packet and decrements TPR in root
to 1. RTV counter not changed. So both are now 1.

Pseudo 2: verdict code sees RTV == TPR, so verdict is set based on
pseudo packet. This is too soon. Packet pool return code frees this
packet and decrements TPR in root to 0.

Real packet: TRP is 0 so set verdict on this packet. As verdict was
already set, NFQ reports an issue.

The decrementing of TPR doesn't seem to make sense as RTV is not
updated.

Solution:

This patch refactors the ref count and verdict count logic. The beef
is now handled in the generic function TmqhOutputPacketpool(). NFQ
and IPFW call a utility function VerdictTunnelPacket to see if they
need to verdict a packet.

Remove some unused macro's for managing these counters.

diff --git a/src/decode.h b/src/decode.h
index 92ae217c828d49346dd548fba406e40d9c534b2d..ca44cacbf7a3c6cdf8fd399b2d8d528aae582f33 100644 (file)
--- a/src/decode.h
+++ b/src/decode.h
@@ -870,10 +870,8 @@ void CaptureStatsSetup(ThreadVars *tv, CaptureStats *s);
      ((p)->action |= a)); \
 } while (0)
 
-#define TUNNEL_INCR_PKT_RTV(p) do {                                                 \
-        SCMutexLock((p)->root ? &(p)->root->tunnel_mutex : &(p)->tunnel_mutex);     \
+#define TUNNEL_INCR_PKT_RTV_NOLOCK(p) do {                                          \
         ((p)->root ? (p)->root->tunnel_rtv_cnt++ : (p)->tunnel_rtv_cnt++);          \
-        SCMutexUnlock((p)->root ? &(p)->root->tunnel_mutex : &(p)->tunnel_mutex);   \
     } while (0)
 
 #define TUNNEL_INCR_PKT_TPR(p) do {                                                 \
@@ -882,16 +880,6 @@ void CaptureStatsSetup(ThreadVars *tv, CaptureStats *s);
         SCMutexUnlock((p)->root ? &(p)->root->tunnel_mutex : &(p)->tunnel_mutex);   \
     } while (0)
 
-#define TUNNEL_DECR_PKT_TPR(p) do {                                                 \
-        SCMutexLock((p)->root ? &(p)->root->tunnel_mutex : &(p)->tunnel_mutex);     \
-        ((p)->root ? (p)->root->tunnel_tpr_cnt-- : (p)->tunnel_tpr_cnt--);          \
-        SCMutexUnlock((p)->root ? &(p)->root->tunnel_mutex : &(p)->tunnel_mutex);   \
-    } while (0)
-
-#define TUNNEL_DECR_PKT_TPR_NOLOCK(p) do {                                          \
-        ((p)->root ? (p)->root->tunnel_tpr_cnt-- : (p)->tunnel_tpr_cnt--);          \
-    } while (0)
-
 #define TUNNEL_PKT_RTV(p) ((p)->root ? (p)->root->tunnel_rtv_cnt : (p)->tunnel_rtv_cnt)
 #define TUNNEL_PKT_TPR(p) ((p)->root ? (p)->root->tunnel_tpr_cnt : (p)->tunnel_tpr_cnt)
 
@@ -1132,5 +1120,37 @@ int DecoderParseDataFromFileSerie(char *fileprefix, DecoderFunc Decoder);
 
 #define PKT_SET_SRC(p, src_val) ((p)->pkt_src = src_val)
 
+/** \brief return true if *this* packet needs to trigger a verdict.
+ *
+ *  If we have the root packet, and we have none outstanding,
+ *  we can verdict now.
+ *
+ *  If we have a upper layer packet, it's the only one and root
+ *  is already processed, we can verdict now.
+ *
+ *  Otherwise, a future packet will issue the verdict.
+ */
+static inline bool VerdictTunnelPacket(Packet *p)
+{
+    bool verdict = true;
+    SCMutex *m = p->root ? &p->root->tunnel_mutex : &p->tunnel_mutex;
+    SCMutexLock(m);
+    const uint16_t outstanding = TUNNEL_PKT_TPR(p) - TUNNEL_PKT_RTV(p);
+    SCLogDebug("tunnel: outstanding %u", outstanding);
+
+    /* if there are packets outstanding, we won't verdict this one */
+    if (IS_TUNNEL_ROOT_PKT(p) && !IS_TUNNEL_PKT_VERDICTED(p) && !outstanding) {
+        // verdict
+        SCLogDebug("root %p: verdict", p);
+    } else if (!IS_TUNNEL_ROOT_PKT(p) && outstanding == 1 && p->root && IS_TUNNEL_PKT_VERDICTED(p->root)) {
+        // verdict
+        SCLogDebug("tunnel %p: verdict", p);
+    } else {
+        verdict = false;
+    }
+    SCMutexUnlock(m);
+    return verdict;
+}
+
 #endif /* __DECODE_H__ */
 

diff --git a/src/source-ipfw.c b/src/source-ipfw.c
index 40742afc55465d8d98f5645ea2cb441c8fa7a10e..6821d69456cdc78b71796bf2a4a7a3a4557873e2 100644 (file)
--- a/src/source-ipfw.c
+++ b/src/source-ipfw.c
@@ -623,30 +623,12 @@ TmEcode VerdictIPFW(ThreadVars *tv, Packet *p, void *data, PacketQueue *pq, Pack
      *  if this is a tunnel packet we check if we are ready to verdict
      * already. */
     if (IS_TUNNEL_PKT(p)) {
-        char verdict = 1;
-
-        SCMutex *m = p->root ? &p->root->tunnel_mutex : &p->tunnel_mutex;
-        SCMutexLock(m);
-
-        /* if there are more tunnel packets than ready to verdict packets,
-         * we won't verdict this one
-         */
-        if (TUNNEL_PKT_TPR(p) > TUNNEL_PKT_RTV(p)) {
-            SCLogDebug("VerdictIPFW: not ready to verdict yet: "
-                    "TUNNEL_PKT_TPR(p) > TUNNEL_PKT_RTV(p) = %" PRId32
-                    " > %" PRId32 "", TUNNEL_PKT_TPR(p), TUNNEL_PKT_RTV(p));
-            verdict = 0;
-        }
-
-        SCMutexUnlock(m);
+        bool verdict = VerdictTunnelPacket(p);
 
         /* don't verdict if we are not ready */
-        if (verdict == 1) {
+        if (verdict == true) {
             SCLogDebug("Setting verdict on tunnel");
             retval = IPFWSetVerdict(tv, ptv, p->root ? p->root : p);
-
-        } else {
-            TUNNEL_INCR_PKT_RTV(p);
         }
     } else {
         /* no tunnel, verdict normally */

diff --git a/src/source-nfq.c b/src/source-nfq.c
index 2a8a2884890da8515e0d56741a755a5a61c0ffc6..4c13f5a73726929abfa0013b3257b3d7b59d9041 100644 (file)
--- a/src/source-nfq.c
+++ b/src/source-nfq.c
@@ -943,7 +943,8 @@ static void NFQRecvPkt(NFQQueueVars *t, NFQThreadVars *tv)
         NFQMutexUnlock(t);
 
         if (ret != 0) {
-            SCLogWarning(SC_ERR_NFQ_HANDLE_PKT, "nfq_handle_packet error %" PRId32 "", ret);
+            SCLogWarning(SC_ERR_NFQ_HANDLE_PKT, "nfq_handle_packet error %"PRId32" %s",
+                    ret, strerror(errno));
         }
     }
 }
@@ -1145,37 +1146,21 @@ TmEcode VerdictNFQ(ThreadVars *tv, Packet *p, void *data, PacketQueue *pq, Packe
     /* if this is a tunnel packet we check if we are ready to verdict
      * already. */
     if (IS_TUNNEL_PKT(p)) {
-        char verdict = 1;
-        //printf("VerdictNFQ: tunnel pkt: %p %s\n", p, p->root ? "upper layer" : "root");
-
-        SCMutex *m = p->root ? &p->root->tunnel_mutex : &p->tunnel_mutex;
-        SCMutexLock(m);
-
-        /* if there are more tunnel packets than ready to verdict packets,
-         * we won't verdict this one */
-        if (TUNNEL_PKT_TPR(p) > TUNNEL_PKT_RTV(p)) {
-            SCLogDebug("not ready to verdict yet: TUNNEL_PKT_TPR(p) > "
-                    "TUNNEL_PKT_RTV(p) = %" PRId32 " > %" PRId32,
-                    TUNNEL_PKT_TPR(p), TUNNEL_PKT_RTV(p));
-            verdict = 0;
-        }
-
-        SCMutexUnlock(m);
-
+        SCLogDebug("tunnel pkt: %p/%p %s", p, p->root, p->root ? "upper layer" : "root");
+        bool verdict = VerdictTunnelPacket(p);
         /* don't verdict if we are not ready */
-        if (verdict == 1) {
-            //printf("VerdictNFQ: setting verdict\n");
+        if (verdict == true) {
             ret = NFQSetVerdict(p->root ? p->root : p);
-            if (ret != TM_ECODE_OK)
+            if (ret != TM_ECODE_OK) {
                 return ret;
-        } else {
-            TUNNEL_INCR_PKT_RTV(p);
+            }
         }
     } else {
         /* no tunnel, verdict normally */
         ret = NFQSetVerdict(p);
-        if (ret != TM_ECODE_OK)
+        if (ret != TM_ECODE_OK) {
             return ret;
+        }
     }
     return TM_ECODE_OK;
 }

diff --git a/src/stream-tcp.c b/src/stream-tcp.c
index c62e539f751962f25073df97d06ceb6cb46c5285..4c2dc44e7f719487d09d4505b6307d237489ed2d 100644 (file)
--- a/src/stream-tcp.c
+++ b/src/stream-tcp.c
@@ -5844,6 +5844,7 @@ static void StreamTcpPseudoPacketCreateDetectLogFlush(ThreadVars *tv,
 #endif
     }
 
+    SCLogDebug("np %p", np);
     PacketEnqueue(pq, np);
 
     StatsIncr(tv, stt->counter_tcp_pseudo);

diff --git a/src/tmqh-packetpool.c b/src/tmqh-packetpool.c
index 913cb3e78c937c588562b21da832ace9ea975200..a94ce20c4e194fd4feaf90fe52d6ce7ac97c1cab 100644 (file)
--- a/src/tmqh-packetpool.c
+++ b/src/tmqh-packetpool.c
@@ -443,7 +443,7 @@ Packet *TmqhInputPacketpool(ThreadVars *tv)
 
 void TmqhOutputPacketpool(ThreadVars *t, Packet *p)
 {
-    int proot = 0;
+    bool proot = false;
 
     SCEnter();
     SCLogDebug("Packet %p, p->root %p, alloced %s", p, p->root, p->flags & PKT_ALLOC ? "true" : "false");
@@ -458,17 +458,19 @@ void TmqhOutputPacketpool(ThreadVars *t, Packet *p)
 
         if (IS_TUNNEL_ROOT_PKT(p)) {
             SCLogDebug("IS_TUNNEL_ROOT_PKT == TRUE");
-            if (TUNNEL_PKT_TPR(p) == 0) {
-                SCLogDebug("TUNNEL_PKT_TPR(p) == 0, no more tunnel packet "
-                        "depending on this root");
+            const uint16_t outstanding = TUNNEL_PKT_TPR(p) - TUNNEL_PKT_RTV(p);
+            SCLogDebug("root pkt: outstanding %u", outstanding);
+            if (outstanding == 0) {
+                SCLogDebug("no tunnel packets outstanding, no more tunnel "
+                        "packet(s) depending on this root");
                 /* if this packet is the root and there are no
-                 * more tunnel packets, return it to the pool */
-
-                /* fall through */
+                 * more tunnel packets to consider
+                 *
+                 * return it to the pool */
             } else {
-                SCLogDebug("tunnel root Packet %p: TUNNEL_PKT_TPR(p) > 0, so "
+                SCLogDebug("tunnel root Packet %p: outstanding > 0, so "
                         "packets are still depending on this root, setting "
-                        "p->tunnel_verdicted == 1", p);
+                        "SET_TUNNEL_PKT_VERDICTED", p);
                 /* if this is the root and there are more tunnel
                  * packets, return this to the pool. It's still referenced
                  * by the tunnel packets, and we will return it
@@ -482,34 +484,34 @@ void TmqhOutputPacketpool(ThreadVars *t, Packet *p)
         } else {
             SCLogDebug("NOT IS_TUNNEL_ROOT_PKT, so tunnel pkt");
 
-            /* the p->root != NULL here seems unnecessary: IS_TUNNEL_PKT checks
-             * that p->tunnel_pkt == 1, IS_TUNNEL_ROOT_PKT checks that +
-             * p->root == NULL. So when we are here p->root can only be
-             * non-NULL, right? CLANG thinks differently. May be a FP, but
-             * better safe than sorry. VJ */
-            if (p->root != NULL && IS_TUNNEL_PKT_VERDICTED(p->root) &&
-                    TUNNEL_PKT_TPR(p) == 1)
+            TUNNEL_INCR_PKT_RTV_NOLOCK(p);
+            const uint16_t outstanding = TUNNEL_PKT_TPR(p) - TUNNEL_PKT_RTV(p);
+            SCLogDebug("tunnel pkt: outstanding %u", outstanding);
+            /* all tunnel packets are processed except us. Root already
+             * processed. So return tunnel pkt and root packet to the
+             * pool. */
+            if (outstanding == 0 &&
+                    p->root && IS_TUNNEL_PKT_VERDICTED(p->root))
             {
-                SCLogDebug("p->root->tunnel_verdicted == 1 && TUNNEL_PKT_TPR(p) == 1");
-                /* the root is ready and we are the last tunnel packet,
-                 * lets enqueue them both. */
-                TUNNEL_DECR_PKT_TPR_NOLOCK(p);
+                SCLogDebug("root verdicted == true && no outstanding");
 
-                /* handle the root */
+                /* handle freeing the root as well*/
                 SCLogDebug("setting proot = 1 for root pkt, p->root %p "
                         "(tunnel packet %p)", p->root, p);
-                proot = 1;
+                proot = true;
 
                 /* fall through */
-            } else {
-                /* root not ready yet, so get rid of the tunnel pkt only */
 
-                SCLogDebug("NOT p->root->tunnel_verdicted == 1 && "
-                        "TUNNEL_PKT_TPR(p) == 1 (%" PRIu32 ")", TUNNEL_PKT_TPR(p));
+            } else {
+                /* root not ready yet, or not the last tunnel packet,
+                 * so get rid of the tunnel pkt only */
 
-                TUNNEL_DECR_PKT_TPR_NOLOCK(p);
+                SCLogDebug("NOT IS_TUNNEL_PKT_VERDICTED (%s) || "
+                        "outstanding > 0 (%u)",
+                        (p->root && IS_TUNNEL_PKT_VERDICTED(p->root)) ? "true" : "false",
+                        outstanding);
 
-                 /* fall through */
+                /* fall through */
             }
         }
         SCMutexUnlock(m);
@@ -520,7 +522,7 @@ void TmqhOutputPacketpool(ThreadVars *t, Packet *p)
     FlowDeReference(&p->flow);
 
     /* we're done with the tunnel root now as well */
-    if (proot == 1) {
+    if (proot == true) {
         SCLogDebug("getting rid of root pkt... alloc'd %s", p->root->flags & PKT_ALLOC ? "true" : "false");
 
         FlowDeReference(&p->root->flow);