Re: [PATCH bpf-next 05/16] libbpf: Add the arena percpu chunk attribute to map creation options

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] libbpf: ABI break in struct bpf_map_create_opts from field insertion
--

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

libbpf: Add the arena percpu chunk attribute to map creation options

This commit adds arena_percpu_chunk_off and arena_percpu_chunk_sz to
union bpf_attr and struct bpf_map_create_opts, and populates them when
creating an arena map.

> diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> index 826d9cc9ab65d..15a544488eb59 100644
> --- a/tools/lib/bpf/bpf.h
> +++ b/tools/lib/bpf/bpf.h
> @@ -70,6 +70,10 @@ struct bpf_map_create_opts {
>  	const void *excl_prog_hash;
>  	__u32 excl_prog_hash_size;
>  
> +	/* offset and size of an arena percpu chunk */
> +	__u32 arena_percpu_chunk_off;
> +	__u32 arena_percpu_chunk_sz;
> +
>  	struct bpf_log_opts *log_opts;
>  
>  	size_t :0;

[Severity: High]
Does inserting arena_percpu_chunk_off and arena_percpu_chunk_sz before
log_opts break binary compatibility for struct bpf_map_create_opts?

struct bpf_map_create_opts is a size-versioned LIBBPF_OPTS structure
validated and accessed through OPTS_VALID() and OPTS_GET() in
bpf_map_create():

tools/lib/bpf/bpf.c:bpf_map_create() {
	...
	if (!OPTS_VALID(opts, bpf_map_create_opts))
		return libbpf_err(-EINVAL);
	...
	attr.arena_percpu_chunk_off = OPTS_GET(opts, arena_percpu_chunk_off, 0);
	attr.arena_percpu_chunk_sz = OPTS_GET(opts, arena_percpu_chunk_sz, 0);

	log_opts = OPTS_GET(opts, log_opts, NULL);
	if (!OPTS_VALID(log_opts, bpf_log_opts))
		return libbpf_err(-EINVAL);
}

Prior to this change, log_opts was the last field in struct
bpf_map_create_opts (at byte offset 72 on 64-bit platforms, with
sizeof(struct bpf_map_create_opts) == 80) and
bpf_map_create_opts__last_field was defined as log_opts:

tools/lib/bpf/bpf.h:
	struct bpf_log_opts *log_opts;

	size_t :0;
};
#define bpf_map_create_opts__last_field log_opts

Placing arena_percpu_chunk_off (offset 68) and arena_percpu_chunk_sz
(offset 72) ahead of log_opts shifts log_opts to offset 80 (with
sizeof(struct bpf_map_create_opts) == 88).

If an existing binary compiled against the previous struct
bpf_map_create_opts layout (opts->sz == 80, log_opts at offset 72) calls
the updated bpf_map_create(), OPTS_GET(opts, arena_percpu_chunk_sz, 0)
will read the lower 32 bits of the caller's log_opts pointer into
attr.arena_percpu_chunk_sz, while OPTS_GET(opts, log_opts, NULL) will see
opts->sz < offsetofend(struct bpf_map_create_opts, log_opts) and silently
drop log_opts.

Conversely, if a binary compiled against the new header sets
arena_percpu_chunk_sz and runs against an older libbpf.so, the older
bpf_map_create() will read the 32-bit arena_percpu_chunk_sz value at
offset 72 as a non-NULL struct bpf_log_opts *log_opts pointer and
dereference it in OPTS_VALID(log_opts, bpf_log_opts).

Should arena_percpu_chunk_off and arena_percpu_chunk_sz be appended after
log_opts, with bpf_map_create_opts__last_field updated to
arena_percpu_chunk_sz?

[ ... ]

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