Re: [PATCH v1 bpf-next 03/11] bpf, x86: add new map type: instructions array

[Eduard Zingerman <eddyz87@gmail.com>]

On Tue, 2025-08-26 at 15:52 +0000, Anton Protopopov wrote:
> On 25/08/25 02:05PM, Eduard Zingerman wrote:
> > On Sat, 2025-08-16 at 18:06 +0000, Anton Protopopov wrote:
> > 
> > [...]
> > 
> > > --- /dev/null
> > > +++ b/kernel/bpf/bpf_insn_array.c
> > 
> > [...]
> > 
> > > +int bpf_insn_array_ready(struct bpf_map *map)
> > > +{
> > > +	struct bpf_insn_array *insn_array = cast_insn_array(map);
> > > +	guard(mutex)(&insn_array->state_mutex);
> > > +	int i;
> > > +
> > > +	for (i = 0; i < map->max_entries; i++) {
> > > +		if (insn_array->ptrs[i].user_value.xlated_off == INSN_DELETED)
> > > +			continue;
> > > +		if (!insn_array->ips[i]) {
> > > +			/*
> > > +			 * Set the map free on failure; the program owning it
> > > +			 * might be re-loaded with different log level
> > > +			 */
> > > +			insn_array->state = INSN_ARRAY_STATE_FREE;
> > > +			return -EFAULT;
> > 
> > This shouldn't happen, right?
> > If so, maybe use verifier_bug here with some description?
> > (and move bpf_insn_array_ready() call to verifier.c:bpf_check(),
> >  so that the log pointer is available).
> 
> Shouldn't happen. But, unfortunately, only after
> bpf_prog_select_runtime() which is executed after bpf_check(). Might
> be nice to allow jit/bpf_prog_select_runtime to use the verifier
> environment.

The insn_array->ips array is filled by bpf_jit_comp.c:do_jit() ->
bpf_insn_array.c:bpf_prog_update_insn_ptr().

My initial thinking was that do_jit() is invoked by the following chain:
verifier.c:bpf_check() -> fixup_call_args() -> jit_subprogs().
Hence it appeared possible to move the above check/call to
bpf_insn_array_ready to bpf_check() itself.
However, looking at jit_subprogs() now I see:

        ...
        if (env->subprog_cnt <= 1)
                return 0;
        ...
	<proceeds jiting all subprogs including main>

So, it looks like the case when only main subprogram is present is
special. Oh, well.

> (Not 100% similar, but related to blinding part of this series:
> blinding is happening as the very first step of every jit (initially
> was implemented for x86, and then copy/pasted to everywhere else).
> Might be nice to move it to be one of the last stages of verifier,
> then code is shared and env is available as well. For this series I
> had to add a bit of custom code to support instruction arrays at this
> blinding stage.)
> 
> > > +		}
> > > +	}
> > > +
> > > +	insn_array->state = INSN_ARRAY_STATE_READY;
> > > +	return 0;
> > > +}
> > 
> > [...]