flow-manager: fix off-by-one in flow_hash row allocation

[awelzel]

Make sure these boxes are signed before submitting your Pull Request -- thank you.

• I have read the contributing guide lines at https://redmine.openinfosecfoundation.org/projects/suricata/wiki/Contributing
• I have signed the Open Information Security Foundation contribution agreement at https://suricata.io/about/contribution-agreement/
• I have updated the user guide (in doc/userguide/) to reflect the changes made (if applicable)

Link to redmine ticket: 5073.

Describe changes:

The current code doesn't cover all rows when more than one flow manager is
used. It leaves a single row between ftd->max and ftd->min of the next
manager orphaned. As an example:

hash_size=1000
flowmgr_number=3
range=333

instance  ftd->min  ftd->max
0         0         333
1         334       666
2         667       1000

Rows not covered: 333, 666

#suricata-verify-pr:
#suricata-verify-repo:
#suricata-verify-branch:
#suricata-update-pr:
#suricata-update-repo:
#suricata-update-branch:
#libhtp-pr:
#libhtp-repo:
#libhtp-branch:

[victorjulien]

Thanks for your work and analysis. A ticket would be great as we use it to track backports to stable branches and to generate release notes.

[codecov]

Codecov Report

Merging #6993 (2a2a800) into master (b5166bd) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #6993      +/-   ##
==========================================
- Coverage   77.70%   77.66%   -0.04%     
==========================================
  Files         628      628              
  Lines      185656   185653       -3     
==========================================
- Hits       144270   144195      -75     
- Misses      41386    41458      +72     

Flag	Coverage Δ
fuzzcorpus	58.01% <0.00%> (-0.34%)	⬇️
suricata-verify	54.43% <100.00%> (-0.03%)	⬇️
unittests	63.03% <0.00%> (+<0.01%)	⬆️

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

[awelzel]

Thanks for your work and analysis. A ticket would be great as we use it to track backports to stable branches and to generate release notes.

I've created 5073.

[inashivb]

Thank you @awelzel for your first contribution to our project! :)
Just adding my thoughts around rounding of range that Victor asked to check.

hash size = 9000
thread count = 57
range (uint32_t) = 9000/57 = 157.8... but stored as 157
But, range x thread_count (our max) = 157 x 57 = 8949
So, we're short of 9000 - 8949 = 51

Although, with the patch, while we're at the last stage when thread count == 57, we'll set the max to 9000 so seems all will be OK..

[victorjulien]

I think if rounding errors can get us into a state of missing rows, or assigning duplicate rows to threads, we need a new approach.

[awelzel]

Thank you @awelzel for your first contribution to our project! :)

Thanks!

Although, with the patch, while we're at the last stage when thread count == 57, we'll set the max to 9000 so seems all will be OK..

Yes, so that was the intention. The last flow-manager just picks up all remaining rows. As in the comment, it'll deal with hash_size % flowmgr_number more rows than the others.

Now, with hash_size of 9000 and 57 flow managers that looks like a lot: Every manager is allocated 157 rows, while the last one deals with 208 rows (~30% more).

However, seems like hash_size 9000 and 57 flow manager isn't a practical setting one would run.

With hash_size 65536 (default) and 7 flow managers (still quite high?), it's 9362 rows for flow-managers 1-6, and 9364 for manager 7, which is only 2 rows more and probably negligible.

[awelzel]

That logic/idea is already in place already in the current code. It's just more explicit now.

[awelzel]

@victorjulien / @inashivb - is there anything more you'd need input on for this PR?

[victorjulien]

Merged in #7187, thanks!