lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 8 Sep 2022 11:23:07 +0800 From: Pu Lehui <pulehui@...wei.com> To: John Fastabend <john.fastabend@...il.com>, <bpf@...r.kernel.org>, <linux-kernel@...r.kernel.org> CC: Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>, Quentin Monnet <quentin@...valent.com>, Martin KaFai Lau <kafai@...com>, Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>, KP Singh <kpsingh@...nel.org>, Jean-Philippe Brucker <jean-philippe@...aro.org>, Stanislav Fomichev <sdf@...gle.com>, Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org> Subject: Re: [PATCH bpf-next 1/2] bpf, cgroup: Fix attach flags being assigned to effective progs On 2022/9/8 11:07, Pu Lehui wrote: > > > On 2022/8/24 5:22, John Fastabend wrote: >> Pu Lehui wrote: >>> Attach flags is only valid for attached progs of this layer cgroup, >>> but not for effective progs. We know that the attached progs is at >>> the beginning of the effective progs array, so we can just populate >>> the elements in front of the prog_attach_flags array. >>> >>> Signed-off-by: Pu Lehui <pulehui@...wei.com> >> >> Trying to parse above, could you add a bit more detail on why this is >> problem so readers don't need to track it down. >> >>> --- >>> kernel/bpf/cgroup.c | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c >>> index 59b7eb60d5b4..9adf72e99907 100644 >>> --- a/kernel/bpf/cgroup.c >>> +++ b/kernel/bpf/cgroup.c >>> @@ -1091,11 +1091,14 @@ static int __cgroup_bpf_query(struct cgroup >>> *cgrp, const union bpf_attr *attr, >>> } >> >> Because we are looking at it let me try to understand. There are two >> paths that set cnt relative bits here, >> > > Hi John, > > Thanks for your review. The reason is that: > For the following cgroup tree: > root > | > cg1 > | > cg2 > > I attached prog1 and prog2 to root cgroup with MULTI attach type, sorry, typo, attach type -> attach flag > attached prog3 to cg1 with OVERRIDE attach type, and then used bpftool > to show this cgroup tree with effective query flag: > > $ bpftool cgroup tree /sys/fs/cgroup effective > CgroupPath > ID AttachType AttachFlags Name > /sys/fs/cgroup > 1 sysctl multi sysctl_tcp_mem > 2 sysctl multi sysctl_tcp_mem > /sys/fs/cgroup/cg1 > 3 sysctl override sysctl_tcp_mem > 1 sysctl override sysctl_tcp_mem <- wrong > 2 sysctl override sysctl_tcp_mem <- wrong > /sys/fs/cgroup/cg1/cg2 > 3 sysctl sysctl_tcp_mem > 1 sysctl sysctl_tcp_mem > 2 sysctl sysctl_tcp_mem > > For cg1, obviously, the attach flags of prog1 and prog2 can not be > OVERRIDE, and the attach flags of prog1 and prog2 is meaningless for > cg1. We only need to care the attach flags of prog which attached to > cg1, other progs attach flags should be omit. > >> if (attr->query.query_flags & BPF_F_QUERY_EFFECTIVE) { >> ... >> cnt = min_t(int, bpf_prog_array_length(effective), total_cnt); >> ... >> } else { >> ... >> progs = &cgrp->bpf.progs[atype]; >> cnt = min_t(int, prog_list_length(progs), total_cnt); >> ... >> } >> >> And the docs claim >> >> * **BPF_F_QUERY_EFFECTIVE** >> * Only return information regarding programs >> which are >> * currently effective at the specified >> *target_fd*. >> >> so in the EFFECTIVE case should we be reporting flags at all if the >> commit message says "attach flags is only valid for attached progs >> of this layer cgroup, but not for effective progs." >> >> And then in the else branch the change is what you have in the diff >> anyways correct? >> >>> if (prog_attach_flags) { >>> + int progs_cnt = prog_list_length(&cgrp->bpf.progs[atype]); >>> flags = cgrp->bpf.flags[atype]; >>> - for (i = 0; i < cnt; i++) >> >> Do we need to min with total_cnt here so we don't walk off a short >> user list? >> > > We should limit it, will fix it in v2. For query without effective flag, > progs_cnt will equal to cnt, and for effective flag situation, progs_cnt > only calculate prog count which attached to this cgroup layer. > >>> + /* attach flags only for attached progs, but not >>> effective progs */ >>> + for (i = 0; i < progs_cnt; i++) >>> if (copy_to_user(prog_attach_flags + i, &flags, >>> sizeof(flags))) >>> return -EFAULT; >>> + >>> prog_attach_flags += cnt; >>> } >>> -- >>> 2.25.1 >>> >> . >> > .
Powered by blists - more mailing lists