RE: [PATCH v4 bpf-next 13/13] bpf: selftest: Add test_btf_skc_cls_ingress

From: John Fastabend <john.fastabend@gmail.com>
To: Martin KaFai Lau <kafai@fb.com>, bpf@vger.kernel.org
Cc: Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	kernel-team@fb.com, Lorenz Bauer <lmb@cloudflare.com>,
	netdev@vger.kernel.org
Subject: RE: [PATCH v4 bpf-next 13/13] bpf: selftest: Add test_btf_skc_cls_ingress
Date: Fri, 25 Sep 2020 09:24:02 -0700	[thread overview]
Message-ID: <5f6e19a2621d8_8ae15208fd@john-XPS-13-9370.notmuch> (raw)
In-Reply-To: <20200925000458.3859627-1-kafai@fb.com>

Martin KaFai Lau wrote:
> This patch attaches a classifier prog to the ingress filter.
> It exercises the following helpers with different socket pointer
> types in different logical branches:
> 1. bpf_sk_release()
> 2. bpf_sk_assign()
> 3. bpf_skc_to_tcp_request_sock(), bpf_skc_to_tcp_sock()
> 4. bpf_tcp_gen_syncookie, bpf_tcp_check_syncookie
> 
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> ---
>  tools/testing/selftests/bpf/bpf_tcp_helpers.h |   5 +
>  .../bpf/prog_tests/btf_skc_cls_ingress.c      | 234 ++++++++++++++++++
>  .../bpf/progs/test_btf_skc_cls_ingress.c      | 174 +++++++++++++
>  3 files changed, 413 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/btf_skc_cls_ingress.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_btf_skc_cls_ingress.c
> 

Hi Martin,

One piece I'm missing is how does this handle null pointer dereferences
from network side when reading BTF objects? I've not got through all the
code yet so maybe I'm just not there yet.

For example,

> diff --git a/tools/testing/selftests/bpf/bpf_tcp_helpers.h b/tools/testing/selftests/bpf/bpf_tcp_helpers.h
> index a0e8b3758bd7..2915664c335d 100644
> --- a/tools/testing/selftests/bpf/bpf_tcp_helpers.h
> +++ b/tools/testing/selftests/bpf/bpf_tcp_helpers.h
> @@ -16,6 +16,7 @@ BPF_PROG(name, args)
>  
>  struct sock_common {
>  	unsigned char	skc_state;
> +	__u16		skc_num;
>  } __attribute__((preserve_access_index));
>  
>  enum sk_pacing {
> @@ -45,6 +46,10 @@ struct inet_connection_sock {
>  	__u64			  icsk_ca_priv[104 / sizeof(__u64)];
>  } __attribute__((preserve_access_index));
>  
> +struct request_sock {
> +	struct sock_common		__req_common;
> +} __attribute__((preserve_access_index));
> +
>  struct tcp_sock {
>  	struct inet_connection_sock	inet_conn;

add some pointer from tcp_sock which is likely not set so should be NULL,

        struct tcp_fastopen_request *fastopen_req;

[...]

> +	if (bpf_skc->state == BPF_TCP_NEW_SYN_RECV) {
> +		struct request_sock *req_sk;
> +
> +		req_sk = (struct request_sock *)bpf_skc_to_tcp_request_sock(bpf_skc);
> +		if (!req_sk) {
> +			LOG();
> +			goto release;
> +		}
> +
> +		if (bpf_sk_assign(skb, req_sk, 0)) {
> +			LOG();
> +			goto release;
> +		}
> +
> +		req_sk_sport = req_sk->__req_common.skc_num;
> +
> +		bpf_sk_release(req_sk);
> +		return TC_ACT_OK;
> +	} else if (bpf_skc->state == BPF_TCP_LISTEN) {
> +		struct tcp_sock *tp;
> +
> +		tp = bpf_skc_to_tcp_sock(bpf_skc);
> +		if (!tp) {
> +			LOG();
> +			goto release;
> +		}
> +
> +		if (bpf_sk_assign(skb, tp, 0)) {
> +			LOG();
> +			goto release;
> +		}
> +

Then use it here without a null check in the BPF program,

                fastopen = tp->fastopen_req;
		if (fastopen->size > 0x1234)
                      (do something)

> +		listen_tp_sport = tp->inet_conn.icsk_inet.sk.__sk_common.skc_num;
> +
> +		test_syncookie_helper(ip6h, th, tp, skb);
> +		bpf_sk_release(tp);
> +		return TC_ACT_OK;
> +	}

My quick check shows this didn't actually fault and the xlated
looks like it did the read and dereference. Must be missing
something? We shouldn't have fault_handler set for cls_ingress.

Perhaps a comment in the cover letter about this would be
helpful? Or if I'm just being dense this morning let me know
as well. ;)

> +
> +	if (bpf_sk_assign(skb, bpf_skc, 0))
> +		LOG();
> +
> +release:
> +	bpf_sk_release(bpf_skc);
> +	return TC_ACT_OK;
> +}