[PATCH bpf-next] bpf: net: emit anonymous enum with BPF_TCP_CLOSE value explicitly

[PATCH bpf-next] bpf: net: emit anonymous enum with BPF_TCP_CLOSE value explicitly

[Yonghong Song]

The selftest failed to compile with clang-built bpf-next.
Adding LLVM=1 to your vmlinux and selftest build will use clang.
The error message is:
  progs/test_sk_storage_tracing.c:38:18: error: use of undeclared identifier 'BPF_TCP_CLOSE'
          if (newstate == BPF_TCP_CLOSE)
                          ^
  1 error generated.
  make: *** [Makefile:423: /bpf-next/tools/testing/selftests/bpf/test_sk_storage_tracing.o] Error 1

The reason for the failure is that BPF_TCP_CLOSE, a value of
an anonymous enum defined in uapi bpf.h, is not defined in
vmlinux.h. gcc does not have this problem. Since vmlinux.h
is derived from BTF which is derived from vmlinux dwarf,
that means gcc-produced vmlinux dwarf has BPF_TCP_CLOSE
while llvm-produced vmlinux dwarf does not have.

BPF_TCP_CLOSE is referenced in net/ipv4/tcp.c as
  BUILD_BUG_ON((int)BPF_TCP_CLOSE != (int)TCP_CLOSE);
The following test mimics the above BUILD_BUG_ON, preprocessed
with clang compiler, and shows gcc dwarf contains BPF_TCP_CLOSE while
llvm dwarf does not.

  $ cat t.c
  enum {
    BPF_TCP_ESTABLISHED = 1,
    BPF_TCP_CLOSE = 7,
  };
  enum {
    TCP_ESTABLISHED = 1,
    TCP_CLOSE = 7,
  };

  int test() {
    do {
      extern void __compiletime_assert_767(void) ;
      if ((int)BPF_TCP_CLOSE != (int)TCP_CLOSE) __compiletime_assert_767();
    } while (0);
    return 0;
  }
  $ clang t.c -O2 -c -g && llvm-dwarfdump t.o | grep BPF_TCP_CLOSE
  $ gcc t.c -O2 -c -g && llvm-dwarfdump t.o | grep BPF_TCP_CLOSE
                    DW_AT_name    ("BPF_TCP_CLOSE")

Further checking clang code find clang actually tried to
evaluate condition at compile time. If it is definitely
true/false, it will perform optimization and the whole if condition
will be removed before generating IR/debuginfo.

This patch explicited add an expression like
  (void)BPF_TCP_ESTABLISHED
to enable generation of debuginfo for the anonymous
enum which also includes BPF_TCP_CLOSE. I put
this explicit type generation in kernel/bpf/core.c
to (1) avoid polute net/ipv4/tcp.c and more importantly
(2) provide a central place to add other types (e.g. in
bpf/btf uapi header) if they are not referenced in the kernel
or generated in vmlinux dwarf.

Signed-off-by: Yonghong Song <yhs@fb.com>
---
 include/linux/btf.h |  1 +
 kernel/bpf/core.c   | 19 +++++++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/include/linux/btf.h b/include/linux/btf.h
index 7fabf1428093..9c1b52738bbe 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -9,6 +9,7 @@
 #include <uapi/linux/bpf.h>
 
 #define BTF_TYPE_EMIT(type) ((void)(type *)0)
+#define BTF_TYPE_EMIT_ENUM(enum_val) ((void)enum_val)
 
 struct btf;
 struct btf_member;
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 3a283bf97f2f..60551bf68ece 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -2378,3 +2378,22 @@ EXPORT_SYMBOL(bpf_stats_enabled_key);
 
 EXPORT_TRACEPOINT_SYMBOL_GPL(xdp_exception);
 EXPORT_TRACEPOINT_SYMBOL_GPL(xdp_bulk_tx);
+
+static int __init bpf_emit_btf_type(void)
+{
+	/* bpf uapi header bpf.h defines an anonymous enum with values
+	 * BPF_TCP_* used by bpf programs. Currently gcc built vmlinux
+	 * is able to emit this enum in dwarf due to the following
+	 * BUILD_BUG_ON test in net/ipv4/tcp.c:
+	 *   BUILD_BUG_ON((int)BPF_TCP_ESTABLISHED != (int)TCP_ESTABLISHED);
+	 * clang built vmlinux does not have this enum in dwarf
+	 * since clang removes the above code before generating IR/debuginfo.
+	 * Let us explicitly emit the type debuginfo to ensure the
+	 * above-mentioned anonymous enum in the vmlinux dwarf and hence BTF
+	 * regardless of which compiler is used.
+	 */
+	BTF_TYPE_EMIT_ENUM(BPF_TCP_ESTABLISHED);
+
+	return 0;
+}
+late_initcall(bpf_emit_btf_type);
-- 
2.24.1

