Improve performance of exec-hash #3752
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
NDStrahilevitz
commented
Dec 11, 2023
NDStrahilevitz
force-pushed
the
nadav_exec_hash_perf
branch
from
6dade93
to
c2ebbb2
Compare
AlonZivony
requested changes
Dec 14, 2023
NDStrahilevitz
force-pushed
the
nadav_exec_hash_perf
branch
from
c2ebbb2
to
3231fef
Compare
rafaeldtinoco
approved these changes
Dec 19, 2023
There was a problem hiding this comment.
LGTM. It is missing some public methods documentation lines, but other than that (and minors from Alon) I think we're good to merge. Also, nice job in using SIMD optimized SHA package, it has multiple x86 flavors (SSE, ...) and arm as well (I wonder if it supports SVE extensions in arm, would make it super fast).
It demonstrate that the current algorithm of addBucketItem with multiple lock/unlock) is optimal compared to a single lock/unlock. goos: linux goarch: amd64 pkg: github.com/aquasecurity/tracee/pkg/bucketscache cpu: 12th Gen Intel(R) Core(TM) i7-12700H BenchmarkAddBucketItemCurrent-20 464 2586038 ns/op 2118 B/op 5 allocs/op BenchmarkAddBucketItemWithOneLock-20 122 9461730 ns/op 424 B/op 1 allocs/op
GetBucket was not thread safe since it was returning a reference to a bucket instead of its copy.
First call to getFileHash() for a given filename was returning an empty string. Context: aquasecurity#3745
Improve the computeFileHash function, by using minio/sha256-simd. Benchmarking indicates the current implementation is nearly three times faster than the old version (21,311 ns/op vs. 61,857 ns/op) while maintaining similar memory efficiency (33,040 B/op vs. 33,056 B/op) and the same number of memory allocations per operation (5 allocs/op). goos: linux goarch: amd64 pkg: github.com/aquasecurity/tracee/pkg/ebpf cpu: 12th Gen Intel(R) Core(TM) i7-12700H BenchmarkComputeFileHashOld-20 19257 61857 ns/op 33056 B/op 5 allocs/op BenchmarkComputeFileHashCurrent-20 56769 21311 ns/op 33040 B/op 5 allocs/op
The user can now choose between the following options: - none (default) - inode - dev-inode - digest-inode 'inode' option recalculates the file hash if the inode's creation time (ctime) differs, which can occur in different namespaces even for identical pathnames. 'dev-inode' option generally offers better performance compared to the pathname option, as it bypasses the need for recalculation by associating the creation time (ctime) with the device (dev) and inode pair. 'digest-inode' option is the most efficient, as it keys the hash to a pair consisting of the container image digest and inode. This approach, however, necessitates container enrichment. Code related to executable hashing was opportunistically refactored and moved to its own package. Co-authored-by: Geyslan Gregório <geyslan@gmail.com>
Previous implementation of sha256 hash computation used io.Copy. This method would allocate small buffers as needed when copying the file content into the hash digest. The result would be more allocations and more GC calls. Mitigate this by using one large buffer and clearing it per call. Benchmark results: goos: linux goarch: amd64 pkg: github.com/aquasecurity/tracee/pkg/exechash cpu: AMD EPYC 7571 BenchmarkComputeFileHashOld-8 18355 65371 ns/op 33056 B/op 5 allocs/op BenchmarkComputeFileHashCurrent-8 30561 41714 ns/op 272 B/op 4 allocs/op
NDStrahilevitz
force-pushed
the
nadav_exec_hash_perf
branch
from
3231fef
to
9b56ded
Compare
yanivagman
reviewed
Dec 19, 2023
| @@ -48,7 +48,10 @@ Other options: | |||
| - **exec-env**: When tracing execve/execveat, show the environment variables that were used for execution. | |||
| - **relative-time**: Use relative timestamp instead of wall timestamp for events. | |||
| - **exec-hash**: When tracing some file related events, show the file hash (sha256). | |||
| - Affected events: sched_process_exec, shared_object_loaded | |||
| - Affected events: *sched_process_exec*, *shared_object_loaded* | |||
| - **inode** option recalculates the file hash if the inode's creation time (ctime) differs, which can occur in different namespaces even for identical inode. This option is performant, but not recommended and should only be used if container enrichment can't be enabled for digest-inode, and if performance is preffered over correctness. | |||
There was a problem hiding this comment.
Ctime is inode change time, not creation time
| - Affected events: sched_process_exec, shared_object_loaded | ||
| - Affected events: *sched_process_exec*, *shared_object_loaded* | ||
| - **inode** option recalculates the file hash if the inode's creation time (ctime) differs, which can occur in different namespaces even for identical inode. This option is performant, but not recommended and should only be used if container enrichment can't be enabled for digest-inode, and if performance is preffered over correctness. | ||
| - **dev-inode** option generally offers better performance compared to the **inode** option, as it bypasses the need for recalculation by associating the creation time (ctime) with the device (dev) and inode pair. It's recommended if correctness is preffered over performance without container enrichment. |
There was a problem hiding this comment.
You say this option offers better performance, while on the end of this sentence it says that this option is more for correctness, and not performance. This is confusing
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
1. Explain what the PR does
c2ebbb2 feat(exechash): optimize hash by reusing buffer
9f9e5a4 feat(ebpf): expand exec-hash output option
eb17fc6 feat(ebpf): optimize computeFileHash
db51e0b fix(ebpf): fix getFileHash()
c469486 fix(bucketscache): GetBucket was not thread safe
041bcff chore(bucketscache): add benchmark
2. Explain how to test it
Added tests to confirm parity with previous hash functionality.
Can be manually tested by using:
tracee -o option:exec-hash={inode|dev-inode|digest-inode}where you may choose which hash option you want, with varying degrees of accuracy and performance.
Close #3734
Close #3745