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;
> > > +}
> >
> > [...]
