Re: [PATCH bpf-next v4 2/3] libbpf: add low level TC-BPF API

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

Daniel Borkmann <daniel@iogearbox.net> writes:

> On 4/23/21 5:05 PM, Kumar Kartikeya Dwivedi wrote:
> [...]
>>   tools/lib/bpf/libbpf.h   |  92 ++++++++
>>   tools/lib/bpf/libbpf.map |   5 +
>>   tools/lib/bpf/netlink.c  | 478 ++++++++++++++++++++++++++++++++++++++-
>>   3 files changed, 574 insertions(+), 1 deletion(-)
>> 
>> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
>> index bec4e6a6e31d..1c717c07b66e 100644
>> --- a/tools/lib/bpf/libbpf.h
>> +++ b/tools/lib/bpf/libbpf.h
>> @@ -775,6 +775,98 @@ LIBBPF_API int bpf_linker__add_file(struct bpf_linker *linker, const char *filen
>>   LIBBPF_API int bpf_linker__finalize(struct bpf_linker *linker);
>>   LIBBPF_API void bpf_linker__free(struct bpf_linker *linker);
>> 
>> +enum bpf_tc_attach_point {
>> +	BPF_TC_INGRESS,
>> +	BPF_TC_EGRESS,
>> +	BPF_TC_CUSTOM_PARENT,
>> +	_BPF_TC_PARENT_MAX,
>
> I don't think we need to expose _BPF_TC_PARENT_MAX as part of the API, I would drop
> the latter.
>
>> +};
>> +
>> +/* The opts structure is also used to return the created filters attributes
>> + * (e.g. in case the user left them unset). Some of the options that were left
>> + * out default to a reasonable value, documented below.
>> + *
>> + *	protocol - ETH_P_ALL
>> + *	chain index - 0
>> + *	class_id - 0 (can be set by bpf program using skb->tc_classid)
>> + *	bpf_flags - TCA_BPF_FLAG_ACT_DIRECT (direct action mode)
>> + *	bpf_flags_gen - 0
>> + *
>> + *	The user must fulfill documented requirements for each function.
>
> Not sure if this is overly relevant as part of the bpf_tc_opts in here. For the
> 2nd part, I would probably just mention that libbpf internally attaches the bpf
> programs with direct action mode. The hw offload may be future todo, and the other
> bits are little used anyway; mentioning them here, what value does it have to
> libbpf users? I'd rather just drop the 2nd part and/or simplify this paragraph
> just stating that the progs are attached in direct action mode.

The idea was that this would be for the benefit of people familiar with
TC concepts. Maybe simplify it to "Programs are attached in direct
action mode with a protocol value of 'all', and all other parameters
that the 'tc' binary supports will be set to 0"?

>> + */
>> +struct bpf_tc_opts {
>> +	size_t sz;
>> +	__u32 handle;
>> +	__u32 parent;
>> +	__u16 priority;
>> +	__u32 prog_id;
>> +	bool replace;
>> +	size_t :0;
>> +};
>> +
>> +#define bpf_tc_opts__last_field replace
>> +
>> +struct bpf_tc_ctx;
>> +
>> +struct bpf_tc_ctx_opts {
>> +	size_t sz;
>> +};
>> +
>> +#define bpf_tc_ctx_opts__last_field sz
>> +
>> +/* Requirements */
>> +/*
>> + * @ifindex: Must be > 0.
>> + * @parent: Must be one of the enum constants < _BPF_TC_PARENT_MAX
>> + * @opts: Can be NULL, currently no options are supported.
>> + */
>
> Up to Andrii, but we don't have such API doc in general inside libbpf.h, I
> would drop it for the time being to be consistent with the rest (same for
> others below).
>
>> +LIBBPF_API struct bpf_tc_ctx *bpf_tc_ctx_init(__u32 ifindex,
>
> nit: in user space s/__u32 ifindex/int ifindex/
>
>> +					      enum bpf_tc_attach_point parent,
>> +					      struct bpf_tc_ctx_opts *opts);
>
> Should we enforce opts being NULL or non-NULL here, or drop the arg from here
> for now altogether? (And if later versions of the functions show up this could
> be mapped to the right one?)

Hmm, extending later is easier if there's already an opts parameter. But
it could be declared 'void *' and enforced as always 0 for now?

>> +/*
>> + * @ctx: Can be NULL, if not, must point to a valid object.
>> + *	 If the qdisc was attached during ctx_init, it will be deleted if no
>> + *	 filters are attached to it.
>> + *	 When ctx == NULL, this is a no-op.
>> + */
>> +LIBBPF_API int bpf_tc_ctx_destroy(struct bpf_tc_ctx *ctx);
>> +/*
>> + * @ctx: Cannot be NULL.
>> + * @fd: Must be >= 0.
>> + * @opts: Cannot be NULL, prog_id must be unset, all other fields can be
>> + *	  optionally set. All fields except replace  will be set as per created
>> + *        filter's attributes. parent must only be set when attach_point of ctx is
>> + *        BPF_TC_CUSTOM_PARENT, otherwise parent must be unset.
>> + *
>> + * Fills the following fields in opts:
>> + *	handle
>> + *	parent
>> + *	priority
>> + *	prog_id
>> + */
>> +LIBBPF_API int bpf_tc_attach(struct bpf_tc_ctx *ctx, int fd,
>> +			     struct bpf_tc_opts *opts);
>> +/*
>> + * @ctx: Cannot be NULL.
>> + * @opts: Cannot be NULL, replace and prog_id must be unset, all other fields
>> + *	  must be set.
>> + */
>> +LIBBPF_API int bpf_tc_detach(struct bpf_tc_ctx *ctx,
>> +			     const struct bpf_tc_opts *opts);
>
> One thing that I find a bit odd from this API is that BPF_TC_INGRESS / BPF_TC_EGRESS
> needs to be set each time via bpf_tc_ctx_init(). So whenever a specific program would
> be attached to both we need to 're-init' in between just to change from hook a to b,
> whereas when you have BPF_TC_CUSTOM_PARENT, you could just use a different opts->parent
> without going this detour (unless the clsact wasn't loaded there in the first place).
>
> Could we add a BPF_TC_UNSPEC to enum bpf_tc_attach_point, which the user would pass to
> bpf_tc_ctx_init(), so that opts.direction = BPF_TC_INGRESS with subsequent bpf_tc_attach()
> can be called, and same opts.direction = BPF_TC_EGRESS with bpf_tc_attach() for different
> fd. The only thing we cared about in bpf_tc_ctx_init() resp. the ctx was that qdisc was
> ready.

This sounds workable - no objections from me :)

-Toke