Packet alerts/v3 #6896

victorjulien · 2022-01-29T15:55:18Z

#6895 with an fix/optimization for alert queue removal/insert.

@catenacyber can you help review the memmove logic? The relation between array index, array size and array use cnt is easily messed up I think.

suricata-verify-pr: 694

Some unittests used SCMalloc for allocating new Packet the unittests. While this is valid, it leads to segmentation faults when we move to dynamic allocation of the maximum alerts allowed to be triggered by a single packet. This massive patch uses PacketGetFromAlloc, which initializes a Packet in such a way that any dynamic allocated structures within will also be initialized. Related to Task#4207

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#4207

Plus small clang formatting change.

codecov · 2022-01-29T16:15:04Z

Codecov Report

Merging #6896 (3078f8f) into master (f8e1430) will decrease coverage by 0.03%.
The diff coverage is 98.10%.

@@            Coverage Diff             @@
##           master    #6896      +/-   ##
==========================================
- Coverage   77.72%   77.69%   -0.04%     
==========================================
  Files         628      628              
  Lines      186493   186392     -101     
==========================================
- Hits       144959   144813     -146     
- Misses      41534    41579      +45

Flag	Coverage Δ
fuzzcorpus	`58.27% <87.50%> (-0.12%)`	⬇️
suricata-verify	`54.05% <96.15%> (-0.03%)`	⬇️
unittests	`63.18% <95.39%> (-0.03%)`	⬇️

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

catenacyber · 2022-01-31T12:57:08Z

src/decode.h

