Skip to content

Commit

Permalink
bpf: Use &skb for --filter-track-skb again
Browse files Browse the repository at this point in the history 
#339 changed how pwru tracks skb from
using &skb to skb->head for xdp tracing. We found it causing problems
such as:
1. An skb can have multiple different skb->head values during its
   lifetime, especically after (e.g. IPsec) encryption. Pwru now is
   likely to lose track of skb after encryption.
2. Some subtle issues like #401. This
   is because some other tracking mechanism relies on &skb.

This commit brings back tracking &skb instead of skb->head. As for XDP
tracing, I'll introduce a new solution in the following patches.

Signed-off-by: gray <gray.liang@isovalent.com>
  • Loading branch information
@jschwinger233 @brb
jschwinger233 authored and brb committed Aug 30, 2024
1 parent 6d10c6f commit 3824944
Show file tree
Hide file tree
Showing 3 changed files with 27 additions and 27 deletions.
42 changes: 21 additions & 21 deletions bpf/kprobe_pwru.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ struct event_t {
u32 type;
u64 addr;
u64 caller_addr;
u64 skb_head;
u64 skb_addr;
u64 ts;
u64 print_skb_id;
u64 print_shinfo_id;
Expand All @@ -101,7 +101,7 @@ struct {
__type(key, __u64);
__type(value, bool);
__uint(max_entries, MAX_TRACK_SIZE);
} skb_heads SEC(".maps");
} skb_addresses SEC(".maps");

