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

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

On 25/10/21 04:26PM, Eduard Zingerman wrote:
> On Sun, 2025-10-19 at 20:21 +0000, Anton Protopopov wrote:
> 
> [...]
> 
> > The functionality provided by this patch will be extended in consequent
> > patches to implement BPF Static Keys, indirect jumps, and indirect calls.
> > 
> > Signed-off-by: Anton Protopopov <a.s.protopopov@gmail.com>
> > ---
> 
> Aside from what Alexei pointed out, I only have a nit regarding jitted_ip.
> 
> Reviewed-by: Eduard Zingerman <eddyz87@gmail.com>
> 
> [...]
> 
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index e53cda0aabb6..363355628d2e 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -3789,4 +3789,40 @@ int bpf_prog_get_file_line(struct bpf_prog *prog, unsigned long ip, const char *
> >  			   const char **linep, int *nump);
> 
> [...]
> 
> > +/*
> > + * The struct bpf_insn_ptr structure describes a pointer to a
> > + * particular instruction in a loaded BPF program. Initially
> > + * it is initialised from userspace via user_value.xlated_off.
> > + * During the program verification all other fields are populated
> > + * accordingly:
> > + *
> > + *   jitted_ip:       address of the instruction in the jitted image
> > + *   user_value:      user-visible original, xlated, and jitted offsets
> > + */
> > +struct bpf_insn_ptr {
> > +	void *jitted_ip;
> 
> I think this one is no longer used anywhere.
> I see it set in bpf_prog_update_insn_ptr() but it is not read anywhere.

Nice, thanks :)

> > +	struct bpf_insn_array_value user_value;
> > +};
> > +
> 
> [...]
> 
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 6829936d33f5..805d441363cd 100644
> 
> [...]
> 
> > @@ -7645,4 +7646,24 @@ enum bpf_kfunc_flags {
> >  	BPF_F_PAD_ZEROS = (1ULL << 0),
> >  };
> >  
> > +/*
> > + * Values of a BPF_MAP_TYPE_INSN_ARRAY entry must be of this type.
> > + *
> > + * Before the map is used the orig_off field should point to an
> > + * instruction inside the program being loaded. The other fields
> > + * must be set to 0.
> > + *
> > + * After the program is loaded, the xlated_off will be adjusted
> > + * by the verifier to point to the index of the original instruction
> > + * in the xlated program. If the instruction is deleted, it will
> > + * be set to (u32)-1. The jitted_off will be set to the corresponding
> > + * offset in the jitted image of the program.
> > + */
> > +struct bpf_insn_array_value {
> > +	__u32 orig_off;
> > +	__u32 xlated_off;
> > +	__u32 jitted_off;
> > +	__u32 :32;
> 
> This :u32, is it for alignment or for future extensibility?

Both. We maybe might have some flags added in future for different use cases...

> > +
> 
> [...]
> 
> > diff --git a/kernel/bpf/bpf_insn_array.c b/kernel/bpf/bpf_insn_array.c
> > new file mode 100644
> 
> [...]
> 
> > @@ -0,0 +1,288 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/* Copyright (c) 2025 Isovalent */
> > +
> > +#include <linux/bpf.h>
> > +
> > +#define MAX_INSN_ARRAY_ENTRIES 256
> > +
> > +struct bpf_insn_array {
> > +	struct bpf_map map;
> > +	atomic_t used;
> > +	long *ips;
> > +	DECLARE_FLEX_ARRAY(struct bpf_insn_ptr, ptrs);
> > +};
> 
> [...]
> 
> > +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_ptr);
> > +
> > +	return base_size + entry_size * max_entries;
> > +}
> 
> Since this is doing a flexible array thing anyway, maybe also include
> size for `ips` here? And in insn_array_alloc() point it at the tail
> area.

Yes, great idea.

> [...]