Use bpf2bpf to simplify libpcap ebpf injection

By using bpf2bpf:

1. We don't have to search for registers holding skb->data and
   skb-data_end, because they must be at R4 and R5 as arguments.
2. We no longer need a bpf_printk() to be replaced, so pwru developers
   can use printk for debugging again.
3. We don't need to assume clang behavior or rely on specific clang
   version.
4. We can delete a lot of code to simplify logic.

To achieve it, we mustn't strip bpf objects because we need symbols to
find `filter_pcap_ebpf`.

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>

bpf/kprobe_pwru.c

@@ -2,14 +2,6 @@
 /* Copyright Martynas Pumputis */
 /* Copyright Authors of Cilium */
 
-/*
- * WARNING: `bpf_printk()` has special intention in this program: it is used for
- * pcap-filter ebpf injection, please see the comment in the `filter_pcap()`. So
- * if you want to add additional `bpf_printk()` for debugging, it is likely to
- * break the injection and fail the bpf verifier. In this case, it is
- * recommended to use perf_output for debugging.
- */
-
 #include "vmlinux.h"
 #include "bpf/bpf_helpers.h"
 #include "bpf/bpf_core_read.h"
@@ -141,6 +133,12 @@ filter_meta(struct sk_buff *skb) {
 	return true;
 }
 
