On Fri, Sep 23, 2022 at 10:54:55PM +0200, Mickaël Salaün wrote:
>
> On 23/09/2022 19:50, Günther Noack wrote:
> > On Fri, Sep 16, 2022 at 07:05:44PM +0200, Mickaël Salaün wrote:
> > > I'd like to have tests similar to base_test.c:ruleset_fd_transfer to check
> > > ftruncate with different kind of file descriptors and not-sandboxed
> > > processes. That would require some code refactoring to reuse the FD passing
> > > code.
> >
> > Done. I factored out the FD sending and receiving into helper function in common.h.
>
> Please use a dedicated patch for this refactoring.
+1, will do.
> > > On 08/09/2022 21:58, Günther Noack wrote:
> > > > diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> > > > index 87b28d14a1aa..ddc8c7e57e86 100644
> > > > --- a/tools/testing/selftests/landlock/fs_test.c
> > > > +++ b/tools/testing/selftests/landlock/fs_test.c
> > > > ...
> > > > +TEST_F_FORK(layout1, truncate)
> > > > +{
> > > > + const char *const file_rwt = file1_s1d1;
> > > > + const char *const file_rw = file2_s1d1;
> > > > + const char *const file_rt = file1_s1d2;
> > > > + const char *const file_t = file2_s1d2;
> > > > + const char *const file_none = file1_s1d3;
> > > > + const char *const dir_t = dir_s2d1;
> > > > + const char *const file_in_dir_t = file1_s2d1;
> > > > + const char *const dir_w = dir_s3d1;
> > > > + const char *const file_in_dir_w = file1_s3d1;
> > > > + int file_rwt_fd, file_rw_fd;
> > >
> > > These variables are unused now.
> >
> > Good catch, done.
> >
> > > > +TEST_F_FORK(layout1, ftruncate)
> > >
> > > Great!
> > >
> > > > +{
> > > > + /*
> > > > + * This test opens a new file descriptor at different stages of
> > > > + * Landlock restriction:
> > > > + *
> > > > + * without restriction: ftruncate works
> > > > + * something else but truncate restricted: ftruncate works
> > > > + * truncate restricted and permitted: ftruncate works
> > > > + * truncate restricted and not permitted: ftruncate fails
> > > > + *
> > > > + * Whether this works or not is expected to depend on the time when the
> > > > + * FD was opened, not to depend on the time when ftruncate() was
> > > > + * called.
> > > > + */
> > > > + const char *const path = file1_s1d1;
> > > > + int fd0, fd1, fd2, fd3;
> > >
> > > You can rename them fd_layer0, fd_layer1…
> >
> > Done.
> >
> > > > + fd0 = open(path, O_WRONLY);
> > > > + EXPECT_EQ(0, test_ftruncate(fd0));
> > > > +
> > > > + landlock_single_path(_metadata, path,
> > > > + LANDLOCK_ACCESS_FS_READ_FILE |
> > > > + LANDLOCK_ACCESS_FS_WRITE_FILE,
> > > > + LANDLOCK_ACCESS_FS_WRITE_FILE);
> > >
> > > I'd prefer to follow the current way to write rule layers: write all struct
> > > rule at first and then call each enforcement steps. It is a bit more verbose
> > > but easier to understand errors. The list of test_ftruncate checks are
> > > straightforward to follow.
> >
> > Done.
> >
> >
> > > > + fd1 = open(path, O_WRONLY);
> > > > + EXPECT_EQ(0, test_ftruncate(fd0));
> > > > + EXPECT_EQ(0, test_ftruncate(fd1));
> > > > +
> > > > + landlock_single_path(_metadata, path, LANDLOCK_ACCESS_FS_TRUNCATE,
> > > > + LANDLOCK_ACCESS_FS_TRUNCATE);
> > > > +
> > > > + fd2 = open(path, O_WRONLY);
> > > > + EXPECT_EQ(0, test_ftruncate(fd0));
> > > > + EXPECT_EQ(0, test_ftruncate(fd1));
> > > > + EXPECT_EQ(0, test_ftruncate(fd2));
> > > > +
> > > > + landlock_single_path(_metadata, path,
> > > > + LANDLOCK_ACCESS_FS_TRUNCATE |
> > > > + LANDLOCK_ACCESS_FS_WRITE_FILE,
> > > > + LANDLOCK_ACCESS_FS_WRITE_FILE);
> > > > +
> > > > + fd3 = open(path, O_WRONLY);
> > > > + EXPECT_EQ(0, test_ftruncate(fd0));
> > > > + EXPECT_EQ(0, test_ftruncate(fd1));
> > > > + EXPECT_EQ(0, test_ftruncate(fd2));
> > > > + EXPECT_EQ(EACCES, test_ftruncate(fd3));
> > > > +
> > > > + ASSERT_EQ(0, close(fd0));
> > > > + ASSERT_EQ(0, close(fd1));
> > > > + ASSERT_EQ(0, close(fd2));
> > > > + ASSERT_EQ(0, close(fd3));
> > > > +}
> > > > +
> > > > /* clang-format off */
> > > > FIXTURE(layout1_bind) {};
> > > > /* clang-format on */
> >
--
