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 ;) And yeah dropping !scalar from BPF_K path makes sense as well.
