Skip to content

Commit

Permalink
bpf: Differentiate skb tracking reasons
Browse files Browse the repository at this point in the history
By differentiating skb track reason (filter/skb/stackid), we can avoid a
mismatching issue that leads to confusing output.

I saw an issue when --filter-track-skb and --filter-track-skb-by-stackid
are used together:
1. At func1, an skb matches the pcap filter, we then fill the map
   stackid_skb[stackid] = skb for --filter-track-skb-by-stackid;
2. At func2, the skb arg doesn't match the pcap filter, but stackid can
   be found in map stackid_skb. We therefore store the map entry
   skb_addresses[skb] = true for --filter-track-skb;
3. Due to some reasons, this skb never gets freed by kfree_skbmem(), so
   pwru keeps matching "wrong" packets due to permanent entry in map
   skb_addresses for --filter-track-skb;

This patch mitigates these risks by not storing new entry in
skb_addresses if the skb is tracked by stackid. This doesn't harm the
original purpose of --filter-track-skb-by-stack, which is designed for
non-skb function tracing on the same stack.

On the otherhand, calling bpf_update_map() only for specific track
reasons improves performance a little bit. For example, if an skb is
already in map skb_addresses, now we don't need to put it in the map
again.

Signed-off-by: gray <[email protected]>
  • Loading branch information
jschwinger233 authored and brb committed Jun 11, 2024
1 parent 7bacf08 commit 059997a
Showing 1 changed file with 16 additions and 6 deletions.
22 changes: 16 additions & 6 deletions bpf/kprobe_pwru.c
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,12 @@ const static bool TRUE = true;

volatile const static __u64 BPF_PROG_ADDR = 0;

enum {
TRACKED_BY_FILTER = (1 << 0),
TRACKED_BY_SKB = (1 << 1),
TRACKED_BY_STACKID = (1 << 2),
};

union addr {
u32 v4addr;
struct {
Expand Down Expand Up @@ -375,7 +381,7 @@ 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) {
bool tracked_by_addr = false;
u8 tracked_by;
u64 skb_addr = (u64) skb;
u64 stackid;

Expand All @@ -384,27 +390,31 @@ handle_everything(struct sk_buff *skb, void *ctx, struct event_t *event, u64 *_s

if (cfg->is_set) {
if (cfg->track_skb && bpf_map_lookup_elem(&skb_addresses, &skb_addr)) {
tracked_by_addr = true;
tracked_by = _stackid ? TRACKED_BY_STACKID : TRACKED_BY_SKB;
goto cont;
}

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

if (!filter(skb)) {
return false;
if (filter(skb)) {
tracked_by = TRACKED_BY_FILTER;
goto cont;
}

return false;

cont:
set_output(ctx, skb, event);
}

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

if (cfg->track_skb_by_stackid) {
if (cfg->track_skb_by_stackid && tracked_by != TRACKED_BY_STACKID) {
u64 *old_stackid = bpf_map_lookup_elem(&skb_stackid, &skb);
if (old_stackid && *old_stackid != stackid) {
bpf_map_delete_elem(&stackid_skb, old_stackid);
Expand Down

0 comments on commit 059997a

Please sign in to comment.