[BPF] Warn about direct access to CO-RE bitfields

[eddyz87]

Summary

Print a diagnostic warning when bitfields are accessed directly in
structures that are marked with preserve_access_index attribute. Such
bitfields should be accessed using BPF_CORE_READ_BITFIELD and
BPF_CORE_READ_BITFIELD_PROBED macro.

These macro generate a specific pattern:

struct foo {
  int field:2;
} *ctx;

BPF_CORE_READ_BITFIELD(ctx, field)

is expanded as:

({ const void *p = (void*) ctx + __builtin_preserve_field_info(...);
   unsigned long long val;
   ... val = *(<cast>)p; ...
   val;
})

Note that ptr->field is not referenced, instead an address is
computed starting from ctx. This commit exploits this fact and simply
warns about usage of ptr->field in load instructions.

[eddyz87]

Updated testcase to avoid duplication with Kernel source code

clang/test/CodeGen/bpf-preserve-attr-index-bitfield-warn.c

+// Check that diagnostic message is printed when bitfields in a
+// structure marked with preserve_access_index are accessed directly.
+
+// REQUIRES: bpf-registered-target
+// RUN: %clang -target bpf -g -S -o /dev/null 2>&1 %s \
+// RUN:        | FileCheck %s
+
+// The below macro definitions are made to be similar to
+// tools/lib/bpf/bpf_core_read.h in the Linux Kernel tree
+// (but not identical to avoid licensing issues).
+
+extern void mutate(void* what, unsigned info);
+extern int some_int(unsigned info);
+
+enum info {
+  OFFSET = 0,
+  SIZE   = 1,
+  EXISTS = 2,
+  SIGNED = 3,
+  LSHIFT = 4,
+  RSHIFT = 5,
+};
+
+#define INFO(field, info) __builtin_preserve_field_info(field, info)
+
+#define BPF_CORE_READ_BITFIELD_PROBED(s, field) ({                \
+      unsigned long long val = 0;                                 \
+      mutate(&val, INFO((s)->field, SIZE));                       \
+      val <<= some_int(INFO((s)->field, LSHIFT));                 \
+      val >>  some_int(INFO((s)->field, RSHIFT));                 \
+      val; })
+
+#define BPF_CORE_READ_BITFIELD(s, field) ({                       \
+      const void *p = (const void *)s + INFO((s)->field, OFFSET); \
+      unsigned long long val;                                     \
+      val = *(const unsigned long long *)p;                       \
+      val <<= some_int(INFO((s)->field, LSHIFT));                 \
+      val >>  some_int(INFO((s)->field, RSHIFT));                 \
+      val; })
+
+struct plain_struct {
+  int a:1;
+  int b;
+  int c:1;
+  int d:1;
+} __attribute__((preserve_access_index));
+
+typedef struct plain_struct plain_struct_alias;
+
+struct plain_union {
+  int a:1;
+  int b;
+} __attribute__((preserve_access_index));
+
+struct anon_struct {
+  struct {
+    int a:1;
+    int b;
+  };
+} __attribute__((preserve_access_index));
+
+struct anon_union {
+  union {
+    int a:1;
+    int b;
+  };
+} __attribute__((preserve_access_index));
+
+struct nested_struct {
+  struct plain_struct ps;
+  struct plain_struct *ps2;
+} __attribute__((preserve_access_index));
+
+struct plain_struct_no_pai {
+  int aa:1;
+  int bb;
+};
+
+struct nested_struct2 {
+  struct plain_struct_no_pai ps;
+} __attribute__((preserve_access_index));
+
+extern void consume_int(int);
+
+void plain_struct_fn(struct plain_struct *ctx) {
+  consume_int(ctx->a); consume_int(ctx->c);
+// CHECK: {{.*}}.c:[[#@LINE-1]]:{{.*}}: warning: Direct access to a bitfield 'a'
+// CHECK: {{.*}}.c:[[#@LINE-2]]:{{.*}}: warning: Direct access to a bitfield 'c'
+  consume_int(ctx->b);
+// CHECK-NOT: {{.*}} 'b'
+  consume_int(ctx->d);
+// CHECK: {{.*}}.c:[[#@LINE-1]]:{{.*}}: warning: Direct access to a bitfield 'd'
+  consume_int(BPF_CORE_READ_BITFIELD(ctx, a));
+  consume_int(BPF_CORE_READ_BITFIELD_PROBED(ctx, a));
+// CHECK-NOT: {{.*}} 'a'
+}
+
+void plain_struct_alias_fn(plain_struct_alias *ctx) {
+  consume_int(ctx->a);
+// CHECK: {{.*}}.c:[[#@LINE-1]]:{{.*}}: warning: Direct access to a bitfield 'a'
+  consume_int(ctx->b);
+// CHECK-NOT: {{.*}} 'b'
+  consume_int(BPF_CORE_READ_BITFIELD(ctx, a));
+  consume_int(BPF_CORE_READ_BITFIELD_PROBED(ctx, a));
+// CHECK-NOT: {{.*}} 'a'
+}
+
+void plain_union_fn(struct plain_union *ctx) {
+  consume_int(ctx->a);
+// CHECK: {{.*}}.c:[[#@LINE-1]]:{{.*}}: warning: Direct access to a bitfield 'a'
+  consume_int(ctx->b);
+// CHECK-NOT: {{.*}} 'b'
+  consume_int(BPF_CORE_READ_BITFIELD(ctx, a));
+  consume_int(BPF_CORE_READ_BITFIELD_PROBED(ctx, a));
+// CHECK-NOT: {{.*}} 'a'
+}
+
+void anon_struct_fn(struct anon_struct *ctx) {
+  consume_int(ctx->a);
+// CHECK: {{.*}}.c:[[#@LINE-1]]:{{.*}}: warning: Direct access to a bitfield 'a'
+  consume_int(ctx->b);
+// CHECK-NOT: {{.*}} 'b'
+  consume_int(BPF_CORE_READ_BITFIELD(ctx, a));
+  consume_int(BPF_CORE_READ_BITFIELD_PROBED(ctx, a));
+// CHECK-NOT: {{.*}} 'a'
+}
+
+void anon_union_fn(struct anon_union *ctx) {
+  consume_int(ctx->a);
+// CHECK: {{.*}}.c:[[#@LINE-1]]:{{.*}}: warning: Direct access to a bitfield 'a'
+  consume_int(ctx->b);
+// CHECK-NOT: {{.*}} 'b'
+  consume_int(BPF_CORE_READ_BITFIELD(ctx, a));
+  consume_int(BPF_CORE_READ_BITFIELD_PROBED(ctx, a));
+// CHECK-NOT: {{.*}} 'a'
+}
+
+void nested_struct_fn(struct nested_struct *ctx) {
+  consume_int(ctx->ps.b);
+// CHECK-NOT: {{.*}} 'b'
+  consume_int(BPF_CORE_READ_BITFIELD(&ctx->ps, a));
+  consume_int(BPF_CORE_READ_BITFIELD_PROBED(&ctx->ps, a));
+// CHECK-NOT: {{.*}} 'a'
+  consume_int(ctx->ps.a);
+// CHECK: {{.*}}.c:[[#@LINE-1]]:{{.*}}: warning: Direct access to a bitfield 'a'
+
+  consume_int(ctx->ps2->a);
+// CHECK: {{.*}}.c:[[#@LINE-1]]:{{.*}}: warning: Direct access to a bitfield 'a'
+  consume_int(ctx->ps2->b);
+// CHECK-NOT: {{.*}} 'b'
+  consume_int(BPF_CORE_READ_BITFIELD(ctx->ps2, a));
+  consume_int(BPF_CORE_READ_BITFIELD_PROBED(ctx->ps2, a));
+// CHECK-NOT: {{.*}} 'a'
+}
+
+void nested_struct_fn2(struct nested_struct2 *ctx) {
+  consume_int(ctx->ps.aa);
+// CHECK-NOT: {{.*}} 'aa'
+  consume_int(ctx->ps.bb);
+// CHECK-NOT: {{.*}} 'bb'
+}

llvm/lib/Target/BPF/BPFAbstractMemberAccess.cpp

 //
 //===----------------------------------------------------------------------===//
 
 #include "BPF.h"
 #include "BPFCORE.h"
 #include "BPFTargetMachine.h"
 #include "llvm/BinaryFormat/Dwarf.h"
 #include "llvm/IR/DebugInfoMetadata.h"
+#include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/GlobalVariable.h"
 #include "llvm/IR/Instruction.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/IntrinsicsBPF.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/PassManager.h"
 #include "llvm/IR/Type.h"
 #include "llvm/IR/User.h"
 #include "llvm/IR/Value.h"
 #include "llvm/Pass.h"
 #include "llvm/Transforms/Utils/BasicBlockUtils.h"
+#include <queue>
 #include <stack>
 
 #define DEBUG_TYPE "bpf-abstract-member-access"
 
 namespace llvm {
 constexpr StringRef BPFCoreSharedInfo::AmaAttr;
 uint32_t BPFCoreSharedInfo::SeqNum;
 
uint32_t GetFieldInfo(uint32_t InfoKind, DICompositeType *CTy,
                         MaybeAlign RecordAlignment);
 
   Value *computeBaseAndAccessKey(CallInst *Call, CallInfo &CInfo,
                                  std::string &AccessKey, MDNode *&BaseMeta);
   MDNode *computeAccessKey(CallInst *Call, CallInfo &CInfo,
                            std::string &AccessKey, bool &IsInt32Ret);
   uint64_t getConstant(const Value *IndexValue);
   bool transformGEPChain(CallInst *Call, CallInfo &CInfo);
+  void doSemanticChecks();
+  DIDerivedType *isBitFieldAccess(CallInfo &CInfo);
 };
 
 std::map<std::string, GlobalVariable *> BPFAbstractMemberAccess::GEPGlobals;
 
 class BPFAbstractMemberAccessLegacyPass final : public FunctionPass {
   BPFTargetMachine *TM;
 
   bool runOnFunction(Function &F) override {
     return BPFAbstractMemberAccess(TM).run(F);
   }
 
 public:
   static char ID;
 
   // Add optional BPFTargetMachine parameter so that BPF backend can add the
   // phase with target machine to find out the endianness. The default
   // constructor (without parameters) is used by the pass manager for managing
   // purposes.
   BPFAbstractMemberAccessLegacyPass(BPFTargetMachine *TM = nullptr)
       : FunctionPass(ID), TM(TM) {}
 };
 
