[PATCH bpf-next v2 0/3] Fixes for sock_fields selftests

[PATCH bpf-next v2 0/3] Fixes for sock_fields selftests

[Jakub Sitnicki]

This is a respin of a fix for error reporting in sock_fields test.

Fixing the error reporting has uncovered bugs in the recently added test for
sk->dst_port access. Series now includes patches that address the broken test.

The series has been tested on x86_64 and s390.

v1 -> v2:
- Limit read_sk_dst_port only to client traffic (patch 2)
- Make read_sk_dst_port pass on litte- and big-endian (patch 3)

v1: https://lore.kernel.org/bpf/20220225184130.483208-1-jakub@cloudflare.com/

Jakub Sitnicki (3):
  selftests/bpf: Fix error reporting from sock_fields programs
  selftests/bpf: Check dst_port only on the client socket
  selftests/bpf: Fix test for 4-byte load from dst_port on big-endian

 .../selftests/bpf/progs/test_sock_fields.c    | 30 ++++++++++++++++---
 1 file changed, 26 insertions(+), 4 deletions(-)

-- 
2.35.1

[PATCH bpf-next v2 1/3] selftests/bpf: Fix error reporting from sock_fields programs

[Jakub Sitnicki]

The helper macro that records an error in BPF programs that exercise sock
fields access has been inadvertently broken by adaptation work that happened
in commit b18c1f0aa477 ("bpf: selftest: Adapt sock_fields test to use skel
and global variables").

BPF_NOEXIST flag cannot be used to update BPF_MAP_TYPE_ARRAY. The operation
always fails with -EEXIST, which in turn means the error never gets
recorded, and the checks for errors always pass.

Revert the change in update flags.

Fixes: b18c1f0aa477 ("bpf: selftest: Adapt sock_fields test to use skel and global variables")
Acked-by: Martin KaFai Lau <kafai@fb.com>
Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 tools/testing/selftests/bpf/progs/test_sock_fields.c | 2 +-
 1 file changed, 1 insertion(+), 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 246f1f001813..3e2e3ee51cc9 100644
--- a/tools/testing/selftests/bpf/progs/test_sock_fields.c
+++ b/tools/testing/selftests/bpf/progs/test_sock_fields.c
@@ -114,7 +114,7 @@ static void tpcpy(struct bpf_tcp_sock *dst,
 
 #define RET_LOG() ({						\
 	linum = __LINE__;					\
-	bpf_map_update_elem(&linum_map, &linum_idx, &linum, BPF_NOEXIST);	\
+	bpf_map_update_elem(&linum_map, &linum_idx, &linum, BPF_ANY);	\
 	return CG_OK;						\
 })
 
-- 
2.35.1

[PATCH bpf-next v2 2/3] selftests/bpf: Check dst_port only on the client socket

[Jakub Sitnicki]

cgroup_skb/egress programs which sock_fields test installs process packets
flying in both directions, from the client to the server, and in reverse
direction.

Recently added dst_port check relies on the fact that destination
port (remote peer port) of the socket which sends the packet is known ahead
of time. This holds true only for the client socket, which connects to the
known server port.

Filter out any traffic that is not bound to be egressing from the client
socket in the test program for reading the dst_port.

Fixes: 8f50f16ff39d ("selftests/bpf: Extend verifier and bpf_sock tests for dst_port loads")
Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 .../testing/selftests/bpf/progs/test_sock_fields.c  | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/test_sock_fields.c b/tools/testing/selftests/bpf/progs/test_sock_fields.c
index 3e2e3ee51cc9..186fed1deaab 100644
--- a/tools/testing/selftests/bpf/progs/test_sock_fields.c
+++ b/tools/testing/selftests/bpf/progs/test_sock_fields.c
@@ -42,6 +42,11 @@ struct {
 	__type(value, struct bpf_spinlock_cnt);
 } sk_pkt_out_cnt10 SEC(".maps");
 
+enum {
+	TCP_SYN_SENT = 2,
+	TCP_LISTEN = 10,
+};
+
 struct bpf_tcp_sock listen_tp = {};
 struct sockaddr_in6 srv_sa6 = {};
 struct bpf_tcp_sock cli_tp = {};
