On Tue, Oct 31, 2023 at 9:24 AM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Mon, Oct 30, 2023 at 11:03 PM Andrii Nakryiko
> <andrii.nakryiko@xxxxxxxxx> wrote:
> >
> > On Mon, Oct 30, 2023 at 7:02 PM Alexei Starovoitov
> > <alexei.starovoitov@xxxxxxxxx> wrote:
> > >
> > > On Fri, Oct 27, 2023 at 11:13:40AM -0700, Andrii Nakryiko wrote:
> > > > static void reg_set_min_max(struct bpf_reg_state *true_reg1,
> > > > + struct bpf_reg_state *true_reg2,
> > > > struct bpf_reg_state *false_reg1,
> > > > - u64 val, u32 val32,
> > > > + struct bpf_reg_state *false_reg2,
> > > > u8 opcode, bool is_jmp32)
> > > > {
> > > > - struct tnum false_32off = tnum_subreg(false_reg1->var_off);
> > > > - struct tnum false_64off = false_reg1->var_off;
> > > > - struct tnum true_32off = tnum_subreg(true_reg1->var_off);
> > > > - struct tnum true_64off = true_reg1->var_off;
> > > > - s64 sval = (s64)val;
> > > > - s32 sval32 = (s32)val32;
> > > > -
> > > > - /* If the dst_reg is a pointer, we can't learn anything about its
> > > > - * variable offset from the compare (unless src_reg were a pointer into
> > > > - * the same object, but we don't bother with that.
> > > > - * Since false_reg1 and true_reg1 have the same type by construction, we
> > > > - * only need to check one of them for pointerness.
> > > > + struct tnum false_32off, false_64off;
> > > > + struct tnum true_32off, true_64off;
> > > > + u64 val;
> > > > + u32 val32;
> > > > + s64 sval;
> > > > + s32 sval32;
> > > > +
> > > > + /* If either register is a pointer, we can't learn anything about its
> > > > + * variable offset from the compare (unless they were a pointer into
> > > > + * the same object, but we don't bother with that).
> > > > */
> > > > - if (__is_pointer_value(false, false_reg1))
> > >
> > > The removal of the above check, but not the comment was surprising and concerning,
> > > so I did a bit of git-archaeology.
> > > It was added in commit f1174f77b50c ("bpf/verifier: rework value tracking")
> > > back in 2017 !
> > > and in that commit reg_set_min_max() was always called with reg == scalar.
> > > It looked like premature check. Then I spotted a comment in that commit:
> > > * this is only legit if both are scalars (or pointers to the same
> > > * object, I suppose, but we don't support that right now), because
> > > * otherwise the different base pointers mean the offsets aren't
> > > * comparable.
> > > so the intent back then was to generalize reg_set_min_max() to be used with pointers too,
> > > but we never got around to do that and the comment now reads:
> >
> > Yeah, it shouldn't be too hard to "generalize" to pointer vs pointer,
> > if we ensure they point to exactly the same thing (I haven't thought
> > much about how), because beyond that it's still basically SCALAR
> > offsets. But I figured it's out of scope for these changes :)
> >
> > > * this is only legit if both are scalars (or pointers to the same
> > > * object, I suppose, see the PTR_MAYBE_NULL related if block below),
> > > * because otherwise the different base pointers mean the offsets aren't
> > > * comparable.
> > >
> > > So please remove is_pointer check and remove the comment,
> >
> > So I'm a bit confused. I did remove __is_pointer_value() check, but I
> > still need to guard against having pointers, which is why I have:
> >
> > if (false_reg1->type != SCALAR_VALUE || false_reg2->type != SCALAR_VALUE).
> > return;
> >
> > I think I need this check, because reg_set_min_max() can be called
> > from check_cond_jmp_op() with pointer regs, and we shouldn't try to
> > adjust them. Or am I missing something? And the comment I have here
> > now:
>
> I don't see a code path where reg_set_min_max() is called
> with pointers. At least not in the current code base.
> Are you saying somewhere in your later patch it happens?
>
Hm.. no, it's all in this patch. Check check_cond_jmp_op(). We at
least allow `(reg_is_pkt_pointer_any(dst_reg) &&
reg_is_pkt_pointer_any(src_reg)` case to get into is_branch_taken(),
which, btw, does handle pointer x pointer, pointer x scalar, and
scalar x scalar cases. Then, we go straight to reg_set_min_max(), both
for BPF_X and BPF_K cases. So reg_set_min_max() has to guard itself
against pointers.
> Then the question is whether to do this is_scalar check inside
> reg_set_min_max() or outside. Both options are probably fine.
Given we have two separate calls to reg_set_min_max(), BPF_X and
BPF_K, it seems cleaner to do it once at the beginning of
reg_set_min_max(). And if in the future we do support pointer
variants, I'd handle them inside reg_set_min_max(), just like
is_branch_taken() handles different situations in one place
transparently to the caller.
>
> >
> > + /* If either register is a pointer, we can't learn anything about its
> > + * variable offset from the compare (unless they were a pointer into
> > + * the same object, but we don't bother with that).
> > */
> >
> > is trying to explain that we don't really adjust two pointers.
> >
> > > and fixup the comment in check_cond_jmp_op() where reg_set_min_max().
> >
> > I have this locally for now, please let me know if this is fine or you
> > had something else in mind:
> >
> > -/* Adjusts the register min/max values in the case that the dst_reg is the
> > - * variable register that we are working on, and src_reg is a constant or we're
> > - * simply doing a BPF_K check.
> > - * In JEQ/JNE cases we also adjust the var_off values.
> > +/* Adjusts the register min/max values in the case that the dst_reg and
> > + * src_reg are both SCALAR_VALUE registers (or we are simply doing a BPF_K
> > + * check, in which case we havea fake SCALAR_VALUE representing insn->imm).
> > + * Technically we can do similar adjustments for pointers to the same object,
> > + * but we don't support that right now.
>
> Looks fine. I'm trying to say that we had such comment forever and
> it never lead to actually doing the work.
> So I'd just remove the last sentence about pointers ...
Ah, ok, yep, sure.
>
> > */
> > static void reg_set_min_max(struct bpf_reg_state *true_reg1,
> > struct bpf_reg_state *true_reg2,
> > @@ -14884,13 +14885,6 @@ static int check_cond_jmp_op(struct
> > bpf_verifier_env *env,
> > return -EFAULT;
> > other_branch_regs = other_branch->frame[other_branch->curframe]->regs;
> >
> > - /* detect if we are comparing against a constant value so we can adjust
> > - * our min/max values for our dst register.
> > - * this is only legit if both are scalars (or pointers to the same
> > - * object, I suppose, see the PTR_MAYBE_NULL related if block below),
> > - * because otherwise the different base pointers mean the offsets aren't
> > - * comparable.
> > - */
>
> ... and removing this comment is good thing too.
> In general the comments should be in front of the function body
> (as you're doing) instead of the callsite.
yep, sounds good
