On Mon May 25, 2026 at 12:21 AM PDT, Yuyang Huang wrote: > > For this example, the regression lies in legacy CGROUP queries: > 1. BPF_PROG_QUERY for cgroups is not a new feature; it has been in the > kernel since 2017. > 2. There are existing legacy userspace applications and language > wrappers (like our android net-tests) written years ago with older > structure layouts (passing size = 40, ending at > query.prog_attach_flags) that query cgroups. > 3. In June 2025, Commit 120933984460 backported "revision" support to > cgroup queries, introducing an unconditional writeback in > `__cgroup_bpf_query()` at offset 56: > ```c > // In __cgroup_bpf_query() (kernel/bpf/cgroup.c) - Introduced in 120933984460: > if (copy_to_user(&uattr->query.revision, &revision, sizeof(revision))) > return -EFAULT; > ```
Ok. This is fair. 120933984460 is indeed problematic. > Now lets talk about BPF_TCX_EGRESS: > >> bpf_mprog_query() and tcx_prog_query() were only introduced there. >> How come your userspace passes shorter uatter to query newer >> BPF_TCX_EGRESS attachment? > > I understand your point, but two common cases exist where userspace > will legitimately query BPF_TCX_EGRESS with a 40-byte structure: > > First, a generic BPF querying tool (assume it called `./bpf-query`) > compiled in 2020 (when the query buffer size was 40 bytes) might > accept the attach type dynamically from the command line: > ./bpf-query --type 47 --fd 3 # 47 is BPF_TCX_EGRESS Don't buy this at all. This one is clearly a user space bug. > Add gating to the TCX path, will not be able to fix the legacy cgroup > query path. As shown above, the cgroup query path has the exact same > OOB writeback regression affecting legacy userspace. Since we must > plumb uattr_size to cgroup.c to resolve the cgroup regression safely, > applying the same consistent size-gating in bpf_mprog_query() seemed > like the most consistent and robust architectural path. Not really. Your patch adds checks to what looks like "random" copy_to_user() places. This thread is clear indication that it's not a "robust architectural path". True robust path would be to wrap every copy_to_user() into another helper that takes uattr start pointer and size, does extra check, and returns something like ENOSPC (and not EFAULT), but that would be a significant churn. So I prefer a minimal patch that add single check in bpf_prog_query() that checks that user space supplied buffer is large enough for attr->query. If not -> EFAULT. That would be better than overwritting. One can argue that partial copy_to_user() in __cgroup_bpf_query() should still be allowed, but I'm against partial and inconsistent queries. query.revision might seem benign, but if we do it for all copy_to_user() we better return ENOSPC to differentiate vs EFAULT to tell user space to fix itself.
