Re: [PATCH bpf-next 13/16] bpftool: Emit the arena percpu datasec into skeleton

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] bpftool: NULL pointer dereference in do_skeleton() when BTF is absent
- [Low] bpftool: missing pointer cast in do_skeleton() breaks C++ skeleton build
--

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

bpftool: Emit the arena percpu datasec into skeleton

This commit updates bpftool skeleton generation to emit a separate
arena_percpu data section struct when .arena.percpu variables are present
and initializes obj->arena_percpu to cpu 0's per-cpu arena base in the
generated load function.

> diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
> index ee680f7c21160..8a321ec1155a1 100644
> --- a/tools/bpf/bpftool/gen.c
> +++ b/tools/bpf/bpftool/gen.c

[ ... ]

> @@ -1511,11 +1523,58 @@ static int do_skeleton(int argc, char **argv)
>  			return %1$s__open_opts(NULL);			    \n\
>  		}							    \n\
>  									    \n\
> -		static inline int					    \n\
> -		%1$s__load(struct %1$s *obj)				    \n\
> -		{							    \n\
> -			return bpf_object__load_skeleton(obj->skeleton);    \n\
> -		}							    \n\
> +		",
> +		obj_name
> +	);
> +
> +	/* The per-cpu arena data has no mmaped slot in the map skeleton.
> +	 * Point obj->arena_percpu at cpu 0's chunk once the object is
> +	 * loaded.
> +	 */
> +	{
> +		char arena_map_ident[256] = "";
> +		bool have_percpu_arena = false;
> +
> +		bpf_object__for_each_map(map, obj) {
> +			if (bpf_map__type(map) != BPF_MAP_TYPE_ARENA)
> +				continue;
> +			if (find_type_for_map(bpf_object__btf(obj), "arena_percpu") &&
> +			    get_map_ident(map, arena_map_ident, sizeof(arena_map_ident))) {

[Severity: Medium]
Can this dereference a NULL btf pointer when generating a skeleton for an
object with an arena map but no BTF information?

Earlier in do_skeleton(), codegen_datasecs() is guarded by checking if btf
is non-NULL:

    if (btf) {
        err = codegen_datasecs(obj, obj_name);
        if (err)
            goto out;
    }

Here, bpf_object__btf(obj) is passed directly to find_type_for_map()
without checking if btf is NULL:

    do_skeleton()
      find_type_for_map(bpf_object__btf(obj), "arena_percpu")
        btf__type_cnt(btf)
          return btf->start_id + btf->nr_types;

Should this check that btf is non-NULL before calling find_type_for_map()?

> +				have_percpu_arena = true;
> +				break;
> +			}
> +		}
> +
> +		if (have_percpu_arena) {
> +			codegen("\
> +				\n\
> +				static inline int \n\
> +				%1$s__load(struct %1$s *obj) \n\
> +				{ \n\
> +					int err; \n\
> +					 \n\
> +					err = bpf_object__load_skeleton(obj->skeleton); \n\
> +					if (err) \n\
> +						return err; \n\
> +					obj->arena_percpu = \n\
> +						bpf_map__arena_percpu_base(obj->maps.%2$s); \n\

[Severity: Low]
Will this assignment fail to compile when the generated skeleton header is
included in C++ code?

bpf_map__arena_percpu_base() returns void *, while obj->arena_percpu is a
typed struct pointer (struct <obj_name>__arena_percpu *). In C++, implicit
conversion from void * to a typed pointer is not permitted.

Elsewhere in tools/bpf/bpftool/gen.c, such as in gen_st_ops_shadow_init(),
void * returns are cast using __typeof__:

    obj->struct_ops.%1$s = (__typeof__(obj->struct_ops.%1$s))
        bpf_map__initial_value(obj->maps.%1$s, NULL);

Should obj->arena_percpu also use a (__typeof__(obj->arena_percpu)) cast
here?

> +					return 0; \n\
> +				} \n\
> +				", obj_name, arena_map_ident);

[ ... ]

>

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