netdev - Re: [PATCH bpf-next v5 9/9] bpf: xfrm: Add selftest for bpf_xdp_get_xfrm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 14 Dec 2023 11:23:02 -0700
From: Daniel Xu <dxu@...uu.xyz>
To: Kumar Kartikeya Dwivedi <memxor@...il.com>
Cc: Eyal Birger <eyal.birger@...il.com>, daniel@...earbox.net, 
	davem@...emloft.net, shuah@...nel.org, ast@...nel.org, john.fastabend@...il.com, 
	kuba@...nel.org, andrii@...nel.org, hawk@...nel.org, steffen.klassert@...unet.com, 
	antony.antony@...unet.com, alexei.starovoitov@...il.com, yonghong.song@...ux.dev, 
	eddyz87@...il.com, mykolal@...com, martin.lau@...ux.dev, song@...nel.org, 
	kpsingh@...nel.org, sdf@...gle.com, haoluo@...gle.com, jolsa@...nel.org, 
	bpf@...r.kernel.org, linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org, 
	netdev@...r.kernel.org, devel@...ux-ipsec.org
Subject: Re: [PATCH bpf-next v5 9/9] bpf: xfrm: Add selftest for
 bpf_xdp_get_xfrm_state()

On Thu, Dec 14, 2023 at 05:16:08PM +0100, Kumar Kartikeya Dwivedi wrote:
> On Thu, 14 Dec 2023 at 17:08, Kumar Kartikeya Dwivedi <memxor@...il.com> wrote:
> >
> > On Thu, 14 Dec 2023 at 00:49, Eyal Birger <eyal.birger@...il.com> wrote:
> > >
> > > On Wed, Dec 13, 2023 at 3:15 PM Daniel Xu <dxu@...uu.xyz> wrote:
> > > > > > [...]
> > > > > >
> > > > > > diff --git a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > > > > > index c0dd38616562..f00dba85ac5d 100644
> > > > > > --- a/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > > > > > +++ b/tools/testing/selftests/bpf/progs/test_tunnel_kern.c
> > > > > > @@ -8,8 +8,9 @@
> > > > > >   */
> > > > > >  #include "vmlinux.h"
> > > > > >  #include <bpf/bpf_core_read.h>
> > > > > > -#include <bpf/bpf_helpers.h>
> > > > > >  #include <bpf/bpf_endian.h>
> > > > > > +#include <bpf/bpf_helpers.h>
> > > > > > +#include "bpf_experimental.h"
> > > > > >  #include "bpf_kfuncs.h"
> > > > > >  #include "bpf_tracing_net.h"
> > > > > >
> > > > > > @@ -988,8 +989,9 @@ int xfrm_get_state_xdp(struct xdp_md *xdp)
> > > > > >         opts.family = AF_INET;
> > > > > >
> > > > > >         x = bpf_xdp_get_xfrm_state(xdp, &opts, sizeof(opts));
> > > > > > -       if (!x || opts.error)
> > > > > > +       if (!x)
> > > > > >                 goto out;
> > > > > > +       bpf_assert_with(opts.error == 0, XDP_PASS);
> > > > > >
> > > > > >         if (!x->replay_esn)
> > > > > >                 goto out;
> > > > > >
> > > > > > results in:
> > > > > >
> > > > > > 57: (b7) r1 = 2                       ; R1_w=2 refs=5
> > > > > > 58: (85) call bpf_throw#115436
> > > > > > calling kernel function bpf_throw is not allowed
> > > > > >
> > > > >
> > > > > I think this might be because bpf_throw is not registered for use by
> > > > > BPF_PROG_TYPE_XDP. I would simply register the generic_kfunc_set for
> > > > > this program type as well, since it's already done for TC.
> > > >
> > > > Ah yeah, that was it.
> > > >
> > > > >
> > > > > > It looks like the above error comes from verifier.c:fetch_kfunc_meta,
> > > > > > but I can run the exceptions selftests just fine with the same bzImage.
> > > > > > So I'm thinking it's not a kfunc registration or BTF issue.
> > > > > >
> > > > > > Maybe it's cuz I'm holding onto KFUNC_ACQUIRE'd `x`? Not sure.
> > > > > >
> > > > >
> > > > > Yes, even once you enable this, this will fail for now. I am sending
> > > > > out a series later this week that enables bpf_throw with acquired
> > > > > references, but until then may I suggest the following:
> > > > >
> > > > > #define bpf_assert_if(cond) for (int ___i = 0, ___j = (cond); !(___j) \
> > > > > && !___j; bpf_throw(), ___i++)
> > > > >
> > > > > This will allow you to insert some cleanup code with an assertion.
> > > > > Then in my series, I will convert this temporary bpf_assert_if back to
> > > > > the normal bpf_assert.
> > > > >
> > > > > It would look like:
> > > > > bpf_assert_if(opts.error == 0) {
> > > > >   // Execute if assertion failed
> > > > >   bpf_xdp_xfrm_state_release(x);
> > > > > }
> > > > >
> > > > > Likewise for bpf_assert_with_if, you get the idea.
> > > >
> > > > I gave it a try and I'm getting this compile error:
> > > >
> > > >         progs/test_tunnel_kern.c:996:2: error: variable '___j' used in loop condition not modified in loop body [-Werror,-Wfor-loop-analysis]
> > > >                 bpf_assert_with_if(opts.error == 0, XDP_PASS) {
> > > >                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > >         /home/dxu/dev/linux/tools/testing/selftests/bpf/bpf_experimental.h:295:38: note: expanded from macro 'bpf_assert_with_if'
> > > >                 for (int ___i = 0, ___j = (cond); !(___j) && !___j; bpf_throw(value), ___i++)
> > > >                                                     ^~~~      ~~~~
> > > >         1 error generated.
> > > >         make: *** [Makefile:618: /home/dxu/dev/linux/tools/testing/selftests/bpf/test_tunnel_kern.bpf.o] Error 1
> > > >
> > > > Seems like the compiler is being clever.
> > >
> > > It looks like ___j is used twice - maybe it was meant to be ___i? i.e.:
> > >
> > >    for (int ___i = 0, ___j = (cond); !(___j) && !___i; bpf_throw(value), ___i++)
> > >
> >
> > Ah, yes, that's a typo. Eyal is right, it should be ___i.
> 
> Additionally, I would modify the macro to do ___j = !!(cond).

Makes sense. Will send out v6 with these fixes today.