Re: [PATCH v5 bpf-next 17/23] bpf: generalize reg_set_min_max() to handle two sets of two registers

Andrii Nakryiko
2023-10-31T18:06:23Z

On Tue, Oct 31, 2023 at 11:04 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Oct 31, 2023 at 10:56 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> > > >
> > > > 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.
> >
> > Correction, BPF_K branch does check for dst_reg->type == SCALAR_VALUE.
> > But BPF_X doesn't. I stared at this code for so long that I don't even
> > notice those checks anymore :(
> >
> > I'd rather drop this SCALAR check for the BPF_K case and keep
> > reg_set_min_max() as generic as is_branch_taken(), if that's ok. I
> > think it's less error-prone and a more consistent approach.
>
> Ahh. Now I see that the patch is doing reg_set_min_max()
> right after BPF_X check.
> So before the patch all !scalar checks were done outside
> and extra __is_pointer_value() inside was useless (reserved for future).
> With this change the !scalar change inside is necessary.
> Makes sense now. Commit log could have explained that bit
> and avoided this back and forth ;)

Ok, I'll call this out in the commit message, np.

>
> And yeah dropping !scalar from BPF_K path makes sense as well.

Done.