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
