2023-04-21 09:07:54

by Feng Zhou

[permalink] [raw]
Subject: [PATCH bpf-next v2 0/2] Introduce a new kfunc of bpf_task_under_cgroup

From: Feng Zhou <[email protected]>

Trace sched related functions, such as enqueue_task_fair, it is necessary to
specify a task instead of the current task which within a given cgroup.

Feng Zhou (2):
bpf: Add bpf_task_under_cgroup() kfunc
selftests/bpf: Add testcase for bpf_task_under_cgroup

Changelog:
v1->v2: Addressed comments from Alexei Starovoitov
- Add kfunc instead.
Details in here:
https://lore.kernel.org/all/[email protected]/

kernel/bpf/helpers.c | 19 ++++++++
.../bpf/prog_tests/task_under_cgroup.c | 46 +++++++++++++++++++
.../selftests/bpf/progs/cgrp_kfunc_common.h | 1 +
.../bpf/progs/test_task_under_cgroup.c | 40 ++++++++++++++++
4 files changed, 106 insertions(+)
create mode 100644 tools/testing/selftests/bpf/prog_tests/task_under_cgroup.c
create mode 100644 tools/testing/selftests/bpf/progs/test_task_under_cgroup.c

--
2.20.1


2023-04-21 09:18:01

by Feng Zhou

[permalink] [raw]
Subject: [PATCH bpf-next v2 1/2] bpf: Add bpf_task_under_cgroup() kfunc

From: Feng Zhou <[email protected]>

Add a kfunc that's similar to the bpf_current_task_under_cgroup.
The difference is that it is a designated task.

When hook sched related functions, sometimes it is necessary to
specify a task instead of the current task.

Signed-off-by: Feng Zhou <[email protected]>
---
kernel/bpf/helpers.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 00e5fb0682ac..88e3247b5c44 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -2142,6 +2142,24 @@ __bpf_kfunc struct cgroup *bpf_cgroup_from_id(u64 cgid)
return NULL;
return cgrp;
}
+
+/**
+ * bpf_task_under_cgroup - Check whether the task is a given subset of the
+ * cgroup2 hierarchy. The cgroup2 to test is assigned by cgrp.
+ * @cgrp: assigned cgrp.
+ * @task: assigned task.
+ */
+__bpf_kfunc int bpf_task_under_cgroup(struct cgroup *cgrp,
+ struct task_struct *task)
+{
+ if (unlikely(!cgrp))
+ return -EAGAIN;
+
+ if (unlikely(!task))
+ return -ENOENT;
+
+ return task_under_cgroup_hierarchy(task, cgrp);
+}
#endif /* CONFIG_CGROUPS */

