nfq: stricter thread sync

No longer update `Packet::flags` for tracking packet modifications,
as thread safety was not guaranteed.

Clearly separate between various kinds of `Packet::nfq_v` accesses for:
- mark
- mark_modified
- verdicted
These are either done under lock (Packet::persistent.tunnel_lock) or,
if the Packet is not part of a tunnel, not under lock.

This is safe as in all the related logic the Packet's tunnel state
is fixed and can no longer change.

diff --git a/src/decode.h b/src/decode.h
index 1388e24efb24cc090a70db87d1d0f6640d914066..ea73bb2837f0ad90473ec20ca09913488cbdb0a3 100644 (file)
--- a/src/decode.h
+++ b/src/decode.h
@@ -1021,8 +1021,9 @@ void DecodeUnregisterCounters(void);
 /** Packet is modified by the stream engine, we need to recalc the csum and       \
                    reinject/replace */
 #define PKT_STREAM_MODIFIED BIT_U32(10)
-/** Packet mark is modified */
-#define PKT_MARK_MODIFIED BIT_U32(11)
+
+// vacancy
+
 /** Exclude packet from pcap logging as it's part of a stream that has reassembly \
                    depth reached. */
 #define PKT_STREAM_NOPCAPLOG BIT_U32(12)

diff --git a/src/detect-mark.c b/src/detect-mark.c
index 9c5b9f46ea8ef582e0ada336560cec0529d3b1b2..cccdefe8fd5de8bcd5a7598538be63b8440bb61d 100644 (file)
--- a/src/detect-mark.c
+++ b/src/detect-mark.c
@@ -229,10 +229,15 @@ static int DetectMarkPacket(DetectEngineThreadCtx *det_ctx, Packet *p,
     const DetectMarkData *nf_data = (const DetectMarkData *)ctx;
     if (nf_data->mask) {
         if (PacketIsNotTunnel(p)) {
+            /* for a non-tunnel packet we don't need a lock,
+             * and if we're here we can't turn into a tunnel
+             * packet anymore. */
+
             /* coverity[missing_lock] */
             p->nfq_v.mark = (nf_data->mark & nf_data->mask)
                 | (p->nfq_v.mark & ~(nf_data->mask));
-            p->flags |= PKT_MARK_MODIFIED;
+            /* coverity[missing_lock] */
+            p->nfq_v.mark_modified = true;
         } else {
             /* real tunnels may have multiple flows inside them, so marking
              * might 'mark' too much. Rebuilt packets from IP fragments
@@ -242,7 +247,7 @@ static int DetectMarkPacket(DetectEngineThreadCtx *det_ctx, Packet *p,
                 SCSpinLock(&tp->persistent.tunnel_lock);
                 tp->nfq_v.mark = (nf_data->mark & nf_data->mask)
                     | (tp->nfq_v.mark & ~(nf_data->mask));
-                tp->flags |= PKT_MARK_MODIFIED;
+                tp->nfq_v.mark_modified = true;
                 SCSpinUnlock(&tp->persistent.tunnel_lock);
             }
         }

diff --git a/src/source-nfq.c b/src/source-nfq.c
index a64f02632bc78c29fa9a31fb33016f90545622a1..4e85336e42d6c4ae2316c70f608203c703faa812 100644 (file)
--- a/src/source-nfq.c
+++ b/src/source-nfq.c
@@ -142,7 +142,7 @@ static TmEcode DecodeNFQ(ThreadVars *, Packet *, void *);
 static TmEcode DecodeNFQThreadInit(ThreadVars *, const void *, void **);
 static TmEcode DecodeNFQThreadDeinit(ThreadVars *tv, void *data);
 
-static TmEcode NFQSetVerdict(Packet *p);
+static TmEcode NFQSetVerdict(Packet *p, const uint32_t mark_value, const bool mark_modified);
 static void NFQReleasePacket(Packet *p);
 
 typedef enum NFQMode_ {
@@ -324,7 +324,8 @@ static void NFQVerdictCacheFlush(NFQQueueVars *t)
 #endif
 }
 
-static int NFQVerdictCacheAdd(NFQQueueVars *t, Packet *p, uint32_t verdict)
+static int NFQVerdictCacheAdd(NFQQueueVars *t, Packet *p, const uint32_t verdict,
+        const uint32_t mark_value, const bool mark_modified)
 {
 #ifdef HAVE_NFQ_SET_VERDICT_BATCH
     if (t->verdict_cache.maxlen == 0)
@@ -333,13 +334,13 @@ static int NFQVerdictCacheAdd(NFQQueueVars *t, Packet *p, uint32_t verdict)
     if (p->flags & PKT_STREAM_MODIFIED || verdict == NF_DROP)
         goto flush;
 
-    if (p->flags & PKT_MARK_MODIFIED) {
+    if (mark_modified) {
         if (!t->verdict_cache.mark_valid) {
             if (t->verdict_cache.len)
                 goto flush;
             t->verdict_cache.mark_valid = 1;
-            t->verdict_cache.mark = p->nfq_v.mark;
-        } else if (t->verdict_cache.mark != p->nfq_v.mark) {
+            t->verdict_cache.mark = mark_value;
+        } else if (t->verdict_cache.mark != mark_value) {
             goto flush;
         }
     } else if (t->verdict_cache.mark_valid) {
@@ -439,6 +440,7 @@ static int NFQSetupPkt (Packet *p, struct nfq_q_handle *qh, void *data)
     p->ReleasePacket = NFQReleasePacket;
     p->nfq_v.ifi  = nfq_get_indev(tb);
     p->nfq_v.ifo  = nfq_get_outdev(tb);
+    /* coverity[missing_lock] */
     p->nfq_v.verdicted = false;
 
 #ifdef NFQ_GET_PAYLOAD_SIGNED
@@ -481,9 +483,28 @@ static int NFQSetupPkt (Packet *p, struct nfq_q_handle *qh, void *data)
 
 static void NFQReleasePacket(Packet *p)
 {
-    if (unlikely(!p->nfq_v.verdicted)) {
-        PacketDrop(p, ACTION_DROP, PKT_DROP_REASON_NFQ_ERROR);
-        NFQSetVerdict(p);
+    if (PacketIsNotTunnel(p)) {
+        if (unlikely(!p->nfq_v.verdicted)) {
+            PacketDrop(p, ACTION_DROP, PKT_DROP_REASON_NFQ_ERROR);
+            /* coverity[missing_lock] */
+            NFQSetVerdict(p, p->nfq_v.mark, p->nfq_v.mark_modified);
+            /* coverity[missing_lock] */
+            p->nfq_v.verdicted = true;
+        }
+    } else {
+        Packet *root_p = p->root ? p->root : p;
+        SCSpinlock *lock = &root_p->persistent.tunnel_lock;
+        SCSpinLock(lock);
+        const bool verdicted = p->nfq_v.verdicted;
+        // taken from root packet
+        const bool mark_modified = root_p->nfq_v.mark_modified;
+        const uint32_t mark_value = root_p->nfq_v.mark;
+        root_p->nfq_v.verdicted = true;
+        SCSpinUnlock(lock);
+        if (!verdicted) {
+            PacketDrop(p, ACTION_DROP, PKT_DROP_REASON_NFQ_ERROR);
+            NFQSetVerdict(p, mark_value, mark_modified);
+        }
     }
     PacketFreeOrRelease(p);
 }
@@ -504,7 +525,7 @@ static int NFQBypassCallback(Packet *p)
             SCSpinLock(&tp->persistent.tunnel_lock);
             tp->nfq_v.mark = (nfq_config.bypass_mark & nfq_config.bypass_mask)
                 | (tp->nfq_v.mark & ~nfq_config.bypass_mask);
-            tp->flags |= PKT_MARK_MODIFIED;
+            tp->nfq_v.mark_modified = true;
             SCSpinUnlock(&tp->persistent.tunnel_lock);
             return 1;
         }
@@ -513,7 +534,8 @@ static int NFQBypassCallback(Packet *p)
         /* coverity[missing_lock] */
         p->nfq_v.mark = (nfq_config.bypass_mark & nfq_config.bypass_mask)
                         | (p->nfq_v.mark & ~nfq_config.bypass_mask);
-        p->flags |= PKT_MARK_MODIFIED;
+        /* coverity[missing_lock] */
+        p->nfq_v.mark_modified = true;
     }
 
     return 1;
@@ -1073,15 +1095,14 @@ static inline void UpdateCounters(NFQQueueVars *t, const Packet *p)
 
 /** \internal
  *  \brief NFQ verdict function
+ *  \param p Packet to work with. Will be the tunnel root packet in case of tunnel.
  */
-static TmEcode NFQSetVerdict(Packet *p)
+static TmEcode NFQSetVerdict(Packet *p, const uint32_t mark_value, const bool mark_modified)
 {
     int iter = 0;
     /* we could also have a direct pointer but we need to have a ref count in this case */
     NFQQueueVars *t = g_nfq_q + p->nfq_v.nfq_index;
 
-    p->nfq_v.verdicted = true;
-
     /* can't verdict a "fake" packet */
     if (PKT_IS_PSEUDOPKT(p)) {
         return TM_ECODE_OK;
@@ -1101,7 +1122,7 @@ static TmEcode NFQSetVerdict(Packet *p)
     UpdateCounters(t, p);
 #endif /* COUNTERS */
 
-    int ret = NFQVerdictCacheAdd(t, p, verdict);
+    int ret = NFQVerdictCacheAdd(t, p, verdict, mark_value, mark_modified);
     if (ret == 0) {
         NFQMutexUnlock(t);
         return TM_ECODE_OK;
@@ -1112,26 +1133,21 @@ static TmEcode NFQSetVerdict(Packet *p)
             default:
             case NFQ_ACCEPT_MODE:
             case NFQ_ROUTE_MODE:
-                if (p->flags & PKT_MARK_MODIFIED) {
+                if (mark_modified) {
 #ifdef HAVE_NFQ_SET_VERDICT2
                     if (p->flags & PKT_STREAM_MODIFIED) {
-                        ret = nfq_set_verdict2(t->qh, p->nfq_v.id, verdict,
-                                p->nfq_v.mark,
+                        ret = nfq_set_verdict2(t->qh, p->nfq_v.id, verdict, mark_value,
                                 GET_PKT_LEN(p), GET_PKT_DATA(p));
                     } else {
-                        ret = nfq_set_verdict2(t->qh, p->nfq_v.id, verdict,
-                                p->nfq_v.mark,
-                                0, NULL);
+                        ret = nfq_set_verdict2(t->qh, p->nfq_v.id, verdict, mark_value, 0, NULL);
                     }
 #else /* fall back to old function */
                     if (p->flags & PKT_STREAM_MODIFIED) {
-                        ret = nfq_set_verdict_mark(t->qh, p->nfq_v.id, verdict,
-                                htonl(p->nfq_v.mark),
+                        ret = nfq_set_verdict_mark(t->qh, p->nfq_v.id, verdict, htonl(mark_value),
                                 GET_PKT_LEN(p), GET_PKT_DATA(p));
                     } else {
-                        ret = nfq_set_verdict_mark(t->qh, p->nfq_v.id, verdict,
-                                htonl(p->nfq_v.mark),
-                                0, NULL);
+                        ret = nfq_set_verdict_mark(
+                                t->qh, p->nfq_v.id, verdict, htonl(mark_value), 0, NULL);
                     }
 #endif /* HAVE_NFQ_SET_VERDICT2 */
                 } else {
@@ -1141,28 +1157,29 @@ static TmEcode NFQSetVerdict(Packet *p)
                     } else {
                         ret = nfq_set_verdict(t->qh, p->nfq_v.id, verdict, 0, NULL);
                     }
-
                 }
                 break;
             case NFQ_REPEAT_MODE:
 #ifdef HAVE_NFQ_SET_VERDICT2
                 if (p->flags & PKT_STREAM_MODIFIED) {
                     ret = nfq_set_verdict2(t->qh, p->nfq_v.id, verdict,
-                            (nfq_config.mark & nfq_config.mask) | (p->nfq_v.mark & ~nfq_config.mask),
+                            (nfq_config.mark & nfq_config.mask) | (mark_value & ~nfq_config.mask),
                             GET_PKT_LEN(p), GET_PKT_DATA(p));
                 } else {
                     ret = nfq_set_verdict2(t->qh, p->nfq_v.id, verdict,
-                            (nfq_config.mark & nfq_config.mask) | (p->nfq_v.mark & ~nfq_config.mask),
+                            (nfq_config.mark & nfq_config.mask) | (mark_value & ~nfq_config.mask),
                             0, NULL);
                 }
 #else /* fall back to old function */
                 if (p->flags & PKT_STREAM_MODIFIED) {
                     ret = nfq_set_verdict_mark(t->qh, p->nfq_v.id, verdict,
-                            htonl((nfq_config.mark & nfq_config.mask) | (p->nfq_v.mark & ~nfq_config.mask)),
+                            htonl((nfq_config.mark & nfq_config.mask) |
+                                    (mark_value & ~nfq_config.mask)),
                             GET_PKT_LEN(p), GET_PKT_DATA(p));
                 } else {
                     ret = nfq_set_verdict_mark(t->qh, p->nfq_v.id, verdict,
-                            htonl((nfq_config.mark & nfq_config.mask) | (p->nfq_v.mark & ~nfq_config.mask)),
+                            htonl((nfq_config.mark & nfq_config.mask) |
+                                    (mark_value & ~nfq_config.mask)),
                             0, NULL);
                 }
 #endif /* HAVE_NFQ_SET_VERDICT2 */
@@ -1186,19 +1203,33 @@ TmEcode VerdictNFQ(ThreadVars *tv, Packet *p, void *data)
 {
     /* if this is a tunnel packet we check if we are ready to verdict
      * already. */
-    if (p->ttype != PacketTunnelNone) {
+    if (PacketIsTunnel(p)) {
+        Packet *root_p = p->root ? p->root : p;
+
         SCLogDebug("tunnel pkt: %p/%p %s", p, p->root, p->root ? "upper layer" : "root");
-        bool verdict = VerdictTunnelPacket(p);
+
+        SCSpinlock *lock = &root_p->persistent.tunnel_lock;
+        SCSpinLock(lock);
+        const bool do_verdict = VerdictTunnelPacketInternal(p);
+        // taken from root packet
+        const bool mark_modified = root_p->nfq_v.mark_modified;
+        const uint32_t mark_value = root_p->nfq_v.mark;
+        root_p->nfq_v.verdicted = do_verdict;
+        SCSpinUnlock(lock);
         /* don't verdict if we are not ready */
-        if (verdict == true) {
-            int ret = NFQSetVerdict(p->root ? p->root : p);
+        if (do_verdict == true) {
+            int ret = NFQSetVerdict(root_p, mark_value, mark_modified);
             if (ret != TM_ECODE_OK) {
                 return ret;
             }
         }
     } else {
         /* no tunnel, verdict normally */
-        int ret = NFQSetVerdict(p);
+
+        /* coverity[missing_lock] */
+        p->nfq_v.verdicted = true;
+
+        int ret = NFQSetVerdict(p, p->nfq_v.mark, p->nfq_v.mark_modified);
         if (ret != TM_ECODE_OK) {
             return ret;
         }

diff --git a/src/source-nfq.h b/src/source-nfq.h
index c7dbd58fd08dc05dc26379c580af6ca1e0cf2bd5..d3d0781bfdbef830eead11f9250ba49e73781f6d 100644 (file)
--- a/src/source-nfq.h
+++ b/src/source-nfq.h
@@ -41,6 +41,7 @@ typedef struct NFQPacketVars_
     int id; /* this nfq packets id */
     uint16_t nfq_index; /* index in NFQ array */
     bool verdicted;
+    bool mark_modified;
 
     uint32_t mark;
     uint32_t ifi;