Re: [PATCH bpf 3/3] bpftool: Add support for BPF_F_PREORDER cgroup attach flag

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 6612 bytes --]

> diff --git a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
> index e8185596a759b..8ec2546c0b8ee 100644
> --- a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
> +++ b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
> @@ -45,7 +45,7 @@ CGROUP COMMANDS
> |     **cgroup_unix_recvmsg** | **cgroup_sysctl** |
> |     **cgroup_getsockopt** | **cgroup_setsockopt** |
> |     **cgroup_inet_sock_release** }
> -| *ATTACH_FLAGS* := { **multi** | **override** }
> +| *ATTACH_FLAGS* := { **multi** | **override** | **preorder** }
>
> DESCRIPTION
> ===========
> @@ -75,10 +75,13 @@ bpftool cgroup attach *CGROUP* *ATTACH_TYPE* *PROG* [*ATTACH_FLAGS*]
>     Attach program *PROG* to the cgroup *CGROUP* with attach type *ATTACH_TYPE*
>     and optional *ATTACH_FLAGS*.
>
> -    *ATTACH_FLAGS* can be one of: **override** if a sub-cgroup installs some
> +    *ATTACH_FLAGS* can include: **override** if a sub-cgroup installs some
>     bpf program, the program in this cgroup yields to sub-cgroup program;
>     **multi** if a sub-cgroup installs some bpf program, that cgroup program
> -    gets run in addition to the program in this cgroup.
> +    gets run in addition to the program in this cgroup;
> +    **preorder** requests that this program executes before programs attached
> +    further down the cgroup hierarchy during evaluation. **preorder** can be
> +    combined with **multi**.

Does the new description accurately capture BPF_F_PREORDER's ordering
behavior? The paragraph says preorder programs execute "before programs
attached further down the cgroup hierarchy", which focuses on descendant
cgroups. But compute_effective_progs() in kernel/bpf/cgroup.c places all
preorder programs before all non-preorder programs regardless of where they
are attached:

    fstart = preorder_cnt;
    bstart = preorder_cnt - 1;
    ...
        if (pl->flags & BPF_F_PREORDER) {
            item = &progs->items[bstart];
            bstart--;
        } else {
            item = &progs->items[fstart];
            fstart++;
        }

This means a preorder program runs before every non-preorder program,
including those attached in the same cgroup and in ancestor cgroups. Commit
4b82b181a26c's example shows root with p1,p2 and a child with p3,p4 where
p2 and p4 are preorder - the effective order is "p2 p4 p3 p1". So the
child's preorder p4 runs before the root's non-preorder p1, and p2 runs
before its same-cgroup sibling p1.

Also, the paragraph says preorder "can be combined with multi", but
do_attach() now accepts "preorder" as a standalone flag. When used alone
(without multi), does __cgroup_bpf_attach() stay in the NONE regime where
"Only one program is allowed to be attached to a cgroup"? The phrase "can
be combined" leaves it ambiguous whether preorder alone enables multi-prog
attachment.

>
>     Only one program is allowed to be attached to a cgroup with no attach flags
>     or the **override** flag. Attaching another program will release old

[ ... ]

> diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
> index 75cbcb512ebae..819757d7a3609 100644
> --- a/tools/bpf/bpftool/bash-completion/bpftool
> +++ b/tools/bpf/bpftool/bash-completion/bpftool
> @@ -1057,7 +1057,7 @@ _bpftool()
>                 attach|detach)
>                     local BPFTOOL_CGROUP_ATTACH_TYPES="$(bpftool feature list_builtins attach_types 2>/dev/null | \
>                         grep '^cgroup_')"
> -                    local ATTACH_FLAGS='multi override'
> +                    local ATTACH_FLAGS='multi override preorder'
>                     # Check for $prev = $command first
>                     if [ $prev = $command ]; then
>                         _filedir

Can bash completion handle combined flags after this change? The commit
message's After: example shows:

  $ bpftool cgroup attach <cg> cgroup_inet_ingress id 5678 multi preorder

and the rst documentation states "preorder can be combined with multi". But
ATTACH_FLAGS is passed to _bpftool_one_of_list(), which is defined as:

    _bpftool_one_of_list()
    {
        _bpftool_search_list $* && return 1
        COMPREPLY+=( $( compgen -W "$*" -- "$cur" ) )
    }

This helper returns early once any word from the list is already on the
command line, leaving COMPREPLY empty. That was correct when the list was
the mutually-exclusive pair {multi, override}, but now that preorder can
combine with multi, typing "... multi <TAB>" finds "multi" in the words
array and suppresses all completions. Won't this prevent completing the
second flag?

> diff --git a/tools/bpf/bpftool/cgroup.c b/tools/bpf/bpftool/cgroup.c
> index ce69d1e5468e3..aec1281c95d8e 100644
> --- a/tools/bpf/bpftool/cgroup.c
> +++ b/tools/bpf/bpftool/cgroup.c
> @@ -56,7 +56,7 @@ static const int cgroup_attach_types[] = {
> };
>
> #define HELP_SPEC_ATTACH_FLAGS						\
> -	"ATTACH_FLAGS := { multi | override }"
> +	"ATTACH_FLAGS := { multi | override | preorder }"

[ ... ]

> +static const char *format_attach_flags(__u32 attach_flags, char *buf, size_t sz)
> +{
> +	static const struct {
> +		__u32 flag;
> +		const char *name;
> +	} flags[] = {
> +		{ BPF_F_ALLOW_MULTI, "multi" },
> +		{ BPF_F_ALLOW_OVERRIDE, "override" },
> +		{ BPF_F_PREORDER, "preorder" },
> +	};
> +	size_t len = 0;
> +	size_t i;
> +	int n;
> +
> +	buf[0] = '\0';
> +	for (i = 0; i < ARRAY_SIZE(flags); i++) {
> +		if (attach_flags & flags[i].flag) {
> +			n = snprintf(buf + len, sz - len, "%s%s",
> +				     len ? "," : "", flags[i].name);
> +			if (n < 0 || (size_t)n >= sz - len)
> +				return buf;
> +			len += n;
> +			attach_flags &= ~flags[i].flag;
> +		}
> +	}
> +
> +	if (attach_flags)
> +		snprintf(buf + len, sz - len, "%sunknown(%x)",
> +			 len ? "," : "", attach_flags);
> +
> +	return buf;
> +}

[ ... ]

> @@ -593,6 +612,8 @@ static int do_attach(int argc, char **argv)
> 			attach_flags |= BPF_F_ALLOW_MULTI;
> 		} else if (is_prefix(argv[i], "override")) {
> 			attach_flags |= BPF_F_ALLOW_OVERRIDE;
> +		} else if (is_prefix(argv[i], "preorder")) {
> +			attach_flags |= BPF_F_PREORDER;
> 		} else {
> 			p_err("unknown option: %s", argv[i]);
> 			goto exit_cgroup;

[ ... ]

This looks like a bug fix for commit 4b82b181a26c. Should this include:

  Fixes: 4b82b181a26c ("bpf: Allow pre-ordering for bpf cgroup progs")


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/35376083935