Re: [PATCH bpf-next] bpf: net: emit anonymous enum with BPF_TCP_CLOSE value explicitly

[Daniel Borkmann]

On 3/14/21 4:58 AM, Yonghong Song wrote:
[...]
> This patch explicited add an expression like
>    (void)BPF_TCP_ESTABLISHED
> to enable generation of debuginfo for the anonymous
> enum which also includes BPF_TCP_CLOSE. I put
> this explicit type generation in kernel/bpf/core.c
> to (1) avoid polute net/ipv4/tcp.c and more importantly
> (2) provide a central place to add other types (e.g. in
> bpf/btf uapi header) if they are not referenced in the kernel
> or generated in vmlinux dwarf.
> 
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
>   include/linux/btf.h |  1 +
>   kernel/bpf/core.c   | 19 +++++++++++++++++++
>   2 files changed, 20 insertions(+)
> 
> diff --git a/include/linux/btf.h b/include/linux/btf.h
> index 7fabf1428093..9c1b52738bbe 100644
> --- a/include/linux/btf.h
> +++ b/include/linux/btf.h
> @@ -9,6 +9,7 @@
>   #include <uapi/linux/bpf.h>
>   
>   #define BTF_TYPE_EMIT(type) ((void)(type *)0)
> +#define BTF_TYPE_EMIT_ENUM(enum_val) ((void)enum_val)
>   
>   struct btf;
>   struct btf_member;
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 3a283bf97f2f..60551bf68ece 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -2378,3 +2378,22 @@ EXPORT_SYMBOL(bpf_stats_enabled_key);
>   
>   EXPORT_TRACEPOINT_SYMBOL_GPL(xdp_exception);
>   EXPORT_TRACEPOINT_SYMBOL_GPL(xdp_bulk_tx);
> +
> +static int __init bpf_emit_btf_type(void)
> +{
> +	/* bpf uapi header bpf.h defines an anonymous enum with values
> +	 * BPF_TCP_* used by bpf programs. Currently gcc built vmlinux
> +	 * is able to emit this enum in dwarf due to the following
> +	 * BUILD_BUG_ON test in net/ipv4/tcp.c:
> +	 *   BUILD_BUG_ON((int)BPF_TCP_ESTABLISHED != (int)TCP_ESTABLISHED);
> +	 * clang built vmlinux does not have this enum in dwarf
> +	 * since clang removes the above code before generating IR/debuginfo.
> +	 * Let us explicitly emit the type debuginfo to ensure the
> +	 * above-mentioned anonymous enum in the vmlinux dwarf and hence BTF
> +	 * regardless of which compiler is used.
> +	 */
> +	BTF_TYPE_EMIT_ENUM(BPF_TCP_ESTABLISHED);
> +
> +	return 0;
> +}
> +late_initcall(bpf_emit_btf_type);

Does this have to be late_initcall() given this adds minor init call
overhead, what if this would be exported as symbol for modules instead?

Thanks,
Daniel

Re: [PATCH bpf-next] bpf: net: emit anonymous enum with BPF_TCP_CLOSE value explicitly

[Yonghong Song]

On 3/15/21 4:33 PM, Daniel Borkmann wrote:
> On 3/14/21 4:58 AM, Yonghong Song wrote:
> [...]
>> This patch explicited add an expression like
>>    (void)BPF_TCP_ESTABLISHED
>> to enable generation of debuginfo for the anonymous
>> enum which also includes BPF_TCP_CLOSE. I put
>> this explicit type generation in kernel/bpf/core.c
>> to (1) avoid polute net/ipv4/tcp.c and more importantly
>> (2) provide a central place to add other types (e.g. in
>> bpf/btf uapi header) if they are not referenced in the kernel
>> or generated in vmlinux dwarf.
>>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>>   include/linux/btf.h |  1 +
>>   kernel/bpf/core.c   | 19 +++++++++++++++++++
>>   2 files changed, 20 insertions(+)
>>
>> diff --git a/include/linux/btf.h b/include/linux/btf.h
>> index 7fabf1428093..9c1b52738bbe 100644
>> --- a/include/linux/btf.h
>> +++ b/include/linux/btf.h
>> @@ -9,6 +9,7 @@
>>   #include <uapi/linux/bpf.h>
>>   #define BTF_TYPE_EMIT(type) ((void)(type *)0)
>> +#define BTF_TYPE_EMIT_ENUM(enum_val) ((void)enum_val)
>>   struct btf;
>>   struct btf_member;
>> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
>> index 3a283bf97f2f..60551bf68ece 100644
>> --- a/kernel/bpf/core.c
>> +++ b/kernel/bpf/core.c
>> @@ -2378,3 +2378,22 @@ EXPORT_SYMBOL(bpf_stats_enabled_key);
>>   EXPORT_TRACEPOINT_SYMBOL_GPL(xdp_exception);
>>   EXPORT_TRACEPOINT_SYMBOL_GPL(xdp_bulk_tx);
>> +
>> +static int __init bpf_emit_btf_type(void)
>> +{
>> +    /* bpf uapi header bpf.h defines an anonymous enum with values
>> +     * BPF_TCP_* used by bpf programs. Currently gcc built vmlinux
>> +     * is able to emit this enum in dwarf due to the following
>> +     * BUILD_BUG_ON test in net/ipv4/tcp.c:
>> +     *   BUILD_BUG_ON((int)BPF_TCP_ESTABLISHED != (int)TCP_ESTABLISHED);
>> +     * clang built vmlinux does not have this enum in dwarf
>> +     * since clang removes the above code before generating 
>> IR/debuginfo.
>> +     * Let us explicitly emit the type debuginfo to ensure the
>> +     * above-mentioned anonymous enum in the vmlinux dwarf and hence BTF
>> +     * regardless of which compiler is used.
>> +     */
>> +    BTF_TYPE_EMIT_ENUM(BPF_TCP_ESTABLISHED);
>> +
>> +    return 0;
>> +}
>> +late_initcall(bpf_emit_btf_type);
> 
> Does this have to be late_initcall() given this adds minor init call
> overhead, what if this would be exported as symbol for modules instead?

If issuing types in module, if I understand correctly, it will not be in
main vmlinux btf, so programs will not be able to use unless module
is loaded. I would prefer such types always available in vmlinux btf.

I am using a separate late_initcall just to cleaner codes. But
this BTF_TYPE_EMIT_ENUM can be in any init call.

$ grep _initcall *.c
btf.c:fs_initcall(btf_module_init);
core.c:pure_initcall(bpf_jit_charge_init);
cpumap.c:subsys_initcall(cpu_map_init);
devmap.c:subsys_initcall(dev_map_init);
inode.c:fs_initcall(bpf_init);
map_iter.c:late_initcall(bpf_map_iter_init);
net_namespace.c:subsys_initcall(netns_bpf_init);
prog_iter.c:late_initcall(bpf_prog_iter_init);
stackmap.c:subsys_initcall(stack_map_init);
sysfs_btf.c:subsys_initcall(btf_vmlinux_init);
task_iter.c:late_initcall(task_iter_init);
trampoline.c:late_initcall(init_trampolines);
$

I think we can use any above in kernel/bpf directory. This way,
we will have 0 runtime overhead as the code will be optimized away.
Any preference?

> 
> Thanks,
> Daniel

Re: [PATCH bpf-next] bpf: net: emit anonymous enum with BPF_TCP_CLOSE value explicitly

[Yonghong Song]

