On Thu, May 1, 2025 at 1:04 PM Quentin Monnet <qmo@xxxxxxxxxx> wrote:
>
> 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.
Hmm. I'm not strongly against it, but consider the current
cgroup_has_attached_progs. If I see
int count = count_attached_bpf_progs(cgroup_fd, type);
if (count < 0 && errno != EINVAL)
return -1;
I know "negative is error, error number is propagated though errno".
If instead, I see
int count = count_attached_bpf_progs(cgroup_fd, type);
if (count < 0 && count != -EINVAL)
return count;
I know "negative is error, error number is propagated though return
value". So I would expect the caller to do
has_attached_progs = cgroup_has_attached_progs(cgroup_fd);
if (has_attached_progs < 0) {
p_err("can't query bpf programs attached to %s: %s",
path, strerror(-has_attached_progs));
(strerror(-has_attached_progs) rather than strerror(errno)). While I'm
fine with such a change, it looks a bit extraneous for what was a
simple one-line bug fix, and feels like it should be on a separate
patch and probably on bpf-next and not bpf. Wdyt?
YiFei Zhu
> Thanks,
> Quentin
