On Tue, Oct 31, 2023 at 11:04 AM Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> wrote: > > On Tue, Oct 31, 2023 at 10:56 AM Andrii Nakryiko > <andrii.nakryiko@xxxxxxxxx> 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.
