Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Yonghong Song <yhs@fb.com>
Cc: Hao Luo <haoluo@google.com>, Networking <netdev@vger.kernel.org>,
	bpf <bpf@vger.kernel.org>,
	open list <linux-kernel@vger.kernel.org>,
	"open list:KERNEL SELFTEST FRAMEWORK" 
	<linux-kselftest@vger.kernel.org>, Shuah Khan <shuah@kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Andrii Nakryiko <andriin@fb.com>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>,
	John Fastabend <john.fastabend@gmail.com>,
	KP Singh <kpsingh@chromium.org>,
	Quentin Monnet <quentin@isovalent.com>,
	Steven Rostedt <rostedt@goodmis.org>,
	Ingo Molnar <mingo@redhat.com>, Andrey Ignatov <rdna@fb.com>,
	Jakub Sitnicki <jakub@cloudflare.com>
Subject: Re: [PATCH bpf-next v1 3/8] bpf: Introduce help function to validate ksym's type.
Date: Fri, 21 Aug 2020 14:50:04 -0700	[thread overview]
Message-ID: <CAEf4BzZmLUcw4M16U6w-s2Zd6KbsuY4dzzkeEBx9CejetT5BwQ@mail.gmail.com> (raw)
In-Reply-To: <d50a1530-9a9f-45b2-5aba-05fe4b895fbc@fb.com>

On Thu, Aug 20, 2020 at 10:22 AM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 8/19/20 3:40 PM, Hao Luo wrote:
> > For a ksym to be safely dereferenced and accessed, its type defined in
> > bpf program should basically match its type defined in kernel. Implement
> > a help function for a quick matching, which is used by libbpf when
> > resolving the kernel btf_id of a ksym.
> >
> > Signed-off-by: Hao Luo <haoluo@google.com>
> > ---
> >   tools/lib/bpf/btf.c | 171 ++++++++++++++++++++++++++++++++++++++++++++
> >   tools/lib/bpf/btf.h |   2 +
> >   2 files changed, 173 insertions(+)
> >
> > diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> > index a3d259e614b0..2ff31f244d7a 100644
> > --- a/tools/lib/bpf/btf.c
> > +++ b/tools/lib/bpf/btf.c
> > @@ -1005,6 +1005,177 @@ int btf__get_map_kv_tids(const struct btf *btf, const char *map_name,
> >       return 0;
> >   }
> >
> > +/*
> > + * Basic type check for ksym support. Only checks type kind and resolved size.
> > + */
> > +static inline
> > +bool btf_ksym_equal_type(const struct btf *ba, __u32 type_a,
> > +                      const struct btf *bb, __u32 type_b)
>
> "ba" and "bb" is not descriptive. Maybe "btf_a" or "btf_b"?
> or even "btf1" or "btf2" since the number does not carry
> extra meaning compared to letters.
>
> The same for below, may be t1, t2?
>
> > +{
> > +     const struct btf_type *ta, *tb;
> > +
> > +     ta = btf__type_by_id(ba, type_a);
> > +     tb = btf__type_by_id(bb, type_b);
> > +
> > +     /* compare type kind */
> > +     if (btf_kind(ta) != btf_kind(tb))
> > +             return false;
> > +
> > +     /* compare resolved type size */
> > +     return btf__resolve_size(ba, type_a) == btf__resolve_size(bb, type_b);
> > +}
> > +
> > +/*
> > + * Match a ksym's type defined in bpf programs against its type encoded in
> > + * kernel btf.
> > + */
> > +bool btf_ksym_type_match(const struct btf *ba, __u32 id_a,
> > +                      const struct btf *bb, __u32 id_b)
> > +{

[...]

> > +                     }
> > +             }
>
> I am wondering whether this is too strict and how this can co-work with
> CO-RE. Forcing users to write almost identical structure definition to
> the underlying kernel will not be user friendly and may not work cross
> kernel versions even if the field user cares have not changed.
>
> Maybe we can relax the constraint here. You can look at existing
> libbpf CO-RE code.

Right. Hao, can you just re-use bpf_core_types_are_compat() instead?
See if semantics makes sense, but I think it should. BPF CO-RE has
been permissive in terms of struct size and few other type aspects,
because it handles relocations so well. This approach allows to not
have to exactly match all possible variations of some struct
definition, which is a big problem with ever-changing kernel data
structures.

>
> > +             break;
> > +     }

[...]

