newtools / ebpf Goto Github PK

@lmb Thoughts?

Hi,
I am trying to learn about eBPF and interested in writing BPF code in C and then call from my Go library. I am 2 weeks into my exploration into this. So, pardon my ignorance. How does it compare to https://github.com/iovisor/gobpf ?

Thanks.

We don't have a way of testing 32bit support (not available on Semaphore) and it's probably not really used anyway. I think we should remove it.

Thoughts @nathanjsweet ?

Seems like only individuals get 20$ in free builds per month. I've contacted Semaphore support to see if there is some option for open source projects.

It's useful to do some basic checks on a Collection if there is a user space component. Basically,
let the user specific a list of MapSpec and make sure that they are present in the collection and have the right types. The same goes for ProgramSpec.

Open question: how to do this for pinned collections? In that case we do not have *Spec to work with.

It's very easy to mess up benchmarks when using Program.Benchmark, since the kernel feeds the output from benchmark run N to run N+1. This means we don't measure the exact same action on each iteration.

Ideally the kernel would fix this, but I'm not sure that is going to happen. I propose that Benchmark() checks that input == output, and raises an error otherwise.

libbcc seems to use Semaphore CI with a custom stage1 to run eBPF unit tests. We can probably do the same.

Describe the bug
Rewriting a value > MaxInt32 truncates it.

Expected behavior
The value should not be truncated.

I think there might be two problems here: we now use MovImm to load rewritten values. This only works with 32bit immediates, which we don't check.

The second problem is that Instruction.Constant is a signed value. Converting signed values in Go has certain sign-preserving semantics. Marshalling to eBPF does a few of these conversions, and I might have gotten them wrong.

being able to quickly close "collection" is needed for error handling at least.

Anyway, we have ebpf.NewCollection()
we should have collection.Close()

@lmb What do you think?

on behalf of @lmb
Make Map and Program structs and keep some of the metadata passed to New* around for better introspection (cmd/ebpf-test would benefit)

on behalf of @lmb
Get rid of *Instructions since it is equivalent to *[]BPFInstruction and pointers to slices usually aren't necessary (need to double check that)

the project seems migrated to https://github.com/cilium/ebpf

Please update README to reflect the current status?

It does not seem possible to atomically modify map values with sync/atomic right now.

If there was a version of Map.Get, maybe called Map.GetIntPtr that returned a pointer to the value atomic updates should be possible right?

Wonderful library btw.

Describe the bug
If I execute Example_socket() from example_sock_test.go the validator reject the ebpf program. I compared the ebpf with that of "Linux/samples/bpf/sock_example.c" and noticed some difference. After modifying the ebpf to match that of sample program from Linux kernel I got the program to work. I suspect this is because of incorrect generated ebpf, don't think it's because of different setup (kernel, ..). But if it work's on you computer it might be.

To Reproduce
If I execute Example_socket() i get:

         0: MovReg dst: r6 src: r1
         1: LdAbsB imm: 25
         2: StXMemW dst: rfp src: r0 off: -4 imm: 0
         3: MovReg dst: r2 src: rfp
         4: SubImm dst: r2 imm: 4
         5: LdImmDW dst: r1 imm: 3
         7: Call MapLookupElement
         8: JEqImm dst: r0 off: -1 imm: 0 <out>
         9: MovImm dst: r1 imm: 1
        10: INVALID
out:
        11: MovImm dst: r0 imm: 0
        12: Exit

panic: invalid operation at position 9

changed xadd 10 instruction to the one from kernel

opcode := asm.OpCode(asm.StXClass) | asm.OpCode(asm.XAddMode).SetSize(asm.DWord)
newXAdd := asm.XAdd(asm.R0, asm.R1)
newXAdd.OpCode = opcode 

Now i got a error about subtraction from stack pointer
changed substraction in addition of negative number as in kernel

         0: MovReg dst: r6 src: r1
         1: LdAbsB imm: 25
         2: StXMemW dst: rfp src: r0 off: -4 imm: 0
         3: MovReg dst: r2 src: rfp
         4: SubImm dst: r2 imm: 4
         5: LdImmDW dst: r1 imm: 3
         7: Call MapLookupElement
         8: JEqImm dst: r0 off: -1 imm: 0 <out>
         9: MovImm dst: r1 imm: 1
        10: StXXAddDW dst: r0 src: r1
out:
        11: MovImm dst: r0 imm: 0
        12: Exit

panic: failed to load program: permission denied: 0: (bf) r6 = r1
1: (30) r0 = *(u8 *)skb[25]
2: (63) *(u32 *)(r10 -4) = r0
3: (bf) r2 = r10
4: (17) r2 -= 4
R2 subtraction from stack pointer prohibited

