ltp
ltp copied to clipboard
linux-test-project
syscalls/{f,l,}chown: Don't pass undocumented flags to open and chmod
Some chown-related tests were using undocumented flags, e.g. here:
https://github.com/linux-test-project/ltp/blob/bcf373385e8c06e80b73fb3bbf2cc57ba341ac10/testcases/kernel/syscalls/fchown/fchown02.c#L42-L45 https://github.com/linux-test-project/ltp/blob/bcf373385e8c06e80b73fb3bbf2cc57ba341ac10/testcases/kernel/syscalls/fchown/fchown02.c#L143-L146
man says that mode argument supports only standard mode flags. On Linux, those LTP tests seem to work by an accident, despite passing S_IFREG there - Linux masks mode with 07777 and ignores any other bits, including S_IFREG.
If we are going to remove these flags we also have to add a new tests that call the *chown() syscalls with additional the bits enabled, because otherwise we will loose coverage. That is because if Linux fails to mask the mode somewhere in the future it likely cause a serious regression.
@mkow Well, tests should test the reality, not what man page claims. And if man page is wrong, it should be fixed (http://vger.kernel.org/vger-lists.html#linux-man, https://marc.info/?l=linux-man, https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git).
If we are going to remove these flags we also have to add a new tests that call the *chown() syscalls with additional the bits enabled
I can add this to the tests if you think that this is something which should be tested. In this particular case, the original test intention is clearly wrong - those syscalls doesn't work the way it uses them and those tests weren't intended to test unknown bits. I could add a test with a clear intention to test those other bits, probably just an additional test case to one of the tests for each syscall?
because otherwise we will loose coverage.
This was kind of an "accidental coverage" and tested just one specific bit ;)
That is because if Linux fails to mask the mode somewhere in the future it likely cause a serious regression.
@mkow Well, tests should test the reality, not what man page claims.
I think this depends on the case, i.e. whether there may exist real applications that does the same. E.g., if you'd notice that mmap always allocates addresses divisible by PAGE_SIZE*2 in the current kernel implementation, I assume you wouldn't test this unless there's an existing application that relies on this quirk, so that it de facto became a part of kernel interface because of this app. Same for not failing on undocumented flags - this was rather a bug in Linux which is now unfixable in some of the interfaces (see e.g. the MAP_SHARED_VALIDATE story: https://lwn.net/Articles/758594/). Do you think this interface is the same case? I'm just afraid that LTP may become that app which will make this interface unfixable.
And if man page is wrong, it should be fixed
Why do you think it's wrong? It just lists supported flags, but doesn't define the behavior if you pass some garbage there.
Well LTP has quite a few testcases that test behavior like that, if they start to fail we talk to the kernel maintaners if the change was intentional or not and either decide to fix the kernel or the test. So far we caught a few unintentional interface changes before they managed to slip into a released kernel hence I think that having a test like this makes sense.
Ok, I'm assuming then that it's fine to over-assume things on the LTP side.
What about the other issue? (that the current version of those tests is not trying to test if the kernel ignores the other bits, they seem to work by an accident and are misleading to the reader) Wouldn't it be better to remove these unintended flags and create a separate test which sets ~07777 and checks if everything still works?
Sorry for the delay, I was busy with other stuff, but finally found a moment to finish this PR. I implemented separate tests for ignoring of all additional bits (in a separate commit) + rebased the whole branch to the current master. Please take a look and check if everything's fine - I've never added a test to LTP before, so I might have missed something.
@metan-ucw Any chance to get this merged? Are there any more fixes needed from my side?
Not a complete review, just a note: please don't use empty setup and cleanup functions. If you want to get more audience than just overloaded metan-ucw, send it to our mailing list: https://lists.linux.it/listinfo/ltp https://github.com/linux-test-project/ltp/wiki#developers-corner
The CI just checks if the code could be compiled on different distributions, as writing portable code even for Linux is not easy if you need to cover popular distributions that have been released for last ten years...
Running these tests is not really possible on travis for various reasons, one of them is that the CVE tests would crash the machine unless it has latest patches in kernel, the testsuite would run for too long, etc. I would love to have a CI that actually runs the testcases as well, but we would need a dedicated hardware lab for that.
The CI just checks if the code could be compiled on different distributions, as writing portable code even for Linux is not easy if you need to cover popular distributions that have been released for last ten years...
Running these tests is not really possible on travis for various reasons, one of them is that the CVE tests would crash the machine unless it has latest patches in kernel, the testsuite would run for too long, etc. I would love to have a CI that actually runs the testcases as well, but we would need a dedicated hardware lab for that.
Hmm. One idea would be to run only a subset of the tests (exclude ones requiring root, CVE tests, long-running stress tests etc) and only run them on the most recent kernel, or something like this. It would be far from ideal, but I guess most of the tests aren't too intrusive an would work in this model? It would still require you to control the exact kernel version, which AFAIK is not possible in Travis. Anyways, you've probably already had this discussion multiple times in other places, so I won't mind if you decide to skip this discussion here ;)
Going back to the PR itself, Travis failed with the following message:
recvmmsg01.c: In function 'do_test':
recvmmsg01.c:86:9: error: request for member 'type' in something not a structure or union
timeout.type = tv->ts_type;
^
but I didn't touch this test and I wasn't able to reproduce this locally. Does Travis run on master merged with my branch and master is currently broken? Or I'm missing something?