On Wed, Jul 5, 2023 at 9:50 PM Yonghong Song <yhs@xxxxxxxx> wrote:
>
>
>
> On 7/4/23 6:30 AM, Lorenz Bauer wrote:
> > Hi,
> >
> > I think that CO-RE has inconsistent behaviour wrt. BPF_TYPE_ID_LOCAL
> > and BPF_TYPE_ID_TARGET when dealing with qualifiers (modifiers?) Given
> > the following C:
> >
> > enum bpf_type_id_kind {
> > BPF_TYPE_ID_LOCAL = 0, /* BTF type ID in local program */
> > BPF_TYPE_ID_TARGET = 1, /* BTF type ID in target kernel */
> > };
> >
> > int foo(void) {
> > return __builtin_btf_type_id(*(const int *)0, BPF_TYPE_ID_TARGET)
> > != __builtin_btf_type_id(*(const int *)0, BPF_TYPE_ID_LOCAL);
> > }
> >
> > That line with __builtin_btf_type_id is just the expansion of
> > bpf_core_type_id_kernel, etc. clang generates the following BPF:
> >
> > foo:
> > 18 01 00 00 02 00 00 00 00 00 00 00 00 00 00 00 r1 = 0x2 ll
> > 79 11 00 00 00 00 00 00 r1 = *(u64 *)(r1 + 0x0)
> > 18 02 00 00 04 00 00 00 00 00 00 00 00 00 00 00 r2 = 0x4 ll
> > 79 22 00 00 00 00 00 00 r2 = *(u64 *)(r2 + 0x0)
> > b7 03 00 00 00 00 00 00 r3 = 0x0
> > 7b 3a f0 ff 00 00 00 00 *(u64 *)(r10 - 0x10) = r3
> > b7 03 00 00 01 00 00 00 r3 = 0x1
> > 7b 3a f8 ff 00 00 00 00 *(u64 *)(r10 - 0x8) = r3
> > 5d 21 02 00 00 00 00 00 if r1 != r2 goto +0x2 <LBB0_2>
> > 79 a1 f0 ff 00 00 00 00 r1 = *(u64 *)(r10 - 0x10)
> > 7b 1a f8 ff 00 00 00 00 *(u64 *)(r10 - 0x8) = r1
> > LBB0_2:
> > 79 a0 f8 ff 00 00 00 00 r0 = *(u64 *)(r10 - 0x8)
> > 95 00 00 00 00 00 00 00 exit
> >
> > Link to godbolt: https://godbolt.org/z/jr63hKz9E (contains version info)
> >
> > Note that the first two ldimm64 have distinct type IDs. I added some
> > debug logging to cilium/ebpf and found that the compiler indeed also
> > emits distinct CO-RE relocations:
> >
> > foo {InsnOff:0 TypeID:2 AccessStrOff:69 Kind:local_type_id}
> > foo {InsnOff:2 TypeID:4 AccessStrOff:69 Kind:target_type_id}
> >
> > It seems that for BPF_TYPE_ID_TARGET the outer const is peeled, while
> > this doesn't happen for the local variant.
> >
> > CORERelocation(local_type_id, Const[0], local_id=4) local_type_id=4->4
> > CORERelocation(target_type_id, Int:"int"[0], local_id=2) target_type_id=2->2
> >
> > Similar behaviour exists for BPF_TYPE_EXISTS, probably others.
> >
> > The behaviour goes away if I drop the pointer casting magic:
> >
> > __builtin_btf_type_id((const int)0, BPF_TYPE_ID_TARGET) !=
> > __builtin_btf_type_id((const int)0, BPF_TYPE_ID_LOCAL)
> >
> > Intuitively I'd say that the root cause is that dereferencing the
> > pointer drops the constness of the type. Why does TARGET behave
> > differently than LOCAL though?
>
> Thanks for reporting. The difference of type w.r.t. 'const' modifier
> is a deliberate choice in llvm:
>
> See
> https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/BPF/BPFPreserveDIType.cpp#L84-L103
>
> if (FlagValue == BPFCoreSharedInfo::BTF_TYPE_ID_LOCAL_RELOC) {
> Reloc = BPFCoreSharedInfo::BTF_TYPE_ID_LOCAL;
> } else {
> Reloc = BPFCoreSharedInfo::BTF_TYPE_ID_REMOTE;
> DIType *Ty = cast<DIType>(MD);
> while (auto *DTy = dyn_cast<DIDerivedType>(Ty)) {
> unsigned Tag = DTy->getTag();
> if (Tag != dwarf::DW_TAG_const_type &&
> Tag != dwarf::DW_TAG_volatile_type)
> break;
> Ty = DTy->getBaseType();
> }
>
> if (Ty->getName().empty()) {
> if (isa<DISubroutineType>(Ty))
> report_fatal_error(
> "SubroutineType not supported for BTF_TYPE_ID_REMOTE reloc");
> else
> report_fatal_error("Empty type name for BTF_TYPE_ID_REMOTE
> reloc");
> }
> MD = Ty;
> }
>
> Basically, the BTF_TYPE_ID_REMOTE (the kernel term BPF_TYPE_ID_TARGET)
> needs further checking to prevent some invalid cases.
> Also for kernel type matching, it would be good to eliminate modifiers
> otherwise, there could be many instances of 'const' which makes
> kernel matching is more complicated.
>
> But I see your point. Maybe we should preserve the original type
> for BTF_TYPE_ID_TARGET as well. Will check what libbpf/kernel
> will handle 'const int *' case and get back to this thread later.
I think it's better the other way around: make BTF_TYPE_ID_LOCAL strip
const/volatile/restrict modifiers. For all other relocations we rely
on having named types, so const/volatile makes no sense and will fail
relocation. It's hard to come up with the situation where recording
const/volatile/restrict in BTF_TYPE_ID_LOCAL would make sense, so I'd
say that it should behave just like all the other relos.
>
> >
> > Cheers
> > Lorenz