On 3/15/21 5:18 PM, Yonghong Song wrote:
> 
> 
> On 3/15/21 4:33 PM, Daniel Borkmann wrote:
>> On 3/14/21 4:58 AM, Yonghong Song wrote:
>> [...]
>>> This patch explicited add an expression like
>>>    (void)BPF_TCP_ESTABLISHED
>>> to enable generation of debuginfo for the anonymous
>>> enum which also includes BPF_TCP_CLOSE. I put
>>> this explicit type generation in kernel/bpf/core.c
>>> to (1) avoid polute net/ipv4/tcp.c and more importantly
>>> (2) provide a central place to add other types (e.g. in
>>> bpf/btf uapi header) if they are not referenced in the kernel
>>> or generated in vmlinux dwarf.
>>>
>>> Signed-off-by: Yonghong Song <yhs@fb.com>
>>> ---
>>>   include/linux/btf.h |  1 +
>>>   kernel/bpf/core.c   | 19 +++++++++++++++++++
>>>   2 files changed, 20 insertions(+)
>>>
>>> diff --git a/include/linux/btf.h b/include/linux/btf.h
>>> index 7fabf1428093..9c1b52738bbe 100644
>>> --- a/include/linux/btf.h
>>> +++ b/include/linux/btf.h
>>> @@ -9,6 +9,7 @@
>>>   #include <uapi/linux/bpf.h>
>>>   #define BTF_TYPE_EMIT(type) ((void)(type *)0)
>>> +#define BTF_TYPE_EMIT_ENUM(enum_val) ((void)enum_val)
>>>   struct btf;
>>>   struct btf_member;
>>> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
>>> index 3a283bf97f2f..60551bf68ece 100644
>>> --- a/kernel/bpf/core.c
>>> +++ b/kernel/bpf/core.c
>>> @@ -2378,3 +2378,22 @@ EXPORT_SYMBOL(bpf_stats_enabled_key);
>>>   EXPORT_TRACEPOINT_SYMBOL_GPL(xdp_exception);
>>>   EXPORT_TRACEPOINT_SYMBOL_GPL(xdp_bulk_tx);
>>> +
>>> +static int __init bpf_emit_btf_type(void)
>>> +{
>>> +    /* bpf uapi header bpf.h defines an anonymous enum with values
>>> +     * BPF_TCP_* used by bpf programs. Currently gcc built vmlinux
>>> +     * is able to emit this enum in dwarf due to the following
>>> +     * BUILD_BUG_ON test in net/ipv4/tcp.c:
>>> +     *   BUILD_BUG_ON((int)BPF_TCP_ESTABLISHED != 
>>> (int)TCP_ESTABLISHED);
>>> +     * clang built vmlinux does not have this enum in dwarf
>>> +     * since clang removes the above code before generating 
>>> IR/debuginfo.
>>> +     * Let us explicitly emit the type debuginfo to ensure the
>>> +     * above-mentioned anonymous enum in the vmlinux dwarf and hence 
>>> BTF
>>> +     * regardless of which compiler is used.
>>> +     */
>>> +    BTF_TYPE_EMIT_ENUM(BPF_TCP_ESTABLISHED);
>>> +
>>> +    return 0;
>>> +}
>>> +late_initcall(bpf_emit_btf_type);
>>
>> Does this have to be late_initcall() given this adds minor init call
>> overhead, what if this would be exported as symbol for modules instead?

You mean EXPORT_SYMBOL, right? I think it should work, just one extra
unused empty global function, but a little bit misleading since the 
function is not intended to use by anybody. I am using init call and
wants to remove this function after init...

> 
> If issuing types in module, if I understand correctly, it will not be in
> main vmlinux btf, so programs will not be able to use unless module
> is loaded. I would prefer such types always available in vmlinux btf.
> 
> I am using a separate late_initcall just to cleaner codes. But
> this BTF_TYPE_EMIT_ENUM can be in any init call.
> 
> $ grep _initcall *.c
> btf.c:fs_initcall(btf_module_init);
> core.c:pure_initcall(bpf_jit_charge_init);
> cpumap.c:subsys_initcall(cpu_map_init);
> devmap.c:subsys_initcall(dev_map_init);
> inode.c:fs_initcall(bpf_init);
> map_iter.c:late_initcall(bpf_map_iter_init);
> net_namespace.c:subsys_initcall(netns_bpf_init);
> prog_iter.c:late_initcall(bpf_prog_iter_init);
> stackmap.c:subsys_initcall(stack_map_init);
> sysfs_btf.c:subsys_initcall(btf_vmlinux_init);
> task_iter.c:late_initcall(task_iter_init);
> trampoline.c:late_initcall(init_trampolines);
> $
> 
> I think we can use any above in kernel/bpf directory. This way,
> we will have 0 runtime overhead as the code will be optimized away.
> Any preference?
> 
>>
>> Thanks,
>> Daniel

Re: [PATCH bpf-next] bpf: net: emit anonymous enum with BPF_TCP_CLOSE value explicitly

[Andrii Nakryiko]

