Re: [PATCH bpf-next v5 5/9] bpftool: Generate skeleton for global percpu data

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Exposing `.percpu` maps via `skel->percpu` in generated skeletons introduces severe abstraction violations: silent write failures in Light Skeletons and stale data reads in Subskeletons.
--

commit 8d5ae684c03b8ed70309315b60b9b65b471e24bd
Author: Leon Hwang <leon.hwang@linux.dev>

bpftool: Generate skeleton for global percpu data

This patch enhances bpftool to generate skeletons that properly handle global
percpu variables. The generated skeleton includes a dedicated nested structure
for percpu data, allowing easier initialization.

> diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
> index 6ae7262ebe0c1..e0c5c643fe9ea 100644
> --- a/tools/bpf/bpftool/gen.c
> +++ b/tools/bpf/bpftool/gen.c

[ ... ]

> @@ -263,13 +268,12 @@ static bool is_mmapable_map(const struct bpf_map *map, char *buf, size_t sz)
>  		return true;
>  	}
>  
> -	if (!bpf_map__is_internal(map) || !(bpf_map__map_flags(map) & BPF_F_MMAPABLE))
> -		return false;
> -
> -	if (!get_map_ident(map, buf, sz))
> -		return false;
> +	if (bpf_map__is_internal(map) &&
> +	    ((bpf_map__map_flags(map) & BPF_F_MMAPABLE) || bpf_map_is_percpu_data(map)) &&
> +	    get_map_ident(map, buf, sz))
> +		return true;
>  
> -	return true;
> +	return false;
>  }

[Severity: High]
Does forcibly treating the percpu maps as mmapable here break subskeletons?

Since subskeletons operate on an already-loaded BPF object to read or write
live state, this change might cause do_subskeleton() to export pointers to
disconnected user-space memory.

If they point to the disconnected PROT_READ initialization buffer instead of
live state, would reads always return stale initial data and fail to reflect
live kernel modifications?

[ ... ]

> @@ -847,7 +851,7 @@ static int gen_trace(struct bpf_object *obj, const char *obj_name, const char *h
>  	bpf_object__for_each_map(map, obj) {
>  		const char *mmap_flags;
>  
> -		if (!is_mmapable_map(map, ident, sizeof(ident)))
> +		if (!is_mmapable_map(map, ident, sizeof(ident)) || bpf_map_is_percpu_data(map))
>  			continue;
>  
>  		if (bpf_map__map_flags(map) & BPF_F_RDONLY_PROG)

[Severity: High]
Does skipping map finalization for percpu maps in light skeletons leave the
user-space initialization buffer mapped as PROT_READ | PROT_WRITE?

In standard skeletons managed by libbpf, the buffer is protected with
PROT_READ to explicitly crash on invalid writes.

If we skip finalization here in gen_trace(), it seems writes to lskel->percpu
after load() might silently succeed without actually updating the kernel map.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608145113.65857-1-leon.hwang@linux.dev?part=5