Re: [PATCH bpf-next v3] bpftool: Compute map size of light skeletons at runtime

[bot+bpf-ci]

> diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
> index a50540ef6521..ee680f7c2116 100644
> --- a/tools/bpf/bpftool/gen.c
> +++ b/tools/bpf/bpftool/gen.c

[ ... ]

> diff --git a/tools/lib/bpf/skel_internal.h b/tools/lib/bpf/skel_internal.h
> index 1f3f332dffbe..9e16b8e8cb7c 100644
> --- a/tools/lib/bpf/skel_internal.h
> +++ b/tools/lib/bpf/skel_internal.h
> @@ -11,6 +11,7 @@
>  #include <linux/bpf.h>
>  #else
>  #include <unistd.h>
> +#include <sys/param.h>
>  #include <sys/syscall.h>
>  #include <sys/mman.h>
>  #include <linux/keyctl.h>
> @@ -127,7 +128,7 @@ static inline void skel_free(const void *p)
>   * either bpf_probe_read_kernel() or bpf_copy_from_user() from initial_value
>   * depending on bpf_loader_ctx->flags.
>   */
> -static inline void skel_free_map_data(void *p, __u64 addr, size_t sz)
> +static inline void skel_free_map_data(void *p, __u64 addr, size_t val_sz, __u32 max_entries)
>  {
>  	if (addr != ~0ULL)
>  		kvfree(p);
> @@ -138,18 +139,20 @@ static inline void skel_free_map_data(void *p, __u64 addr, size_t sz)
>  	 */
>  }
>
> -static inline void *skel_prep_map_data(const void *val, size_t mmap_sz, size_t val_sz)
> +static inline void *skel_prep_map_data(const void *val, size_t val_sz, __u32 max_entries,
> +				       size_t data_sz)
>  {
>  	void *addr;
>
> -	addr = kvmalloc(val_sz, GFP_KERNEL);
> +	addr = kvmalloc(data_sz, GFP_KERNEL);
>  	if (!addr)
>  		return NULL;
> -	memcpy(addr, val, val_sz);
> +	memcpy(addr, val, data_sz);
>  	return addr;
>  }

This isn't a bug, but would naming these something like value_sz and
init_sz make it harder to mix up the map value size and the embedded
blob length? The two names differ by three characters and describe
different quantities, and in the __KERNEL__ variant above val_sz is not
referenced at all while data_sz drives both the kvmalloc() and the
memcpy(). A future edit that reaches for the wrong one would size an
allocation or a copy incorrectly.

>
> -static inline void *skel_finalize_map_data(__u64 *init_val, size_t mmap_sz, int flags, int fd)
> +static inline void *skel_finalize_map_data(__u64 *init_val, size_t val_sz, __u32 max_entries,
> +					   int flags, int fd)

[ ... ]

> @@ -193,25 +197,39 @@ static inline void skel_free(void *p)
>  	free(p);
>  }

[ ... ]

>
> -static inline void *skel_prep_map_data(const void *val, size_t mmap_sz, size_t val_sz)
> +static inline void *skel_prep_map_data(const void *val, size_t val_sz, __u32 max_entries,
> +				       size_t data_sz)
>  {
> +	size_t mmap_sz = skel_map_mmap_sz(val_sz, max_entries);
>  	void *addr;
>
>  	addr = mmap(NULL, mmap_sz, PROT_READ | PROT_WRITE,
>  		    MAP_SHARED | MAP_ANONYMOUS, -1, 0);
>  	if (addr == (void *) -1)
>  		return NULL;
> -	memcpy(addr, val, val_sz);
> +	memcpy(addr, val, data_sz);
>  	return addr;
>  }

[ ... ]

commit 417792b69014155cfa0ecc10cdffc4c2e54b9e60
Author: Leon Hwang <leon.hwang@xxxxxxxxx>

>     bpftool: Compute map size of light skeletons at runtime
>
>     bpftool rounds memory-mapped data map sizes to the host page size when
>     generating a light skeleton. The generated code therefore uses a 64K
>     mapping size when bpftool runs on a 64K-page host, even if the skeleton
>     runs on a 4K-page target. The target rejects the oversized map mmap(),
>     causing failure of loading the light skeleton.
>
>     When try to run 64K-page selftests on 4K-page VM, the error message does
>     not provide the reason about page size.
>
>      test_atomics:PASS:atomics skeleton open 0 nsec
>      test_atomics:FAIL:atomics skeleton load unexpected error: -12 (errno 22)
>      #15      atomics:FAIL

This isn't a bug, but could this sentence be reworded ("When running
64K-page selftests on a 4K-page VM ...") or dropped, since the patch
does not change the error message itself? The sentence is missing a
subject and describes a diagnostics shortcoming that this patch does not
address.


---
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

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/34994524877