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

syscalls/{f,l,}chown: Don't pass undocumented flags to open and chmod #671

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mkow
Copy link
Contributor

@mkow mkow commented Apr 23, 2020

Some chown-related tests were using undocumented flags, e.g. here:

#define FILE_MODE S_IFREG | S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH
#define NEW_PERMS1 S_IFREG | S_IRWXU | S_IRWXG | S_ISUID | S_ISGID
#define NEW_PERMS2 S_IFREG | S_IRWXU | S_ISGID
#define EXP_PERMS S_IFREG | S_IRWXU | S_IRWXG

fd1 = SAFE_OPEN(cleanup, TESTFILE1, O_RDWR | O_CREAT, FILE_MODE);
SAFE_CHMOD(cleanup, TESTFILE1, NEW_PERMS1);
fd2 = SAFE_OPEN(cleanup, TESTFILE2, O_RDWR | O_CREAT, FILE_MODE);
SAFE_CHMOD(cleanup, TESTFILE2, NEW_PERMS2);

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.

@metan-ucw
Copy link
Member

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.

@pevik
Copy link
Member

pevik commented Apr 24, 2020

@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).

@mkow
Copy link
Contributor Author

mkow commented Apr 24, 2020

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.

@mkow
Copy link
Contributor Author

mkow commented Jul 20, 2020

@metan-ucw @pevik friendly ping ;)

@metan-ucw
Copy link
Member

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.

@mkow
Copy link
Contributor Author

mkow commented Jul 20, 2020

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?

@metan-ucw
Copy link
Member

Yes the fucntional test should be separated like that.

mkow added 2 commits August 20, 2020 00:05
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.
@mkow mkow force-pushed the mkow/fix-invalid-open-mode branch from 005983d to 66987ed Compare August 19, 2020 22:08
@mkow
Copy link
Contributor Author

mkow commented Aug 19, 2020

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.

@mkow
Copy link
Contributor Author

mkow commented Sep 21, 2020

@metan-ucw Any chance to get this merged? Are there any more fixes needed from my side?

@pevik
Copy link
Member

pevik commented Sep 22, 2020

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

TEST(open(tc->filename, tc->flags, tc->mode));
fd = TST_RET;
if (fd == -1) {
tst_res(TFAIL, "Cannot open the file");
Copy link
Member

Choose a reason for hiding this comment

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

Two minor things: we use TTERRNO to print errno on failure. And we usually prefer to use return than have else block (readability).

if (fd == -1) {
	tst_res(TFAIL | TTERRNO, "Cannot open the file");
	return;
}

tst_res(TPASS, "Unknown mode bits were ignored as expected");
SAFE_CLOSE(fd);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

if (stat(TESTFILE, &stat_buf) == -1) {
tst_brk(TFAIL | TTERRNO, "stat failed");
}

Copy link
Member

Choose a reason for hiding this comment

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

We do have a safe macros to simplify error handling. So this if () could be replaced and is equivalent to just:

SAFE_STAT(TESTFILE, &stat_buf);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.
Should I also use SAFE_CHMOD above? From what I see these macros are used only for things which shouldn't fail / are unrelated to the test itself, because they use TBROK instead of TFAIL. So, I guess the answer is "no"?

#define CHMOD_MODE (0777 | ~07777)
#define TESTFILE "testfile"

void test_chmod(void)
Copy link
Member

Choose a reason for hiding this comment

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

Local functions like these should be declared static, but that is very minor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied this from some other chmod test and didn't notice the problem, sorry. Should be fixed now.

Copy link
Contributor Author

@mkow mkow left a comment

Choose a reason for hiding this comment

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

Not a complete review, just a note: please don't use empty setup and cleanup functions.

Fixed.

If you want to get more audience than just overloaded metan-ucw, send it to our mailing list:

I didn't know this when I started this PR, I'll submit my future PRs to the list. For this one I think it would be better to finish reviewing it here to keep the whole discussion at one place.

btw. There's an issue with openat204 test: none of my OSes support openat2 syscall and I thought your CI takes care of this, but now I'm not that sure - I just fixed a bug in it which should have made it failing before, but I remember Travis passing. So, is openat2 actually tested in your CI?

#define CHMOD_MODE (0777 | ~07777)
#define TESTFILE "testfile"

void test_chmod(void)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied this from some other chmod test and didn't notice the problem, sorry. Should be fixed now.

TEST(open(tc->filename, tc->flags, tc->mode));
fd = TST_RET;
if (fd == -1) {
tst_res(TFAIL, "Cannot open the file");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

if (stat(TESTFILE, &stat_buf) == -1) {
tst_brk(TFAIL | TTERRNO, "stat failed");
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.
Should I also use SAFE_CHMOD above? From what I see these macros are used only for things which shouldn't fail / are unrelated to the test itself, because they use TBROK instead of TFAIL. So, I guess the answer is "no"?

@metan-ucw
Copy link
Member

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.

@mkow
Copy link
Contributor Author

mkow commented Sep 25, 2020

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?

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.

3 participants