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

[Martin KaFai Lau <martin.lau@linux.dev>]

On 4/28/23 5:32 PM, Stanislav Fomichev wrote:
> On Fri, Apr 28, 2023 at 4:59 PM Stanislav Fomichev <sdf@google.com> wrote:
>>
>> On Fri, Apr 28, 2023 at 4:57 PM Martin KaFai Lau <martin.lau@linux.dev> 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@google.com>
>>>> ---
>>>>    .../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.