Now i got error that 5: LdImmDW dst: r1 imm: 3 was not a map_ptr
(3 was file descriptor)

         0: MovReg dst: r6 src: r1
         1: LdAbsB imm: 25
         2: StXMemW dst: rfp src: r0 off: -4 imm: 0
         3: MovReg dst: r2 src: rfp
         4: AddImm dst: r2 imm: -4
         5: LdImmDW dst: r1 imm: 3
         7: Call MapLookupElement
         8: JEqImm dst: r0 off: -1 imm: 0 <out>
         9: MovImm dst: r1 imm: 1
        10: StXXAddDW dst: r0 src: r1
out:
        11: MovImm dst: r0 imm: 0
        12: Exit

panic: failed to load program: permission denied: 0: (bf) r6 = r1
1: (30) r0 = *(u8 *)skb[25]
2: (63) *(u32 *)(r10 -4) = r0
3: (bf) r2 = r10
4: (07) r2 += -4
5: (18) r1 = 0x3
7: (85) call bpf_map_lookup_elem#1
R1 type=inv expected=map_ptr

Linux kernel use separate marco BPF_LD_MAP to load map,
found not much in there documentation about this, but in the source code its defined as:

#ifndef BPF_PSEUDO_MAP_FD
# define BPF_PSEUDO_MAP_FD  1
#endif

/* pseudo BPF_LD_IMM64 insn used to refer to process-local map_fd */
#define BPF_LD_MAP_FD(DST, MAP_FD)              \
    BPF_LD_IMM64_RAW(DST, BPF_PSEUDO_MAP_FD, MAP_FD)

so changed the load with:

loadMap := asm.LoadImm(asm.R1, int64(mapFd), asm.DWord)
loadMap.Src = 1 

Now the programs runs successful, but output is incorrect.
This was because unsafe.Offsetof(ip.Protocol)) was incorrect because
IPHrd has no flags filed

type IPHdr struct {
      VersionIHL uint8
      Tos        uint8                                                                                                                                                                                                                                                           
      Length     uint16
      ID         uint16
      // Flags          uint8
      FragmentOffset uint16
      TTL            uint8
      Protocol       uint8
      Checksum       uint16
      SrcIP          [4]byte
      DestIP         [4]byte
      PayloadLength  int
}

Now it worked for me.

**Linux Information **

• Version : Linux Arch 4.19.12-arch1-1-ARCH #1 SMP PREEMPT Fri Dec 21 13:56:54 UTC 2018 x86_64 GNU/Linux ( had same problems with 4.14 but have not test that extensive)

• Distribution: Arch linux

**Additional **
I like the project, and hope this issue Helps. Hope it was clear.
If you want want, I can send pull request for:

• change opcode for xadd
• add LoadMap instruction ( probaly should also be changed in example_sock_extract_dist_test.go)
• change example_sock_test.go
  • change asm.Sub.Imm(asm.R2, 4), -> asm.Add.Imm(asm.R2, -4),
  • remove IPHdr.Flags

What should ebpf do?
eBPF should expose some general purpose functions for creating kernel tracepoints with bpf

Why should ebpf do this?
The abstraction is powerful and requires minimal code, testing examples, etc will be helpful documentation to folks using this library.

Additional context
It used to be in the library here:
https://github.com/newtools/ebpf/pull/62/files#diff-fafe0b379191c3838559e4573260057aL1
But it was taken out because of no tests.

Given the following C code

#define __section(NAME) __attribute__((section(NAME), used))

char __license[] __section("license") = "MIT";

const unsigned short my_val;

__section("xdp") int xdp_prog() {
	if (my_val < 5) {
		return 1;
	}

	if (~my_val == 0) {
		return 2;
	}

	return my_val;
}

generates the following assembly

Disassembly of section xdp:
xdp_prog:
       0:	18 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 	r1 = 0 ll
       2:	69 11 00 00 00 00 00 00 	r1 = *(u16 *)(r1 + 0)
       3:	b7 00 00 00 01 00 00 00 	r0 = 1
       4:	b7 02 00 00 05 00 00 00 	r2 = 5
       5:	2d 12 01 00 00 00 00 00 	if r2 > r1 goto +1 <LBB0_2>
       6:	bf 10 00 00 00 00 00 00 	r0 = r1
LBB0_2:
       7:	95 00 00 00 00 00 00 00 	exit

Following the 64bit load we have a dereference of the load. In effect, a relocation is always a pointer, and the emitted asm treats it as such. Doing the naive rewriting we have means the code tries to read undefined memory, which is rejected by the verifier.

on behalf of @lmb

Add functions to attach an XDP program to a device (separate package?)

on behalf of @lmb
Make the map instruction replacement mechanism more generic, so that e.g. constants could be swapped in at runtime. (Might be enough to parse more symbols from ELF)

When global arrays are accessed multiple times consecutively, llvm does not re-emit a LOAD, DEREF instruction pair, it reuses the register that was previously LOAD'd into. The current implementation rewrites LOADs to actually load the constant, and the DEREFs to MOV the constant from the register it was LOAD'd into. Multiple access to an array cause the same value (or whatever is left in the original register) to be used.

