A previous commit expanded the usage scope of bpf_get_cgroup_classid() to all contexts (see Fixes tag), but this was inappropriate.
First, syzkaller reported a bug [1]. Second, it uses skb as an argument, but its implementation varies across different bpf prog types. For example, in sock_filter and sock_addr, it retrieves the classid from the current context (bpf_get_cgroup_classid_curr_proto) instead of from skb. In tc egress and lwt, it fetches the classid from skb->sk, but in tc ingress, it returns 0.
In summary, the definition of bpf_get_cgroup_classid() is ambiguous and its usage scenarios are limited. It should not be treated as a general-purpose helper. This patch reverts part of the previous commit.
[1] https://syzkaller.appspot.com/bug?extid=9767c7ed68b95cfa69e6
Fixes: ee971630f20f ("bpf: Allow some trace helpers for all prog types") Reported-by: syzbot+9767c7ed68b95cfa69e6@syzkaller.appspotmail.com Signed-off-by: Jiayuan Chen jiayuan.chen@linux.dev --- include/linux/bpf-cgroup.h | 8 ++++++++ kernel/bpf/cgroup.c | 25 +++++++++++++++++++++++++ kernel/bpf/helpers.c | 4 ---- 3 files changed, 33 insertions(+), 4 deletions(-)
diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h index 4847dcade917..9de7adb68294 100644 --- a/include/linux/bpf-cgroup.h +++ b/include/linux/bpf-cgroup.h @@ -427,6 +427,8 @@ int cgroup_bpf_prog_query(const union bpf_attr *attr,
const struct bpf_func_proto * cgroup_common_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog); +const struct bpf_func_proto * +cgroup_current_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog); #else
static inline int cgroup_bpf_inherit(struct cgroup *cgrp) { return 0; } @@ -463,6 +465,12 @@ cgroup_common_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) return NULL; }
+static inline const struct bpf_func_proto * +cgroup_current_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) +{ + return NULL; +} + static inline int bpf_cgroup_storage_assign(struct bpf_prog_aux *aux, struct bpf_map *map) { return 0; } static inline struct bpf_cgroup_storage *bpf_cgroup_storage_alloc( diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c index 62a1d8deb3dc..a99b72e6f1c9 100644 --- a/kernel/bpf/cgroup.c +++ b/kernel/bpf/cgroup.c @@ -1653,6 +1653,10 @@ cgroup_dev_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) if (func_proto) return func_proto;
+ func_proto = cgroup_current_func_proto(func_id, prog); + if (func_proto) + return func_proto; + switch (func_id) { case BPF_FUNC_perf_event_output: return &bpf_event_output_data_proto; @@ -2200,6 +2204,10 @@ sysctl_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) if (func_proto) return func_proto;
+ func_proto = cgroup_current_func_proto(func_id, prog); + if (func_proto) + return func_proto; + switch (func_id) { case BPF_FUNC_sysctl_get_name: return &bpf_sysctl_get_name_proto; @@ -2343,6 +2351,10 @@ cg_sockopt_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) if (func_proto) return func_proto;
+ func_proto = cgroup_current_func_proto(func_id, prog); + if (func_proto) + return func_proto; + switch (func_id) { #ifdef CONFIG_NET case BPF_FUNC_get_netns_cookie: @@ -2589,3 +2601,16 @@ cgroup_common_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) return NULL; } } + +const struct bpf_func_proto * +cgroup_current_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) +{ + switch (func_id) { +#ifdef CONFIG_CGROUP_NET_CLASSID + case BPF_FUNC_get_cgroup_classid: + return &bpf_get_cgroup_classid_curr_proto; +#endif + default: + return NULL; + } +} diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c index b71e428ad936..9d0d54f4f0de 100644 --- a/kernel/bpf/helpers.c +++ b/kernel/bpf/helpers.c @@ -2024,10 +2024,6 @@ bpf_base_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) return &bpf_get_current_ancestor_cgroup_id_proto; case BPF_FUNC_current_task_under_cgroup: return &bpf_current_task_under_cgroup_proto; -#endif -#ifdef CONFIG_CGROUP_NET_CLASSID - case BPF_FUNC_get_cgroup_classid: - return &bpf_get_cgroup_classid_curr_proto; #endif case BPF_FUNC_task_storage_get: if (bpf_prog_check_recur(prog))
Covers all scenarios where classid can be retrieved with bpf.
./test_progs -a cgroup_get_classid 53/1 cgroup_get_classid/get classid from tc:OK 53/2 cgroup_get_classid/get classid from sysctl:OK 53/3 cgroup_get_classid/get classid from cgroup dev:OK 53/4 cgroup_get_classid/get classid from cgroup sockopt:OK 53 cgroup_get_classid:OK Summary: 1/4 PASSED, 0 SKIPPED, 0 FAILED
Signed-off-by: Jiayuan Chen jiayuan.chen@linux.dev --- .../selftests/bpf/prog_tests/cgroup_classid.c | 212 ++++++++++++++++++ .../selftests/bpf/progs/test_cgroup_classid.c | 51 +++++ 2 files changed, 263 insertions(+) create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup_classid.c create mode 100644 tools/testing/selftests/bpf/progs/test_cgroup_classid.c
diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_classid.c b/tools/testing/selftests/bpf/prog_tests/cgroup_classid.c new file mode 100644 index 000000000000..f00da952e52c --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_classid.c @@ -0,0 +1,212 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <sys/types.h> +#include <unistd.h> +#include <test_progs.h> +#include "cgroup_helpers.h" +#include "network_helpers.h" +#include "test_cgroup_classid.skel.h" + +#define TEST_CGROUP "/cgroup_classid" + +static int test_cgroup_get_classid_from_tc(int cgroup_fd, int srv_fd, int srv_port, bool egress) +{ + struct test_cgroup_classid *skel; + int cli_fd = -1, ret = -1, expected; + + LIBBPF_OPTS(bpf_tcx_opts, optl); + + skel = test_cgroup_classid__open_and_load(); + if (!ASSERT_OK_PTR(skel, "skel_open")) + return ret; + + skel->bss->classid = -1; + if (egress) { + expected = getpid(); + skel->links.tc_egress = + bpf_program__attach_tcx(skel->progs.tc_egress, 1, &optl); + } else { + expected = 0; + skel->links.tc_ingress = + bpf_program__attach_tcx(skel->progs.tc_ingress, 1, &optl); + } + + cli_fd = connect_to_fd_opts(srv_fd, NULL); + if (!ASSERT_GE(cli_fd, 0, "connect_to_fd_opts")) + goto out; + + ASSERT_EQ(skel->bss->classid, expected, "classid mismatch"); + ret = 0; +out: + if (cli_fd > 0) + close(cli_fd); + + test_cgroup_classid__destroy(skel); + return ret; +} + +static void test_cgroup_get_classid_tc(void) +{ + int srv_fd = -1, srv_port = -1; + int cgroup_fd = -1; + + setup_classid_environment(); + set_classid(); + join_classid(); + + cgroup_fd = open_classid(); + if (!ASSERT_GE(cgroup_fd, 0, "open_classid")) + goto out; + + srv_fd = start_server(AF_INET, SOCK_STREAM, NULL, 0, 0); + if (!ASSERT_GE(srv_fd, 0, "srv_fd")) + goto out; + + srv_port = get_socket_local_port(srv_fd); + if (!ASSERT_GE(srv_port, 0, "get_socket_local_port")) + goto out; + + ASSERT_OK(test_cgroup_get_classid_from_tc(cgroup_fd, srv_fd, srv_port, 1), "egress"); + ASSERT_OK(test_cgroup_get_classid_from_tc(cgroup_fd, srv_fd, srv_port, 0), "ingress"); +out: + if (srv_fd > 0) + close(srv_fd); + if (cgroup_fd > 0) + close(cgroup_fd); + cleanup_classid_environment(); +} + +static void test_cgroup_get_classid_cgroup_dev(void) +{ + struct test_cgroup_classid *skel = NULL; + int cgroup_fd = -1; + + cgroup_fd = test__join_cgroup(TEST_CGROUP); + if (!ASSERT_GE(cgroup_fd, 0, "join cgroup")) + goto out; + + if (!ASSERT_OK(setup_classid_environment(), "setup env")) + goto out; + + if (!ASSERT_OK(set_classid(), "set_classid")) + goto out; + + skel = test_cgroup_classid__open_and_load(); + if (!ASSERT_OK_PTR(skel, "load program")) + goto out; + + skel->links.cg_dev = + bpf_program__attach_cgroup(skel->progs.cg_dev, cgroup_fd); + + if (!ASSERT_OK_PTR(skel->links.cg_dev, "attach_program")) + goto out; + + skel->bss->classid = -1; + if (!ASSERT_OK(join_classid(), "join_classid")) + goto out; + + open("/dev/null", O_RDWR); + ASSERT_EQ(skel->bss->classid, getpid(), "classid mismatch"); +out: + if (cgroup_fd > 0) + close(cgroup_fd); + test_cgroup_classid__destroy(skel); + cleanup_classid_environment(); +} + +static void test_cgroup_get_classid_sysctl(void) +{ + struct test_cgroup_classid *skel = NULL; + int cgroup_fd = -1; + + cgroup_fd = test__join_cgroup(TEST_CGROUP); + if (!ASSERT_GE(cgroup_fd, 0, "join cgroup")) + goto out; + + if (!ASSERT_OK(setup_classid_environment(), "setup env")) + goto out; + + if (!ASSERT_OK(set_classid(), "set_classid")) + goto out; + + skel = test_cgroup_classid__open_and_load(); + if (!ASSERT_OK_PTR(skel, "load program")) + goto out; + + skel->links.sysctl_tcp_mem = + bpf_program__attach_cgroup(skel->progs.sysctl_tcp_mem, cgroup_fd); + if (!ASSERT_OK_PTR(skel->links.sysctl_tcp_mem, "attach_program")) + goto out; + + skel->bss->classid = -1; + if (!ASSERT_OK(join_classid(), "join_classid")) + goto out; + + SYS_NOFAIL("cat /proc/sys/net/ipv4/tcp_mem"); + ASSERT_EQ(skel->bss->classid, getpid(), "classid mismatch"); +out: + if (cgroup_fd > 0) + close(cgroup_fd); + test_cgroup_classid__destroy(skel); + cleanup_classid_environment(); +} + +static void test_cgroup_get_classid_sockopt(void) +{ + struct test_cgroup_classid *skel = NULL; + int cgroup_fd = -1, fd = -1, val, err; + socklen_t val_len; + + cgroup_fd = test__join_cgroup(TEST_CGROUP); + if (!ASSERT_GE(cgroup_fd, 0, "join cgroup")) + goto out; + + if (!ASSERT_OK(setup_classid_environment(), "setup env")) + goto out; + + if (!ASSERT_OK(set_classid(), "set_classid")) + goto out; + + skel = test_cgroup_classid__open_and_load(); + if (!ASSERT_OK_PTR(skel, "load program")) + goto out; + + skel->links.cg_getsockopt = + bpf_program__attach_cgroup(skel->progs.cg_getsockopt, cgroup_fd); + if (!ASSERT_OK_PTR(skel->links.cg_getsockopt, "attach_program")) + goto out; + + skel->bss->classid = -1; + if (!ASSERT_OK(join_classid(), "join_classid")) + goto out; + + fd = socket(AF_INET, SOCK_STREAM, 0); + if (!ASSERT_OK_FD(fd, "socket")) + goto out; + + val_len = sizeof(val); + err = getsockopt(fd, SOL_SOCKET, SO_SNDBUF, &val, &val_len); + if (!ASSERT_OK(err, "getsockopt")) + goto out; + + ASSERT_EQ(skel->bss->classid, getpid(), "classid mismatch"); +out: + if (fd > 0) + close(fd); + if (cgroup_fd > 0) + close(cgroup_fd); + test_cgroup_classid__destroy(skel); + cleanup_classid_environment(); +} + +void test_cgroup_get_classid(void) +{ + if (test__start_subtest("get classid from tc")) + test_cgroup_get_classid_tc(); + if (test__start_subtest("get classid from sysctl")) + test_cgroup_get_classid_sysctl(); + if (test__start_subtest("get classid from cgroup dev")) + test_cgroup_get_classid_cgroup_dev(); + if (test__start_subtest("get classid from cgroup sockopt")) + test_cgroup_get_classid_sockopt(); +} diff --git a/tools/testing/selftests/bpf/progs/test_cgroup_classid.c b/tools/testing/selftests/bpf/progs/test_cgroup_classid.c new file mode 100644 index 000000000000..7a555ba6bb17 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/test_cgroup_classid.c @@ -0,0 +1,51 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <linux/bpf.h> +#include <sys/socket.h> +#include <bpf/bpf_helpers.h> +#include <bpf/bpf_endian.h> + +int classid; + +SEC("tc/egress") +int tc_egress(struct __sk_buff *skb) +{ + /* expecte real classid */ + classid = bpf_get_cgroup_classid(skb); + return TCX_PASS; +} + +SEC("tc/ingress") +int tc_ingress(struct __sk_buff *skb) +{ + /* expecte 0 */ + classid = bpf_get_cgroup_classid(skb); + return TCX_PASS; +} + +SEC("cgroup/dev") +int cg_dev(struct bpf_cgroup_dev_ctx *ctx) +{ + /* expecte real classid */ + classid = bpf_get_cgroup_classid((struct __sk_buff *)ctx); + /* Allow all */ + return 1; +} + +SEC("cgroup/sysctl") +int sysctl_tcp_mem(struct bpf_sysctl *ctx) +{ + /* expecte real classid */ + classid = bpf_get_cgroup_classid((struct __sk_buff *)ctx); + return 1; +} + +SEC("cgroup/getsockopt") +int cg_getsockopt(struct bpf_sockopt *ctx) +{ + /* expecte real classid */ + classid = bpf_get_cgroup_classid((struct __sk_buff *)ctx); + return 1; +} + +char _license[] SEC("license") = "GPL";
On Wed, May 28, 2025 at 9:17 AM Jiayuan Chen jiayuan.chen@linux.dev wrote:
A previous commit expanded the usage scope of bpf_get_cgroup_classid() to all contexts (see Fixes tag), but this was inappropriate.
First, syzkaller reported a bug [1]. Second, it uses skb as an argument, but its implementation varies across different bpf prog types. For example, in sock_filter and sock_addr, it retrieves the classid from the current context (bpf_get_cgroup_classid_curr_proto) instead of from skb. In tc egress and lwt, it fetches the classid from skb->sk, but in tc ingress, it returns 0.
In summary, the definition of bpf_get_cgroup_classid() is ambiguous and its usage scenarios are limited. It should not be treated as a general-purpose helper. This patch reverts part of the previous commit.
I think it's better to make it generic enough instead of reintroducing a bunch of prog specific proto handlers. See this discussion: https://lore.kernel.org/bpf/20250528020755.33182-1-yangfeng59949@163.com/
pw-bot: cr
linux-kselftest-mirror@lists.linaro.org
-
Alexei Starovoitov -
Jiayuan Chen