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

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

On 25/10/27 02:44PM, Eduard Zingerman wrote:
> On Sun, 2025-10-26 at 19:26 +0000, Anton Protopopov wrote:
> 
> [...]
> 
> > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> > index d4c93d9e73e4..d8ee0c4d9af8 100644
> > --- a/arch/x86/net/bpf_jit_comp.c
> > +++ b/arch/x86/net/bpf_jit_comp.c
> > @@ -3648,6 +3648,22 @@ struct x64_jit_data {
> >  	struct jit_context ctx;
> >  };
> >  
> > +struct insn_ptrs_data {
> > +	int *addrs;
> > +	u8 *image;
> > +};
> > +
> > +static void update_insn_ptr(void *jit_priv, u32 xlated_off, u32 *jitted_off, long *ip)
> > +{
> > +	struct insn_ptrs_data *data = jit_priv;
> > +
> > +	if (!data->addrs || !data->image || !jitted_off || !ip)
> > +		return;
> > +
> > +	*jitted_off = data->addrs[xlated_off];
> > +	*ip = (long)(data->image + *jitted_off);
> > +}
> > +
> >  #define MAX_PASSES 20
> >  #define PADDING_PASSES (MAX_PASSES - 5)
> >  
> > @@ -3658,6 +3674,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
> >  	struct bpf_prog *tmp, *orig_prog = prog;
> >  	void __percpu *priv_stack_ptr = NULL;
> >  	struct x64_jit_data *jit_data;
> > +	struct insn_ptrs_data insn_ptrs_data;
> >  	int priv_stack_alloc_sz;
> >  	int proglen, oldproglen = 0;
> >  	struct jit_context ctx = {};
> > @@ -3827,6 +3844,12 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
> >  			jit_data->header = header;
> >  			jit_data->rw_header = rw_header;
> >  		}
> > +
> > +		/* jit_data may not contain proper info, copy the required fields */
> > +		insn_ptrs_data.addrs = addrs;
> > +		insn_ptrs_data.image = image;
> > +		bpf_prog_update_insn_ptrs(prog, &insn_ptrs_data, update_insn_ptr);
> > +
> >  		/*
> >  		 * ctx.prog_offset is used when CFI preambles put code *before*
> >  		 * the function. See emit_cfi(). For FineIBT specifically this code
> 
> [...]
> 
> > diff --git a/kernel/bpf/bpf_insn_array.c b/kernel/bpf/bpf_insn_array.c
> > new file mode 100644
> > index 000000000000..f9f875ee2027
> > +++ b/kernel/bpf/bpf_insn_array.c
> 
> [...]
> 
> > +void bpf_prog_update_insn_ptrs(struct bpf_prog *prog, void *jit_priv,
> > +			       update_insn_ptr_func_t update_insn_ptr)
> > +{
> 
> Nit: I think the control flow becomes a bit convoluted with this
>      function pointer. Wdyt about changing the signature to:
> 
>        void bpf_prog_update_insn_ptrs(struct bpf_prog *prog,
>                                       u32 *offsets /* maps xlated_off to offset in image */,
>                                       void *image)
> 
>      x86 jit provides this info, it looks like arm64 and riscv jits do too
>      (arch/arm64/net/bpf_jit_comp.c:jit_ctx->offset field,
>       arch/riscv/net/bpf_jit.h:rv_jit_context->offset).
>      So, seem to be a reasonable assumption.
> 
>      Wdyt?

I did it a bit more abstract variant right away because in future
(read "static keys") there will be more data passed around.  I will
switch to your variant, and then, once I follow up with static keys,
it can be generalized, if needed.

> > +	struct bpf_insn_array *insn_array;
> > +	struct bpf_map *map;
> > +	u32 xlated_off;
> > +	int i, j;
> > +
> > +	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;
> > +
> > +			update_insn_ptr(jit_priv, xlated_off,
> > +					&insn_array->values[j].jitted_off,
> > +					&insn_array->ips[j]);
> > +		}
> > +	}
> > +}
> 
> [...]