Re: [PATCH bpf-next v2 2/4] selftests/bpf: Update EFAULT {g,s}etsockopt selftests

[Martin KaFai Lau <martin.lau@xxxxxxxxx>]

On 4/28/23 5:32 PM, Stanislav Fomichev wrote:
> On Fri, Apr 28, 2023 at 4:59 PM Stanislav Fomichev <sdf@xxxxxxxxxx> wrote:
> > On Fri, Apr 28, 2023 at 4:57 PM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:
> > > On 4/27/23 1:04 PM, Stanislav Fomichev wrote:
> > > > Instead of assuming EFAULT, let's assume the BPF program's
> > > > output is ignored.
> > > >
> > > > Remove "getsockopt: deny arbitrary ctx->retval" because it
> > > > was actually testing optlen. We have separate set of tests
> > > > for retval.
> > > >
> > > > Signed-off-by: Stanislav Fomichev <sdf@xxxxxxxxxx>
> > > > ---
> > > >    .../selftests/bpf/prog_tests/sockopt.c        | 80 +++++++++++++++++--
> > > >    1 file changed, 74 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/tools/testing/selftests/bpf/prog_tests/sockopt.c b/tools/testing/selftests/bpf/prog_tests/sockopt.c
> > > > index aa4debf62fc6..8dad30ce910e 100644
> > > > --- a/tools/testing/selftests/bpf/prog_tests/sockopt.c
> > > > +++ b/tools/testing/selftests/bpf/prog_tests/sockopt.c
> > > > @@ -273,10 +273,30 @@ static struct sockopt_test {
> > > >                .error = EFAULT_GETSOCKOPT,
> > > >        },
> > > >        {
> > > > -             .descr = "getsockopt: deny arbitrary ctx->retval",
> > > > +             .descr = "getsockopt: ignore >PAGE_SIZE optlen",
> > > >                .insns = {
> > > > -                     /* ctx->retval = 123 */
> > > > -                     BPF_MOV64_IMM(BPF_REG_0, 123),
> > > > +                     /* write 0xFF to the first optval byte */
> > > > +
> > > > +                     /* r6 = ctx->optval */
> > > > +                     BPF_LDX_MEM(BPF_DW, BPF_REG_6, BPF_REG_1,
> > > > +                                 offsetof(struct bpf_sockopt, optval)),
> > > > +                     /* r2 = ctx->optval */
> > > > +                     BPF_MOV64_REG(BPF_REG_2, BPF_REG_6),
> > > > +                     /* r6 = ctx->optval + 1 */
> > > > +                     BPF_ALU64_IMM(BPF_ADD, BPF_REG_6, 1),
> > > > +
> > > > +                     /* r7 = ctx->optval_end */
> > > > +                     BPF_LDX_MEM(BPF_DW, BPF_REG_7, BPF_REG_1,
> > > > +                                 offsetof(struct bpf_sockopt, optval_end)),
> > > > +
> > > > +                     /* if (ctx->optval + 1 <= ctx->optval_end) { */
> > > > +                     BPF_JMP_REG(BPF_JGT, BPF_REG_6, BPF_REG_7, 1),
> > > > +                     /* ctx->optval[0] = 0xF0 */
> > > > +                     BPF_ST_MEM(BPF_B, BPF_REG_2, 0, 0xFF),
> > > > +                     /* } */
> > > > +
> > > > +                     /* ctx->retval = 0 */
> > > > +                     BPF_MOV64_IMM(BPF_REG_0, 0),
> > > >                        BPF_STX_MEM(BPF_W, BPF_REG_1, BPF_REG_0,
> > > >                                    offsetof(struct bpf_sockopt, retval)),
> > > >
> > > > @@ -287,9 +307,10 @@ static struct sockopt_test {
> > > >                .attach_type = BPF_CGROUP_GETSOCKOPT,
> > > >                .expected_attach_type = BPF_CGROUP_GETSOCKOPT,
> > > >
> > > > -             .get_optlen = 64,
> > > > -
> > > > -             .error = EFAULT_GETSOCKOPT,
> > > > +             .get_level = 1234,
> > > > +             .get_optname = 5678,
> > > > +             .get_optval = {}, /* the changes are ignored */
> > > > +             .get_optlen = 4096 + 1,
> > >
> > > The patchset looks good. Thanks for taking care of it.
> > >
> > > One question, is it safe to the assume 4096 page size for all platforms in the
> > > selftests?
> >
> > Good question; let me respin with sysconf() just to be safe..
>
> Argh, the compiler yells at me:
> error: initializer element is not a compile-time constant
>
> I guess I'm just gonna do #define PAGE_SIZE 4096 and if we do hit some
> problems on the other archs, I'll ifdef it in one place.

or run_test() can reinit optlen to sysconf_page_size + 1 if optlen == 4097.