Alert queue optimizations - v4 #7314

jufajardini · 2022-04-26T19:34:41Z

Previous PR: #7297

Link to redmine ticket: https://redmine.openinfosecfoundation.org/issues/4943
https://redmine.openinfosecfoundation.org/issues/5179
https://redmine.openinfosecfoundation.org/issues/4207

Describe changes:

extracted WARN_ONCE macro to util-debug
move AlertQueue functions to detect-engine-alert
incorporate feedback from previous PR
(tried to) handle AlertQueueExpand failure more gracefully (this includes trying again with a smaller size, warning only once per thread AND incrementing discarded alerts stats, if it really fails)
pass on tx_id field value if frame has it

Left this as a draft because I'm not sure the solutions here are good AND because the handle alert queue expand failure commit should be squashed, if good enough

suricata-verify-pr: 808
OISF/suricata-verify#808

The maximum of possible alerts triggered by a unique packet was hardcoded to 15. With usage of 'noalert' rules, that limit could be reached somewhat easily. Make that configurable via suricata.yaml. Conf Bug#4941 Task OISF#4207

Initial work to bring part of the alert queue processing to DetectEngineThreadCtx. Task OISF#4943

Task OISF#4943

Do all alert queue processing before actually appending the PacketAlerts to the Packet's alert queue. Task OISF#4943

Since we now only copy the PacketAlerts to the Packet's queue after processing them, we no longer do packet alert appending from detect-engine-alert, nor do we remove PacketAlerts from the queue (if they're discarded by overflow or thresholding, they're not copied to the final alert queue). Task OISF#4943

Add a counter to our stats log with the total of alerts that have been discarded due to packet alert queue overflow. Task OISF#5179

Related to Task OISF#4943 Task OISF#5179

Since more than one file needed a similar function, make it available from util/debug instead of having it as a local macro. Task OISF#4943

In the unlikely case of an alert queue expansion reallocation failure, we want to make sure that we don't just ignore it. But we also don't want to break things or flood Suri with warnings. Task OISF#4943

Task OISF#4943

codecov · 2022-04-26T20:23:15Z

Codecov Report

Merging #7314 (704e85c) into master (ddf9c9d) will decrease coverage by 0.03%.
The diff coverage is 85.57%.

@@            Coverage Diff             @@
##           master    #7314      +/-   ##
==========================================
- Coverage   75.82%   75.78%   -0.04%     
==========================================
  Files         656      656              
  Lines      190051   190099      +48     
==========================================
- Hits       144102   144069      -33     
- Misses      45949    46030      +81

