Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Feedback-ID: i6a694271:Fastmail
Date:   Fri, 1 Dec 2023 17:10:47 -0700
From:   Daniel Xu <dxu@dxuuu.xyz>
To:     Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc:     ast@kernel.org, daniel@iogearbox.net, shuah@kernel.org,
        andrii@kernel.org, steffen.klassert@secunet.com,
        antony.antony@secunet.com, alexei.starovoitov@gmail.com,
        yonghong.song@linux.dev, eddyz87@gmail.com, mykolal@fb.com,
        martin.lau@linux.dev, song@kernel.org, john.fastabend@gmail.com,
        kpsingh@kernel.org, sdf@google.com, haoluo@google.com,
        jolsa@kernel.org, linux-kernel@vger.kernel.org,
        bpf@vger.kernel.org, linux-kselftest@vger.kernel.org,
        devel@linux-ipsec.org, netdev@vger.kernel.org
Subject: Re: [PATCH ipsec-next v3 5/9] libbpf: selftests: Add verifier tests
 for CO-RE bitfield writes
Message-ID: <ka2irjz53qjkax545o67mvouyytzqw3dvorqixe2q72crgzjpi@he2uiobuelvd>
References: <cover.1701462010.git.dxu@dxuuu.xyz>
 <e4d14fb5f07145ff4a367cc01d8dcf6c82581c88.1701462010.git.dxu@dxuuu.xyz>
 <CAEf4Bzaz+_y=kxBpPmwYsvzaHypmL=ZBfOK12vLom04DRDWyPg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAEf4Bzaz+_y=kxBpPmwYsvzaHypmL=ZBfOK12vLom04DRDWyPg@mail.gmail.com>
Precedence: bulk

Hi Andrii,

On Fri, Dec 01, 2023 at 03:52:25PM -0800, Andrii Nakryiko wrote:
> On Fri, Dec 1, 2023 at 12:24 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
> >
> > Add some tests that exercise BPF_CORE_WRITE_BITFIELD() macro. Since some
> > non-trivial bit fiddling is going on, make sure various edge cases (such
> > as adjacent bitfields and bitfields at the edge of structs) are
> > exercised.
> >
> > Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
> > ---
> >  .../selftests/bpf/prog_tests/verifier.c       |   2 +
> >  .../bpf/progs/verifier_bitfield_write.c       | 100 ++++++++++++++++++
> >  2 files changed, 102 insertions(+)
> >  create mode 100644 tools/testing/selftests/bpf/progs/verifier_bitfield_write.c
> >
> 
> LGTM, but I'm not sure why we need all those __failure_unpriv, see
> below. Regardless:
> 
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> 
> > diff --git a/tools/testing/selftests/bpf/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c
> > index 5cfa7a6316b6..67b4948865a3 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/verifier.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/verifier.c
> > @@ -6,6 +6,7 @@
> >  #include "verifier_and.skel.h"
> >  #include "verifier_array_access.skel.h"
> >  #include "verifier_basic_stack.skel.h"
> > +#include "verifier_bitfield_write.skel.h"
> >  #include "verifier_bounds.skel.h"
> >  #include "verifier_bounds_deduction.skel.h"
> >  #include "verifier_bounds_deduction_non_const.skel.h"
> > @@ -115,6 +116,7 @@ static void run_tests_aux(const char *skel_name,
> >
> >  void test_verifier_and(void)                  { RUN(verifier_and); }
> >  void test_verifier_basic_stack(void)          { RUN(verifier_basic_stack); }
> > +void test_verifier_bitfield_write(void)       { RUN(verifier_bitfield_write); }
> >  void test_verifier_bounds(void)               { RUN(verifier_bounds); }
> >  void test_verifier_bounds_deduction(void)     { RUN(verifier_bounds_deduction); }
> >  void test_verifier_bounds_deduction_non_const(void)     { RUN(verifier_bounds_deduction_non_const); }
> > diff --git a/tools/testing/selftests/bpf/progs/verifier_bitfield_write.c b/tools/testing/selftests/bpf/progs/verifier_bitfield_write.c
> > new file mode 100644
> > index 000000000000..8fe355a19ba6
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/verifier_bitfield_write.c
> > @@ -0,0 +1,100 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <linux/bpf.h>
> > +#include <stdint.h>
> > +
> > +#include <bpf/bpf_helpers.h>
> > +#include <bpf/bpf_core_read.h>
> > +
> > +#include "bpf_misc.h"
> > +
> > +struct core_reloc_bitfields {
> > +       /* unsigned bitfields */
> > +       uint8_t         ub1: 1;
> > +       uint8_t         ub2: 2;
> > +       uint32_t        ub7: 7;
> > +       /* signed bitfields */
> > +       int8_t          sb4: 4;
> > +       int32_t         sb20: 20;
> > +       /* non-bitfields */
> > +       uint32_t        u32;
> > +       int32_t         s32;
> > +} __attribute__((preserve_access_index));
> > +
> > +SEC("tc")
> > +__description("single CO-RE bitfield roundtrip")
> > +__btf_path("btf__core_reloc_bitfields.bpf.o")
> > +__success __failure_unpriv
> 
> do we want __failure_unpriv at all? Is this failure related to
> *bitfield* logic at all?

Oh, I pre-emptively added it. From the docs, I thought __failure_unpriv
meant "don't try to load this as an unprivileged used cuz it'll fail".
And since I used the tc hook, I figured it'd fail.

Removing the annotation doesn't seem to do anything bad so I'll drop it
for v4.

[...]

Thanks,
Daniel