Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect global variables usage for outputting skb and shinfo #491

Open
Asphaltt opened this issue Jan 23, 2025 · 0 comments
Open

Incorrect global variables usage for outputting skb and shinfo #491

Asphaltt opened this issue Jan 23, 2025 · 0 comments

Comments

@Asphaltt
Copy link
Contributor

In PR #484 , when I confirm whether it's necesssary to share .bss map between different collections, I confirm that the global variables are used incorrectly.

What global variables in .bss map?

# bpftool m d n .bss
[{
        "value": {
            ".bss": [{
                    "set_skb_btf.p": {
                        "ptr": "(nil)",
                        "type_id": 0,
                        "flags": 0
                    }
                },{
                    "set_skb_btf.v": {
                        "len": 0,
                        "str": ""
                    }
                },{
                    "set_shinfo_btf.p": {
                        "ptr": "(nil)",
                        "type_id": 0,
                        "flags": 0
                    }
                },{
                    "set_shinfo_btf.v": {
                        "len": 0,
                        "str": ""
                    }
                }
            ]
        }
    }
]

But wait, what are they?

// bpf/kprobe_pwru.c

static __always_inline void
set_skb_btf(struct sk_buff *skb, u64 *event_id) {
	static struct btf_ptr p = {};
	static struct print_skb_value v = {};
	u64 id;

	p.type_id = cfg->skb_btf_id;
	p.ptr = skb;
	*event_id = sync_fetch_and_add(&print_skb_id_map);

	v.len = bpf_snprintf_btf(v.str, PRINT_SKB_STR_SIZE, &p, sizeof(p), 0);
	if (v.len < 0) {
		return;
	}

	bpf_map_update_elem(&print_skb_map, event_id, &v, BPF_ANY);
}

static __always_inline void
set_shinfo_btf(struct sk_buff *skb, u64 *event_id) {
	struct skb_shared_info *shinfo;
	static struct btf_ptr p = {};
	static struct print_shinfo_value v = {};
	unsigned char *head;
	unsigned int end;

        /* skb_shared_info is located at the end of skb data.
         * When CONFIG_NET_SKBUFF_DATA_USES_OFFSET is enabled, skb->end
         * is an offset from skb->head to the end of skb data. If not,
         * skb->end is a pointer to the end of skb data. For amd64 and
         * arm64 (in 64bit arch in general), CONFIG_NET_SKBUFF_DATA_USES_OFFSET
	 * is enabled by default.
         */
        head = BPF_CORE_READ(skb, head);
	end = BPF_CORE_READ(skb, end);
	shinfo = (struct skb_shared_info *)(head + end);

	p.type_id = cfg->shinfo_btf_id;
	p.ptr = shinfo;

	*event_id = sync_fetch_and_add(&print_shinfo_id_map);

	v.len = bpf_snprintf_btf(v.str, PRINT_SHINFO_STR_SIZE, &p, sizeof(p), 0);
	if (v.len < 0) {
		return;
	}

	bpf_map_update_elem(&print_shinfo_map, event_id, &v, BPF_ANY);
}

Aha, they are used for outputting skb and shinfo.

But when there are more-than-1 running pwru bpf progs on different CPUs, they will share the same globals at the same time without memory protection.

To resolve the memory-race issue, these variables should be set as global percpu variables by utilizing percpu_array map.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant