Re: [PATCH v3 bpf-next 05/13] bpf: support instructions arrays with constants blinding

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

On 25/09/22 09:28AM, Anton Protopopov wrote:
> On 25/09/19 01:47PM, Eduard Zingerman wrote:
> > On Fri, 2025-09-19 at 20:27 +0000, Anton Protopopov wrote:
> > > On 25/09/19 12:44PM, Eduard Zingerman wrote:
> > > > On Fri, 2025-09-19 at 21:28 +0200, Daniel Borkmann wrote:
> > > > > On 9/19/25 8:26 PM, Alexei Starovoitov wrote:
> > > > > > On Fri, Sep 19, 2025 at 12:12 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > > > > > > On Fri, 2025-09-19 at 07:05 +0000, Anton Protopopov wrote:
> > > > > > > > On 25/09/18 11:35PM, Eduard Zingerman wrote:
> > > > > > > > > On Thu, 2025-09-18 at 09:38 +0000, Anton Protopopov wrote:
> > > > > > > > > 
> > > > > > > > > [...]
> > > > > > > > > 
> > > > > > > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > > > > > > > > index a7ad4fe756da..5c1e4e37d1f8 100644
> > > > > > > > > > --- a/kernel/bpf/verifier.c
> > > > > > > > > > +++ b/kernel/bpf/verifier.c
> > > > > > > > > > @@ -21578,6 +21578,7 @@ static int jit_subprogs(struct bpf_verifier_env *env)
> > > > > > > > > >    struct bpf_insn *insn;
> > > > > > > > > >    void *old_bpf_func;
> > > > > > > > > >    int err, num_exentries;
> > > > > > > > > > + int old_len, subprog_start_adjustment = 0;
> > > > > > > > > > 
> > > > > > > > > >    if (env->subprog_cnt <= 1)
> > > > > > > > > >            return 0;
> > > > > > > > > > @@ -21652,7 +21653,7 @@ static int jit_subprogs(struct bpf_verifier_env *env)
> > > > > > > > > >            func[i]->aux->func_idx = i;
> > > > > > > > > >            /* Below members will be freed only at prog->aux */
> > > > > > > > > >            func[i]->aux->btf = prog->aux->btf;
> > > > > > > > > > -         func[i]->aux->subprog_start = subprog_start;
> > > > > > > > > > +         func[i]->aux->subprog_start = subprog_start + subprog_start_adjustment;
> > > > > > > > > >            func[i]->aux->func_info = prog->aux->func_info;
> > > > > > > > > >            func[i]->aux->func_info_cnt = prog->aux->func_info_cnt;
> > > > > > > > > >            func[i]->aux->poke_tab = prog->aux->poke_tab;
> > > > > > > > > > @@ -21705,7 +21706,15 @@ static int jit_subprogs(struct bpf_verifier_env *env)
> > > > > > > > > >            func[i]->aux->might_sleep = env->subprog_info[i].might_sleep;
> > > > > > > > > >            if (!i)
> > > > > > > > > >                    func[i]->aux->exception_boundary = env->seen_exception;
> > > > > > > > > > +
> > > > > > > > > > +         /*
> > > > > > > > > > +          * To properly pass the absolute subprog start to jit
> > > > > > > > > > +          * all instruction adjustments should be accumulated
> > > > > > > > > > +          */
> > > > > > > > > > +         old_len = func[i]->len;
> > > > > > > > > >            func[i] = bpf_int_jit_compile(func[i]);
> > > > > > > > > > +         subprog_start_adjustment += func[i]->len - old_len;
> > > > > > > > > > +
> > > > > > > > > >            if (!func[i]->jited) {
> > > > > > > > > >                    err = -ENOTSUPP;
> > > > > > > > > >                    goto out_free;
> > > > > > > > > 
> > > > > > > > > This change makes sense, however, would it be possible to move
> > > > > > > > > bpf_jit_blind_constants() out from jit to verifier.c:do_check,
> > > > > > > > > somewhere after do_misc_fixups?
> > > > > > > > > Looking at the source code, bpf_jit_blind_constants() is the first
> > > > > > > > > thing any bpf_int_jit_compile() does.
> > > > > > > > > Another alternative is to add adjust_subprog_starts() call to this
> > > > > > > > > function. Wdyt?
> > > > > > > > 
> > > > > > > > Yes, it makes total sense. Blinding was added to x86 jit initially and then
> > > > > > > > every other jit copy-pasted it.  I was considering to move blinding up some
> > > > > > > > time back (see https://lore.kernel.org/bpf/20250318143318.656785-1-aspsk@isovalent.com/),
> > > > > > > > but then I've decided to avoid this, as this requires to patch every JIT, and I
> > > > > > > > am not sure what is the way to test such a change (any hints?)
> > > > > > > 
> > > > > > > We have the following covered by CI:
> > > > > > > - arch/x86/net/bpf_jit_comp.c
> > > > > > > - arch/s390/net/bpf_jit_comp.c
> > > > > > > - arch/arm64/net/bpf_jit_comp.c
> > > > > > > 
> > > > > > > People work on these jits actively:
> > > > > > > - arch/riscv/net/bpf_jit_core.c
> > > > > > > - arch/loongarch/net/bpf_jit.c
> > > > > > > - arch/powerpc/net/bpf_jit_comp.c
> > > > > > > 
> > > > > > > So, we can probably ask to test the patch-set.
> > > > > > > 
> > > > > > > The remaining are:
> > > > > > > - arch/x86/net/bpf_jit_comp32.c
> > > > > > > - arch/parisc/net/bpf_jit_core.c
> > > > > > > - arch/mips/net/bpf_jit_comp.c
> > > > > > > - arch/arm/net/bpf_jit_32.c
> > > > > > > - arch/sparc/net/bpf_jit_comp_64.c
> > > > > > > - arch/arc/net/bpf_jit_core.c
> > > > > > > 
> > > > > > > The change to each individual jit is not complicated, just removing
> > > > > > > the transformation call. Idk, I'd just go for it.
> > > > > > > Maybe Alexei has concerns?
> > > > > > 
> > > > > > No concerns.
> > > > > > I don't remember why JIT calls it instead of the verifier.
> > > > > > 
> > > > > > Daniel,
> > > > > > do you recall? Any concern?
> > > > > 
> > > > > Hm, I think we did this in the JIT back then for couple of reasons iirc,
> > > > > the constant blinding needs to work from native bpf(2) as well as from
> > > > > cbpf->ebpf (seccomp-bpf, filters, etc), so the JIT was a natural location
> > > > > to capture them all, and to fallback to interpreter with the non-blinded
> > > > > BPF-insns when something went wrong during blinding or JIT process (e.g.
> > > > > JIT hits some internal limits etc). Moving bpf_jit_blind_constants() out
> > > > > from JIT to verifier.c:do_check() means constant blinding of cbpf->ebpf
> > > > > are not covered anymore (and in this case its reachable from unpriv).
> > > > 
> > > > Hi Daniel,
> > > > 
> > > > Thank you for the context.
> > > > So, the ideal location for bpf_jit_blind_constants() would be in
> > > > core.c in some wrapper function for bpf_int_jit_compile():
> > > > 
> > > >   static struct bpf_prog *jit_compile(prog)
> > > >   {
> > > >   	tmp = bpf_jit_blind_constants()
> > > >         if (!tmp)
> > > >            return prog;
> > > >         return bpf_int_jit_compile(tmp);
> > > >   }
> > > > 
> > > > A bit of a hassle.
> > > > 
> > > > Anton, wdyt about a second option: adding adjust_subprog_starts()
> > > > to bpf_jit_blind_constants() and leaving all the rest as-is?
> > > > It would have to happen either way of call to bpf_jit_blind_constants()
> > > > itself is moved.
> > > 
> > > So, to be clear, in this case adjust_insn_arrays() stays as in the
> > > original patch, but the "subprog_start_adjustment" chunks are
> > > replaced by calling the adjust_subprog_starts() (for better
> > > readability and consistency, right?)
> > 
> > Yes, by adding adjust_subprog_starts() call inside
> > bpf_jit_blind_constants() it should be possible to read
> > env->subprog_info[*].start in the jit_subprogs() loop directly,
> > w/o tracking the subprog_start_adjustment delta.
> > (At-least I think this should work).
> 
> Ok, will do this way, thanks.

Actually, I think I will skip it this time. During jit_subprogs
the code of the original program is split into subfuncs via the
_unchanged_ subprog info, as the xlated code is copied for each
new subprog in the loop. So this "adjustment" thing will appear
in some form in any case.

Also, doing adjust_subprog_starts() requires passing env to jits,
which wasn't done yet, and needs to be faked for non-ebpf progs,
I think. So maybe this is better to cleanup/generalize this later,
not as part of this patch.