+static __noinline bool
+filter_pcap_ebpf(void *_skb, void *__skb, void *___skb, void *data, void* data_end)
+{
+	return data != data_end && _skb == __skb && __skb == ___skb;
+}
+
 static __always_inline bool
 filter_pcap(struct sk_buff *skb) {
 	BPF_CORE_READ(skb, head);
@@ -150,19 +148,7 @@ filter_pcap(struct sk_buff *skb) {
 	u16 l4_off = BPF_CORE_READ(skb, transport_header);
 	u16 len = BPF_CORE_READ(skb, len);
 	void *data_end = skb_head + l4_off + len;
-	/*
-	 * The next two lines of code won't be executed; they will be replaced
-	 * by ebpf instructions compiled from pcap-filter expression before
-	 * loading to kernel.
-	 * However they are important placeholders to:
-	 * 1. let us know the registers holding data and data_end, which are
-	 * needed to convert cbpf to ebpf;
-	 * 2. leave r0, r1, r2, r3, r4 available for pcap filter ebpf
-	 * instructions;
-	 * 3. mark the position to inject pcap filter ebpf instructions;
-	 */
-	bpf_printk("%d %d", data, data_end);
-	return data != data_end;
+	return filter_pcap_ebpf((void *)skb, (void *)skb, (void *)skb, data, data_end);
 }
 
 static __always_inline bool

build.go

@@ -2,10 +2,10 @@
 // Copyright (C) 2021 Authors of Cilium */
 
 //go:generate sh -c "echo Generating for $TARGET_GOARCH"
-//go:generate go run github.com/cilium/ebpf/cmd/bpf2go -target $TARGET_GOARCH -cc clang KProbePWRU ./bpf/kprobe_pwru.c -- -DOUTPUT_SKB -I./bpf/headers -Wno-address-of-packed-member
-//go:generate go run github.com/cilium/ebpf/cmd/bpf2go -target $TARGET_GOARCH -cc clang KProbeMultiPWRU ./bpf/kprobe_pwru.c -- -DOUTPUT_SKB -DHAS_KPROBE_MULTI -I./bpf/headers -Wno-address-of-packed-member
-//go:generate go run github.com/cilium/ebpf/cmd/bpf2go -target $TARGET_GOARCH -cc clang KProbePWRUWithoutOutputSKB ./bpf/kprobe_pwru.c -- -I./bpf/headers -Wno-address-of-packed-member
-//go:generate go run github.com/cilium/ebpf/cmd/bpf2go -target $TARGET_GOARCH -cc clang KProbeMultiPWRUWithoutOutputSKB ./bpf/kprobe_pwru.c -- -D HAS_KPROBE_MULTI -I./bpf/headers -Wno-address-of-packed-member
+//go:generate go run github.com/cilium/ebpf/cmd/bpf2go -target $TARGET_GOARCH -cc clang -no-strip KProbePWRU ./bpf/kprobe_pwru.c -- -DOUTPUT_SKB -I./bpf/headers -Wno-address-of-packed-member
+//go:generate go run github.com/cilium/ebpf/cmd/bpf2go -target $TARGET_GOARCH -cc clang -no-strip KProbeMultiPWRU ./bpf/kprobe_pwru.c -- -DOUTPUT_SKB -DHAS_KPROBE_MULTI -I./bpf/headers -Wno-address-of-packed-member
+//go:generate go run github.com/cilium/ebpf/cmd/bpf2go -target $TARGET_GOARCH -cc clang -no-strip KProbePWRUWithoutOutputSKB ./bpf/kprobe_pwru.c -- -I./bpf/headers -Wno-address-of-packed-member
+//go:generate go run github.com/cilium/ebpf/cmd/bpf2go -target $TARGET_GOARCH -cc clang -no-strip KProbeMultiPWRUWithoutOutputSKB ./bpf/kprobe_pwru.c -- -D HAS_KPROBE_MULTI -I./bpf/headers -Wno-address-of-packed-member
 //go:generate go run ./tools/getgetter.go -struct ^(KProbePWRU|KProbeMultiPWRU|KProbePWRUWithoutOutputSKB|KProbeMultiPWRUWithoutOutputSKB)(Programs|Maps)$
 
 package main

internal/libpcap/compile.go

@@ -34,9 +34,7 @@ func CompileCbpf(expr string) (insts []bpf.Instruction, err error) {
 		DLT_RAW linktype tells pcap_compile() to generate cbpf instructions for
 		skb without link layer. This is because kernel doesn't supply L2 data
 		for many of functions, where skb->mac_len == 0, while the default
-		pcap_compile mode only works for a complete frame data, so we have to
-		specify this linktype to tell pcap that the data starts from L3 network
-		header.
+		pcap_compile mode only works for a complete frame data.
 	*/
 	pcap := C.pcap_open_dead(C.DLT_RAW, MAXIMUM_SNAPLEN)
 	if pcap == nil {
@@ -91,23 +89,21 @@ func CompileEbpf(expr string, opts cbpfc.EBPFOpts) (insts asm.Instructions, err
 
 /*
 We have to adjust the ebpf instructions because verifier prevents us from
-directly loading data from memory. For example, the instruction "r0 = *(u8 *)(r9 +0)"
-will break verifier with error "R9 invalid mem access 'scalar", we therefore
+directly loading data from memory. For example, the instruction "r0 = *(u8 *)(r4 +0)"
+will break verifier with error "R4 invalid mem access 'scalar", we therefore
 need to convert this direct memory load to bpf_probe_read_kernel function call:
 
 - r1 = r10  // r10 is stack top
 - r1 += -8  // r1 = r10-8
 - r2 = 1    // r2 = sizeof(u8)
-- r3 = r9   // r9 is start of packet data, aka L3 header
-- r3 += 0   // r3 = r9+0
-- call bpf_probe_read_kernel  // *(r10-8) = *(u8 *)(r9+0)
+- r3 = r4   // r4 is start of packet data, aka L3 header
+- r3 += 0   // r3 = r4+0
+- call bpf_probe_read_kernel  // *(r10-8) = *(u8 *)(r4+0)
 - r0 = *(u8 *)(r10 -8)  // r0 = *(r10-8)
 
 To safely borrow R1, R2 and R3 for setting up the arguments for
 bpf_probe_read_kernel(), we need to save the original values of R1, R2 and R3
 on stack, and restore them after the function call.
-
-More details in the comments below.
 */
 func adjustEbpf(insts asm.Instructions, opts cbpfc.EBPFOpts) (newInsts asm.Instructions, err error) {
 	replaceIdx := []int{}
@@ -147,6 +143,9 @@ func adjustEbpf(insts asm.Instructions, opts cbpfc.EBPFOpts) (newInsts asm.Instr
 
 				// inst.Dst = *(RFP-8)
 				asm.LoadMem(inst.Dst, asm.RFP, -8, inst.OpCode.Size()),
+
+				asm.LoadMem(asm.R4, asm.RFP, -40, asm.DWord),
+				asm.LoadMem(asm.R5, asm.RFP, -48, asm.DWord),
 			)
 
 			/*
@@ -181,21 +180,17 @@ func adjustEbpf(insts asm.Instructions, opts cbpfc.EBPFOpts) (newInsts asm.Instr
 		insts = append(insts[:idx], append(replaceInsts[idx], insts[idx+1:]...)...)
 	}
 
-	/*
-	 Prepend instructions to init R1, R2, R3 so as to avoid verifier error:
-	 permission denied: *(u64 *)(r10 -24) = r2: R2 !read_ok
-	*/
 	insts = append([]asm.Instruction{
-		asm.Mov.Imm(asm.R1, 0),
-		asm.Mov.Imm(asm.R2, 0),
-		asm.Mov.Imm(asm.R3, 0),
+		asm.StoreMem(asm.RFP, -40, asm.R4, asm.DWord),
+		asm.StoreMem(asm.RFP, -48, asm.R5, asm.DWord),
 	}, insts...)
 
 	insts = append(insts,
-		asm.Mov.Imm(asm.R0, 0).WithSymbol("result"), // r0 = 0
-		asm.Mov.Reg(opts.PacketStart, opts.Result),  // skb->data = $result
-		asm.Mov.Imm(opts.PacketEnd, 0),              // skb->data_end = 0
-
+		asm.Mov.Imm(asm.R1, 0).WithSymbol("result"), // r1 = 0 (_skb)
+		asm.Mov.Imm(asm.R2, 0),                      // r2 = 0 (__skb)
+		asm.Mov.Imm(asm.R3, 0),                      // r3 = 0 (___skb)
+		asm.Mov.Reg(asm.R4, opts.Result),            // r4 = $result (data)
+		asm.Mov.Imm(asm.R5, 0),                      // r5 = 0 (data_end)
 	)
 
 	return insts, nil

internal/libpcap/inject.go

@@ -2,158 +2,37 @@ package libpcap
 
 import (
 	"errors"
-	"fmt"
 
 	"github.com/cilium/ebpf"
 	"github.com/cilium/ebpf/asm"
 	"github.com/cloudflare/cbpfc"
 )
 
-/*
-Steps:
-1. Find the injection position, which is the bpf_printk call
-2. Make some necessary preparations for the injection
-3. Compile the filter expression into ebpf instructions
-4. Inject the instructions
-*/
 func InjectFilter(program *ebpf.ProgramSpec, filterExpr string) (err error) {
-	/*
-	   First let's mark references and symbols for the jump instructions.
-	   This is even required when filterExpr is empty, because we still
-	   need to remove the bpf_printk call in that case, which breaks the
-	   jump instructions as well.
-	*/
-	injectIdx := 0
-	for idx, inst := range program.Instructions {
-		// In the kprobe_pwru.c, we deliberately put a bpf_printk call to mark the injection position, see the comments over there.
-		if inst.OpCode.JumpOp() == asm.Call && inst.Constant == int64(asm.FnTracePrintk) {
-			injectIdx = idx
-			break
-		}
-
-		/*
-			As we are injecting a bunch of instructions into the
-			program, the jump instructions are likely to require
-			adjustments on their pc-related offsets. For example,
-			we have the original bpf program as follows:
-
-			26: 	 if r9 >= r8 goto +384 <LBB1_39>
-			...
-			96: 	 call bpf_trace_printk#6
-			...
-
-			After the injection, the instruction No.96 is replaced
-			by multiple instructions, leaving the instruction No.26
-			jumping to a wrong instruction. The offset should be
-			adjusted accordingly!
-
-			We solve this problem smart way by using references and
-			symbols. The code below sets -1 to the affected jump
-			instructions' offsets, adds necessary symbols and
-			references, let cilium/ebpf collectionLoader adjust the
-			offsets according to these additional information. This
-			way, we don't have to calculate the new offsets by
-			hand, which is extremely likely to mess up.
-		*/
-		if inst.OpCode.Class().IsJump() {
-			// Zero jump offset implies a function call, leave it alone
-			if inst.Offset == 0 {
-				continue
-			}
-
-			// If there already is a reference and corresponding
-			// symbol, we don't have to create new symbol, just set -1
-			// to the offset so that cilium/ebpf loader can adjust it.
-			if inst.Reference() != "" {
-				program.Instructions[idx].Offset = -1
-				continue
-			}
-
-			var gotoIns *asm.Instruction
-			iter := asm.Instructions(program.Instructions[idx+1:]).Iterate()
-			for iter.Next() {
-				if int16(iter.Offset) == inst.Offset {
-					gotoIns = iter.Ins
-					break
-				}
-			}
-			if gotoIns == nil {
-				return errors.New("Cannot find the jump target")
-			}
-			symbol := gotoIns.Symbol()
-			if symbol == "" {
-				symbol = fmt.Sprintf("PWRU_%d", idx)
-				*gotoIns = gotoIns.WithSymbol(symbol)
-			}
-			program.Instructions[idx] = program.Instructions[idx].WithReference(symbol)
-			program.Instructions[idx].Offset = -1
-		}
-	}
-	if injectIdx == 0 {
-		return errors.New("Cannot find the injection position")
-	}
-
 	if filterExpr == "" {
-		/*
-		   No need to inject anything, just remove the bpf_printk call
-		   to avoid the unnecessary overhead.
-
-		   bpf_printk() compiles to 5 instructions from index idx-4 to
-		   idx: the former 4 are setting up registers from R1 to R4,
-		   the last one calls printk().
-
-		   But we can't delete former 4 instructions, otherwise we'll
-		   hit verifier with "R1 !read_ok"; they're required to stay
-		   there for register initialization.
-		*/
-		program.Instructions = append(program.Instructions[:injectIdx],
-			program.Instructions[injectIdx+1:]...,
-		)
 		return
 	}
 
-	/*
-		Conversion from cbpf to ebpf requires indication of the packet
-		start and end positions. These two position should be held by
-		two registers, thanks to the `bpf_printk("..", start, end)`
-		statement, which makes it clear that start is at R3 and end is
-		at R4.
-		The code below searches the instructions prior to the
-		injection position to find the registers holding the packet
-		start and end positions, by looking for the mov instructions
-		targeting R3 and R4.
-	*/
-	var (
-		dataReg    asm.Register = 255
-		dataEndReg asm.Register = 255
-	)
-	for idx := injectIdx - 1; idx >= 0; idx-- {
-		inst := program.Instructions[idx]
-		if inst.OpCode.ALUOp() == asm.Mov {
-			if inst.Dst == asm.R3 {
-				dataReg = inst.Src
-			} else if inst.Dst == asm.R4 {
-				dataEndReg = inst.Src
-			}
-		}
-		if dataReg != 255 && dataEndReg != 255 {
+	injectIdx := -1
+	for idx, inst := range program.Instructions {
+		if inst.Symbol() == "filter_pcap_ebpf" {
+			injectIdx = idx
 			break
 		}
 	}
-	if dataReg == 255 || dataEndReg == 255 {
-		return errors.New("Cannot find the data / data_end registers")
+	if injectIdx == -1 {
+		return errors.New("Cannot find the injection position")
 	}
 
 	filterEbpf, err := CompileEbpf(filterExpr, cbpfc.EBPFOpts{
-		PacketStart: dataReg,
-		PacketEnd:   dataEndReg,
-		// R4 is safe to use, because at the injection position, we are
-		// originally preparing to perform a bpf-helper func call with 4
-		// arguments, which leaves r0, r1, r2, r3 and r4 registers ready
-		// to use.
-		Result:      asm.R4,
+		// The rejection position is in the beginning of the `filter_pcap_ebpf` function:
+		// filter_pcap_ebpf(void *_skb, void *__skb, void *___skb, void *data, void* data_end)
+		// So we can confidently say, skb->data is at r4, skb->data_end is at r5.
+		PacketStart: asm.R4,
+		PacketEnd:   asm.R5,
+		Result:      asm.R0,
 		ResultLabel: "result",
-		// Same reason stated above, r0, r1, r2, r3 are safe to use.
+		// R0-R3 are also safe to use thanks to the placeholder parameters _skb, __skb, ___skb.
 		Working:     [4]asm.Register{asm.R0, asm.R1, asm.R2, asm.R3},
 		LabelPrefix: "filter",
 		// In the kprobe_pwru.c:handle_everything, the first line of
@@ -162,21 +41,16 @@ func InjectFilter(program *ebpf.ProgramSpec, filterExpr string) (err error) {
 		// stack area is safe to use. Here we use stack from -40
 		// because -32, -24, -16 are reserved for pcap-filter ebpf, see
 		// the comments in compile.go
-		StackOffset: 32,
+		StackOffset: 48,
 	})
 	if err != nil {
 		return
 	}
-	/*
-					;       bpf_printk("%d %d", data, data_end);
-		    injectIdx-4 ->	    88:       r1 = 54 ll
-					    90:       r2 = 6
-					    91:       r3 = r9
-					    92:       r4 = r8
-		    injectIdx ->	    93:       call 6
-	*/
-	program.Instructions = append(program.Instructions[:injectIdx-4],
-		append(filterEbpf, program.Instructions[injectIdx+1:]...)...,
+
+	filterEbpf[0] = filterEbpf[0].WithMetadata(program.Instructions[injectIdx].Metadata)
+	program.Instructions[injectIdx] = program.Instructions[injectIdx].WithMetadata(asm.Metadata{})
+	program.Instructions = append(program.Instructions[:injectIdx],
+		append(filterEbpf, program.Instructions[injectIdx:]...)...,
 	)
 
 	return nil