| Message ID | 20220227202757.519015-4-jakub@cloudflare.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | Fixes for sock_fields selftests | expand |
On Sun, Feb 27, 2022 at 09:27:57PM +0100, Jakub Sitnicki wrote: > The check for 4-byte load from dst_port offset into bpf_sock is failing on > big-endian architecture - s390. The bpf access converter rewrites the > 4-byte load to a 2-byte load from sock_common at skc_dport offset, as shown > below. > > * s390 / llvm-objdump -S --no-show-raw-insn > > 00000000000002a0 <sk_dst_port__load_word>: > 84: r1 = *(u32 *)(r1 + 48) > 85: w0 = 1 > 86: if w1 == 51966 goto +1 <LBB5_2> > 87: w0 = 0 > 00000000000002c0 <LBB5_2>: > 88: exit > > * s390 / bpftool prog dump xlated > > _Bool sk_dst_port__load_word(struct bpf_sock * sk): > 35: (69) r1 = *(u16 *)(r1 +12) > 36: (bc) w1 = w1 > 37: (b4) w0 = 1 > 38: (16) if w1 == 0xcafe goto pc+1 > 39: (b4) w0 = 0 > 40: (95) exit > > * s390 / llvm-objdump -S --no-show-raw-insn x86_64 > > 00000000000002a0 <sk_dst_port__load_word>: > 84: r1 = *(u32 *)(r1 + 48) > 85: w0 = 1 > 86: if w1 == 65226 goto +1 <LBB5_2> > 87: w0 = 0 > 00000000000002c0 <LBB5_2>: > 88: exit > > * x86_64 / bpftool prog dump xlated > > _Bool sk_dst_port__load_word(struct bpf_sock * sk): > 33: (69) r1 = *(u16 *)(r1 +12) > 34: (b4) w0 = 1 > 35: (16) if w1 == 0xfeca goto pc+1 > 36: (b4) w0 = 0 > 37: (95) exit > > This leads to surprisings results. On big-endian platforms, the loaded > value is as expected. The user observes no difference between a 4-byte load > and 2-byte load. However, on little-endian platforms, the access conversion > is not what would be expected, that is the result is left shifted after > converting the value to the native byte order. > > That said, 4-byte loads in BPF from sk->dst_port are not a use case we > expect to see, now that the dst_port field is clearly declared as a u16. > > Account for the quirky behavior of the access converter in the test case, > so that the check passes on both endian variants. > > Fixes: 8f50f16ff39d ("selftests/bpf: Extend verifier and bpf_sock tests for dst_port loads") > Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com> > --- > .../selftests/bpf/progs/test_sock_fields.c | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/bpf/progs/test_sock_fields.c b/tools/testing/selftests/bpf/progs/test_sock_fields.c > index 186fed1deaab..3dddc173070c 100644 > --- a/tools/testing/selftests/bpf/progs/test_sock_fields.c > +++ b/tools/testing/selftests/bpf/progs/test_sock_fields.c > @@ -256,10 +256,23 @@ int ingress_read_sock_fields(struct __sk_buff *skb) > return CG_OK; > } > > +/* > + * NOTE: 4-byte load from bpf_sock at dst_port offset is quirky. The > + * result is left shifted on little-endian architectures because the > + * access is converted to a 2-byte load. The quirky behavior is kept > + * for backward compatibility. > + */ > static __noinline bool sk_dst_port__load_word(struct bpf_sock *sk) > { > +#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ > + const __u8 SHIFT = 16; > +#elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ > + const __u8 SHIFT = 0; > +#else > +#error "Unrecognized __BYTE_ORDER__" > +#endif > __u32 *word = (__u32 *)&sk->dst_port; > - return word[0] == bpf_htonl(0xcafe0000); > + return word[0] == bpf_htonl(0xcafe << SHIFT); I believe it should be fine. It is the behavior even before commit 4421a582718a ("bpf: Make dst_port field in struct bpf_sock 16-bit wide") ? btw, is it the same as testing "return word[0] == bpf_hton's'(0xcafe);" > } > > static __noinline bool sk_dst_port__load_half(struct bpf_sock *sk) > -- > 2.35.1 >
On Mon, Feb 28, 2022 at 10:22 PM -08, Martin KaFai Lau wrote: > On Sun, Feb 27, 2022 at 09:27:57PM +0100, Jakub Sitnicki wrote: [...] >> --- a/tools/testing/selftests/bpf/progs/test_sock_fields.c >> +++ b/tools/testing/selftests/bpf/progs/test_sock_fields.c >> @@ -256,10 +256,23 @@ int ingress_read_sock_fields(struct __sk_buff *skb) >> return CG_OK; >> } >> >> +/* >> + * NOTE: 4-byte load from bpf_sock at dst_port offset is quirky. The >> + * result is left shifted on little-endian architectures because the >> + * access is converted to a 2-byte load. The quirky behavior is kept >> + * for backward compatibility. >> + */ >> static __noinline bool sk_dst_port__load_word(struct bpf_sock *sk) >> { >> +#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ >> + const __u8 SHIFT = 16; >> +#elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ >> + const __u8 SHIFT = 0; >> +#else >> +#error "Unrecognized __BYTE_ORDER__" >> +#endif >> __u32 *word = (__u32 *)&sk->dst_port; >> - return word[0] == bpf_htonl(0xcafe0000); >> + return word[0] == bpf_htonl(0xcafe << SHIFT); > I believe it should be fine. It is the behavior even before > commit 4421a582718a ("bpf: Make dst_port field in struct bpf_sock 16-bit wide") ? Yes, exactly. AFAICT there was no change in behavior in commit 4421a582718a, that is: 1. 4-byte load behaves like it did, in its quirky way, 2. 2-byte load at offset dst_port works the same 3. 2-byte load at offset dst_port+2 continues to be rejected. > btw, is it the same as testing "return word[0] == bpf_hton's'(0xcafe);" Right. Clever observation. I got the impression from the original problem report [1] that the users were failing when trying to do: bpf_htonl(sk->dst_port) == 0xcafe Hence I the bpf_htonl() use here. But perhaps it's better to promote this cleaner pattern in tests. I will respin it once we hash out the details of what the access should look like on big-endian with Ilya. [1] https://lore.kernel.org/bpf/20220113070245.791577-1-imagedong@tencent.com/ [...]
diff --git a/tools/testing/selftests/bpf/progs/test_sock_fields.c b/tools/testing/selftests/bpf/progs/test_sock_fields.c index 186fed1deaab..3dddc173070c 100644 --- a/tools/testing/selftests/bpf/progs/test_sock_fields.c +++ b/tools/testing/selftests/bpf/progs/test_sock_fields.c @@ -256,10 +256,23 @@ int ingress_read_sock_fields(struct __sk_buff *skb) return CG_OK; } +/* + * NOTE: 4-byte load from bpf_sock at dst_port offset is quirky. The + * result is left shifted on little-endian architectures because the + * access is converted to a 2-byte load. The quirky behavior is kept + * for backward compatibility. + */ static __noinline bool sk_dst_port__load_word(struct bpf_sock *sk) { +#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ + const __u8 SHIFT = 16; +#elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ + const __u8 SHIFT = 0; +#else +#error "Unrecognized __BYTE_ORDER__" +#endif __u32 *word = (__u32 *)&sk->dst_port; - return word[0] == bpf_htonl(0xcafe0000); + return word[0] == bpf_htonl(0xcafe << SHIFT); } static __noinline bool sk_dst_port__load_half(struct bpf_sock *sk)
The check for 4-byte load from dst_port offset into bpf_sock is failing on big-endian architecture - s390. The bpf access converter rewrites the 4-byte load to a 2-byte load from sock_common at skc_dport offset, as shown below. * s390 / llvm-objdump -S --no-show-raw-insn 00000000000002a0 <sk_dst_port__load_word>: 84: r1 = *(u32 *)(r1 + 48) 85: w0 = 1 86: if w1 == 51966 goto +1 <LBB5_2> 87: w0 = 0 00000000000002c0 <LBB5_2>: 88: exit * s390 / bpftool prog dump xlated _Bool sk_dst_port__load_word(struct bpf_sock * sk): 35: (69) r1 = *(u16 *)(r1 +12) 36: (bc) w1 = w1 37: (b4) w0 = 1 38: (16) if w1 == 0xcafe goto pc+1 39: (b4) w0 = 0 40: (95) exit * s390 / llvm-objdump -S --no-show-raw-insn 00000000000002a0 <sk_dst_port__load_word>: 84: r1 = *(u32 *)(r1 + 48) 85: w0 = 1 86: if w1 == 65226 goto +1 <LBB5_2> 87: w0 = 0 00000000000002c0 <LBB5_2>: 88: exit * x86_64 / bpftool prog dump xlated _Bool sk_dst_port__load_word(struct bpf_sock * sk): 33: (69) r1 = *(u16 *)(r1 +12) 34: (b4) w0 = 1 35: (16) if w1 == 0xfeca goto pc+1 36: (b4) w0 = 0 37: (95) exit This leads to surprisings results. On big-endian platforms, the loaded value is as expected. The user observes no difference between a 4-byte load and 2-byte load. However, on little-endian platforms, the access conversion is not what would be expected, that is the result is left shifted after converting the value to the native byte order. That said, 4-byte loads in BPF from sk->dst_port are not a use case we expect to see, now that the dst_port field is clearly declared as a u16. Account for the quirky behavior of the access converter in the test case, so that the check passes on both endian variants. Fixes: 8f50f16ff39d ("selftests/bpf: Extend verifier and bpf_sock tests for dst_port loads") Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com> --- .../selftests/bpf/progs/test_sock_fields.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)