BPF Archive on lore.kernel.org
 help / color / Atom feed
From: Yonghong Song <yhs@fb.com>
To: Ilya Leoshkevich <iii@linux.ibm.com>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>,
	Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: John Fastabend <john.fastabend@gmail.com>, <bpf@vger.kernel.org>,
	Heiko Carstens <heiko.carstens@de.ibm.com>,
	Vasily Gorbik <gor@linux.ibm.com>
Subject: Re: [PATCH v2 bpf-next 2/6] libbpf: Add BTF_KIND_FLOAT support
Date: Fri, 19 Feb 2021 15:35:56 -0800
Message-ID: <a5dacc42-66cf-ed1b-812c-d54bf7d42c2c@fb.com> (raw)
In-Reply-To: <0184706c7c4ba2d4a2c3a64d5bd51e5e7d65d3b5.camel@linux.ibm.com>



On 2/19/21 3:01 PM, Ilya Leoshkevich wrote:
> On Thu, 2021-02-18 at 20:22 -0800, Yonghong Song wrote:
>>
>>
>> On 2/18/21 6:25 PM, Ilya Leoshkevich wrote:
>>> The logic follows that of BTF_KIND_INT most of the time.
>>> Sanitization
>>> replaces BTF_KIND_FLOATs with BTF_KIND_TYPEDEFs pointing to
>>> equally-sized BTF_KIND_ARRAYs on older kernels.
>>>
>>> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
>>> ---
>>>    tools/lib/bpf/btf.c             | 44
>>> ++++++++++++++++++++++++++++++++
>>>    tools/lib/bpf/btf.h             |  8 ++++++
>>>    tools/lib/bpf/btf_dump.c        |  4 +++
>>>    tools/lib/bpf/libbpf.c          | 45
>>> ++++++++++++++++++++++++++++++++-
>>>    tools/lib/bpf/libbpf.map        |  5 ++++
>>>    tools/lib/bpf/libbpf_internal.h |  2 ++
>>>    6 files changed, 107 insertions(+), 1 deletion(-)
>>>
>> [...]
>>> diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
>>> index 2f9d685bd522..5e957fcceee6 100644
>>> --- a/tools/lib/bpf/btf_dump.c
>>> +++ b/tools/lib/bpf/btf_dump.c
>>> @@ -279,6 +279,7 @@ static int btf_dump_mark_referenced(struct
>>> btf_dump *d)
>>>                  case BTF_KIND_INT:
>>>                  case BTF_KIND_ENUM:
>>>                  case BTF_KIND_FWD:
>>> +               case BTF_KIND_FLOAT:
>>>                          break;
>>>    
>>>                  case BTF_KIND_VOLATILE:
>>> @@ -453,6 +454,7 @@ static int btf_dump_order_type(struct btf_dump
>>> *d, __u32 id, bool through_ptr)
>>>    
>>>          switch (btf_kind(t)) {
>>>          case BTF_KIND_INT:
>>> +       case BTF_KIND_FLOAT:
>>>                  tstate->order_state = ORDERED;
>>>                  return 0;
>>>    
>>> @@ -1133,6 +1135,7 @@ static void btf_dump_emit_type_decl(struct
>>> btf_dump *d, __u32 id,
>>>                  case BTF_KIND_STRUCT:
>>>                  case BTF_KIND_UNION:
>>>                  case BTF_KIND_TYPEDEF:
>>> +               case BTF_KIND_FLOAT:
>>>                          goto done;
>>>                  default:
>>>                          pr_warn("unexpected type in decl chain,
>>> kind:%u, id:[%u]\n",
>>> @@ -1247,6 +1250,7 @@ static void btf_dump_emit_type_chain(struct
>>> btf_dump *d,
>>>    
>>>                  switch (kind) {
>>>                  case BTF_KIND_INT:
>>> +               case BTF_KIND_FLOAT:
>>>                          btf_dump_emit_mods(d, decls);
>>>                          name = btf_name_of(d, t->name_off);
>>>                          btf_dump_printf(d, "%s", name);
>>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>>> index d43cc3f29dae..3b170066d613 100644
>>> --- a/tools/lib/bpf/libbpf.c
>>> +++ b/tools/lib/bpf/libbpf.c
>>> @@ -178,6 +178,8 @@ enum kern_feature_id {
>>>          FEAT_PROG_BIND_MAP,
>>>          /* Kernel support for module BTFs */
>>>          FEAT_MODULE_BTF,
>>> +       /* BTF_KIND_FLOAT support */
>>> +       FEAT_BTF_FLOAT,
>>>          __FEAT_CNT,
>>>    };
>>>    
>>> @@ -1935,6 +1937,7 @@ static const char *btf_kind_str(const struct
>>> btf_type *t)
>>>          case BTF_KIND_FUNC_PROTO: return "func_proto";
>>>          case BTF_KIND_VAR: return "var";
>>>          case BTF_KIND_DATASEC: return "datasec";
>>> +       case BTF_KIND_FLOAT: return "float";
>>>          default: return "unknown";
>>>          }
>>>    }
>>> @@ -2384,18 +2387,22 @@ static bool btf_needs_sanitization(struct
>>> bpf_object *obj)
>>>    {
>>>          bool has_func_global =
>>> kernel_supports(FEAT_BTF_GLOBAL_FUNC);
>>>          bool has_datasec = kernel_supports(FEAT_BTF_DATASEC);
>>> +       bool has_float = kernel_supports(FEAT_BTF_FLOAT);
>>>          bool has_func = kernel_supports(FEAT_BTF_FUNC);
>>>    
>>> -       return !has_func || !has_datasec || !has_func_global;
>>> +       return !has_func || !has_datasec || !has_func_global ||
>>> !has_float;
>>>    }
>>>    
>>>    static void bpf_object__sanitize_btf(struct bpf_object *obj,
>>> struct btf *btf)
>>>    {
>>>          bool has_func_global =
>>> kernel_supports(FEAT_BTF_GLOBAL_FUNC);
>>>          bool has_datasec = kernel_supports(FEAT_BTF_DATASEC);
>>> +       bool has_float = kernel_supports(FEAT_BTF_FLOAT);
>>>          bool has_func = kernel_supports(FEAT_BTF_FUNC);
>>>          struct btf_type *t;
>>>          int i, j, vlen;
>>> +       int t_u32 = 0;
>>> +       int t_u8 = 0;
>>>    
>>>          for (i = 1; i <= btf__get_nr_types(btf); i++) {
>>>                  t = (struct btf_type *)btf__type_by_id(btf, i);
>>> @@ -2445,6 +2452,23 @@ static void bpf_object__sanitize_btf(struct
>>> bpf_object *obj, struct btf *btf)
>>>                  } else if (!has_func_global && btf_is_func(t)) {
>>>                          /* replace BTF_FUNC_GLOBAL with
>>> BTF_FUNC_STATIC */
>>>                          t->info = BTF_INFO_ENC(BTF_KIND_FUNC, 0,
>>> 0);
>>> +               } else if (!has_float && btf_is_float(t)) {
>>> +                       /* replace FLOAT with TYPEDEF(u8[]) */
>>> +                       int t_array;
>>> +                       __u32 size;
>>> +
>>> +                       size = t->size;
>>> +                       if (!t_u8)
>>> +                               t_u8 = btf__add_int(btf, "u8", 1,
>>> 0);
>>> +                       if (!t_u32)
>>> +                               t_u32 = btf__add_int(btf, "u32", 4,
>>> 0);
>>> +                       t_array = btf__add_array(btf, t_u32, t_u8,
>>> size);
>>> +
>>> +                       /* adding new types may have invalidated t
>>> */
>>> +                       t = (struct btf_type *)btf__type_by_id(btf,
>>> i);
>>> +
>>> +                       t->info = BTF_INFO_ENC(BTF_KIND_TYPEDEF, 0,
>>> 0);
>>
>> This won't work. The typedef must have a valid name. Otherwise,
>> kernel
>> will reject it. A const char array should be okay here.
> 
> Wouldn't it reuse the old t->name? At least in my testing with v5.7
> kernel this was the case, and BTF wasn't rejected. And the original
> BTF_KIND_FLOAT always has a valid name, this is enforced in libbpf and
> in the verifier. For example:
> 
> [1] PTR '(anon)' type_id=2
> [2] STRUCT 'foo' size=4 vlen=1
> 	'bar' type_id=3 bits_offset=0
> [3] FLOAT 'float' size=4
> [4] FUNC_PROTO '(anon)' ret_type_id=5 vlen=1
> 	'f' type_id=1
> [5] PTR '(anon)' type_id=0
> [6] FUNC 'func' type_id=4 linkage=global
> 
> becomes
> 
> [1] PTR '(anon)' type_id=2
> [2] STRUCT 'foo' size=4 vlen=1
> 	'bar' type_id=3 bits_offset=0
> [3] TYPEDEF 'float' type_id=9
> [4] FUNC_PROTO '(anon)' ret_type_id=5 vlen=1
> 	'f' type_id=1
> [5] PTR '(anon)' type_id=0
> [6] FUNC 'func' type_id=4 linkage=global
> [7] INT 'u8' size=1 bits_offset=0 nr_bits=8 encoding=(none)
> [8] INT 'u32' size=4 bits_offset=0 nr_bits=32 encoding=(none)
> [9] ARRAY '(anon)' type_id=7 index_type_id=8 nr_elems=4

good point, the name is indeed there.

I originally also thought about using "typedef" but worried
whether new typedef may polute the existing type names.
Could you try to dump the modified BTF to a C file
and compile to see whether typedef mechanism works or not?
I tried the following code and compilation failed.

-bash-4.4$ cat t.c
typedef char * float;
-bash-4.4$ gcc -c t.c
t.c:1:16: error: expected identifier or ‘(’ before ‘float’
  typedef char * float;
                 ^
-bash-4.4$

Changing to a different name may interfere with existing types.

It may work with the kernel today because we didn't do strict type 
legality checking, but it would be still be good if we can
avoid it.

> 

  reply index

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-19  2:25 [PATCH v2 bpf-next 0/6] " Ilya Leoshkevich
2021-02-19  2:25 ` [PATCH v2 bpf-next 1/6] bpf: Add BTF_KIND_FLOAT to uapi Ilya Leoshkevich
2021-02-19  2:25 ` [PATCH v2 bpf-next 2/6] libbpf: Add BTF_KIND_FLOAT support Ilya Leoshkevich
2021-02-19  4:22   ` Yonghong Song
2021-02-19 23:01     ` Ilya Leoshkevich
2021-02-19 23:35       ` Yonghong Song [this message]
2021-02-19 23:43         ` Ilya Leoshkevich
2021-02-19  2:25 ` [PATCH v2 bpf-next 3/6] tools/bpftool: " Ilya Leoshkevich
2021-02-19  2:25 ` [PATCH v2 bpf-next 4/6] bpf: " Ilya Leoshkevich
2021-02-19  4:04   ` kernel test robot
2021-02-19  4:13   ` kernel test robot
2021-02-19  2:25 ` [PATCH v2 bpf-next 5/6] selftest/bpf: Add BTF_KIND_FLOAT tests Ilya Leoshkevich
2021-02-19  5:38   ` Yonghong Song
2021-02-19  5:44   ` Yonghong Song
2021-02-19  2:25 ` [PATCH v2 bpf-next 6/6] bpf: Document BTF_KIND_FLOAT in btf.rst Ilya Leoshkevich
2021-02-19  5:41   ` Yonghong Song
2021-02-19 13:00     ` Ilya Leoshkevich
2021-02-19 15:26       ` Yonghong Song

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=a5dacc42-66cf-ed1b-812c-d54bf7d42c2c@fb.com \
    --to=yhs@fb.com \
    --cc=acme@redhat.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=gor@linux.ibm.com \
    --cc=heiko.carstens@de.ibm.com \
    --cc=iii@linux.ibm.com \
    --cc=john.fastabend@gmail.com \
    /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

BPF Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/bpf/0 bpf/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 bpf bpf/ https://lore.kernel.org/bpf \
		bpf@vger.kernel.org
	public-inbox-index bpf

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.bpf


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git