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

[Eduard Zingerman <eddyz87@gmail.com>]

On Tue, 2022-11-01 at 11:35 -0700, Alexei Starovoitov wrote:
> On Tue, Nov 1, 2022 at 9:02 AM Alan Maguire <alan.maguire@oracle.com> wrote:
> > 
> > > > 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
> 
> That would be a great clang feature.
> Thanks Yonghong!
> 
> > > > 
> > > > 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 think it's totally fine to require #include "vmlinux.h" to be last.
> The attr(preserve_access_index) is only useful for kernel internal
> structs. uapi structs don't need it.
> 
> > 
> > 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)
> 
> Sounds like a bunch of complexity for the use case that is not
> clear to me.

Well, my RFC is not shy of complexity :)
What Alan suggests should solve the confilicts described in [1] or any
other confilicts of such kind.

[1] https://lore.kernel.org/bpf/999da51bdf050f155ba299500061b3eb6e0dcd0d.camel@gmail.com/


> > 
> > 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.
> 
> Including vmlinux.h last won't have this enum conflicts, right?
> 
> > 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).
> 
> I think changing the uapi header like in.h is no-go.
> Touching anything in uapi is too much of a risk.