Re: [PATCH bpf-next 1/2] bpf: align syscall writeback behavior with caller-declared size

[Alexei Starovoitov]

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.