Re: [PATCH v5 bpf-next 04/15] bpf, x86: add new map type: instructions array

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

On 25/10/02 05:50PM, Eduard Zingerman wrote:
> On Tue, 2025-09-30 at 12:51 +0000, Anton Protopopov wrote:
> 
> Overall I think this patch is fine.
> We discussed this some time ago, but I can't find the previous discussion:
> would it be possible to make this map element a tuple of three elements
> (orig_off, xlated_off, jitted_off)?
> Visible to user as well.

See https://lore.kernel.org/bpf/8ff2059d38afbd49eccb4bb3fd5ba741fefc5b57.camel@gmail.com/

In short, this will make the map element to be of different size
from userspace and kernel (BPF) perspective.

(Userspace can build the orig_off -> xlated_off mapping easily, if needed,
just keep a copy of the map before the load.)

> [...]
> 
> > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> > index 4c497e839526..cc06e6d57faa 100644
> > --- a/include/linux/bpf_verifier.h
> > +++ b/include/linux/bpf_verifier.h
> 
> [...]
> 
> > @@ -7645,4 +7646,14 @@ enum bpf_kfunc_flags {
> >  	BPF_F_PAD_ZEROS = (1ULL << 0),
> >  };
> >  
> > +/*
> > + * Values of a BPF_MAP_TYPE_INSN_ARRAY entry must be of this type.
> > + * On updates jitted_off must be equal to 0.
> > + */
> > +struct bpf_insn_array_value {
> > +	__u32 jitted_off;
> > +	__u32 xlated_off;
> > +};
> 
> Could you please expand the comment a bit?  Describe the meaning of
> the fields both before and after program load.

Sure.

> > +
> > +
> >  #endif /* _UAPI__LINUX_BPF_H__ */
> 
> [...]
> 
> > @@ -0,0 +1,285 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/* Copyright (c) 2025 Isovalent */
> > +
> > +#include <linux/bpf.h>
> > +
> > +#define MAX_INSN_ARRAY_ENTRIES 256
> 
> Hm, did not notice this before.  We probably need an option limiting
> max number of jump table alternatives.
> 
> Yonghong, wdyt?

This one comes from the fact I've mentioned in the other place: need
to optimize the lookup from jit (not it is brute force). Then this
limitation will go away.

But also curious, what LLVM thinks about this. Will it,
theoretically, create say 65K tables or so?

> [...]
> 
> > +void bpf_insn_array_adjust_after_remove(struct bpf_map *map, u32 off, u32 len)
> > +{
> > +	struct bpf_insn_array *insn_array = cast_insn_array(map);
> > +	int i;
> > +
> > +	for (i = 0; i < map->max_entries; i++) {
> > +		if (insn_array->ptrs[i].user_value.xlated_off < off)
> > +			continue;
> > +		if (insn_array->ptrs[i].user_value.xlated_off == INSN_DELETED)
> > +			continue;
> > +		if (insn_array->ptrs[i].user_value.xlated_off >= off &&
>                                                               ^^^^^^
> Nit:                                       this condition is redundant

True, thanks, will remove it.

> 
> > +		    insn_array->ptrs[i].user_value.xlated_off < off + len)
> > +			insn_array->ptrs[i].user_value.xlated_off = INSN_DELETED;
> > +		else
> > +			insn_array->ptrs[i].user_value.xlated_off -= len;
> > +	}
> > +}
> > +
> > +void bpf_prog_update_insn_ptr(struct bpf_prog *prog,
> > +			      u32 xlated_off,
> > +			      u32 jitted_off,
> > +			      void *jitted_ip)
> > +{
> > +	struct bpf_insn_array *insn_array;
> > +	struct bpf_map *map;
> > +	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++) {
> > +			if (insn_array->ptrs[j].user_value.xlated_off == xlated_off) {
> 
> If this would check for `insn_array->ptrs[j].orig_xlated_off == xlated_off`
> there would be no need in `user_value.xlated_off = orig_xlated_off`
> in the `bpf_insn_array_init()`, right?

The copy of the original offset is kept inside the map for the
following reason.  When the map is first loaded, it is frozen. Thus
user can't update it anymore.  During load some of xlated_off are
changed (together with program code). If the program load fails, it
is common to reload it with a log buffer. If map was changed, it now
will point to incorrect instructions. So in this case the map should
be seen as the original one, and the orig_xlated_off is used to reset
it.

> > +				insn_array->ips[j] = (long)jitted_ip;
> > +				insn_array->ptrs[j].jitted_ip = jitted_ip;
> > +				insn_array->ptrs[j].user_value.jitted_off = jitted_off;
> > +			}
> > +		}
> > +	}
> > +}
> 
> [...]