Skip to content

Commit

Permalink
bpf: Fix veth tracing + --track-by-stackid
Browse files Browse the repository at this point in the history
cilium#391 allows --track-skb to track new
skb on veth, it relies on a map lookup to decide whether to track or
not:

```
SEC("kprobe/veth_convert_skb_to_xdp_buff")
int kprobe_veth_convert_skb_to_xdp_buff(struct pt_regs *ctx) {
[...]
	u64 skb_head = (u64) BPF_CORE_READ(skb, head);
	if (bpf_map_lookup_elem(&skb_heads, &skb_head)) {
[...]
	}
	return BPF_OK;
}
```

However, when --track-skb-by-stackid is used along with --track-skb, the
tracked skbs have risks not being recorded in skb_heads map.

This is because:
1. cilium#384 no more updates skb_heads
map when track reason is "by_stackid".
2. cilium#339 changes --track-skb from
   tracking &skb to tracking skb->head.

So imagine an skb whose original skb->head = 0xa, whose value is
updated to 0xb after a while. The first time pwru sees this skb,
skb_heads map will insert 0xa entry. However, after skb->head is set to
0xb, pwru sees the skb being tracked due to "by_stackid", we won't
insert 0xb entry into skb_heads map.

Then when the skb is on veth, we can't find an entry by looking up 0xb
from skb_heads map, ending up with losing track of veth skb again.

This patch fixes the issue by raising the priority of track_by_filter:
if an skb can be defined as tracked_by_filter and tracked_by_stackid,
use tracked_by_filter over tracked_by_stackid.

Another issue cilium#339 brings about is,
an skb can have multiple skb->head stored in skb_heads map during its
lifetime, but we only clean the latest value at
kprobe_skb_lifetime_termination. This issue is beyond this patch.

Signed-off-by: gray <[email protected]>
  • Loading branch information
jschwinger233 committed Jul 12, 2024
1 parent 0aa10ab commit 444c5f2
Showing 1 changed file with 5 additions and 5 deletions.
10 changes: 5 additions & 5 deletions bpf/kprobe_pwru.c
Original file line number Diff line number Diff line change
Expand Up @@ -438,13 +438,13 @@ handle_everything(struct sk_buff *skb, void *ctx, struct event_t *event, u64 *_s
goto cont;
}

if (cfg->track_skb_by_stackid && bpf_map_lookup_elem(&stackid_skb, &stackid)) {
tracked_by = TRACKED_BY_STACKID;
if (filter(skb)) {
tracked_by = TRACKED_BY_FILTER;
goto cont;
}

if (filter(skb)) {
tracked_by = TRACKED_BY_FILTER;
if (cfg->track_skb_by_stackid && bpf_map_lookup_elem(&stackid_skb, &stackid)) {
tracked_by = TRACKED_BY_STACKID;
goto cont;
}

Expand Down Expand Up @@ -538,7 +538,7 @@ int kprobe_skb_lifetime_termination(struct pt_regs *ctx) {
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);
}

return BPF_OK;
Expand Down

0 comments on commit 444c5f2

Please sign in to comment.