Feat/flowbit prefilter/v26 #12662

victorjulien · 2025-02-24T13:13:46Z

https://redmine.openinfosecfoundation.org/issues/2486

Rebase of #12380, also addresses some of the issues.

Allow for more efficient rules that 'prefilter' on flowbits with 'isset' logic. This prefilter is enabled by default, which means that if no mpm is present or no explicit prefilter is used, the flowbits prefilter will be set up for a rule. flowbits 'isset' prefilter For rules that have a 'flowbits:isset,<bit>' statement, a "regular" prefilter facility is created. It means that the rules are removed from the normal match list(s) and added to a prefilter engine that runs prior to the individual rule inspection stage. Implementation: the prefilter is implemented as an RB_TREE of flowbits, with the rule id's they "enable" stored per tree node. The matching logic is walking the list of bits set in the flow and looking each of them up in the RB_TREE, adding the rule ids of each of the matching bits to the list of rule candidates. The 'isset' prefilter has one important corner case, which is that bits can in fact be set during the rule evaluation stage. This is different from all other prefilter engines, that evaluate an immutable state (for the lifetime of the packets inspection). flowbits 'set' post-match prefilter For flowbits 'set' action, special post-match 'prefilter' facilities deal with this corner case. The high level logic is that these track which 'isset' sigs depend on them, and add these dependencies to the candidates list when a 'set' action occurs. This is implemented in a few steps: 1. flowbits 'set' is flagged 2. when 'set' action occurs the flowbit is added to a "post rule match work queue" 3. when the rule evaluation ends, the post-match "prefilter" engine is run on each of the flowbits in the "post rule match work queue" 4. these engines ammend the candidates list with the rule id dependencies for the flowbit 5. the candidates list is sorted to make sure within the execution for that packet the inspection order is maintained Ticket: OISF#2486.

codecov · 2025-02-24T13:40:41Z

Codecov Report

Attention: Patch coverage is 81.83333% with 109 lines in your changes missing coverage. Please review.

Project coverage is 80.78%. Comparing base (3bc2a14) to head (8ed6125).

Additional details and impacted files

@@           Coverage Diff            @@
##           master   #12662    +/-   ##
========================================
  Coverage   80.77%   80.78%            
========================================
  Files         932      932            
  Lines      259517   259990   +473     
========================================
+ Hits       209629   210028   +399     
- Misses      49888    49962    +74

Flag	Coverage Δ
fuzzcorpus	`56.83% <20.06%> (-0.16%)`	⬇️
livemode	`19.31% <1.34%> (-0.07%)`	⬇️
pcap	`44.06% <15.51%> (-0.09%)`	⬇️
suricata-verify	`63.57% <81.33%> (+0.06%)`	⬆️
unittests	`58.23% <16.52%> (-0.10%)`	⬇️

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

suricata-qa · 2025-02-24T15:35:02Z

Information: QA ran without warnings.

Pipeline 24875

jlucovsky · 2025-02-25T13:38:25Z

src/detect-engine-prefilter.c

+    if (sgh->init->post_rule_match_engines == NULL) {
+        sgh->init->post_rule_match_engines = e;
+    } else {
+        PrefilterEngineList *t = sgh->init->post_rule_match_engines;


Is this list expected to be more than a few items? If so, would a list/queue that allows tail adds be more efficient?

Currently just one I think.

src/detect-engine-prefilter.h

src/detect-flowbits.c

src/detect.c

src/flow-bit.c

catenacyber · 2025-02-25T17:14:53Z

src/flow-bit.c

+/** \brief add a flowbit to the flow
+ *  \retval -1 error
+ *  \retval 0 not added, already set before
+ *  \retval 1 added */


wonder if using an enum would be better...

adding a enum just for this func seems like overkill to me, but perhaps it makes sense explore this for a bit more generally throughout more API calls

catenacyber · 2025-02-25T17:17:02Z

I wonder how much of this need is covered by #12646 ...

src/detect-engine-build.c

catenacyber · 2025-02-25T17:34:39Z

the prefilter is implemented as an RB_TREE of flowbits, with the rule id's they "enable" stored per tree node.

Why ?
Do flowbits have a unique id ?
Could it not be a simple array ? (even if it is painful to build)

catenacyber · 2025-02-25T17:46:02Z

The 'isset' prefilter has one important corner case, which is that bits can in fact be set during the rule evaluation stage. This is different from all other prefilter engines, that evaluate an immutable state (for the lifetime of the packets inspection).

This looks strange and counterintuitive.
Do you mean that alerts like this will trigger on the same packet ?

alert http any any -> any any (http.uri; content:"test"; flowbits:set,uritest; sid:11;)
alert http any any -> any any (http.method; content:"HEAD"; flowbits:isset,uritest; sid:12;)

How does the engine know to run first the rules that set flowbits before the ones that need them ?

victorjulien · 2025-02-25T17:48:13Z

The 'isset' prefilter has one important corner case, which is that bits can in fact be set during the rule evaluation stage. This is different from all other prefilter engines, that evaluate an immutable state (for the lifetime of the packets inspection).

This looks strange and counterintuitive. Do you mean that alerts like this will trigger on the same packet ?
alert http any any -> any any (http.uri; content:"test"; flowbits:set,uritest; sid:11;)
alert http any any -> any any (http.method; content:"HEAD"; flowbits:isset,uritest; sid:12;)
How does the engine know to run first the rules that set flowbits before the ones that need them ?

During rule ordering a rule that sets a bit will be put before a sigs that checks the bit.

victorjulien · 2025-02-25T17:49:21Z

the prefilter is implemented as an RB_TREE of flowbits, with the rule id's they "enable" stored per tree node.

Why ? Do flowbits have a unique id ? Could it not be a simple array ? (even if it is painful to build)

Most flowbits will not use this, so the id space is quite sparse. An array is going to be large.

catenacyber · 2025-02-25T20:06:03Z

During rule ordering a rule that sets a bit will be put before a sigs that checks the bit.

How do you handle cycles ?

Like

alert ip any any -> any any (someconditions; flowbits:isset,bitC; flowbits:set,bitA; sid:11;)
alert ip any any -> any any (someconditions; flowbits:isset,bitA; flowbits:set,bitB; sid:12;)
alert ip any any -> any any (someconditions; flowbits:isset,bitB; flowbits:set,bitC; sid:13;)

alert ip any any -> any any (someconditions; flowbits:set,bitA; sid:9;)
alert ip any any -> any any (someconditions; flowbits:set,bitB; sid:8;)
alert ip any any -> any any (someconditions; flowbits:set,bitC; sid:7;)

catenacyber · 2025-02-25T20:09:05Z

Most flowbits will not use this, so the id space is quite sparse. An array is going to be large.

Indeed, but are not a few kilobytes of RAM worth the nanoseconds for direct access ?

I think prefilter is about optimizing speed, right ?
For the structure, we want to optimize getting an element, right ?
There are not so many flowbits, so I think we can afford a big sparse array as it brings speed improvement, whereas a rb tree takes O(log(n)) steps to get an element

victorjulien · 2025-02-26T06:07:57Z

During rule ordering a rule that sets a bit will be put before a sigs that checks the bit.

How do you handle cycles ?

Like

alert ip any any -> any any (someconditions; flowbits:isset,bitC; flowbits:set,bitA; sid:11;)
alert ip any any -> any any (someconditions; flowbits:isset,bitA; flowbits:set,bitB; sid:12;)
alert ip any any -> any any (someconditions; flowbits:isset,bitB; flowbits:set,bitC; sid:13;)

alert ip any any -> any any (someconditions; flowbits:set,bitA; sid:9;)
alert ip any any -> any any (someconditions; flowbits:set,bitB; sid:8;)
alert ip any any -> any any (someconditions; flowbits:set,bitC; sid:7;)

This isn't new, the ordering code exists for a long time already.

victorjulien · 2025-02-26T06:09:02Z

Most flowbits will not use this, so the id space is quite sparse. An array is going to be large.

Indeed, but are not a few kilobytes of RAM worth the nanoseconds for direct access ?

I think prefilter is about optimizing speed, right ? For the structure, we want to optimize getting an element, right ? There are not so many flowbits, so I think we can afford a big sparse array as it brings speed improvement, whereas a rb tree takes O(log(n)) steps to get an element

This discussion is getting ahead of itself. If you want to argue about the performance you'll have to bring profiling results, not just theory to the table here.

catenacyber · 2025-02-26T07:08:26Z

This isn't new, the ordering code exists for a long time already.

Sure.
For reviewing this PR, I just want to understand this flowbits behavior as this PR commit message highlights the importance of this edge case (that I did not know of)

catenacyber · 2025-02-26T07:14:41Z

This discussion is getting ahead of itself. If you want to argue about the performance you'll have to bring profiling results, not just theory to the table here.

I am just discussing the design, I did not even look at the code.

I can do a benchmark to show that PFB_RB_FIND(&ctx->fb_tree, &lookup); is slower than ctx->fb_array[gv->idx]; but is this what we want to achieve ? (optimize speed of PrefilterFlowbitMatch )

victorjulien · 2025-02-26T07:39:42Z

This discussion is getting ahead of itself. If you want to argue about the performance you'll have to bring profiling results, not just theory to the table here.

I am just discussing the design, I did not even look at the code.

I can do a benchmark to show that PFB_RB_FIND(&ctx->fb_tree, &lookup); is slower than ctx->fb_array[gv->idx]; but is this what we want to achieve ? (optimize speed of PrefilterFlowbitMatch )

I mean of this implementation with a real world use case.

catenacyber · 2025-02-26T14:29:49Z

For the "important corner case", am I correct to say that a dumb implementation of prefilter without this corner case will make

alert http any any -> any any (http.uri; content:"test"; flowbits:set,uritest; sid:11;)
alert http any any -> any any (http.method; content:"HEAD"; flowbits:isset,uritest; prefilter; sid:12;)

with a packet that is HEAD /test HTTP/1.1\n\n

not trigger rule 12 when it should (and does trigger without the prefilter keyword), because rule 12 will be skipped at prefilter stage ?

Does it make sense for rule writers to prefilter on a flowbit that will be set during rules evaluation ?
But I guess we cannot know, based on the rules only, when one flowbit will be set (same packet, or strictly before its isset rule evaluation)

catenacyber · 2025-02-26T14:38:58Z

src/detect.c

@@ -861,6 +871,51 @@ static inline void DetectRulePacketRules(
            }
        }
        AlertQueueAppend(det_ctx, s, p, txid, alert_flags);
