diff mbox series

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

Message ID 20230427200409.1785263-3-sdf@google.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series bpf: Don't EFAULT for {g,s}setsockopt with wrong optlen | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/cc_maintainers warning 4 maintainers not CCed: mykolal@fb.com wangyufen@huawei.com shuah@kernel.org linux-kselftest@vger.kernel.org
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 111 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-34 success Logs for test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-35 success Logs for test_verifier on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-36 success Logs for veristat
bpf/vmtest-bpf-next-VM_Test-33 success Logs for test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-22 fail Logs for test_progs_no_alu32 on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-23 success Logs for test_progs_no_alu32_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for test_progs_no_alu32_parallel on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-25 success Logs for test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for test_progs_no_alu32_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-27 success Logs for test_progs_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for test_progs_parallel on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-29 success Logs for test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-30 success Logs for test_progs_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-31 success Logs for test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-32 success Logs for test_verifier on aarch64 with llvm-16
bpf/vmtest-bpf-next-PR fail PR summary
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-2 success Logs for build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-3 success Logs for build for aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-4 success Logs for build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-5 success Logs for build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for build for x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-7 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-8 success Logs for test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for test_maps on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-10 success Logs for test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for test_maps on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-13 fail Logs for test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-14 fail Logs for test_progs on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-15 fail Logs for test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-16 fail Logs for test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-17 fail Logs for test_progs on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-18 fail Logs for test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-19 fail Logs for test_progs_no_alu32 on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-20 fail Logs for test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-21 fail Logs for test_progs_no_alu32 on x86_64 with gcc

Commit Message

Stanislav Fomichev April 27, 2023, 8:04 p.m. UTC 
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(-)

Comments

Martin KaFai Lau April 28, 2023, 11:57 p.m. UTC | #1 
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?
Stanislav Fomichev April 28, 2023, 11:59 p.m. UTC | #2 
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..
Stanislav Fomichev April 29, 2023, 12:32 a.m. UTC | #3 
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.
Martin KaFai Lau April 29, 2023, 12:44 a.m. UTC | #4 
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.
Stanislav Fomichev May 1, 2023, 5:22 p.m. UTC | #5 
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. For the BPF
side, I can probably pass proper value via bss..
Martin KaFai Lau May 1, 2023, 7:04 p.m. UTC | #6 
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.
diff mbox series

Patch

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,
 	},
 	{
 		.descr = "getsockopt: support smaller ctx->optlen",
@@ -648,6 +669,49 @@  static struct sockopt_test {
 
 		.error = EFAULT_SETSOCKOPT,
 	},
+	{
+		.descr = "setsockopt: ignore >PAGE_SIZE optlen",
+		.insns = {
+			/* 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),
+			/* } */
+
+			BPF_MOV64_IMM(BPF_REG_0, 1),
+			BPF_EXIT_INSN(),
+		},
+		.attach_type = BPF_CGROUP_SETSOCKOPT,
+		.expected_attach_type = BPF_CGROUP_SETSOCKOPT,
+
+		.set_level = SOL_IP,
+		.set_optname = IP_TOS,
+		.set_optval = { 1 << 3 },
+		.set_optlen = 4096 + 1,
+
+		.get_level = SOL_IP,
+		.get_optname = IP_TOS,
+#if __BYTE_ORDER == __LITTLE_ENDIAN
+		.get_optval = { 1 << 3, 0, 0, 0 }, /* the changes are ignored */
+#else
+		.get_optval = { 0, 0, 0, 1 << 3 }, /* the changes are ignored */
+#endif
+		.get_optlen = 4,
+	},
 	{
 		.descr = "setsockopt: allow changing ctx->optlen within bounds",
 		.insns = {
@@ -922,6 +986,7 @@  static int run_test(int cgroup_fd, struct sockopt_test *test)
 
 	if (test->get_optlen) {
 		optval = malloc(test->get_optlen);
+		memset(optval, 0, test->get_optlen);
 		socklen_t optlen = test->get_optlen;
 		socklen_t expected_get_optlen = test->get_optlen_ret ?:
 			test->get_optlen;
@@ -946,6 +1011,9 @@  static int run_test(int cgroup_fd, struct sockopt_test *test)
 			goto free_optval;
 		}
 
+		if (optlen > sizeof(test->get_optval))
+			optlen = sizeof(test->get_optval);
+
 		if (memcmp(optval, test->get_optval, optlen) != 0) {
 			errno = 0;
 			log_err("getsockopt returned unexpected optval");