Re: [PATCH v9 bpf-next 01/11] bpf, x86: add new map type: instructions array

[Anton Protopopov <a.s.protopopov@gmail.com>]

On 25/11/01 11:29AM, bot+bpf-ci@kernel.org wrote:
> 
> ```
> commit 77dc1e7091f8bbd42ffa1730c0339642333528c6
> Author: Anton Protopopov <a.s.protopopov@gmail.com>
> 
> bpf, x86: add new map type: instructions array
> 
> This patch adds BPF_MAP_TYPE_INSN_ARRAY to track instruction offset
> mappings (original -> xlated -> jitted) during BPF program verification
> and JIT compilation.
> 
> > diff --git a/kernel/bpf/bpf_insn_array.c b/kernel/bpf/bpf_insn_array.c
> > new file mode 100644
> > index 000000000..d29f38721
> > --- /dev/null
> > +++ b/kernel/bpf/bpf_insn_array.c
> 
> [ ... ]
> 
> > @@ -0,0 +1,286 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/* Copyright (c) 2025 Isovalent */
> > +
> > +#include <linux/bpf.h>
> > +
> > +struct bpf_insn_array {
> > +	struct bpf_map map;
> > +	atomic_t used;
> > +	long *ips;
> > +	DECLARE_FLEX_ARRAY(struct bpf_insn_array_value, values);
> > +};
> > +
> > +#define cast_insn_array(MAP_PTR) \
> > +	container_of((MAP_PTR), struct bpf_insn_array, map)
> > +
> > +#define INSN_DELETED ((u32)-1)
> > +
> > +static inline u32 insn_array_alloc_size(u32 max_entries)
> > +{
> > +	const u32 base_size = sizeof(struct bpf_insn_array);
> > +	const u32 entry_size = sizeof(struct bpf_insn_array_value);
> > +
> > +	return base_size + max_entries * (entry_size + sizeof(long));
>                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > +}
> 
> Can this overflow u32? With entry_size=16 and sizeof(long)=8 on 64-bit,
> max_entries around 200 million would cause: 200M * 24 = 4.8B which wraps
> to around 504MB when cast to u32. The subsequent bpf_map_area_alloc()
> would then allocate only 504MB, but later accesses to values[150000000]
> would be out of bounds. Should this function return u64 instead, or
> should insn_array_alloc_check() validate an upper bound on max_entries?
> 
> [ ... ]

Yes, it can now, thanks AI. (The latest verion removed the limit check
for the max_entires, set previously to 256.) I will convert types to
u64 in insn_array_alloc_size as all the call sites use u64 in any case.

> > +static int insn_array_alloc_check(union bpf_attr *attr)
> > +{
> > +	u32 value_size = sizeof(struct bpf_insn_array_value);
> > +
> > +	if (attr->max_entries == 0 || attr->key_size != 4 ||
> > +	    attr->value_size != value_size || attr->map_flags != 0)
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> 
> [ ... ]
> 
> > +void bpf_prog_update_insn_ptrs(struct bpf_prog *prog, u32 *offsets, void *image)
> > +{
> > +	struct bpf_insn_array *insn_array;
> > +	struct bpf_map *map;
> > +	u32 xlated_off;
> > +	int i, j;
> > +
> > +	if (!offsets || !image)
> > +		return;
> > +
> > +	for (i = 0; i < prog->aux->used_map_cnt; i++) {
> > +		map = prog->aux->used_maps[i];
> > +		if (!is_insn_array(map))
> > +			continue;
> > +
> > +		insn_array = cast_insn_array(map);
> > +		for (j = 0; j < map->max_entries; j++) {
> > +			xlated_off = insn_array->values[j].xlated_off;
> > +			if (xlated_off == INSN_DELETED)
> > +				continue;
> > +			if (xlated_off < prog->aux->subprog_start)
> > +				continue;
> > +			xlated_off -= prog->aux->subprog_start;
> > +			if (xlated_off >= prog->len)
> > +				continue;
> > +
> > +			insn_array->values[j].jitted_off = offsets[xlated_off];
> > +			insn_array->ips[j] = (long)(image + offsets[xlated_off]);
> > +		}
> > +	}
> > +}
> 
> Does this handle programs with function calls correctly? When
> jit_subprogs() in the verifier splits a program with function calls, it
> creates separate bpf_prog structures for the main program and each
> subprogram. During JIT compilation, bpf_int_jit_compile() calls
> bpf_prog_update_insn_ptrs() for each subprogram separately.
> 
> However, subprograms don't have used_maps copied to their aux structure

They have. However, this change is introduced in a later commit.
One thing the robot is right about is that it should have been done
as part of this patch. Will move these chunks here.

> (see jit_subprogs() in verifier.c). So when bpf_prog_update_insn_ptrs()
> is called on a subprogram, prog->aux->used_map_cnt is 0 and the loop
> doesn't execute.
> 
> When called on the main program (func[0]), prog->len only covers the
> main program's instructions (not subprograms), so any tracked instruction
> with xlated_off >= prog->len (i.e., in a subprogram) hits the continue
> statement and doesn't get its ips[] field set.
> 
> Then bpf_insn_array_ready() checks if ips[i] is NULL and returns -EFAULT,
> causing program load to fail. Should this function iterate through all
> func[] entries and handle each subprogram, or should the main program's
> used_maps be copied to subprograms so this works as-is?
> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> 
> [ ... ]
> 
> > @@ -2853,6 +2854,23 @@ static int bpf_prog_verify_signature(struct bpf_prog *prog, union bpf_attr *attr
> >  	return err;
> >  }
> >
> > +static int bpf_prog_mark_insn_arrays_ready(struct bpf_prog *prog)
> > +{
> > +	int err;
> > +	int i;
> > +
> > +	for (i = 0; i < prog->aux->used_map_cnt; i++) {
> > +		if (prog->aux->used_maps[i]->map_type != BPF_MAP_TYPE_INSN_ARRAY)
> > +			continue;
> > +
> > +		err = bpf_insn_array_ready(prog->aux->used_maps[i]);
> > +		if (err)
> > +			return err;
> > +	}
> > +
> > +	return 0;
> > +}
> 
> [ ... ]
> 
> 
> ```
> 
> ---
> 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
> 
> In-Reply-To-Subject: `bpf, x86: add new map type: instructions array`
> CI run summary: https://github.com/kernel-patches/bpf/actions/runs/18995945884