Re: [PATCH bpf-next 1/2] libbpf: Move XDP frags flag to prog_flags

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

Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Thu, Aug 27, 2026 at 6:27 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> bot+bpf-ci@kernel.org writes:
>>
>> >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> >> index b749c01742ee0..7d6f0fe518d99 100644
>> >> --- a/tools/lib/bpf/libbpf.c
>> >> +++ b/tools/lib/bpf/libbpf.c
>> >
>> > [ ... ]
>> >
>> >> @@ -7879,6 +7877,12 @@ static int tracing_multi_mod_fd(struct bpf_program *prog, int *btf_obj_fd)
>> >>      return 0;
>> >>  }
>> >>
>> >> +static int setup_xdp_frags(struct bpf_program *prog, long cookie)
>> >> +{
>> >> +    prog->prog_flags |= BPF_F_XDP_HAS_FRAGS;
>> >> +    return 0;
>> >> +}
>> >
>> > Does storing BPF_F_XDP_HAS_FRAGS in prog->prog_flags here break callers
>> > that use bpf_program__set_flags()?
>> >
>> > bpf_program__set_flags() overwrites rather than OR-ing:
>> >
>> >     int bpf_program__set_flags(struct bpf_program *prog, __u32 flags)
>> >     {
>> >             ...
>> >             prog->prog_flags = flags;
>> >     }
>> >
>> > Previously the flag was OR'd into the load-time opts by
>> > libbpf_prepare_prog_load(), so it survived any user manipulation of
>> > prog_flags between open and load. The new code latches it into
>> > prog->prog_flags at open time, so it's lost whenever a caller assigns
>> > prog_flags instead of OR-ing.
>>
>> Making it possible to manipulate the flag after loading is the whole
>> point of the patch, so this is expected. As for users like:
>>
>> > bpf_object_load_prog() passes prog->prog_flags straight to the kernel:
>> >
>> >     load_attr.prog_flags = prog->prog_flags;
>> >
>> > Two in-tree callers do exactly that on SEC("xdp.frags") programs.
>> >
>> > bpftool (tools/bpf/bpftool/prog.c), i.e. 'bpftool prog load FILE PIN
>> > xdpmeta_dev DEV':
>> >
>> >     if (prog_type == BPF_PROG_TYPE_XDP && xdpmeta_ifindex) {
>> >             bpf_program__set_flags(pos, BPF_F_XDP_DEV_BOUND_ONLY);
>> >             bpf_program__set_ifindex(pos, xdpmeta_ifindex);
>>
>> ...doing a set_flags without preserving the existing flags will lead to
>> clobbering, that's what's implied by the API? So any users doing that
>> are buggy, I'd argue.
>>
>> > Keeping the load-time 'opts->prog_flags
>> > |= BPF_F_XDP_HAS_FRAGS' in addition to the new prog_flags initialisation,
>> > or having bpf_program__set_flags() preserve section-implied bits, would give
>> > visibility without breaking existing callers.
>>
>> Both of these options will still make it impossible to turn off the
>> frags bit after loading the object. I guess we could do the "preserve
>> section-implied bits" part and add an explicit
>> bpf_program__clear_flags() to remove everything. What do others think?
>>
>> >>  /* this is called as prog->sec_def->prog_prepare_load_fn for libbpf-supported sec_defs */
>> >>  static int libbpf_prepare_prog_load(struct bpf_program *prog,
>> >>                                  struct bpf_prog_load_opts *opts, long cookie)
>> >>  @@ -7892,9 +7896,6 @@ static int libbpf_prepare_prog_load(struct bpf_program *prog,
>> >>      if (def & SEC_SLEEPABLE)
>> >>              opts->prog_flags |= BPF_F_SLEEPABLE;
>> >>
>> >> -    if (prog->type == BPF_PROG_TYPE_XDP && (def & SEC_XDP_FRAGS))
>> >> -            opts->prog_flags |= BPF_F_XDP_HAS_FRAGS;
>> >> -
>> >
>> > Should setup_xdp_frags() keep a program type guard?
>>
>> No, the setup callback callback is called too early for this to make
>> sense. However:
>>
>> > The trigger requires a deliberate retype, but the guard removal is a
>> > behaviour change that the changelog does not mention. If the flag is meant
>> > to be XDP-only, should setup_xdp_frags() keep the type test or should
>> > bpf_program__set_type() drop the bit when moving away from
>> > BPF_PROG_TYPE_XDP?
>>
>> Clearing the type-specific flags on type change could make sense. I can
>> add that in v2 if others agree?
>
> no, let's not.
>
> But instead of making this XDP-specific custom callback, let's have a
> generic default libbpf setup callback that will do the same for
> BPF_F_SLEEPABLE, seems a fair game (and technically will allow to
> dynamically downgrade sleepable to non-sleepable, if there is ever any
> good reason to do that)

Alright, will do. I'll also fix up the destructive use of
bpf_program__set_flags() in the selftests, then :)

-Toke