Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 30 Aug 2022 08:46:12 -0500
From:   David Vernet <void@manifault.com>
To:     Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc:     bpf@vger.kernel.org, ast@kernel.org, andrii@kernel.org,
        daniel@iogearbox.net, kernel-team@fb.com, martin.lau@linux.dev,
        song@kernel.org, yhs@fb.com, john.fastabend@gmail.com,
        kpsingh@kernel.org, sdf@google.com, haoluo@google.com,
        jolsa@kernel.org, joannelkoong@gmail.com, tj@kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 4/4] selftests/bpf: Add selftests validating the user
 ringbuf
Message-ID: <Yw4UpK4uPZP9fYD+@maniforge.dhcp.thefacebook.com>
References: <20220818221212.464487-1-void@manifault.com>
 <20220818221212.464487-5-void@manifault.com>
 <CAEf4Bzbj0ACUmZqQLhRR5DvEX9Zphqz5UwBWdkTdXfKqxWM0mQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAEf4Bzbj0ACUmZqQLhRR5DvEX9Zphqz5UwBWdkTdXfKqxWM0mQ@mail.gmail.com>
User-Agent: Mutt/2.2.7 (2022-08-07)
Precedence: bulk

On Wed, Aug 24, 2022 at 03:03:54PM -0700, Andrii Nakryiko wrote:

[...]

> > +                       CHECK(read <= 0, "snprintf_comm",
> > +                             "Failed to write index %d to comm\n", i);
> 
> please, no CHECK() use in new tests, we have ASSERT_xxx() covering all
> common cases

No problem, I'll make this change in v4. Unless I missed something,
CHECK() isn't documented as deprecated in test_progs.h. I'll take care
of adding that in a separate change.

> > +static long
> > +bad_access1(struct bpf_dynptr *dynptr, void *context)
> > +{
> > +       const struct sample *sample;
> > +
> > +       sample = bpf_dynptr_data(dynptr - 1, 0, sizeof(*sample));
> > +       bpf_printk("Was able to pass bad pointer %lx\n", (__u64)dynptr - 1);
> > +
> > +       return 0;
> > +}
> > +
> > +/* A callback that accesses a dynptr in a bpf_user_ringbuf_drain callback should
> > + * not be able to read before the pointer.
> > + */
> > +SEC("?raw_tp/sys_nanosleep")
> 
> there is no sys_nanosleep raw tracepoint, use SEC("?raw_tp") to
> specify type, that's enough

Got it, thanks for catching that. Will fix.

> > diff --git a/tools/testing/selftests/bpf/test_user_ringbuf.h b/tools/testing/selftests/bpf/test_user_ringbuf.h
> > new file mode 100644
> > index 000000000000..1643b4d59ba7
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/test_user_ringbuf.h
> 
> nit: I'd probably put it under progs/test_user_ringbuf.h so it's
> closer to BPF source code. As it is right now, it's neither near
> user-space part of tests nor near BPF part.

Fair enough, will fix.

Thanks,
David