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

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

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?

-Toke