| Message ID | 20210423150600.498490-3-memxor@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Add TC-BPF API | expand |
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. > + */ > +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?) > +/* > + * @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. > +/* > + * @ctx: Cannot be NULL. > + * @opts: Cannot be NULL, replace and prog_id must be unset, all other fields > + * must be set. > + * > + * Fills the following fields in opts: > + * handle > + * parent > + * priority > + * prog_id > + */ > +LIBBPF_API int bpf_tc_query(struct bpf_tc_ctx *ctx, > + struct bpf_tc_opts *opts); > + > #ifdef __cplusplus > } /* extern "C" */ > #endif
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
On Tue, Apr 27, 2021 at 08:34:30PM IST, Daniel Borkmann wrote: > 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. > Ok, will drop. > > +}; > > + > > +/* 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 goal was to just document whatever attributes were set to by default, but I can see your point. I'll trim it. > > + */ > > +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). > I think we need to keep this somewhere. We dropped bpf_tc_info since it wasn't really serving any purpose, but that meant we would put the only extra thing it returned (prog_id) into bpf_tc_opts. That means we also need to take care that it is unset (along with replace) in functions where it isn't used, to allow for reuse for some future purpose. If we don't document that the user needs to unset them whenever working with bpf_tc_query and bpf_tc_detach, how are they supposed to know? Maybe a man page and/or a blog post would be better? Just putting it above the function seemed best for now. > > +LIBBPF_API struct bpf_tc_ctx *bpf_tc_ctx_init(__u32 ifindex, > > nit: in user space s/__u32 ifindex/int ifindex/ > Ok. > > + 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?) > I can enforce it to be NULL for now, but I'd rather keep it this way than add another with opts later. The way this works, you could extend it to attach other kinds of qdiscs (by taking a simple config in opts), which I think can be very useful. > > +/* > > + * @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). > Currently I check that opts->parent is unset when BPF_TC_INGRESS or BPF_TC_EGRESS is set as attach point. But since both map to clsact, we could allow the user to specify opts->parent as BPF_TC_INGRESS or BPF_TC_EGRESS (no need to use TC_H_MAKE, we can detect it from ctx->parent that it won't be a parent id). This would mean that by default attach point is what you set for ctx, but for bpf_tc_attach you can temporarily override to be some other attach point (for the same qdisc). You still won't be able to set anything other than the two though. > 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. > > > +/* > > + * @ctx: Cannot be NULL. > > + * @opts: Cannot be NULL, replace and prog_id must be unset, all other fields > > + * must be set. > > + * > > + * Fills the following fields in opts: > > + * handle > > + * parent > > + * priority > > + * prog_id > > + */ > > +LIBBPF_API int bpf_tc_query(struct bpf_tc_ctx *ctx, > > + struct bpf_tc_opts *opts); > > + > > #ifdef __cplusplus > > } /* extern "C" */ > > #endif -- Kartikeya
Kumar Kartikeya Dwivedi <memxor@gmail.com> writes: > On Tue, Apr 27, 2021 at 08:34:30PM IST, Daniel Borkmann wrote: >> 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. >> > > Ok, will drop. > >> > +}; >> > + >> > +/* 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 goal was to just document whatever attributes were set to by default, but I can see > your point. I'll trim it. > >> > + */ >> > +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). >> > > I think we need to keep this somewhere. We dropped bpf_tc_info since it wasn't > really serving any purpose, but that meant we would put the only extra thing it > returned (prog_id) into bpf_tc_opts. That means we also need to take care that > it is unset (along with replace) in functions where it isn't used, to allow for > reuse for some future purpose. If we don't document that the user needs to unset > them whenever working with bpf_tc_query and bpf_tc_detach, how are they supposed > to know? > > Maybe a man page and/or a blog post would be better? Just putting it above the > function seemed best for now. You could document it together with the struct definition instead of for each function? See the inline comments in bpf_object_open_opts for instance... -Toke
On Tue, Apr 27, 2021 at 8:04 AM Daniel Borkmann <daniel@iogearbox.net> wrote: > > 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. > > > + */ > > +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). +1 > > > +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?) > OPTS_VALID check handles all the cases. Opts are always allowed to be NULL. If it's not null, all the bytes beyond what current libbpf version supports should be zero. All that is handled by OPTS_VALID, so I don't think anything extra needs to be checked. > > +/* > > + * @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. > > > +/* > > + * @ctx: Cannot be NULL. > > + * @opts: Cannot be NULL, replace and prog_id must be unset, all other fields > > + * must be set. > > + * > > + * Fills the following fields in opts: > > + * handle > > + * parent > > + * priority > > + * prog_id > > + */ > > +LIBBPF_API int bpf_tc_query(struct bpf_tc_ctx *ctx, > > + struct bpf_tc_opts *opts); > > + > > #ifdef __cplusplus > > } /* extern "C" */ > > #endif
On 4/27/21 8:02 PM, Kumar Kartikeya Dwivedi wrote: > On Tue, Apr 27, 2021 at 08:34:30PM IST, Daniel Borkmann wrote: >> On 4/23/21 5:05 PM, Kumar Kartikeya Dwivedi wrote: [...] >>> +/* >>> + * @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). > > Currently I check that opts->parent is unset when BPF_TC_INGRESS or BPF_TC_EGRESS > is set as attach point. But since both map to clsact, we could allow the user to > specify opts->parent as BPF_TC_INGRESS or BPF_TC_EGRESS (no need to use > TC_H_MAKE, we can detect it from ctx->parent that it won't be a parent id). This > would mean that by default attach point is what you set for ctx, but for > bpf_tc_attach you can temporarily override to be some other attach point (for > the same qdisc). You still won't be able to set anything other than the two > though. I think the assumption on auto-detecting the parent id in that case might not hold given major number could very well be 0. Wrt BPF_TC_UNSPEC ... maybe it's not even needed, back to drawing board ... Here's how the whole API could look like, usage examples below: enum bpf_tc_attach_point { BPF_TC_INGRESS = 1 << 0, BPF_TC_EGRESS = 1 << 1, BPF_TC_CUSTOM = 1 << 2, }; enum bpf_tc_attach_flags { BPF_TC_F_REPLACE = 1 << 0, }; struct bpf_tc_hook { size_t sz; int ifindex; enum bpf_tc_attach_point which; __u32 parent; size_t :0; }; struct bpf_tc_opts { size_t sz; __u32 handle; __u16 priority; union { int prog_fd; __u32 prog_id; }; size_t :0; }; LIBBPF_API int bpf_tc_hook_create(struct bpf_tc_hook *hook); LIBBPF_API int bpf_tc_hook_destroy(struct bpf_tc_hook *hook); LIBBPF_API int bpf_tc_attach(const struct bpf_tc_hook *hook, const struct bpf_tc_opts *opts, int flags); LIBBPF_API int bpf_tc_detach(const struct bpf_tc_hook *hook, const struct bpf_tc_opts *opts); LIBBPF_API int bpf_tc_query(const struct bpf_tc_hook *hook, struct bpf_tc_opts *opts); So a user could do just: DECLARE_LIBBPF_OPTS(bpf_tc_hook, hook, .ifindex = 42, .which = BPF_TC_INGRESS); DECLARE_LIBBPF_OPTS(bpf_tc_opts, opts, .handle = 1, .priority = 1, .prog_fd = fd); err = bpf_tc_attach(&hook, &opts, BPF_TC_F_REPLACE); [...] If it's not known whether the hook exists, then a preceding call to: err = bpf_tc_hook_create(&hook); [...] The bpf_tc_query() would look like: DECLARE_LIBBPF_OPTS(bpf_tc_hook, hook, .ifindex = 42, .which = BPF_TC_EGRESS); DECLARE_LIBBPF_OPTS(bpf_tc_opts, opts, .handle = 1, .priority = 1); err = bpf_tc_query(&hook, &opts); if (!err) { [...] // gives access to: opts.prog_id } The bpf_tc_detach(): DECLARE_LIBBPF_OPTS(bpf_tc_hook, hook, .ifindex = 42, .which = BPF_TC_INGRESS); DECLARE_LIBBPF_OPTS(bpf_tc_opts, opts, .handle = 1, .priority = 1); err = bpf_tc_detach(&hook, &opts); [...] The nice thing would be that hook and opts are kept semantically separate, meaning with hook you can iterate though a bunch of devs and ingress/egress locations without changing opts, whereas with opts you could iterate on the cls_bpf instance itself w/o changing hook. Both are kept extensible via DECLARE_LIBBPF_OPTS(). Now the bpf_tc_hook_destroy() one: DECLARE_LIBBPF_OPTS(bpf_tc_hook, hook, .ifindex = 42, .which = BPF_TC_INGRESS|BPF_TC_EGRESS); err = bpf_tc_hook_destroy(&hook, &opts); [...] For triggering a remove of the clsact qdisc on the device, both directions are passed in. Combining both is only ever allowed for bpf_tc_hook_destroy(). If /only/ BPF_TC_INGRESS or only BPF_TC_EGRESS is passed, it could flush their lists (aka equivalent of `tc filter del dev eth0 ingress` and `tc filter del dev eth0 egress` command). For bpf_tc_hook_{create,destroy}() with BPF_TC_CUSTOM, we just return -EINVAL or -EOPNOTSUPP. I think the above interface would work nicely and feels intuitive while being extensible. Thoughts? Thanks, Daniel
On 4/27/21 11:55 PM, Daniel Borkmann wrote: > On 4/27/21 8:02 PM, Kumar Kartikeya Dwivedi wrote: >> On Tue, Apr 27, 2021 at 08:34:30PM IST, Daniel Borkmann wrote: >>> On 4/23/21 5:05 PM, Kumar Kartikeya Dwivedi wrote: > [...] >>>> +/* >>>> + * @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). >> >> Currently I check that opts->parent is unset when BPF_TC_INGRESS or BPF_TC_EGRESS >> is set as attach point. But since both map to clsact, we could allow the user to >> specify opts->parent as BPF_TC_INGRESS or BPF_TC_EGRESS (no need to use >> TC_H_MAKE, we can detect it from ctx->parent that it won't be a parent id). This >> would mean that by default attach point is what you set for ctx, but for >> bpf_tc_attach you can temporarily override to be some other attach point (for >> the same qdisc). You still won't be able to set anything other than the two >> though. > > I think the assumption on auto-detecting the parent id in that case might not hold given > major number could very well be 0. Wrt BPF_TC_UNSPEC ... maybe it's not even needed, back > to drawing board ... > > Here's how the whole API could look like, usage examples below: > > enum bpf_tc_attach_point { > BPF_TC_INGRESS = 1 << 0, > BPF_TC_EGRESS = 1 << 1, > BPF_TC_CUSTOM = 1 << 2, > }; > > enum bpf_tc_attach_flags { > BPF_TC_F_REPLACE = 1 << 0, > }; > > struct bpf_tc_hook { > size_t sz; > int ifindex; > enum bpf_tc_attach_point which; > __u32 parent; > size_t :0; > }; > > struct bpf_tc_opts { > size_t sz; > __u32 handle; > __u16 priority; > union { > int prog_fd; > __u32 prog_id; > }; > size_t :0; > }; > > LIBBPF_API int bpf_tc_hook_create(struct bpf_tc_hook *hook); > LIBBPF_API int bpf_tc_hook_destroy(struct bpf_tc_hook *hook); > > LIBBPF_API int bpf_tc_attach(const struct bpf_tc_hook *hook, const struct bpf_tc_opts *opts, int flags); > LIBBPF_API int bpf_tc_detach(const struct bpf_tc_hook *hook, const struct bpf_tc_opts *opts); > LIBBPF_API int bpf_tc_query(const struct bpf_tc_hook *hook, struct bpf_tc_opts *opts); > > So a user could do just: > > DECLARE_LIBBPF_OPTS(bpf_tc_hook, hook, .ifindex = 42, .which = BPF_TC_INGRESS); > DECLARE_LIBBPF_OPTS(bpf_tc_opts, opts, .handle = 1, .priority = 1, .prog_fd = fd); > > err = bpf_tc_attach(&hook, &opts, BPF_TC_F_REPLACE); > [...] > > If it's not known whether the hook exists, then a preceding call to: > > err = bpf_tc_hook_create(&hook); > [...] > > The bpf_tc_query() would look like: > > DECLARE_LIBBPF_OPTS(bpf_tc_hook, hook, .ifindex = 42, .which = BPF_TC_EGRESS); > DECLARE_LIBBPF_OPTS(bpf_tc_opts, opts, .handle = 1, .priority = 1); > > err = bpf_tc_query(&hook, &opts); > if (!err) { > [...] // gives access to: opts.prog_id > } > > The bpf_tc_detach(): > > DECLARE_LIBBPF_OPTS(bpf_tc_hook, hook, .ifindex = 42, .which = BPF_TC_INGRESS); > DECLARE_LIBBPF_OPTS(bpf_tc_opts, opts, .handle = 1, .priority = 1); > > err = bpf_tc_detach(&hook, &opts); > [...] > > The nice thing would be that hook and opts are kept semantically separate, meaning with > hook you can iterate though a bunch of devs and ingress/egress locations without changing > opts, whereas with opts you could iterate on the cls_bpf instance itself w/o changing > hook. Both are kept extensible via DECLARE_LIBBPF_OPTS(). > > Now the bpf_tc_hook_destroy() one: > > DECLARE_LIBBPF_OPTS(bpf_tc_hook, hook, .ifindex = 42, .which = BPF_TC_INGRESS|BPF_TC_EGRESS); > > err = bpf_tc_hook_destroy(&hook); > [...] > > For triggering a remove of the clsact qdisc on the device, both directions are passed in. > Combining both is only ever allowed for bpf_tc_hook_destroy(). Small addendum: DECLARE_LIBBPF_OPTS(bpf_tc_hook, hook, .ifindex = 42, .which = BPF_TC_INGRESS|BPF_TC_EGRESS); err = bpf_tc_hook_create(&hook); [...] ... is also possible, of course, and then both bpf_tc_hook_{create,destroy}() are symmetric. > If /only/ BPF_TC_INGRESS or only BPF_TC_EGRESS is passed, it could flush their lists (aka > equivalent of `tc filter del dev eth0 ingress` and `tc filter del dev eth0 egress` command). > > For bpf_tc_hook_{create,destroy}() with BPF_TC_CUSTOM, we just return -EINVAL or -EOPNOTSUPP. > > I think the above interface would work nicely and feels intuitive while being extensible. > Thoughts? > > Thanks, > Daniel
On 4/28/21 12:05 AM, Daniel Borkmann wrote: > On 4/27/21 11:55 PM, Daniel Borkmann wrote: >> On 4/27/21 8:02 PM, Kumar Kartikeya Dwivedi wrote: >>> On Tue, Apr 27, 2021 at 08:34:30PM IST, Daniel Borkmann wrote: >>>> On 4/23/21 5:05 PM, Kumar Kartikeya Dwivedi wrote: >> [...] >>>>> +/* >>>>> + * @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). >>> >>> Currently I check that opts->parent is unset when BPF_TC_INGRESS or BPF_TC_EGRESS >>> is set as attach point. But since both map to clsact, we could allow the user to >>> specify opts->parent as BPF_TC_INGRESS or BPF_TC_EGRESS (no need to use >>> TC_H_MAKE, we can detect it from ctx->parent that it won't be a parent id). This >>> would mean that by default attach point is what you set for ctx, but for >>> bpf_tc_attach you can temporarily override to be some other attach point (for >>> the same qdisc). You still won't be able to set anything other than the two >>> though. >> >> I think the assumption on auto-detecting the parent id in that case might not hold given >> major number could very well be 0. Wrt BPF_TC_UNSPEC ... maybe it's not even needed, back >> to drawing board ... >> >> Here's how the whole API could look like, usage examples below: And one last follow-up thought: >> enum bpf_tc_attach_point { >> BPF_TC_INGRESS = 1 << 0, >> BPF_TC_EGRESS = 1 << 1, >> BPF_TC_CUSTOM = 1 << 2, >> }; >> >> enum bpf_tc_attach_flags { >> BPF_TC_F_REPLACE = 1 << 0, >> }; >> >> struct bpf_tc_hook { >> size_t sz; >> int ifindex; >> enum bpf_tc_attach_point which; >> __u32 parent; >> size_t :0; >> }; >> >> struct bpf_tc_opts { >> size_t sz; >> __u32 handle; >> __u16 priority; To avoid gaps, priority could be __u32 as well, but we need to enforce [0, u16max]. >> union { >> int prog_fd; >> __u32 prog_id; >> }; We should rather remove the union to make it less error-prone. >> size_t :0; >> }; >> >> LIBBPF_API int bpf_tc_hook_create(struct bpf_tc_hook *hook); >> LIBBPF_API int bpf_tc_hook_destroy(struct bpf_tc_hook *hook); >> >> LIBBPF_API int bpf_tc_attach(const struct bpf_tc_hook *hook, const struct bpf_tc_opts *opts, int flags); Changing s/const struct bpf_tc_opts *opts/struct bpf_tc_opts *opts/ so that if a user did: DECLARE_LIBBPF_OPTS(bpf_tc_hook, hook, .ifindex = 42, .which = BPF_TC_INGRESS); DECLARE_LIBBPF_OPTS(bpf_tc_opts, opts, .prog_fd = fd); err = bpf_tc_attach(&hook, &opts, 0); [...] Then we'd rely on the kernel (cls_bpf) to auto-allocate handle/prio. libbpf in that case would populate opts.handle and opts.priority upon success, which can then later be used again for bpf_tc_detach() / bpf_tc_query() calls. >> LIBBPF_API int bpf_tc_detach(const struct bpf_tc_hook *hook, const struct bpf_tc_opts *opts); >> LIBBPF_API int bpf_tc_query(const struct bpf_tc_hook *hook, struct bpf_tc_opts *opts); >> >> So a user could do just: >> >> DECLARE_LIBBPF_OPTS(bpf_tc_hook, hook, .ifindex = 42, .which = BPF_TC_INGRESS); >> DECLARE_LIBBPF_OPTS(bpf_tc_opts, opts, .handle = 1, .priority = 1, .prog_fd = fd); >> >> err = bpf_tc_attach(&hook, &opts, BPF_TC_F_REPLACE); >> [...] >> >> If it's not known whether the hook exists, then a preceding call to: >> >> err = bpf_tc_hook_create(&hook); >> [...] >> >> The bpf_tc_query() would look like: >> >> DECLARE_LIBBPF_OPTS(bpf_tc_hook, hook, .ifindex = 42, .which = BPF_TC_EGRESS); >> DECLARE_LIBBPF_OPTS(bpf_tc_opts, opts, .handle = 1, .priority = 1); >> >> err = bpf_tc_query(&hook, &opts); >> if (!err) { >> [...] // gives access to: opts.prog_id >> } >> >> The bpf_tc_detach(): >> >> DECLARE_LIBBPF_OPTS(bpf_tc_hook, hook, .ifindex = 42, .which = BPF_TC_INGRESS); >> DECLARE_LIBBPF_OPTS(bpf_tc_opts, opts, .handle = 1, .priority = 1); >> >> err = bpf_tc_detach(&hook, &opts); >> [...] >> >> The nice thing would be that hook and opts are kept semantically separate, meaning with >> hook you can iterate though a bunch of devs and ingress/egress locations without changing >> opts, whereas with opts you could iterate on the cls_bpf instance itself w/o changing >> hook. Both are kept extensible via DECLARE_LIBBPF_OPTS(). >> >> Now the bpf_tc_hook_destroy() one: >> >> DECLARE_LIBBPF_OPTS(bpf_tc_hook, hook, .ifindex = 42, .which = BPF_TC_INGRESS|BPF_TC_EGRESS); >> >> err = bpf_tc_hook_destroy(&hook); >> [...] >> >> For triggering a remove of the clsact qdisc on the device, both directions are passed in. >> Combining both is only ever allowed for bpf_tc_hook_destroy(). > > Small addendum: > > DECLARE_LIBBPF_OPTS(bpf_tc_hook, hook, .ifindex = 42, .which = BPF_TC_INGRESS|BPF_TC_EGRESS); > > err = bpf_tc_hook_create(&hook); > [...] > > ... is also possible, of course, and then both bpf_tc_hook_{create,destroy}() are symmetric. > >> If /only/ BPF_TC_INGRESS or only BPF_TC_EGRESS is passed, it could flush their lists (aka >> equivalent of `tc filter del dev eth0 ingress` and `tc filter del dev eth0 egress` command). >> >> For bpf_tc_hook_{create,destroy}() with BPF_TC_CUSTOM, we just return -EINVAL or -EOPNOTSUPP. >> >> I think the above interface would work nicely and feels intuitive while being extensible. >> Thoughts? >> >> Thanks, >> Daniel
Daniel Borkmann <daniel@iogearbox.net> writes: > On 4/27/21 11:55 PM, Daniel Borkmann wrote: >> On 4/27/21 8:02 PM, Kumar Kartikeya Dwivedi wrote: >>> On Tue, Apr 27, 2021 at 08:34:30PM IST, Daniel Borkmann wrote: >>>> On 4/23/21 5:05 PM, Kumar Kartikeya Dwivedi wrote: >> [...] >>>>> +/* >>>>> + * @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). >>> >>> Currently I check that opts->parent is unset when BPF_TC_INGRESS or BPF_TC_EGRESS >>> is set as attach point. But since both map to clsact, we could allow the user to >>> specify opts->parent as BPF_TC_INGRESS or BPF_TC_EGRESS (no need to use >>> TC_H_MAKE, we can detect it from ctx->parent that it won't be a parent id). This >>> would mean that by default attach point is what you set for ctx, but for >>> bpf_tc_attach you can temporarily override to be some other attach point (for >>> the same qdisc). You still won't be able to set anything other than the two >>> though. >> >> I think the assumption on auto-detecting the parent id in that case might not hold given >> major number could very well be 0. Wrt BPF_TC_UNSPEC ... maybe it's not even needed, back >> to drawing board ... >> >> Here's how the whole API could look like, usage examples below: >> >> enum bpf_tc_attach_point { >> BPF_TC_INGRESS = 1 << 0, >> BPF_TC_EGRESS = 1 << 1, >> BPF_TC_CUSTOM = 1 << 2, >> }; >> >> enum bpf_tc_attach_flags { >> BPF_TC_F_REPLACE = 1 << 0, >> }; >> >> struct bpf_tc_hook { >> size_t sz; >> int ifindex; >> enum bpf_tc_attach_point which; >> __u32 parent; >> size_t :0; >> }; >> >> struct bpf_tc_opts { >> size_t sz; >> __u32 handle; >> __u16 priority; >> union { >> int prog_fd; >> __u32 prog_id; >> }; >> size_t :0; >> }; >> >> LIBBPF_API int bpf_tc_hook_create(struct bpf_tc_hook *hook); >> LIBBPF_API int bpf_tc_hook_destroy(struct bpf_tc_hook *hook); >> >> LIBBPF_API int bpf_tc_attach(const struct bpf_tc_hook *hook, const struct bpf_tc_opts *opts, int flags); >> LIBBPF_API int bpf_tc_detach(const struct bpf_tc_hook *hook, const struct bpf_tc_opts *opts); >> LIBBPF_API int bpf_tc_query(const struct bpf_tc_hook *hook, struct bpf_tc_opts *opts); >> >> So a user could do just: >> >> DECLARE_LIBBPF_OPTS(bpf_tc_hook, hook, .ifindex = 42, .which = BPF_TC_INGRESS); >> DECLARE_LIBBPF_OPTS(bpf_tc_opts, opts, .handle = 1, .priority = 1, .prog_fd = fd); >> >> err = bpf_tc_attach(&hook, &opts, BPF_TC_F_REPLACE); >> [...] >> >> If it's not known whether the hook exists, then a preceding call to: >> >> err = bpf_tc_hook_create(&hook); >> [...] >> >> The bpf_tc_query() would look like: >> >> DECLARE_LIBBPF_OPTS(bpf_tc_hook, hook, .ifindex = 42, .which = BPF_TC_EGRESS); >> DECLARE_LIBBPF_OPTS(bpf_tc_opts, opts, .handle = 1, .priority = 1); >> >> err = bpf_tc_query(&hook, &opts); >> if (!err) { >> [...] // gives access to: opts.prog_id >> } >> >> The bpf_tc_detach(): >> >> DECLARE_LIBBPF_OPTS(bpf_tc_hook, hook, .ifindex = 42, .which = BPF_TC_INGRESS); >> DECLARE_LIBBPF_OPTS(bpf_tc_opts, opts, .handle = 1, .priority = 1); >> >> err = bpf_tc_detach(&hook, &opts); >> [...] >> >> The nice thing would be that hook and opts are kept semantically separate, meaning with >> hook you can iterate though a bunch of devs and ingress/egress locations without changing >> opts, whereas with opts you could iterate on the cls_bpf instance itself w/o changing >> hook. Both are kept extensible via DECLARE_LIBBPF_OPTS(). >> >> Now the bpf_tc_hook_destroy() one: >> >> DECLARE_LIBBPF_OPTS(bpf_tc_hook, hook, .ifindex = 42, .which = BPF_TC_INGRESS|BPF_TC_EGRESS); >> >> err = bpf_tc_hook_destroy(&hook); >> [...] >> >> For triggering a remove of the clsact qdisc on the device, both directions are passed in. >> Combining both is only ever allowed for bpf_tc_hook_destroy(). > > Small addendum: > > DECLARE_LIBBPF_OPTS(bpf_tc_hook, hook, .ifindex = 42, .which = BPF_TC_INGRESS|BPF_TC_EGRESS); > > err = bpf_tc_hook_create(&hook); > [...] > > ... is also possible, of course, and then both bpf_tc_hook_{create,destroy}() are symmetric. It should be allowed, but it wouldn't actually make any difference which combination of TC_INGRESS and TC_EGRESS you specify, as long as one of them is set, right? I.e., we just attach the clsact qdisc in both cases... -Toke
On 4/28/21 12:36 AM, Toke Høiland-Jørgensen wrote: > Daniel Borkmann <daniel@iogearbox.net> writes: [...] >> Small addendum: >> >> DECLARE_LIBBPF_OPTS(bpf_tc_hook, hook, .ifindex = 42, .which = BPF_TC_INGRESS|BPF_TC_EGRESS); >> >> err = bpf_tc_hook_create(&hook); >> [...] >> >> ... is also possible, of course, and then both bpf_tc_hook_{create,destroy}() are symmetric. > > It should be allowed, but it wouldn't actually make any difference which > combination of TC_INGRESS and TC_EGRESS you specify, as long as one of > them is set, right? I.e., we just attach the clsact qdisc in both > cases... Yes, that is correct, for the bpf_tc_hook_create() whether you pass in BPF_TC_INGRESS, BPF_TC_EGRESS or BPF_TC_INGRESS|BPF_TC_EGRESS, you'll end up creating clsact qdisc in either of the three cases. Only the bpf_tc_hook_destroy() differs between all of them. Thanks, Daniel
Daniel Borkmann <daniel@iogearbox.net> writes: > On 4/28/21 12:36 AM, Toke Høiland-Jørgensen wrote: >> Daniel Borkmann <daniel@iogearbox.net> writes: > [...] >>> Small addendum: >>> >>> DECLARE_LIBBPF_OPTS(bpf_tc_hook, hook, .ifindex = 42, .which = BPF_TC_INGRESS|BPF_TC_EGRESS); >>> >>> err = bpf_tc_hook_create(&hook); >>> [...] >>> >>> ... is also possible, of course, and then both bpf_tc_hook_{create,destroy}() are symmetric. >> >> It should be allowed, but it wouldn't actually make any difference which >> combination of TC_INGRESS and TC_EGRESS you specify, as long as one of >> them is set, right? I.e., we just attach the clsact qdisc in both >> cases... > > Yes, that is correct, for the bpf_tc_hook_create() whether you pass in BPF_TC_INGRESS, > BPF_TC_EGRESS or BPF_TC_INGRESS|BPF_TC_EGRESS, you'll end up creating clsact qdisc in > either of the three cases. Only the bpf_tc_hook_destroy() differs > between all of them. Right, just checking. Other than that, I like your proposal; it loses the "automatic removal of qdisc if we added it" feature, but that's probably OK: less magic is good. And as long as bpf_tc_hook_create() returns EEXIST if the qdisc already exists, the caller can do the same thing if they want. -Toke
On 4/28/21 12:51 AM, Toke Høiland-Jørgensen wrote: > Daniel Borkmann <daniel@iogearbox.net> writes: >> On 4/28/21 12:36 AM, Toke Høiland-Jørgensen wrote: >>> Daniel Borkmann <daniel@iogearbox.net> writes: >> [...] >>>> Small addendum: >>>> >>>> DECLARE_LIBBPF_OPTS(bpf_tc_hook, hook, .ifindex = 42, .which = BPF_TC_INGRESS|BPF_TC_EGRESS); >>>> >>>> err = bpf_tc_hook_create(&hook); >>>> [...] >>>> >>>> ... is also possible, of course, and then both bpf_tc_hook_{create,destroy}() are symmetric. >>> >>> It should be allowed, but it wouldn't actually make any difference which >>> combination of TC_INGRESS and TC_EGRESS you specify, as long as one of >>> them is set, right? I.e., we just attach the clsact qdisc in both >>> cases... >> >> Yes, that is correct, for the bpf_tc_hook_create() whether you pass in BPF_TC_INGRESS, >> BPF_TC_EGRESS or BPF_TC_INGRESS|BPF_TC_EGRESS, you'll end up creating clsact qdisc in >> either of the three cases. Only the bpf_tc_hook_destroy() differs >> between all of them. > > Right, just checking. Other than that, I like your proposal; it loses > the "automatic removal of qdisc if we added it" feature, but that's > probably OK: less magic is good. And as long as bpf_tc_hook_create() > returns EEXIST if the qdisc already exists, the caller can do the same > thing if they want. Yes exactly. Less magic the better, especially given this has global effect. Thanks, Daniel
On Wed, Apr 28, 2021 at 04:44:23AM IST, Daniel Borkmann wrote: > On 4/28/21 12:51 AM, Toke Høiland-Jørgensen wrote: > > Daniel Borkmann <daniel@iogearbox.net> writes: > > > On 4/28/21 12:36 AM, Toke Høiland-Jørgensen wrote: > > > > Daniel Borkmann <daniel@iogearbox.net> writes: > > > [...] > > > > > Small addendum: > > > > > > > > > > DECLARE_LIBBPF_OPTS(bpf_tc_hook, hook, .ifindex = 42, .which = BPF_TC_INGRESS|BPF_TC_EGRESS); > > > > > > > > > > err = bpf_tc_hook_create(&hook); > > > > > [...] > > > > > > > > > > ... is also possible, of course, and then both bpf_tc_hook_{create,destroy}() are symmetric. > > > > > > > > It should be allowed, but it wouldn't actually make any difference which > > > > combination of TC_INGRESS and TC_EGRESS you specify, as long as one of > > > > them is set, right? I.e., we just attach the clsact qdisc in both > > > > cases... > > > > > > Yes, that is correct, for the bpf_tc_hook_create() whether you pass in BPF_TC_INGRESS, > > > BPF_TC_EGRESS or BPF_TC_INGRESS|BPF_TC_EGRESS, you'll end up creating clsact qdisc in > > > either of the three cases. Only the bpf_tc_hook_destroy() differs > > > between all of them. > > > > Right, just checking. Other than that, I like your proposal; it loses > > the "automatic removal of qdisc if we added it" feature, but that's > > probably OK: less magic is good. And as long as bpf_tc_hook_create() > > returns EEXIST if the qdisc already exists, the caller can do the same > > thing if they want. > > Yes exactly. Less magic the better, especially given this has global effect. > Everything sounds good. I'll do a resend. > Thanks, > Daniel -- Kartikeya
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, +}; + +/* 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. + */ +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. + */ +LIBBPF_API struct bpf_tc_ctx *bpf_tc_ctx_init(__u32 ifindex, + enum bpf_tc_attach_point parent, + struct bpf_tc_ctx_opts *opts); +/* + * @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); +/* + * @ctx: Cannot be NULL. + * @opts: Cannot be NULL, replace and prog_id must be unset, all other fields + * must be set. + * + * Fills the following fields in opts: + * handle + * parent + * priority + * prog_id + */ +LIBBPF_API int bpf_tc_query(struct bpf_tc_ctx *ctx, + struct bpf_tc_opts *opts); + #ifdef __cplusplus } /* extern "C" */ #endif diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map index b9b29baf1df8..f6490d521601 100644 --- a/tools/lib/bpf/libbpf.map +++ b/tools/lib/bpf/libbpf.map @@ -361,4 +361,9 @@ LIBBPF_0.4.0 { bpf_linker__new; bpf_map__inner_map; bpf_object__set_kversion; + bpf_tc_ctx_init; + bpf_tc_ctx_destroy; + bpf_tc_attach; + bpf_tc_detach; + bpf_tc_query; } LIBBPF_0.3.0; diff --git a/tools/lib/bpf/netlink.c b/tools/lib/bpf/netlink.c index c79e30484e81..a389e1151391 100644 --- a/tools/lib/bpf/netlink.c +++ b/tools/lib/bpf/netlink.c @@ -4,7 +4,11 @@ #include <stdlib.h> #include <memory.h> #include <unistd.h> +#include <inttypes.h> +#include <arpa/inet.h> #include <linux/bpf.h> +#include <linux/if_ether.h> +#include <linux/pkt_cls.h> #include <linux/rtnetlink.h> #include <sys/socket.h> #include <errno.h> @@ -73,6 +77,11 @@ static int libbpf_netlink_open(__u32 *nl_pid) return ret; } +enum { + BPF_NL_CONT, + BPF_NL_NEXT, +}; + static int bpf_netlink_recv(int sock, __u32 nl_pid, int seq, __dump_nlmsg_t _fn, libbpf_dump_nlmsg_t fn, void *cookie) @@ -84,6 +93,7 @@ static int bpf_netlink_recv(int sock, __u32 nl_pid, int seq, int len, ret; while (multipart) { +start: multipart = false; len = recv(sock, buf, sizeof(buf), 0); if (len < 0) { @@ -121,8 +131,16 @@ static int bpf_netlink_recv(int sock, __u32 nl_pid, int seq, } if (_fn) { ret = _fn(nh, fn, cookie); - if (ret) + if (ret < 0) + return ret; + switch (ret) { + case BPF_NL_CONT: + break; + case BPF_NL_NEXT: + goto start; + default: return ret; + } } } } @@ -131,6 +149,21 @@ static int bpf_netlink_recv(int sock, __u32 nl_pid, int seq, return ret; } +/* In TC-BPF we use seqnum to form causal order of operations on shared ctx + * socket, so we want to skip messages older than the one we are looking for, + * in case they are left in socket buffer for some reason (e.g. errors). */ +static int bpf_netlink_recv_skip(int sock, __u32 nl_pid, int seq, __dump_nlmsg_t fn, + void *cookie) +{ + int ret; + +restart: + ret = bpf_netlink_recv(sock, nl_pid, seq, fn, NULL, cookie); + if (ret < 0 && ret == -LIBBPF_ERRNO__INVSEQ) + goto restart; + return ret; +} + static int __bpf_set_link_xdp_fd_replace(int ifindex, int fd, int old_fd, __u32 flags) { @@ -365,3 +398,446 @@ int libbpf_nl_get_link(int sock, unsigned int nl_pid, return bpf_netlink_recv(sock, nl_pid, seq, __dump_link_nlmsg, dump_link_nlmsg, cookie); } + +/* TC-CTX */ + +struct bpf_tc_ctx { + __u32 ifindex; + enum bpf_tc_attach_point parent; + int sock; + __u32 nl_pid; + __u32 seq; + bool created_qdisc; +}; + +typedef int (*qdisc_config_t)(struct nlmsghdr *nh, struct tcmsg *t, + size_t maxsz); + +static int clsact_config(struct nlmsghdr *nh, struct tcmsg *t, size_t maxsz) +{ + int ret; + + t->tcm_parent = TC_H_CLSACT; + t->tcm_handle = TC_H_MAKE(TC_H_CLSACT, 0); + + ret = nlattr_add(nh, maxsz, TCA_KIND, "clsact", sizeof("clsact")); + if (ret < 0) + return ret; + + return 0; +} + +static const qdisc_config_t parent_to_qdisc[_BPF_TC_PARENT_MAX] = { + [BPF_TC_INGRESS] = &clsact_config, + [BPF_TC_EGRESS] = &clsact_config, + [BPF_TC_CUSTOM_PARENT] = NULL, +}; + +static int tc_qdisc_modify(struct bpf_tc_ctx *ctx, int cmd, int flags, + qdisc_config_t config) +{ + int ret = 0; + struct { + struct nlmsghdr nh; + struct tcmsg t; + char buf[256]; + } req; + + if (!ctx || !config) + return -EINVAL; + + memset(&req, 0, sizeof(req)); + req.nh.nlmsg_len = NLMSG_LENGTH(sizeof(struct tcmsg)); + req.nh.nlmsg_flags = + NLM_F_REQUEST | NLM_F_ACK | flags; + req.nh.nlmsg_type = cmd; + req.nh.nlmsg_pid = 0; + req.nh.nlmsg_seq = ++ctx->seq; + req.t.tcm_family = AF_UNSPEC; + req.t.tcm_ifindex = ctx->ifindex; + + ret = config(&req.nh, &req.t, sizeof(req)); + if (ret < 0) + return ret; + + ret = send(ctx->sock, &req.nh, req.nh.nlmsg_len, 0); + if (ret < 0) + return ret; + + return bpf_netlink_recv_skip(ctx->sock, ctx->nl_pid, ctx->seq, NULL, NULL); +} + +static int tc_qdisc_create_excl(struct bpf_tc_ctx *ctx, qdisc_config_t config) +{ + return tc_qdisc_modify(ctx, RTM_NEWQDISC, NLM_F_CREATE | NLM_F_EXCL, config); +} + +static int tc_qdisc_delete(struct bpf_tc_ctx *ctx, qdisc_config_t config) +{ + return tc_qdisc_modify(ctx, RTM_DELQDISC, 0, config); +} + +struct bpf_tc_ctx *bpf_tc_ctx_init(__u32 ifindex, enum bpf_tc_attach_point parent, + struct bpf_tc_ctx_opts *opts) +{ + struct bpf_tc_ctx *ctx = NULL; + qdisc_config_t config; + int ret, sock; + __u32 nl_pid; + + if (!ifindex || parent >= _BPF_TC_PARENT_MAX || + !OPTS_VALID(opts, bpf_tc_ctx_opts)) { + errno = EINVAL; + return NULL; + } + + sock = libbpf_netlink_open(&nl_pid); + if (sock < 0) { + errno = -sock; + return NULL; + } + + ctx = calloc(1, sizeof(*ctx)); + if (!ctx) { + errno = ENOMEM; + goto end_sock; + } + + ctx->ifindex = ifindex; + ctx->parent = parent; + ctx->seq = time(NULL); + ctx->nl_pid = nl_pid; + ctx->sock = sock; + + config = parent_to_qdisc[parent]; + if (config) { + ret = tc_qdisc_create_excl(ctx, config); + if (ret < 0 && ret != -EEXIST) { + errno = -ret; + goto end_ctx; + } + ctx->created_qdisc = ret == 0; + } + + return ctx; + +end_ctx: + free(ctx); +end_sock: + close(sock); + return NULL; +} + +struct pass_info { + struct bpf_tc_opts *opts; + bool processed; +}; + +static int __tc_query(struct bpf_tc_ctx *ctx, + struct bpf_tc_opts *opts); + +int bpf_tc_ctx_destroy(struct bpf_tc_ctx *ctx) +{ + qdisc_config_t config; + int ret = 0; + + if (!ctx) + return 0; + + config = parent_to_qdisc[ctx->parent]; + if (ctx->created_qdisc && config) { + /* ctx->parent cannot be BPF_TC_CUSTOM_PARENT, as this doesn't + * map to a qdisc that can be created, so opts being NULL won't + * be an error (e.g. in tc_ctx_get_tcm_parent). + */ + if (__tc_query(ctx, NULL) == -ENOENT) + ret = tc_qdisc_delete(ctx, config); + } + + close(ctx->sock); + free(ctx); + return ret; +} + +static long long int tc_ctx_get_tcm_parent(enum bpf_tc_attach_point type, + __u32 parent) +{ + long long int ret; + + switch (type) { + case BPF_TC_INGRESS: + ret = TC_H_MAKE(TC_H_CLSACT, TC_H_MIN_INGRESS); + if (parent && parent != ret) + return -EINVAL; + break; + case BPF_TC_EGRESS: + ret = TC_H_MAKE(TC_H_CLSACT, TC_H_MIN_EGRESS); + if (parent && parent != ret) + return -EINVAL; + break; + case BPF_TC_CUSTOM_PARENT: + if (!parent) + return -EINVAL; + ret = parent; + break; + default: + return -ERANGE; + } + + return ret; +} + +/* TC-BPF */ + +static int tc_bpf_add_fd_and_name(struct nlmsghdr *nh, size_t maxsz, int fd) +{ + struct bpf_prog_info info = {}; + __u32 info_len = sizeof(info); + char name[256] = {}; + int len, ret; + + ret = bpf_obj_get_info_by_fd(fd, &info, &info_len); + if (ret < 0) + return ret; + + ret = nlattr_add(nh, maxsz, TCA_BPF_FD, &fd, sizeof(fd)); + if (ret < 0) + return ret; + + len = snprintf(name, sizeof(name), "%s:[%" PRIu32 "]", info.name, + info.id); + if (len < 0 || len >= sizeof(name)) + return len < 0 ? -EINVAL : -ENAMETOOLONG; + + return nlattr_add(nh, maxsz, TCA_BPF_NAME, name, len + 1); +} + +static int tc_cls_bpf_modify(struct bpf_tc_ctx *ctx, int fd, int cmd, int flags, + struct bpf_tc_opts *opts, __dump_nlmsg_t fn) +{ + unsigned int bpf_flags = 0; + struct pass_info info = {}; + __u32 protocol, priority; + long long int tcm_parent; + struct nlattr *nla; + int ret; + struct { + struct nlmsghdr nh; + struct tcmsg t; + char buf[256]; + } req; + + if (cmd == RTM_NEWTFILTER) + flags |= OPTS_GET(opts, replace, false) ? NLM_F_REPLACE : + NLM_F_EXCL; + priority = OPTS_GET(opts, priority, 0); + protocol = ETH_P_ALL; + + memset(&req, 0, sizeof(req)); + req.nh.nlmsg_len = NLMSG_LENGTH(sizeof(struct tcmsg)); + req.nh.nlmsg_flags = NLM_F_REQUEST | NLM_F_ACK | flags; + req.nh.nlmsg_type = cmd; + req.nh.nlmsg_pid = 0; + req.nh.nlmsg_seq = ++ctx->seq; + req.t.tcm_family = AF_UNSPEC; + req.t.tcm_handle = OPTS_GET(opts, handle, 0); + req.t.tcm_ifindex = ctx->ifindex; + req.t.tcm_info = TC_H_MAKE(priority << 16, htons(protocol)); + + tcm_parent = tc_ctx_get_tcm_parent(ctx->parent, OPTS_GET(opts, parent, 0)); + if (tcm_parent < 0) + return tcm_parent; + req.t.tcm_parent = tcm_parent; + + ret = nlattr_add(&req.nh, sizeof(req), TCA_KIND, "bpf", sizeof("bpf")); + if (ret < 0) + return ret; + + nla = nlattr_begin_nested(&req.nh, sizeof(req), TCA_OPTIONS); + if (!nla) + return -EMSGSIZE; + + if (cmd != RTM_DELTFILTER) { + ret = tc_bpf_add_fd_and_name(&req.nh, sizeof(req), fd); + if (ret < 0) + return ret; + + /* direct action mode is always enabled */ + bpf_flags |= TCA_BPF_FLAG_ACT_DIRECT; + ret = nlattr_add(&req.nh, sizeof(req), TCA_BPF_FLAGS, + &bpf_flags, sizeof(bpf_flags)); + if (ret < 0) + return ret; + } + + nlattr_end_nested(&req.nh, nla); + + ret = send(ctx->sock, &req.nh, req.nh.nlmsg_len, 0); + if (ret < 0) + return ret; + + info.opts = opts; + ret = bpf_netlink_recv_skip(ctx->sock, ctx->nl_pid, ctx->seq, fn, + &info); + if (ret < 0) + return ret; + + /* Failed to process unicast response */ + if (fn && !info.processed) + ret = -ENOENT; + + return ret; +} + +static int cls_get_info(struct nlmsghdr *nh, libbpf_dump_nlmsg_t fn, + void *cookie); + +int bpf_tc_attach(struct bpf_tc_ctx *ctx, int fd, + struct bpf_tc_opts *opts) +{ + if (!ctx || fd < 0 || !opts) + return -EINVAL; + + if (!OPTS_VALID(opts, bpf_tc_opts) || OPTS_GET(opts, prog_id, 0)) + return -EINVAL; + + if (OPTS_GET(opts, parent, 0) && ctx->parent < BPF_TC_CUSTOM_PARENT) + return -EINVAL; + + return tc_cls_bpf_modify(ctx, fd, RTM_NEWTFILTER, + NLM_F_ECHO | NLM_F_CREATE, + opts, cls_get_info); +} + +int bpf_tc_detach(struct bpf_tc_ctx *ctx, + const struct bpf_tc_opts *opts) +{ + if (!ctx || !opts) + return -EINVAL; + + if (!OPTS_VALID(opts, bpf_tc_opts) || !OPTS_GET(opts, handle, 0) || + !OPTS_GET(opts, priority, 0) || !OPTS_GET(opts, parent, 0) || + OPTS_GET(opts, replace, false) || OPTS_GET(opts, prog_id, 0)) + return -EINVAL; + + /* Won't write to opts when fn is NULL */ + return tc_cls_bpf_modify(ctx, 0, RTM_DELTFILTER, 0, + (struct bpf_tc_opts *)opts, NULL); +} + +static int __cls_get_info(void *cookie, void *msg, struct nlattr **tb, + bool unicast) +{ + struct nlattr *tbb[TCA_BPF_MAX + 1]; + struct pass_info *info = cookie; + struct tcmsg *t = msg; + + if (!info) + return -EINVAL; + if (unicast && info->processed) + return -EINVAL; + /* We use BPF_NL_CONT even after finding the filter to consume all + * remaining multipart messages. + */ + if (info->processed || !tb[TCA_OPTIONS]) + return BPF_NL_CONT; + + libbpf_nla_parse_nested(tbb, TCA_BPF_MAX, tb[TCA_OPTIONS], NULL); + if (!tbb[TCA_BPF_ID]) + return BPF_NL_CONT; + + OPTS_SET(info->opts, handle, t->tcm_handle); + OPTS_SET(info->opts, parent, t->tcm_parent); + OPTS_SET(info->opts, priority, TC_H_MAJ(t->tcm_info) >> 16); + OPTS_SET(info->opts, prog_id, libbpf_nla_getattr_u32(tbb[TCA_BPF_ID])); + + info->processed = true; + return unicast ? BPF_NL_NEXT : BPF_NL_CONT; +} + +static int cls_get_info(struct nlmsghdr *nh, libbpf_dump_nlmsg_t fn, + void *cookie) +{ + struct tcmsg *t = NLMSG_DATA(nh); + struct nlattr *tb[TCA_MAX + 1]; + + libbpf_nla_parse(tb, TCA_MAX, + (struct nlattr *)((char *)t + NLMSG_ALIGN(sizeof(*t))), + NLMSG_PAYLOAD(nh, sizeof(*t)), NULL); + if (!tb[TCA_KIND]) + return BPF_NL_CONT; + + return __cls_get_info(cookie, t, tb, nh->nlmsg_flags & NLM_F_ECHO); +} + +/* This is the less strict internal helper used to get dump for more than one + * filter, used to determine if there are any filters attach for a bpf_tc_ctx. + * + */ +static int __tc_query(struct bpf_tc_ctx *ctx, + struct bpf_tc_opts *opts) +{ + struct pass_info pinfo = {}; + __u32 priority, protocol; + long long int tcm_parent; + int ret; + struct { + struct nlmsghdr nh; + struct tcmsg t; + char buf[256]; + } req = { + .nh.nlmsg_len = NLMSG_LENGTH(sizeof(struct tcmsg)), + .nh.nlmsg_type = RTM_GETTFILTER, + .nh.nlmsg_flags = NLM_F_REQUEST | NLM_F_DUMP, + .t.tcm_family = AF_UNSPEC, + }; + + if (!ctx) + return -EINVAL; + + priority = OPTS_GET(opts, priority, 0); + protocol = ETH_P_ALL; + + req.nh.nlmsg_seq = ++ctx->seq; + req.t.tcm_ifindex = ctx->ifindex; + req.t.tcm_handle = OPTS_GET(opts, handle, 0); + req.t.tcm_info = TC_H_MAKE(priority << 16, htons(protocol)); + + tcm_parent = tc_ctx_get_tcm_parent(ctx->parent, OPTS_GET(opts, parent, 0)); + if (tcm_parent < 0) + return tcm_parent; + req.t.tcm_parent = tcm_parent; + + ret = nlattr_add(&req.nh, sizeof(req), TCA_KIND, "bpf", sizeof("bpf")); + if (ret < 0) + return ret; + + ret = send(ctx->sock, &req.nh, req.nh.nlmsg_len, 0); + if (ret < 0) + return ret; + + pinfo.opts = opts; + ret = bpf_netlink_recv_skip(ctx->sock, ctx->nl_pid, ctx->seq, + cls_get_info, &pinfo); + if (ret < 0) + return ret; + + if (!pinfo.processed) + ret = -ENOENT; + + return ret; +} + +int bpf_tc_query(struct bpf_tc_ctx *ctx, + struct bpf_tc_opts *opts) +{ + if (!ctx || !opts) + return -EINVAL; + + if (!OPTS_VALID(opts, bpf_tc_opts) || !OPTS_GET(opts, handle, 0) || + !OPTS_GET(opts, priority, 0) || !OPTS_GET(opts, parent, 0) || + OPTS_GET(opts, replace, false) || OPTS_GET(opts, prog_id, 0)) + return -EINVAL; + + return __tc_query(ctx, opts); +}
This adds functions that wrap the netlink API used for adding, manipulating, and removing traffic control filters. An API summary: bpf_tc_ctx_init may be used to create a context structure represented by the opaque struct bpf_tc_ctx pointer returned to the caller. A bpf context represents the attach point for a filter, which means it takes the ifindex of the device user is attaching to, and an attach point that translates to the parent of the filter. When specifying BPF_TC_INGRESS or BPF_TC_EGRESS, the ctx initialization will automatically setup the clsact qdisc if possible, and also clean it up if it was created during ctx_init and there are no more remaining filters. A custom parent value can be set in opts during filter attach and indicated by setting BPF_TC_CUSTOM_PARENT during ctx initialization. bpf_tc_ctx_destroy is used to release a bpf_tc_ctx object and remove any qdiscs owned by it. Note that there is a small race between the checking of existing filters and removal of qdisc. The chances of this happening are quite slim however, as we only remove a qdisc we set up. bpf_tc_attach may be used to attach, and replace a filter and bind a SCHED_CLS prog as its bpf classifier. Direct action mode is always enabled, and protocol is always set to ETH_P_ALL. The filter is attached to the main chain with index 0. bpf_tc_detach may be used to delete existing filter and detach the SCHED_CLS filter. Example (using bpf skeleton infrastructure): BPF program (test_tc_bpf.c): #include <linux/bpf.h> #include <bpf/bpf_helpers.h> SEC("classifier") int cls(struct __sk_buff *skb) { return 0; } Userspace loader: DECLARE_LIBBPF_OPTS(bpf_tc_opts, opts, 0); struct test_tc_bpf *skel; struct bpf_tc_ctx *ctx; int fd, r; skel = test_tc_bpf__open_and_load(); if (!skel) return -ENOMEM; fd = bpf_program__fd(skel->progs.cls); ctx = bpf_tc_ctx_init(if_nametoindex("lo"), BPF_TC_INGRESS, NULL); if (!ctx) return -ENOMEM; r = bpf_tc_attach(ctx, fd, &opts); if (r < 0) return r; ... which is roughly equivalent to: # tc qdisc add dev lo clsact # tc filter add dev lo ingress bpf obj foo.o sec classifier da To replace an existing filter (e.g. the one we just created): /* opts.{handle, parent, priority, prog_id} was filled * in by attach. */ opts.replace = true; /* clear fields that must be unset, we must also clear parent as our attach point is ingress hook */ opts.parent = opts.prog_id = 0; r = bpf_tc_attach(ctx, fd, &opts); if (r < 0) return r; To obtain info of a particular filter: /* Find info for filter with handle 1 and priority 50 */ DECLARE_LIBBPF_OPTS(bpf_tc_opts, info_opts, .handle = 1, .priority = 50, .parent = opts.parent) r = bpf_tc_query(ctx, &info_opts); if (r < 0 && r == -ENOENT) printf("Filter not found\n"); else return r; Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> --- 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(-) -- 2.30.2