On Sat, Mar 13, 2021 at 8:15 PM Yonghong Song <yhs@fb.com> wrote:
>
> The selftest failed to compile with clang-built bpf-next.
> Adding LLVM=1 to your vmlinux and selftest build will use clang.
> The error message is:
>   progs/test_sk_storage_tracing.c:38:18: error: use of undeclared identifier 'BPF_TCP_CLOSE'
>           if (newstate == BPF_TCP_CLOSE)
>                           ^
>   1 error generated.
>   make: *** [Makefile:423: /bpf-next/tools/testing/selftests/bpf/test_sk_storage_tracing.o] Error 1
>
> The reason for the failure is that BPF_TCP_CLOSE, a value of
> an anonymous enum defined in uapi bpf.h, is not defined in
> vmlinux.h. gcc does not have this problem. Since vmlinux.h
> is derived from BTF which is derived from vmlinux dwarf,
> that means gcc-produced vmlinux dwarf has BPF_TCP_CLOSE
> while llvm-produced vmlinux dwarf does not have.
>
> BPF_TCP_CLOSE is referenced in net/ipv4/tcp.c as
>   BUILD_BUG_ON((int)BPF_TCP_CLOSE != (int)TCP_CLOSE);
> The following test mimics the above BUILD_BUG_ON, preprocessed
> with clang compiler, and shows gcc dwarf contains BPF_TCP_CLOSE while
> llvm dwarf does not.
>
>   $ cat t.c
>   enum {
>     BPF_TCP_ESTABLISHED = 1,
>     BPF_TCP_CLOSE = 7,
>   };
>   enum {
>     TCP_ESTABLISHED = 1,
>     TCP_CLOSE = 7,
>   };
>
>   int test() {
>     do {
>       extern void __compiletime_assert_767(void) ;
>       if ((int)BPF_TCP_CLOSE != (int)TCP_CLOSE) __compiletime_assert_767();
>     } while (0);
>     return 0;
>   }
>   $ clang t.c -O2 -c -g && llvm-dwarfdump t.o | grep BPF_TCP_CLOSE
>   $ gcc t.c -O2 -c -g && llvm-dwarfdump t.o | grep BPF_TCP_CLOSE
>                     DW_AT_name    ("BPF_TCP_CLOSE")
>
> Further checking clang code find clang actually tried to
> evaluate condition at compile time. If it is definitely
> true/false, it will perform optimization and the whole if condition
> will be removed before generating IR/debuginfo.
>
> This patch explicited add an expression like
>   (void)BPF_TCP_ESTABLISHED
> to enable generation of debuginfo for the anonymous
> enum which also includes BPF_TCP_CLOSE. I put
> this explicit type generation in kernel/bpf/core.c
> to (1) avoid polute net/ipv4/tcp.c and more importantly
> (2) provide a central place to add other types (e.g. in
> bpf/btf uapi header) if they are not referenced in the kernel
> or generated in vmlinux dwarf.
>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---

Acked-by: Andrii Nakryiko <andrii@kernel.org>

But given

$ rg '\sdwarf\s' | wc -l
33
$ rg '\sDWARF\s' | wc -l
151

we should probably stick to using "DWARF", not "dwarf", everywhere.


>  include/linux/btf.h |  1 +
>  kernel/bpf/core.c   | 19 +++++++++++++++++++
>  2 files changed, 20 insertions(+)
>
> diff --git a/include/linux/btf.h b/include/linux/btf.h
> index 7fabf1428093..9c1b52738bbe 100644
> --- a/include/linux/btf.h
> +++ b/include/linux/btf.h
> @@ -9,6 +9,7 @@
>  #include <uapi/linux/bpf.h>
>
>  #define BTF_TYPE_EMIT(type) ((void)(type *)0)
> +#define BTF_TYPE_EMIT_ENUM(enum_val) ((void)enum_val)
>
>  struct btf;
>  struct btf_member;
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 3a283bf97f2f..60551bf68ece 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -2378,3 +2378,22 @@ EXPORT_SYMBOL(bpf_stats_enabled_key);
>
>  EXPORT_TRACEPOINT_SYMBOL_GPL(xdp_exception);
>  EXPORT_TRACEPOINT_SYMBOL_GPL(xdp_bulk_tx);
> +
> +static int __init bpf_emit_btf_type(void)
> +{
> +       /* bpf uapi header bpf.h defines an anonymous enum with values
> +        * BPF_TCP_* used by bpf programs. Currently gcc built vmlinux
> +        * is able to emit this enum in dwarf due to the following
> +        * BUILD_BUG_ON test in net/ipv4/tcp.c:
> +        *   BUILD_BUG_ON((int)BPF_TCP_ESTABLISHED != (int)TCP_ESTABLISHED);
> +        * clang built vmlinux does not have this enum in dwarf
> +        * since clang removes the above code before generating IR/debuginfo.
> +        * Let us explicitly emit the type debuginfo to ensure the
> +        * above-mentioned anonymous enum in the vmlinux dwarf and hence BTF
> +        * regardless of which compiler is used.
> +        */
> +       BTF_TYPE_EMIT_ENUM(BPF_TCP_ESTABLISHED);
> +
> +       return 0;
> +}
> +late_initcall(bpf_emit_btf_type);
> --
> 2.24.1
>