Detect tcp noupdatetotx 6299 v9 by catenacyber · Pull Request #10145 · OISF/suricata

catenacyber · 2024-01-09T21:09:48Z

Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/6299

Describe changes:

Optimization to not run transaction detection when a TCP packet did not update anything (no call to AppLayerParserParse) in the packet direction
Merge sorted lists should be faster than qsort
do not store state if there is no flags set
cleans mqtt code for setting events

#10127 with better commit messages and inline helper function

When a TCP flow packet has not led to app-layer updates, it is useless to run DetectRunTx, as there cannot be new matches. This happens for instance, when one side sends in a row multiple packets which are not acked (and thus not parsed in IDS mode). Doing so requires to move up the call to AppLayerParserSetTransactionInspectId so that it is only run after DetectRunTx is run, and not in the case where the transaction was not updated. Ticket: 6299

Ticket: OISF#6299 Simply because it is faster (just linear).

If flags are zero, there is nothing to store and remember. Stored signatures will be reused on a later packet, and qsorted (which may be expensive), with newer matches candidates. Avoiding to store, leads to avoid the call to qsort.

Especially sets transactions to complete when we get a response without having seen the request, so that the transactions end up getting cleaned (instead of living/leaking in the state). Also try to set the event on the relevant transaction, instead of creating a new transaction just for the purpose of having the event. Ticket: OISF#6299

suricata-qa · 2024-01-09T22:30:03Z

WARNING:

field	baseline	test	%
	build_asan

Pipeline 17437

victorjulien · 2024-01-10T11:20:32Z

src/detect.c

    }
 }

+static inline RuleMatchCandidateMergeSorted(DetectEngineThreadCtx *det_ctx, uint32_t j, uint32_t k)


I'm having trouble understanding this. When we get called we have have 0-N entries in tx_candidates from prefilter. These are already ordered. The candidates coming from the match_array are merged into this, and the result has to be a sorted list by the candidates s->num (internal sig id). I guess I'm not seeing how we insert sort things into the already existing list.

Is this a matter of where this has been called, together with the fact that we still have the qsort called later on, (and we're not taking num into consideration when doing the ordering comparison), or are these factors irrelevant here?

with the fact that we still have the qsort called later on

The point is to avoid the call to qsort (in the case we do not have match candidates added after by stored flags) cf removal of do_sort = (array_idx > x); // sort if match added anything

I'm having trouble understanding this

Will put more comments.

Quick and dirty answer :
This is basically merging two sorted lists.
The trick is do it in place, without shifting all the elements.
So we start from the end (if we start from the beginning, and the new element should be first, we would have to shift all j elements already in place)

victorjulien · 2024-01-10T11:23:32Z

src/detect.c

            TRACE_SID_TXS(s->id, tx, "no need to store no-match sig, "
                    "mpm will revisit it");
-        } else {
+        } else if (inspect_flags != 0) {


we store more than just the flags, we also store file_no_match. Can we have a case where we'd need that stored still?

Very good point from your knowledge and wisdom. Will look that through

Side note, feels weird to see file_no_match as u16 when it is only 0 or 1 up to its use in StoreFileNoMatchCnt as += file_no_match;

The goal of the logic, but I wouldn't be surprised if it is broken, is to stop tracking the files if all sigs that need it definitively failed to match. So it should increment this for each unique sig that fails to match.

The goal of the logic, but I wouldn't be surprised if it is broken, is to stop tracking the files if all sigs that need it definitively failed to match. So it should increment this for each unique sig that fails to match.

That is what I understood from reading the code

jufajardini

Feels a bit out of my league, but tried to add some comments to understand better...

jufajardini · 2024-01-10T14:18:01Z

src/detect.c

@@ -1296,6 +1296,46 @@ static inline void StoreDetectFlags(DetectTransaction *tx, const uint8_t flow_fl
    }
 }



If we merge this one, could we have a comment indicating what params j and k are?

Good idea, will do. IIRC they are size of both sorted lists to merge into a big one

jufajardini · 2024-01-10T14:31:40Z

src/detect.c

+            const SigIntId id = s->num;
+            if (j > 0) {
+                const RuleMatchCandidateTx *s0 = &det_ctx->tx_candidates[j - 1];
+                if (s->id > s0->id) {


Shouldn't this comparison be with num instead of id, since num is what's used for sorting?

id is correct. I do think the names of objects should probably have been consistent in these structs.
Edit: Just realized what you meant, indeed the first item should be id or s->num.

Thinking about renaming Signature::num to Signature::iid (internal id)

cf DetectRunTxSortHelper

Nice catch Juliana

jufajardini · 2024-01-10T14:43:02Z

src/detect.c

    }
 }

+static inline RuleMatchCandidateMergeSorted(DetectEngineThreadCtx *det_ctx, uint32_t j, uint32_t k)


