Re: [RFC bpf-next 2/9] bpf, x86: add new map type: instructions set

[Eduard Zingerman <eddyz87@gmail.com>]

On Sun, 2025-06-15 at 08:59 +0000, Anton Protopopov wrote:

Meta: "instruction set" is a super confusing name, at-least for me the
      first thought is about actual set of instructions supported by
      some h/w. instruction_info? instruction_offset? just
      "iset"/"ioffset"?

[...]

> On map creation/initialization, before loading the program, each
> element of the map should be initialized to point to an instruction
> offset within the program. Before the program load such maps should
> be made frozen. After the program verification xlated and jitted
> offsets can be read via the bpf(2) syscall.

I think such maps would be a bit more ergonomic it original
instruction index would be saved as well, e.g:

  (original_offset, xlated_offset, jitted_offset)

Otherwise user would have to recover original offset from some
external mapping. This information is stored in orig_xlated_off
anyway.

[...]

> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 15672cb926fc..923c38f212dc 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -1615,6 +1615,8 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image, u8 *rw_image

[...]

> @@ -2642,6 +2645,14 @@ st:			if (is_imm8(insn->off))
>  				return -EFAULT;
>  			}
>  			memcpy(rw_image + proglen, temp, ilen);
> +
> +			/*
> +			 * Instruction sets need to know how xlated code
> +			 * maps to jited code
> +			 */
> +			abs_xlated_off = bpf_prog->aux->subprog_start + i - 1 - adjust_off;

Nit: `adjust_off` is a bit hard to follow, maybe move the following:

	abs_xlated_off = bpf_prog->aux->subprog_start + i - 1;

     to the beginning of the loop?

> +			bpf_prog_update_insn_ptr(bpf_prog, abs_xlated_off, proglen, ilen,
> +						 jmp_offset, image + proglen);

Nit: initialize `jmp_offset` at each loop iteration to 0?
     otherwise it would denote jump offset of the last processed
     jump instruction for all following non-jump instructions.

>  		}
>  		proglen += ilen;
>  		addrs[i] = proglen;
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 8189f49e43d6..008bcd44c60e 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -3596,4 +3596,25 @@ static inline bool bpf_is_subprog(const struct bpf_prog *prog)
>  	return prog->aux->func_idx != 0;
>  }
>
> +int bpf_insn_set_init(struct bpf_map *map, const struct bpf_prog *prog);
> +int bpf_insn_set_ready(struct bpf_map *map);
> +void bpf_insn_set_release(struct bpf_map *map);
> +void bpf_insn_set_adjust(struct bpf_map *map, u32 off, u32 len);
> +void bpf_insn_set_adjust_after_remove(struct bpf_map *map, u32 off, u32 len);
> +
> +struct bpf_insn_ptr {

Could you please add comments describing each field?
E.g.: "address of the instruction in the jitted image",
      "for jump instructions, the relative offset of the jump target",
      "index of the original instruction",
      "original value of the corresponding bpf_insn_set_value.xlated_off".

> +	void *jitted_ip;
> +	u32 jitted_len;
> +	int jitted_jump_offset;
> +	struct bpf_insn_set_value user_value; /* userspace-visible value */
> +	u32 orig_xlated_off;
> +};

[...]

> diff --git a/kernel/bpf/bpf_insn_set.c b/kernel/bpf/bpf_insn_set.c
> new file mode 100644

[...]

> +static int insn_set_check_btf(const struct bpf_map *map,
> +			      const struct btf *btf,
> +			      const struct btf_type *key_type,
> +			      const struct btf_type *value_type)
> +{
> +	u32 int_data;
> +
> +	if (BTF_INFO_KIND(key_type->info) != BTF_KIND_INT)
> +		return -EINVAL;
> +
> +	if (BTF_INFO_KIND(value_type->info) != BTF_KIND_INT)
> +		return -EINVAL;
> +
> +	int_data = *(u32 *)(key_type + 1);

Nit: use btf_type_int() accessor?

> +	if (BTF_INT_BITS(int_data) != 32 || BTF_INT_OFFSET(int_data))
> +		return -EINVAL;
> +
> +	int_data = *(u32 *)(value_type + 1);
> +	if (BTF_INT_BITS(int_data) != 32 || BTF_INT_OFFSET(int_data))

Should this check for `BTF_INT_BITS(int_data) != 64`?

> +		return -EINVAL;
> +
> +	return 0;
> +}

[...]

> +int bpf_insn_set_init(struct bpf_map *map, const struct bpf_prog *prog)
> +{
> +	struct bpf_insn_set *insn_set = cast_insn_set(map);
> +	int i;
> +
> +	if (!is_frozen(map))
> +		return -EINVAL;
> +
> +	if (!valid_offsets(insn_set, prog))
> +		return -EINVAL;
> +
> +	/*
> +	 * There can be only one program using the map
> +	 */
> +	mutex_lock(&insn_set->state_mutex);
> +	if (insn_set->state != INSN_SET_STATE_FREE) {
> +		mutex_unlock(&insn_set->state_mutex);
> +		return -EBUSY;
> +	}
> +	insn_set->state = INSN_SET_STATE_INIT;
> +	mutex_unlock(&insn_set->state_mutex);
> +
> +	/*
> +	 * Reset all the map indexes to the original values.  This is needed,
> +	 * e.g., when a replay of verification with different log level should
> +	 * be performed.
> +	 */
> +	for (i = 0; i < map->max_entries; i++)
> +		insn_set->ptrs[i].user_value.xlated_off = insn_set->ptrs[i].orig_xlated_off;
> +
> +	return 0;
> +}
> +
> +int bpf_insn_set_ready(struct bpf_map *map)

What is the reasoning for not needing to take the mutex here and in
the bpf_insn_set_release?

> +{
> +	struct bpf_insn_set *insn_set = cast_insn_set(map);
> +	int i;
> +
> +	for (i = 0; i < map->max_entries; i++) {
> +		if (insn_set->ptrs[i].user_value.xlated_off == INSN_DELETED)
> +			continue;
> +		if (!insn_set->ips[i])
> +			return -EFAULT;
> +	}
> +
> +	insn_set->state = INSN_SET_STATE_READY;
> +	return 0;
> +}
> +
> +void bpf_insn_set_release(struct bpf_map *map)
> +{
> +	struct bpf_insn_set *insn_set = cast_insn_set(map);
> +
> +	insn_set->state = INSN_SET_STATE_FREE;
> +}

[...]

(... I'll continue reading through patch-set a bit later ...)