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

[Daniel Borkmann <daniel@iogearbox.net>]

On 4/22/21 1:30 AM, Kumar Kartikeya Dwivedi wrote:
> On Thu, Apr 22, 2021 at 04:51:55AM IST, Daniel Borkmann wrote:
>> On 4/22/21 1:08 AM, Kumar Kartikeya Dwivedi wrote:
>>> On Thu, Apr 22, 2021 at 04:29:28AM IST, Daniel Borkmann wrote:
>>>> On 4/20/21 9:37 PM, Kumar Kartikeya Dwivedi wrote:
>>>> [...]
>>>>> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
>>>>> index bec4e6a6e31d..b4ed6a41ea70 100644
>>>>> --- a/tools/lib/bpf/libbpf.h
>>>>> +++ b/tools/lib/bpf/libbpf.h
>>>>> @@ -16,6 +16,8 @@
>>>>>     #include <stdbool.h>
>>>>>     #include <sys/types.h>  // for size_t
>>>>>     #include <linux/bpf.h>
>>>>> +#include <linux/pkt_sched.h>
>>>>> +#include <linux/tc_act/tc_bpf.h>
>>>>>     #include "libbpf_common.h"
>>>>> @@ -775,6 +777,48 @@ 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);
>>>>> +/* Convenience macros for the clsact attach hooks */
>>>>> +#define BPF_TC_CLSACT_INGRESS TC_H_MAKE(TC_H_CLSACT, TC_H_MIN_INGRESS)
>>>>> +#define BPF_TC_CLSACT_EGRESS TC_H_MAKE(TC_H_CLSACT, TC_H_MIN_EGRESS)
>>>>
>>>> I would abstract those away into an enum, plus avoid having to pull in
>>>> linux/pkt_sched.h and linux/tc_act/tc_bpf.h from main libbpf.h header.
>>>>
>>>> Just add a enum { BPF_TC_DIR_INGRESS, BPF_TC_DIR_EGRESS, } and then the
>>>> concrete tc bits (TC_H_MAKE()) can be translated internally.
>>>
>>> Ok, will do.
>>>
>>>>> +struct bpf_tc_opts {
>>>>> +	size_t sz;
>>>>
>>>> Is this set anywhere?
>>>
>>> This is needed by the OPTS_* infrastructure.
>>>
>>>>> +	__u32 handle;
>>>>> +	__u32 class_id;
>>>>
>>>> I'd remove class_id from here as well given in direct-action a BPF prog can
>>>> set it if needed.
>>>
>>> Ok, makes sense.
>>>
>>>>> +	__u16 priority;
>>>>> +	bool replace;
>>>>> +	size_t :0;
>>>>
>>>> What's the rationale for this padding?
>>>
>>> dde7b3f5f2f4 ("libbpf: Add explicit padding to bpf_xdp_set_link_opts")
>>
>> Hm, fair enough.
>>
>>>>> +};
>>>>> +
>>>>> +#define bpf_tc_opts__last_field replace
>>>>> +
>>>>> +/* Acts as a handle for an attached filter */
>>>>> +struct bpf_tc_attach_id {
>>>>
>>>> nit: maybe bpf_tc_ctx
>>>
>>> Noted.
>>>
>>>>> +	__u32 handle;
>>>>> +	__u16 priority;
>>>>> +};
>>>>> +
>>>>> +struct bpf_tc_info {
>>>>> +	struct bpf_tc_attach_id id;
>>>>> +	__u16 protocol;
>>>>> +	__u32 chain_index;
>>>>> +	__u32 prog_id;
>>>>> +	__u8 tag[BPF_TAG_SIZE];
>>>>> +	__u32 class_id;
>>>>> +	__u32 bpf_flags;
>>>>> +	__u32 bpf_flags_gen;
>>>>
>>>> Given we do not yet have any setters e.g. for offload, etc, the one thing
>>>> I'd see useful and crucial initially is prog_id.
>>>>
>>>> The protocol, chain_index, and I would also include tag should be dropped.
>>>
>>> A future user of this API needs to know the tag, so I would like to keep that.
>>> The rest we can drop, and probably document the default values explicitly.
>>
>> Couldn't this be added along with the future patch for the [future] user?
> 
> True.
> 
>> The tag should be the tag of the prog itself, so if you have prog_id, you
>> could also retrieve the same tag from the prog. The tag was basically from
>> the early days where we didn't have bpf_prog_get_info_by_fd().
>>
>> What does that future user need to do different here?
> 
>  From Shaun Crampton:
> "My particular use case is to load a program, link it with its maps and then
> check if its tag matches the existing program on the interface (and if so, abort
> the update)"
> 
> Also CC'd, they would be able to elaborate better, and whether or not dropping
> it is ok.

Nope, just get it from the prog itself.