Re: [PATCH bpf-next 12/16] libbpf: Introduce arena percpu helpers for user space

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] libbpf: missing errno on error paths in bpf_map__arena_percpu_base()
- [Low] libbpf: 32-bit pointer cast warnings in bpf_map__arena_percpu_ptr()
- [Low] libbpf: inaccurate @return doc comment for bpf_map__arena_percpu_ptr()
--

commit e49cfa2af43e08885b5b19954e2551d5cbd7f6e2
Author: Xu Kuohai <xukuohai@huawei.com>

libbpf: Introduce arena percpu helpers for user space

This commit adds bpf_map__arena_percpu_base() and bpf_map__arena_percpu_ptr()
to libbpf so user space and generated skeletons can access arena per-cpu
areas.

> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index be2ddaf966318..0edce476451c3 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -12287,6 +12287,55 @@ void *bpf_map__initial_value(const struct bpf_map *map, size_t *psize)
>  	return map->mmaped;
>  }
>  
> +void *bpf_map__arena_percpu_ptr(const struct bpf_map *map, void *ptr, unsigned int cpu)
> +{
> +	struct bpf_object *obj;
> +	uint64_t base;
> +
> +	if (!map || !ptr)
> +		return libbpf_err_ptr(-EINVAL);
> +
> +	obj = map->obj;
> +	if (map->def.type != BPF_MAP_TYPE_ARENA || !obj->arena_percpu.present)
> +		return libbpf_err_ptr(-EINVAL);
> +
> +	if (cpu >= obj->arena_percpu.nr_chunks)
> +		return libbpf_err_ptr(-ERANGE);
> +
> +	/*
> +	 * obj->arena_percpu.chunk is freed by arena_percpu_populate(), and only
> +	 * then the map is mmaped
> +	 */
> +	if (obj->arena_percpu.chunk)
> +		return libbpf_err_ptr(-EINVAL);
> +
> +	base = (uint64_t)map->mmaped + obj->arena_percpu.offset;
> +	if ((uint64_t)ptr < base || (uint64_t)ptr >= base + obj->arena_percpu.chunk_sz)
> +		return libbpf_err_ptr(-EINVAL);
> +
> +	return (void *)((uint64_t)ptr + (uint64_t)cpu * obj->arena_percpu.chunk_sz);
> +}

[Severity: Low]
Will casting map->mmaped and ptr directly to uint64_t and back to void * in
bpf_map__arena_percpu_ptr() fail the build on 32-bit architectures?

Since tools/lib/bpf/Makefile sets -Wall -Werror, casting between a 32-bit
pointer and a 64-bit integer triggers -Wpointer-to-int-cast and
-Wint-to-pointer-cast warnings. Would using uintptr_t or void * pointer
arithmetic with size_t avoid this?

> +
> +void *bpf_map__arena_percpu_base(const struct bpf_map *map)
> +{
> +	struct bpf_object *obj;
> +
> +	if (!map)
> +		return NULL;
> +
> +	obj = map->obj;
> +	/*
> +	 * The per-cpu chunks exist once the map is created and populated;
> +	 * obj->arena_percpu.chunk is freed by arena_percpu_populate(), and
> +	 * only then the map is mmaped
> +	 */
> +	if (map->def.type != BPF_MAP_TYPE_ARENA || !obj->arena_percpu.present ||
> +	    obj->arena_percpu.chunk)
> +		return NULL;

[Severity: Medium]
Should the error paths in bpf_map__arena_percpu_base() return
libbpf_err_ptr(-EINVAL) instead of a bare NULL?

Because bpf_map__arena_percpu_base() is exported as a public LIBBPF_API
function in tools/lib/bpf/libbpf.h and tools/lib/bpf/libbpf.map, returning
NULL without setting errno when map is NULL, when map->def.type is not
BPF_MAP_TYPE_ARENA, when !obj->arena_percpu.present, or when
obj->arena_percpu.chunk is still set prevents callers from inspecting errno
or using libbpf_get_error().

> +
> +	return map->mmaped + obj->arena_percpu.offset;
> +}
> +
>  bool bpf_map__is_internal(const struct bpf_map *map)
>  {
>  	return map->libbpf_type != LIBBPF_MAP_UNSPEC;
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 3e0b7bc91bef9..31d3982226cc7 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -1230,6 +1230,32 @@ LIBBPF_API int bpf_map__set_initial_value(struct bpf_map *map,
>   */
>  LIBBPF_API void *bpf_map__initial_value(const struct bpf_map *map, size_t *psize);
>  
> +/**
> + * @brief **bpf_map__arena_percpu_ptr()** translates an address within
> + * cpu 0's chunk of the arena's per-cpu area (e.g. the address of an
> + * `__arena_percpu` variable as seen in the skeleton's arena_percpu
> + * section) to the per-cpu chunk of the requested @cpu.
> + * @param map the ARENA map
> + * @param ptr address of an `__arena_percpu` variable (cpu 0's chunk)
> + * @param cpu cpu id to obtain the chunk for
> + * @return pointer to the variable in the chunk of @cpu; error pointer, if the
> + * map is not a loaded ARENA map with a per-cpu area, @cpu is out of range,
> + * or @ptr is not within cpu 0's chunk
> + */
> +LIBBPF_API void *bpf_map__arena_percpu_ptr(const struct bpf_map *map, void *ptr,
> +					   unsigned int cpu);

[Severity: Low]
In the @return comment for bpf_map__arena_percpu_ptr() in
tools/lib/bpf/libbpf.h, it states that the function returns an "error
pointer" on failure.

In tools/lib/bpf/libbpf.c, bpf_map__arena_percpu_ptr() returns via
libbpf_err_ptr(), which sets errno and returns NULL rather than an ERR_PTR.
Should the comment state that it returns NULL with errno set instead?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792503001.git.xukuohai@huawei.com?part=12