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

Make prime conformance test pass #423

Merged
merged 3 commits into from
Dec 19, 2022
Merged

Make prime conformance test pass #423

merged 3 commits into from
Dec 19, 2022

Conversation

dthaler
Copy link
Contributor

@dthaler dthaler commented Nov 21, 2022

Fixes #416

prime.data now returns either true or false, passing verification.
The best answer would be to know it always returns true, but it is at least not unsafe now.

Signed-off-by: Dave Thaler dthaler@microsoft.com

@coveralls
Copy link

coveralls commented Nov 21, 2022

Coverage Status

Coverage increased (+0.08%) to 84.49% when pulling 5c03762 on dthaler:prime-test into 012f13f on vbpf:main.

@elazarg elazarg requested a review from caballa November 21, 2022 21:48
@caballa
Copy link
Collaborator

caballa commented Dec 1, 2022

The fix looks good to me. You probably need to do the same for the narrowing phase (https://github.com/vbpf/ebpf-verifier/blob/main/src/crab/fwd_analyzer.cpp#L206) but I would try to debug first. Probably in this case the analyzer won't be unsound but it might not be effective.

The reason why took me longer to reply was to see why the analyzer cannot prove the program. See next message

@caballa
Copy link
Collaborator

caballa commented Dec 1, 2022

This is the essence of the program:

    x = 2;
L:  x++;
    if (x u< 67) goto L else exit

After widening we infer that x=[2,oo]. Then, since we try to model machine arithmetic after the increment we assume that x can overflow so we lose the lower bound. What it's surprising to me is that the transfer function for x u< 67 doesn't take any effect. We would cover the case for x u<= 67 (see https://github.com/vbpf/ebpf-verifier/blob/main/src/crab/ebpf_domain.cpp#L197) but not the strict inequality.

In any case, the problem is that the widening is too aggressive because it jumps directly to -oo or +oo. In ebpf programs we expect that loops are bounded to relatively small numbers so during widening we could jump to smaller numbers (e.g., 1000, 5000, etc) and of course jumping to +oo if convergence is not achieved after few iterations. In this way, we wouldn't give up so early by assuming that arithmetic operations can overflow.
In Crab, we call it widening with thresholds and in fact Prevail borrrowed the code (https://github.com/vbpf/ebpf-verifier/blob/main/src/crab/thresholds.hpp) but AFIK the code is unused. That code actually tries to learn some good integers to jump in during widening by looking at the code. But it might be enough something much simpler. It might be enough to change slightly the interval and zones domains to jump to a small (predefined) number of integers before jump to +oo. With that I think we should handle this example.

Fixes vbpf#416

There may be a better fix that makes the test pass, but for now
the fix implied by Jorge resolves the issue of an incorrect computation.

Signed-off-by: Dave Thaler <dthaler@microsoft.com>
Signed-off-by: Dave Thaler <dthaler@microsoft.com>
@dthaler dthaler changed the title Make prime conformance test fail verification Make prime conformance test pass Dec 12, 2022
@dthaler
Copy link
Contributor Author

dthaler commented Dec 12, 2022

@elazarg any idea why the analyze pass failed? The error looks unrelated to this PR.

@dthaler
Copy link
Contributor Author

dthaler commented Dec 13, 2022

@elazarg any idea why the analyze pass failed? The error looks unrelated to this PR.

Looks to be due to this change in Linux: https://stackoverflow.com/questions/71454588/minsigstksz-error-after-update-in-my-manjaro-linux and covered by catch2 here: catchorg/Catch2#2421

Signed-off-by: Dave Thaler <dthaler@microsoft.com>
@dthaler
Copy link
Contributor Author

dthaler commented Dec 13, 2022

@elazarg Ok I fixed the analyze problem with catch2 by updating to use the latest header which fixed both this issue and the one you previously had to work around by editing catch.hpp. Anything else needed before merging?

@elazarg elazarg merged commit af7301b into vbpf:main Dec 19, 2022
@dthaler dthaler deleted the prime-test branch December 20, 2022 00:58
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.

Intervals might be overly narrowed
4 participants