Re: [PATCH v2 1/3] selftests/landlock: Add filesystem access benchmark

["Mickaël Salaün" <mic@digikod.net>]

On Fri, Feb 06, 2026 at 01:24:02PM +0100, Günther Noack wrote:
> Hello!
> 
> On Wed, Jan 28, 2026 at 10:31:23PM +0100, Mickaël Salaün wrote:
> > On Sun, Jan 25, 2026 at 08:58:51PM +0100, Günther Noack wrote:
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/landlock/fs_bench.c

> > > +		if (abi < 7)
> > > +			err(1, "Landlock ABI too low: got %d, wanted 7+", abi);
> > > +	}
> > > +
> > > +	ruleset_fd = -1;
> > > +	if (use_landlock) {
> > > +		struct landlock_ruleset_attr attr = {
> > > +			.handled_access_fs =
> > > +				0xffff, /* All FS access rights as of 2026-01 */
> > > +		};
> > > +		ruleset_fd = syscall(SYS_landlock_create_ruleset, &attr,
> > > +				     sizeof(attr), 0U);
> > > +		if (ruleset_fd < 0)
> > > +			err(1, "landlock_create_ruleset");
> > > +	}
> > > +
> > > +	current = open(".", O_PATH);
> > > +	if (current < 0)
> > > +		err(1, "open(.)");
> > > +
> > > +	while (depth--) {
> > > +		if (use_landlock) {
> > > +			struct landlock_path_beneath_attr attr = {
> > > +				.allowed_access = LANDLOCK_ACCESS_FS_IOCTL_DEV,
> > > +				.parent_fd = current,
> > > +			};
> > > +			if (syscall(SYS_landlock_add_rule, ruleset_fd,
> > > +				    LANDLOCK_RULE_PATH_BENEATH, &attr, 0) < 0)
> > > +				err(1, "landlock_add_rule");
> > > +		}
> > > +
> > > +		if (mkdirat(current, path, 0700) < 0)
> > > +			err(1, "mkdirat(%s)", path);
> > 
> > We should have a loop to build the directories, then start the timer and
> > have another loop to add Landlock rules.
> 
> I have to politely push back on this; the granularity of time
> measurement is not high enough and the measurement below only works
> because we repeat it 100000 times.  This is not the case when we
> construct a Landlock ruleset, and it would IMHO be weird to build the
> ruleset multiple times as well.  It feels like this would better be
> measured in a separate benchmark.
> 
> Adding a rule is an operation whose runtime does not depend on the
> depth of the nested directories, so such a separate benchmark would
> then also be simpler and wouldn't need to construct such a deeply
> nested hierarchy.

OK.  Please add this explanation in a comment.

> 
> 
> > > +	printf("*** Benchmark ***\n");
> > 
> > We should probably use ksft_*() helpers in main (see
> > seccomp_benchmark.c).
> 
> Among the benchmarks, the seccomp benchmark is the one exception in
> that it uses these ksft_*() helpers, and it's not clear to me that it
> has any benefit.  These helpers are for producing TAP-formatted
> output, and assume that there will be individual test cases with
> success/failure results, which is not the case here.  The seccomp test
> uses approximate assertions about the expected timing of operations
> (+-10%), but I don't think we can easily do that in our case.
> 
> I would therefore prefer to use a normal textual output format,
> similar to the other benchmarks in tools/testing/kselftests.

OK

> 
> 
> > > +	printf("%zu dirs, %zu iterations, %s landlock\n", num_subdirs,
> > > +	       num_iterations, use_landlock ? "with" : "without");
> > > +
> > > +	if (times(&start_time) == -1)
> > > +		err(1, "times");
> > > +
> > > +	current = build_directory(num_subdirs, use_landlock);
> > > +
> > > +	for (int i = 0; i < num_iterations; i++) {
> > > +		fd = openat(current, ".", O_DIRECTORY);
> > 
> > We can use AT_EMPTY_PATH (with an empty path) instead of "."
> > I guess the benchmark should not change, but better to check again.
> 
> This had to change anyway; now that I added cleanup of the created
> directories, I had to use another operation here that would trigger
> the path walk (file open for creation).  Opening directories and
> removing directories both need to continue working so that we can
> later remove the directories. (See discussion below.)
> 
> 
> > > +		if (fd != -1) {
> > > +			if (use_landlock)
> > > +				errx(1, "openat succeeded, expected error");
> > > +
> > > +			close(fd);
> > > +		}
> > > +	}
> > > +
> > > +	if (times(&end_time) == -1)
> > > +		err(1, "times");
> > 
> > The created directories should be removed here (setup and teardown).
> 
> Done.
> 
> Minor implementation remark: This is also done with explicit loops
> that use openat() to walk the directory tree with file descriptors and
> then unlinkat(fd, "d", ...).  At this nesting depth, the paths don't
> fit into PATH_MAX any more and relative dirfds are the only way to do
> that AFAIK.  (The directory walk function nftw(3) also breaks down
> FWIW, because it uses long paths relative to cwd.)
> 
> –Günther
>