Skip to content

Conversation

@catenacyber
Copy link
Contributor

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

Describe changes:

  • stream/tcp: add config parameter to limit data processed per packet

DRAFT :
Is this the right solution design ?
( I bet third and last commit's code is wrong )

I think so: let's consider the following case with an established flow, client is at sequence 1000

  • we get packets from sequence 2000 to 1M (with a big enough tcp window) : we store them
  • we finally get the packet sequence 1000-2000, we can now process the 1Mbyte data

Proposed solution here is to dilute the work by only processing 65Kbytes or so at a time (per packet), and thus to have multiple pseudo-packets ending the flow if needed

Another solution could be to limit the window we accept (and set an event if a packet is outside our window but inside the flow's window) but Suricata processes less data this way...

Ticket: 6977

When we use max data per packet, we may timeout the flow with
unprocessed data.
In this case, process a chunk with a peudo packet, and do not
erase the flow, until all data has been consumed...
*data_len = 0;
}
}
if (*data_len > UINT16_MAX) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the core of the solution !

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not there in the PR now. Is that on purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is still there, but the condition changed to be configurable

@suricata-qa
Copy link

ERROR:

ERROR: QA failed on ASAN_TLPR1_suri.

Pipeline = 28454

@victorjulien
Copy link
Member

victorjulien commented Nov 13, 2025

I think this logic would have to apply to "raw" content as well, as that is what the detection engine uses for rules with just content w/o buffer. These need to match up to allow rules that are quite common that mix raw content with app buffers.

I guess I also fear we're just pushing the problem to the end of the flow. If we have a large blob of data getting ready for processing in one step, does it really matter if it is "real time" in the packet path or at flow timeout? Both will block the worker thread for other packets.

I don't have an alternative solution to offer at this point, but I am a bit skeptical here.

@catenacyber
Copy link
Contributor Author

I guess I also fear we're just pushing the problem to the end of the flow. If we have a large blob of data getting ready for processing in one step, does it really matter if it is "real time" in the packet path or at flow timeout? Both will block the worker thread for other packets.

I am not sure I understand what you say, but this draft is supposed to solve this problem :
The idea is that the end of the flow becomes 16 pseudo packets, one every minute handling 65K bytes of data, instead of one big packet handling 1Mbyte of data.
We split the work to do some later (which may not be a good idea if we want "real time" results)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants