Re: [RFC bpf-next 00/12] Use uapi kernel headers with vmlinux.h

[Alan Maguire <alan.maguire@oracle.com>]

On 28/10/2022 22:35, Andrii Nakryiko wrote:
> On Fri, Oct 28, 2022 at 11:57 AM Yonghong Song <yhs@meta.com> wrote:
>>
>>
>>
>> On 10/28/22 10:13 AM, Andrii Nakryiko wrote:
>>> On Thu, Oct 27, 2022 at 6:33 PM Yonghong Song <yhs@meta.com> wrote:
>>>>
>>>>
>>>>
>>>> On 10/27/22 4:14 PM, Andrii Nakryiko wrote:
>>>>> On Tue, Oct 25, 2022 at 3:28 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>>>>>>
>>>>>> Hi BPF community,
>>>>>>
>>>>>> AFAIK there is a long standing feature request to use kernel headers
>>>>>> alongside `vmlinux.h` generated by `bpftool`. For example significant
>>>>>> effort was put to add an attribute `bpf_dominating_decl` (see [1]) to
>>>>>> clang, unfortunately this effort was stuck due to concerns regarding C
>>>>>> language semantics.
>>>>>>
>>>>>
>>>>> Maybe we should make another attempt to implement bpf_dominating_decl?
>>>>> That seems like a more elegant solution than any other implemented or
>>>>> discussed alternative. Yonghong, WDYT?
>>>>
>>>> I would say it would be very difficult for upstream to agree with
>>>> bpf_dominating_decl. We already have lots of discussions and we
>>>> likely won't be able to satisfy Aaron who wants us to emit
>>>> adequate diagnostics which will involve lots of other work
>>>> and he also thinks this is too far away from C standard and he
>>>> wants us to implement in a llvm/clang tool which is not what
>>>> we want.
>>>
>>> Ok, could we change the problem to detecting if some type is defined.
>>> Would it be possible to have something like
>>>
>>> #if !__is_type_defined(struct abc)
>>> struct abc {
>>> };
>>> #endif
>>>
>>> I think we talked about this and there were problems with this
>>> approach, but I don't remember details and how insurmountable the
>>> problem is. Having a way to check whether some type is defined would
>>> be very useful even outside of -target bpf parlance, though, so maybe
>>> it's the problem worth attacking?
>>
>> Yes, we discussed this before. This will need to add additional work
>> in preprocessor. I just made a discussion topic in llvm discourse
>>
>> https://discourse.llvm.org/t/add-a-type-checking-macro-is-type-defined-type/66268
>>
>> Let us see whether we can get some upstream agreement or not.
>>
> 
> Thanks for starting the conversation! I'll be following along.
>


I think this sort of approach assumes that vmlinux.h is included after
any uapi headers, and would guard type definitions with 

#if type_is_defined(foo)
struct foo {

};
#endif

...is that right? My concern is that the vmlinux.h definitions have
the CO-RE attributes. From a BPF perspective, would we like the vmlinux.h
definitions to dominate over UAPI definitions do you think, or does it
matter?

I was wondering if there might be yet another way to crack this;
if we did want the vmlinux.h type definitions to be authoritative
because they have the preserve access index attribute, and because
bpftool knows all vmlinux types, it could use that info to selectively
redefine those type names such that we avoid name clashes when later
including UAPI headers. Something like

#ifdef __VMLINUX_H__
//usual vmlinux.h type definitions
#endif /* __VMLINUX_H__ */

#ifdef __VMLINUX_ALIAS__
if !defined(timespec64)
#define timespec64 __VMLINUX_ALIAS__timespec64
#endif
// rest of the types define aliases here
#undef __VMLINUX_ALIAS__
#else /* unalias */
#if defined(timespec64)
#undef timespec64
#endif
// rest of types undef aliases here
#endif /* __VMLINUX_ALIAS__ */


Then the consumer does this:

#define __VMLINUX_ALIAS__
#include "vmlinux.h"
// include uapi headers
#include "vmlinux.h"

(the latter include of vmlinux.h is needed to undef all the type aliases)

I tried hacking up bpftool to support this aliasing scheme and while 
it is kind of hacky it does seem to work, aside from some issues with 
IPPROTO_* definitions - for the enumerated IPPROTO_ values linux/in.h does
this:

enum {
  IPPROTO_IP = 0,               /* Dummy protocol for TCP               */
#define IPPROTO_IP              IPPROTO_IP
  IPPROTO_ICMP = 1,             /* Internet Control Message Protocol    */
#define IPPROTO_ICMP            IPPROTO_ICMP


...so our enum value definitions for IPPROTO_ values clash with the above 
definitions. These could be individually ifdef-guarded if needed though I think.

I can send the proof-of-concept patch if it would help, I just wanted to 
check in case that might be a workable path too, since it just requires 
changes to bpftool (and changes to in.h).

Thanks!

Alan

 
>>>
>>>>
>>>>>
>>>>> BTW, I suggest splitting libbpf btf_dedup and btf_dump changes into a
>>>>> separate series and sending them as non-RFC sooner. Those improvements
>>>>> are independent of all the header guards stuff, let's get them landed
>>>>> sooner.
>>>>>
>>>>>> After some discussion with Alexei and Yonghong I'd like to request
>>>>>> your comments regarding a somewhat brittle and partial solution to
>>>>>> this issue that relies on adding `#ifndef FOO_H ... #endif` guards in
>>>>>> the generated `vmlinux.h`.
>>>>>>
>>>>>
>>>>> [...]
>>>>>
>>>>>> Eduard Zingerman (12):
>>>>>>     libbpf: Deduplicate unambigous standalone forward declarations
>>>>>>     selftests/bpf: Tests for standalone forward BTF declarations
>>>>>>       deduplication
>>>>>>     libbpf: Support for BTF_DECL_TAG dump in C format
>>>>>>     selftests/bpf: Tests for BTF_DECL_TAG dump in C format
>>>>>>     libbpf: Header guards for selected data structures in vmlinux.h
>>>>>>     selftests/bpf: Tests for header guards printing in BTF dump
>>>>>>     bpftool: Enable header guards generation
>>>>>>     kbuild: Script to infer header guard values for uapi headers
>>>>>>     kbuild: Header guards for types from include/uapi/*.h in kernel BTF
>>>>>>     selftests/bpf: Script to verify uapi headers usage with vmlinux.h
>>>>>>     selftests/bpf: Known good uapi headers for test_uapi_headers.py
>>>>>>     selftests/bpf: script for infer_header_guards.pl testing
>>>>>>
>>>>>>    scripts/infer_header_guards.pl                | 191 +++++
>>>>>>    scripts/link-vmlinux.sh                       |  13 +-
>>>>>>    tools/bpf/bpftool/btf.c                       |   4 +-
>>>>>>    tools/lib/bpf/btf.c                           | 178 ++++-
>>>>>>    tools/lib/bpf/btf.h                           |   7 +-
>>>>>>    tools/lib/bpf/btf_dump.c                      | 232 +++++-
>>>>>>    .../selftests/bpf/good_uapi_headers.txt       | 677 ++++++++++++++++++
>>>>>>    tools/testing/selftests/bpf/prog_tests/btf.c  | 152 ++++
>>>>>>    .../selftests/bpf/prog_tests/btf_dump.c       |  11 +-
>>>>>>    .../bpf/progs/btf_dump_test_case_decl_tag.c   |  39 +
>>>>>>    .../progs/btf_dump_test_case_header_guards.c  |  94 +++
>>>>>>    .../bpf/test_uapi_header_guards_infer.sh      |  33 +
>>>>>>    .../selftests/bpf/test_uapi_headers.py        | 197 +++++
>>>>>>    13 files changed, 1816 insertions(+), 12 deletions(-)
>>>>>>    create mode 100755 scripts/infer_header_guards.pl
>>>>>>    create mode 100644 tools/testing/selftests/bpf/good_uapi_headers.txt
>>>>>>    create mode 100644 tools/testing/selftests/bpf/progs/btf_dump_test_case_decl_tag.c
>>>>>>    create mode 100644 tools/testing/selftests/bpf/progs/btf_dump_test_case_header_guards.c
>>>>>>    create mode 100755 tools/testing/selftests/bpf/test_uapi_header_guards_infer.sh
>>>>>>    create mode 100755 tools/testing/selftests/bpf/test_uapi_headers.py
>>>>>>
>>>>>> --
>>>>>> 2.34.1
>>>>>>