Re: [PATCH bpf-next 3/7] bpf: enforce precision for r0 on callback return

Andrii Nakryiko
2023-11-09T18:01:15Z

On Thu, Nov 9, 2023 at 9:58 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Thu, Nov 9, 2023 at 9:50 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> > >
> > > The r0 returned from bpf_loop's callback says bpf_loop to stop iteration,
> > > bpf_loop returns the number of completed iterations. However, the return
> > > value of bpf_loop modeled by verifier is unbounded scalar.
> > > Same for map's for each.
> >
> > return value of bpf_loop() is a different thing from return value of
> > bpf_loop's callback. Right now bpf_loop implementation in kernel does
> >
> > ret = callback(...);
> > /* return value: 0 - continue, 1 - stop and return */
> > if (ret)
> > return i + 1;
> >
> > So yes, it doesn't rely explicitly on return value to be 1 just due to
> > the above implementation. But verifier is meant to enforce that and
> > the protocol is that bpf_loop and other callback calling helpers
> > should rely on this value.
> >
> > I think we have the same problem in check_return_code() for entry BPF
> > programs. So let me taking this one out of this patch set and post a
> > new one concentrating on this particular issue. I've been meaning to
> > use umin/umax for return value checking anyways, so might be a good
> > idea to do this anyways.
>
> Just like Ed I was also initially confused by this.
> As you said check_return_code() has the same problem.
> I think the issue this patch and similar in check_return_code()
> should be fixing is the case where one state went through
> ret code checking, but another state with potentially out-of-range
> r0 got state pruned since r0 wasn't marked precise.

Right.

> Not sure how hard it would be to come up with a selftest for such a scenario.

Yep, I'll think of something. Lots of tests to come up with :)