-
Notifications
You must be signed in to change notification settings - Fork 170
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
BUG: test 53/55 failures/errors on aarch64 #210
Comments
@drakenclimber I'm not sure if you have access to an aarch64 system (maybe a Raspberry Pi running a 64-bit build?), but I'm going to assign this one to you for obvious reasons. If you don't have access to an aarch64 system let me know and I'll look into this. |
EDIT: sorry, that is error code 14 NOT error 16 (CORRECTED BELOW) I did a little digging just now and it appears that test |
I added some quick instrumentation (below): diff --git a/tests/53-sim-binary_tree.c b/tests/53-sim-binary_tree.c
index 2c7890e..c154f67 100644
--- a/tests/53-sim-binary_tree.c
+++ b/tests/53-sim-binary_tree.c
@@ -81,6 +81,7 @@ int main(int argc, char *argv[])
SCMP_A0(SCMP_CMP_EQ, i));
else
rc = seccomp_rule_add(ctx, SCMP_ACT_ERRNO(i), i, 0);
+ printf("PMD: i=%d, rc=%d\n", i, rc);
if (rc < 0)
goto out;
} ... which resulted in the following output:
|
Interesting. My Raspberry Pi died a couple months ago, but this seems like a good excuse to order a new one. The new 2GB version looks really tempting. Until it arrives, I should be able to find an ARM machine within Oracle. I'll start looking... |
Sounds good. I'll be curious to hear how the new RPi 4 does, I've only got a 3B. Regardless, if you can't find a system let me know. |
aarch64's syscall table is sparse in several places, and thus Here's the exact list of syscall numbers that currently do not exist in aarch64:
I'm not really sure of the best way to proceed. Should we limit test 53 and 55 only to x86_64? I really want to ensure we're testing large, unbalanced binary trees. It seems like in a case like this we can skip these tests. Or perhaps we should make a smaller, simpler test for these architectures. Thoughts? |
Bummer. Although feeding in a bogus syscall number and expecting a valid result isn't quite fair ;) One possibility would be to first make a call to I think it is also worth noting that this is purely a test problem and not a general aarch64 problem. In the real world only valid syscall numbers would be fed into the API with the expectation of proper results. |
I knew I was playing with fire when I did it :). This is a tricky feature to test because the end seccomp behavior shouldn't change when using a binary tree. Only the way we traverse the bpf (or pfc) should change.
I thought about that, but that makes it really difficult to compare against a pre-populated *.pfc file like we're doing in test 55.
Agreed. I need to step back and think about this for a bit. Perhaps there's a way to use some of the bpf tools you have made to help in the testing. |
Perhaps the answer is that for test 55 we use a smaller subset of syscalls (one dozen, two dozen?) such that we can do the number resolution from the syscall name to ensure they exist. |
Yeah, I think that's likely the best and safest answer. So obviously aarch64's syscall table has no holes up until 163. Do you know what the other architecture's tables look like? And do we feel confident that a binary tree of 24 or so syscalls fully tests the code? (Probably more of a question for me than you.) My gut feeling is yes, but I would like to look through it and run it through coveralls as well. |
If only we had a pre-built table of syscall architectures ... oh wait, we do! ;) Example using aarch64, sorted by syscall number:
A test that is unreliable, regardless of it's level of comprehensiveness isn't useful. I would rather have a "good enough" test that is always consistent, everywhere it is run, every time it is run that we can later build upon. |
I had a feeling there was a tool for that. Thanks ;)
Agreed. I got greedy and thought I could create the home run of tests. Oh well, a "good enough" test in this case should be sufficient. I'll get to work on it today. |
Hey, no worries, we've all fallen into that trap from time to time. It's still a nice improvement and we've got at least a basic test; this could be much worse :) |
FWIW, it fails on ppc64 too: batch name: 53-sim-binary_tree |
And 55-basic-pfc_binary_tree too: batch name: 55-basic-pfc_binary_tree |
Hi @piso77, thanks for the report. Once @drakenclimber has a fix would you be willing to test it? Unfortunately, I imagine there may be a few other ABIs that have similar failures; it is simply that aarch64 is the only non-x86 ABI that I have easy access to for testing purposes. The fix we have been discussing in this thread should address all these failures (I think). |
I wrote a quick python script to find the first hole in each architecture. Here are the results:
Based upon these results, I think we should lower tests 53 and 55 to limit out at syscall number 12. Note that there are a few architectures (mips, etc.) that will fail regardless of how low of a number we pick. |
Sure, ping me when there's a patch available! |
How do you plan to address that? |
I was thinking that I would check the native architecture, and if it matched arm, mips*, or x32, then we would skip the tests. Not pretty, but I don't see an easy way to test the binary tree on those systems. |
If we are going to be skipping ABIs based on this, we might as well limit it to just x86_64. Which is pretty bad. I'm not sure if we have a precedence with any of the other tests, but I'm beginning to think the right way to fix this is to use a list of syscall names and not numbers. It's a little more work in the test, but it should avoid this probably completely. NOTE: the syscalls don't have to be valid for that particular ABI so long as they are valid syscall names in our internal syscall table. |
I believe you are correct. This is a special situation.
I'm good with this choice. I'm not yet sure how I can make it work for the pfc case (test 55), but I'll give it a shot! |
It looks like we have a number test failures and errors on aarch64 with the current master branch; HEAD is 38f04da ("tests: add tests for the binary tree").
Test FAILUREs:
Test ERRORs:
The text was updated successfully, but these errors were encountered: