[PATCH] D81859: [RFC BPF] Prevent Speculative Code Motion

[Yonghong Song via Phabricator via llvm-commits]

yonghong-song created this revision.
yonghong-song added reviewers: lebedev.ri, spatel, shawnl, ast.
Herald added subscribers: llvm-commits, hiraditya.
Herald added a project: LLVM.

This RFC patch tries to prevent speculative code motion for BPF.
We may need a BPF target option to control this, but this can 
be added later. Here, I am more concerned how to address
a SimplifyCFG.cpp change (w.r.t. SpeculateOneExpensiveInst)
and intends to seek advice about the best way to address it.

First, some use cases on why we want to prevent speculative
code motion.

Use case 1: bpf map value or some other data with a range.

  data = bpf_map_lookup_elem(&map, &key);
  if (!data) return 0;
  payload = data->payload;
  len = bpf_probe_read_kernel_str(payload, 16, &task->comm);
  if (len <= 16) 
    payload += len;
  ... 

The compiler may generate code like:

  data = bpf_map_lookup_elem(&map, &key);
  if (!data) return 0;
  payload = data->payload;
  len = bpf_probe_read_kernel_str(payload, 16, &task->comm);
  new_payload = payload + len;
  if (len > 16) 
    new_payload = payload
  ... 

The "payload + len" may cause kernel verifier failure as
the "len" can be anything at this moment.

Use case 2: CO-RE relocatons

  field_exist = ... 
  if (field_exist) {
    offset = non-memory-access-builtin-expr1;
  } else {
    offset = non-memory-access-builtin-expr2;
  }   
  use "offset" to read kernel memory

The compiler may generate code like:

  field_exist = ... 
  offset = non-memory-access-builtin-expr1;
  if (!field_exist)
    offset = non-memory-access-builtin-expr2;
  use "offset" to read kernel memory

This may cause failures since if field_exist is false and 
libbpf is not able to perform relation
for "offset = non-memory-access-builtin-expr1".
The instruction itself will be rewritten as
an illegal instruction and this will cause
program load failures.

To address the above issues, people use

  . inline assembly
  . artificial complex control flow

to prevent the above optimizations.

This patch tries to do with a more user friendly way.

First BPF backend TargetTransformInfo implements getUserCost()
to return UserCost INT_MAX to prevent compiler moving codes
speculatively.

Second, in SimplifyCFG, we have SpeculateOneExpensiveInst, true
by default, regardless of UserCost, still allows speculative
code movement for one instruction. The comment mentions if there
is no further transformation, this optimization will be undone
in PrepareCodeGen. Unfortunately, the code is indeed further
transformed and later on it is not undone in my example.
I illustrated my point in this patch by simply changing its 
default value to false, and of course it is not correct.
One possible way is implement a backend hook in TTI.
Or we may be able to flip it in clang frontend? Typically
BPF compilation only involves "clang" so using separate
"opt" to control this is not desirable.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D81859

Files:
  llvm/lib/Target/BPF/BPFTargetMachine.cpp
  llvm/lib/Target/BPF/BPFTargetMachine.h
  llvm/lib/Target/BPF/BPFTargetTransformInfo.h
  llvm/lib/Transforms/Utils/SimplifyCFG.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D81859.270806.patch
Type: text/x-patch
Size: 3944 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200615/d3e273b3/attachment.bin>