> > +
> >   struct btf_ext_sec_setup_param {
> >       __u32 off;
> >       __u32 len;
> > diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
> > index 91f0ad0e0325..5ef220e52485 100644
> > --- a/tools/lib/bpf/btf.h
> > +++ b/tools/lib/bpf/btf.h
> > @@ -52,6 +52,8 @@ LIBBPF_API int btf__get_map_kv_tids(const struct btf *btf, const char *map_name,
> >                                   __u32 expected_key_size,
> >                                   __u32 expected_value_size,
> >                                   __u32 *key_type_id, __u32 *value_type_id);
> > +LIBBPF_API bool btf_ksym_type_match(const struct btf *ba, __u32 id_a,
> > +                                 const struct btf *bb, __u32 id_b);
> >
> >   LIBBPF_API struct btf_ext *btf_ext__new(__u8 *data, __u32 size);
> >   LIBBPF_API void btf_ext__free(struct btf_ext *btf_ext);
>
> The new API function should be added to libbpf.map.

My question is why does this even have to be a public API?

  reply	other threads:[~2020-08-21 21:50 UTC|newest]

Thread overview: 42+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-19 22:40 [PATCH bpf-next v1 0/8] bpf: BTF support for ksyms Hao Luo
2020-08-19 22:40 ` [PATCH bpf-next v1 1/8] bpf: Introduce pseudo_btf_id Hao Luo
2020-08-20 15:22   ` Yonghong Song
2020-08-20 17:04     ` Hao Luo
2020-08-25  0:05     ` Hao Luo
2020-08-25  0:43       ` Yonghong Song
2020-08-20 16:43   ` Yonghong Song
2020-08-20 21:53   ` Alexei Starovoitov
2020-08-21  2:22     ` Hao Luo
2020-08-19 22:40 ` [PATCH bpf-next v1 2/8] bpf: Propagate BPF_PSEUDO_BTF_ID to uapi headers in /tools Hao Luo
2020-08-20 16:44   ` Yonghong Song
2020-08-19 22:40 ` [PATCH bpf-next v1 3/8] bpf: Introduce help function to validate ksym's type Hao Luo
2020-08-20 17:20   ` Yonghong Song
2020-08-21 21:50     ` Andrii Nakryiko [this message]
2020-08-22  0:43       ` Hao Luo
2020-08-22  2:43         ` Andrii Nakryiko
2020-08-22  7:04           ` Hao Luo
2020-08-19 22:40 ` [PATCH bpf-next v1 4/8] bpf/libbpf: BTF support for typed ksyms Hao Luo
2020-08-21 22:37   ` Andrii Nakryiko
2020-08-27 22:29     ` Hao Luo
2020-09-01 18:11       ` Andrii Nakryiko
2020-09-01 20:35         ` Hao Luo
2020-09-01 23:54           ` Andrii Nakryiko
2020-09-02  0:46             ` Hao Luo
2020-08-19 22:40 ` [PATCH bpf-next v1 5/8] bpf/selftests: ksyms_btf to test " Hao Luo
2020-08-20 17:28   ` Yonghong Song
2020-08-21 23:03     ` Andrii Nakryiko
2020-08-22  7:26       ` Hao Luo
2020-08-22  7:35         ` Andrii Nakryiko
2020-08-19 22:40 ` [PATCH bpf-next v1 6/8] bpf: Introduce bpf_per_cpu_ptr() Hao Luo
2020-08-22  3:26   ` Andrii Nakryiko
2020-08-22  3:31     ` Andrii Nakryiko
2020-08-22  7:49       ` Hao Luo
2020-08-22  7:55         ` Andrii Nakryiko
2020-08-25  1:03           ` Hao Luo
2020-08-19 22:40 ` [PATCH bpf-next v1 7/8] bpf: Propagate bpf_per_cpu_ptr() to /tools Hao Luo
2020-08-20 17:30   ` Yonghong Song
2020-08-19 22:40 ` [PATCH bpf-next v1 8/8] bpf/selftests: Test for bpf_per_cpu_ptr() Hao Luo
2020-08-22  3:30   ` Andrii Nakryiko
2020-08-28  3:42     ` Hao Luo
2020-09-01 18:12       ` Andrii Nakryiko
2020-09-01 19:47         ` Hao Luo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAEf4BzZmLUcw4M16U6w-s2Zd6KbsuY4dzzkeEBx9CejetT5BwQ@mail.gmail.com \
    --to=andrii.nakryiko@gmail.com \
    --cc=andriin@fb.com \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=haoluo@google.com \
    --cc=jakub@cloudflare.com \
    --cc=john.fastabend@gmail.com \
    --cc=kafai@fb.com \
    --cc=kpsingh@chromium.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=netdev@vger.kernel.org \
    --cc=quentin@isovalent.com \
    --cc=rdna@fb.com \
    --cc=rostedt@goodmis.org \
    --cc=shuah@kernel.org \
    --cc=songliubraving@fb.com \
    --cc=yhs@fb.com \
    --subject='Re: [PATCH bpf-next v1 3/8] bpf: Introduce help function to validate ksym'\''s type.' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).