+// Base class for diagnostic messages output by
+// BPFAbstractMemberAccess::doSemanticChecks. In order to report these
+// messages in the source location order these have to be deferred.
+struct DiagnosticMessage {
+  const DiagnosticLocation Loc;
+  DiagnosticMessage(const DiagnosticLocation &Loc) : Loc(Loc) {}
+  virtual void report(LLVMContext &C) = 0;
+  virtual ~DiagnosticMessage() {}
+};
+
+struct SortMsgByLineNumber {
+  bool operator()(const std::unique_ptr<DiagnosticMessage> &A,
+                  const std::unique_ptr<DiagnosticMessage> &B) {
+
+    auto &LocA = A->Loc;
+    auto &LocB = B->Loc;
+    // Currently reported for a single function, thus skipping
+    // file name comparison.
+    return LocA.getLine() > LocB.getLine() ||
+      (LocA.getLine() == LocB.getLine() &&
+       LocA.getColumn() > LocB.getColumn());
+  }
+};
+
+// A queue of diagnostic messages sorted by location.
+using DiagnosticMessageQueue = std::priority_queue
+  <std::unique_ptr<DiagnosticMessage>,
+   std::vector<std::unique_ptr<DiagnosticMessage>>,
+   SortMsgByLineNumber>;
+
 } // End anonymous namespace
 
 char BPFAbstractMemberAccessLegacyPass::ID = 0;
 INITIALIZE_PASS(BPFAbstractMemberAccessLegacyPass, DEBUG_TYPE,
                 "BPF Abstract Member Access", false, false)
 
 FunctionPass *llvm::createBPFAbstractMemberAccess(BPFTargetMachine *TM) {
   return new BPFAbstractMemberAccessLegacyPass(TM);
bool BPFAbstractMemberAccess::transformGEPChain(CallInst *Call,
   Instruction *PassThroughInst =
       BPFCoreSharedInfo::insertPassThrough(M, BB, BCInst2, Call);
   Call->replaceAllUsesWith(PassThroughInst);
   Call->eraseFromParent();
 
   return true;
 }
 
+namespace {
+
+class DirectBitfieldAccessMessage : public DiagnosticMessage {
+  DIDerivedType *DTy;
+  Instruction *User;
+public:
+  DirectBitfieldAccessMessage(DIDerivedType *DTy, Instruction *User)
+    : DiagnosticMessage(User->getDebugLoc()), DTy(DTy), User(User) {}
+  void report(LLVMContext &C) override {
+    auto Msg = DiagnosticInfoUnsupported
+     (*User->getFunction(),
+      Twine("Direct access to a bitfield ")
+      .concat("'").concat(DTy->getName()).concat("'")
+      .concat(" of a structure with preserve_access_index attribute "
+              "is not supported, consider using BPF_CORE_READ_BITFIELD "
+              "or BPF_CORE_READ_BITFIELD_PROBED macro, see bpf_core_read.h"),
+      User->getDebugLoc(),
+      DS_Warning);
+    C.diagnose(Msg);
+  }
+};
+
+} // Anonymous namespace
+
+DIDerivedType *BPFAbstractMemberAccess::isBitFieldAccess(CallInfo &CI) {
+    if (CI.Kind != BPFPreserveStructAI)
+      return nullptr;
+    if (!CI.Metadata)
+      return nullptr;
+    auto *DTy = dyn_cast<DIType>(CI.Metadata);
+    if (!DTy)
+      return nullptr;
+    auto *CTy = dyn_cast<DICompositeType>(stripQualifiers(DTy));
+    if (!CTy)
+      return nullptr;
+    assert(CI.AccessIndex < CTy->getElements().size() &&
+           "isBitFieldAccess: Out of bounds access to CTy->getElements()");
+    auto *Elem = CTy->getElements()[CI.AccessIndex];
+    auto *ElemTy = dyn_cast<DIDerivedType>(Elem);
+    if (ElemTy && ElemTy->getFlags() & DINode::DIFlags::FlagBitField)
+      return ElemTy;
+    return nullptr;
+}
+
+// So far only one semantic check: bitfield usage.
+void BPFAbstractMemberAccess::doSemanticChecks() {
+  // Iteration over BaseAICalls not necessarily follows source code
+  // order, so use a priority queue sorting by location to defer the
+  // diagnostic messages.
+  DiagnosticMessageQueue MsgQueue;
+  auto Enqueue= [&](DiagnosticMessage *Msg) {
+    MsgQueue.push(std::unique_ptr<DiagnosticMessage>(Msg));
+  };
+  for (auto &[Call, CInfo] : BaseAICalls) {
+    // Verify that direct LoadInst is not used for CORE bitfields.
+    // CORE bitfields should be accessed using BPF_CORE_READ_BITFIELD
+    // and BPF_CORE_READ_BITFIELD_PROBED macro.
+    // These macro generate a specific pattern (for a bitfield ctx->field):
+    //
+    //   BPF_CORE_READ_BITFIELD(ctx, field) is expanded as:
+    //
+    // ({ const void *p = (void*) ctx + __builtin_preserve_field_info(...);
+    //    unsigned long long val;
+    //    ... val = *(<cast>)p; ...
+    //    val;
+    // })
+    //
+    // Note that ptr->field is not referenced, instead an address is
+    // computed starting from ctx.
+    //
+    // BPF_CORE_READ_BITFIELD_PROBED is a bit different but similar.
+    if (auto *ElemTy = isBitFieldAccess(CInfo))
+      for (auto *User: Call->users())
+        if (isa<LoadInst>(User))
+          Enqueue(new DirectBitfieldAccessMessage
+                  (ElemTy, cast<Instruction>(User)));
+  }
+  auto &C = M->getContext();
+  while (!MsgQueue.empty()) {
+    MsgQueue.top()->report(C);
+    MsgQueue.pop();
+  }
+}
+
 bool BPFAbstractMemberAccess::doTransformation(Function &F) {
   bool Transformed = false;
 
   // Collect PreserveDIAccessIndex Intrinsic call chains.
   // The call chains will be used to generate the access
   // patterns similar to GEP.
   collectAICallChains(F);
 
+  doSemanticChecks();
+
   for (auto &C : BaseAICalls)
     Transformed = transformGEPChain(C.first, C.second) || Transformed;
 
   return removePreserveAccessIndexIntrinsic(F) || Transformed;
 }
 
 PreservedAnalyses
 BPFAbstractMemberAccessPass::run(Function &F, FunctionAnalysisManager &AM) {
   return BPFAbstractMemberAccess(TM).run(F) ? PreservedAnalyses::none()
                                             : PreservedAnalyses::all();
 }