Re: [PATCH bpf-next 08/10] bpf: support precision propagation in the presence of subprogs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, May 04, 2023 at 03:19:08PM -0700, Andrii Nakryiko wrote:
> On Thu, May 4, 2023 at 12:41 PM Alexei Starovoitov
> <alexei.starovoitov@xxxxxxxxx> wrote:
> >
> > On Tue, Apr 25, 2023 at 04:49:09PM -0700, Andrii Nakryiko wrote:
> > >       if (insn->code == 0)
> > >               return 0;
> > > @@ -3424,14 +3449,72 @@ static int backtrack_insn(struct bpf_verifier_env *env, int idx,
> > >               if (class == BPF_STX)
> > >                       bt_set_reg(bt, sreg);
> > >       } else if (class == BPF_JMP || class == BPF_JMP32) {
> > > -             if (opcode == BPF_CALL) {
> > > -                     if (insn->src_reg == BPF_PSEUDO_CALL)
> > > -                             return -ENOTSUPP;
> > > -                     /* BPF helpers that invoke callback subprogs are
> > > -                      * equivalent to BPF_PSEUDO_CALL above
> > > +             if (bpf_pseudo_call(insn)) {
> > > +                     int subprog_insn_idx, subprog;
> > > +                     bool is_global;
> > > +
> > > +                     subprog_insn_idx = idx + insn->imm + 1;
> > > +                     subprog = find_subprog(env, subprog_insn_idx);
> > > +                     if (subprog < 0)
> > > +                             return -EFAULT;
> > > +                     is_global = subprog_is_global(env, subprog);
> > > +
> > > +                     if (is_global) {
> >
> > could you add a warn_on here that checks that jmp history doesn't have insns from subprog.
> 
> wouldn't this be very expensive to go over the entire jmp history to
> check that no jump point there overlaps with the global function? Or
> what do you have in mind specifically for this check?

recalling how jmp_history works and reading this comment when we process any call:
        /* when we exit from subprog, we need to record non-linear history */
        mark_jmp_point(env, t + 1);

so for static subprog the history will be:
call
  jmps inside subprog
insn after call.

for global it will be:
call
insn after call.

I was thinking that we can do simple check that call + 1 == subseq_idx for globals.
For statics that should never be the case.

We don't have to do it. Mostly checking my understanding of patches and jmp history.

> 
> >
> > > +                             /* r1-r5 are invalidated after subprog call,
> > > +                              * so for global func call it shouldn't be set
> > > +                              * anymore
> > > +                              */
> > > +                             if (bt_reg_mask(bt) & BPF_REGMASK_ARGS)
> > > +                                     return -EFAULT;
> >
> > This shouldn't be happening, but backtracking is delicate.
> > Could you add verbose("backtracking bug") here, so we know why the prog got rejected.
> > I'd probably do -ENOTSUPP, but EFAULT is ok too.
> 
> Will add verbose(). EFAULT because valid code should never use r1-r5
> after call. Invalid code should be rejected before that, and if not,
> then it is really a bug and best to bail out.
> 
> 
> >
> > > +                             /* global subprog always sets R0 */
> > > +                             bt_clear_reg(bt, BPF_REG_0);
> > > +                             return 0;
> > > +                     } else {
> > > +                             /* static subprog call instruction, which
> > > +                              * means that we are exiting current subprog,
> > > +                              * so only r1-r5 could be still requested as
> > > +                              * precise, r0 and r6-r10 or any stack slot in
> > > +                              * the current frame should be zero by now
> > > +                              */
> > > +                             if (bt_reg_mask(bt) & ~BPF_REGMASK_ARGS)
> > > +                                     return -EFAULT;
> >
> > same here.
> 
> ack
> 
> >
> > > +                             /* we don't track register spills perfectly,
> > > +                              * so fallback to force-precise instead of failing */
> > > +                             if (bt_stack_mask(bt) != 0)
> > > +                                     return -ENOTSUPP;
> > > +                             /* propagate r1-r5 to the caller */
> > > +                             for (i = BPF_REG_1; i <= BPF_REG_5; i++) {
> > > +                                     if (bt_is_reg_set(bt, i)) {
> > > +                                             bt_clear_reg(bt, i);
> > > +                                             bt_set_frame_reg(bt, bt->frame - 1, i);
> > > +                                     }
> > > +                             }
> > > +                             if (bt_subprog_exit(bt))
> > > +                                     return -EFAULT;
> > > +                             return 0;
> > > +                     }
> > > +             } else if ((bpf_helper_call(insn) &&
> > > +                         is_callback_calling_function(insn->imm) &&
> > > +                         !is_async_callback_calling_function(insn->imm)) ||
> > > +                        (bpf_pseudo_kfunc_call(insn) && is_callback_calling_kfunc(insn->imm))) {
> > > +                     /* callback-calling helper or kfunc call, which means
> > > +                      * we are exiting from subprog, but unlike the subprog
> > > +                      * call handling above, we shouldn't propagate
> > > +                      * precision of r1-r5 (if any requested), as they are
> > > +                      * not actually arguments passed directly to callback
> > > +                      * subprogs
> > >                        */
> > > -                     if (insn->src_reg == 0 && is_callback_calling_function(insn->imm))
> > > +                     if (bt_reg_mask(bt) & ~BPF_REGMASK_ARGS)
> > > +                             return -EFAULT;
> > > +                     if (bt_stack_mask(bt) != 0)
> > >                               return -ENOTSUPP;
> > > +                     /* clear r1-r5 in callback subprog's mask */
> > > +                     for (i = BPF_REG_1; i <= BPF_REG_5; i++)
> > > +                             bt_clear_reg(bt, i);
> > > +                     if (bt_subprog_exit(bt))
> > > +                             return -EFAULT;
> > > +                     return 0;
> >
> > jmp history will include callback insn, right?
> > So skip of jmp history is missing here ?
> 
> This is, say, `call bpf_loop;` instruction. Which means we just got
> out of bpf_loop's callback's jump history (so already "skipped" them,
> except we didn't skip, we properly processed them). So there is
> nothing to skip anymore. We are in the parent program already.

Got it. Makes sense now.




[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux