Re: [PATCH bpf] bpftool: Fix regression of "bpftool cgroup tree" EINVAL on older kernels

[Quentin Monnet <qmo@kernel.org>]

2025-05-01 10:54 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com>
> On Mon, Apr 28, 2025 at 2:15 PM YiFei Zhu <zhuyifei@google.com> wrote:
>>
>> If cgroup_has_attached_progs queries an attach type not supported
>> by the running kernel, due to the kernel being older than the bpftool
>> build, it would encounter an -EINVAL from BPF_PROG_QUERY syscall.
>>
>> Prior to commit 98b303c9bf05 ("bpftool: Query only cgroup-related
>> attach types"), this EINVAL would be ignored by the function, allowing
>> the function to only consider supported attach types. The commit
>> changed so that, instead of querying all attach types, only attach
>> types from the array `cgroup_attach_types` is queried. The assumption
>> is that because these are only cgroup attach types, they should all
>> be supported. Unfortunately this assumption may be false when the
>> kernel is older than the bpftool build, where the attach types queried
>> by bpftool is not yet implemented in the kernel. This would result in
>> errors such as:
>>
>>   $ bpftool cgroup tree
>>   CgroupPath
>>   ID       AttachType      AttachFlags     Name
>>   Error: can't query bpf programs attached to /sys/fs/cgroup: Invalid argument
>>
>> This patch restores the logic of ignoring EINVAL from prior to that patch.
>>
>> Fixes: 98b303c9bf05 ("bpftool: Query only cgroup-related attach types")
>> Reported-by: Sagarika Sharma <sharmasagarika@google.com>
>> Reported-by: Minh-Anh Nguyen <minhanhdn@google.com>
>> Signed-off-by: YiFei Zhu <zhuyifei@google.com>
>> ---
>>  tools/bpf/bpftool/cgroup.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tools/bpf/bpftool/cgroup.c b/tools/bpf/bpftool/cgroup.c
>> index 93b139bfb9880..3f1d6be512151 100644
>> --- a/tools/bpf/bpftool/cgroup.c
>> +++ b/tools/bpf/bpftool/cgroup.c
>> @@ -221,7 +221,7 @@ static int cgroup_has_attached_progs(int cgroup_fd)
>>         for (i = 0; i < ARRAY_SIZE(cgroup_attach_types); i++) {
>>                 int count = count_attached_bpf_progs(cgroup_fd, cgroup_attach_types[i]);
>>
>> -               if (count < 0)
>> +               if (count < 0 && errno != EINVAL)
>>                         return -1;
> 
> let's maybe change count_attached_bpf_progs() to return error directly
> as returned by bpf_prog_query(), instead of translating that to -1 and
> then requiring relying on errno?
> 
> so just
> 
> if (ret)
>     return ret;
> 
> and then just
> 
> if (count < 0 && count != -EINVAL)
>     return /* well whatever, I'd return error probably instead of -1 again */
> 
> Thoughts?

It feels maybe slightly less intuitive to me to compare "count" - rather
than "errno" - with "-EINVAL", but I don't mind really. It does make
sense to check the return code from the function. Looks OK from my side.

Thanks,
Quentin