+#define PACKET_INITIALIZE(p)                                                                       \
+    {                                                                                              \
+        SCMutexInit(&(p)->tunnel_mutex, NULL);                                                     \
+        (p)->alerts.alerts = CreatePacketAlert();                                                  \


Could/Should we not defer the need for allocation until there truly is one alert ?

Victor has mentioned something similar, actually, iirc. I'll have a look at how to do address that...

The packets and their alert structures are pooled and reused all the time. We don't free them, only clear them. So allocing this at init makes sense and avoids alloc time overhead at runtime for an already expensive packet (ie an alerting packet)

catenacyber · 2022-01-31T12:58:07Z

src/decode.c

+        SCLogDebug("detect->packet_alert_max set to %" PRIdMAX "", max);
+        packet_alert_max = max;
+    } else {
+        packet_alert_max = PACKET_ALERT_MAX;


is it not redundant with line 77 ?

Yes, you're right.

catenacyber · 2022-01-31T12:58:39Z

src/decode.c

+{
+    intmax_t max = 0;
+    if (ConfGetInt("packet-alert-max", &max) == 1) {
+        SCLogDebug("detect->packet_alert_max set to %" PRIdMAX "", max);


Could have only one debug line for both cases I guess

catenacyber · 2022-01-31T13:00:35Z

src/decode.c

+{
+    PacketAlert *pa_array = NULL;
+    if (RunmodeIsUnittests()) {
+        pa_array = SCCalloc(PACKET_ALERT_MAX, sizeof(PacketAlert));


Do we have any idea what is the performance impact ?
Could/should we use a pool of preallocated buffers ?

The packets themselves are pooled and reused, so this allocation is a once per packet step.

catenacyber · 2022-01-31T13:07:00Z

src/detect-engine-alert.c

-    for (i = pos; i <= p->alerts.cnt - 1; i++) {
-        memcpy(&p->alerts.alerts[i], &p->alerts.alerts[i + 1], sizeof(PacketAlert));
+    const uint16_t after = (p->alerts.cnt - 1) - pos;
+    if (after) {


Is p->alerts.alerts somehow sorted ?
If not, it would be more efficient to just move the last alert to the removed position.
That would be like memmove(p->alerts.alerts + pos, &p->alerts.alerts[p->alerts.cnt - 1], sizeof(PacketAlert)

yes this logic is to have it sorted by Signature::num (the internal id, assigned after sort by sig priority).

catenacyber · 2022-01-31T13:09:19Z

src/detect-engine-alert.c

+        const uint16_t target_pos = i + 1;
+        const uint16_t space_post_target = packet_alert_max - 1 - i;
+        const uint16_t to_move = MIN(space_post_target, (p->alerts.cnt - i));
+        memmove(p->alerts.alerts + target_pos, p->alerts.alerts + i,


Will this work correctly if to_move == 0 ?

I'll add a BUG_ON but given the check above it should not ever be 0

it should not ever be 0

It can be 0 : if space_post_target is 0 c-which can be the case if you have like packet_alert_max= 3, you have 3 alerts with num 3,5,7 and you want to add one with num 6

Than no memmove, just replace the last one

Otherwise, all is good :-)

but if we had packet_alert_max=3 in this example, the function would have returned 0 before, so we would not reach this stage, right?

S-V tests failing with BUG_ON(to_move == 0) atm:
===> http2-bugfixes: FAILED: got exit code -6, expected 0
===> http2-files: FAILED: got exit code -6, expected 0
===> http-sticky-server: FAILED: got exit code -6, expected 0
===> smb2-07-frames: FAILED: got exit code -6, expected 0
===> tls13-draft28-frames: FAILED: got exit code -6, expected 0

but if we had packet_alert_max=3 in this example, the function would have returned 0 before, so we would not reach this stage, right?

Indeed, but I guess it should not return 0, because we want to insert the highest priority alert

catenacyber · 2022-01-31T13:12:48Z

src/detect-engine-alert.c

-            memcpy(&p->alerts.alerts[i + 1], &p->alerts.alerts[i], sizeof(PacketAlert));
-        }
+        /* find position to insert */
+        for (i = p->alerts.cnt - 1; i >= 0 && p->alerts.alerts[i].num > s->num; i--)


There is a duplicate test for p->alerts.alerts[p->alerts.cnt - 1].num < s->num

And
if (p->alerts.cnt == 0 || (p->alerts.cnt > 0 && p->alerts.alerts[p->alerts.cnt - 1].num < s->num)) {
can be simplified to
if (p->alerts.cnt == 0 || p->alerts.alerts[p->alerts.cnt - 1].num < s->num) {
as alerts.cnt is unsigned cf PacketAlerts_

There is a duplicate test for p->alerts.alerts[p->alerts.cnt - 1].num < s->num

Sorry, I don't think I get this. Can you point out where?

It is tested a first time in the surrounding if. Then in the else clause, it is tested again in the loop

I think I see it now. Because the surrounding if has it < while the for checks for > I didn't realize it before. Thanks :)

catenacyber · 2022-01-31T13:18:09Z

can you help review the memmove logic? The relation between array index, array size and array use cnt is easily messed up I think.

It looks dangerous indeed... I left some questions

suricata-qa · 2022-02-02T05:05:03Z

ERROR: QA failed on build_asan.

Pipeline 6035

catenacyber · 2022-02-02T20:30:51Z

ERROR: QA failed on build_asan.

Pipeline 6035

Because of needed rebase

jufajardini · 2022-02-04T17:49:35Z

Rebased and addressed things related to my changes on #6929

jufajardini · 2022-02-14T10:31:39Z

Comments addressed with #6999
Can this one be closed?

victorjulien · 2022-02-14T20:44:26Z

Thanks @jufajardini

jufajardini and others added 4 commits January 29, 2022 07:41

decode: make packet_alert_max configurable

48f78ef

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#4207

detect-engine-alert: fix tipo

db17a29

Plus small clang formatting change.

detect/alert: optimize alert queue processing

3078f8f

victorjulien requested a review from catenacyber January 29, 2022 15:55

victorjulien requested a review from a team as a code owner January 29, 2022 15:55

catenacyber reviewed Jan 31, 2022

View reviewed changes

catenacyber mentioned this pull request Feb 2, 2022

decode: use configurable value for packet_alert_max - v1 #6895

Closed

catenacyber added the needs rebase Needs rebase to master label Feb 2, 2022

jufajardini mentioned this pull request Feb 4, 2022

Packet alerts/v4 #6929

Closed

victorjulien mentioned this pull request Feb 8, 2022

Packet alerts/v6 #6943

Closed

jufajardini mentioned this pull request Feb 14, 2022

Packet alerts/v7 #6999

Closed

victorjulien closed this Feb 14, 2022

catenacyber mentioned this pull request Feb 22, 2022

Packet alerts/v8 #7000

Closed

victorjulien deleted the packet-alerts/v3 branch February 24, 2022 10:24

This was referenced Mar 10, 2022

Add tests for packet_alert_max - v4 OISF/suricata-verify#782

Closed

Packet alerts/v9 #7133

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Packet alerts/v3 #6896

Packet alerts/v3 #6896

victorjulien commented Jan 29, 2022

codecov bot commented Jan 29, 2022

catenacyber Jan 31, 2022

jufajardini Jan 31, 2022

victorjulien Jan 31, 2022

catenacyber Jan 31, 2022

jufajardini Jan 31, 2022

catenacyber Jan 31, 2022

catenacyber Jan 31, 2022

victorjulien Jan 31, 2022

catenacyber Jan 31, 2022

victorjulien Jan 31, 2022

catenacyber Jan 31, 2022

victorjulien Jan 31, 2022

catenacyber Jan 31, 2022

jufajardini Feb 11, 2022

jufajardini Feb 11, 2022

catenacyber Feb 22, 2022

catenacyber Jan 31, 2022

catenacyber Jan 31, 2022

jufajardini Feb 11, 2022

catenacyber Feb 22, 2022

jufajardini Feb 23, 2022

catenacyber commented Jan 31, 2022

suricata-qa commented Feb 2, 2022

catenacyber commented Feb 2, 2022

jufajardini commented Feb 4, 2022

jufajardini commented Feb 14, 2022

victorjulien commented Feb 14, 2022

Packet alerts/v3 #6896

Packet alerts/v3 #6896

Conversation

victorjulien commented Jan 29, 2022

codecov bot commented Jan 29, 2022

Codecov Report

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

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

catenacyber commented Jan 31, 2022

suricata-qa commented Feb 2, 2022

catenacyber commented Feb 2, 2022

jufajardini commented Feb 4, 2022

jufajardini commented Feb 14, 2022

victorjulien commented Feb 14, 2022