Re: [PATCH v2 bpf-next] selftests/bpf: Fix cgroup_xattr/read_cgroupfs_xattr

[Ihor Solodrai <ihor.solodrai@xxxxxxxxx>]

On 6/27/25 12:12 PM, Song Liu wrote:
> cgroup_xattr/read_cgroupfs_xattr has two issues:
>
> 1. cgroup_xattr/read_cgroupfs_xattr messes up lo without creating a netns
>     first. This causes issue with other tests.
>
>     Fix this by using a different hook (lsm.s/file_open) and not messing
>     with lo.
>
> 2. cgroup_xattr/read_cgroupfs_xattr sets up cgroups without proper
>     mount namespaces.
>
>     Fix this by using the existing cgroup helpers. A new helper
>     set_cgroup_xattr() is added to set xattr on cgroup files.
>
> Fixes: f4fba2d6d282 ("selftests/bpf: Add tests for bpf_cgroup_read_xattr")
> Reported-by: Alexei Starovoitov <ast@xxxxxxxxxx>
> Closes: https://lore.kernel.org/bpf/CAADnVQ+iqMi2HEj_iH7hsx+XJAsqaMWqSDe4tzcGAnehFWA9Sw@xxxxxxxxxxxxxx/
> Signed-off-by: Song Liu <song@xxxxxxxxxx>
>
> ---
> Changes v1 => v2:
> 1. Add the second fix above.
>
> v1: https://lore.kernel.org/bpf/20250627165831.2979022-1-song@xxxxxxxxxx/
> ---
>   tools/testing/selftests/bpf/cgroup_helpers.c  |  21 ++++
>   tools/testing/selftests/bpf/cgroup_helpers.h  |   4 +
>   .../selftests/bpf/prog_tests/cgroup_xattr.c   | 117 ++++--------------
>   .../selftests/bpf/progs/read_cgroupfs_xattr.c |   4 +-
>   4 files changed, 49 insertions(+), 97 deletions(-)

Hi Song.

I tried this patch on BPF CI, and it appears it fixes the hanging
failure we've been seeing today on bpf-next and netdev.
I am going to add it to ci/diffs.

https://github.com/kernel-patches/vmtest/actions/runs/15935456082

Tested-by: Ihor Solodrai <ihor.solodrai@xxxxxxxxx>

Thanks!

