Smb hashmap/v9 #12036

victorjulien · 2024-10-25T14:48:59Z

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

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

Replaces #12028.

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.

Ticket: OISF#5672.

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.

Ticket: OISF#5672.

codecov · 2024-10-25T15:16:42Z

Codecov Report

Attention: Patch coverage is 86.90096% with 41 lines in your changes missing coverage. Please review.

Project coverage is 83.40%. Comparing base (89aa525) to head (2684860).
Report is 4 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #12036      +/-   ##
==========================================
- Coverage   83.42%   83.40%   -0.02%     
==========================================
  Files         910      910              
  Lines      257642   257671      +29     
==========================================
- Hits       214934   214922      -12     
- Misses      42708    42749      +41

Flag	Coverage Δ
fuzzcorpus	`61.55% <86.58%> (-0.02%)`	⬇️
livemode	`19.41% <3.19%> (+<0.01%)`	⬆️
pcap	`44.40% <73.48%> (-0.10%)`	⬇️
suricata-verify	`62.72% <74.44%> (-0.05%)`	⬇️
unittests	`59.36% <15.01%> (-0.01%)`	⬇️

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

suricata-qa · 2024-10-25T20:10:02Z

Information: QA ran without warnings.

Pipeline 23185

jasonish · 2024-10-30T14:31:03Z

rust/src/smb/smb.rs

+                SCLogError!("Invalid max-guid-cache-size value");
+            }
+        }
+        let retval = conf_get("app-layer.protocols.smb.max-read-offset-cache-size");


Looks like some get_as_usize could be useful here :)

Can be done as a future optimization.

doc/userguide/configuration/suricata-yaml.rst

catenacyber · 2024-10-30T21:20:26Z

rust/src/smb/smb.rs

@@ -80,6 +81,16 @@ pub static mut SMB_CFG_MAX_READ_QUEUE_CNT: u32 = 64;
 pub static mut SMB_CFG_MAX_WRITE_SIZE: u32 = 16777216;
 pub static mut SMB_CFG_MAX_WRITE_QUEUE_SIZE: u32 = 67108864;
 pub static mut SMB_CFG_MAX_WRITE_QUEUE_CNT: u32 = 64;
+/// max size of the per state guid2name cache
+pub static mut SMB_CFG_MAX_GUID_CACHE_SIZE: usize = 1024;


side note, but it would be good to have something like lazy_static to say, yes this is a global variable, but once initialization is done, it is a constant and it can be read safely...

Can be done as a future optimization.

catenacyber · 2024-10-30T21:23:36Z

rust/src/smb/smb.rs

@@ -28,13 +28,14 @@
 use std;
 use std::str;
 use std::ffi::{self, CString};
-
-use std::collections::HashMap;


This looks great overall

catenacyber · 2024-10-30T21:30:19Z

