Re: [PATCH v2 bpf-next 2/6] libbpf: Add BTF_KIND_FLOAT support

Yonghong Song <yhs@xxxxxx> · Fri, 19 Feb 2021 15:35:56 -0800

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@xxxxxxxxxxxxx>
---
   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.








