Thresholds/v20 #11358

victorjulien · 2024-06-24T11:11:56Z

SV_BRANCH=OISF/suricata-verify#1938

Add support for 'by_flow' track option. This allows using the various threshold options in the context of a single flow. Example: alert tcp ... stream-event:pkt_broken_ack; \ threshold:type limit, track by_flow, count 1, seconds 3600; The example would limit the number of alerts to once per hour for packets triggering the 'pkt_broken_ack' stream event. Implemented as a special "flowvar" holding the threshold entries. This means no synchronization is required, making this a cheaper option compared to the other trackers. Ticket: OISF#6822.

Allow rate_filter and thresholds from the global config to specify tracking "by_flow".

Traffic variables (flowvars, flowbits, xbits, etc) use a smaller int for their type than detection types. As a workaround make sure the values fit in a uint8_t.

Limits propegation checked for DETECT_DEPTH as a content flag, which appears to have worked by chance. After reshuffling the keyword id's it no longer worked. This patch uses the proper flag DETECT_CONTENT_DEPTH.

Thresholding often has 2 stages: 1. recording matches 2. appling an action, like suppress E.g. with something like: threshold:type limit, count 10, seconds 3600, track by_src; the recording state is about counting 10 first hits for an IP, then followed by the "suppress" state that might last an hour. By_src/by_dst are expensive, as they do a host table lookup and lock the host. If many threads require this access, lock contention becomes a serious problem. This patch adds a thread local cache to avoid the synchronization overhead. When the threshold for a host enters the "apply" stage, a thread local hash entry is added. This entry knows the expiry time and the action to apply. This way the action can be applied w/o the synchronization overhead. A rbtree is used to handle expiration.

Packet pointer is not used during allocation.

Add a callback and helper function to handle data expiration. Update datasets to explicitly not use expiration.

Instead of a Host and IPPair table thresholding layer, use a dedicated THash to store both. This allows hashing on host+sid+tracker or ippair+sid+tracker, to create more unique hash keys. This allows for fewer hash collisions. The per rule tracking also uses this, so that the single big lock is no longer a single point of contention. Reimplement storage for flow thresholds to reuse as much logic as possible from the host/ippair/rule thresholds. Ticket: OISF#426.

Ticket: OISF#6967.

Use the same hash key as for the regular threshold storage, so include gid, rev, tentant id.

suricata-qa · 2024-06-24T13:35:02Z

Information: QA ran without warnings.

Pipeline 21192

jasonish · 2024-06-25T15:22:37Z

src/detect-engine-threshold.c

+    if (track == TRACK_SRC) {
+        addr = p->src.addr_data32[0];
+    } else if (track == TRACK_DST) {
+        addr = p->dst.addr_data32[0];
+    } else {
+        return -1;
+    }
+


No, implementation is IPv4 only right now.

Ticket required? Or implement before merge?

Why? It's an optimization, nothing is required.

Moreso for completeness. It is not discussed in any of the commits, or the tickets referenced by this PR that this is an IPv4 only optimization, so the lack of IPv6 looks like an oversight.

call sites all use PacketIsIPv4

At least to me that means where is the IPv6 :)

you can create a ticket if you want

inashivb · 2024-06-26T11:22:48Z

src/detect-engine-threshold.c

+{
+    if (SCTIME_CMP_GTE(a->expires_at, b->expires_at)) {
+        return 1;
+    } else {


nit: unneeded else

inashivb · 2024-06-26T11:31:19Z

src/detect-engine-threshold.c

+            SCFree(n);
+        }
+        return -1;
+    } else {


nit: unneeded else

inashivb · 2024-06-26T11:32:48Z

src/detect-engine-threshold.c

+}
+
+/** \brief Check Thread local thresholding cache
+ *  \retval negative value cache miss (-1 miss, -2 expired)


nit: retval values missing

inashivb · 2024-06-26T11:34:31Z

src/detect-engine-threshold.c

+        .retval = retval,
+        .addr = addr,
+        .sid = sid,
+        .expires_at = expires,


Q: Do we need expires_at and retval here as they're not used for lookup?

inashivb · 2024-06-26T11:35:58Z

src/detect-engine-threshold.c

+    };
+    ThresholdCacheItem *found = HashTableLookup(threshold_cache_ht, &lookup, 0);
+    if (found) {
+        if (SCTIME_CMP_GT(p->ts, found->expires_at)) {


Q: what if it has just timed out? So the = condition

inashivb · 2024-06-26T11:36:28Z

src/detect-engine-threshold.c

+            THRESHOLD_CACHE_RB_REMOVE(&threshold_cache_tree, found);
+            HashTableRemove(threshold_cache_ht, found, 0);
+            cache_lookup_miss_expired++;
+            return -2; // CACHE MISS


nit: also mention expired?

inashivb · 2024-06-26T11:48:56Z

src/detect-engine-threshold.c

+typedef struct ThresholdEntry_ {
+    uint32_t sid; /**< Signature id */
+    uint32_t gid; /**< Signature group id */
+    int track;    /**< Track type: by_src, by_src */


nit: typo by_src

inashivb

Makes sense to me, mostly. Will take another look when it is staged.
Nice thoughts around optimizations!

inashivb · 2024-06-26T11:55:43Z

