Re: [PATCH bpf-next v3 14/15] selftests/bpf: Test the LSM policy object kfuncs with Landlock

[bot+bpf-ci]

> diff --git a/tools/testing/selftests/bpf/prog_tests/lsm_policy_landlock.c b/tools/testing/selftests/bpf/prog_tests/lsm_policy_landlock.c
> new file mode 100644
> index 000000000000..f01ae5be8aef
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/lsm_policy_landlock.c
> @@ -0,0 +1,525 @@

[ ... ]

> +static int setup_env(struct policy_test_env *env)
> +{
> +	env->skel = NULL;
> +	env->ruleset_fd = -1;
> +	strcpy(env->tmp_path, "/tmp/lsm_policy_landlock_XXXXXX");
> +	env->tmp_fd = mkstemp(env->tmp_path);
> +	if (!ASSERT_GE(env->tmp_fd, 0, "mkstemp"))
> +		return -1;
> +
> +	env->ruleset_fd = create_ruleset();
> +	if (env->ruleset_fd < 0) {
> +		if (errno == EOPNOTSUPP || errno == ENOSYS)
> +			test__skip();
                          ^^^^^^^^^^^^

A selftests pattern flags this as potentially concerning: skipping with no
message, so the output carries no hint about which prerequisite is absent.

All five subtests funnel through this path, and both errno values look
classified correctly here - EOPNOTSUPP when Landlock is built but not
listed in CONFIG_LSM or in lsm= at boot, ENOSYS when
CONFIG_SECURITY_LANDLOCK=n.  What is not visible in the output is which of
the two happened; a reader sees five skips and cannot tell a config
ordering problem from the feature not being built at all.

Against that, test__skip() in the bpf framework takes no message argument,
and a bare test__skip() is the usual idiom across prog_tests/, so carrying
a reason means adding a separate print rather than passing one to an
existing API.

Would a one-line print naming the missing prerequisite before test__skip()
be worth adding?

> +		else
> +			ASSERT_GE(env->ruleset_fd, 0,
> +				  "landlock_create_ruleset");
> +		return -1;
> +	}
> +
> +	env->skel = lsm_policy_landlock__open_and_load();
> +	if (!ASSERT_OK_PTR(env->skel, "skel_open_and_load"))
> +		return -1;

[ ... ]

> +/*
> + * Exit codes: 4 = unexpected write outcome, 0 = everything as
> + * expected.
> + */
> +static void format_child_cmd(char *cmd, size_t len, bool expect_write_ok,
> +			     const char *tmp_path)
> +{
> +	if (expect_write_ok)
> +		snprintf(cmd, len, "echo x > %s || exit 4; exit 0", tmp_path);
> +	else
> +		snprintf(cmd, len,
> +			 "if echo x > %s 2>/dev/null; then exit 4; fi; exit 0",
                                        ^^^^^^^^^^^^^

Is the redirect order here inverted?  A shell applies redirections left to
right, so the failure of "> %s" is reported before 2>/dev/null has been
applied, and the diagnostic goes to the inherited fd 2:

    $ sh -c 'if echo x > /nodir123/f 2>/dev/null; then exit 4; fi; exit 0'
    sh: 1: cannot create /nodir123/f: Directory nonexistent

    $ sh -c 'if echo x 2>/dev/null > /nodir123/f; then exit 4; fi; exit 0'
    (no output)

bash behaves the same way, reporting "/nodir123/f: No such file or
directory" for the first form.

Does that message then land in the selftest output?  test_progs.c only
reassigns the stdout FILE*:

tools/testing/selftests/bpf/test_progs.c:stdio_hijack_init() {
    ...
	fflush(stdout);
	fflush(stderr);

	stdout = open_memstream(log_buf, log_cnt);
    ...
}

and never redirects fd 1 or fd 2 themselves, while the writer here is a
forked child that inherited the real fd 2.  So each restricted execution
would print

    sh: 1: cannot create /tmp/lsm_policy_landlock_XXXXXX: Permission denied

which is seven executions across test_restrict_binprm(),
test_restrict_binprm_concurrent() and test_restrict_binprm_trace().  The
child's exit status is still 0, so the assertions are unaffected and this
is stray output rather than a false failure.

Would "echo x 2>/dev/null > %s" do what this was written to do?

> +			 tmp_path);
> +}

[ ... ]


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/34398515657