rust/src/smb/smb.rs

 pub struct SMBState<> {
    pub state_data: AppLayerStateData,

    /// map ssn/tree/msgid to vec (guid/name/share)
-    pub ssn2vec_map: HashMap<SMBCommonHdr, Vec<u8>>,
+    pub ssn2vec_cache: LruCache<SMBCommonHdr, Vec<u8>>,


Next question after this work :
So, this work bounds the number of elements by flow (great), but is the size of each element bounded ? Can this Vec and other ones like guid2name_cache: LruCache<Vec<u8>, Vec<u8>> have arbitrary big vectors ?

Good question for a follow up ticket.

catenacyber · 2024-10-30T21:34:13Z

This looks great overall.
I did not look yet into the details of whether we are removing elements from the caches every time we should. (wondering about gaps, and when only one side of the traffic is seen)
How did you test this ?

jasonish · 2024-11-01T16:05:50Z

doc/userguide/configuration/suricata-yaml.rst

+
+::
+
+    smb:
+      max-guid-cache-size: 1024
+      max-rec-offset-cache-size: 128
+      max-tree-cache-size: 512
+      max-dcerpc-frag-cache-size: 128
+      max-session-cache-size: 512
+
+The `max-guid-cache-size` setting controls the size of the hash that maps the GUID to
+filenames. These are added through CREATE commands and removed by CLOSE commands.
+
+`max-rec-offset-cache-size` controls the size of the hash that maps the READ offset
+from READ commands to the READ responses.
+
+The `max-tree-cache-size` option contols the size of the SMB session to SMB tree hash.
+
+`max-dcerpc-frag-cache-size` controls the size of the hash that tracks partial DCERPC
+over SMB records. These are buffered in this hash to only parse the DCERPC record when
+it is fully reassembled.
+
+The `max-session-cache-size` setting controls the size of a generic hash table that maps
+SMB session to filenames, GUIDs and share names.


Any additional comments here on the suitability of the defaults would be great.. Conservative? Good for average use? etc. Why they were chosen?

Fairly random. I don't have a good env to test this stuff with. Want to make some ways to observe the sizes.

victorjulien · 2024-11-02T08:16:43Z

This looks great overall. I did not look yet into the details of whether we are removing elements from the caches every time we should. (wondering about gaps, and when only one side of the traffic is seen) How did you test this ?

Large pcaps with a debug print to see the sizes, and it stays in bounds.

victorjulien · 2024-11-05T11:20:39Z

Replaced by #12087

victorjulien added 11 commits October 25, 2024 16:29

rust: update crates

927e1b3

smb2: use if let for read/write parsing

4f4726f

smb: update to GAP handling

f901bd9

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.

smb2: remove filename on close

8331313

Ticket: OISF#5672.

smb1: remove name on close

77b38d6

Ticket: OISF#5672.

smb: use lru for guid2name map; rename

770e59b

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.

smb: use lru for ssn2vecoffset_map; rename

64e314d

Rename to read_offset_cache. Add `app-layer.protocols.smb.max-read-offset-cache-size` option to control the limit. Ticket: OISF#5672.

smb: use lru for ssn2tree; rename

fe9a98f

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.

smb: use lru for ssnguid2vec_map; rename

2bcb393

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.

doc/userguide: document smb cache size limit options

2684860

Ticket: OISF#5672.

victorjulien requested review from jasonish and jufajardini as code owners October 25, 2024 14:49

victorjulien mentioned this pull request Oct 25, 2024

Smb hashmap/v6 #12028

Closed

jasonish reviewed Oct 30, 2024

View reviewed changes

catenacyber reviewed Oct 30, 2024

View reviewed changes

doc/userguide/configuration/suricata-yaml.rst Show resolved Hide resolved

catenacyber reviewed Oct 30, 2024

View reviewed changes

jasonish reviewed Nov 1, 2024

View reviewed changes

victorjulien mentioned this pull request Nov 5, 2024

Smb hashmap/v10 #12087

Closed

victorjulien closed this Nov 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Smb hashmap/v9 #12036

Smb hashmap/v9 #12036

victorjulien commented Oct 25, 2024

codecov bot commented Oct 25, 2024 •

edited

Loading

suricata-qa commented Oct 25, 2024

jasonish Oct 30, 2024

victorjulien Nov 5, 2024

catenacyber Oct 30, 2024

victorjulien Nov 5, 2024

catenacyber Oct 30, 2024

catenacyber Oct 30, 2024

victorjulien Nov 5, 2024

catenacyber commented Oct 30, 2024

jasonish Nov 1, 2024

victorjulien Nov 2, 2024

victorjulien commented Nov 2, 2024

victorjulien commented Nov 5, 2024

Smb hashmap/v9 #12036

Smb hashmap/v9 #12036

Conversation

victorjulien commented Oct 25, 2024

codecov bot commented Oct 25, 2024 • edited Loading

Codecov Report

suricata-qa commented Oct 25, 2024

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 Oct 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victorjulien commented Nov 2, 2024

victorjulien commented Nov 5, 2024

codecov bot commented Oct 25, 2024 •

edited

Loading