Re: [PATCH v2 bpf-next 05/10] bpf: enforce precise retval range on program exit

[Shung-Hsi Yu <shung-hsi.yu@suse.com>]

On Wed, Nov 29, 2023 at 08:23:41AM -0800, Andrii Nakryiko wrote:
> On Wed, Nov 29, 2023 at 3:24 AM Shung-Hsi Yu <shung-hsi.yu@suse.com> wrote:
> >
> > On Tue, Nov 28, 2023 at 04:36:15PM -0800, Andrii Nakryiko wrote:
> > > Similarly to subprog/callback logic, enforce return value of BPF program
> > > using more precise umin/umax range.
> > >
> > > We need to adjust a bunch of tests due to a changed format of an error
> > > message.
> > >
> > > Acked-by: Eduard Zingerman <eddyz87@gmail.com>
> > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> >
> > ...
> >
> > Q: should the missing register name and values be added?
> 
> Probably not, it makes future refactoring a bit less painful. If the
> important part is to check that there *was* a message about invalid
> return result, rather *what exact format* of that message was, then
> matching for a substring is enough and makes the test a bit more
> robust to future slight refactorings.
> 
> > I know relatively little about selftest, but scrolling through it looks
> > as though the expect verifier message is incomplete. (Admittedly lots of
> > them are like this even before this patch, and this patch improves the
> > situation already)
> 
> Often times it's actually a mistake to expect exact format, it makes
> for painful refactoring and improvements. I feel it every time I touch
> verifier log formatting logic :( So I don't want to add to that pain.

Understood. Thanks for going through the reasoning!

> > e.g.
> >
> > > --- a/tools/testing/selftests/bpf/progs/test_global_func15.c
> > > +++ b/tools/testing/selftests/bpf/progs/test_global_func15.c
> > > @@ -13,7 +13,7 @@ __noinline int foo(unsigned int *v)
> > >  }
> > >
> > >  SEC("cgroup_skb/ingress")
> > > -__failure __msg("At program exit the register R0 has value")
> > > +__failure __msg("At program exit the register R0 has ")
> > >  int global_func15(struct __sk_buff *skb)
> > >  {
> > >       unsigned int v = 1;
> >
> > looks like it is missing umin/umax=1
> >
> > ...
> >
> > > diff --git a/tools/testing/selftests/bpf/progs/verifier_cgroup_inv_retcode.c b/tools/testing/selftests/bpf/progs/verifier_cgroup_inv_retcode.c
> > > index d6c4a7f3f790..4655f01b24aa 100644
> > > --- a/tools/testing/selftests/bpf/progs/verifier_cgroup_inv_retcode.c
> > > +++ b/tools/testing/selftests/bpf/progs/verifier_cgroup_inv_retcode.c
> > > @@ -7,7 +7,7 @@
> > >
> > >  SEC("cgroup/sock")
> > >  __description("bpf_exit with invalid return code. test1")
> > > -__failure __msg("R0 has value (0x0; 0xffffffff)")
> > > +__failure __msg("umax=4294967295 should have been in [0, 1]")
> > >  __naked void with_invalid_return_code_test1(void)
> > >  {
> > >       asm volatile ("                                 \
> >
> > looks like it is missing mention of R0, etc.