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
