diff mbox series

bpftool: Query only cgroup-related attach types

Message ID 20240529131028.41200-1-tadakentaso@gmail.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series bpftool: Query only cgroup-related attach types | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc

Commit Message

Kenta Tada May 29, 2024, 1:10 p.m. UTC
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@gmail.com>
---
 tools/bpf/bpftool/cgroup.c | 47 +++++++++++++++++++++++++++++++++-----
 1 file changed, 41 insertions(+), 6 deletions(-)

Comments

Quentin Monnet May 29, 2024, 8:22 p.m. UTC | #1
On 29/05/2024 14:10, Kenta Tada 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@gmail.com>
> ---
>  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,
> +};


Thanks for this!

I can't say I'm glad to see another version of the list of
cgroup-related attach types (in addition to HELP_SPEC_ATTACH_TYPES and
to the manual page). But the alternative would be to explicitly skip
BPF_NETKIT_PRIMARY, which is not great, either. Too bad we don't have a
way to check whether the type is cgroup-related in libbpf or from the
bpf.h headers; but I don't think there's much interest to add it there,
so we'll probably have the array. We should account for it in
tools/testing/selftests/bpf/test_bpftool_synctypes.py, but I can do this
as a follow-up if you don't feel like messing up with the Python script.


> +
>  #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++) {
> -		int count = count_attached_bpf_progs(cgroup_fd, type);
> +		if (cgroup_attach_types[type]) {


Please change here:

                int count;

                if (!cgroup_attach_types[type])
                        continue;

And no need to further indent the rest of the block.


> +			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;
> +			}
>  		}
>  	}
>
Andrii Nakryiko May 30, 2024, 9:20 p.m. UTC | #2
On Wed, May 29, 2024 at 6:10 AM Kenta Tada <tadakentaso@gmail.com> 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@gmail.com>
> ---
>  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


> -               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
>
Kenta Tada May 31, 2024, 1:01 p.m. UTC | #3
On 2024/05/30 5:22, Quentin Monnet wrote:
> On 29/05/2024 14:10, Kenta Tada 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@gmail.com>
>> ---
>>  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,
>> +};
> 
> 
> Thanks for this!
> 
> I can't say I'm glad to see another version of the list of
> cgroup-related attach types (in addition to HELP_SPEC_ATTACH_TYPES and
> to the manual page). But the alternative would be to explicitly skip
> BPF_NETKIT_PRIMARY, which is not great, either. Too bad we don't have a
> way to check whether the type is cgroup-related in libbpf or from the
> bpf.h headers; but I don't think there's much interest to add it there,
> so we'll probably have the array. We should account for it in
> tools/testing/selftests/bpf/test_bpftool_synctypes.py, but I can do this
> as a follow-up if you don't feel like messing up with the Python script.

I think some bpf management tools require how to get cgroup-related attach types.
So I'm interested in adding the new API to check whether the type is cgroup-related in libbpf.

Thank you for the information about test_bpftool_synctypes.py.
BTW, I'm getting some syntax warnings when I use test_bpftool_synctypes.py in Python 3.12.
Python 3.12 changes the behavior of incorrect escape sequences.
To try test_bpftool_synctypes, I add r to the head and fix it in my local environment.

> 
> 
>> +
>>  #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++) {
>> -		int count = count_attached_bpf_progs(cgroup_fd, type);
>> +		if (cgroup_attach_types[type]) {
> 
> 
> Please change here:
> 
>                 int count;
> 
>                 if (!cgroup_attach_types[type])
>                         continue;
> 
> And no need to further indent the rest of the block.
> 
> 
>> +			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;
>> +			}
>>  		}
>>  	}
>>  
>
Kenta Tada May 31, 2024, 1:03 p.m. UTC | #4
On 2024/05/31 6:20, Andrii Nakryiko wrote:
> On Wed, May 29, 2024 at 6:10 AM Kenta Tada <tadakentaso@gmail.com> 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@gmail.com>
>> ---
>>  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.

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.

> 
> 
>> -               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
>>
Andrii Nakryiko May 31, 2024, 4:30 p.m. UTC | #5
On Fri, May 31, 2024 at 6:04 AM Kenta Tada <tadakentaso@gmail.com> wrote:
>
> On 2024/05/31 6:20, Andrii Nakryiko wrote:
> > On Wed, May 29, 2024 at 6:10 AM Kenta Tada <tadakentaso@gmail.com> 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@gmail.com>
> >> ---
> >>  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
> >>
>
diff mbox series

Patch

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++) {
-		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;
+			}
 		}
 	}