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

Add other atomic instructions #558

Merged
merged 7 commits into from
Mar 14, 2024
Merged

Add other atomic instructions #558

merged 7 commits into from
Mar 14, 2024

Conversation

dthaler
Copy link
Contributor

@dthaler dthaler commented Jan 4, 2024

  • Add the rest of the atomic instructions defined in https://www.ietf.org/archive/id/draft-ietf-bpf-isa-00.html#name-atomic-operations
  • Add various test cases in atomic.yaml
  • Replace uses of (INST_MEM << 5) etc. with the more readable and less bug-prone INST_MODE_MEM.
  • Make the atomic operation conformance tests (all of which do operations on stack memory not shared memory) pass
  • The CMPXCHG instruction is still not as precise as it could be. Improving its precision is left for future work (filed issue Stack memory tracking improvements #566 to track this issue where a number of conformance tests give TOP or a RANGE instead of the exact value)

Fixes #483

src/crab/ebpf_domain.cpp Outdated Show resolved Hide resolved
@dthaler dthaler force-pushed the atomics branch 2 times, most recently from e1c78be to 8f10aef Compare January 4, 2024 19:52
@coveralls
Copy link

coveralls commented Jan 4, 2024

Coverage Status

coverage: 90.322% (-0.02%) from 90.344%
when pulling e96578f on dthaler:atomics
into fe59c86 on vbpf:main.

Fixes vbpf#483

Signed-off-by: Dave Thaler <[email protected]>
@elazarg
Copy link
Collaborator

elazarg commented Mar 5, 2024

I'm probably missing something, but aren't the instructions atomic because they change memory regions that are already volatile?

@dthaler
Copy link
Contributor Author

dthaler commented Mar 5, 2024

I'm probably missing something, but aren't the instructions atomic because they change memory regions that are already volatile?

Apparently atomic operations can also work on stack memory. Not that you need atomic operations for them, but if you do them they shouldn't fail verification per se (and don't on Linux).

Copy link
Collaborator

@elazarg elazarg left a comment

Choose a reason for hiding this comment

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

Looks good in general. See my question about the choice of fields though.

src/asm_syntax.hpp Outdated Show resolved Hide resolved
src/crab/ebpf_domain.cpp Show resolved Hide resolved
src/crab/ebpf_domain.cpp Outdated Show resolved Hide resolved
src/crab/ebpf_domain.cpp Outdated Show resolved Hide resolved
src/crab/ebpf_domain.cpp Outdated Show resolved Hide resolved
src/asm_syntax.hpp Show resolved Hide resolved
Signed-off-by: Dave Thaler <[email protected]>
@dthaler dthaler requested a review from elazarg March 14, 2024 20:41
@dthaler
Copy link
Contributor Author

dthaler commented Mar 14, 2024

Looks good in general. See my question about the choice of fields though.

I think I've now made changes according to all your comments. Please re-review. Thanks!

Copy link
Collaborator

@elazarg elazarg left a comment

Choose a reason for hiding this comment

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

LGTM. Just a minor comment and a question.

src/asm_parse.cpp Outdated Show resolved Hide resolved
src/test/test_marshal.cpp Outdated Show resolved Hide resolved
src/asm_syntax.hpp Outdated Show resolved Hide resolved
Signed-off-by: Dave Thaler <[email protected]>
@elazarg elazarg merged commit a7c4cfc into vbpf:main Mar 14, 2024
13 checks passed
Alan-Jowett pushed a commit to Alan-Jowett/ebpf-verifier that referenced this pull request Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for BPF_FETCH in atomic operations
3 participants