Re: [PATCH bpf-next v2 3/7] libbpf: Add opts-based attach/detach/query API for tcx

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Daniel Borkmann <daniel@iogearbox.net>
Cc: ast@kernel.org, andrii@kernel.org, martin.lau@linux.dev,
	 razor@blackwall.org, sdf@google.com, john.fastabend@gmail.com,
	 kuba@kernel.org, dxu@dxuuu.xyz, joe@cilium.io, toke@kernel.org,
	 davem@davemloft.net, bpf@vger.kernel.org,
	netdev@vger.kernel.org
Subject: Re: [PATCH bpf-next v2 3/7] libbpf: Add opts-based attach/detach/query API for tcx
Date: Thu, 8 Jun 2023 14:37:11 -0700	[thread overview]
Message-ID: <CAEf4BzbMG5rugdOQyWJH2Ac7kAta8LPEhMbYUH2Bu9JOJU0P9w@mail.gmail.com> (raw)
In-Reply-To: <20230607192625.22641-4-daniel@iogearbox.net>

On Wed, Jun 7, 2023 at 12:26 PM Daniel Borkmann <daniel@iogearbox.net> 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@iogearbox.net>
> ---
>  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
>