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

Martin KaFai Lau <martin.lau@xxxxxxxxx> · Mon, 1 May 2023 12:04:37 -0700

On 5/1/23 10:22 AM, Stanislav Fomichev wrote:
On Fri, Apr 28, 2023 at 5:44 PM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:

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.

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.







