On Wed, Jun 7, 2023 at 12:26 PM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
>
> Extend libbpf attach opts and add a new detach opts API so this can be used
> to add/remove fd-based tcx BPF programs. The old-style bpf_prog_detach and
> bpf_prog_detach2 APIs are refactored to reuse the detach opts internally.
>
> The bpf_prog_query_opts API got extended to be able to handle the new link_ids,
> link_attach_flags and revision fields.
>
> For concrete usage examples, see the extensive selftests that have been
> developed as part of this series.
>
> Signed-off-by: Daniel Borkmann <daniel@xxxxxxxxxxxxx>
> ---
> tools/lib/bpf/bpf.c | 78 ++++++++++++++++++++++------------------
> tools/lib/bpf/bpf.h | 54 +++++++++++++++++++++-------
> tools/lib/bpf/libbpf.c | 6 ++++
> tools/lib/bpf/libbpf.map | 1 +
> 4 files changed, 91 insertions(+), 48 deletions(-)
>
> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index ed86b37d8024..a3d1b7ebe224 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -629,11 +629,21 @@ int bpf_prog_attach(int prog_fd, int target_fd, enum bpf_attach_type type,
> return bpf_prog_attach_opts(prog_fd, target_fd, type, &opts);
> }
>
> -int bpf_prog_attach_opts(int prog_fd, int target_fd,
> - enum bpf_attach_type type,
> - const struct bpf_prog_attach_opts *opts)
> +int bpf_prog_detach(int target_fd, enum bpf_attach_type type)
> +{
> + return bpf_prog_detach_opts(0, target_fd, type, NULL);
> +}
> +
> +int bpf_prog_detach2(int prog_fd, int target_fd, enum bpf_attach_type type)
> {
> - const size_t attr_sz = offsetofend(union bpf_attr, replace_bpf_fd);
> + return bpf_prog_detach_opts(prog_fd, target_fd, type, NULL);
> +}
Please put these wrappers after bpf_prog_detach_ops(), it will make
the diff cleaner and will keep them closer to full version of
bpf_prog_detach_opts().
> +
> +int bpf_prog_attach_opts(int prog_fd, int target,
> + enum bpf_attach_type type,
> + const struct bpf_prog_attach_opts *opts)
> +{
> + const size_t attr_sz = offsetofend(union bpf_attr, expected_revision);
> union bpf_attr attr;
> int ret;
>
[...]
> diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> index 9aa0ee473754..480c584a6f7f 100644
> --- a/tools/lib/bpf/bpf.h
> +++ b/tools/lib/bpf/bpf.h
> @@ -312,22 +312,43 @@ LIBBPF_API int bpf_obj_get(const char *pathname);
> LIBBPF_API int bpf_obj_get_opts(const char *pathname,
> const struct bpf_obj_get_opts *opts);
>
> -struct bpf_prog_attach_opts {
> - size_t sz; /* size of this struct for forward/backward compatibility */
> - unsigned int flags;
> - int replace_prog_fd;
> -};
> -#define bpf_prog_attach_opts__last_field replace_prog_fd
> -
> LIBBPF_API int bpf_prog_attach(int prog_fd, int attachable_fd,
> enum bpf_attach_type type, unsigned int flags);
> -LIBBPF_API int bpf_prog_attach_opts(int prog_fd, int attachable_fd,
> - enum bpf_attach_type type,
> - const struct bpf_prog_attach_opts *opts);
> LIBBPF_API int bpf_prog_detach(int attachable_fd, enum bpf_attach_type type);
> LIBBPF_API int bpf_prog_detach2(int prog_fd, int attachable_fd,
> enum bpf_attach_type type);
>
> +struct bpf_prog_attach_opts {
> + size_t sz; /* size of this struct for forward/backward compatibility */
> + __u32 flags;
> + union {
> + int replace_prog_fd;
> + int replace_fd;
> + int relative_fd;
> + __u32 relative_id;
> + };
I tried to not use union for such cases in OPTS-based interfaces, see
bpf_link_create(). Let's keep them all as separate fields and then
return error if, say, both relative_fd and relative_id is specified at
the same time.
It's fine to have replace_prog_fd and replace_fd as a union, as they
are basically just synonyms.
> + __u32 expected_revision;
> +};
> +#define bpf_prog_attach_opts__last_field expected_revision
> +
> +struct bpf_prog_detach_opts {
> + size_t sz; /* size of this struct for forward/backward compatibility */
> + __u32 flags;
> + union {
> + int relative_fd;
> + __u32 relative_id;
> + };
same as above
> + __u32 expected_revision;
> +};
> +#define bpf_prog_detach_opts__last_field expected_revision
> +
> +LIBBPF_API int bpf_prog_attach_opts(int prog_fd, int target,
let's add doc comments to both these APIs, where `target` is
explained. Right now because it doesn't have "_fd" suffix it's not
very clear what sort of value it is (I know why it's not target_fd
anymore due to target_ifindex)
> + enum bpf_attach_type type,
> + const struct bpf_prog_attach_opts *opts);
> +LIBBPF_API int bpf_prog_detach_opts(int prog_fd, int target,
> + enum bpf_attach_type type,
> + const struct bpf_prog_detach_opts *opts);
> +
> union bpf_iter_link_info; /* defined in up-to-date linux/bpf.h */
> struct bpf_link_create_opts {
> size_t sz; /* size of this struct for forward/backward compatibility */
> @@ -489,14 +510,21 @@ struct bpf_prog_query_opts {
> __u32 query_flags;
> __u32 attach_flags; /* output argument */
> __u32 *prog_ids;
> - __u32 prog_cnt; /* input+output argument */
> + union {
> + __u32 prog_cnt; /* input+output argument */
> + __u32 count;
> + };
> __u32 *prog_attach_flags;
> + __u32 *link_ids;
> + __u32 *link_attach_flags;
> + __u32 revision;
> };
> -#define bpf_prog_query_opts__last_field prog_attach_flags
> +#define bpf_prog_query_opts__last_field revision
>
> -LIBBPF_API int bpf_prog_query_opts(int target_fd,
> +LIBBPF_API int bpf_prog_query_opts(int target,
same here for doc comment
> enum bpf_attach_type type,
> struct bpf_prog_query_opts *opts);
> +
> LIBBPF_API int bpf_prog_query(int target_fd, enum bpf_attach_type type,
> __u32 query_flags, __u32 *attach_flags,
> __u32 *prog_ids, __u32 *prog_cnt);
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 47632606b06d..b89127471c6a 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -117,6 +117,8 @@ static const char * const attach_type_name[] = {
> [BPF_PERF_EVENT] = "perf_event",
> [BPF_TRACE_KPROBE_MULTI] = "trace_kprobe_multi",
> [BPF_STRUCT_OPS] = "struct_ops",
> + [BPF_TCX_INGRESS] = "tcx_ingress",
> + [BPF_TCX_EGRESS] = "tcx_egress",
> };
>
> static const char * const link_type_name[] = {
> @@ -8669,6 +8671,10 @@ static const struct bpf_sec_def section_defs[] = {
> SEC_DEF("kretsyscall+", KPROBE, 0, SEC_NONE, attach_ksyscall),
> SEC_DEF("usdt+", KPROBE, 0, SEC_NONE, attach_usdt),
> SEC_DEF("tc", SCHED_CLS, 0, SEC_NONE),
> + SEC_DEF("tc/ingress", SCHED_CLS, BPF_TCX_INGRESS, SEC_ATTACHABLE_OPT),
> + SEC_DEF("tc/egress", SCHED_CLS, BPF_TCX_EGRESS, SEC_ATTACHABLE_OPT),
for tc/ingress and tc/egress, is it intentional that libbpf should set
expected_attach_type to zero if kernel doesn't support BPF_TCX_INGRESS
or BPF_TCX_EGRESS? Or is it just an alias to tcx/ingress and
tcx/egress?
If it's an alias, why do we need it?
If not, let's replace SEC_ATTACHABLE_OPT with just SEC_EXP_ATTACH_OPT ?
> + SEC_DEF("tcx/ingress", SCHED_CLS, BPF_TCX_INGRESS, SEC_ATTACHABLE_OPT),
> + SEC_DEF("tcx/egress", SCHED_CLS, BPF_TCX_EGRESS, SEC_ATTACHABLE_OPT),
at least for tcx attach_type is not optional, right? So I'd drop
SEC_ATTACHABLE_OPT.
> SEC_DEF("classifier", SCHED_CLS, 0, SEC_NONE),
> SEC_DEF("action", SCHED_ACT, 0, SEC_NONE),
> SEC_DEF("tracepoint+", TRACEPOINT, 0, SEC_NONE, attach_tp),
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index 7521a2fb7626..a29b90e9713c 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -395,4 +395,5 @@ LIBBPF_1.2.0 {
> LIBBPF_1.3.0 {
> global:
> bpf_obj_pin_opts;
> + bpf_prog_detach_opts;
> } LIBBPF_1.2.0;
> --
> 2.34.1
>
