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/03 01:48AM, Eduard Zingerman wrote:
> On Fri, 2025-10-03 at 07:39 +0000, Anton Protopopov wrote:
> > 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.
> 
> But why does map element size has to be different between kernel and user?
> For internal use there is an `ips` array and that has to be 64-bit.
> For external use, it appears that any structure can be used.
> I probably don't understand something.

I think that map->value_size is expected to be  between
user space and kernel code (verifier). For this map we
use PTR_TO_MAP_VALUE, etc.

But before diving deeper into this topic, what is missing, really?
The orig->xlated mapping is easy to keep from userspace, if needed,
why is this not sufficient?

> > (Userspace can build the orig_off -> xlated_off mapping easily, if needed,
> > just keep a copy of the map before the load.)
> 
> [...]
> 
> > > > +#define MAX_INSN_ARRAY_ENTRIES 256
> > >
> > > Hm, did not notice this before.  We probably need an option limiting
> > > max number of jump table alternatives.
> 
> (I meant LLVM option, but you probably inferred)
> 
> > >
> > > 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?
> 
> This generates a 4K jump table for me:
> 
>   $ cat gen-foo.py
>   import random as r
> 
>   print('int foo(int i) {')
>   print('  switch(i) {')
>   for c in r.sample(range(0, 4096), 1024):
>       print(f'  case {c}: return {r.randint(-10000, 10000)};')
>   print('  }')
>   print('  return 0;')
>   print('}')
> 
>   $ python3 gen-foo.py | clang -xc -O2 -S -o - - | grep '.quad' | wc -l
>   4093

Thanks for the example.

> [...]
> 
> > > > +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.
> 
> Missed this part:
> 
> > 'If map was changed, it now will point to incorrect instructions.'
> 
> Makes sense, thank you for explaining.
> 
> [...]