Re: [PATCH bpf-next v5 1/6] bpf: Correct the overflow check comment in bpf_iter_num_next()

--===============1611651014434636038==
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit

> diff --git a/kernel/bpf/bpf_iter.c b/kernel/bpf/bpf_iter.c
> index f5eaeb2493d4a..b235e117e206a 100644
> --- a/kernel/bpf/bpf_iter.c
> +++ b/kernel/bpf/bpf_iter.c
> @@ -802,12 +802,11 @@ __bpf_kfunc int *bpf_iter_num_next(struct bpf_iter_num* it)
>  {
>  	struct bpf_iter_num_kern *s = (void *)it;
>
> -	/* check failed initialization or if we are done (same behavior);
> -	 * need to be careful about overflow, so convert to s64 for checks,
> -	 * e.g., if s->cur == s->end == INT_MAX, we can't just do
> -	 * s->cur + 1 >= s->end
> +	/*
> +	 * s->cur < s->end while iterating, else s->cur == s->end == 0; the signed
> +	 * s->cur + 1 >= s->end holds even when s->cur + 1 wraps (start == INT_MIN).
                                  ^^^^^
>  	 */
> -	if ((s64)(s->cur + 1) >= s->end) {
> +	if (s->cur + 1 >= s->end) {
>  		s->cur = s->end = 0;
>  		return NULL;
>  	}

The replacement comment says "s->cur + 1 >= s->end holds even when
s->cur + 1 wraps (start == INT_MIN)". Doesn't the inequality evaluate to
false in the wrap case, not true?

Looking at the code: bpf_iter_num_new() sets s->cur = start - 1, so when
start == INT_MIN, s->cur wraps to INT_MAX. On the first call to
bpf_iter_num_next(), s->cur + 1 wraps back to INT_MIN. For a non-empty
range like the selftest bpf_for(i, INT_MIN, INT_MIN + 2) at
tools/testing/selftests/bpf/progs/iters_num.c:101, the test becomes
INT_MIN >= INT_MIN + 2, which is false, so the function returns the first
element.

The commit message describes this correctly ("the wrapping s->cur + 1
brings it back to start"), but "X >= Y holds" reads as "the inequality is
true," when what seems to be meant is that the comparison mechanism remains
correct under wraparound.

Also, the first clause says "s->cur < s->end while iterating, else
s->cur == s->end == 0". This doesn't cover the post-bpf_iter_num_new(),
pre-first-next() state for start == INT_MIN, where s->cur == INT_MAX and
s->end == INT_MIN + 2 (neither s->cur < s->end nor s->cur == s->end == 0).

Since the sole purpose of this patch is to make the comment accurate, could
the wording be adjusted to avoid asserting a condition that is false in the
cited case?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/30915992491
--===============1611651014434636038==--