Skip to content
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

Smb hashmap/v10 #12087

Closed
wants to merge 11 commits into from
Closed

Smb hashmap/v10 #12087

wants to merge 11 commits into from

Conversation

victorjulien
Copy link
Member

LruCache use for all hashmaps in smb state. This will bound each of them.

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

Replaces #12036, update docs.

Don't tag the session as gap'd when the GAP is in a precise location:

1. in "skip" data, where the GAP just fits the skip data

2. in file data, where we pass the GAP on to the file

This reduces load of GAP post-processing that is unnecessary in these
case.
Use `lru` crate. Rename to reflect this.

Add `app-layer.protocols.smb.max-guid-cache-size` to control the max
size of the LRU cache.

Ticket: OISF#5672.
Rename to read_offset_cache.

Add `app-layer.protocols.smb.max-read-offset-cache-size` option to
control the limit.

Ticket: OISF#5672.
Turn the map mapping the smb session key to smb tree into a lru cache,
limited to 1024 by default.

Add `app-layer.protocols.smb.max-tree-cache-size` option to control the
limit.

Ticket: OISF#5672.
Reimplement the ssnguid2vec_map HashMap as a LruCache.

Since this is a DCERPC record cache, name it as such.

Default size is 128. Can be controlled by
`app-layer.protocols.smb.max-dcerpc-frag-cache-size`.

Ticket: OISF#5672.
Generic ssn2vec_map was a HashMap used for mapping session key to
different types of vector data:
- GUID
- filename
- share name

Turn this into a bounded LruCache. Rename to ssn2vec_cache.

Size of the cache is 512 by default, and can be configured using:

`app-layer.protocols.smb.max-session-cache-size`

Ticket: OISF#5672.
Copy link

codecov bot commented Nov 5, 2024

Codecov Report

Attention: Patch coverage is 86.58147% with 42 lines in your changes missing coverage. Please review.

Project coverage is 83.24%. Comparing base (b1e7917) to head (2a46ce3).
Report is 31 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #12087      +/-   ##
==========================================
- Coverage   83.37%   83.24%   -0.14%     
==========================================
  Files         910      910              
  Lines      257556   257585      +29     
==========================================
- Hits       214748   214417     -331     
- Misses      42808    43168     +360     
Flag Coverage Δ
fuzzcorpus 61.15% <86.26%> (-0.40%) ⬇️
livemode 19.41% <3.19%> (+<0.01%) ⬆️
pcap 44.41% <73.48%> (-0.10%) ⬇️
suricata-verify 62.73% <74.44%> (-0.03%) ⬇️
unittests 59.35% <15.01%> (-0.01%) ⬇️

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

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 23251

Copy link
Member

@jasonish jasonish left a comment

Choose a reason for hiding this comment

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

The code is looking pretty good. My main concerns are the unknowns... We've gone from unbounded to perhaps some tight bounds. For what scenarios could this cause issues? How to know what might need to be increased if there are issues? How to even know if there are issues?

@victorjulien
Copy link
Member Author

The code is looking pretty good. My main concerns are the unknowns... We've gone from unbounded to perhaps some tight bounds. For what scenarios could this cause issues? How to know what might need to be increased if there are issues? How to even know if there are issues?

In some follow up I would like to add events and counters. Events aren't hard, I think. Though they would be along the lines of the most current addition evicted the oldest entry in the hash. Counters would give some insight into how common this is.

@victorjulien victorjulien added this to the 8.0 milestone Nov 6, 2024
@victorjulien
Copy link
Member Author

Merged in #12094, thanks!

@victorjulien
Copy link
Member Author

The code is looking pretty good. My main concerns are the unknowns... We've gone from unbounded to perhaps some tight bounds. For what scenarios could this cause issues? How to know what might need to be increased if there are issues? How to even know if there are issues?

In some follow up I would like to add events and counters. Events aren't hard, I think. Though they would be along the lines of the most current addition evicted the oldest entry in the hash. Counters would give some insight into how common this is.

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

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.

3 participants