Is this a matter of where this has been called, together with the fact that we still have the qsort called later on, (and we're not taking num into consideration when doing the ordering comparison), or are these factors irrelevant here?

inashivb

I think I understand why you call merge sort linear.
Some background for anyone who wants to understand why this kind of makes sense but not all the way (to me).

Quick Sort in most cases is a superior sorting algorithm than Merge Sort. Quick Sort exploits cache locality principles better and choosing the right pivot can make it much better in many aspects although both share an average time complexity of O(nlogn).
Merge Sort works by breaking the given n element array down into smaller parts, sorting them and then putting them back together. Now, it's an incredible advantage for merge sort to get already sorted data and that we know where that is. In best cases, it can then work in O(n) [which is why I think Philippe calls it linear?]
Now, the above points fit in our situation because as Victor said tx_candidates entries are already ordered and then after the merge w match_array elements it stays ordered (didn't check this). Hence, giving quite an advantage to merge sort for this case.

Questions that don't make sense to me all the way:

qsort call at the end of this construct still exists. I would have expected that to be replaced by merge sort.
Some research tells me that Tim Sort is better suited for our usecase here. ref: https://en.wikipedia.org/wiki/Timsort What do you think?

inashivb

Should the commit ea12eeccf8 say "it is useless to run AppLayerParserSetTransactionInspectId"? Because then I can read the code and commit syncing.

catenacyber · 2024-01-11T21:15:32Z

Should the commit ea12eeccf8 say "it is useless to run AppLayerParserSetTransactionInspectId"? Because then I can read the code and commit syncing.

AppLayerParserSetTransactionInspectId should be called together with DetectRunTx. It is either both or them, or none of them

But AppLayerParserSetTransactionInspectId was called from DetectRunPostRules which was called from DetectRun

As we add a case to skip the call of DetectRunTx, but not the call of DetectRunPostRules, we need to move AppLayerParserSetTransactionInspectId up (in the stack call) from DetectRunPostRules to DetectRunTx, so that AppLayerParserSetTransactionInspectId gets skipped along DetectRunTx

catenacyber · 2024-01-11T21:23:27Z

Quick Sort in most cases is a superior sorting algorithm than Merge Sort. Quick Sort exploits cache locality principles better and choosing the right pivot can make it much better in many aspects although both share an average time complexity of O(nlogn).

First disclaimer : I am not sure qsort implements quick sort
But I am sure that lots of time can be spent in qsort with the reproducer provided in the issues.
Quick sort has a worst case complexity which is quadratic.
I fear we get close to the worst case scenario by having the first list sorted...

I do not know what you call merge sort :-/
Here, we have 2 sorted lists, and we merge them together, basically iterating over them and popping the minimum from either queue...

Merge Sort works by breaking the given n element array down into smaller parts, sorting them and then putting them back together. Now, it's an incredible advantage for merge sort to get already sorted data and that we know where that is. In best cases, it can then work in O(n) [which is why I think Philippe calls it linear?]

Now, the above points fit in our situation because as Victor said tx_candidates entries are already ordered and then after the merge w match_array elements it stays ordered (didn't check this). Hence, giving quite an advantage to merge sort for this case.

Questions that don't make sense to me all the way:

qsort call at the end of this construct still exists. I would have expected that to be replaced by merge sort.

qsort is still needed, because there may be a third source of match candidates, ones coming from stored flags

I have no guarantee that this third source comes ordered...

Some research tells me that Tim Sort is better suited for our usecase here. ref: https://en.wikipedia.org/wiki/Timsort What do you think?

Looks nice, but maybe complex

catenacyber · 2024-01-11T21:24:02Z

Thanks for the reviews. Will fix the dummy warning in next iteration as well

inashivb · 2024-01-12T03:18:43Z

First disclaimer : I am not sure qsort implements quick sort But I am sure that lots of time can be spent in qsort with the reproducer provided in the issues. Quick sort has a worst case complexity which is quadratic. I fear we get close to the worst case scenario by having the first list sorted...

hmm I was aware of the quadratic complexity. But, found it can be avoided by choosing the right pivot. I didn't try to check what qsort does in the background. Thanks. Will check. :)

I do not know what you call merge sort :-/ Here, we have 2 sorted lists, and we merge them together, basically iterating over them and popping the minimum from either queue...

Oh. Based on your commit d8dcc8e, I assumed you were referring to the classic merge sort ref: https://en.wikipedia.org/wiki/Merge_sort then I tried to understand why you called it linear.
Maybe it was a matter of reading it a different way. Thanks for explaining! :)

qsort is still needed, because there may be a third source of match candidates, ones coming from stored flags

I have no guarantee that this third source comes ordered...

ok.

Some research tells me that Tim Sort is better suited for our usecase here. ref: https://en.wikipedia.org/wiki/Timsort What do you think?

Looks nice, but maybe complex

ok. Thank you!

catenacyber · 2024-01-14T21:38:51Z

Replaced by #10160

catenacyber added 4 commits January 9, 2024 21:43

detect: merge sorted lists instead of qsort

d8dcc8e

Ticket: OISF#6299 Simply because it is faster (just linear).

detect: do not store state without flags

e461b80

If flags are zero, there is nothing to store and remember. Stored signatures will be reused on a later packet, and qsorted (which may be expensive), with newer matches candidates. Avoiding to store, leads to avoid the call to qsort.

catenacyber requested review from jasonish and victorjulien as code owners January 9, 2024 21:09

catenacyber mentioned this pull request Jan 9, 2024

Detect tcp noupdatetotx 6299 v8 #10127

Closed

victorjulien reviewed Jan 10, 2024

View reviewed changes

jufajardini reviewed Jan 10, 2024

View reviewed changes

inashivb reviewed Jan 11, 2024

View reviewed changes

catenacyber mentioned this pull request Jan 14, 2024

Detect tcp noupdatetotx 6299 v10 #10160

Closed

catenacyber closed this Jan 14, 2024

		@@ -1296,6 +1296,46 @@ static inline void StoreDetectFlags(DetectTransaction *tx, const uint8_t flow_fl
		}
		}

Conversation

catenacyber commented Jan 9, 2024

Uh oh!

suricata-qa commented Jan 9, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jufajardini left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

inashivb Jan 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

inashivb left a comment

Choose a reason for hiding this comment

Uh oh!

inashivb left a comment

Choose a reason for hiding this comment

Uh oh!

catenacyber commented Jan 11, 2024

Uh oh!

catenacyber commented Jan 11, 2024

Uh oh!

catenacyber commented Jan 11, 2024

Uh oh!

inashivb commented Jan 12, 2024

Uh oh!

catenacyber commented Jan 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

inashivb Jan 11, 2024 •

edited

Loading