+
+        if (det_ctx->post_rule_work_queue.len > 0) {


Should it be a while ?

Like for

alert http any any -> any any (http.uri; content:"test"; flowbits:set,uritest; sid:11;) alert http any any -> any any (http.method; content:"HEAD"; flowbits:isset,uritest; prefilter; flowbits:set,headtest; sid:12;) alert http any any -> any any (http.ua; content:"toto"; flowbits:isset, headtest; prefilter; sid:13;)

Prefilter removes sids 12 and 13 because flowbits are not set

Rule evaluation makes sid 11 match and set flowbit uritest

We run post-"prefilter" for flowbit uritest, get sid 12

Rule evaluation makes sid 12 match and set flowbit headtest

We run post-"prefilter" again but for flowbit headtest, get sid 13

Rule evaluation makes sid 13 match and we have no more new flowbits set

I don't see why it should be a while. It's run a single time per signature that has this functionality enabled.

Indeed because the loop is done one level up as for (uint32_t i = 0; i < array_idx; i++) { and we just keep increasing the array

victorjulien · 2025-02-26T19:18:52Z

I wonder how much of this need is covered by #12646 ...

Partly, but flowbits have a much wider scope. Ppl build state machines with them.

victorjulien · 2025-02-26T19:28:06Z

For the "important corner case", am I correct to say that a dumb implementation of prefilter without this corner case will make
alert http any any -> any any (http.uri; content:"test"; flowbits:set,uritest; sid:11;)
alert http any any -> any any (http.method; content:"HEAD"; flowbits:isset,uritest; prefilter; sid:12;)
with a packet that is HEAD /test HTTP/1.1\n\n

not trigger rule 12 when it should (and does trigger without the prefilter keyword), because rule 12 will be skipped at prefilter stage ?

Does it make sense for rule writers to prefilter on a flowbit that will be set during rules evaluation ? But I guess we cannot know, based on the rules only, when one flowbit will be set (same packet, or strictly before its isset rule evaluation)

It should not be affected by prefilter or not. Sid 11 will be evaluated first, because rule sorting puts it before 12 due to the flowbit set.

In non-prefilter, sid 12 would be evaluated, and in the evaluation we check if the flowbit "uritest" was set.

In the prefilter case sid 11 "knows" which sids depend on it, and it will add those dependencies for evaluation to the list of sig candidates. So without the match of sid 11, sid 12 would not have been evaluated. But now it is.

catenacyber · 2025-02-26T20:51:48Z

I wonder how much of this need is covered by #12646 ...

Partly, but flowbits have a much wider scope. Ppl build state machines with them.

Sure, but I can feel people doing flowbits because they do not have bidirectional rules, and then complain that the second rule does not have a good fast_pattern...

catenacyber · 2025-02-26T20:52:14Z

To be closed in favor of #12681 ?

catenacyber · 2025-02-26T21:11:26Z

In the prefilter case sid 11 "knows" which sids depend on it, and it will add those dependencies for evaluation to the list of sig candidates. So without the match of sid 11, sid 12 would not have been evaluated. But now it is.

You mean that is what your smart PR does with PrefilterFlowbitPostRuleMatch, not a dumb implementation of prefilter without the corner case, right ?

victorjulien and others added 3 commits February 24, 2025 13:30

flowbits: unset returns state of the bit

33bc004

flowbits: allow setter to know if set is new

66b6d23

victorjulien mentioned this pull request Feb 24, 2025

Feat/flowbit prefilter/v25 #12380

Closed

jlucovsky reviewed Feb 25, 2025

View reviewed changes

catenacyber reviewed Feb 25, 2025

View reviewed changes

src/flow-bit.c Show resolved Hide resolved

catenacyber reviewed Feb 25, 2025

View reviewed changes

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

catenacyber reviewed Feb 26, 2025

View reviewed changes

victorjulien mentioned this pull request Feb 26, 2025

Feat/flowbit prefilter/v27 #12681

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/flowbit prefilter/v26 #12662

Feat/flowbit prefilter/v26 #12662

victorjulien commented Feb 24, 2025

codecov bot commented Feb 24, 2025

suricata-qa commented Feb 24, 2025

jlucovsky Feb 25, 2025

victorjulien Feb 26, 2025

catenacyber Feb 25, 2025

victorjulien Feb 26, 2025

catenacyber commented Feb 25, 2025

catenacyber commented Feb 25, 2025 •

edited

Loading

catenacyber commented Feb 25, 2025

victorjulien commented Feb 25, 2025

victorjulien commented Feb 25, 2025

catenacyber commented Feb 25, 2025

catenacyber commented Feb 25, 2025

victorjulien commented Feb 26, 2025

victorjulien commented Feb 26, 2025

catenacyber commented Feb 26, 2025

catenacyber commented Feb 26, 2025

victorjulien commented Feb 26, 2025

catenacyber commented Feb 26, 2025

catenacyber Feb 26, 2025

victorjulien Feb 26, 2025

catenacyber Feb 26, 2025

victorjulien commented Feb 26, 2025

victorjulien commented Feb 26, 2025

catenacyber commented Feb 26, 2025

catenacyber commented Feb 26, 2025

catenacyber commented Feb 26, 2025

Feat/flowbit prefilter/v26 #12662

Are you sure you want to change the base?

Feat/flowbit prefilter/v26 #12662

Conversation

victorjulien commented Feb 24, 2025

codecov bot commented Feb 24, 2025

Codecov Report

suricata-qa commented Feb 24, 2025

jlucovsky Feb 25, 2025

Choose a reason for hiding this comment

victorjulien Feb 26, 2025

Choose a reason for hiding this comment

catenacyber Feb 25, 2025

Choose a reason for hiding this comment

victorjulien Feb 26, 2025

Choose a reason for hiding this comment

catenacyber commented Feb 25, 2025

catenacyber commented Feb 25, 2025 • edited Loading

catenacyber commented Feb 25, 2025

victorjulien commented Feb 25, 2025

victorjulien commented Feb 25, 2025

catenacyber commented Feb 25, 2025

catenacyber commented Feb 25, 2025

victorjulien commented Feb 26, 2025

victorjulien commented Feb 26, 2025

catenacyber commented Feb 26, 2025

catenacyber commented Feb 26, 2025

victorjulien commented Feb 26, 2025

catenacyber commented Feb 26, 2025

catenacyber Feb 26, 2025

Choose a reason for hiding this comment

victorjulien Feb 26, 2025

Choose a reason for hiding this comment

catenacyber Feb 26, 2025

Choose a reason for hiding this comment

victorjulien commented Feb 26, 2025

victorjulien commented Feb 26, 2025

catenacyber commented Feb 26, 2025

catenacyber commented Feb 26, 2025

catenacyber commented Feb 26, 2025

catenacyber commented Feb 25, 2025 •

edited

Loading