On Fri, May 31, 2024 at 6:04 AM Kenta Tada <tadakentaso@xxxxxxxxx> wrote:
>
> On 2024/05/31 6:20, Andrii Nakryiko wrote:
> > On Wed, May 29, 2024 at 6:10 AM Kenta Tada <tadakentaso@xxxxxxxxx> wrote:
> >>
> >> When CONFIG_NETKIT=y,
> >> bpftool-cgroup shows error even if the cgroup's path is correct:
> >>
> >> $ bpftool cgroup tree /sys/fs/cgroup
> >> CgroupPath
> >> ID AttachType AttachFlags Name
> >> Error: can't query bpf programs attached to /sys/fs/cgroup: No such device or address
> >>
> >> From strace and kernel tracing, I found netkit returned ENXIO and this command failed.
> >> I think this AttachType(BPF_NETKIT_PRIMARY) is not relevant to cgroup.
> >>
> >> bpftool-cgroup should query just only cgroup-related attach types.
> >>
> >> Signed-off-by: Kenta Tada <tadakentaso@xxxxxxxxx>
> >> ---
> >> tools/bpf/bpftool/cgroup.c | 47 +++++++++++++++++++++++++++++++++-----
> >> 1 file changed, 41 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/tools/bpf/bpftool/cgroup.c b/tools/bpf/bpftool/cgroup.c
> >> index af6898c0f388..bb2703aa4756 100644
> >> --- a/tools/bpf/bpftool/cgroup.c
> >> +++ b/tools/bpf/bpftool/cgroup.c
> >> @@ -19,6 +19,39 @@
> >>
> >> #include "main.h"
> >>
> >> +static const bool cgroup_attach_types[] = {
> >> + [BPF_CGROUP_INET_INGRESS] = true,
> >> + [BPF_CGROUP_INET_EGRESS] = true,
> >> + [BPF_CGROUP_INET_SOCK_CREATE] = true,
> >> + [BPF_CGROUP_INET_SOCK_RELEASE] = true,
> >> + [BPF_CGROUP_INET4_BIND] = true,
> >> + [BPF_CGROUP_INET6_BIND] = true,
> >> + [BPF_CGROUP_INET4_POST_BIND] = true,
> >> + [BPF_CGROUP_INET6_POST_BIND] = true,
> >> + [BPF_CGROUP_INET4_CONNECT] = true,
> >> + [BPF_CGROUP_INET6_CONNECT] = true,
> >> + [BPF_CGROUP_UNIX_CONNECT] = true,
> >> + [BPF_CGROUP_INET4_GETPEERNAME] = true,
> >> + [BPF_CGROUP_INET6_GETPEERNAME] = true,
> >> + [BPF_CGROUP_UNIX_GETPEERNAME] = true,
> >> + [BPF_CGROUP_INET4_GETSOCKNAME] = true,
> >> + [BPF_CGROUP_INET6_GETSOCKNAME] = true,
> >> + [BPF_CGROUP_UNIX_GETSOCKNAME] = true,
> >> + [BPF_CGROUP_UDP4_SENDMSG] = true,
> >> + [BPF_CGROUP_UDP6_SENDMSG] = true,
> >> + [BPF_CGROUP_UNIX_SENDMSG] = true,
> >> + [BPF_CGROUP_UDP4_RECVMSG] = true,
> >> + [BPF_CGROUP_UDP6_RECVMSG] = true,
> >> + [BPF_CGROUP_UNIX_RECVMSG] = true,
> >> + [BPF_CGROUP_SOCK_OPS] = true,
> >> + [BPF_CGROUP_DEVICE] = true,
> >> + [BPF_CGROUP_SYSCTL] = true,
> >> + [BPF_CGROUP_GETSOCKOPT] = true,
> >> + [BPF_CGROUP_SETSOCKOPT] = true,
> >> + [BPF_LSM_CGROUP] = true,
> >> + [__MAX_BPF_ATTACH_TYPE] = false,
> >> +};
> >> +
> >> #define HELP_SPEC_ATTACH_FLAGS \
> >> "ATTACH_FLAGS := { multi | override }"
> >>
> >> @@ -187,14 +220,16 @@ static int cgroup_has_attached_progs(int cgroup_fd)
> >> bool no_prog = true;
> >>
> >> for (type = 0; type < __MAX_BPF_ATTACH_TYPE; type++) {
> >
> > instead of iterating over all possible attach types and then checking
> > if attach type is cgroup-related, why not have an array of just cgroup
> > attach types and iterate it directly?
> >
> > pw-bot: cr
>
> The size of the bool array is smaller than saving each attachment type as the value in an integer array.
> But either is fine.
Those few bytes don't matter, it's about it being a cleaner approach.
You only iterate meaningful types, instead of iterating everything and
skipping some. In the grand scheme of things it's not that important,
but I'd go with the list approach as more natural to understand.
>
> I think the problem is that we don't increase the list of cgroup attach types in multiple files.
> Do you have any plans to add the new API to check whether the attach type is cgroup-related in libbpf?
> I want to call the new API in this patch.
I'm not convinced we need this API. cgroup-specific attach types are
just one possible subset of attach types, not sure it's a good idea to
add a dedicated filtering API for that in libbpf.
>
> >
> >
> >> - int count = count_attached_bpf_progs(cgroup_fd, type);
> >> + if (cgroup_attach_types[type]) {
> >> + int count = count_attached_bpf_progs(cgroup_fd, type);
> >>
> >> - if (count < 0 && errno != EINVAL)
> >> - return -1;
> >> + if (count < 0 && errno != EINVAL)
> >> + return -1;
> >>
> >> - if (count > 0) {
> >> - no_prog = false;
> >> - break;
> >> + if (count > 0) {
> >> + no_prog = false;
> >> + break;
> >> + }
> >> }
> >> }
> >>
> >> --
> >> 2.43.0
> >>
>