src/detect-engine-threshold.c

-                    /* expired, so reset */
-                    lookup_tsh->tv1 = p->ts;
-                    lookup_tsh->current_count = 1;
+            if (SCTIME_CMP_LTE(p->ts, entry)) {


ok. so = is considered within the time limit.

victorjulien · 2024-06-26T12:19:49Z

Makes sense to me, mostly. Will take another look when it is staged. Nice thoughts around optimizations!

Please do the full review at this stage, not when things are staged into a next branch.

inashivb · 2024-06-26T12:27:18Z

Please do the full review at this stage, not when things are staged into a next branch.

ok. Will take another look tomorrow. Thank you. This is big work.

jasonish · 2024-06-26T22:27:18Z

src/detect-engine-threshold.c

+        .key[TC_GID] = gid,
+        .key[TC_REV] = rev,
+        .key[TC_TENANT] = p->tenant_id,
+    };


Should we be vlan aware here? And elsewhere with respect to thresholding?

I've created https://redmine.openinfosecfoundation.org/issues/7125 for this

It's an issue in existing code. I do not intend to address it in this work.

src/detect-engine-threshold.c

victorjulien · 2024-06-28T08:01:14Z

comments addressed in #11388

victorjulien added 25 commits June 24, 2024 10:04

util/var: remove printf; add assert

966dd63

util/var: add comments explaining types

feda2d7

threshold: add by_flow support for global thresholds

f15cf52

Allow rate_filter and thresholds from the global config to specify tracking "by_flow".

detect: group types used in traffic variables

deb16f9

Traffic variables (flowvars, flowbits, xbits, etc) use a smaller int for their type than detection types. As a workaround make sure the values fit in a uint8_t.

detect: group content inspect keyword id's

b0944c6

detect/content: fix wrong value for depth check

fc7f1e9

Limits propegation checked for DETECT_DEPTH as a content flag, which appears to have worked by chance. After reshuffling the keyword id's it no longer worked. This patch uses the proper flag DETECT_CONTENT_DEPTH.

doc: add thresholding by_flow

3567f9e

detect/detection_filter: add support for track by_flow

06ff741

detect: add ticket id to var related todos

783d735

detect/threshold: minor cleanup

75ea5fc

detect/threshold: minor code cleanup

da3439f

Packet pointer is not used during allocation.

detect/threshold: minor rate filter cleanup

6d17006

detect/address: constify ipv6 cmp funcs

21a4cfc

thash: add expiration logic

58a116c

Add a callback and helper function to handle data expiration. Update datasets to explicitly not use expiration.

range: use thash expiry API for timeout

f83901c

detect/threshold: improve hash function

1432add

detect/threshold: include rev in threshold tracking

cccf858

detect/threshold: consider tenant id in tracking

44dcc92

Ticket: OISF#6967.

detect/threshold: expand cache support for rule tracking

11cf1a9

Use the same hash key as for the regular threshold storage, so include gid, rev, tentant id.

detect/threshold: includes cleanup

470a37c

detect/threshold: make hash size and memcap configurable

1fb11d0

doc/userguide: document new threshold config options

dac619d

victorjulien requested review from jufajardini and a team as code owners June 24, 2024 11:11

This was referenced Jun 24, 2024

Thresholds/v17 #11324

Closed

Thresholds/v20 backoff/v1 #11359

Closed

jasonish reviewed Jun 25, 2024

View reviewed changes

inashivb reviewed Jun 26, 2024

View reviewed changes

jasonish reviewed Jun 26, 2024

View reviewed changes

src/detect-engine-threshold.c Show resolved Hide resolved

victorjulien mentioned this pull request Jun 28, 2024

Thresholds/v20 backoff/v2 #11388

Open

victorjulien closed this Jun 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Thresholds/v20 #11358

Thresholds/v20 #11358

victorjulien commented Jun 24, 2024

suricata-qa commented Jun 24, 2024

jasonish Jun 25, 2024

victorjulien Jun 25, 2024

jasonish Jun 25, 2024

victorjulien Jun 25, 2024

jasonish Jun 25, 2024

victorjulien Jun 25, 2024

jasonish Jun 25, 2024

victorjulien Jun 25, 2024

inashivb Jun 26, 2024

inashivb Jun 26, 2024

inashivb Jun 26, 2024

inashivb Jun 26, 2024

inashivb Jun 26, 2024

inashivb Jun 26, 2024

inashivb Jun 26, 2024

inashivb left a comment

inashivb Jun 26, 2024

victorjulien commented Jun 26, 2024 •

edited by inashivb

Loading

inashivb commented Jun 26, 2024

jasonish Jun 26, 2024 •

edited

Loading

victorjulien Jun 28, 2024

victorjulien commented Jun 28, 2024

Thresholds/v20 #11358

Thresholds/v20 #11358

Conversation

victorjulien commented Jun 24, 2024

suricata-qa commented Jun 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inashivb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victorjulien commented Jun 26, 2024 • edited by inashivb Loading

inashivb commented Jun 26, 2024

jasonish Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victorjulien commented Jun 28, 2024

victorjulien commented Jun 26, 2024 •

edited by inashivb

Loading

jasonish Jun 26, 2024 •

edited

Loading