-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Alert queue optimizations - v3 #7297
Conversation
Initial work to bring the alert queue processing to DetectEngineThreadCtx. Task OISF#4943
Sort the PacketAlert queue that will be passed to the Packet handling functions, so there's less work to be done with it from there. 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
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
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
Codecov Report
@@ Coverage Diff @@
## master #7297 +/- ##
==========================================
- Coverage 75.82% 75.77% -0.05%
==========================================
Files 656 656
Lines 190051 190091 +40
==========================================
- Hits 144102 144049 -53
- Misses 45949 46042 +93
Flags with carried forward coverage won't be shown. Click here to find out more. |
QA on my end looks good. |
det_ctx->alert_queue, (size_t)(sizeof(PacketAlert) * det_ctx->alert_queue_capacity)); | ||
if (tmp_queue == NULL) { | ||
SCLogWarning(SC_ERR_MEM_ALLOC, "failed to allocate %" PRIuMAX " bytes", | ||
(uintmax_t)(PACKET_ALERT_MAX * sizeof(PacketAlert))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restore alert_queue_capcity
on error return.
The size value in the message is kind of correct because you're trying to allocate an additional group of bytes of that size. However, the total allocation is sizeof(PacketAlert)* det_ctx->alert_queue_capacity
{ | ||
if (det_ctx->alert_queue_size == det_ctx->alert_queue_capacity) { | ||
/* we must grow the alert queue */ | ||
AlertQueueExpand(det_ctx); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to handle AlertQueueExpand
failures
/** \internal | ||
* \brief sort helper for sorting alerts by priority | ||
* | ||
* The id field is set from Signature::num, so we have higher priority (lower ids) first |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: s/id/tx_id/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This actually needed some rewording, thanks for making me re-read it :P
Note: please make sure all tickets in the commits are mentioned in the PR:
Please update PR body and remove the 'needs ticket' label when done. |
Followed by #7314 |
Previous PR: #7290
Link to redmine ticket: https://redmine.openinfosecfoundation.org/issues/4943
Describe changes:
suricata-verify-pr: 808
OISF/suricata-verify#808