@@ -138,7 +143,7 @@ int egress_read_sock_fields(struct __sk_buff *skb)
 	 * TCP_LISTEN (10) socket will be copied at the ingress side.
 	 */
 	if (sk->family != AF_INET6 || !is_loopback6(sk->src_ip6) ||
-	    sk->state == 10)
+	    sk->state == TCP_LISTEN)
 		return CG_OK;
 
 	if (sk->src_port == bpf_ntohs(srv_sa6.sin6_port)) {
@@ -233,7 +238,7 @@ int ingress_read_sock_fields(struct __sk_buff *skb)
 		return CG_OK;
 
 	/* Only interested in TCP_LISTEN */
-	if (sk->state != 10)
+	if (sk->state != TCP_LISTEN)
 		return CG_OK;
 
 	/* It must be a fullsock for cgroup_skb/ingress prog */
@@ -281,6 +286,10 @@ int read_sk_dst_port(struct __sk_buff *skb)
 	if (!sk)
 		RET_LOG();
 
+	/* Ignore everything but the SYN from the client socket */
+	if (sk->state != TCP_SYN_SENT)
+		return CG_OK;
+
 	if (!sk_dst_port__load_word(sk))
 		RET_LOG();
 	if (!sk_dst_port__load_half(sk))
-- 
2.35.1

[PATCH bpf-next v2 3/3] selftests/bpf: Fix test for 4-byte load from dst_port on big-endian

[Jakub Sitnicki]

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(-)

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)
-- 
2.35.1

Re: [PATCH bpf-next v2 3/3] selftests/bpf: Fix test for 4-byte load from dst_port on big-endian

[Martin KaFai Lau]

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
> 

Re: [PATCH bpf-next v2 2/3] selftests/bpf: Check dst_port only on the client socket

[Martin KaFai Lau]

On Sun, Feb 27, 2022 at 09:27:56PM +0100, Jakub Sitnicki wrote:
> cgroup_skb/egress programs which sock_fields test installs process packets
> flying in both directions, from the client to the server, and in reverse
> direction.
> 
> Recently added dst_port check relies on the fact that destination
> port (remote peer port) of the socket which sends the packet is known ahead
> of time. This holds true only for the client socket, which connects to the
> known server port.
> 
> Filter out any traffic that is not bound to be egressing from the client
> socket in the test program for reading the dst_port.
> 
> Fixes: 8f50f16ff39d ("selftests/bpf: Extend verifier and bpf_sock tests for dst_port loads")
> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
> ---
>  .../testing/selftests/bpf/progs/test_sock_fields.c  | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/progs/test_sock_fields.c b/tools/testing/selftests/bpf/progs/test_sock_fields.c
> index 3e2e3ee51cc9..186fed1deaab 100644
> --- a/tools/testing/selftests/bpf/progs/test_sock_fields.c
> +++ b/tools/testing/selftests/bpf/progs/test_sock_fields.c
> @@ -42,6 +42,11 @@ struct {
>  	__type(value, struct bpf_spinlock_cnt);
>  } sk_pkt_out_cnt10 SEC(".maps");
>  
> +enum {
> +	TCP_SYN_SENT = 2,
> +	TCP_LISTEN = 10,
Thanks for the clean up.

A nit. directly use BPF_TCP_SYN_SENT and BPF_TCP_LISTEN.

Others lgtm.

Acked-by: Martin KaFai Lau <kafai@fb.com>

Re: [PATCH bpf-next v2 3/3] selftests/bpf: Fix test for 4-byte load from dst_port on big-endian

[Jakub Sitnicki]

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/

[...]

Re: [PATCH bpf-next v2 2/3] selftests/bpf: Check dst_port only on the client socket

[Jakub Sitnicki]

On Mon, Feb 28, 2022 at 10:25 PM -08, Martin KaFai Lau wrote:
> On Sun, Feb 27, 2022 at 09:27:56PM +0100, Jakub Sitnicki wrote:
>> cgroup_skb/egress programs which sock_fields test installs process packets
>> flying in both directions, from the client to the server, and in reverse
>> direction.
>> 
>> Recently added dst_port check relies on the fact that destination
>> port (remote peer port) of the socket which sends the packet is known ahead
>> of time. This holds true only for the client socket, which connects to the
>> known server port.
>> 
>> Filter out any traffic that is not bound to be egressing from the client
>> socket in the test program for reading the dst_port.
>> 
>> Fixes: 8f50f16ff39d ("selftests/bpf: Extend verifier and bpf_sock tests for dst_port loads")
>> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
>> ---
>>  .../testing/selftests/bpf/progs/test_sock_fields.c  | 13 +++++++++++--
>>  1 file changed, 11 insertions(+), 2 deletions(-)
>> 
>> diff --git a/tools/testing/selftests/bpf/progs/test_sock_fields.c b/tools/testing/selftests/bpf/progs/test_sock_fields.c
>> index 3e2e3ee51cc9..186fed1deaab 100644
>> --- a/tools/testing/selftests/bpf/progs/test_sock_fields.c
>> +++ b/tools/testing/selftests/bpf/progs/test_sock_fields.c
>> @@ -42,6 +42,11 @@ struct {
>>  	__type(value, struct bpf_spinlock_cnt);
>>  } sk_pkt_out_cnt10 SEC(".maps");
>>  
>> +enum {
>> +	TCP_SYN_SENT = 2,
>> +	TCP_LISTEN = 10,
> Thanks for the clean up.
>
> A nit. directly use BPF_TCP_SYN_SENT and BPF_TCP_LISTEN.

Thanks. Completely forgot about those.