Closed Quuxplusone closed 7 years ago
Quuxplusone
commented
8 years ago | Bugzilla Link | PR26243 |
| Status | RESOLVED FIXED |
| Importance | P normal |
| Reported by | Daniel Borkmann (daniel@iogearbox.net) |
| Reported on | 2016-01-21 13:11:40 -0800 |
| Last modified on | 2017-07-22 23:25:06 -0700 |
| Version | unspecified |
| Hardware | PC Linux |
| CC | alexei.starovoitov@gmail.com, daniel@iogearbox.net, llvm-bugs@lists.llvm.org, rafael@espindo.la, ys114321@gmail.com |
| Fixed by commit(s) | |
| Attachments | |
| Blocks | |
| Blocked by | |
| See also |
Quuxplusone
commented
8 years ago Hmm, thinking more about this, even for that the code should rather generate
two ld_64 instructions. Fixing this up in the loader seems to be fragile as
well, both ldw instructions would need to be replaced by a 2nd ld_64, and
it would kind of require to mess around with registers in the loader code.
So, I think llvm backend needs to be taught if possible to not generate such
relocations (but only map-related ones that were already explicitly present
from the C program).
[...]
ld_64 r1, <MCOperand Expr:(.Lbar.x)>
ldw r2, 0(r1)
ldw r1, 4(r1)
[...]
.section .rodata.cst16,"aM",@progbits,16
.align 4 # @bar.x
.Lbar.x:
.ascii "\357\253\357\253\357\253\357\253\357\313\000\000\000\000\000\377"For this test the following should not be generated:
ld_64 r1, <MCOperand Expr:(.Lbar.x)>
ldw r2, 0(r1)
ldw r1, 4(r1)
I fixed similar bug before and there is a test for it:
test/CodeGen/BPF/sanity.ll foo_printf
is checking that constants are not stored in memory
and instead loaded directly into registers.
This must be some corner case.Btw, the same happens with functions, too.
Simplified test case to demonstrate the issue is below. A workaround
here is to annotate all functions with __attribute__((always_inline)).
I'd see this part here as an enhancement, maybe there's a possibility
in the backend to make everything always inline w/o any annotation in
the program at all? Would be nice if so.
Thanks a lot!
---
#include <iproute2/bpf_api.h>
static int (*data_barrier)(void *key) = (void *) 42;
static inline int bar(struct __sk_buff *skb)
{
int ret = 0;
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
ret += data_barrier(skb);
return ret;
}
static inline int blb(struct __sk_buff *skb)
{
switch (skb->mark) {
case 123:
case 124:
case 222:
bar(skb);
case 223:
return -1;
default:
break;
}
return 0;
}
__section("entry") int foo(struct __sk_buff *skb)
{
int ret;
if (skb->priority == 42)
ret = blb(skb);
else
ret = bar(skb);
return ret;
}
BPF_LICENSE("GPL");
---
# clang -O2 -target bpf -c out.c -o out.o
# readelf -a out.o
ELF Header:
Magic: 7f 45 4c 46 02 01 01 00 00 00 00 00 00 00 00 00
Class: ELF64
Data: 2's complement, little endian
Version: 1 (current)
OS/ABI: UNIX - System V
ABI Version: 0
Type: REL (Relocatable file)
Machine: None
Version: 0x1
Entry point address: 0x0
Start of program headers: 0 (bytes into file)
Start of section headers: 1664 (bytes into file)
Flags: 0x0
Size of this header: 64 (bytes)
Size of program headers: 0 (bytes)
Number of program headers: 0
Size of section headers: 64 (bytes)
Number of section headers: 7
Section header string table index: 1
Section Headers:
[Nr] Name Type Address Offset
Size EntSize Flags Link Info Align
[ 0] NULL 0000000000000000 00000000
0000000000000000 0000000000000000 0 0 0
[ 1] .strtab STRTAB 0000000000000000 00000628
0000000000000051 0000000000000000 0 0 1
[ 2] .text PROGBITS 0000000000000000 00000040
0000000000000450 0000000000000000 AX 0 0 8
[ 3] entry PROGBITS 0000000000000000 00000490
0000000000000098 0000000000000000 AX 0 0 8
[ 4] .relentry REL 0000000000000000 00000608
0000000000000020 0000000000000010 6 3 8
[ 5] license PROGBITS 0000000000000000 00000528
0000000000000004 0000000000000000 WA 0 0 1
[ 6] .symtab SYMTAB 0000000000000000 00000530
00000000000000d8 0000000000000018 1 7 8
Key to Flags:
W (write), A (alloc), X (execute), M (merge), S (strings)
I (info), L (link order), G (group), T (TLS), E (exclude), x (unknown)
O (extra OS processing required) o (OS specific), p (processor specific)
There are no section groups in this file.
There are no program headers in this file.
Relocation section '.relentry' at offset 0x608 contains 2 entries:
Offset Info Type Sym. Value Sym. Name
000000000058 000600000002 unrecognized: 2 0000000000000000 .text
000000000068 000600000002 unrecognized: 2 0000000000000000 .text
The decoding of unwind sections for machine type None is not currently
supported.
Symbol table '.symtab' contains 9 entries:
Num: Value Size Type Bind Vis Ndx Name
0: 0000000000000000 0 NOTYPE LOCAL DEFAULT UND
1: 0000000000000058 0 NOTYPE LOCAL DEFAULT 3 LBB0_4
2: 0000000000000080 0 NOTYPE LOCAL DEFAULT 3 LBB0_5
3: 0000000000000068 0 NOTYPE LOCAL DEFAULT 3 LBB0_6
4: 0000000000000088 0 NOTYPE LOCAL DEFAULT 3 LBB0_7
5: 0000000000000000 0 NOTYPE LOCAL DEFAULT 2 bar
6: 0000000000000000 0 SECTION LOCAL DEFAULT 2
7: 0000000000000000 0 NOTYPE GLOBAL DEFAULT 5 ____license
8: 0000000000000000 0 NOTYPE GLOBAL DEFAULT 3 foo
No version information found in this file.
# tc filter ... bpf da obj ./out.o sec entry
Prog section 'entry' (type:3 insns:19 license:'GPL') rejected: Invalid argument
(22)!
0: (61) r2 = *(u32 *)(r1 +32)
1: (55) if r2 != 0x2a goto pc+11
R1=ctx R2=imm42 R10=fp
2: (18) r6 = 0xffffffff
4: (61) r2 = *(u32 *)(r1 +8)
5: (bf) r3 = r2
6: (07) r3 += -123
7: (b7) r4 = 2
8: (2d) if r4 > r3 goto pc+2
R1=ctx R2=inv R3=inv R4=imm2 R6=inv R10=fp
9: (15) if r2 == 0xdf goto pc+7
R1=ctx R2=inv R3=inv R4=imm2 R6=inv R10=fp
10: (55) if r2 != 0xde goto pc+5
R1=ctx R2=imm222 R3=inv R4=imm2 R6=inv R10=fp
11: (85) call 0
unknown func 0
Error fetching program/map!
Failed to retrieve (e)BPF data!Not inlining 'inline' functions is a different 'problem'. BCC already adds 'always_inline' when it's ok to do so. backend cannot do such things, since it's too late.
Quuxplusone
commented
8 years ago (In reply to comment #4)
> Not inlining 'inline' functions is a different 'problem'. BCC already adds
> 'always_inline' when it's ok to do so. backend cannot do such things, since
> it's too late.
Hmm yeah, that's rather unfortunate. We're not using bcc and 'normal' users
that work with clang directly might have hard time debugging this ... would be
nice if this works out of the box and users don't need to worry about this.
Best one could do right now is probably to point out in the loader to use
__always_inline annotation when such relocs are found. :/> So, Lbar.fmt doesn't seem to be referenced. It seems this is a bug?
How data_barrier is implemented? Looks like "Lbar.fmt" is referenced and that
is why a relocation record is generated?
> Hex dump of section '.rodata':
> 0x00000000 deadbeef c0de ......
I have not seen this. Maybe just because I did not try a lot :-)(In reply to comment #6)
> > So, Lbar.fmt doesn't seem to be referenced. It seems this is a bug?
>
> How data_barrier is implemented? Looks like "Lbar.fmt" is referenced and that
> is why a relocation record is generated?
In the code above it was:
static void (*data_barrier)(void *key) = (void *) 42;
I think main issue here is that there are a couple of ways where such entries
can be generated (another one on the top of my head is when you declare your
format string as 'const' and pass that to trace printk).
The only reloc records allowed in eBPF context would be in relation to maps,
otherwise loaders cannot handle such programs.(In reply to comment #2)
> For this test the following should not be generated:
> ld_64 r1, <MCOperand Expr:(.Lbar.x)>
> ldw r2, 0(r1)
> ldw r1, 4(r1)
>
> I fixed similar bug before and there is a test for it:
> test/CodeGen/BPF/sanity.ll foo_printf
> is checking that constants are not stored in memory
> and instead loaded directly into registers.
> This must be some corner case.
Do you happen to have some progress on this array initialization bug as this
occurs from time to time and we have to use ugly workarounds? Thanks!The sanity.ll test case does not address the read-only section issue. The code
is clean as it is indeed accessing the readonly string from the stack by
copying, but it does not eliminate readonly section.
A simple example:
yhs@ubuntu:~/tmp$ cat bar.c
#include <stdio.h>
int bar() {
char str[] = "aaaaaaaaaaaaa\n";
printf(str);
return -1;
}
yhs@ubuntu:~/tmp$ clang -O2 -target bpf -c bar.c -S -o -
bar.c:4:11: warning: format string is not a string literal (potentially
insecure) [-Wformat-security]
printf(str);
^~~
bar.c:4:11: note: treat the string as an argument to avoid this
printf(str);
^
"%s",
.text
.globl bar
.p2align 3
bar: # @bar
# BB#0:
mov r1, 0
stb -2(r10), r1
mov r1, 2657
sth -4(r10), r1
mov r1, 1633771873
stw -8(r10), r1
ld_64 r1, 7016996765293437281
std -16(r10), r1
mov r1, r10
addi r1, -16
call printf
ld_64 r0, 4294967295
ret
.section .rodata.str1.1,"aMS",@progbits,1
.Lbar.str: # @bar.str
.asciz "aaaaaaaaaaaaa\n"
1 warning generated.
yhs@ubuntu:~/tmp$
The IR dump before pre instruction selection.
*** IR Dump Before Pre-ISel Intrinsic Lowering ***; ModuleID = 'bar.ll'
source_filename = "bar.c"
target datalayout = "e-m:e-p:64:64-i64:64-n32:64-S128"
target triple = "bpf"
@bar.str = private unnamed_addr constant [15 x i8] c"aaaaaaaaaaaaa\0A\00",
align 1
; Function Attrs: nounwind
define i32 @bar() local_unnamed_addr #0 {
%1 = alloca [15 x i8], align 1
%2 = getelementptr inbounds [15 x i8], [15 x i8]* %1, i64 0, i64 0
call void @llvm.lifetime.start(i64 15, i8* %2) #3
call void @llvm.memcpy.p0i8.p0i8.i64(i8* %2, i8* getelementptr inbounds ([15 x i8], [15 x i8]* @bar.str, i64 0, i64 0), i64 15, i32 1, i1 false)
%3 = call i32 (i8*, ...) @printf(i8* %2)
call void @llvm.lifetime.end(i64 15, i8* %2) #3
ret i32 -1
}
Later on, @bar.str is turned into a readonly section string and become global.
(see a similar example at http://llvm.org/releases/3.3/docs/LangRef.html)
Since all these readonly strings become global, LLVM does not try to remove
them at all.
To really fix the issue, interprocedural analysis will be needed to ensure in
the module, there is no reference to @bar.str, and the payoff for non-BPF
program may be small (just a few bytes in readonly section).I am working on this bug now.
The idea is during insn selection DAG2DAG translation, recognizing loads which
read from read-only session and turns those loads into constants.
In BPF backend DAG2DAG translation, a byte array will be constructed when first
access to a readonly section (mostly a ConstantStruct data structure) and later
references to the global readonly section can get data from the constructed
byte array.Fix in 5.0 now.