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

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

On 5/1/23 10:22 AM, Stanislav Fomichev wrote:
> On Fri, Apr 28, 2023 at 5:44 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>>
>> 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.
> 
> Maybe I can do something like the following?
> 
>                 if (test->set_optlen >= PAGE_SIZE) {
>                         int num_pages = test->set_optlen / PAGE_SIZE;
>                         int remainder = test->set_optlen % PAGE_SIZE;
> 
>                         test->set_optlen = num_pages *
> sysconf(_SC_PAGESIZE) + remainder;
>                 }
> 
> More verbose, but less magical than depending on 4097. 

LGTM.

> For the BPF side, I can probably pass proper value via bss..

Make sense also.