Re: [PATCH bpf-next v2 1/4] libbpf: Move section-defined program flags to prog_flags

["Toke Høiland-Jørgensen" <toke@redhat.com>]

bot+bpf-ci@kernel.org writes:

>> commit 24627f60026bd2b231e09d4373e14ca1975a2d96
>> Author: Toke Høiland-Jørgensen <toke@redhat.com>
>> Date:   Mon Aug 31 15:26:41 2026 +0200
>>
>>     libbpf: Move section-defined program flags to prog_flags
>>
>>     The libbpf section definition modifiers for XDP frags support and
>>     sleepable programs stores the flags bits only in the private section
>>     definition cookie from object open to load time. This has the
>>     unfortunate consequence that API consumers cannot see (or manipulate)
>>     the flag between object open and program load.
>>
>>     In particular, libxdp has special handling of frags-enabled programs to
>>     make them compatible with the dispatcher. This doesn't work on XDP
>>     programs that enable frags through the 'xdp.frags' section definition
>>     because the flag is not visible through bpf_program__flags()[0].
>>
>>     Fix this by changing how libbpf loads the program flags from section
>>     definitions: instead of using the private section definition cookie, add
>>     a setup function to the default section definitions that stores the
>>     flags for sleepable and XDP frags programs in the prog_flags field of
>>     struct bpf_program.
>>
>>     Exposing the flags this way means that any use of
>>     bpf_program__set_flags() will override the flags unless the caller takes
>>     care of updating flags in a non-destructive way. This is unavoidable
>>     with the set-only API, and any user setting flags unconditionally is
>>     already broken in the sense that they will also override any other
>>     current and future flags. A subsequent patch fixes up all in-tree users
>>     of the API.
>
> This looks like a bug fix for SEC(\"xdp.frags\") flag visibility. Should
> this include:
>
>   Fixes: 082c4bfba4f7 ("libbpf: Add SEC name for xdp frags programs")

Sure, why not?

> The commit message states "A subsequent patch fixes up all in-tree users
> of the API." Is this statement accurate?
>
> Looking at the end of the series (645bdf199b3cb), there is one destructive
> bpf_program__set_flags() call site that remains unconverted:
>
>     tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c:38
>         bpf_program__set_flags(prog, testing_prog_flags());

As I replied to the other bot (why are there so many bots?), I missed
this one. Will add and respin.

[...]

> Is keeping the flags across a type change intended? If so, that seems worth
> noting in the changelog. If not, should the type check be kept or should
> bpf_program__set_type() clear the SEC-derived bits?

Sure, will note this in the patch description.

[...]

> Should the in-tree user fix-ups land before (or be folded into) this commit
> to avoid the bisection hazard? The usual kernel ordering is to fix the
> users first so that no intermediate commit regresses.

Sure, why not? Will reorder in the next version.

>> @@ -10099,6 +10106,7 @@ int bpf_program__clone(struct bpf_program *prog, const struct bpf_prog_load_opts
>>  	.prog_type = BPF_PROG_TYPE_##ptype,				    \
>>  	.expected_attach_type = atype,					    \
>>  	.cookie = (long)(flags),					    \
>> +	.prog_setup_fn = libbpf_setup_prog_flags,			    \
>>  	.prog_prepare_load_fn = libbpf_prepare_load_fn,		    \
>>  	__VA_ARGS__							    \
>>  }
>
> Does bpf_program__clone() lose the SEC()-derived flags whenever the caller
> supplies a non-zero opts->prog_flags?
>
> Before this patch, bpf_program__clone() got the SEC flags back from the
> sec_def callback it invokes:
>
>     if (prog->sec_def && prog->sec_def->prog_prepare_load_fn)
>         err = prog->sec_def->prog_prepare_load_fn(prog, &attr,
>                                                    prog->sec_def->cookie);
>
> which used to do opts->prog_flags |= BPF_F_SLEEPABLE / |= BPF_F_XDP_HAS_FRAGS.
> So even when the caller passed its own prog_flags, a SEC("fentry.s/...") or
> SEC("xdp.frags") program was still cloned with the right bit set.
>
> After this patch libbpf_prepare_prog_load() no longer touches
> opts->prog_flags, so this line in bpf_program__clone():
>
>     attr.prog_flags = OPTS_GET(opts, prog_flags, 0) ?: prog->prog_flags;
>
> discards prog->prog_flags entirely as soon as the caller sets any flag of
> its own. Cloning a sleepable program with, say, opts.prog_flags =
> BPF_F_TEST_STATE_FREQ now loads it without BPF_F_SLEEPABLE, which
> kernel/bpf/verifier.c:20602-20610 rejects for BPF_PROG_TYPE_SYSCALL and for
> sleepable-only LSM/tracing attach points (-EINVAL).
>
> No in-tree caller triggers this today: the only opts-passing caller,
> process_prog() in tools/testing/selftests/bpf/veristat.c:1730-1737, leaves
> opts.prog_flags at 0, and none of the follow-up commits in this series add
> such a caller. Would it be safer to OR the caller's flags into
> prog->prog_flags, or is the all-or-nothing fallback intended to let callers
> override SEC-derived flags explicitly?

Yes, giving the caller control over the flags is the point of this
patch, so this is expected.

-Toke