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

mkow · 2020-04-23T23:04:15Z

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

ltp/testcases/kernel/syscalls/fchown/fchown02.c

Lines 42 to 45 in bcf3733

    
           #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

ltp/testcases/kernel/syscalls/fchown/fchown02.c

Lines 143 to 146 in bcf3733

    
           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 · 2020-04-24T09:18:05Z

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 · 2020-04-24T10:59:53Z

@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 · 2020-04-24T12:34:18Z

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 · 2020-07-20T10:54:37Z

@metan-ucw @pevik friendly ping ;)

metan-ucw · 2020-07-20T15:05:20Z

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 · 2020-07-20T15:19:39Z

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 · 2020-07-20T15:22:06Z

Yes the fucntional test should be separated like that.

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 · 2020-08-19T22:11:29Z

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 · 2020-09-21T23:14:24Z

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

pevik · 2020-09-22T07:02:11Z

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

pevik · 2020-09-22T07:08:01Z

testcases/kernel/syscalls/open/open15.c

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


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);

metan-ucw · 2020-09-22T09:22:12Z

testcases/kernel/syscalls/chmod/chmod08.c

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


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);

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 · 2020-09-22T09:24:15Z

testcases/kernel/syscalls/chmod/chmod08.c

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


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

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

mkow

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?

mkow · 2020-09-22T22:58:02Z

testcases/kernel/syscalls/chmod/chmod08.c

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


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

mkow · 2020-09-22T23:11:56Z

testcases/kernel/syscalls/open/open15.c

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


mkow · 2020-09-22T23:12:03Z

testcases/kernel/syscalls/chmod/chmod08.c

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


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 · 2020-09-25T13:05:26Z

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 · 2020-09-25T18:46:32Z

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?

mkow added 2 commits August 20, 2020 00:05

Test ignoring of unknown bits in open/openat/openat2/chmod mode

66987ed

mkow force-pushed the mkow/fix-invalid-open-mode branch from 005983d to 66987ed Compare August 19, 2020 22:08

pevik reviewed Sep 22, 2020

View reviewed changes

metan-ucw reviewed Sep 22, 2020

View reviewed changes

fixup! Test ignoring of unknown bits in open/openat/openat2/chmod mode

279f4fa

mkow commented Sep 23, 2020

View reviewed changes

metan-ucw force-pushed the master branch from 9e52a6c to 735a69e Compare November 13, 2020 15:28

wangli5665 force-pushed the master branch from b28d946 to 3490c28 Compare June 16, 2023 02:50

pevik force-pushed the master branch from 5cfb7f5 to 3fe59ef Compare May 7, 2024 17:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mkow commented Apr 23, 2020

metan-ucw commented Apr 24, 2020

pevik commented Apr 24, 2020 •

edited

mkow commented Apr 24, 2020

mkow commented Jul 20, 2020

metan-ucw commented Jul 20, 2020

mkow commented Jul 20, 2020

metan-ucw commented Jul 20, 2020

mkow commented Aug 19, 2020

mkow commented Sep 21, 2020

pevik commented Sep 22, 2020

pevik Sep 22, 2020

mkow Sep 22, 2020

metan-ucw Sep 22, 2020

mkow Sep 22, 2020

metan-ucw Sep 22, 2020

mkow Sep 22, 2020

mkow left a comment •

edited

mkow Sep 22, 2020

mkow Sep 22, 2020

mkow Sep 22, 2020

metan-ucw commented Sep 25, 2020

mkow commented Sep 25, 2020

	#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);

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

Are you sure you want to change the base?

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

Conversation

mkow commented Apr 23, 2020

metan-ucw commented Apr 24, 2020

pevik commented Apr 24, 2020 • edited

mkow commented Apr 24, 2020

mkow commented Jul 20, 2020

metan-ucw commented Jul 20, 2020

mkow commented Jul 20, 2020

metan-ucw commented Jul 20, 2020

mkow commented Aug 19, 2020

mkow commented Sep 21, 2020

pevik commented Sep 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkow left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metan-ucw commented Sep 25, 2020

mkow commented Sep 25, 2020

pevik commented Apr 24, 2020 •

edited

mkow left a comment •

edited