Flag	Coverage Δ
fuzzcorpus	`60.30% <73.26%> (-0.12%)`	⬇️
suricata-verify	`51.56% <85.26%> (+0.02%)`	⬆️
unittests	`61.01% <65.95%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

suricata-qa · 2022-04-27T06:25:03Z

ERROR:

ERROR: QA failed on tlpw1_files_sha256.

Pipeline 7189

victorjulien · 2022-04-27T08:48:58Z

src/detect-engine-alert.c

+    uint16_t new_cap = det_ctx->alert_queue_capacity * 2;
+    void *tmp_queue = SCRealloc(det_ctx->alert_queue, (size_t)(sizeof(PacketAlert) * new_cap));
+    if (unlikely(tmp_queue == NULL)) {
+        /* if first realloc fail, try to grow just 50% */


I think this is a bit too much. Lets just handle the initial failure and not try to be smart, which ultimately leads to more complexity and more untested error code.

victorjulien · 2022-04-27T08:49:35Z

src/detect-engine-alert.c

+    }
+    det_ctx->alert_queue = tmp_queue;
+    det_ctx->alert_queue_capacity = new_cap;
+    SCLogNotice("Alert queue size doubled: %u elements, bytes: %" PRIuMAX "",


victorjulien · 2022-04-27T08:50:50Z

suricata.yaml.in

@@ -1180,6 +1180,10 @@ legacy:
 #   - reject
 #   - alert

+# Define maximum number of possible alerts that can be triggered for the same
+# packet. Default is 15
+packet-alert-max: 15


we can leave commented out

victorjulien · 2022-04-27T08:51:50Z

src/detect-engine-alert.c

@@ -34,6 +34,13 @@ static Signature g_tag_signature;
 /** tag packet alert structure for tag alerts */
 static PacketAlert g_tag_pa;

+/* For WARN_ONCE, a record of warnings that have already been
+ * issued. */
+static thread_local bool once_err[SC_ERR_MAX];


@jlucovsky any thoughts on how this works wrt multiple static declarations with the same name in different modules?

I changed the name, as I wasn't sure if that could cause issues or not.

@jlucovsky any thoughts on how this works wrt multiple static declarations with the same name in different modules?

Because it's static, the scope should be local and treated like any other static with the same name.

A quick experiment confirms that the scope is restricted to the current source module.

However, this add a per-thread array of size SC_ERR_MAX to detect-engine-alert.c but it's only use with a single error code SC_ERR_MEM_ALLOC. If memory is sufficiently low that a small alloc will fail we'll get a flurry of error messages (one from each thread) and then (it's likely) Suricata will fail elsewhere.

Maybe the realloc logic isn't needed or a FatalError should occur or perhaps, now that errors are handled, simply return the old capacity on realloc failures.

From your analysis, I would prefer then simply return the old capacity.

jlucovsky · 2022-04-27T14:14:42Z

src/detect-engine-alert.c

+    det_ctx->alert_queue_size = 0;
+    det_ctx->alert_queue = SCCalloc(packet_alert_max, sizeof(PacketAlert));
+    if (det_ctx->alert_queue == NULL) {
+        FatalError(SC_ERR_MEM_ALLOC, "failed to allocate %" PRIu64 " bytes",


Suggest adding mention of the alert queue in the failure message.

jlucovsky · 2022-04-27T14:19:28Z

src/detect-engine-alert.c

+{
+    const PacketAlert *pa0 = a;
+    const PacketAlert *pa1 = b;
+    if (pa1->num == pa0->num)


Related -- can you update the comment in decode.h for the struct PacketAlert to reflect what's actually sorted? There, the comments state that only num an action are used for sorting. I think tx_id needs a similar comment.

jlucovsky · 2022-04-27T14:22:26Z

src/detect-engine-alert.c

+            /* we will not copy this to the AlertQueue */
+            p->alerts.suppressed++;
+        } else if (p->alerts.cnt < packet_alert_max) {
+            memcpy(&p->alerts.alerts[p->alerts.cnt], &det_ctx->alert_queue[i], sizeof(PacketAlert));


Suggest simple structure copy instea

p->alerts.alerts[p->alerts.cnt] = det_ctx->alert_queue[i];

jlucovsky · 2022-04-27T14:29:13Z

src/detect-engine-alert.c

@@ -34,6 +34,13 @@ static Signature g_tag_signature;
 /** tag packet alert structure for tag alerts */
 static PacketAlert g_tag_pa;

+/* For WARN_ONCE, a record of warnings that have already been
+ * issued. */
+static thread_local bool once_err[SC_ERR_MAX];


However, this add a per-thread array of size SC_ERR_MAX to detect-engine-alert.c but it's only use with a single error code SC_ERR_MEM_ALLOC. If memory is sufficiently low that a small alloc will fail we'll get a flurry of error messages (one from each thread) and then (it's likely) Suricata will fail elsewhere.

Maybe the realloc logic isn't needed or a FatalError should occur or perhaps, now that errors are handled, simply return the old capacity on realloc failures.

jufajardini · 2022-04-27T16:16:46Z

Replaced by: #7326

jufajardini added 12 commits April 25, 2022 13:00

decode: make packet_alert_max configurable

6848aae

The maximum of possible alerts triggered by a unique packet was hardcoded to 15. With usage of 'noalert' rules, that limit could be reached somewhat easily. Make that configurable via suricata.yaml. Conf Bug#4941 Task OISF#4207

detect/alert: add infra for new alert queue

10eb803

Initial work to bring part of the alert queue processing to DetectEngineThreadCtx. Task OISF#4943

detect/engine: use alert queue from det_ctx

734282d

Task OISF#4943

detect/alert: process alert queue before appending

0c72ae3

Do all alert queue processing before actually appending the PacketAlerts to the Packet's alert queue. Task OISF#4943

detect/stats: log out total of discarded alerts

56d7718

Add a counter to our stats log with the total of alerts that have been discarded due to packet alert queue overflow. Task OISF#5179

detect/stats: log out total of suppressed alerts

4c9f684

Related to Task OISF#4943 Task OISF#5179

assorted: fix low hanging typos

f08aa43

detect: update copyright years

ccd00ad

util/debug: make WARN_ONCE available for all files

65b697a

Since more than one file needed a similar function, make it available from util/debug instead of having it as a local macro. Task OISF#4943

detect/alert: handle alert queue expand failure

ee22759

In the unlikely case of an alert queue expansion reallocation failure, we want to make sure that we don't just ignore it. But we also don't want to break things or flood Suri with warnings. Task OISF#4943

detect/alert: use tx id in alert if frame has it

704e85c

Task OISF#4943

jufajardini mentioned this pull request Apr 26, 2022

Alert queue optimizations - v3 #7297

Closed

jufajardini added the needs doc update label Apr 26, 2022

jufajardini marked this pull request as ready for review April 26, 2022 21:35

jufajardini requested a review from a team as a code owner April 26, 2022 21:35

jufajardini marked this pull request as draft April 26, 2022 21:36

victorjulien reviewed Apr 27, 2022

View reviewed changes

jlucovsky reviewed Apr 27, 2022

View reviewed changes

jufajardini mentioned this pull request Apr 27, 2022

Alert queue optimizations - v5 #7326

Closed

jufajardini closed this Apr 27, 2022

jufajardini deleted the alert-queue-det-ctx/v4 branch May 2, 2022 12:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alert queue optimizations - v4 #7314

Alert queue optimizations - v4 #7314

jufajardini commented Apr 26, 2022

codecov bot commented Apr 26, 2022

suricata-qa commented Apr 27, 2022

victorjulien Apr 27, 2022

victorjulien Apr 27, 2022

victorjulien Apr 27, 2022

victorjulien Apr 27, 2022

jufajardini Apr 27, 2022

jlucovsky Apr 27, 2022

jlucovsky Apr 27, 2022

jufajardini Apr 27, 2022

jlucovsky Apr 27, 2022

jlucovsky Apr 27, 2022

jlucovsky Apr 27, 2022

jlucovsky Apr 27, 2022

jufajardini commented Apr 27, 2022

Alert queue optimizations - v4 #7314

Alert queue optimizations - v4 #7314

Conversation

jufajardini commented Apr 26, 2022

codecov bot commented Apr 26, 2022

Codecov Report

suricata-qa commented Apr 27, 2022

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

jufajardini commented Apr 27, 2022