Re: [PATCH v3 2/5] selftests/landlock: Test ioctl support

["Günther Noack" <gnoack@google.com>]

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