Page MenuHomePhabricator

BPF: support type exist/size and enum exist/value relocations
ClosedPublic

Authored by yonghong-song on Wed, Jul 15, 8:04 AM.

Details

Summary

Four new CO-RE relocations are introduced:

  • TYPE_EXISTENCE: whether a typedef/record/enum type exists
  • TYPE_SIZE: the size of a typedef/record/enum type
  • ENUM_VALUE_EXISTENCE: whether an enum value of an enum type exists
  • ENUM_VALUE: the enum value of an enum type

These additional relocations will make CO-RE bpf programs
more adaptive for potential kernel internal data structure
changes.

Diff Detail

Event Timeline

yonghong-song created this revision.Wed, Jul 15, 8:04 AM
ast accepted this revision.Wed, Jul 15, 12:28 PM

lgtm

This revision is now accepted and ready to land.Wed, Jul 15, 12:28 PM
yonghong-song edited the summary of this revision. (Show Details)

using a new relocation TYPE_EXISTENCE for typedef/struct/union types.

yonghong-song retitled this revision from BPF: support CORE existence checking for typedef and struct variables to BPF: support type existence/size and enum value relocations.
yonghong-song edited the summary of this revision. (Show Details)

support additional type size and enum value relocations.

yonghong-song retitled this revision from BPF: support type existence/size and enum value relocations to BPF: support type exist/size and enum exist/value relocations.
yonghong-song edited the summary of this revision. (Show Details)

added support to enum_value_existence relocation and also use different IR builtins for new relocations. This makes code better structured.

anakryiko added inline comments.Thu, Jul 30, 12:24 PM
llvm/lib/Target/BPF/BPFAbstractMemberAccess.cpp
901

shouldn't PatchImm be a 64-bit value?

904

so if I understand this correctly, access string will encode the integer value of ENUM_VAL, is that right? If yes, that's a bit problematic, because it's ambiguous. Multiple ENUM_VALs can have the same integer value, but we actually want to capture which of those values have to be checked/relocated. Is it possible to record enumerator index, i.e.:

for enum ENUM {

ENUM_VAL1 = 123,
ENUM_VAL2 = 123,

}

we record either 0 for ENUM_VAL1 or 1 for ENUM_VAL2, not 123?

yonghong-song added inline comments.Thu, Jul 30, 12:56 PM
llvm/lib/Target/BPF/BPFAbstractMemberAccess.cpp
901

I kind of lazy for this and that is why I have 32bit for now. Yes, we can have 64bit value. Will need to change clang builtin return value to 64bit int and then in llvm we will store 64bit value instead of 32bit.

904

This is a good point. I missed this point. The ENUM_VAL1 information is lost in IR and only available in clang frontend. Will make necessary changes to have enumerator string encoded instead of just int number.

use ld_imm64 for enum value [existence] relocations.
put enumerator name in the relocation access_string to bpf loader can
relocate the value based on enum value name.

for enum value and enum value relocation, put the "index" in debuginfo enumerator list as the access string.

anakryiko accepted this revision.Mon, Aug 3, 4:18 PM

Looks good. I've tested locally all this and it works as expected. Thanks!

This revision was landed with ongoing or failed builds.Tue, Aug 4, 12:36 PM
This revision was automatically updated to reflect the committed changes.