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

[Quentin Monnet <qmo@kernel.org>]

2026-09-22 03:01 UTC+0000 ~ sashiko-bot@kernel.org
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] bpf: bpftool JSON ABI breakage for attach_flags field
> --
> 
> commit 7ba7f84eaca8177e2045dffce655aae4da559465
> Author: Hui Su <sh_def@163.com>
> 
> bpftool: Add support for BPF_F_PREORDER cgroup attach flag
> 
> This commit adds support for the BPF_F_PREORDER cgroup attach flag to
> bpftool. It updates attach operations to accept the new flag and modifies
> the show operations to output combinations of attach flags, rather than
> just a single flag.
> 
>> diff --git a/tools/bpf/bpftool/cgroup.c b/tools/bpf/bpftool/cgroup.c
>> index ce69d1e5468e3..fee1a260f6d1a 100644
>> --- a/tools/bpf/bpftool/cgroup.c
>> +++ b/tools/bpf/bpftool/cgroup.c
> 
> [ ... ]
> 
>> +static void show_attach_flags_json(__u32 flags)
>> +{
>> +	char buf[32];
>> +	size_t i;
>> +
>> +	jsonw_name(json_wtr, "attach_flags");
>> +	jsonw_start_array(json_wtr);
>> +
>> +	for (i = 0; i < ARRAY_SIZE(attach_flag_names); i++) {
>> +		if (!(flags & attach_flag_names[i].flag))
>> +			continue;
>> +
>> +		jsonw_string(json_wtr, attach_flag_names[i].name);
>> +		flags &= ~attach_flag_names[i].flag;
>> +	}
>> +
>> +	if (flags) {
>> +		snprintf(buf, sizeof(buf), "unknown(%x)", flags);
>> +		jsonw_string(json_wtr, buf);
>> +	}
>> +
>> +	jsonw_end_array(json_wtr);
>> +}
> 
> [ ... ]
> 
>> @@ -182,7 +240,7 @@ static int show_bpf_prog(int id, enum bpf_attach_type attach_type,
>>  		else
>>  			jsonw_uint_field(json_wtr, "attach_type", attach_type);
>>  		if (!(query_flags & BPF_F_QUERY_EFFECTIVE))
>> -			jsonw_string_field(json_wtr, "attach_flags", attach_flags_str);
>> +			show_attach_flags_json(attach_flags);
> 
> [Severity: Medium]
> Does this change break bpftool's JSON ABI by changing the attach_flags
> field from a string to an array?


It does. I suggested it, given that appending ",preorder" to the flag
may break parsing anyway, so we may just as well take this chance to use
the right JSON format for these flags. I'm fine taking in that change,
and documenting it in the next release notes on GitHub. Alexei may
disagree, though, so let's see.

Quentin


> Previously, this field was output as a single string. Changing the data type
> to an array could cause external scripts or applications using strict JSON
> parsers to crash or fail to unmarshal the output when running 
> bpftool cgroup show -j.
> 
> Would it be safer to format the multiple flags as a single comma-separated
> string in the JSON output, similar to what is done for the plain-text output,
> to maintain backwards compatibility?
> 
>>  		jsonw_string_field(json_wtr, "name", prog_name);
>>  		if (attach_btf_name)
>>  			jsonw_string_field(json_wtr, "attach_btf_name", attach_btf_name);
>