On Mon, Oct 30, 2023 at 11:16 PM Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > On Mon, Oct 30, 2023 at 7:20 PM Alexei Starovoitov > <alexei.starovoitov@xxxxxxxxx> wrote: > > > > On Fri, Oct 27, 2023 at 11:13:43AM -0700, Andrii Nakryiko wrote: > > > Use 32-bit subranges to prune some 64-bit BPF_JEQ/BPF_JNE conditions > > > that otherwise would be "inconclusive" (i.e., is_branch_taken() would > > > return -1). This can happen, for example, when registers are initialized > > > as 64-bit u64/s64, then compared for inequality as 32-bit subregisters, > > > and then followed by 64-bit equality/inequality check. That 32-bit > > > inequality can establish some pattern for lower 32 bits of a register > > > (e.g., s< 0 condition determines whether the bit #31 is zero or not), > > > while overall 64-bit value could be anything (according to a value range > > > representation). > > > > > > This is not a fancy quirky special case, but actually a handling that's > > > necessary to prevent correctness issue with BPF verifier's range > > > tracking: set_range_min_max() assumes that register ranges are > > > non-overlapping, and if that condition is not guaranteed by > > > is_branch_taken() we can end up with invalid ranges, where min > max. > > > > This is_scalar_branch_taken() logic makes sense, > > but if set_range_min_max() is delicate, it should have its own sanity > > check for ranges. > > Shouldn't be difficult to check for that dangerous overlap case. > > So let me clarify. As far as I'm concerned, is_branch_taken() is such > a check for set_reg_min_max, and so duplicating such checks in > set_reg_min_max() is just that a duplication of code and logic, and > just a chance for more typos and subtle bugs. > > But the concern about invalid ranges is valid, so I don't know, > perhaps we should just do a quick check after adjustment to validate > that umin<=umax and so on? E.g., we can do that outside of > reg_set_min_max(), to keep reg_set_min_max() non-failing. WDYT? Sounds like a good option too. Just trying to minimize breakage in the future. Sanity check before or after should catch it.