> diff --git a/tools/testing/selftests/bpf/cgroup_helpers.c b/tools/testing/selftests/bpf/cgroup_helpers.c
> index e4535451322e..15f626014872 100644
> --- a/tools/testing/selftests/bpf/cgroup_helpers.c
> +++ b/tools/testing/selftests/bpf/cgroup_helpers.c
> @@ -4,6 +4,7 @@
>   #include <sys/mount.h>
>   #include <sys/stat.h>
>   #include <sys/types.h>
> +#include <sys/xattr.h>
>   #include <linux/limits.h>
>   #include <stdio.h>
>   #include <stdlib.h>
> @@ -318,6 +319,26 @@ int join_parent_cgroup(const char *relative_path)
>   	return join_cgroup_from_top(cgroup_path);
>   }
>   
> +/**
> + * set_cgroup_xattr() - Set xattr on a cgroup dir
> + * @relative_path: The cgroup path, relative to the workdir, to set xattr
> + * @name: xattr name
> + * @value: xattr value
> + *
> + * This function set xattr on cgroup dir.
> + *
> + * On success, it returns 0, otherwise on failure it returns -1.
> + */
> +int set_cgroup_xattr(const char *relative_path,
> +		     const char *name,
> +		     const char *value)
> +{
> +	char cgroup_path[PATH_MAX + 1];
> +
> +	format_cgroup_path(cgroup_path, relative_path);
> +	return setxattr(cgroup_path, name, value, strlen(value) + 1, 0);
> +}
> +
>   /**
>    * __cleanup_cgroup_environment() - Delete temporary cgroups
>    *
> diff --git a/tools/testing/selftests/bpf/cgroup_helpers.h b/tools/testing/selftests/bpf/cgroup_helpers.h
> index 502845160d88..182e1ac36c95 100644
> --- a/tools/testing/selftests/bpf/cgroup_helpers.h
> +++ b/tools/testing/selftests/bpf/cgroup_helpers.h
> @@ -26,6 +26,10 @@ int join_cgroup(const char *relative_path);
>   int join_root_cgroup(void);
>   int join_parent_cgroup(const char *relative_path);
>   
> +int set_cgroup_xattr(const char *relative_path,
> +		     const char *name,
> +		     const char *value);
> +
>   int setup_cgroup_environment(void);
>   void cleanup_cgroup_environment(void);
>   
> diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_xattr.c b/tools/testing/selftests/bpf/prog_tests/cgroup_xattr.c
> index 87978a0f7eb7..e0dd966e4a3e 100644
> --- a/tools/testing/selftests/bpf/prog_tests/cgroup_xattr.c
> +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_xattr.c
> @@ -7,133 +7,60 @@
>   #include <string.h>
>   #include <unistd.h>
>   #include <sys/socket.h>
> -#include <sys/xattr.h>
> -
>   #include <test_progs.h>
> +#include "cgroup_helpers.h"
>   
>   #include "read_cgroupfs_xattr.skel.h"
>   #include "cgroup_read_xattr.skel.h"
>   
> -#define CGROUP_FS_ROOT "/sys/fs/cgroup/"
> -#define CGROUP_FS_PARENT CGROUP_FS_ROOT "foo/"
> +#define CGROUP_FS_PARENT "foo/"
>   #define CGROUP_FS_CHILD CGROUP_FS_PARENT "bar/"
> -
> -static int move_pid_to_cgroup(const char *cgroup_folder, pid_t pid)
> -{
> -	char filename[128];
> -	char pid_str[64];
> -	int procs_fd;
> -	int ret;
> -
> -	snprintf(filename, sizeof(filename), "%scgroup.procs", cgroup_folder);
> -	snprintf(pid_str, sizeof(pid_str), "%d", pid);
> -
> -	procs_fd = open(filename, O_WRONLY | O_APPEND);
> -	if (!ASSERT_OK_FD(procs_fd, "open"))
> -		return -1;
> -
> -	ret = write(procs_fd, pid_str, strlen(pid_str));
> -	close(procs_fd);
> -	if (!ASSERT_GT(ret, 0, "write cgroup.procs"))
> -		return -1;
> -	return 0;
> -}
> -
> -static void reset_cgroups_and_lo(void)
> -{
> -	rmdir(CGROUP_FS_CHILD);
> -	rmdir(CGROUP_FS_PARENT);
> -	system("ip addr del 1.1.1.1/32 dev lo");
> -	system("ip link set dev lo down");
> -}
> +#define TMP_FILE "/tmp/selftests_cgroup_xattr"
>   
>   static const char xattr_value_a[] = "bpf_selftest_value_a";
>   static const char xattr_value_b[] = "bpf_selftest_value_b";
>   static const char xattr_name[] = "user.bpf_test";
>   
> -static int setup_cgroups_and_lo(void)
> -{
> -	int err;
> -
> -	err = mkdir(CGROUP_FS_PARENT, 0755);
> -	if (!ASSERT_OK(err, "mkdir 1"))
> -		goto error;
> -	err = mkdir(CGROUP_FS_CHILD, 0755);
> -	if (!ASSERT_OK(err, "mkdir 2"))
> -		goto error;
> -
> -	err = setxattr(CGROUP_FS_PARENT, xattr_name, xattr_value_a,
> -		       strlen(xattr_value_a) + 1, 0);
> -	if (!ASSERT_OK(err, "setxattr 1"))
> -		goto error;
> -
> -	err = setxattr(CGROUP_FS_CHILD, xattr_name, xattr_value_b,
> -		       strlen(xattr_value_b) + 1, 0);
> -	if (!ASSERT_OK(err, "setxattr 2"))
> -		goto error;
> -
> -	err = system("ip link set dev lo up");
> -	if (!ASSERT_OK(err, "lo up"))
> -		goto error;
> -
> -	err = system("ip addr add 1.1.1.1 dev lo");
> -	if (!ASSERT_OK(err, "lo addr v4"))
> -		goto error;
> -
> -	err = write_sysctl("/proc/sys/net/ipv4/ping_group_range", "0 0");
> -	if (!ASSERT_OK(err, "write_sysctl"))
> -		goto error;
> -
> -	return 0;
> -error:
> -	reset_cgroups_and_lo();
> -	return err;
> -}
> -
>   static void test_read_cgroup_xattr(void)
>   {
> -	struct sockaddr_in sa4 = {
> -		.sin_family = AF_INET,
> -		.sin_addr.s_addr = htonl(INADDR_LOOPBACK),
> -	};
> +	int tmp_fd, parent_cgroup_fd = -1, child_cgroup_fd = -1;
>   	struct read_cgroupfs_xattr *skel = NULL;
> -	pid_t pid = gettid();
> -	int sock_fd = -1;
> -	int connect_fd = -1;
>   
> -	if (!ASSERT_OK(setup_cgroups_and_lo(), "setup_cgroups_and_lo"))
> +	parent_cgroup_fd = test__join_cgroup(CGROUP_FS_PARENT);
> +	if (!ASSERT_OK_FD(parent_cgroup_fd, "create parent cgroup"))
>   		return;
> -	if (!ASSERT_OK(move_pid_to_cgroup(CGROUP_FS_CHILD, pid),
> -		       "move_pid_to_cgroup"))
> +	if (!ASSERT_OK(set_cgroup_xattr(CGROUP_FS_PARENT, xattr_name, xattr_value_a),
> +		       "set parent xattr"))
> +		goto out;
> +
> +	child_cgroup_fd = test__join_cgroup(CGROUP_FS_CHILD);
> +	if (!ASSERT_OK_FD(child_cgroup_fd, "create child cgroup"))
> +		goto out;
> +	if (!ASSERT_OK(set_cgroup_xattr(CGROUP_FS_CHILD, xattr_name, xattr_value_b),
> +		       "set child xattr"))
>   		goto out;
>   
>   	skel = read_cgroupfs_xattr__open_and_load();
>   	if (!ASSERT_OK_PTR(skel, "read_cgroupfs_xattr__open_and_load"))
>   		goto out;
>   
> -	skel->bss->target_pid = pid;
> +	skel->bss->target_pid = gettid();
>   
>   	if (!ASSERT_OK(read_cgroupfs_xattr__attach(skel), "read_cgroupfs_xattr__attach"))
>   		goto out;
>   
> -	sock_fd = socket(AF_INET, SOCK_DGRAM, IPPROTO_ICMP);
> -	if (!ASSERT_OK_FD(sock_fd, "sock create"))
> -		goto out;
> -
> -	connect_fd = connect(sock_fd, &sa4, sizeof(sa4));
> -	if (!ASSERT_OK_FD(connect_fd, "connect 1"))
> -		goto out;
> -	close(connect_fd);
> +	tmp_fd = open(TMP_FILE, O_RDONLY | O_CREAT);
> +	ASSERT_OK_FD(tmp_fd, "open tmp file");
> +	close(tmp_fd);
>   
>   	ASSERT_TRUE(skel->bss->found_value_a, "found_value_a");
>   	ASSERT_TRUE(skel->bss->found_value_b, "found_value_b");
>   
>   out:
> -	close(connect_fd);
> -	close(sock_fd);
> +	close(child_cgroup_fd);
> +	close(parent_cgroup_fd);
>   	read_cgroupfs_xattr__destroy(skel);
> -	move_pid_to_cgroup(CGROUP_FS_ROOT, pid);
> -	reset_cgroups_and_lo();
> +	unlink(TMP_FILE);
>   }
>   
>   void test_cgroup_xattr(void)
> diff --git a/tools/testing/selftests/bpf/progs/read_cgroupfs_xattr.c b/tools/testing/selftests/bpf/progs/read_cgroupfs_xattr.c
> index 855f85fc5522..405adbe5e8b0 100644
> --- a/tools/testing/selftests/bpf/progs/read_cgroupfs_xattr.c
> +++ b/tools/testing/selftests/bpf/progs/read_cgroupfs_xattr.c
> @@ -17,8 +17,8 @@ static const char expected_value_b[] = "bpf_selftest_value_b";
>   bool found_value_a;
>   bool found_value_b;
>   
> -SEC("lsm.s/socket_connect")
> -int BPF_PROG(test_socket_connect)
> +SEC("lsm.s/file_open")
> +int BPF_PROG(test_file_open)
>   {
>   	u64 cgrp_id = bpf_get_current_cgroup_id();
>   	struct cgroup_subsys_state *css, *tmp;