2025-05-01 10:54 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx>
> On Mon, Apr 28, 2025 at 2:15 PM YiFei Zhu <zhuyifei@xxxxxxxxxx> 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@xxxxxxxxxx>
>> Reported-by: Minh-Anh Nguyen <minhanhdn@xxxxxxxxxx>
>> Signed-off-by: YiFei Zhu <zhuyifei@xxxxxxxxxx>
>> ---
>> 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
