Re: [PATCH v2 bpf-next 3/6] bpf: Exit early if reg_bounds_sync gets invalid inputs

Harishankar Vishwanathan
2026-03-24T17:46:32Z

On Mon, Mar 23, 2026 at 8:12 AM Shung-Hsi Yu <shung-hsi.yu@suse.com> wrote:
>
> On Fri, Mar 20, 2026 at 05:49:30PM +0100, Paul Chaignon wrote:
> > From: Harishankar Vishwanathan <harishankar.vishwanathan@gmail.com>
[...]
> Thought given reg_bounds_sync() is used if various different places, I
> wonder how this will affect current syzbot reporting. In comparison the
> rest of the patchset seems to try to preserve current behavior.
>
> Being nosey, I also look at the Sashiko review[1], which gave the
> following review:
>
> Does returning early here expose invalid bounds to
> reg_bounds_sanity_check()?
> When regs_refine_cond_op() produces overlapping bounds for logically dead
> branches (e.g., umin > umax), reg_bounds_sync() previously repaired them.
> With this early exit, the invalid bounds remain.
> Will the subsequent call to reg_bounds_sanity_check() detect this violation
> and trigger a kernel warning via verifier_bug() and BPF_WARN_ONCE?
> Additionally, since reg_bounds_sanity_check() handles failures by marking
> the register as unbounded, could this cause the verifier to explore dead
> branches with unconstrained registers and falsely reject valid programs?
> While this is resolved in the subsequent commit "bpf: Simulate branches to
> prune based on range violations", could this intermediate state break
> git bisectability?
>
> Given if we get invalid bounds in reg_bounds_sync() to begin with, then
> there is no guarantee that what the BPF verifier ended up with can be
> trusted at all, so returning early shouldn't introduce new safety issue,
> and the rest about falsely rejecting valid program doesn't seem like a
> concern, too, for similar reason.

Thanks for taking a look at the review!

As you know, the main reason for this commit is to allow the next
commit to identify dead branches
with a single call to range_bounds_violation(), instead of two calls
(one after regs_refine_cond_op(),
and one after sync()).

You are right, if the inputs to sync() itself are invalid, then
whatever "fixes" sync()
can come up with don't make sense anyway. So it should be okay to exit early.

One argument to keep sync() unchanged is, as you say, to "preserve
existing behavior".

I'm okay either way. If it makes sense to keep sync() unchanged + use
two calls to range_bounds_violation(), please advise.
Otherwise will continue with this commit.

> 1: https://sashiko.dev/#/patchset/cover.1774025082.git.paul.chaignon%40gmail.com
>
> ...