Re: [PATCH bpf-next v4 2/2] selftests/bpf: Cover attach type checks in link update

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 2582 bytes --]

> Add regression coverage for link update compatibility. Verify that a UDP6
> sock_addr program cannot replace a UDP4 program and that an LSM_MAC
> program cannot replace an LSM_CGROUP program.
> Also verify that CGROUP_SKB attachment still requires CAP_NET_ADMIN, while
> an existing link can be updated after dropping CAP_NET_ADMIN and
> CAP_SYS_ADMIN.

This isn't a bug, but could the changelog say why this coverage is needed,
rather than restating what each assertion does? The tests appear to pin
down attach-type validation in link update (which didn't exist before the
companion patch) and the permission-check split for CGROUP_SKB (capability
at LINK_CREATE but not LINK_UPDATE).

> diff --git a/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c b/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c
> index 41e867467f6c6..85b0af4f486af 100644
> --- a/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c
> +++ b/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c
> @@ -74,6 +74,7 @@ static void test_lsm_cgroup_functional(void)
>  	int bind_prog_fd = -1;
>  	int bind_link_fd = -1;
>  	int clone_prog_fd = -1;
> +	int mac_prog_fd;
>  	int err, fd, prio;
>  	socklen_t socklen;
>
> @@ -156,6 +157,15 @@ static void test_lsm_cgroup_functional(void)
>  	ASSERT_EQ(query_prog_cnt(cgroup_fd, "bpf_lsm_socket_bind"), 1, "prog count");
>  	ASSERT_EQ(query_prog_cnt(cgroup_fd, NULL), 4, "total prog count");
>
> +	fd = bpf_link_create(bpf_program__fd(skel->progs.socket_first),
> +			     cgroup_fd, BPF_LSM_CGROUP, NULL);
> +	if (!ASSERT_GE(fd, 0, "link create socket_first"))
> +		goto detach_cgroup;
> +	mac_prog_fd = bpf_program__fd(skel->progs.socket_create_lsm);
> +	err = bpf_link_update(fd, mac_prog_fd, NULL);
> +	ASSERT_EQ(err, -EINVAL, "reject lsm_mac link update");
> +	close(fd);
> +

This isn't a bug, but would it read more consistently to inline
bpf_program__fd(skel->progs.socket_create_lsm) into the bpf_link_update()
call? Every other '*_prog_fd' local in this function's declaration block is
initialised to -1, while mac_prog_fd is uninitialised and used exactly once
on the line after assignment; the bpf_link_create() call twelve lines above
already inlines the equivalent expression. Also, the new link fd is held in
the generic 'fd' variable, whereas the link created earlier uses the
descriptive 'bind_link_fd'.


---
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/33352119890