From e688c3db7ca69bea1872c5706aec6a7fdf89df17 Mon Sep 17 00:00:00 2001 From: Alexei Starovoitov Date: Wed, 14 Oct 2020 10:56:08 -0700 Subject: bpf: Fix register equivalence tracking. The 64-bit JEQ/JNE handling in reg_set_min_max() was clearing reg->id in either true or false branch. In the case 'if (reg->id)' check was done on the other branch the counter part register would have reg->id == 0 when called into find_equal_scalars(). In such case the helper would incorrectly identify other registers with id == 0 as equivalent and propagate the state incorrectly. Fix it by preserving ID across reg_set_min_max(). In other words any kind of comparison operator on the scalar register should preserve its ID to recognize: r1 = r2 if (r1 == 20) { #1 here both r1 and r2 == 20 } else if (r2 < 20) { #2 here both r1 and r2 < 20 } The patch is addressing #1 case. The #2 was working correctly already. Fixes: 75748837b7e5 ("bpf: Propagate scalar ranges through register assignments.") Signed-off-by: Alexei Starovoitov Signed-off-by: Daniel Borkmann Acked-by: Andrii Nakryiko Acked-by: John Fastabend Tested-by: Yonghong Song Link: https://lore.kernel.org/bpf/20201014175608.1416-1-alexei.starovoitov@gmail.com --- tools/testing/selftests/bpf/verifier/regalloc.c | 26 +++++++++++++++++++++++++ 1 file changed, 26 insertions(+) (limited to 'tools') diff --git a/tools/testing/selftests/bpf/verifier/regalloc.c b/tools/testing/selftests/bpf/verifier/regalloc.c index ac71b824f97a..4ad7e05de706 100644 --- a/tools/testing/selftests/bpf/verifier/regalloc.c +++ b/tools/testing/selftests/bpf/verifier/regalloc.c @@ -241,3 +241,29 @@ .result = ACCEPT, .prog_type = BPF_PROG_TYPE_TRACEPOINT, }, +{ + "regalloc, spill, JEQ", + .insns = { + BPF_MOV64_REG(BPF_REG_6, BPF_REG_1), + BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0), + BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), + BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8), + BPF_LD_MAP_FD(BPF_REG_1, 0), + BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem), + BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_0, -8), /* spill r0 */ + BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 0), + /* The verifier will walk the rest twice with r0 == 0 and r0 == map_value */ + BPF_EMIT_CALL(BPF_FUNC_get_prandom_u32), + BPF_MOV64_REG(BPF_REG_2, BPF_REG_0), + BPF_JMP_IMM(BPF_JEQ, BPF_REG_2, 20, 0), + /* The verifier will walk the rest two more times with r0 == 20 and r0 == unknown */ + BPF_LDX_MEM(BPF_DW, BPF_REG_3, BPF_REG_10, -8), /* fill r3 with map_value */ + BPF_JMP_IMM(BPF_JEQ, BPF_REG_3, 0, 1), /* skip ldx if map_value == NULL */ + /* Buggy verifier will think that r3 == 20 here */ + BPF_LDX_MEM(BPF_DW, BPF_REG_0, BPF_REG_3, 0), /* read from map_value */ + BPF_EXIT_INSN(), + }, + .fixup_map_hash_48b = { 4 }, + .result = ACCEPT, + .prog_type = BPF_PROG_TYPE_TRACEPOINT, +}, -- cgit From 8a3feed90e75283f15f830b1051bf048e4127c91 Mon Sep 17 00:00:00 2001 From: Martin KaFai Lau Date: Mon, 12 Oct 2020 16:49:40 -0700 Subject: bpf, selftest: Fix flaky tcp_hdr_options test when adding addr to lo The tcp_hdr_options test adds a "::eB9F" addr to the lo dev. However, this non loopback address will have a race on ipv6 dad which may lead to EADDRNOTAVAIL error from time to time. Even nodad is used in the iproute2 command, there is still a race in when the route will be added. This will then lead to ENETUNREACH from time to time. To avoid the above, this patch uses the default loopback address "::1" to do the test. Fixes: ad2f8eb0095e ("bpf: selftests: Tcp header options") Reported-by: Andrii Nakryiko Signed-off-by: Martin KaFai Lau Signed-off-by: Daniel Borkmann Acked-by: Andrii Nakryiko Link: https://lore.kernel.org/bpf/20201012234940.1707941-1-kafai@fb.com --- .../selftests/bpf/prog_tests/tcp_hdr_options.c | 26 +--------------------- .../bpf/progs/test_misc_tcp_hdr_options.c | 2 +- 2 files changed, 2 insertions(+), 26 deletions(-) (limited to 'tools') diff --git a/tools/testing/selftests/bpf/prog_tests/tcp_hdr_options.c b/tools/testing/selftests/bpf/prog_tests/tcp_hdr_options.c index c86e67214a9e..c85174cdcb77 100644 --- a/tools/testing/selftests/bpf/prog_tests/tcp_hdr_options.c +++ b/tools/testing/selftests/bpf/prog_tests/tcp_hdr_options.c @@ -15,7 +15,7 @@ #include "test_tcp_hdr_options.skel.h" #include "test_misc_tcp_hdr_options.skel.h" -#define LO_ADDR6 "::eB9F" +#define LO_ADDR6 "::1" #define CG_NAME "/tcpbpf-hdr-opt-test" struct bpf_test_option exp_passive_estab_in; @@ -40,27 +40,6 @@ struct sk_fds { int active_lport; }; -static int add_lo_addr(void) -{ - char ip_addr_cmd[256]; - int cmdlen; - - cmdlen = snprintf(ip_addr_cmd, sizeof(ip_addr_cmd), - "ip -6 addr add %s/128 dev lo scope host", - LO_ADDR6); - - if (CHECK(cmdlen >= sizeof(ip_addr_cmd), "compile ip cmd", - "failed to add host addr %s to lo. ip cmdlen is too long\n", - LO_ADDR6)) - return -1; - - if (CHECK(system(ip_addr_cmd), "run ip cmd", - "failed to add host addr %s to lo\n", LO_ADDR6)) - return -1; - - return 0; -} - static int create_netns(void) { if (CHECK(unshare(CLONE_NEWNET), "create netns", @@ -72,9 +51,6 @@ static int create_netns(void) "failed to bring lo link up\n")) return -1; - if (add_lo_addr()) - return -1; - return 0; } diff --git a/tools/testing/selftests/bpf/progs/test_misc_tcp_hdr_options.c b/tools/testing/selftests/bpf/progs/test_misc_tcp_hdr_options.c index 72ec0178f653..6077a025092c 100644 --- a/tools/testing/selftests/bpf/progs/test_misc_tcp_hdr_options.c +++ b/tools/testing/selftests/bpf/progs/test_misc_tcp_hdr_options.c @@ -16,7 +16,7 @@ #define BPF_PROG_TEST_TCP_HDR_OPTIONS #include "test_tcp_hdr_options.h" -__u16 last_addr16_n = __bpf_htons(0xeB9F); +__u16 last_addr16_n = __bpf_htons(1); __u16 active_lport_n = 0; __u16 active_lport_h = 0; __u16 passive_lport_n = 0; -- cgit