/**
@@ -2341,6 +2359,7 @@ BTF_ID_FLAGS(func, bpf_cgroup_acquire, KF_ACQUIRE | KF_RCU | KF_RET_NULL)
BTF_ID_FLAGS(func, bpf_cgroup_release, KF_RELEASE)
BTF_ID_FLAGS(func, bpf_cgroup_ancestor, KF_ACQUIRE | KF_RCU | KF_RET_NULL)
BTF_ID_FLAGS(func, bpf_cgroup_from_id, KF_ACQUIRE | KF_RET_NULL)
+BTF_ID_FLAGS(func, bpf_task_under_cgroup, KF_RCU)
#endif
BTF_ID_FLAGS(func, bpf_task_from_pid, KF_ACQUIRE | KF_RET_NULL)
BTF_SET8_END(generic_btf_ids)
--
2.20.1

2023-04-21 09:20:52

by Feng Zhou

[permalink] [raw]
Subject: [PATCH bpf-next v2 2/2] selftests/bpf: Add testcase for bpf_task_under_cgroup

From: Feng Zhou <[email protected]>

test_progs:
Tests new kfunc bpf_task_under_cgroup().

The bpf program saves the pid which call the getuid syscall within a
given cgroup to the remote_pid, which is convenient for the user-mode
program to verify the test correctness.

The user-mode program creates its own mount namespace, and mounts the
cgroupsv2 hierarchy in there, call the getuid syscall, then check if
remote_pid and local_pid are equal.

Signed-off-by: Feng Zhou <[email protected]>
---
.../bpf/prog_tests/task_under_cgroup.c | 46 +++++++++++++++++++
.../selftests/bpf/progs/cgrp_kfunc_common.h | 1 +
.../bpf/progs/test_task_under_cgroup.c | 40 ++++++++++++++++
3 files changed, 87 insertions(+)
create mode 100644 tools/testing/selftests/bpf/prog_tests/task_under_cgroup.c
create mode 100644 tools/testing/selftests/bpf/progs/test_task_under_cgroup.c

diff --git a/tools/testing/selftests/bpf/prog_tests/task_under_cgroup.c b/tools/testing/selftests/bpf/prog_tests/task_under_cgroup.c
new file mode 100644
index 000000000000..bd3deb469938
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/task_under_cgroup.c
@@ -0,0 +1,46 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2023 Bytedance */
+
+#include <test_progs.h>
+#include <cgroup_helpers.h>
+#include "test_task_under_cgroup.skel.h"
+
+#define FOO "/foo"
+
+void test_task_under_cgroup(void)
+{
+ struct test_task_under_cgroup *skel;
+ int ret, foo = -1;
+
+ foo = test__join_cgroup(FOO);
+ if (!ASSERT_OK(foo < 0, "cgroup_join_foo"))
+ return;
+
+ skel = test_task_under_cgroup__open();
+ if (!ASSERT_OK_PTR(skel, "test_task_under_cgroup__open"))
+ goto cleanup;
+
+ skel->rodata->local_pid = getpid();
+ skel->rodata->cgid = get_cgroup_id(FOO);
+
+ ret = test_task_under_cgroup__load(skel);
+ if (!ASSERT_OK(ret, "test_task_under_cgroup__load"))
+ goto cleanup;
+
+ ret = test_task_under_cgroup__attach(skel);
+ if (!ASSERT_OK(ret, "test_task_under_cgroup__attach"))
+ goto cleanup;
+
+ syscall(__NR_getuid);
+
+ test_task_under_cgroup__detach(skel);
+
+ ASSERT_EQ(skel->bss->remote_pid, skel->rodata->local_pid,
+ "test task_under_cgroup");
+
+cleanup:
+ if (foo)
+ close(foo);
+
+ test_task_under_cgroup__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/cgrp_kfunc_common.h b/tools/testing/selftests/bpf/progs/cgrp_kfunc_common.h
index 22914a70db54..41b3ea231698 100644
--- a/tools/testing/selftests/bpf/progs/cgrp_kfunc_common.h
+++ b/tools/testing/selftests/bpf/progs/cgrp_kfunc_common.h
@@ -26,6 +26,7 @@ struct cgroup *bpf_cgroup_ancestor(struct cgroup *cgrp, int level) __ksym;
struct cgroup *bpf_cgroup_from_id(u64 cgid) __ksym;
void bpf_rcu_read_lock(void) __ksym;
void bpf_rcu_read_unlock(void) __ksym;
+int bpf_task_under_cgroup(struct cgroup *cgrp, struct task_struct *task) __ksym;

static inline struct __cgrps_kfunc_map_value *cgrps_kfunc_map_value_lookup(struct cgroup *cgrp)
{
diff --git a/tools/testing/selftests/bpf/progs/test_task_under_cgroup.c b/tools/testing/selftests/bpf/progs/test_task_under_cgroup.c
new file mode 100644
index 000000000000..e2740f9b029d
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_task_under_cgroup.c
@@ -0,0 +1,40 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2023 Bytedance */
+
+#include <vmlinux.h>
+#include <asm/unistd.h>
+#include <bpf/bpf_tracing.h>
+#include <bpf/bpf_helpers.h>
+
+#include "cgrp_kfunc_common.h"
+
+const volatile int local_pid;
+const volatile long cgid;
+int remote_pid;
+
+SEC("tp_btf/sys_enter")
+int BPF_PROG(sysenter, struct pt_regs *regs, long id)
+{
+ struct cgroup *cgrp;
+
+ if (id != __NR_getuid)
+ return 0;
+
+ if (local_pid != (bpf_get_current_pid_tgid() >> 32))
+ return 0;
+
+ cgrp = bpf_cgroup_from_id(cgid);
+ if (!cgrp)
+ return 0;
+
+ if (!bpf_task_under_cgroup(cgrp, bpf_get_current_task_btf()))
+ goto out;
+
+ remote_pid = local_pid;
+
+out:
+ bpf_cgroup_release(cgrp);
+ return 0;
+}
+
+char _license[] SEC("license") = "GPL";
--
2.20.1

2023-04-21 18:27:12

by Alexei Starovoitov

[permalink] [raw]
Subject: Re: [PATCH bpf-next v2 1/2] bpf: Add bpf_task_under_cgroup() kfunc

On Fri, Apr 21, 2023 at 05:04:02PM +0800, Feng zhou wrote:
> From: Feng Zhou <[email protected]>
>
> Add a kfunc that's similar to the bpf_current_task_under_cgroup.
> The difference is that it is a designated task.
>
> When hook sched related functions, sometimes it is necessary to
> specify a task instead of the current task.
>
> Signed-off-by: Feng Zhou <[email protected]>
> ---
> kernel/bpf/helpers.c | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 00e5fb0682ac..88e3247b5c44 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -2142,6 +2142,24 @@ __bpf_kfunc struct cgroup *bpf_cgroup_from_id(u64 cgid)
> return NULL;
> return cgrp;
> }
> +
> +/**
> + * bpf_task_under_cgroup - Check whether the task is a given subset of the
> + * cgroup2 hierarchy. The cgroup2 to test is assigned by cgrp.

That doesn't read right.

> + * @cgrp: assigned cgrp.
> + * @task: assigned task.

This is also wrong.
Please copy paste from task_under_cgroup_hierarchy.

> + */
> +__bpf_kfunc int bpf_task_under_cgroup(struct cgroup *cgrp,
> + struct task_struct *task)

return type needs to be 'long'. 'int' might have issues.
Also the args should be task, cgrp to match task_under_cgroup_hierarchy.

> +{
> + if (unlikely(!cgrp))
> + return -EAGAIN;
> +
> + if (unlikely(!task))
> + return -ENOENT;

Please do
if (unlikely(!cgrp || !task))
return -EINVAL;

> +
> + return task_under_cgroup_hierarchy(task, cgrp);
> +}
> #endif /* CONFIG_CGROUPS */
>
> /**
> @@ -2341,6 +2359,7 @@ BTF_ID_FLAGS(func, bpf_cgroup_acquire, KF_ACQUIRE | KF_RCU | KF_RET_NULL)
> BTF_ID_FLAGS(func, bpf_cgroup_release, KF_RELEASE)
> BTF_ID_FLAGS(func, bpf_cgroup_ancestor, KF_ACQUIRE | KF_RCU | KF_RET_NULL)
> BTF_ID_FLAGS(func, bpf_cgroup_from_id, KF_ACQUIRE | KF_RET_NULL)
> +BTF_ID_FLAGS(func, bpf_task_under_cgroup, KF_RCU)
> #endif
> BTF_ID_FLAGS(func, bpf_task_from_pid, KF_ACQUIRE | KF_RET_NULL)
> BTF_SET8_END(generic_btf_ids)
> --
> 2.20.1
>

2023-04-21 18:38:04

by Alexei Starovoitov

[permalink] [raw]
Subject: Re: [PATCH bpf-next v2 2/2] selftests/bpf: Add testcase for bpf_task_under_cgroup

On Fri, Apr 21, 2023 at 05:04:03PM +0800, Feng zhou wrote:
> From: Feng Zhou <[email protected]>
>
> test_progs:
> Tests new kfunc bpf_task_under_cgroup().
>
> The bpf program saves the pid which call the getuid syscall within a
> given cgroup to the remote_pid, which is convenient for the user-mode
> program to verify the test correctness.
>
> The user-mode program creates its own mount namespace, and mounts the
> cgroupsv2 hierarchy in there, call the getuid syscall, then check if
> remote_pid and local_pid are equal.
>
> Signed-off-by: Feng Zhou <[email protected]>
> ---
> .../bpf/prog_tests/task_under_cgroup.c | 46 +++++++++++++++++++
> .../selftests/bpf/progs/cgrp_kfunc_common.h | 1 +
> .../bpf/progs/test_task_under_cgroup.c | 40 ++++++++++++++++
> 3 files changed, 87 insertions(+)
> create mode 100644 tools/testing/selftests/bpf/prog_tests/task_under_cgroup.c
> create mode 100644 tools/testing/selftests/bpf/progs/test_task_under_cgroup.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/task_under_cgroup.c b/tools/testing/selftests/bpf/prog_tests/task_under_cgroup.c
> new file mode 100644
> index 000000000000..bd3deb469938
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/task_under_cgroup.c
> @@ -0,0 +1,46 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2023 Bytedance */
> +
> +#include <test_progs.h>
> +#include <cgroup_helpers.h>
> +#include "test_task_under_cgroup.skel.h"
> +
> +#define FOO "/foo"
> +
> +void test_task_under_cgroup(void)
> +{
> + struct test_task_under_cgroup *skel;
> + int ret, foo = -1;
> +
> + foo = test__join_cgroup(FOO);
> + if (!ASSERT_OK(foo < 0, "cgroup_join_foo"))
> + return;
> +
> + skel = test_task_under_cgroup__open();
> + if (!ASSERT_OK_PTR(skel, "test_task_under_cgroup__open"))
> + goto cleanup;
> +
> + skel->rodata->local_pid = getpid();
> + skel->rodata->cgid = get_cgroup_id(FOO);
> +
> + ret = test_task_under_cgroup__load(skel);
> + if (!ASSERT_OK(ret, "test_task_under_cgroup__load"))
> + goto cleanup;
> +
> + ret = test_task_under_cgroup__attach(skel);
> + if (!ASSERT_OK(ret, "test_task_under_cgroup__attach"))
> + goto cleanup;
> +
> + syscall(__NR_getuid);
> +
> + test_task_under_cgroup__detach(skel);
> +
> + ASSERT_EQ(skel->bss->remote_pid, skel->rodata->local_pid,
> + "test task_under_cgroup");
> +
> +cleanup:
> + if (foo)
> + close(foo);

Looks wrong. should be if (foo >= 0) ?

> +
> + test_task_under_cgroup__destroy(skel);
> +}
> diff --git a/tools/testing/selftests/bpf/progs/cgrp_kfunc_common.h b/tools/testing/selftests/bpf/progs/cgrp_kfunc_common.h
> index 22914a70db54..41b3ea231698 100644
> --- a/tools/testing/selftests/bpf/progs/cgrp_kfunc_common.h
> +++ b/tools/testing/selftests/bpf/progs/cgrp_kfunc_common.h
> @@ -26,6 +26,7 @@ struct cgroup *bpf_cgroup_ancestor(struct cgroup *cgrp, int level) __ksym;
> struct cgroup *bpf_cgroup_from_id(u64 cgid) __ksym;
> void bpf_rcu_read_lock(void) __ksym;
> void bpf_rcu_read_unlock(void) __ksym;
> +int bpf_task_under_cgroup(struct cgroup *cgrp, struct task_struct *task) __ksym;
>
> static inline struct __cgrps_kfunc_map_value *cgrps_kfunc_map_value_lookup(struct cgroup *cgrp)
> {
> diff --git a/tools/testing/selftests/bpf/progs/test_task_under_cgroup.c b/tools/testing/selftests/bpf/progs/test_task_under_cgroup.c
> new file mode 100644
> index 000000000000..e2740f9b029d
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_task_under_cgroup.c
> @@ -0,0 +1,40 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2023 Bytedance */
> +
> +#include <vmlinux.h>
> +#include <asm/unistd.h>
> +#include <bpf/bpf_tracing.h>
> +#include <bpf/bpf_helpers.h>
> +
> +#include "cgrp_kfunc_common.h"
> +
> +const volatile int local_pid;
> +const volatile long cgid;
> +int remote_pid;
> +
> +SEC("tp_btf/sys_enter")

pls narrow down to specific syscall. Like you use in user space part: getuid

Also add this test to denylist.s390. See BPF CI failure.

> +int BPF_PROG(sysenter, struct pt_regs *regs, long id)
> +{
> + struct cgroup *cgrp;
> +
> + if (id != __NR_getuid)
> + return 0;
> +
> + if (local_pid != (bpf_get_current_pid_tgid() >> 32))
> + return 0;
> +
> + cgrp = bpf_cgroup_from_id(cgid);
> + if (!cgrp)
> + return 0;
> +
> + if (!bpf_task_under_cgroup(cgrp, bpf_get_current_task_btf()))
> + goto out;
> +
> + remote_pid = local_pid;
> +
> +out:
> + bpf_cgroup_release(cgrp);
> + return 0;
> +}
> +
> +char _license[] SEC("license") = "GPL";
> --
> 2.20.1
>