struct {
__uint(type, BPF_MAP_TYPE_HASH);
Expand Down Expand Up @@ -426,14 +426,14 @@ set_output(void *ctx, struct sk_buff *skb, struct event_t *event) {
static __noinline bool
handle_everything(struct sk_buff *skb, void *ctx, struct event_t *event, u64 *_stackid) {
u8 tracked_by;
u64 skb_head = (u64) BPF_CORE_READ(skb, head);
u64 skb_addr = (u64) skb;
u64 stackid;

if (cfg->track_skb_by_stackid)
stackid = _stackid ? *_stackid : get_stackid(ctx);

if (cfg->is_set) {
if (cfg->track_skb && bpf_map_lookup_elem(&skb_heads, &skb_head)) {
if (cfg->track_skb && bpf_map_lookup_elem(&skb_addresses, &skb_addr)) {
tracked_by = _stackid ? TRACKED_BY_STACKID : TRACKED_BY_SKB;
goto cont;
}
Expand All @@ -455,7 +455,7 @@ handle_everything(struct sk_buff *skb, void *ctx, struct event_t *event, u64 *_s
}

if (cfg->track_skb && tracked_by == TRACKED_BY_FILTER) {
bpf_map_update_elem(&skb_heads, &skb_head, &TRUE, BPF_ANY);
bpf_map_update_elem(&skb_addresses, &skb_addr, &TRUE, BPF_ANY);
}

if (cfg->track_skb_by_stackid && tracked_by != TRACKED_BY_STACKID) {
Expand All @@ -481,7 +481,7 @@ kprobe_skb(struct sk_buff *skb, struct pt_regs *ctx, bool has_get_func_ip, u64 *
if (!handle_everything(skb, ctx, &event, _stackid))
return BPF_OK;

event.skb_head = (u64) BPF_CORE_READ(skb, head);
event.skb_addr = (u64) skb;
event.addr = has_get_func_ip ? bpf_get_func_ip(ctx) : PT_REGS_IP(ctx);
event.param_second = PT_REGS_PARM2(ctx);
if (CFG.output_caller)
Expand Down Expand Up @@ -531,25 +531,25 @@ int kprobe_skb_by_stackid(struct pt_regs *ctx) {
SEC("kprobe/skb_lifetime_termination")
int kprobe_skb_lifetime_termination(struct pt_regs *ctx) {
struct sk_buff *skb = (typeof(skb)) PT_REGS_PARM1(ctx);
u64 skb_head = (u64) BPF_CORE_READ(skb, head);
u64 skb_addr = (u64) skb;

bpf_map_delete_elem(&skb_heads, &skb_head);
bpf_map_delete_elem(&skb_addresses, &skb_addr);

if (cfg->track_skb_by_stackid) {
u64 stackid = get_stackid(ctx);
bpf_map_delete_elem(&stackid_skb, &stackid);
bpf_map_delete_elem(&skb_stackid, &skb_head);
bpf_map_delete_elem(&skb_stackid, &skb_addr);
}

return BPF_OK;
}

static __always_inline int
track_skb_clone(struct sk_buff *old, struct sk_buff *new) {
u64 skb_addr_old = (u64) BPF_CORE_READ(old, head);
u64 skb_addr_new = (u64) BPF_CORE_READ(new, head);
if (bpf_map_lookup_elem(&skb_heads, &skb_addr_old))
bpf_map_update_elem(&skb_heads, &skb_addr_new, &TRUE, BPF_ANY);
u64 skb_addr_old = (u64) old;
u64 skb_addr_new = (u64) new;
if (bpf_map_lookup_elem(&skb_addresses, &skb_addr_old))
bpf_map_update_elem(&skb_addresses, &skb_addr_new, &TRUE, BPF_ANY);

return BPF_OK;
}
Expand Down Expand Up @@ -577,7 +577,7 @@ int BPF_PROG(fentry_tc, struct sk_buff *skb) {
if (!handle_everything(skb, ctx, &event, NULL))
return BPF_OK;

event.skb_head = (u64) BPF_CORE_READ(skb, head);
event.skb_addr = (u64) skb;
event.addr = BPF_PROG_ADDR;
event.type = EVENT_TYPE_TC;
bpf_map_push_elem(&events, &event, BPF_EXIST);
Expand Down Expand Up @@ -668,11 +668,11 @@ int BPF_PROG(fentry_xdp, struct xdp_buff *xdp) {

if (cfg->is_set) {
if (cfg->track_skb) {
if (!bpf_map_lookup_elem(&skb_heads, &skb_head)) {
if (!bpf_map_lookup_elem(&skb_addresses, &skb_head)) {
if (!filter_xdp(xdp))
return BPF_OK;

bpf_map_update_elem(&skb_heads, &skb_head, &TRUE, BPF_ANY);
bpf_map_update_elem(&skb_addresses, &skb_head, &TRUE, BPF_ANY);
}

} else if (!filter_xdp(xdp)) {
Expand All @@ -685,7 +685,7 @@ int BPF_PROG(fentry_xdp, struct xdp_buff *xdp) {
event.pid = bpf_get_current_pid_tgid() >> 32;
event.ts = bpf_ktime_get_ns();
event.cpu_id = bpf_get_smp_processor_id();
event.skb_head = (u64) skb_head;
event.skb_addr = (u64) skb_head;
event.addr = BPF_PROG_ADDR;
event.type = EVENT_TYPE_XDP;
bpf_map_push_elem(&events, &event, BPF_EXIST);
Expand All @@ -698,8 +698,8 @@ int kprobe_veth_convert_skb_to_xdp_buff(struct pt_regs *ctx) {
struct sk_buff **pskb = (struct sk_buff **)PT_REGS_PARM3(ctx);
struct sk_buff *skb;
bpf_probe_read_kernel(&skb, sizeof(skb), (void *)pskb);
u64 skb_head = (u64) BPF_CORE_READ(skb, head);
if (bpf_map_lookup_elem(&skb_heads, &skb_head)) {
u64 skb_addr = (u64) skb;
if (bpf_map_lookup_elem(&skb_addresses, &skb_addr)) {
u64 pid_tgid = bpf_get_current_pid_tgid();
bpf_map_update_elem(&veth_skbs, &pid_tgid, &pskb, BPF_ANY);
}
Expand All @@ -713,8 +713,8 @@ int kretprobe_veth_convert_skb_to_xdp_buff(struct pt_regs *ctx) {
if (pskb && *pskb) {
struct sk_buff *skb;
bpf_probe_read_kernel(&skb, sizeof(skb), (void *)*pskb);
u64 skb_head = (u64) BPF_CORE_READ(skb, head);
bpf_map_update_elem(&skb_heads, &skb_head, &TRUE, BPF_ANY);
u64 skb_addr = (u64) skb;
bpf_map_update_elem(&skb_addresses, &skb_addr, &TRUE, BPF_ANY);
bpf_map_delete_elem(&veth_skbs, &pid_tgid);
}
return BPF_OK;
Expand Down
10 changes: 5 additions & 5 deletions internal/pwru/output.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,15 +158,15 @@ func (o *output) PrintJson(event *Event) {
d := &jsonPrinter{}

// add the data to the struct
d.Skb = fmt.Sprintf("%#x", event.SkbHead)
d.Skb = fmt.Sprintf("%#x", event.SkbAddr)
d.Cpu = event.CPU
d.Process = getExecName(int(event.PID))
d.Func = getOutFuncName(o, event, event.Addr)
if o.flags.OutputCaller {
d.CallerFunc = o.addr2name.findNearestSym(event.CallerAddr)
}

o.lastSeenSkb[event.SkbHead] = event.Timestamp
o.lastSeenSkb[event.SkbAddr] = event.Timestamp

// add the timestamp to the struct if it is not set to none
if o.flags.OutputTS != "none" {
Expand Down Expand Up @@ -227,7 +227,7 @@ func getAbsoluteTs() string {

func getRelativeTs(event *Event, o *output) uint64 {
ts := event.Timestamp
if last, found := o.lastSeenSkb[event.SkbHead]; found {
if last, found := o.lastSeenSkb[event.SkbAddr]; found {
ts = ts - last
} else {
ts = 0
Expand Down Expand Up @@ -393,12 +393,12 @@ func (o *output) Print(event *Event) {

outFuncName := getOutFuncName(o, event, addr)

fmt.Fprintf(o.writer, "%-18s %-3s %-16s", fmt.Sprintf("%#x", event.SkbHead),
fmt.Fprintf(o.writer, "%-18s %-3s %-16s", fmt.Sprintf("%#x", event.SkbAddr),
fmt.Sprintf("%d", event.CPU), fmt.Sprintf("%s", execName))
if o.flags.OutputTS != "none" {
fmt.Fprintf(o.writer, " %-16d", ts)
}
o.lastSeenSkb[event.SkbHead] = event.Timestamp
o.lastSeenSkb[event.SkbAddr] = event.Timestamp

if o.flags.OutputMeta {
fmt.Fprintf(o.writer, " %s", getMetaData(event, o))
Expand Down
2 changes: 1 addition & 1 deletion internal/pwru/types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ type Event struct {
Type uint32
Addr uint64
CallerAddr uint64
SkbHead uint64
SkbAddr uint64
Timestamp uint64
PrintSkbId uint64
PrintShinfoId uint64
Expand Down

0 comments on commit 3824944

Please # to comment.