A fix can be found here: arthurfabre/ebpf@d3b783d

We haven't benchmarked the performance hit of using maps instead of array rewriting yet, but if it's low it may not be worth having to constantly fudge the output of clang - ideally global rewriting would be something that would be supported by clang (eg by generating a single instruction for each access). This could also avoid wasting a register.

I think load mapfd instruction takes two opcodes, while ebpf printout shows only one

from ebpf lib printout

10: op: LdImmDW dst: r1 imm: 5  # r1 = mapfd
        ex-10-1: op: LdImmW dst: r0 imm: 0
11: op: Call MapLookupElement # mapLookupelement(r1=mapfd, r2=fp-4= pointer to value of r0)

From kernel message

        10: (18) r1 = 0x0
        12: (85) call bpf_map_lookup_elem#1

Notice - kernel doesn't show instruction #11.

The approach in #22 works well when parsing a program from an ELF, but it's cumbersome to use when assembling a program using the DSL.

e.g. from TestRewriteUint64:

	ins := Instructions{
		BPFILdImm64(Reg0, 0),
		BPFIOp(Exit),
	}

	spec := &ProgramSpec{
		XDP,
		ins,
		"MIT",
		0,
		map[string][]*BPFInstruction{
			"ret": []*BPFInstruction{ins[0]}, // <---- this part
		},
	}

	spec.RewriteUint64("ret", 42)

	prog, err := NewProgram(spec)
	if err != nil {
		t.Fatal(err)
	}
	defer prog.Close()

For this use case it would be nicer to attach labels / symbols to specific instructions, rather than having to manually create the map[str][]*BPFInstruction. BPFInstruction already has sectionName, but that isn't exposed to users of the API. I also noticed that it's not possible to compare BPFInstruction loaded from an ELF file and the return value of e.g. BPFIDstSrc. I think this is due to *extra being different in these cases. The mechanism to convert Instructions via bpfInstruction and unsafe to a byte array is also a bit hacky imo.

I'd like to propose the following:

• Make BPFInstruction the logical representation of a BPF instruction, meaning it can store full 64bit values without the extra field.
• Add a MarshalBinary function to Instructions which serialises BPFInstruction to []byte
• Make BPFI* functions return a private type asmStatement (open to a better name)
• Add a function func Assemble([]asmStatement) *ProgramSpec
• Add a function func (asmStatement) Symbol(string) asmStatement
• (Optional, but good idea IMO) move assembly related functions to sub-package ebpf/asm and drop BPF* prefix in names

The above code would then become:

	spec := asm.Assemble(&ProgramSpec{
		Type: XDP,
		License: "MIT",
	}, []asm.Instruction{
		asm.LdImm64(Reg0, 0).Reference("ret"),
		asm.Op(Exit),
	})

	// All references to "ret" will be overridden with 42
	spec.RewriteUint64("ret", 42)

	prog, err := NewProgram(spec)
	if err != nil {
		t.Fatal(err)
	}
	defer prog.Close()

As a bonus, we'd be able to compare instructions parsed from ELF and the result of asm.* functions, which is useful in unit tests.

on behalf of @lmb
Make *Spec regular structs instead of interfaces, and make GetELFSections return a "list of specs" (CollectionSpec? not sure). This will allow users to load from ELF, but then tweak the programs if they desire.

Describe the bug
Seems like Semaphore CI changed their platform somehow. There is now a good chance that builds see the following error:

KVM: entry failed, hardware error 0x0
EAX=00000000 EBX=00000000 ECX=00000000 EDX=00000663
ESI=00000000 EDI=00000000 EBP=00000000 ESP=00000000
EIP=0000fff0 EFL=00000002 [-------] CPL=0 II=0 A20=1 SMM=0 HLT=0
ES =0000 00000000 0000ffff 00009300
CS =f000 ffff0000 0000ffff 00009b00
SS =0000 00000000 0000ffff 00009300
DS =0000 00000000 0000ffff 00009300
FS =0000 00000000 0000ffff 00009300
GS =0000 00000000 0000ffff 00009300
LDT=0000 00000000 0000ffff 00008200
TR =0000 00000000 0000ffff 00008b00
GDT=     00000000 0000ffff
IDT=     00000000 0000ffff
CR0=60000010 CR2=00000000 CR3=00000000 CR4=00000000
DR0=0000000000000000 DR1=0000000000000000 DR2=0000000000000000 DR3=0000000000000000 
DR6=00000000ffff0ff0 DR7=0000000000000400
EFER=0000000000000000
Code=00 66 89 d8 66 e8 0c ad ff ff 66 83 c4 0c 66 5b 66 5e 66 c3 <ea> 5b e0 00 f0 30 36 2f 32 33 2f 39 39 00 fc 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Googling only turns up unhelpful Ubuntu bug reports.

I've contacted their support, no luck so far.