Hello! On Fri, Aug 18, 2023 at 07:06:07PM +0200, Mickaël Salaün wrote: > On Mon, Aug 14, 2023 at 07:28:13PM +0200, Günther Noack wrote: > > @@ -3639,7 +3639,7 @@ TEST_F_FORK(ftruncate, open_and_ftruncate) > > }; > > int fd, ruleset_fd; > > > > - /* Enable Landlock. */ > > + /* Enables Landlock. */ > > ruleset_fd = create_ruleset(_metadata, variant->handled, rules); > > ASSERT_LE(0, ruleset_fd); > > enforce_ruleset(_metadata, ruleset_fd); > > @@ -3732,6 +3732,96 @@ TEST(memfd_ftruncate) > > ASSERT_EQ(0, close(fd)); > > } > > We should also check with O_PATH to make sure the correct error is > returned (and not EACCES). Is this remark referring to the code before it or after it? My interpretation is that you are asking to test that test_fioqsize_ioctl() will return errnos correctly? Do I understand that correctly? (I think that would be a little bit overdone, IMHO - it's just a test utility of ~10 lines after all, which is below the threshold where it can be verified by staring at it for a bit. :)) > > +/* Invokes the FIOQSIZE ioctl(2) and returns its errno or 0. */ > > +static int test_fioqsize_ioctl(int fd) > > +{ > > + loff_t size; > > + > > + if (ioctl(fd, FIOQSIZE, &size) < 0) > > + return errno; > > + return 0; > > +} > > + dir_s1d1_fd = open(dir_s1d1, O_RDONLY); > > You can use O_CLOEXEC everywhere. Done. > > + ASSERT_LE(0, dir_s1d1_fd); > > + file1_s1d1_fd = open(file1_s1d1, O_RDONLY); > > + ASSERT_LE(0, file1_s1d1_fd); > > + dir_s2d1_fd = open(dir_s2d1, O_RDONLY); > > + ASSERT_LE(0, dir_s2d1_fd); > > + > > + /* > > + * Checks that FIOQSIZE works on files where LANDLOCK_ACCESS_FS_IOCTL is > > + * permitted. > > + */ > > + EXPECT_EQ(EACCES, test_fioqsize_ioctl(dir_s1d1_fd)); > > + EXPECT_EQ(0, test_fioqsize_ioctl(file1_s1d1_fd)); > > + EXPECT_EQ(0, test_fioqsize_ioctl(dir_s2d1_fd)); > > + > > + /* Closes all file descriptors. */ > > + ASSERT_EQ(0, close(dir_s1d1_fd)); > > + ASSERT_EQ(0, close(file1_s1d1_fd)); > > + ASSERT_EQ(0, close(dir_s2d1_fd)); > > +} > > + > > +TEST_F_FORK(layout1, ioctl_always_allowed) > > +{ > > + struct landlock_ruleset_attr attr = { > > const struct landlock_ruleset_attr attr = { Done. I am personally unsure whether "const" is worth it for local variables, but I am happy to abide by whatever the dominant style is. (The kernel style guide doesn't seem to mention it though.) BTW, it's somewhat inconsistent within this file already -- we should maybe clean this up. > > + .handled_access_fs = LANDLOCK_ACCESS_FS_IOCTL, > > + }; > > + int ruleset_fd, fd; > > + int flag = 0; > > + int n; > > const int flag = 0; > int ruleset_fd, test_fd, n; Done. Thanks for the review! —Günther -- Sent using Mutt 🐕 Woof Woof