Skip to content

Conversation

@kernel-patches-daemon-bpf
Copy link

Pull request for series with
subject: libbpf: move arena variables out of the zero page
version: 3
url: https://patchwork.kernel.org/project/netdevbpf/list/?series=1033358

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: 6f0b824
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1033358
version: 3

@kernel-patches-review-bot
Copy link

commit 49bb5d28995a3cb106b1d655dc8acc99a2eb6c04
Author: Emil Tsalapatis <[email protected]>

libbpf: turn relo_core->sym_off unsigned

This patch changes the sym_off field from int to unsigned int to support
offsets larger than 2GiB for arena globals. The commit message explains
that large offsets should not be misinterpreted as negative values.

> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index c7c79014d46c..5e66bbc2ab85 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -380,7 +380,7 @@ struct reloc_desc {
>  		const struct bpf_core_relo *core_relo; /* used when type == RELO_CORE */
>  		struct {
>  			int map_idx;
> -			int sym_off;
> +			unsigned int sym_off;
>  			/*
>  			 * The following two fields can be unionized, as the
>  			 * ext_idx field is used for extern symbols, and the
> @@ -763,7 +763,7 @@ struct bpf_object {
>
>  	struct {
>  		struct bpf_program *prog;
> -		int sym_off;
> +		unsigned int sym_off;
>  		int fd;
>  	} *jumptable_maps;
>  	size_t jumptable_map_cnt;

The field type change looks correct, but does create_jt_map() defeat
the purpose of this patch? That function declares a local variable
"int sym_off" that immediately converts the unsigned field back to
signed. When sym_off is >= 2GiB (the exact scenario this patch enables),
the local variable will interpret it as negative, causing incorrect
behavior in the modulo check, bounds check, and pointer arithmetic that
follow.



AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

In-Reply-To-Subject: libbpf: turn relo_core->sym_off unsigned
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/20239524817

@kernel-patches-daemon-bpf
Copy link
Author

Forwarding comment 3656567761 via email
In-Reply-To: [email protected]
Patch: https://patchwork.kernel.org/project/netdevbpf/patch/[email protected]/

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: 6f0b824
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1033358
version: 3

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: 6f0b824
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1033358
version: 3

The big_alloc1 test in verifier_arena_large assumes that the arena base
and the first page allocated by bpf_arena_alloc_pages are identical.
This is not the case, because the first page in the arena is populated
by global arena data. The test still passes because the code makes the
tacit assumption that the first page is on offset PAGE_SIZE instead of
0.

Make this distinction explicit in the code, and adjust the page offsets
requested during the test to count from the beginning of the arena
instead of using the address of the first allocated page.

Reviewed-by: Eduard Zingerman <[email protected]>
Signed-off-by: Emil Tsalapatis <[email protected]>
The verifier currently limits direct offsets into a map to 512MiB
to avoid overflow during pointer arithmetic. However, this prevents
arena maps from using direct addressing instructions to access data
at the end of > 512MiB arena maps. This is necessary when moving
arena globals to the end of the arena instead of the front.

Refactor the verifier code to remove the offset calculation during
direct value access calculations. This is possible because the only
two map types that implement .map_direct_value_addr() are arrays and
arenas, and they both do their own internal checks to ensure the
offset is within bounds.

Signed-off-by: Emil Tsalapatis <[email protected]>
Acked-by: Eduard Zingerman <[email protected]>
The symbols' relocation offsets in BPF are stored in an int field,
but cannot actually be negative. When in the next patch libbpf relocates
globals to the end of the arena, it is also possible to have valid
offsets > 2GiB that are used to calculate the final relo offsets.
Avoid accidentally interpreting large offsets as negative by turning
the sym_off field unsigned.

Signed-off-by: Emil Tsalapatis <[email protected]>
Acked-by: Eduard Zingerman <[email protected]>
Arena globals are currently placed at the beginning of the arena
by libbpf. This is convenient, but prevents users from reserving
guard pages in the beginning of the arena to identify NULL pointer
dereferences. Adjust the load logic to place the globals at the
end of the arena instead.

Also modify bpftool to set the arena pointer in the program's BPF
skeleton to point to the globals. Users now call bpf_map__initial_value()
to find the beginning of the arena mapping and use the arena pointer
in the skeleton to determine which part of the mapping holds the
arena globals and which part is free.

Suggested-by: Andrii Nakryiko <[email protected]>
Signed-off-by: Emil Tsalapatis <[email protected]>
Acked-by: Eduard Zingerman <[email protected]>
@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: 6f0b824
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1033358
version: 3

Add tests for the new libbpf globals arena offset logic. The
tests cover the case of globals being as large as the arena
itself, and being smaller than the arena. In that case, the
data is placed at the end of the arena, and the beginning
of the arena is free.

Signed-off-by: Emil Tsalapatis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants