* [PATCH bpf v2 0/2] Fix narrow loads from an offset outside of target field
@ 2020-07-23 9:59 Jakub Sitnicki
2020-07-23 9:59 ` [PATCH bpf v2 1/2] bpf: Load zeros for narrow loads beyond " Jakub Sitnicki
2020-07-23 9:59 ` [PATCH bpf v2 2/2] selftests/bpf: Add test for narrow loads from context at an offset Jakub Sitnicki
0 siblings, 2 replies; 6+ messages in thread
From: Jakub Sitnicki @ 2020-07-23 9:59 UTC (permalink / raw)
To: bpf
Cc: netdev, kernel-team, Alexei Starovoitov, Daniel Borkmann, Yonghong Song
This is a second attempt at fixing narrow loads from context fields backed
by smaller-in-size target fields, when load offset is beyond the target
field.
Following Yonghong suggestion, verifier now emits an 'wX = 0' or 'rX = 0'
instruction for loads from offsets outside of target field.
Cc: Yonghong Song <yhs@fb.com>
[v1] https://lore.kernel.org/bpf/20200710173123.427983-1-jakub@cloudflare.com/
Jakub Sitnicki (2):
bpf: Load zeros for narrow loads beyond target field
selftests/bpf: Add test for narrow loads from context at an offset
kernel/bpf/verifier.c | 23 ++++-
.../selftests/bpf/prog_tests/narrow_load.c | 84 +++++++++++++++++++
.../selftests/bpf/progs/test_narrow_load.c | 43 ++++++++++
3 files changed, 148 insertions(+), 2 deletions(-)
create mode 100644 tools/testing/selftests/bpf/prog_tests/narrow_load.c
create mode 100644 tools/testing/selftests/bpf/progs/test_narrow_load.c
--
2.25.4
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH bpf v2 1/2] bpf: Load zeros for narrow loads beyond target field
2020-07-23 9:59 [PATCH bpf v2 0/2] Fix narrow loads from an offset outside of target field Jakub Sitnicki
@ 2020-07-23 9:59 ` Jakub Sitnicki
2020-07-23 21:56 ` Yonghong Song
2020-07-23 9:59 ` [PATCH bpf v2 2/2] selftests/bpf: Add test for narrow loads from context at an offset Jakub Sitnicki
1 sibling, 1 reply; 6+ messages in thread
From: Jakub Sitnicki @ 2020-07-23 9:59 UTC (permalink / raw)
To: bpf
Cc: netdev, kernel-team, Alexei Starovoitov, Daniel Borkmann, Yonghong Song
For narrow loads from context that are:
1) as big in size as the target field, and
2) at an offset beyond the target field,
the verifier does not emit the shift-and-mask instruction sequence
following the target field load instruction, as it happens for narrow loads
smaller in size than the target field width.
This has an unexpected effect of loading the same data, no matter what the
offset. While, arguably, the expected behavior is to load zeros for offsets
that beyond the target field.
For instance, 2-byte load from a 4-byte context field, backed by a 2-byte
target field at an offset of 2 bytes results in:
$ cat progs/test_narrow_load.c
[...]
SEC("sk_reuseport/narrow_load_half_word")
int narrow_load_half_word(struct sk_reuseport_md *ctx)
{
__u16 *half;
half = (__u16 *)&ctx->ip_protocol;
if (half[0] != IPPROTO_UDP)
return SK_DROP;
if (half[1] != 0)
return SK_DROP;
return SK_PASS;
}
$ llvm-objdump -S --no-show-raw-insn ...
[...]
0000000000000000 narrow_load_half_word:
; {
0: w0 = 0
; if (half[0] != IPPROTO_UDP)
1: r2 = *(u16 *)(r1 + 24)
2: if w2 != 17 goto +4 <LBB1_3>
; if (half[1] != 0)
3: r1 = *(u16 *)(r1 + 26)
4: w0 = 1
5: if w1 == 0 goto +1 <LBB1_3>
6: w0 = 0
0000000000000038 LBB1_3:
; }
7: exit
$ bpftool prog dump xlated ...
int narrow_load_half_word(struct sk_reuseport_md * ctx):
; int narrow_load_half_word(struct sk_reuseport_md *ctx)
0: (b4) w0 = 0
; if (half[0] != IPPROTO_UDP)
1: (79) r2 = *(u64 *)(r1 +8)
2: (69) r2 = *(u16 *)(r2 +924)
; if (half[0] != IPPROTO_UDP)
3: (56) if w2 != 0x11 goto pc+5
; if (half[1] != 0)
4: (79) r1 = *(u64 *)(r1 +8)
5: (69) r1 = *(u16 *)(r1 +924)
6: (b4) w0 = 1
; if (half[1] != 0)
7: (16) if w1 == 0x0 goto pc+1
8: (b4) w0 = 0
; }
9: (95) exit
In this case half[0] == half[1] == sk->sk_protocol, which is the target
field for the ctx->ip_protocol.
Fix it by emitting 'wX = 0' or 'rX = 0' instruction for all narrow loads
from an offset that is beyond the target field.
Going back to the example, with the fix in place, the upper half load from
ctx->ip_protocol yields zero:
int narrow_load_half_word(struct sk_reuseport_md * ctx):
; int narrow_load_half_word(struct sk_reuseport_md *ctx)
0: (b4) w0 = 0
; if (half[0] != IPPROTO_UDP)
1: (79) r2 = *(u64 *)(r1 +8)
2: (69) r2 = *(u16 *)(r2 +924)
; if (half[0] != IPPROTO_UDP)
3: (56) if w2 != 0x11 goto pc+4
; if (half[1] != 0)
4: (b4) w1 = 0
5: (b4) w0 = 1
; if (half[1] != 0)
6: (16) if w1 == 0x0 goto pc+1
7: (b4) w0 = 0
; }
8: (95) exit
Fixes: f96da09473b5 ("bpf: simplify narrower ctx access")
Suggested-by: Yonghong Song <yhs@fb.com>
Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
kernel/bpf/verifier.c | 23 +++++++++++++++++++++--
1 file changed, 21 insertions(+), 2 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 94cead5a43e5..0a9dbcdd6341 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -9614,11 +9614,11 @@ static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env,
*/
static int convert_ctx_accesses(struct bpf_verifier_env *env)
{
+ u32 target_size, size_default, off, access_off;
const struct bpf_verifier_ops *ops = env->ops;
int i, cnt, size, ctx_field_size, delta = 0;
const int insn_cnt = env->prog->len;
struct bpf_insn insn_buf[16], *insn;
- u32 target_size, size_default, off;
struct bpf_prog *new_prog;
enum bpf_access_type type;
bool is_narrower_load;
@@ -9760,7 +9760,26 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
return -EINVAL;
}
- if (is_narrower_load && size < target_size) {
+ /* When context field is wider than the target field,
+ * narrow load from an offset beyond the target field
+ * can be reduced to loading zero because there is
+ * nothing to load from memory.
+ */
+ access_off = off & (size_default - 1);
+ if (is_narrower_load && access_off >= target_size) {
+ cnt = 0;
+ if (ctx_field_size <= 4)
+ insn_buf[cnt++] = BPF_MOV32_IMM(insn->dst_reg, 0);
+ else
+ insn_buf[cnt++] = BPF_MOV64_IMM(insn->dst_reg, 0);
+ }
+ /* Narrow load from an offset within the target field,
+ * smaller in size than the target field, needs
+ * shifting and masking because convert_ctx_access
+ * always emits full-size target field load.
+ */
+ if (is_narrower_load && access_off < target_size &&
+ size < target_size) {
u8 shift = bpf_ctx_narrow_access_offset(
off, size, size_default) * 8;
if (ctx_field_size <= 4) {
--
2.25.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH bpf v2 2/2] selftests/bpf: Add test for narrow loads from context at an offset
2020-07-23 9:59 [PATCH bpf v2 0/2] Fix narrow loads from an offset outside of target field Jakub Sitnicki
2020-07-23 9:59 ` [PATCH bpf v2 1/2] bpf: Load zeros for narrow loads beyond " Jakub Sitnicki
@ 2020-07-23 9:59 ` Jakub Sitnicki
2020-07-23 22:05 ` Yonghong Song
1 sibling, 1 reply; 6+ messages in thread
From: Jakub Sitnicki @ 2020-07-23 9:59 UTC (permalink / raw)
To: bpf; +Cc: netdev, kernel-team, Alexei Starovoitov, Daniel Borkmann
Check that narrow loads at various offsets from a context field backed by a
target field that is smaller in size work as expected. That is target field
value is loaded only when the offset is less than the target field size.
While for offsets beyond the target field, the loaded value is zero.
Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
.../selftests/bpf/prog_tests/narrow_load.c | 84 +++++++++++++++++++
.../selftests/bpf/progs/test_narrow_load.c | 43 ++++++++++
2 files changed, 127 insertions(+)
create mode 100644 tools/testing/selftests/bpf/prog_tests/narrow_load.c
create mode 100644 tools/testing/selftests/bpf/progs/test_narrow_load.c
diff --git a/tools/testing/selftests/bpf/prog_tests/narrow_load.c b/tools/testing/selftests/bpf/prog_tests/narrow_load.c
new file mode 100644
index 000000000000..6d79d722a66d
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/narrow_load.c
@@ -0,0 +1,84 @@
+// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
+// Copyright (c) 2020 Cloudflare
+
+#include "test_progs.h"
+#include "test_narrow_load.skel.h"
+
+static int duration;
+
+void run_sk_reuseport_prog(struct bpf_program *reuseport_prog)
+{
+ static const struct timeval timeo = { .tv_sec = 3 };
+ struct sockaddr_in addr = {
+ .sin_family = AF_INET,
+ .sin_port = 0,
+ .sin_addr.s_addr = htonl(INADDR_LOOPBACK),
+ };
+ socklen_t len = sizeof(addr);
+ int err, fd, prog_fd;
+ const int one = 1;
+ char buf = 42;
+ ssize_t n;
+
+ prog_fd = bpf_program__fd(reuseport_prog);
+ if (CHECK(prog_fd < 0, "bpf_program__fd", "errno %d\n", errno))
+ return;
+
+ fd = socket(AF_INET, SOCK_DGRAM, 0);
+ if (CHECK(fd < 0, "socket", "errno %d\n", errno))
+ return;
+
+ /* Setup timeouts */
+ err = setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &timeo, sizeof(timeo));
+ if (CHECK(err, "setsockopt(SO_RCVTIMEO)", "errno %d\n", errno))
+ goto out_close;
+ err = setsockopt(fd, SOL_SOCKET, SO_SNDTIMEO, &timeo, sizeof(timeo));
+ if (CHECK(err, "setsockopt(SO_RCVTIMEO)", "errno %d\n", errno))
+ goto out_close;
+
+ /* Setup reuseport prog */
+ err = setsockopt(fd, SOL_SOCKET, SO_REUSEPORT, &one, sizeof(one));
+ if (CHECK(err, "setsockopt(SO_REUSEPORT)", "errno %d\n", errno))
+ goto out_close;
+ err = setsockopt(fd, SOL_SOCKET, SO_ATTACH_REUSEPORT_EBPF,
+ &prog_fd, sizeof(prog_fd));
+ if (CHECK(err, "setsockopt(SO_ATTACH_REUEPORT_EBPF)",
+ "errno %d\n", errno))
+ goto out_close;
+
+ err = bind(fd, (void *)&addr, len);
+ if (CHECK(err, "bind", "errno %d\n", errno))
+ goto out_close;
+ err = getsockname(fd, (void *)&addr, &len);
+ if (CHECK(err, "getsockname", "errno %d\n", errno))
+ goto out_close;
+
+ /* Send a message to itself to trigger reuseport prog */
+ n = sendto(fd, &buf, sizeof(buf), 0, (void *)&addr, len);
+ if (CHECK(n < 1, "sendto", "ret %ld errno %d\n", n, errno))
+ goto out_close;
+ n = recv(fd, &buf, sizeof(buf), 0);
+ if (CHECK(n < 1, "recv", "ret %ld errno %d\n", n, errno))
+ goto out_close;
+
+ /* Pass, reuseport prog didn't drop the packet */
+
+out_close:
+ close(fd);
+}
+
+void test_narrow_load(void)
+{
+ struct test_narrow_load *skel;
+
+ skel = test_narrow_load__open_and_load();
+ if (CHECK(!skel, "skel open_and_load", "failed\n"))
+ return;
+
+ if (test__start_subtest("narrow load byte"))
+ run_sk_reuseport_prog(skel->progs.narrow_load_byte);
+ if (test__start_subtest("narrow load half word"))
+ run_sk_reuseport_prog(skel->progs.narrow_load_half_word);
+
+ test_narrow_load__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_narrow_load.c b/tools/testing/selftests/bpf/progs/test_narrow_load.c
new file mode 100644
index 000000000000..57444720df16
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_narrow_load.c
@@ -0,0 +1,43 @@
+// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
+// Copyright (c) 2020 Cloudflare
+
+#include <stdint.h>
+
+#include <linux/bpf.h>
+#include <linux/in.h>
+
+#include <bpf/bpf_helpers.h>
+
+char _license[] SEC("license") = "Dual BSD/GPL";
+
+/* Check 1-byte load from 2-byte wide target field */
+SEC("sk_reuseport/narrow_load_byte")
+int narrow_load_byte(struct sk_reuseport_md *ctx)
+{
+ __u8 *byte;
+
+ byte = (__u8 *)&ctx->ip_protocol;
+ if (byte[0] != IPPROTO_UDP)
+ return SK_DROP;
+ if (byte[1] != 0)
+ return SK_DROP;
+ if (byte[2] != 0)
+ return SK_DROP;
+ if (byte[3] != 0)
+ return SK_DROP;
+ return SK_PASS;
+}
+
+/* Check 2-byte load from 2-byte wide target field */
+SEC("sk_reuseport/narrow_load_half_word")
+int narrow_load_half_word(struct sk_reuseport_md *ctx)
+{
+ __u16 *half;
+
+ half = (__u16 *)&ctx->ip_protocol;
+ if (half[0] != IPPROTO_UDP)
+ return SK_DROP;
+ if (half[1] != 0)
+ return SK_DROP;
+ return SK_PASS;
+}
--
2.25.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH bpf v2 1/2] bpf: Load zeros for narrow loads beyond target field
2020-07-23 9:59 ` [PATCH bpf v2 1/2] bpf: Load zeros for narrow loads beyond " Jakub Sitnicki
@ 2020-07-23 21:56 ` Yonghong Song
2020-07-27 14:53 ` Jakub Sitnicki
0 siblings, 1 reply; 6+ messages in thread
From: Yonghong Song @ 2020-07-23 21:56 UTC (permalink / raw)
To: Jakub Sitnicki, bpf
Cc: netdev, kernel-team, Alexei Starovoitov, Daniel Borkmann
On 7/23/20 2:59 AM, Jakub Sitnicki wrote:
> For narrow loads from context that are:
>
> 1) as big in size as the target field, and
> 2) at an offset beyond the target field,
>
> the verifier does not emit the shift-and-mask instruction sequence
> following the target field load instruction, as it happens for narrow loads
> smaller in size than the target field width.
>
> This has an unexpected effect of loading the same data, no matter what the
> offset. While, arguably, the expected behavior is to load zeros for offsets
> that beyond the target field.
>
> For instance, 2-byte load from a 4-byte context field, backed by a 2-byte
> target field at an offset of 2 bytes results in:
>
> $ cat progs/test_narrow_load.c
> [...]
> SEC("sk_reuseport/narrow_load_half_word")
> int narrow_load_half_word(struct sk_reuseport_md *ctx)
> {
> __u16 *half;
>
> half = (__u16 *)&ctx->ip_protocol;
> if (half[0] != IPPROTO_UDP)
> return SK_DROP;
> if (half[1] != 0)
> return SK_DROP;
> return SK_PASS;
> }
>
> $ llvm-objdump -S --no-show-raw-insn ...
> [...]
> 0000000000000000 narrow_load_half_word:
> ; {
> 0: w0 = 0
> ; if (half[0] != IPPROTO_UDP)
> 1: r2 = *(u16 *)(r1 + 24)
> 2: if w2 != 17 goto +4 <LBB1_3>
> ; if (half[1] != 0)
> 3: r1 = *(u16 *)(r1 + 26)
> 4: w0 = 1
> 5: if w1 == 0 goto +1 <LBB1_3>
> 6: w0 = 0
>
> 0000000000000038 LBB1_3:
> ; }
> 7: exit
>
> $ bpftool prog dump xlated ...
> int narrow_load_half_word(struct sk_reuseport_md * ctx):
> ; int narrow_load_half_word(struct sk_reuseport_md *ctx)
> 0: (b4) w0 = 0
> ; if (half[0] != IPPROTO_UDP)
> 1: (79) r2 = *(u64 *)(r1 +8)
> 2: (69) r2 = *(u16 *)(r2 +924)
> ; if (half[0] != IPPROTO_UDP)
> 3: (56) if w2 != 0x11 goto pc+5
> ; if (half[1] != 0)
> 4: (79) r1 = *(u64 *)(r1 +8)
> 5: (69) r1 = *(u16 *)(r1 +924)
> 6: (b4) w0 = 1
> ; if (half[1] != 0)
> 7: (16) if w1 == 0x0 goto pc+1
> 8: (b4) w0 = 0
> ; }
> 9: (95) exit
>
> In this case half[0] == half[1] == sk->sk_protocol, which is the target
> field for the ctx->ip_protocol.
>
> Fix it by emitting 'wX = 0' or 'rX = 0' instruction for all narrow loads
> from an offset that is beyond the target field.
>
> Going back to the example, with the fix in place, the upper half load from
> ctx->ip_protocol yields zero:
>
> int narrow_load_half_word(struct sk_reuseport_md * ctx):
> ; int narrow_load_half_word(struct sk_reuseport_md *ctx)
> 0: (b4) w0 = 0
> ; if (half[0] != IPPROTO_UDP)
> 1: (79) r2 = *(u64 *)(r1 +8)
> 2: (69) r2 = *(u16 *)(r2 +924)
> ; if (half[0] != IPPROTO_UDP)
> 3: (56) if w2 != 0x11 goto pc+4
> ; if (half[1] != 0)
> 4: (b4) w1 = 0
> 5: (b4) w0 = 1
> ; if (half[1] != 0)
> 6: (16) if w1 == 0x0 goto pc+1
> 7: (b4) w0 = 0
> ; }
> 8: (95) exit
>
> Fixes: f96da09473b5 ("bpf: simplify narrower ctx access")
> Suggested-by: Yonghong Song <yhs@fb.com>
> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
Thanks for the fix. The final code is much better now.
Ack with some nits below.
Acked-by: Yonghong Song <yhs@fb.com>
> ---
> kernel/bpf/verifier.c | 23 +++++++++++++++++++++--
> 1 file changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 94cead5a43e5..0a9dbcdd6341 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -9614,11 +9614,11 @@ static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env,
> */
> static int convert_ctx_accesses(struct bpf_verifier_env *env)
> {
> + u32 target_size, size_default, off, access_off;
> const struct bpf_verifier_ops *ops = env->ops;
> int i, cnt, size, ctx_field_size, delta = 0;
> const int insn_cnt = env->prog->len;
> struct bpf_insn insn_buf[16], *insn;
> - u32 target_size, size_default, off;
> struct bpf_prog *new_prog;
> enum bpf_access_type type;
> bool is_narrower_load;
> @@ -9760,7 +9760,26 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
> return -EINVAL;
> }
>
> - if (is_narrower_load && size < target_size) {
> + /* When context field is wider than the target field,
> + * narrow load from an offset beyond the target field
> + * can be reduced to loading zero because there is
> + * nothing to load from memory.
Maybe it is worthwhile to mention that the below codegen undos
what convert_ctx_access() just did.
> + */
> + access_off = off & (size_default - 1);
> + if (is_narrower_load && access_off >= target_size) {
> + cnt = 0;
> + if (ctx_field_size <= 4)
> + insn_buf[cnt++] = BPF_MOV32_IMM(insn->dst_reg, 0);
> + else
> + insn_buf[cnt++] = BPF_MOV64_IMM(insn->dst_reg, 0);
> + }
> + /* Narrow load from an offset within the target field,
> + * smaller in size than the target field, needs
> + * shifting and masking because convert_ctx_access
> + * always emits full-size target field load.
> + */
> + if (is_narrower_load && access_off < target_size &&
> + size < target_size) {
The code becomes a little bit complex here. I think it is worthwhile
to have a static function to do codegen if is_narrower_load is true.
The above two if statements are exclusive. It would be good to
make it clear with "else if ...", and things will become easier
if the narrower codegen is factored to a separate function.
> u8 shift = bpf_ctx_narrow_access_offset(
> off, size, size_default) * 8;
> if (ctx_field_size <= 4) {
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf v2 2/2] selftests/bpf: Add test for narrow loads from context at an offset
2020-07-23 9:59 ` [PATCH bpf v2 2/2] selftests/bpf: Add test for narrow loads from context at an offset Jakub Sitnicki
@ 2020-07-23 22:05 ` Yonghong Song
0 siblings, 0 replies; 6+ messages in thread
From: Yonghong Song @ 2020-07-23 22:05 UTC (permalink / raw)
To: Jakub Sitnicki, bpf
Cc: netdev, kernel-team, Alexei Starovoitov, Daniel Borkmann
On 7/23/20 2:59 AM, Jakub Sitnicki wrote:
> Check that narrow loads at various offsets from a context field backed by a
> target field that is smaller in size work as expected. That is target field
> value is loaded only when the offset is less than the target field size.
> While for offsets beyond the target field, the loaded value is zero.
>
> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
Ack with minor nit below.
Acked-by: Yonghong Song <yhs@fb.com>
> ---
> .../selftests/bpf/prog_tests/narrow_load.c | 84 +++++++++++++++++++
> .../selftests/bpf/progs/test_narrow_load.c | 43 ++++++++++
> 2 files changed, 127 insertions(+)
> create mode 100644 tools/testing/selftests/bpf/prog_tests/narrow_load.c
> create mode 100644 tools/testing/selftests/bpf/progs/test_narrow_load.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/narrow_load.c b/tools/testing/selftests/bpf/prog_tests/narrow_load.c
> new file mode 100644
> index 000000000000..6d79d722a66d
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/narrow_load.c
> @@ -0,0 +1,84 @@
> +// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
> +// Copyright (c) 2020 Cloudflare
> +
> +#include "test_progs.h"
> +#include "test_narrow_load.skel.h"
> +
> +static int duration;
> +
> +void run_sk_reuseport_prog(struct bpf_program *reuseport_prog)
static function?
> +{
> + static const struct timeval timeo = { .tv_sec = 3 };
> + struct sockaddr_in addr = {
> + .sin_family = AF_INET,
> + .sin_port = 0,
> + .sin_addr.s_addr = htonl(INADDR_LOOPBACK),
> + };
> + socklen_t len = sizeof(addr);
> + int err, fd, prog_fd;
> + const int one = 1;
> + char buf = 42;
> + ssize_t n;
> +
[...]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf v2 1/2] bpf: Load zeros for narrow loads beyond target field
2020-07-23 21:56 ` Yonghong Song
@ 2020-07-27 14:53 ` Jakub Sitnicki
0 siblings, 0 replies; 6+ messages in thread
From: Jakub Sitnicki @ 2020-07-27 14:53 UTC (permalink / raw)
To: Yonghong Song
Cc: bpf, netdev, kernel-team, Alexei Starovoitov, Daniel Borkmann
On Thu, Jul 23, 2020 at 11:56 PM CEST, Yonghong Song wrote:
> On 7/23/20 2:59 AM, Jakub Sitnicki wrote:
>> For narrow loads from context that are:
>>
>> 1) as big in size as the target field, and
>> 2) at an offset beyond the target field,
>>
>> the verifier does not emit the shift-and-mask instruction sequence
>> following the target field load instruction, as it happens for narrow loads
>> smaller in size than the target field width.
>>
>> This has an unexpected effect of loading the same data, no matter what the
>> offset. While, arguably, the expected behavior is to load zeros for offsets
>> that beyond the target field.
>>
>> For instance, 2-byte load from a 4-byte context field, backed by a 2-byte
>> target field at an offset of 2 bytes results in:
>>
>> $ cat progs/test_narrow_load.c
>> [...]
>> SEC("sk_reuseport/narrow_load_half_word")
>> int narrow_load_half_word(struct sk_reuseport_md *ctx)
>> {
>> __u16 *half;
>>
>> half = (__u16 *)&ctx->ip_protocol;
>> if (half[0] != IPPROTO_UDP)
>> return SK_DROP;
>> if (half[1] != 0)
>> return SK_DROP;
>> return SK_PASS;
>> }
>>
>> $ llvm-objdump -S --no-show-raw-insn ...
>> [...]
>> 0000000000000000 narrow_load_half_word:
>> ; {
>> 0: w0 = 0
>> ; if (half[0] != IPPROTO_UDP)
>> 1: r2 = *(u16 *)(r1 + 24)
>> 2: if w2 != 17 goto +4 <LBB1_3>
>> ; if (half[1] != 0)
>> 3: r1 = *(u16 *)(r1 + 26)
>> 4: w0 = 1
>> 5: if w1 == 0 goto +1 <LBB1_3>
>> 6: w0 = 0
>>
>> 0000000000000038 LBB1_3:
>> ; }
>> 7: exit
>>
>> $ bpftool prog dump xlated ...
>> int narrow_load_half_word(struct sk_reuseport_md * ctx):
>> ; int narrow_load_half_word(struct sk_reuseport_md *ctx)
>> 0: (b4) w0 = 0
>> ; if (half[0] != IPPROTO_UDP)
>> 1: (79) r2 = *(u64 *)(r1 +8)
>> 2: (69) r2 = *(u16 *)(r2 +924)
>> ; if (half[0] != IPPROTO_UDP)
>> 3: (56) if w2 != 0x11 goto pc+5
>> ; if (half[1] != 0)
>> 4: (79) r1 = *(u64 *)(r1 +8)
>> 5: (69) r1 = *(u16 *)(r1 +924)
>> 6: (b4) w0 = 1
>> ; if (half[1] != 0)
>> 7: (16) if w1 == 0x0 goto pc+1
>> 8: (b4) w0 = 0
>> ; }
>> 9: (95) exit
>>
>> In this case half[0] == half[1] == sk->sk_protocol, which is the target
>> field for the ctx->ip_protocol.
>>
>> Fix it by emitting 'wX = 0' or 'rX = 0' instruction for all narrow loads
>> from an offset that is beyond the target field.
>>
>> Going back to the example, with the fix in place, the upper half load from
>> ctx->ip_protocol yields zero:
>>
>> int narrow_load_half_word(struct sk_reuseport_md * ctx):
>> ; int narrow_load_half_word(struct sk_reuseport_md *ctx)
>> 0: (b4) w0 = 0
>> ; if (half[0] != IPPROTO_UDP)
>> 1: (79) r2 = *(u64 *)(r1 +8)
>> 2: (69) r2 = *(u16 *)(r2 +924)
>> ; if (half[0] != IPPROTO_UDP)
>> 3: (56) if w2 != 0x11 goto pc+4
>> ; if (half[1] != 0)
>> 4: (b4) w1 = 0
>> 5: (b4) w0 = 1
>> ; if (half[1] != 0)
>> 6: (16) if w1 == 0x0 goto pc+1
>> 7: (b4) w0 = 0
>> ; }
>> 8: (95) exit
>>
>> Fixes: f96da09473b5 ("bpf: simplify narrower ctx access")
>> Suggested-by: Yonghong Song <yhs@fb.com>
>> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
>
> Thanks for the fix. The final code is much better now.
> Ack with some nits below.
>
> Acked-by: Yonghong Song <yhs@fb.com>
>
>> ---
>> kernel/bpf/verifier.c | 23 +++++++++++++++++++++--
>> 1 file changed, 21 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 94cead5a43e5..0a9dbcdd6341 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -9614,11 +9614,11 @@ static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env,
>> */
>> static int convert_ctx_accesses(struct bpf_verifier_env *env)
>> {
>> + u32 target_size, size_default, off, access_off;
>> const struct bpf_verifier_ops *ops = env->ops;
>> int i, cnt, size, ctx_field_size, delta = 0;
>> const int insn_cnt = env->prog->len;
>> struct bpf_insn insn_buf[16], *insn;
>> - u32 target_size, size_default, off;
>> struct bpf_prog *new_prog;
>> enum bpf_access_type type;
>> bool is_narrower_load;
>> @@ -9760,7 +9760,26 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
>> return -EINVAL;
>> }
>> - if (is_narrower_load && size < target_size) {
>> + /* When context field is wider than the target field,
>> + * narrow load from an offset beyond the target field
>> + * can be reduced to loading zero because there is
>> + * nothing to load from memory.
>
> Maybe it is worthwhile to mention that the below codegen undos
> what convert_ctx_access() just did.
>
>> + */
>> + access_off = off & (size_default - 1);
>> + if (is_narrower_load && access_off >= target_size) {
>> + cnt = 0;
>> + if (ctx_field_size <= 4)
>> + insn_buf[cnt++] = BPF_MOV32_IMM(insn->dst_reg, 0);
>> + else
>> + insn_buf[cnt++] = BPF_MOV64_IMM(insn->dst_reg, 0);
>> + }
>> + /* Narrow load from an offset within the target field,
>> + * smaller in size than the target field, needs
>> + * shifting and masking because convert_ctx_access
>> + * always emits full-size target field load.
>> + */
>> + if (is_narrower_load && access_off < target_size &&
>> + size < target_size) {
>
> The code becomes a little bit complex here. I think it is worthwhile
> to have a static function to do codegen if is_narrower_load is true.
>
> The above two if statements are exclusive. It would be good to
> make it clear with "else if ...", and things will become easier
> if the narrower codegen is factored to a separate function.
Thanks for comments. I will circle back to it in a bit.
>
>> u8 shift = bpf_ctx_narrow_access_offset(
>> off, size, size_default) * 8;
>> if (ctx_field_size <= 4) {
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-07-27 14:53 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-23 9:59 [PATCH bpf v2 0/2] Fix narrow loads from an offset outside of target field Jakub Sitnicki
2020-07-23 9:59 ` [PATCH bpf v2 1/2] bpf: Load zeros for narrow loads beyond " Jakub Sitnicki
2020-07-23 21:56 ` Yonghong Song
2020-07-27 14:53 ` Jakub Sitnicki
2020-07-23 9:59 ` [PATCH bpf v2 2/2] selftests/bpf: Add test for narrow loads from context at an offset Jakub Sitnicki
2020-07-23 22:05 ` Yonghong Song
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.