On Thu, Feb 10, 2022 at 2:51 PM Yonghong Song <yhs@xxxxxx> wrote: > > > > On 2/9/22 4:36 PM, Delyan Kratunov wrote: > > As reported in [0], kernel and userspace can sometimes disagree > > on the definition of a typedef (in particular, the size). > > This leads to trouble when userspace maps the memory of a bpf program > > and reads/writes to it assuming a different memory layout. > > I am thinking whether we can do better since this only resolved > some basic types. But it is totally possible some types in vmlinux.h, > who are kernel internal types, happen in some uapi or user header > as well but with different sizes. > > Currently, the exposed bpf program types (in skeleton) are all > from global variables. Since we intend to ensure their size > be equal, and bpf program itself provides the size of the type. > > For example, in bpf program, we have following, > TypeA variable; > > Since TypeA will appear in the skel.h file, user must define it > somehow before skel.h. Let us say TypeA size is 20 from bpf program > BTF type. > > So we could insert a > BUILD_BUG_ON(sizeof(TypeA) != 20) > in the skeleton file to ensure the size match and this applies > to all types. > > In the skel.h file, we can have > #ifndef BUILD_BUG_ON > #define BUILD_BUG_ON ... > #endif > to have BUILD_BUG_ON to cause compilation error if the condition is true. > > User can define BUILD_BUG_ON before skel.h if they want to > override. > > This should apply to all types put in bss/data/rodata sections > by skeleton. > > If this indeed happens as in [0], user can detect the problem > and they may look at vmlinux.h and use proper underlying types > to resolve the issue. > > WDYT? Going back to https://github.com/iovisor/bcc/pull/3777 issue.. The user space part that included skel.h might have used dev_t in other places in the code. When bpftool automagically replaces dev_t in skel.h with int32_t to match the kernel it only moves the problem further into the user space where dev_t would still mismatch. So adding _Static_assert(sizeof(type_in_skel) == const_size_from_kernel); to skel.h would force users to pick types that are the same both in bpf prog and in corresponding user space part. I think that's a better approach. Maybe we don't need to add static_assert for all types, but that's a minor tweak.
